Skip to content

feat: Added token grant_type in JWT payload - #30519

Merged
jawad-khan merged 1 commit into
masterfrom
LEARNER-8886
Jun 30, 2022
Merged

feat: Added token grant_type in JWT payload#30519
jawad-khan merged 1 commit into
masterfrom
LEARNER-8886

Conversation

@jawad-khan

Copy link
Copy Markdown
Member

Description

To exchange jwt with session cookies we need to determine JWT grant type in AccessTokenExchangeView.
JWT only having password grant type will be allowed to exchange session.

Supporting information

JIRA Issue: https://2u-internal.atlassian.net/browse/LEARNER-8886

@jawad-khan
jawad-khan requested a review from robrap June 1, 2022 09:55

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Happy to discuss any questions if any of this is not clear.

Comment thread openedx/core/djangoapps/oauth_dispatch/tests/mixins.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py Outdated

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite done with the review, but submitting so you can see comments.

Comment thread openedx/core/djangoapps/oauth_dispatch/jwt.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/mixins.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/tests/test_jwt.py Outdated

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possibly-blocking comment. Thanks.

Comment thread openedx/core/djangoapps/oauth_dispatch/jwt.py Outdated
Comment thread openedx/core/djangoapps/oauth_dispatch/jwt.py Outdated

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jawad-khan: This is ready from my end. Please review the ADR changes, and squash and merge if you are content. If not, I can review your questions or updates. Thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Comment on lines 73 to 74

Copy link
Copy Markdown
Contributor

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.

@jawad-khan
jawad-khan force-pushed the LEARNER-8886 branch 4 times, most recently from c867dca to 42846b3 Compare June 23, 2022 21:49
@robrap

robrap commented Jun 24, 2022

Copy link
Copy Markdown
Contributor

[inform] I tried rebasing, but still getting test failures. I am trying to re-run to see what happens.

To exchange jwt with session cookies we need to determine JWT grant type in
AccessTokenExchangeView. JWT only having password grant type will be allowed to exchange session.
Added ADR for mobile migration to JWT authentication.

LEARNER-8886
@jawad-khan
jawad-khan merged commit 2dc7990 into master Jun 30, 2022
@jawad-khan
jawad-khan deleted the LEARNER-8886 branch June 30, 2022 11:49
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants