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

Update OEP-42 Authentication proposing consolidation of authentication code #541

Open
robrap opened this issue Nov 17, 2023 · 7 comments
Open
Assignees
Labels
waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Comments

@robrap
Copy link
Contributor

robrap commented Nov 17, 2023

Our authentication code is mostly consolidated under edx-drf-extensions and edx-platform's oauth_dispatch (as examples).

Some services use custom authentication code, such as edx-platform XBlocks code or notes custom JWT decoder. These special cases are usually discovered through breaking changes in Production.

This ticket is about codifying our wish to both consolidate authentication code in fewer locations, and to not introduce custom authentication code unless absolutely necessary. Even if the custom code is used in a single service, it is probably better to add it to the consolidated code base, because it almost certainly interacts with the rest of the authentication code.

@robrap robrap self-assigned this Nov 17, 2023
@robrap
Copy link
Contributor Author

robrap commented Nov 17, 2023

Note that any exceptional custom authentication code examples should also be documented in the OEP and/or ticketed to be moved or replaced. This includes at least the two examples for XBlock handler and notes JWT decoder.

@robrap
Copy link
Contributor Author

robrap commented Nov 17, 2023

Notes had created custom JWT authentication long ago that should be replaced.

See private ticket https://2u-internal.atlassian.net/browse/TNLARCHIVE-4501 with description:

Authorize user requests which have valid access tokens
https://github.com/edx/edx-platform/pull/6125
openedx/edx-notes-api#2

@sarina
Copy link
Contributor

sarina commented May 17, 2024

Is this ever going to happen? We should be moving away from writing "aspirational" OEPs that don't actually reflect either our current architecture or a new architecture that a team is actively working on

@sarina sarina added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 17, 2024
@robrap
Copy link
Contributor Author

robrap commented May 17, 2024

To start, this should just be an ADR (or OEP update) stating what people should do or not do. That is not aspirational.

Follow-up work to either attempt to prevent this, and/or fix any past exceptions to this rule may or may not be done, and is potentially aspirational, but is a separate issue.

@sarina
Copy link
Contributor

sarina commented May 17, 2024

That makes sense. How would you feel about a bit of a middle ground around adding a "Best Practices" section that calls out what our ideal case is, and possibly points at places we do it right (and wrong!)

I know this isn't a thing you can do today, but of all of your open issues in this repo, this one feels like the most needing of your attention - you're likely the best person in the community to address it. Would it be possible to get this on your radar, maybe July-September timeframe? I'd be happy to help copyedit / do the mechanics of the PR / be arbiter.

@robrap
Copy link
Contributor Author

robrap commented May 17, 2024

  1. Let's try for that.
  2. That said, if Axim Improvements ever invests in Auth (which I thought was a possibility), maybe someone can pick this up?

@sarina
Copy link
Contributor

sarina commented May 17, 2024

Auth isn't on our immediate radar but I'll add @feanil in case that changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

No branches or pull requests

2 participants