Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write an ADR for use of a user's JWT now that EdxApiRestClient has been removed #277

Open
robrap opened this issue Jun 15, 2023 · 5 comments
Labels
help wanted Ready to be picked up by anyone in the community

Comments

@robrap
Copy link
Contributor

robrap commented Jun 15, 2023

During the DEPR of EdxApiRestClient, some cases were replaced with plain requests object code, rather than using the updated OAuth client, using the user's JWT for service-to-service calls.

TODO: Find an example link of this change from the DEPR work.

The original intention of the new client was intentionally not to add this functionality to the new client, because it was thought that the client credentials token should be used instead of the user's JWT.

  • We should have an ADR that clarifies when and if using the user's JWT is appropriate.
    • We might potentially enhance the existing client if we wish to allow for other ways of using it.
  • Note: The new client also provides shared observability code, but is lost when using the requests code directly without a client.
  • The Authentication OEP should also link to this ADR.
@robrap robrap changed the title Document and implement alternatives for EdxApiRestClient Document and possibly implement alternatives for EdxApiRestClient Jun 15, 2023
@robrap robrap changed the title Document and possibly implement alternatives for EdxApiRestClient Write an ADR for use of a user's JWT now that EdxApiRestClient has been removed Jun 15, 2023
@robrap
Copy link
Contributor Author

robrap commented Sep 13, 2023

Other thoughts to consider: How might different solutions affect traceability and rate limiting.

Note: If we enhance the client with additional features around retries, backoffs, circuit breakers, etc., we'll have even more reasons for people to use our client over requests directly.

@robrap
Copy link
Contributor Author

robrap commented Sep 13, 2023

Note that once this ADR exists, there should be a follow-up ticket for getting the platform aligned with the decision.

@robrap robrap added the help wanted Ready to be picked up by anyone in the community label Sep 13, 2023
@robrap
Copy link
Contributor Author

robrap commented Dec 4, 2023

Additional notes:

@robrap
Copy link
Contributor Author

robrap commented Dec 4, 2023

If we are unable to make a decision, we could at a minimum document the de facto decision that was made for all the legacy code in a how-to and/or ADR.

@robrap
Copy link
Contributor Author

robrap commented Dec 12, 2023

Note that in general we should be avoided doing synchronous calls to other services within a user's request. This is one reason why we may want to avoid reuse of the user's existing JWT in general. However, it may be ok to do so for the exceptional cases where a synchronous call is required. Note that there may be legacy calls which could be refactored, and these should possibly be documented as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Ready to be picked up by anyone in the community
Projects
None yet
Development

No branches or pull requests

1 participant