feat: Add JwtAuthentication as a default DRF auth class. #32802

feanil · 2023-07-20T19:15:23Z

By default DRF sets 'DEFAULT_AUTHENTICATION_CLASSES' to:

[
    'rest_framework.authentication.SessionAuthentication',
    'rest_framework.authentication.BasicAuthentication'
]

We also want to allow for JWT Authentication as a valid default auth
choice. This will allow users to send JWT tokens in the authorization
header to any existing API endpoints and access them. If any APIs have
set custom authentication classes, this will not override that.

I believe this is a fairly safe change to make since it only adds one
authentication class and does not impact authorization of any of the
endpoints that might be affected.

robrap

Thanks. Some thoughts...

lms/envs/common.py

robrap · 2023-07-20T20:19:15Z

Also, there is cms.

feanil · 2023-07-20T20:36:26Z

@robrap yea, this is indeed blocked on the forgiving JWT auth, because without it we will have auth failures where the JWT is not provided. It took going through this exercise to see that unfortunately, sorry for the ping. Without the JWTAuthentication class behaving like DRF expects, it's hard to move forward on this so I'm gonna focus back on that.

robrap · 2023-07-20T21:33:04Z

@robrap yea, this is indeed blocked on the forgiving JWT auth, because without it we will have auth failures where the JWT is not provided.

UPDATED: when the JWT is not provided, it should just move on to the next authentication class. If an invalid JWT is provided, it would fail and not try any other means.

Did you actually see this cause a problem, and if so, can you explain where? Note that the JWT Cookie doesn’t become a single cookie (it starts as two) unless the special header is supplied which only the MFEs supply.

feanil · 2023-07-21T17:27:07Z

Still digging further but pytest openedx/core/djangoapps/embargo/tests/test_views.py::CheckCourseAccessViewTest::test_course_access_endpoint_with_logged_out_user is the failing test here and it looks to be failing because it's getting a 401 instead of a 403, which it looks like is happening because of an exception somewhere in the auth chain. I haven't figured out exactly what yet though.

robrap · 2023-07-21T18:12:26Z

Still digging further but pytest openedx/core/djangoapps/embargo/tests/test_views.py::CheckCourseAccessViewTest::test_course_access_endpoint_with_logged_out_user is the failing test here and it looks to be failing because it's getting a 401 instead of a 403

What’s interesting is that for a logged out user (from the name of the test), a 401 seems more correct, right? We should still probably understand why this is changing the response.

robrap · 2023-07-24T22:57:38Z

@feanil: What I discovered is that with or without this change, we are getting rest_framework.exceptions.NotAuthenticated. The problem is in the exception handler here:
https://github.com/encode/django-rest-framework/blob/56946fac8f29aa44ce84391f138d63c4c8a2a285/rest_framework/views.py#L456C3-L456C3

When including JwtAuthentication, the auth_header becomes 'JWT realm="api"'. Without it, it is None. We'll need to discuss. This is not ideal, because I feel like we might have code that acts differently for 401s and 403s.

Even the forgiving JWT cookie code won't fix this. We'll need to discuss.

robrap · 2023-07-25T18:38:18Z

@feanil: More thoughts on my previous comment.

I found https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/authentication.md#unauthorized-and-forbidden-responses, which explains the 401 vs 403 code I found.
We'll just need to live with the change (i.e. update the unit test), and I'll just need to ensure nothing breaks on edx.org.
Because this is mostly just a setting change, are you ok if I work on deploying/testing on edx.org in advance of merging this?
- The only delay on this is if I move these errors from ignored to expected in New Relic for better visibility. I haven't decided if I want to block on that. Does that seem pointless? Should I just track 4XX to ensure nothing gets out of hand?

robrap · 2023-07-26T15:31:31Z

@feanil: Here is my updated plan:

Trying to deploy this change on edx.org in Stage today.
Hoping to deploy to Prod on Tuesday.

feanil · 2023-07-26T15:56:39Z

@robrap this seems fine to me but what setting are you changing?

robrap · 2023-07-26T18:02:27Z

@robrap this seems fine to me but what setting are you changing?

@feanil: I am adding the following as temp commits to edx-internal:

  DEFAULT_AUTHENTICATION_CLASSES: [
    'edx_rest_framework_extensions.auth.jwt.authentication.JwtAuthentication',
    'rest_framework.authentication.SessionAuthentication',
    'rest_framework.authentication.BasicAuthentication',
  ]

I will back them out once they are tested and we land the defaults in edx-platform in this PR. Does that make sense?

feanil · 2023-07-26T18:31:40Z

Gotcha, sounds good to me.

robrap · 2023-07-26T18:38:20Z

[request] @feanil:

Please fix the failed test. Let's make sure that there aren't other failures.
Can you edit the title to include "LMS" and create another PR for "CMS" to ensure no tests break there?

feanil · 2023-07-26T20:39:02Z

1. Please fix the failed test. Let's make sure that there aren't other failures.

Done, added as a new commit.

2. Can you edit the title to include "LMS" and create another PR for "CMS" to ensure no tests break there?

This is not needed because cms/envs/common.py imports the DRF defaults from the LMS so this change actually updates the defaults for both.

Reference: https://github.com/openedx/edx-platform/blob/master/cms/envs/common.py#L73

robrap · 2023-07-26T21:24:54Z

@feanil: Looks like there are more failures, but hopefully they are all for 403 => 401.

feanil · 2023-07-27T18:44:23Z

@robrap looks like they are, I'll keep stomping at them, until this is green.

robrap · 2023-08-02T20:20:42Z

@feanil: Mostly copied from Slack so it doesn't get lost. Are you planning on following up on this?

Regarding AuthN MFE:

I see an error related to 401 and CSRF after loading the page (see below). The same error appears, in addition to other CSRF failures, when I try logging in as [email protected].
I do not see these errors on edx.org.
I don't know if these errors are a red herring, or related to why I can't log in to the sandbox.
Additionally, the 403 specific code in the MFE may still be important to fix for proper error handling, but I agree that at first glance, it doesn't seem like it would cause a failed login: https://github.com/search?q=repo%3Aopenedx%2Ffrontend-app-authn+403&type=code
Error: Uncaught (in promise)
{
"customAttributes": {
"httpErrorType": "api-response-error",
"httpErrorStatus": 401,
"httpErrorResponseData": "{"detail":"Invalid username/password."}",
"httpErrorRequestUrl": "https://robrap.sandbox.edx.org/csrf/api/v1/token",
"httpErrorRequestMethod": "get"
},
"message": "Axios Error (Response): 401 - See custom attributes for details."
}

Potential known issues for Mobile:

Mobile team wrote: "We are using 403 returned from the API /oauth2/exchange_access_token/{backend}/ and the status (403) means user is disabled. {backend}: Apple/Google/Microsoft/Facebook.
I did not look into whether this endpoint would be affected or not. If it would, we could override.

robrap · 2023-08-02T20:41:50Z

@feanil: I've also documented the observability enhancement idea in #32899, because it might result in a separate PR that could merge sooner.

robrap · 2023-08-22T20:20:02Z

I'm keeping this as a Draft until I land my testing. I'm hoping it won't be long.

robrap · 2023-08-22T20:36:53Z

@feanil: I am copying this info from elsewhere. I also added some notes from edx.org monitoring which help me feel more confident that this is unlikely to cause problems.

Consider adding a backward incompatible note to the commit comment and using feat!:.

See potential comments:

BREAKING CHANGE: For any affected endpoint that also required the user to be authenticated, the endpoint will now return a 401 in place of a 403 when the user is not authenticated.

See these DRF docs for a deeper explanation about why this changes.
Here is an example endpoint that does not override defaults and checks for IsAuthenticated.

Generally speaking, this is should not be a problem.

An issue would appear only if the caller of the endpoint is specifically handling 403s in a way that would be missed for 401s.

Note: On edx.org, the only endpoints using the default authentication classes that also had 403 responses were the following:

/api/embargo/v1/course_access/
/api/notifications/count/
/api/notifications/enrollments/
/courses/yt_video_metadata

robrap · 2023-08-23T16:22:17Z

@feanil: I just discovered we will possibly be breaking docstrings as well. Here is one example of a docstring for NotificationCountView that would need to be updated.

Based on monitoring, I've found 37 views that will be affected by this change:

WebTransaction/Function/openedx.core.djangoapps.notifications.views:NotificationCountView.get
WebTransaction/Function/csrf.api.v1.views:CsrfTokenView.get
WebTransaction/Function/openedx.core.djangoapps.user_authn.api.views:MFEContextView.get
WebTransaction/Function/lms.djangoapps.courseware.views.views:yt_video_metadata.get
WebTransaction/Function/openedx.core.djangoapps.user_api.views:CountryTimeZoneListView.get
WebTransaction/Function/openedx.core.djangoapps.embargo.views:CheckCourseAccessView.get
WebTransaction/Function/openedx.core.djangoapps.user_authn.views.password_reset:LogistrationPasswordResetView.post
WebTransaction/Function/openedx.core.djangoapps.user_authn.views.password_reset:PasswordResetTokenValidation.post
WebTransaction/Function/openedx.core.djangoapps.notifications.views:CourseEnrollmentListView.get
WebTransaction/Function/lms.djangoapps.mfe_config_api.views:MFEConfigView.get
WebTransaction/Function/lms.djangoapps.support.views.manage_user:ManageUserDetailView.get
WebTransaction/Function/lms.djangoapps.support.views.enrollments:EnrollmentSupportListView.get
WebTransaction/Function/lms.djangoapps.support.views.sso_records:SsoView.get
WebTransaction/Function/lms.djangoapps.support.views.onboarding_status:OnboardingView.get
WebTransaction/Function/enterprise.heartbeat.views:heartbeat.get
WebTransaction/Function/lms.djangoapps.course_home_api.outline.views:unsubscribe_from_course_goal_by_token.post
WebTransaction/Function/lms.djangoapps.support.views.program_enrollments:SAMLProvidersWithOrg.get
WebTransaction/Function/drf_yasg.views:get_schema_view.<locals>.SchemaView.get
WebTransaction/Function/lms.djangoapps.verify_student.views:VerificationStatusAPIView.get
WebTransaction/Function/ai_aside.config_api.views:CourseSummaryConfigEnabledAPIView.get
WebTransaction/Function/ai_aside.config_api.views:CourseEnabledAPIView.get
WebTransaction/Function/enterprise.api.v1.views.plotly_auth:PlotlyAuthView.get
WebTransaction/Function/ai_aside.config_api.views:CourseEnabledAPIView.post
WebTransaction/Function/lms.djangoapps.support.views.enrollments:EnrollmentSupportListView.patch
WebTransaction/Function/edx_proctoring.views:AnonymousReviewCallback.post
WebTransaction/Function/lms.djangoapps.support.views.enrollments:EnrollmentSupportListView.post
WebTransaction/Function/openedx.core.djangoapps.notifications.views:NotificationListAPIView.get
WebTransaction/Function/openedx.core.djangoapps.notifications.views:UserNotificationPreferenceView.patch
WebTransaction/Function/lms.djangoapps.learner_recommendations.views:CrossProductRecommendationsView.get
WebTransaction/Function/integrated_channels.api.v1.moodle.views:MoodleConfigurationViewSet.update
WebTransaction/Function/ai_aside.config_api.views:CourseEnabledAPIView.delete
WebTransaction/Function/edx_recommendations.api.cross_product_recommendations:CrossProductRecommendationsView.get
WebTransaction/Function/rest_framework.routers:APIRootView.get
WebTransaction/Function/openedx.core.djangoapps.notifications.views:UserNotificationPreferenceView.get
WebTransaction/Function/enterprise.admin.views:CatalogQueryPreviewView.get
WebTransaction/Function/coaching.rest_api.v1.views:ConsentFormViewset.partial_update
WebTransaction/Function/openedx.core.djangoapps.notifications.views:MarkNotificationsSeenAPIView.put

feanil · 2023-08-23T17:41:19Z

Good point, thanks for realizing and running the query, I'm on PTO for the rest of this week but I'll update all those docstrings when I'm back next week. We should consider this PR blocked until that update is made.

robrap · 2023-08-23T20:17:10Z

We should consider this PR blocked until that update is made.

@feanil: The likely situation is that this PR will be blocked until I've rolled out forgiving JWTs and this change as overrides in edx.org. By that time, I imagine this PR will be ready to remove from Draft and merge.

feanil · 2023-09-01T14:14:46Z

@robrap I took a closer look at this and I don't think the docstrings need to change. Though there is a behavioral change. Because we set JWT Auth to be first in the list. If the user uses JWT auth to hit the endpoints, they'll get the correct behavior of 403 for permission denied and 401 for when they have failed to auth. But if they hit the endpoint with a session id cookie, to hit the same endpoint, they'll get a 401 now while they used to get a 403.

This is a change in behavior so we should make sure there are no adverse effects when you roll this out but in terms of docstrings, they are always going to be right/wrong depending on which auth type we're using. In my opinion, this seems like a shortcoming of DRF(I feel like it should be able to understand if you succeeded in authenticating independently from running into permission issues but it dosen't do that at the moment.)

I think the best course of action is to leave the docs as is because they convey the correct behavior for APIs using JWT auth which is what we're moving towards.

robrap · 2023-09-01T14:39:11Z

@feanil: It is indeed complicated.

If you provide no credentials, it will return a response based on the first authentication class. In this case, it will be JwtAuthentication and a 401.
If you provide invalid credentials, like a bad session token (which I think is the case you are referring to?), then that authentication class itself would fail, and in the case of SessionAuthentication, you would get a 403.
For other permission errors that are not authentication related, I think you just get a 403.

I am asking some teams to make error handling updates to respond to 401 or 403, which will be more resilient to this. Based on all of our findings, it seems the docs should really state "401 or 403", and not just 403, so people know how to appropriately handle errors. Thoughts?

feanil · 2023-09-01T15:01:33Z

So in all of my testing I was using valid tokens to actually check the permission issue, it sounds like there was a historic confusing issue where we used to say 403 for bad credentials which we no longer do and that could cause issues?

robrap · 2023-09-01T18:59:39Z

@feanil: I think we agreed offline that:

The switch from 403 to 401 only seems to affect the situation where no credentials are provided.
We don't think it is worth documenting this (docstrings, etc.) outside of the failing tests that were switched to 401.

Thus, there is nothing left to do except wait on my deployments and testing and I'll let you know when we are ready for this. Does that sound right?

feanil · 2023-09-05T18:34:44Z

@robrap that sounds right to me. Do you want to leave this PR in draft till then in-case we find there are more changes to make?

robrap · 2023-09-05T18:49:44Z

@feanil: Yes. Let's leave it in draft. I hope there will be no more issues, but this will ensure I complete testing this change in Prod on edx.org before we merge this.

By default DRF sets 'DEFAULT_AUTHENTICATION_CLASSES' to: ``` [ 'rest_framework.authentication.SessionAuthentication', 'rest_framework.authentication.BasicAuthentication' ] ``` We also want to allow for JWT Authentication as a valid default auth choice. This will allow users to send JWT tokens in the authorization header to any existing API endpoints and access them. If any APIs have set custom authentication classes, this will not override that. I believe this is a fairly safe change to make since it only adds one authentication class and does not impact authorization of any of the endpoints that might be affected. Note: This change changes the default for both the LMS and CMS because `cms/envs/common.py` imports this value from the LMS. BREAKING CHANGE: For any affected endpoint that also required the user to be authenticated, the endpoint will now return a 401 in place of a 403 when the user is not authenticated. - See [these DRF docs](https://github.com/encode/django-rest-framework/blob/master/docs/api-guide/authentication.md#unauthorized-and-forbidden-responses) for a deeper explanation about why this changes. - Here is [an example endpoint](https://github.com/openedx/edx-platform/blob/b8ecfed67dc0520b8c4d95de3096b35acc083611/openedx/core/djangoapps/embargo/views.py#L20-L21) that does not override defaults and checks for IsAuthenticated. Generally speaking, this is should not be a problem. An issue would appear only if the caller of the endpoint is specifically handling 403s in a way that would be missed for 401s.

When including `JwtAuthentication`, the auth_header becomes `JWT realm="api"`. Without it, it is `None`. This changes the behavior of the code in DRF and returns a slightly different auth response. Relevant Code: https://github.com/encode/django-rest-framework/blob/56946fac8f29aa44ce84391f138d63c4c8a2a285/rest_framework/views.py#L456C3-L456C3

edx-pipeline-bot · 2023-11-02T15:10:31Z

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

edx-pipeline-bot · 2023-11-02T15:33:21Z

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

feanil requested review from kdmccormick and a team July 20, 2023 19:24

robrap reviewed Jul 20, 2023

View reviewed changes

lms/envs/common.py Outdated Show resolved Hide resolved

lms/envs/common.py Outdated Show resolved Hide resolved

feanil marked this pull request as draft July 20, 2023 20:34

feanil removed the request for review from kdmccormick July 20, 2023 20:34

robrap mentioned this pull request Jul 26, 2023

Support adding default authentication classes to LMS and Studio edx/edx-arch-experiments#373

Closed

1 task

feanil force-pushed the feanil/default_drf_auth_class branch from 99dce18 to bbd3e63 Compare July 26, 2023 20:37

feanil force-pushed the feanil/default_drf_auth_class branch 2 times, most recently from 96debf3 to 6e9b271 Compare July 31, 2023 21:03

feanil marked this pull request as ready for review July 31, 2023 21:03

robrap mentioned this pull request Aug 2, 2023

Add observability for DEFAULT_AUTHENTICATION_CLASSES #32899

Closed

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

feanil force-pushed the feanil/default_drf_auth_class branch from 6e9b271 to 2eedb40 Compare August 8, 2023 14:17

feanil force-pushed the feanil/default_drf_auth_class branch from dafe868 to b5cccf9 Compare August 22, 2023 19:50

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

robrap marked this pull request as draft August 22, 2023 20:19

feanil force-pushed the feanil/default_drf_auth_class branch 2 times, most recently from be6adb8 to 653d44d Compare August 23, 2023 13:36

feanil force-pushed the feanil/default_drf_auth_class branch from 653d44d to 93cc859 Compare August 30, 2023 14:52

robrap marked this pull request as ready for review October 31, 2023 16:43

robrap approved these changes Oct 31, 2023

View reviewed changes

feanil force-pushed the feanil/default_drf_auth_class branch 3 times, most recently from a2e1921 to efefe80 Compare November 1, 2023 15:00

feanil added 2 commits November 1, 2023 11:03

feanil force-pushed the feanil/default_drf_auth_class branch from efefe80 to ac2cc15 Compare November 1, 2023 15:03

feanil merged commit 9ba9935 into master Nov 2, 2023
65 checks passed

feanil deleted the feanil/default_drf_auth_class branch November 2, 2023 14:05

feanil mentioned this pull request Feb 7, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add JwtAuthentication as a default DRF auth class. #32802

feat: Add JwtAuthentication as a default DRF auth class. #32802

feanil commented Jul 20, 2023

robrap left a comment

robrap commented Jul 20, 2023

feanil commented Jul 20, 2023

robrap commented Jul 20, 2023 •

edited

Loading

feanil commented Jul 21, 2023

robrap commented Jul 21, 2023

robrap commented Jul 24, 2023

robrap commented Jul 25, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 27, 2023

robrap commented Aug 2, 2023

robrap commented Aug 2, 2023

robrap commented Aug 22, 2023

robrap commented Aug 22, 2023

robrap commented Aug 23, 2023

feanil commented Aug 23, 2023

robrap commented Aug 23, 2023

feanil commented Sep 1, 2023

robrap commented Sep 1, 2023

feanil commented Sep 1, 2023

robrap commented Sep 1, 2023

feanil commented Sep 5, 2023

robrap commented Sep 5, 2023

edx-pipeline-bot commented Nov 2, 2023

edx-pipeline-bot commented Nov 2, 2023

feat: Add JwtAuthentication as a default DRF auth class. #32802

feat: Add JwtAuthentication as a default DRF auth class. #32802

Conversation

feanil commented Jul 20, 2023

robrap left a comment

Choose a reason for hiding this comment

robrap commented Jul 20, 2023

feanil commented Jul 20, 2023

robrap commented Jul 20, 2023 • edited Loading

feanil commented Jul 21, 2023

robrap commented Jul 21, 2023

robrap commented Jul 24, 2023

robrap commented Jul 25, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 26, 2023

robrap commented Jul 26, 2023

feanil commented Jul 27, 2023

robrap commented Aug 2, 2023

robrap commented Aug 2, 2023

robrap commented Aug 22, 2023

robrap commented Aug 22, 2023

robrap commented Aug 23, 2023

feanil commented Aug 23, 2023

robrap commented Aug 23, 2023

feanil commented Sep 1, 2023

robrap commented Sep 1, 2023

feanil commented Sep 1, 2023

robrap commented Sep 1, 2023

feanil commented Sep 5, 2023

robrap commented Sep 5, 2023

edx-pipeline-bot commented Nov 2, 2023

edx-pipeline-bot commented Nov 2, 2023

robrap commented Jul 20, 2023 •

edited

Loading