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

Update all edx-platform REST endpoints to support JWT Auth #34152

Merged
merged 15 commits into from
Feb 13, 2024

Conversation

salman2013
Copy link
Contributor

@salman2013 salman2013 commented Jan 31, 2024

Description: The default DRF Auth classes were recently updated to allow for both JWT and Session auth by default. Any endpoint that overrides the AUTHENTICATION_CLASSES but has just session, just JWT or just both of those should be updated to remove the override.

Ticket: https://github.com/orgs/openedx/projects/55/views/1?pane=issue&itemId=39035029

@salman2013 salman2013 changed the title Salman/update api end point Update all edx-platform REST endpoints to support JWT Auth Feb 1, 2024
Copy link
Contributor

@farhan farhan left a comment

Choose a reason for hiding this comment

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

@salman2013 salman2013 requested a review from a team as a code owner February 7, 2024 15:37
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

This loos great, thanks @salman2013!

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Saw more things after the review. One quick fix and then this should be good to merge.

@@ -1146,6 +1146,7 @@ class ReplaceUsernamesView(APIView):

"""

authentication_classes = (JwtAuthentication,)
Copy link
Contributor

Choose a reason for hiding this comment

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

@salman2013 why was this added back in? It looks like there was a mismatch of response codes for a test. 403 vs 401. I think the more correct thing to do is to update the test in this case. We saw this issue here as well.

@salman2013
Copy link
Contributor Author

@feanil I have fixed the above mention things, i believe its good to merge now. thanks

@feanil feanil merged commit 57b480b into openedx:master Feb 13, 2024
46 checks passed
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX 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.

4 participants