fix: upgrade oauthlib and django-oauth-toolkit to new versions. #32631

awais786 · 2023-07-03T14:23:34Z

🔗🔗🔗 Issue link

#32515

📝📝📝 Description

django-oauth-toolkit and oauthlib versions have been pinned for the last 3 years due to test failures
In this PR we're unpinning them to django-oauth-toolkit==1.7.1 and oauthlib==3.2.2
Moving oauthlib to latest version wasn't a big deal as we only had to update a few method names
But moving django-oauth-toolkit from 1.x.x to 2.x.x is a little bit tricky as in 2.x.x the client_secret is being hashed
https://django-oauth-toolkit.readthedocs.io/en/latest/changelog.html#id12:~:text=%231093%20(Breaking,before%20hitting%20Save
whereas in the previous releases, it was just a plain text. That's why we're just upgrading django-oauth-toolkit to the latest 1.x release for now.

🔍🔍🔍 Tested on

Sandbox configured with ecommerce.

⚠️⚠️⚠️ Migrating from django-oauth-toolkit from 1.x.x to 2.x.x

We need to be aware that in the future if we do any attempt to migrate DOT to 2.x.x, it will update all the existing client secrets as well.
And then we can't downgrade its version. It's only one way.

raise IrreversibleError("Operation %s in %s is not reversible" % (operation, self))
django.db.migrations.exceptions.IrreversibleError: Operation <RunPython <function forwards_func at 0x7f6956652430>> in oauth2_provider.0006_alter_application_client_secret is not reversible

mumarkhan999 · 2023-07-07T12:40:45Z

openedx/core/djangoapps/oauth_dispatch/api.py

+    token = token_generator.create_token(request, refresh_token=True)
+    token_generator.request_validator.save_token(token, request)


The create_token method was also saving token in the database in the previous OAuth lib (oauthlib==3.0.1) version which we were using.
https://github.com/oauthlib/oauthlib/blob/v3.0.1/oauthlib/oauth2/rfc6749/tokens.py#L341.

But in the latest version save_token has been deprecated.
https://github.com/oauthlib/oauthlib/blob/v3.2.2/oauthlib/oauth2/rfc6749/tokens.py#L303

Now we need to save tokens explicitly.

PR-653: PR in which save_token functionaly was removed from oauthlib

I am guessing that the refresh token must be saved either way. Is that correct?

I am guessing that we don't need to save JWT tokens, but I'm not certain. I don't think you should spend time working on this, but does the following comment make sense to add?:

# We may not need to save our JWT tokens, but we do need to save the opaque Bearer tokens. # However, since BearerAuthentication is deprecated, we may want to explore removing this save # call in the future once the DEPR has been completed. # See https://github.com/openedx/edx-drf-extensions/issues/284

(1)
It seems refresh_token is independent of the change that we're talking about here.
As you can see refresh token implementation in both versions is similar.

v3.2.2 implementaiton v3.0.1 implementation

Your thoughts?

(2)
Yeah, it makes sense that if we're moving towards JWT we can skip the save call.

But there are other things to consider:

Although this OEP (oep-42) states that BearerAuthentication (edx-drf-extension) is deprecated, but it's still there (link to BearerAuthentication
class)

Also this PR is in edx-platform where we have another quite similarBearerAuthentication class (link to class). This class and its subclasses are being widely used in the code base (link to usage)

So final thoughts are, as long as the BearerAuthentication is there, we need the save call.

Thank you for the note on the edx-platform BearerAuthentication classes. That was a surprise to me. I have updated the DEPR with the additional information. The point of that DEPR is to remove all of BearerAuthentication, so I simply missed these implementation details.

What do you think of the following updated comment proposal:

# This save_token call is required with BearerAuthentication. Once the DEPR for # BearerAuthentication is complete and it has been fully removed, we may no # longer need to save the token, since JWT tokens don't rely on the database. # See DEPR https://github.com/openedx/edx-drf-extensions/issues/284

Additionally, or alternatively, once this lands we can add a comment in the DEPR ticket pointing to the save_token call, and note the same thing.

common/djangoapps/third_party_auth/lti.py

robrap · 2023-07-14T20:16:29Z

[inform] Apologies. I know I was tagged on this, and have it on my backlog, but have yet to get to this. Maybe someone else from arch-bom will pick it up. I am out until 19th or 20th. Thanks for this work.

robrap · 2023-07-19T20:28:12Z

I still plan to review, but one high-level comment would be to create a new issue for "Migrating django-oauth-toolkit from 1.x.x to 2.x.x", and move all relevant warnings and discussions on that topic to that ticket. From this ticket, you could simply mentioned that migrating DOT from 1.x.x to 2.x.x has complications that were moved to . Also, please tag me on the new ticket, because I do have thoughts about it. Thanks.

robrap

Thanks. Minor comments.

robrap · 2023-07-20T21:27:16Z

openedx/core/djangoapps/oauth_dispatch/api.py

+    token = token_generator.create_token(request, refresh_token=True)
+    token_generator.request_validator.save_token(token, request)


I am guessing that the refresh token must be saved either way. Is that correct?

I am guessing that we don't need to save JWT tokens, but I'm not certain. I don't think you should spend time working on this, but does the following comment make sense to add?:

# We may not need to save our JWT tokens, but we do need to save the opaque Bearer tokens. # However, since BearerAuthentication is deprecated, we may want to explore removing this save # call in the future once the DEPR has been completed. # See https://github.com/openedx/edx-drf-extensions/issues/284

robrap · 2023-07-20T21:28:49Z

requirements/constraints.txt

-django-oauth-toolkit<=1.3.2
+# greater version has breaking changes and requires some migration steps.
+# In 2.x.x releases the client_secret is being saved as a hash instead of plain text.
+# https://django-oauth-toolkit.readthedocs.io/en/latest/changelog.html#id12


As noted in another comment, can we create a ticket for the 2.x.x upgrade and link to it here? We can note that the 2.x.x has some complications that are detailed in the ticket.

Additionally, can you confirm whether you can see the secrets unencrypted in the Admin UI, or if you only see it at creation time, and after that it needs to be stored outside the app? Thank you.

The ticket has been created.

Comment having link ticket link

As mentioned in the doc we can only obtain the secrets before hitting save button.

Click here to see doc

mumarkhan999 · 2023-07-27T11:13:11Z

@robrap :
[Inform] Apologies for the late reply. Actually, I was busy with the pymemcache related task. Will get back to you with the answers you asked on this PR. Thanks.

robrap

Thank you. I have some nits around potential updates to two comments, but I'll leave that up to you.

I would like to add a comment on the BearerAuthentication DEPR that points back to the save_token call once this merges.

robrap · 2023-08-17T16:45:46Z

requirements/constraints.txt

+# django-oauth-toolkit version >2.0.0 has breaking changes.
+# More details mentioned on this issue https://github.com/openedx/edx-platform/issues/32884
+# Versions from 1.3.3 to 2.0.0 have some migrations related changes.
+# so we're upgrading minor versions one by one.
+django-oauth-toolkit==1.4.1


Proposed update to:
a. Make it more clear that these are two different points (although related), and
b. Replace 1.X.X with the next version that has migrations that you wish to protect against. Note that we'll already be past 1.3.3 in this PR, so it is already irrelevant.

Suggested change

# django-oauth-toolkit version >2.0.0 has breaking changes.

# More details mentioned on this issue https://github.com/openedx/edx-platform/issues/32884

# Versions from 1.3.3 to 2.0.0 have some migrations related changes.

# so we're upgrading minor versions one by one.

django-oauth-toolkit==1.4.1

# 1. django-oauth-toolkit version >2.0.0 has breaking changes. More details

# mentioned on this issue https://github.com/openedx/edx-platform/issues/32884

# 2. Versions from 1.X.X to 2.0.0 have some migrations related changes.

# so we're upgrading minor versions one by one.

django-oauth-toolkit<1.X.X

edx-pipeline-bot · 2023-08-18T10:30:43Z

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

edx-pipeline-bot · 2023-08-18T10:52:12Z

2U Release Notice: This PR has been deployed to the edX production environment.

…s. (#32631)" This reverts commit 64abfd1.

awais786 · 2023-08-21T09:00:44Z

#33065

…edx#32631) * chore: bump django-oauth-toolkit and oauthlib --------- Co-authored-by: Muhammad Umar Khan <[email protected]>

mumarkhan999 force-pushed the upgrade-auth-lib branch 2 times, most recently from f98bb6c to 840cad7 Compare July 4, 2023 14:45

mumarkhan999 self-assigned this Jul 4, 2023

mumarkhan999 force-pushed the upgrade-auth-lib branch 5 times, most recently from ae87584 to d4a574e Compare July 7, 2023 12:14

mumarkhan999 reviewed Jul 7, 2023

View reviewed changes

mumarkhan999 force-pushed the upgrade-auth-lib branch from a1feddb to a6b3d3e Compare July 11, 2023 08:12

mumarkhan999 reviewed Jul 11, 2023

View reviewed changes

common/djangoapps/third_party_auth/lti.py Show resolved Hide resolved

mumarkhan999 requested a review from robrap July 11, 2023 15:24

mumarkhan999 marked this pull request as ready for review July 11, 2023 15:25

mumarkhan999 force-pushed the upgrade-auth-lib branch from a6b3d3e to 24042ef Compare July 11, 2023 15:29

mumarkhan999 force-pushed the upgrade-auth-lib branch 2 times, most recently from ff078bc to f0abc03 Compare July 17, 2023 12:14

mumarkhan999 mentioned this pull request Jul 17, 2023

build: update django-oauth-toolkit version #31115

Closed

mumarkhan999 force-pushed the upgrade-auth-lib branch from f0abc03 to b7ec85a Compare July 17, 2023 12:25

robrap reviewed Jul 20, 2023

View reviewed changes

mumarkhan999 mentioned this pull request Jul 31, 2023

[Django4.2 upgrade]: Upgrade django-oauth-lib to latest 2.x.x version #32884

Open

robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 3, 2023

mumarkhan999 force-pushed the upgrade-auth-lib branch 3 times, most recently from 5b0aa19 to e2bc311 Compare August 17, 2023 08:54

mumarkhan999 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 17, 2023

mumarkhan999 requested review from robrap and mumarkhan999 August 17, 2023 11:09

robrap approved these changes Aug 17, 2023

View reviewed changes

mumarkhan999 added 2 commits August 18, 2023 10:10

chore: bump django-oauth-toolkit and oauthlib

90e7047

squash! add django-oauth-ticket 2.x upgrade issue link

ba8e6ab

mumarkhan999 force-pushed the upgrade-auth-lib branch from ef0f9cd to 9cf2da3 Compare August 18, 2023 05:10

squash! update comments

5e8e75d

mumarkhan999 force-pushed the upgrade-auth-lib branch from 9cf2da3 to 5e8e75d Compare August 18, 2023 05:11

mumarkhan999 merged commit 64abfd1 into master Aug 18, 2023

mumarkhan999 deleted the upgrade-auth-lib branch August 18, 2023 09:21

awais786 restored the upgrade-auth-lib branch August 21, 2023 08:24

awais786 added a commit that referenced this pull request Aug 21, 2023

Revert "fix: upgrade oauthlib and django-oauth-toolkit to new version…

f16ede0

…s. (#32631)" This reverts commit 64abfd1.

awais786 mentioned this pull request Aug 21, 2023

Revert "fix: upgrade oauthlib and django-oauth-toolkit to new versions." #33065

Closed

mumarkhan999 deleted the upgrade-auth-lib branch September 4, 2023 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: upgrade oauthlib and django-oauth-toolkit to new versions. #32631

fix: upgrade oauthlib and django-oauth-toolkit to new versions. #32631

awais786 commented Jul 3, 2023 •

edited by mumarkhan999

Loading

mumarkhan999 Jul 7, 2023 •

edited

Loading

robrap Jul 20, 2023

mumarkhan999 Aug 17, 2023 •

edited

Loading

mumarkhan999 Aug 17, 2023 •

edited

Loading

robrap Aug 17, 2023

robrap commented Jul 14, 2023

robrap commented Jul 19, 2023

robrap left a comment

robrap Jul 20, 2023

robrap Jul 20, 2023

robrap Jul 26, 2023

mumarkhan999 Aug 17, 2023 •

edited

Loading

mumarkhan999 commented Jul 27, 2023 •

edited

Loading

robrap left a comment

robrap Aug 17, 2023

edx-pipeline-bot commented Aug 18, 2023

edx-pipeline-bot commented Aug 18, 2023

awais786 commented Aug 21, 2023

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

fix: upgrade oauthlib and django-oauth-toolkit to new versions. #32631

fix: upgrade oauthlib and django-oauth-toolkit to new versions. #32631

Conversation

awais786 commented Jul 3, 2023 • edited by mumarkhan999 Loading

mumarkhan999 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

robrap Jul 20, 2023

Choose a reason for hiding this comment

mumarkhan999 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

mumarkhan999 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

robrap Aug 17, 2023

Choose a reason for hiding this comment

robrap commented Jul 14, 2023

robrap commented Jul 19, 2023

robrap left a comment

Choose a reason for hiding this comment

robrap Jul 20, 2023

Choose a reason for hiding this comment

robrap Jul 20, 2023

Choose a reason for hiding this comment

robrap Jul 26, 2023

Choose a reason for hiding this comment

mumarkhan999 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

mumarkhan999 commented Jul 27, 2023 • edited Loading

robrap left a comment

Choose a reason for hiding this comment

robrap Aug 17, 2023

Choose a reason for hiding this comment

edx-pipeline-bot commented Aug 18, 2023

edx-pipeline-bot commented Aug 18, 2023

awais786 commented Aug 21, 2023

awais786 commented Jul 3, 2023 •

edited by mumarkhan999

Loading

mumarkhan999 Jul 7, 2023 •

edited

Loading

mumarkhan999 Aug 17, 2023 •

edited

Loading

mumarkhan999 Aug 17, 2023 •

edited

Loading

mumarkhan999 Aug 17, 2023 •

edited

Loading

mumarkhan999 commented Jul 27, 2023 •

edited

Loading