From 7690fa8912c2f6b40f1c53b8b371ae33b9c8ca24 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 15 Jan 2025 12:20:51 -0500 Subject: [PATCH 1/4] test: get view --- .../v0/tests/test_course_optimizer.py | 26 ++++++++++++++++--- .../rest_api/v0/views/course_optimizer.py | 4 ++- .../contentstore/tests/test_tasks.py | 1 - openedx/core/lib/api/view_utils.py | 1 + 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py index d8227d1670d7..9a5523a16be9 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py @@ -1,4 +1,5 @@ from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase +from common.djangoapps.student.tests.factories import InstructorFactory from rest_framework import status from django.test import TestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -13,10 +14,29 @@ def test_inherited(self): pass def make_request(self, course_id=None, data=None): - return self.client.get(self.get_url(course_id), data) + url = self.get_url(self.course.id) + print('make_request url: ', url) + response = self.client.get(url, data) + print('make_request response status code: ', response.status_code) + print('make_request response content: ', response.content) + return response def get_url(self, course_key): - return reverse( + url = reverse( 'cms.djangoapps.contentstore:v0:link_check_status', - kwargs={'course_id': 'course-v1:someOrg+someCourse+someRun'} + kwargs={'course_id': self.course.id} ) + print('get_url: ', url) + return url + + def test_course_instructor(self, expect_status=status.HTTP_200_OK): + self.course_instructor = InstructorFactory( + username='instructor', + password=self.password, + course_key=self.course.id, + ) + self.client.login(username=self.course_instructor.username, password=self.password) + response = self.make_request() + print('test_course_instructor response status code: ', response.status_code) + assert response.status_code == expect_status + return response diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py index 82e43d629a87..421fa274d33c 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py @@ -150,6 +150,7 @@ def get(self, request: Request, course_id: str): """ course_key = CourseKey.from_string(course_id) if not has_course_author_access(request.user, course_key): + print('missing course author access') self.permission_denied(request) task_status = _latest_task_status(request, course_id) @@ -190,8 +191,9 @@ def get(self, request: Request, course_id: str): **({'LinkCheckError': error} if error else {}) } + print('data', data) serializer = LinkCheckSerializer(data=data) - serializer.is_valid(raise_exception=True) + serializer.is_valid(raise_exception=False) return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/tests/test_tasks.py b/cms/djangoapps/contentstore/tests/test_tasks.py index 8d1b29a0b033..7d96a2095cf6 100644 --- a/cms/djangoapps/contentstore/tests/test_tasks.py +++ b/cms/djangoapps/contentstore/tests/test_tasks.py @@ -268,7 +268,6 @@ def test_check_broken_links_stores_broken_and_locked_urls( ### Check that _save_broken_links_file was called with the correct arguments mock_save_broken_links_file.assert_called_once_with(mock_user_task_artifact.return_value, mock.ANY) - def test_user_does_not_exist_raises_exception(self): raise NotImplementedError diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 054755ae3cc1..117585206288 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -125,6 +125,7 @@ def _decorator(func_or_class): SessionAuthenticationAllowInactiveUser ) func_or_class.permission_classes = () + print('is_authenticated', is_authenticated) if is_authenticated: func_or_class.permission_classes += (IsAuthenticated,) if is_user: From 41ff45c1e3fab98dede12069ceefad2f5ccca941 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 15 Jan 2025 15:35:59 -0500 Subject: [PATCH 2/4] test: add validation tests and document how to test views --- .../how-tos/test_course_related_view_auth.rst | 60 +++++++++++++++ .../v0/tests/test_course_optimizer.py | 77 +++++++++++++------ 2 files changed, 114 insertions(+), 23 deletions(-) create mode 100644 cms/djangoapps/contentstore/docs/how-tos/test_course_related_view_auth.rst diff --git a/cms/djangoapps/contentstore/docs/how-tos/test_course_related_view_auth.rst b/cms/djangoapps/contentstore/docs/how-tos/test_course_related_view_auth.rst new file mode 100644 index 000000000000..80b96b015242 --- /dev/null +++ b/cms/djangoapps/contentstore/docs/how-tos/test_course_related_view_auth.rst @@ -0,0 +1,60 @@ +============================================== +How to test View Auth for course-related views +============================================== + +What to test +------------ +Each view endpoint that exposes an internal API endpoint - like in files in the rest_api folder - must +be tested for the following. + +- Only authenticated users can access the endpoint. +- Only users with the correct permissions (authorization) can access the endpoint. +- All data and params that are part of the request are properly validated. + +How to test +----------- +A lot of these tests can be easily implemented by inheriting from the `AuthorizeStaffTestCase`. +This parent class assumes that the view is for a specific course and that only users who have access +to the course can access the view. (They are either staff or instructors for the course, or global admin). + +Here is an example of how to test a view that requires a user to be authenticated and have access to a course. + +.. code-block:: python + + from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase + from django.test import TestCase + from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + from django.urls import reverse + + class TestMyGetView(AuthorizeStaffTestCase, ModuleStoreTestCase, TestCase): + def make_request(self, course_id=None, data=None): + url = self.get_url(self.course.id) + response = self.client.get(url, data) + return response + + def get_url(self, course_key): + url = reverse( + 'cms.djangoapps.contentstore:v0:my_get_view', + kwargs={'course_id': self.course.id} + ) + return url + +As you can see, you need to inherit from `AuthorizeStaffTestCase` and `ModuleStoreTestCase`, and then either +`TestCase` or `APITestCase` depending on the type of view you are testing. For cookie-based +authentication, `TestCase` is sufficient, for Oauth2 use `ApiTestCase`. + +The only two methods you need to implement are `make_request` and `get_url`. The `make_request` method +should make the request to the view and return the response. The `get_url` method should return the URL +for the view you are testing. + +Overwriting Tests +----------------- +If you need different behavior you can overwrite the tests from the parent class. +For example, if students should have access to the view, simply implement the +`test_student` method in your test class. + +Adding other tests +------------------ +If you want to test other things in the view - let's say validation - +it's easy to just add another `test_...` function to your test class +and you can use the `make_request` method to make the request. diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py index 9a5523a16be9..5143e9b60ac3 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py @@ -1,24 +1,16 @@ from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase -from common.djangoapps.student.tests.factories import InstructorFactory -from rest_framework import status from django.test import TestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.urls import reverse -class TestCourseOptimizer(AuthorizeStaffTestCase, ModuleStoreTestCase, TestCase): +class TestGetLinkCheckStatus(AuthorizeStaffTestCase, ModuleStoreTestCase, TestCase): ''' - Tests for CourseOptimizer + Authentication and Authorization Tests for CourseOptimizer. + For concrete tests that are run, check `AuthorizeStaffTestCase`. ''' - def test_inherited(self): - # This method ensures that pytest recognizes this class as containing tests - pass - - def make_request(self, course_id=None, data=None): + def make_request(self, course_id=None, data=None, **kwargs): url = self.get_url(self.course.id) - print('make_request url: ', url) response = self.client.get(url, data) - print('make_request response status code: ', response.status_code) - print('make_request response content: ', response.content) return response def get_url(self, course_key): @@ -26,17 +18,56 @@ def get_url(self, course_key): 'cms.djangoapps.contentstore:v0:link_check_status', kwargs={'course_id': self.course.id} ) - print('get_url: ', url) return url + + def test_produces_4xx_when_invalid_course_id(self): + ''' + Test course_id validation + ''' + response = self.make_request(course_id='invalid_course_id') + self.assertIn(response.status_code, range(400, 500)) + + def test_produces_4xx_when_additional_kwargs(self): + ''' + Test additional kwargs validation + ''' + response = self.make_request(course_id=self.course.id, malicious_kwarg='malicious_kwarg') + self.assertIn(response.status_code, range(400, 500)) - def test_course_instructor(self, expect_status=status.HTTP_200_OK): - self.course_instructor = InstructorFactory( - username='instructor', - password=self.password, - course_key=self.course.id, - ) - self.client.login(username=self.course_instructor.username, password=self.password) - response = self.make_request() - print('test_course_instructor response status code: ', response.status_code) - assert response.status_code == expect_status +class TestPostLinkCheck(AuthorizeStaffTestCase, ModuleStoreTestCase, TestCase): + ''' + Authentication and Authorization Tests for CourseOptimizer. + For concrete tests that are run, check `AuthorizeStaffTestCase`. + ''' + def make_request(self, course_id=None, data=None, **kwargs): + url = self.get_url(self.course.id) + response = self.client.post(url, data) return response + + def get_url(self, course_key): + url = reverse( + 'cms.djangoapps.contentstore:v0:link_check', + kwargs={'course_id': self.course.id} + ) + return url + + def test_produces_4xx_when_invalid_course_id(self): + ''' + Test course_id validation + ''' + response = self.make_request(course_id='invalid_course_id') + self.assertIn(response.status_code, range(400, 500)) + + def test_produces_4xx_when_additional_kwargs(self): + ''' + Test additional kwargs validation + ''' + response = self.make_request(course_id=self.course.id, malicious_kwarg='malicious_kwarg') + self.assertIn(response.status_code, range(400, 500)) + + def test_produces_4xx_when_unexpected_data(self): + ''' + Test validation when request contains unexpected data + ''' + response = self.make_request(course_id=self.course.id, data={'unexpected_data': 'unexpected_data'}) + self.assertIn(response.status_code, range(400, 500)) From 37d8be61e3d3160fa2742e505bf7e8d26fc9ecce Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 15 Jan 2025 15:37:11 -0500 Subject: [PATCH 3/4] fix: lint --- .../rest_api/v0/tests/test_course_optimizer.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py index 5143e9b60ac3..8185c1a628e3 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_course_optimizer.py @@ -1,8 +1,9 @@ -from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase from django.test import TestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.urls import reverse +from cms.djangoapps.contentstore.tests.test_utils import AuthorizeStaffTestCase + class TestGetLinkCheckStatus(AuthorizeStaffTestCase, ModuleStoreTestCase, TestCase): ''' Authentication and Authorization Tests for CourseOptimizer. @@ -19,14 +20,14 @@ def get_url(self, course_key): kwargs={'course_id': self.course.id} ) return url - + def test_produces_4xx_when_invalid_course_id(self): ''' Test course_id validation ''' response = self.make_request(course_id='invalid_course_id') self.assertIn(response.status_code, range(400, 500)) - + def test_produces_4xx_when_additional_kwargs(self): ''' Test additional kwargs validation From 91c0ba57bc1c8f9c70d0ba324dfb4181cec02652 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 16 Jan 2025 18:26:17 -0500 Subject: [PATCH 4/4] fix: serialization --- .../contentstore/rest_api/v0/views/course_optimizer.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py index 421fa274d33c..9d6f8df3a4fc 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/course_optimizer.py @@ -190,10 +190,7 @@ def get(self, request: Request, course_id: str): **({'LinkCheckOutput': broken_links_dto} if broken_links_dto else {}), **({'LinkCheckError': error} if error else {}) } - - print('data', data) - serializer = LinkCheckSerializer(data=data) - serializer.is_valid(raise_exception=False) + serializer = LinkCheckSerializer(data) return Response(serializer.data)