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

[DEPR]: BearerAuthentication #284

Open
robrap opened this issue Dec 8, 2022 · 2 comments
Open

[DEPR]: BearerAuthentication #284

robrap opened this issue Dec 8, 2022 · 2 comments
Labels
depr Proposal for deprecation & removal per OEP-21

Comments

@robrap
Copy link
Contributor

robrap commented Dec 8, 2022

Proposal Date

2022-12-08

Target Ticket Acceptance Date

2022-12-23

Earliest Open edX Named Release Without This Functionality

Q - 2023-10

Rationale

The use of BearerAuthentication was informally documented as deprecated in OEP-42: Authentication, created on 2020-02-28. This OEP was just capturing an earlier (possibly undocumented) decision to deprecate in favor of the newer JWT authentication. At the time OEP-42 was documented, the only reason to keep BearerAuthentication was in support of mobile apps, because they didn't yet support JWT authentication. However, JWT Authentication is now supported for mobile apps as well.

The reason to remove BearerAuthentication is to simplify our authentication to fewer technologies, making it safer and easier to understand.

Removal

Docs and referencing code will need to be updated. See BearerAuthentication GitHub search results for a start.

This ADR will be partially superseded/updated, because BearerAuthentication would no longer be an option.

Replacement

Usage will be replaced by JwtAuthentication:

class JwtAuthentication(JSONWebTokenAuthentication):

Deprecation

There is no active plan.

  • The BearerAuthentication class is not currently marked as deprecated, but it could be.
  • We could mark as deprecated when access tokens are requested in the form of Bearer (opaque) tokens.

Migration

It should be pretty simple for clients to request a JWT access token, in place of a Bearer token, and pass it in the request header instead.

However, there are some issues to be aware of:

  1. This might be used by third parties, like other enterprises, and a long enough communication effort would be needed if we don't automatically fix requests.
  2. Requests currently require a separate authorization header prefix (e.g. "JWT" vs "Bearer" ), so when we ultimately switch default access token requests from Bearer to JWT, we'd either start failing a lot of requests, or we'd need some code to try to fix the header if the wrong one was used (or some other code like this).
  3. Depending on your config, it is likely that JWTs will expire after an hour, but Bearer tokens would have lasted for 2 weeks? If a third-party is running a long job (> an hour) and reusing a token without checking its expiration, they would run into issues once the token expires.

Additional Info

There is a custom attribute that allows for monitoring of BearerAuthentication usage.

UPDATE: The edx-platform implementation uses the same custom attribute, which is very misleading. Additional monitoring is required to ensure we know which version of the class is in-use so that we can retire the edx-drf-extensions version, and then ultimately retire the edx-platform version.

@robrap robrap added the depr Proposal for deprecation & removal per OEP-21 label Dec 8, 2022
@robrap robrap moved this from Proposed to Communicated in DEPR: Deprecation & Removal Dec 9, 2022
@robrap robrap self-assigned this Dec 9, 2022
@robrap robrap removed their assignment Jan 12, 2023
@dianakhuang dianakhuang moved this from Communicated to Accepted in DEPR: Deprecation & Removal Jan 12, 2023
@robrap
Copy link
Contributor Author

robrap commented May 11, 2023

Note: When we start switching to JWTs where opaque tokens were formerly used, the new token will be much larger than the old token. In general this shouldn't be an issue, but if anyone has special code that make assumptions about the max size of this token (like some proctoring code had), it could cause unexpected issues.

@robrap
Copy link
Contributor Author

robrap commented Aug 17, 2023

Additional details:

  1. Ecommerce is configured with its [own subclass of BearerAuthentication](See https://github.com/search?q=repo%3Aopenedx%2Fecommerce%20BearerAuthentication&type=code.). However, since the tokens are all in the LMS database, it makes no sense for this to be used. According to monitoring, all uses have the result of None as reported by this line, so it should be a no-op to just remove this from ecommerce.
  2. Edx-platform has its own versions of BearerAuthentication (and related authentication classes), and are really what this DEPR is about. See https://github.com/openedx/edx-platform/blob/f20b7ec985e9a5450b48b9725c4f76821041e844/openedx/core/lib/api/authentication.py. I'm not clear on how we ended up with a version in edx-drf-extensions, since this form of authentication was only ever available inside edx-platform.

Based on the above, it seems like clean up of ecommerce should be the simplest. Clean-up of edx-drf-extensions versions of BearerAuthentication should be done next, simply switching to the edx-platform implementation. Finally, there would be the work of getting people to stop using these Bearer tokens, which is the harder part of this DEPR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depr Proposal for deprecation & removal per OEP-21
Projects
Status: Proposed
Development

No branches or pull requests

1 participant