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

feat: Added token grant_type in JWT payload #30519

Merged
merged 1 commit into from
Jun 30, 2022
Merged

feat: Added token grant_type in JWT payload #30519

merged 1 commit into from
Jun 30, 2022

Conversation

jawad-khan
Copy link
Contributor

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
Copy link
Contributor

@robrap robrap left a comment

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.

Copy link
Contributor

@robrap robrap left a comment

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.

Copy link
Contributor

@robrap robrap left a comment

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.

Copy link
Contributor

@robrap robrap left a comment

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.


# .. 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)
Copy link
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
# TODO: Remove this broad except if proven this doesn't happen.
grant_type = 'unknown-error'
Copy link
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
Copy link
Contributor

robrap commented Jun 24, 2022

[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
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
Contributor

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

@edx-pipeline-bot
Copy link
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
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