From 7af2b1db2417ed06ddace6dd8c6bf7c28de85f38 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 20 Jul 2023 15:02:34 -0400 Subject: [PATCH 1/2] feat!: Add JwtAuthentication as a default DRF auth class. 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. --- lms/envs/common.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lms/envs/common.py b/lms/envs/common.py index ac4d9ccd58..de26b10128 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3332,7 +3332,14 @@ CROSS_DOMAIN_CSRF_COOKIE_NAME = '' REST_FRAMEWORK = { # These default classes add observability around endpoints using defaults, and should # not be used anywhere else. + # Notes on Order: + # 1. `JwtAuthentication` does not check `is_active`, so email validation does not affect it. However, + # `SessionAuthentication` does. These work differently, and order changes in what way, which really stinks. See + # https://github.com/openedx/public-engineering/issues/165 for details. + # 2. `JwtAuthentication` may also update the database based on contents. Since the LMS creates these JWTs, this + # shouldn't have any affect at this time. But it could, when and if another service started creating the JWTs. 'DEFAULT_AUTHENTICATION_CLASSES': [ + 'openedx.core.djangolib.default_auth_classes.DefaultJwtAuthentication', 'openedx.core.djangolib.default_auth_classes.DefaultSessionAuthentication', ], 'DEFAULT_PAGINATION_CLASS': 'edx_rest_framework_extensions.paginators.DefaultPagination', From ac2cc158f8e13cfc6deecea203a3a9a25b441922 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 26 Jul 2023 16:32:29 -0400 Subject: [PATCH 2/2] test: Update tests to the new return code. 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 --- .../commerce/api/v0/tests/test_views.py | 2 +- .../djangoapps/embargo/tests/test_views.py | 2 +- .../djangoapps/user_api/tests/test_views.py | 28 +++++++++---------- openedx/core/lib/api/test_utils.py | 4 +++ 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/commerce/api/v0/tests/test_views.py b/lms/djangoapps/commerce/api/v0/tests/test_views.py index d773598f0a..ef9327fe6f 100644 --- a/lms/djangoapps/commerce/api/v0/tests/test_views.py +++ b/lms/djangoapps/commerce/api/v0/tests/test_views.py @@ -307,4 +307,4 @@ class BasketOrderViewTests(UserMixin, TestCase): """ The view should return 403 if the user is not logged in. """ self.client.logout() response = self.client.get(self.path) - assert response.status_code == 403 + assert response.status_code == 401 diff --git a/openedx/core/djangoapps/embargo/tests/test_views.py b/openedx/core/djangoapps/embargo/tests/test_views.py index fae484a32d..1c8ea72684 100644 --- a/openedx/core/djangoapps/embargo/tests/test_views.py +++ b/openedx/core/djangoapps/embargo/tests/test_views.py @@ -148,7 +148,7 @@ class CheckCourseAccessViewTest(CourseApiFactoryMixin, ModuleStoreTestCase): def test_course_access_endpoint_with_logged_out_user(self): self.client.logout() response = self.client.get(self.url, data=self.request_data) - assert response.status_code == 403 + assert response.status_code == 401 def test_course_access_endpoint_with_non_staff_user(self): user = UserFactory(is_staff=False) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 28b4c57505..fe577d7b85 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -150,12 +150,12 @@ class RoleTestCase(UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.LIST_URI)) def test_list_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=True) @override_settings(EDX_API_KEY=None) def test_debug_auth(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=False) @override_settings(EDX_API_KEY=TEST_API_KEY) @@ -164,7 +164,7 @@ class RoleTestCase(UserApiTestCase): self.assertHttpOK( self.request_with_auth("get", self.LIST_URI, **self.basic_auth("someuser", "somepass"))) - self.assertHttpForbidden( + self.assertHttpNotAuthorized( self.client.get(self.LIST_URI, **self.basic_auth("someuser", "somepass"))) def test_get_list_nonempty(self): @@ -236,12 +236,12 @@ class UserViewSetTest(UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.LIST_URI)) def test_list_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=True) @override_settings(EDX_API_KEY=None) def test_debug_auth(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=False) @override_settings(EDX_API_KEY=TEST_API_KEY) @@ -250,7 +250,7 @@ class UserViewSetTest(UserApiTestCase): self.assertHttpOK( self.request_with_auth("get", self.LIST_URI, **self.basic_auth('someuser', 'somepass'))) - self.assertHttpForbidden( + self.assertHttpNotAuthorized( self.client.get(self.LIST_URI, **self.basic_auth('someuser', 'somepass'))) def test_get_list_nonempty(self): @@ -303,7 +303,7 @@ class UserViewSetTest(UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.detail_uri)) def test_get_detail_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.detail_uri)) + self.assertHttpNotAuthorized(self.client.get(self.detail_uri)) def test_get_detail(self): user = self.users[1] @@ -342,12 +342,12 @@ class UserPreferenceViewSetTest(CacheIsolationTestCase, UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.LIST_URI)) def test_list_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=True) @override_settings(EDX_API_KEY=None) def test_debug_auth(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) def test_get_list_nonempty(self): result = self.get_json(self.LIST_URI) @@ -433,7 +433,7 @@ class UserPreferenceViewSetTest(CacheIsolationTestCase, UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.detail_uri)) def test_detail_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.detail_uri)) + self.assertHttpNotAuthorized(self.client.get(self.detail_uri)) def test_get_detail(self): pref = self.prefs[1] @@ -466,12 +466,12 @@ class PreferenceUsersListViewTest(UserApiTestCase): self.assertHttpMethodNotAllowed(self.request_with_auth("delete", self.LIST_URI)) def test_unauthorized(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) @override_settings(DEBUG=True) @override_settings(EDX_API_KEY=None) def test_debug_auth(self): - self.assertHttpForbidden(self.client.get(self.LIST_URI)) + self.assertHttpNotAuthorized(self.client.get(self.LIST_URI)) def test_get_basic(self): result = self.get_json(self.LIST_URI) @@ -583,8 +583,8 @@ class UpdateEmailOptInTestCase(UserAPITestCase, SharedModuleStoreTestCase): def test_update_email_opt_in_anonymous_user(self): """ - Test that an anonymous user gets 403 response when - updating email optin preference. + Test that an anonymous user gets 401 response when + updating email opt-in preference. """ self.client.logout() response = self.client.post(self.url, { diff --git a/openedx/core/lib/api/test_utils.py b/openedx/core/lib/api/test_utils.py index 20a26ea4a5..6a647b4854 100644 --- a/openedx/core/lib/api/test_utils.py +++ b/openedx/core/lib/api/test_utils.py @@ -64,6 +64,10 @@ class ApiTestCase(TestCase): """Assert that the given response has the status code 201""" assert response.status_code == 201 + def assertHttpNotAuthorized(self, response): + """Assert that the given response has the status code 401""" + assert response.status_code == 401 + def assertHttpForbidden(self, response): """Assert that the given response has the status code 403""" assert response.status_code == 403