-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: Added token grant_type in JWT payload #30519
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
61 changes: 61 additions & 0 deletions
61
.../core/djangoapps/oauth_dispatch/docs/decisions/0013-mobile-migration-to-jwt.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
13. Mobile migration to JWT | ||
########################### | ||
|
||
Status | ||
------ | ||
|
||
Accepted | ||
|
||
Context | ||
------- | ||
|
||
The Open edX mobile apps contain the final usage of the deprecated opaque Bearer access tokens, as documented in the `OAuth2 and Bearer Tokens section of OEP-42 on Authentication`_. Once the mobile apps are no longer using these opaque tokens, BearerAuthentication could be removed through the DEPR process. | ||
|
||
Additionally, the Open edX mobile apps plan to add payment features which require calling some ecommerce apis. However, the opaque Bearer access tokens can only be used for authentication and api calls in the lms. As detailed in OEP-42, the JWT access token is the recommended method of authenticating across services. | ||
|
||
Until this time, JWT access tokens were not supported for parts of the mobile authentication workflow. Once JWT authentication is enabled for mobile, most of the required apis for the new features already support JWT, and the ones that don't can be easily updated. | ||
|
||
.. _OAuth2 and Bearer Tokens section of OEP-42 on Authentication: https://github.com/openedx/open-edx-proposals/blob/6accfc7d5440c9c02f0c17e6ce65c7141af9551f/oeps/best-practices/oep-0042-bp-authentication.rst#oauth2-and-bearer-tokens | ||
|
||
Decisions | ||
--------- | ||
|
||
The mobile apps will migrate its authentication worklflow from using opaque Bearer access tokens to JWT access tokens for authentication, enabling the use of JWTs for cross-service authenticated api calls. | ||
|
||
The mobile app currently obtains an edX-issued access token in either of the following ways: | ||
|
||
* ``AccessToken View: username/email + password combo with grant_type=password`` | ||
* ``AccessTokenExchangeView: 3rd party (social-auth) OAuth 2.0 access token -> 1st party (Open edX) OAuth 2.0 access token`` | ||
|
||
Additionally, the mobile app can exchange an access token for a session cookie that can be used in a WebView: | ||
|
||
* ``LoginWithAccessTokenView: 1st party (Open edX) OAuth 2.0 access token -> session cookie`` | ||
|
||
The above three endpoints will be updated to also support JWTs. The mobile apps will then use ``JwtAuthentication`` on all the apis they call. Note: almost all the apis already support ``JwtAuthentication``. | ||
|
||
Consequences | ||
------------ | ||
|
||
For migrating the mobile authentication flow from opaque Bearer access tokens to JWTs, the following security risks were identified, and will be mitigated as detailed below: | ||
|
||
* Reduce JWT access token expiration. | ||
|
||
* One major difference between JWT and Bearer access tokens is that the JWT is non-revocable. Intentionally, there is no database lookup for a JWT and it is simply trusted if found to be valid. | ||
* One consequence of this is that a JWT should have a short lifetime in order to limit security risks if the token is hijacked. | ||
* JWT access token expiration time needs to be configurable separately from the opaque Bearer tokens to enable this change. (Completed as of the writing of this ADR.) | ||
|
||
* User Account Status: | ||
|
||
* For 1st-party token exchange for session login using JWTs, it is especially important to ensure that the user account has not been disabled, since the JWT is non-revocable. | ||
|
||
* Password Grant Check | ||
|
||
* For the currently proposed 1st-party token exchange for session login using JWTs, we would need an equivalent check to the existing `_is_grant_password` to not expand permissiveness of the endpoint. | ||
* For resolution, see the `ADR to add grant type in JWT payload`_. | ||
|
||
* Asymmetrically Signed JWT | ||
|
||
* We need to check if the JWT was asymmetrically signed by the LMS. We want to ensure that a symmetrically signed JWT, created (signed) by another IDA, could not be compromised and used by an attacker to exchange for a session cookie, which would allow for full compromise of the user. | ||
* Implementation will involve adding a method to ``edx-drf-extensions`` like ``get_decoded_jwt_from_auth``, but that will decode only asymmetric JWTs. | ||
|
||
.. _ADR to add grant type in JWT payload: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0014-add-grant-type-in-jwt-payload.rst |
29 changes: 29 additions & 0 deletions
29
...djangoapps/oauth_dispatch/docs/decisions/0014-add-grant-type-in-jwt-payload.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
14. Added grant type in JWT payload | ||
################################### | ||
|
||
Status | ||
------ | ||
|
||
Accepted | ||
|
||
Context | ||
------- | ||
|
||
Open edX mobile apps are `migrating to JWT token`_ authentication. | ||
As part of the current mobile authentication flow, we exchange an opaque access token for a session. | ||
This is only allowed for access tokens created with the password grant type. | ||
However, as we migrate from using the opaque access token to a JWT, we are no longer able to determine the grant type of the JWT. | ||
|
||
.. _migrating to JWT token: 0013-mobile-migration-to-jwt.rst | ||
|
||
Decisions | ||
--------- | ||
|
||
A new claim ``grant_type`` will be added to the payload while creating the JWT. | ||
The claim ``grant_type`` will be determined using the grant type information stored for the django-oauth-toolkit (DOT) opaque access token that the JWT was based off of during the mobile flow. | ||
JWTs created from an access token in mobile flow will have a grant type of ``password``, and can be used for the session exchange. | ||
|
||
Consequences | ||
------------ | ||
|
||
* JWT will have an additional claim "grant_type" in its payload, which we can use to take decisions regarding auth exchange. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,19 @@ | |
|
||
|
||
import json | ||
import logging | ||
from time import time | ||
|
||
from django.conf import settings | ||
from edx_django_utils.monitoring import set_custom_attribute | ||
from edx_rbac.utils import create_role_auth_claim_for_user | ||
from jwkest import jwk | ||
from jwkest.jws import JWS | ||
|
||
from common.djangoapps.student.models import UserProfile, anonymous_id_for_user | ||
|
||
log = logging.getLogger(__name__) | ||
|
||
|
||
def create_jwt_for_user(user, secret=None, aud=None, additional_claims=None, scopes=None): | ||
""" | ||
|
@@ -63,6 +67,16 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): | |
client = oauth_adapter.get_client_for_token(access_token) | ||
|
||
jwt_expires_in = _get_jwt_access_token_expire_seconds() | ||
try: | ||
grant_type = access_token.application.authorization_grant_type | ||
except Exception: # pylint: disable=broad-except | ||
# TODO: Remove this broad except if proven this doesn't happen. | ||
grant_type = 'unknown-error' | ||
log.exception('Unable to get grant_type from access token.') | ||
robrap marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# .. custom_attribute_name: create_jwt_grant_type | ||
# .. custom_attribute_description: The grant type of the newly created JWT. | ||
set_custom_attribute('create_jwt_grant_type', grant_type) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jawad-khan: [inform] I added a custom attribute in case we want to review this data in New Relic. |
||
|
||
jwt_access_token = _create_jwt( | ||
access_token.user, | ||
|
@@ -71,6 +85,7 @@ def create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key=None): | |
use_asymmetric_key=use_asymmetric_key, | ||
is_restricted=oauth_adapter.is_client_restricted(client), | ||
filters=oauth_adapter.get_authorization_filters(client), | ||
grant_type=grant_type, | ||
robrap marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
jwt_token_dict = token_dict.copy() | ||
|
@@ -120,6 +135,7 @@ def _create_jwt( | |
additional_claims=None, | ||
use_asymmetric_key=None, | ||
secret=None, | ||
grant_type=None, | ||
): | ||
""" | ||
Returns an encoded JWT (string). | ||
|
@@ -132,6 +148,7 @@ def _create_jwt( | |
expires_in (int): Optional. Overrides time to token expiry, specified in seconds. | ||
filters (list): Optional. Filters to include in the JWT. | ||
is_restricted (Boolean): Whether the client to whom the JWT is issued is restricted. | ||
grant_type (str): grant type of the new JWT token. | ||
|
||
Deprecated Arguments (to be removed): | ||
aud (string): Optional. Overrides configured JWT audience claim. | ||
|
@@ -152,6 +169,7 @@ def _create_jwt( | |
# TODO (ARCH-204) Consider getting rid of the 'aud' claim since we don't use it. | ||
'aud': aud if aud else settings.JWT_AUTH['JWT_AUDIENCE'], | ||
'exp': exp, | ||
'grant_type': grant_type or '', | ||
'iat': iat, | ||
'iss': settings.JWT_AUTH['JWT_ISSUER'], | ||
'preferred_username': user.username, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jawad-khan: [inform] I changed the grant type for this error condition so we can more easily tell the difference between this issue, and an empty string in the db.