[FC-0118] docs: add ADR for standardizing authentication patterns#38301
Conversation
|
related doc worth reading: https://docs.openedx.org/projects/openedx-proposals/en/latest/best-practices/oep-0042-bp-authentication.html#consequences, we can make changes to the ADR based on the attached doc if needed. |
Faraz32123
changed the title
Faraz32123
force-pushed
the
docs/ADR-standardize_authentication_patterns_and_security_scheme_usage
branch
from
f5cb0c1 to
a6a25d3
Compare
| Decision | ||
| ======== | ||
|
|
||
| 1. **OAuth2 via Django OAuth Toolkit (DOT) MUST be the standard authentication |
There was a problem hiding this comment.
How is this different from JWT authentication? My understanding was that JWTs were what the OAUTH2 mechanism produced for use in the authorization header?
There was a problem hiding this comment.
You are right! I think the wording needs to be changed a bit because showing Oauth2 & JWT as separate can cause confusion. In the codebase, we have two JWT issuance paths:
create_jwt_token_dict(): wraps a DOT OAuth2 access token into a JWT (DB-backed, revocable, for external clients)create_jwt_for_user(): issues a JWT directly with no OAuth2 flow and no DB row (non-revocable, for internal service calls), it is used for internal service-to-service communication.
Both are validated by the sameJwtAuthenticationclass.
The real distinction is between JwtAuthentication (supported) and BearerAuthentication (deprecated per OEP-0042) and the use of multiple authentication methods in single API endpoint. I am updating the ADR to reflect this and we will no longer treat "OAuth2" and "JWT" as separate mechanisms.
| authentication mechanism for all API access** (external and internal), per `OEP-0042`_ | ||
| 2. **``BearerAuthentication`` and ``BearerAuthenticationAllowInactiveUser`` are | ||
| deprecated and MUST NOT be used in new code** | ||
| 3. **Session authentication MUST be used only for browser-based UI interactions** |
There was a problem hiding this comment.
I'm not sure if this distinction is super important. What is the bad thing that happens if your API endpoints support session auth also? Right now DRF in openedx-platform supports both by default: https://github.com/openedx/openedx-platform/blob/master/openedx/envs/common.py#L822-L825 and I don't see a problem with that. I think it's more valuable that all endpoints support any valid auth scheme than having API calls not support the browser.
There was a problem hiding this comment.
@feanil I think you have valid point here that we should support any valid auth scheme instead of categorizing them on the basis of their usage.
I am adding a new commit that addresses above point with some additional remains(includes removal of JWT issuers & use of asymmetric keys) of OEP-0042, But it had some consequences(Link) that you might want to look in. That way we can decide the scope of this ADR. And we can remove the part that is not needed for this ADR accordingly.
| 1. **JWT authentication via** ``JwtAuthentication`` **MUST be the standard | ||
| authentication mechanism for all API access** (external and internal), per `OEP-0042`_ | ||
| 2. **Session authentication MAY be supported alongside** ``JwtAuthentication`` | ||
| on any endpoint — this is the platform default and has no security implications |
There was a problem hiding this comment.
i think saying there are zero implications is inaccurate. maybe soften it to something like "this is the platform default and is acceptable for most endpoints", since session auth carries CSRF concerns, cookie-handling rules, and a different threat model from JWT
There was a problem hiding this comment.
let me change the wording a bit.
|
|
||
| class BrowserUIViewSet(viewsets.ViewSet): | ||
| """Browser UI API - Session authentication only.""" | ||
| authentication_classes = [SessionAuthenticationAllowInactiveUser] |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we can miss it, decision no.1 says, JwtAuthentication MUST be the standard authentication mechanism for all API access i.e. for external and internal API types. May be I can change decision no.1 to all API(external and internal) access to resolve the confusion.
For browser based APIs, session authentication is kind of must with an option of JWTAuthentication to support any valid auth scheme.
There was a problem hiding this comment.
@feanil: Can you point to an example in edx-platform? I'm not clear on what this is, and thus not clear on the recommendation.
|
@feanil: You should review openedx/edx-drf-extensions#284 and all its comments to see if you are missing anything, and you may want to reference it in this ADR. |
5 participants
related issue: #38169