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

build: upgrading django-auth-toolkit and oauthlib. #30634

Closed
wants to merge 1 commit into from

Conversation

awais786
Copy link
Contributor

@awais786 awais786 commented Jun 21, 2022

https://2u-internal.atlassian.net/browse/BOM-3360

  • This PR is not priority but I tried to remove to pin.
  • Created sandbox and did signup and purchased a course. Things looks good.

django-oauth-toolkit greater version has some breaking changes so I added a new pin. Further upgrade need more discovery.*
https://github.com/jazzband/django-oauth-toolkit/blob/master/CHANGELOG.md#200-2022-04-24

Changes details

Method names changes
PR with this change.

Issue about OIDC request_type "code token" does not save access token.

In version v3.0.1 it has a param
and it was saving the token.

But in version v3.2.0 it ask to save it explicitly.

# for details https://github.com/oauthlib/oauthlib/blob/v3.2.0/oauthlib/oauth2/rfc6749/tokens.py#L303

token = token_generator.create_token(request, refresh_token=True)
token_generator.request_validator.save_token(token, request)
Copy link
Contributor Author

@awais786 awais786 Jun 22, 2022

Choose a reason for hiding this comment

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

Issue about OIDC request_type "code token" does not save access token.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, the old create_token() method had a save_token optional parameter that defaulted to True. Since that param was dropped, we have replaced it with this explicit call to save_token. This looks fine.

@awais786 awais786 force-pushed the django-oauth-toolkit branch 3 times, most recently from e2bae37 to a478cce Compare June 24, 2022 07:56
Copy link
Member

@aht007 aht007 left a comment

Choose a reason for hiding this comment

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

LGTM. Manually reviewed all the changes within oauthlib repository

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.

Nice.

  1. Did we search other repos in both orgs, just in case any edx-platform dependencies also make use of this?
  2. Did you do LTI specific testing?

# for details https://github.com/oauthlib/oauthlib/blob/v3.2.0/oauthlib/oauth2/rfc6749/tokens.py#L303

token = token_generator.create_token(request, refresh_token=True)
token_generator.request_validator.save_token(token, request)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, the old create_token() method had a save_token optional parameter that defaulted to True. Since that param was dropped, we have replaced it with this explicit call to save_token. This looks fine.

Comment on lines 37 to 40
# save_token` has been deprecated, it was not called internally
# If you do, call `request_validator.save_token()`.
# for details https://github.com/oauthlib/oauthlib/blob/v3.2.0/oauthlib/oauth2/rfc6749/tokens.py#L303

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this comment. It seems useful as a comment in the PR, but once this code change lands, the comment just is confusing.

Suggested change
# save_token` has been deprecated, it was not called internally
# If you do, call `request_validator.save_token()`.
# for details https://github.com/oauthlib/oauthlib/blob/v3.2.0/oauthlib/oauth2/rfc6749/tokens.py#L303

Comment on lines 77 to 79
# greater version has breaking changes.
# https://github.com/jazzband/django-oauth-toolkit/blob/master/CHANGELOG.md#200-2022-04-24
django-oauth-toolkit<2.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please alphabetize. Thanks.

@awais786
Copy link
Contributor Author

awais786 commented Jun 24, 2022

Nice.

  1. Did we search other repos in both orgs, just in case any edx-platform dependencies also make use of this?
  2. Did you do LTI specific testing?

I am searching it in both orgs and share my findings.
I will explore LTI and will do testing.

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.

Looks good as long as all the testing and searching other code bases doesn't turn up anything. Also, maybe we should give a #warroom note when deploying to be on the lookout for any auth related issues and let you know.

# https://github.com/jazzband/django-oauth-toolkit/blob/master/CHANGELOG.md#200-2022-04-24
django-oauth-toolkit<2.0.0


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: single line break for consistency.

@awais786
Copy link
Contributor Author

Verified LTI zoom. It is integrated and working.

Screen Shot 2022-06-27 at 3 15 37 PM

** method names changed from `construct_base_string` to `signature_base_string` and from `normalize_base_string_uri` to `base_string_uri`.
** explicitly trigger the `save_token` call.
for more details check
* oauthlib/oauthlib#755
* https://github.com/oauthlib/oauthlib/pull/651/files
@robrap
Copy link
Contributor

robrap commented Oct 6, 2022

FYI: I don't plan on paying attention to this PR until you ask again. I am going to stop watching. I don't want to bring my head into this unless and until it makes sense to. Thank you!

@mumarkhan999
Copy link
Member

Closing this PR in favor of #33341

@nedbat nedbat deleted the django-oauth-toolkit branch January 8, 2024 15:06
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.

None yet

4 participants