From 582c157ac500ef92414083f0b3e64a0b2c5973bb Mon Sep 17 00:00:00 2001 From: German Date: Tue, 29 Aug 2023 16:50:10 -0300 Subject: [PATCH] feat: add studio write access validation https://jira.2u.com/browse/ACADEMIC-16359 --- ai_aside/config_api/api.py | 17 ++-- ai_aside/config_api/exceptions.py | 18 +++++ ai_aside/config_api/internal.py | 13 ++- ai_aside/config_api/permissions.py | 20 +++++ ai_aside/config_api/validators.py | 32 ++++++++ ai_aside/config_api/view_utils.py | 45 +++++++++++ ai_aside/config_api/views.py | 125 +++++++++-------------------- ai_aside/platform_imports.py | 7 ++ tests/api/test_api.py | 12 +-- tests/api/test_views.py | 104 ++++++++++++++++++++++-- 10 files changed, 278 insertions(+), 115 deletions(-) create mode 100644 ai_aside/config_api/exceptions.py create mode 100644 ai_aside/config_api/permissions.py create mode 100644 ai_aside/config_api/validators.py create mode 100644 ai_aside/config_api/view_utils.py diff --git a/ai_aside/config_api/api.py b/ai_aside/config_api/api.py index dda9fe7..9570266 100644 --- a/ai_aside/config_api/api.py +++ b/ai_aside/config_api/api.py @@ -1,7 +1,8 @@ """ Implements an API for updating unit and course settings. """ -from ai_aside.config_api.internal import NotFoundError, _get_course, _get_course_units, _get_unit +from ai_aside.config_api.exceptions import AiAsideNotFoundException +from ai_aside.config_api.internal import _get_course, _get_course_units, _get_unit from ai_aside.models import AIAsideCourseEnabled, AIAsideUnitEnabled from ai_aside.waffle import summaries_configuration_enabled @@ -28,7 +29,7 @@ def set_course_settings(course_key, settings): Expects: settings to be a dictionary of the form: `{'enabled': bool}` - Raises NotFoundError if the settings are not found. + Raises AiAsideNotFoundException if the settings are not found. """ enabled = settings['enabled'] @@ -47,7 +48,7 @@ def delete_course_settings(course_key): """ Deletes the settings of a course. - Raises NotFoundError if the settings are not found. + Raises AiAsideNotFoundException if the settings are not found. """ reset_course_unit_settings(course_key) record = _get_course(course_key) @@ -84,7 +85,7 @@ def set_unit_settings(course_key, unit_key, settings): Expects: settings as a dictionary of the form: `{'enabled': bool}` - Raises NotFoundError if the settings are not found. + Raises AiAsideNotFoundException if the settings are not found. """ enabled = settings['enabled'] @@ -104,7 +105,7 @@ def delete_unit_settings(course_key, unit_key): """ Deletes the settings of a unit. - Raises NotFoundError if the settings are not found. + Raises AiAsideNotFoundException if the settings are not found. """ record = _get_unit(course_key, unit_key) record.delete() @@ -127,7 +128,7 @@ def is_course_settings_present(course_key): try: course = _get_course(course_key) return course is not None - except NotFoundError: + except AiAsideNotFoundException: return False @@ -147,12 +148,12 @@ def is_summary_enabled(course_key, unit_key=None): if unit is not None: return unit.enabled - except NotFoundError: + except AiAsideNotFoundException: pass try: course = _get_course(course_key) - except NotFoundError: + except AiAsideNotFoundException: return False if course is not None: diff --git a/ai_aside/config_api/exceptions.py b/ai_aside/config_api/exceptions.py new file mode 100644 index 0000000..0dae776 --- /dev/null +++ b/ai_aside/config_api/exceptions.py @@ -0,0 +1,18 @@ +""" +Custom exceptions for ai-aside +""" +from rest_framework import status + + +class AiAsideException(Exception): + """ + A common base class for all exceptions + """ + http_status = status.HTTP_400_BAD_REQUEST + + +class AiAsideNotFoundException(AiAsideException): + """ + A 404 exception class + """ + http_status = status.HTTP_404_NOT_FOUND diff --git a/ai_aside/config_api/internal.py b/ai_aside/config_api/internal.py index 0560dad..8e3ad41 100644 --- a/ai_aside/config_api/internal.py +++ b/ai_aside/config_api/internal.py @@ -1,21 +1,18 @@ """ Internal methods for the API. """ +from ai_aside.config_api.exceptions import AiAsideNotFoundException from ai_aside.models import AIAsideCourseEnabled, AIAsideUnitEnabled -class NotFoundError(Exception): - "Raised when the course/unit is not found in the database" - - def _get_course(course_key): "Private method that gets a course based on an id" try: record = AIAsideCourseEnabled.objects.get( course_key=course_key, ) - except AIAsideCourseEnabled.DoesNotExist as exc: - raise NotFoundError from exc + except AIAsideCourseEnabled.DoesNotExist as error: + raise AiAsideNotFoundException from error return record @@ -27,8 +24,8 @@ def _get_unit(course_key, unit_key): course_key=course_key, unit_key=unit_key, ) - except AIAsideUnitEnabled.DoesNotExist as exc: - raise NotFoundError from exc + except AIAsideUnitEnabled.DoesNotExist as error: + raise AiAsideNotFoundException from error return record diff --git a/ai_aside/config_api/permissions.py b/ai_aside/config_api/permissions.py new file mode 100644 index 0000000..3fda108 --- /dev/null +++ b/ai_aside/config_api/permissions.py @@ -0,0 +1,20 @@ +""" Permissions for ai-aside API""" + +from rest_framework.permissions import BasePermission + +from ai_aside.config_api.validators import validate_course_key +from ai_aside.platform_imports import can_change_summaries_settings + + +class HasStudioWriteAccess(BasePermission): + """ + Check if the user has studio write access to a course. + """ + def has_permission(self, request, view): + """ + Check permissions for this class. + """ + course_key_string = view.kwargs.get('course_id') + course_key = validate_course_key(course_key_string) + + return can_change_summaries_settings(request.user, course_key) diff --git a/ai_aside/config_api/validators.py b/ai_aside/config_api/validators.py new file mode 100644 index 0000000..a871698 --- /dev/null +++ b/ai_aside/config_api/validators.py @@ -0,0 +1,32 @@ +""" +Utilities related to API views +""" + +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey + +from ai_aside.config_api.exceptions import AiAsideException + + +def validate_course_key(course_key_string: str) -> CourseKey: + """ + Validate and parse a course_key string, if supported. + """ + try: + course_key = CourseKey.from_string(course_key_string) + except InvalidKeyError as error: + raise AiAsideException(f"{course_key_string} is not a valid CourseKey") from error + if course_key.deprecated: + raise AiAsideException("Deprecated CourseKeys (Org/Course/Run) are not supported.") + return course_key + + +def validate_unit_key(unit_key_string: str) -> UsageKey: + """ + Validate and parse a unit_key string, if supported. + """ + try: + usage_key = UsageKey.from_string(unit_key_string) + except InvalidKeyError as error: + raise AiAsideException(f"{unit_key_string} is not a valid UsageKey") from error + return usage_key diff --git a/ai_aside/config_api/view_utils.py b/ai_aside/config_api/view_utils.py new file mode 100644 index 0000000..b39f1d2 --- /dev/null +++ b/ai_aside/config_api/view_utils.py @@ -0,0 +1,45 @@ +""" +Config API Utilities +""" +import logging + +from rest_framework import status +from rest_framework.response import Response +from rest_framework.views import APIView + +from ai_aside.config_api.exceptions import AiAsideException + +log = logging.getLogger(__name__) + + +def handle_ai_aside_exception(exc, name=None): # pylint: disable=inconsistent-return-statements + """ + Converts ai_aside exceptions into restframework responses + """ + if isinstance(exc, AiAsideException): + log.exception(name) + return APIResponse(http_status=exc.http_status, data={'message': str(exc)}) + + +class APIResponse(Response): + """API Response""" + def __init__(self, data=None, http_status=None, content_type=None, success=False): + _status = http_status or status.HTTP_200_OK + data = data or {} + reply = {'response': {'success': success}} + reply['response'].update(data) + super().__init__(data=reply, status=_status, content_type=content_type) + + +class AiAsideAPIView(APIView): + """ + Overrides APIView to handle ai_aside exceptions + """ + def handle_exception(self, exc): + """ + Converts ai-aside exceptions into standard restframework responses + """ + resp = handle_ai_aside_exception(exc, name=self.__class__.__name__) + if not resp: + resp = super().handle_exception(exc) + return resp diff --git a/ai_aside/config_api/views.py b/ai_aside/config_api/views.py index 126ddbb..70015d3 100644 --- a/ai_aside/config_api/views.py +++ b/ai_aside/config_api/views.py @@ -15,14 +15,7 @@ Both GET and DELETE methods respond with a 404 if the setting cannot be found. """ -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey -from rest_framework import status -from rest_framework.response import Response -from rest_framework.views import APIView - from ai_aside.config_api.api import ( - NotFoundError, delete_course_settings, delete_unit_settings, get_course_settings, @@ -32,52 +25,41 @@ set_course_settings, set_unit_settings, ) +from ai_aside.config_api.exceptions import AiAsideException, AiAsideNotFoundException +from ai_aside.config_api.permissions import HasStudioWriteAccess +from ai_aside.config_api.validators import validate_course_key, validate_unit_key +from ai_aside.config_api.view_utils import AiAsideAPIView, APIResponse -class APIResponse(Response): - """API Response""" - def __init__(self, data=None, http_status=None, content_type=None, success=False): - _status = http_status or status.HTTP_200_OK - data = data or {} - reply = {'response': {'success': success}} - reply['response'].update(data) - super().__init__(data=reply, status=_status, content_type=content_type) - - -class CourseSummaryConfigEnabledAPIView(APIView): +class CourseSummaryConfigEnabledAPIView(AiAsideAPIView): """ Simple GET endpoint to expose whether the course may use summary config. """ + permission_classes = (HasStudioWriteAccess,) + def get(self, request, course_id=None): """Expose whether the course may use summary config""" if course_id is None: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) + raise AiAsideNotFoundException - try: - enabled = is_summary_config_enabled(CourseKey.from_string(course_id)) - return APIResponse(success=True, data={'enabled': enabled}) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) + course_key = validate_course_key(course_id) + enabled = is_summary_config_enabled(course_key) + return APIResponse(success=True, data={'enabled': enabled}) -class CourseEnabledAPIView(APIView): +class CourseEnabledAPIView(AiAsideAPIView): """Handlers for course level settings""" + permission_classes = (HasStudioWriteAccess,) + def get(self, request, course_id=None): """Gets the enabled state for a course""" if course_id is None: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) - - try: - settings = get_course_settings(CourseKey.from_string(course_id)) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except NotFoundError: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) + raise AiAsideNotFoundException + course_key = validate_course_key(course_id) + settings = get_course_settings(course_key) return APIResponse(success=True, data=settings) def post(self, request, course_id=None): @@ -90,16 +72,12 @@ def post(self, request, course_id=None): reset = request.data.get('reset') try: - course_key = CourseKey.from_string(course_id) + course_key = validate_course_key(course_id) set_course_settings(course_key, {'enabled': enabled}) if reset: reset_course_unit_settings(course_key) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except TypeError: - data = {'message': 'Invalid parameters'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) + except TypeError as error: + raise AiAsideException('Invalid parameters') from error return APIResponse(success=True) @@ -107,38 +85,26 @@ def delete(self, request, course_id=None): """Deletes the settings for a module""" if course_id is None: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) - - try: - delete_course_settings(CourseKey.from_string(course_id)) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except NotFoundError: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) + raise AiAsideNotFoundException + course_key = validate_course_key(course_id) + delete_course_settings(course_key) return APIResponse(success=True) -class UnitEnabledAPIView(APIView): +class UnitEnabledAPIView(AiAsideAPIView): """Handlers for module level settings""" + permission_classes = (HasStudioWriteAccess,) + def get(self, request, course_id=None, unit_id=None): """Gets the enabled state for a unit""" if course_id is None or unit_id is None: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) - - try: - settings = get_unit_settings( - CourseKey.from_string(course_id), - UsageKey.from_string(unit_id), - ) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except NotFoundError: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) + raise AiAsideNotFoundException + course_key = validate_course_key(course_id) + unit_key = validate_unit_key(unit_id) + settings = get_unit_settings(course_key, unit_key) return APIResponse(success=True, data=settings) def post(self, request, course_id=None, unit_id=None): @@ -147,16 +113,11 @@ def post(self, request, course_id=None, unit_id=None): enabled = request.data.get('enabled') try: - set_unit_settings( - CourseKey.from_string(course_id), - UsageKey.from_string(unit_id), - {'enabled': enabled}) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except TypeError: - data = {'message': 'Invalid parameters'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) + course_key = validate_course_key(course_id) + unit_key = validate_unit_key(unit_id) + set_unit_settings(course_key, unit_key, {'enabled': enabled}) + except TypeError as error: + raise AiAsideException('Invalid parameters') from error return APIResponse(success=True) @@ -164,17 +125,9 @@ def delete(self, request, course_id=None, unit_id=None): """Deletes the settings for a unit""" if course_id is None or unit_id is None: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) - - try: - delete_unit_settings( - CourseKey.from_string(course_id), - UsageKey.from_string(unit_id), - ) - except InvalidKeyError: - data = {'message': 'Invalid Key'} - return APIResponse(http_status=status.HTTP_400_BAD_REQUEST, data=data) - except NotFoundError: - return APIResponse(http_status=status.HTTP_404_NOT_FOUND) + raise AiAsideNotFoundException + course_key = validate_course_key(course_id) + unit_key = validate_unit_key(unit_id) + delete_unit_settings(course_key, unit_key,) return APIResponse(success=True) diff --git a/ai_aside/platform_imports.py b/ai_aside/platform_imports.py index f346eb7..ec881fd 100644 --- a/ai_aside/platform_imports.py +++ b/ai_aside/platform_imports.py @@ -26,3 +26,10 @@ def get_block(usage_key): # pylint: disable=import-error, import-outside-toplevel from xmodule.modulestore.django import modulestore return modulestore().get_item(usage_key) + + +def can_change_summaries_settings(user, course_key): + """Check if the user can change the summaries settings by checking for studio write access.""" + # pylint: disable=import-error, import-outside-toplevel + from common.djangoapps.student.auth import has_studio_write_access + return has_studio_write_access(user, course_key) diff --git a/tests/api/test_api.py b/tests/api/test_api.py index e174ffd..6eeaa88 100644 --- a/tests/api/test_api.py +++ b/tests/api/test_api.py @@ -6,8 +6,8 @@ from django.test import TestCase from opaque_keys.edx.keys import CourseKey, UsageKey +from ai_aside.config_api.exceptions import AiAsideNotFoundException from ai_aside.config_api.api import ( - NotFoundError, delete_course_settings, delete_unit_settings, get_course_settings, @@ -83,7 +83,7 @@ def test_get_course_settings(self): self.assertTrue(settings['enabled']) def test_get_course_settings_not_found(self): - with self.assertRaises(NotFoundError): + with self.assertRaises(AiAsideNotFoundException): get_course_settings(course_keys[1]) def test_course_delete(self): @@ -110,7 +110,7 @@ def test_course_delete(self): self.assertEqual(units.count(), 0) def test_course_delete_not_found(self): - with self.assertRaises(NotFoundError): + with self.assertRaises(AiAsideNotFoundException): delete_course_settings(course_keys[1]) def test_course_delete_not_found_reset_all_units(self): @@ -124,7 +124,7 @@ def test_course_delete_not_found_reset_all_units(self): units = AIAsideUnitEnabled.objects.filter(course_key=course_key) - with self.assertRaises(NotFoundError): + with self.assertRaises(AiAsideNotFoundException): self.assertEqual(units.count(), 1) delete_course_settings(course_keys[1]) self.assertEqual(units.count(), 0) @@ -195,7 +195,7 @@ def test_get_unit_settings_not_found(self): course_key = course_keys[1] unit_key = unit_keys[1] - with self.assertRaises(NotFoundError): + with self.assertRaises(AiAsideNotFoundException): get_unit_settings(course_key, unit_key) def test_unit_delete(self): @@ -222,7 +222,7 @@ def test_unit_delete_not_found(self): course_key = course_keys[1] unit_key = unit_keys[1] - with self.assertRaises(NotFoundError): + with self.assertRaises(AiAsideNotFoundException): delete_unit_settings(course_key, unit_key) @patch('ai_aside.config_api.api.summaries_configuration_enabled') diff --git a/tests/api/test_views.py b/tests/api/test_views.py index e34905a..028a083 100644 --- a/tests/api/test_views.py +++ b/tests/api/test_views.py @@ -1,7 +1,7 @@ """ Tests for the API """ -from unittest.mock import patch +from unittest.mock import patch, Mock import ddt from django.urls import reverse @@ -20,10 +20,20 @@ 'block-v1:edX+DemoX+Demo_Course+type@vertical+block@vertical_321ac313f2de', ] +can_change_summaries_settings = Mock() + @ddt.ddt -class TestApiViews(APITestCase): - """API Endpoint View tests""" +class TestApiViewsWithPermissions(APITestCase): + """API Endpoint View tests with permissions""" + def setUp(self): + can_change_summaries_settings.return_value = True + self.access_mock = patch('ai_aside.platform_imports.can_change_summaries_settings', + can_change_summaries_settings) + self.access_mock.start() + + def tearDown(self): + self.access_mock.stop() @ddt.data(True, False) @patch('ai_aside.config_api.api.summaries_configuration_enabled') @@ -211,9 +221,9 @@ def test_unit_enabled_setter_invalid_key(self): 'unit_id': unit_id, }) response = self.client.post(api_url, {'enabled': True}, format='json') - + message = response.data['response']['message'] self.assertEqual(response.status_code, 400) - self.assertEqual(response.data['response']['message'], 'Invalid Key') + self.assertEqual(message, 'this:is:not_a-valid~key#either! is not a valid UsageKey') def test_unit_enabled_getter_valid(self): course_id = course_keys[0] @@ -244,9 +254,9 @@ def test_unit_enabled_getter_invalid_key(self): 'unit_id': unit_id, }) response = self.client.get(api_url) - + message = response.data['response']['message'] self.assertEqual(response.status_code, 400) - self.assertEqual(response.data['response']['message'], 'Invalid Key') + self.assertEqual(message, 'this:is:not_a-valid~key#either! is not a valid UsageKey') def test_unit_enabled_getter_404(self): course_id = course_keys[1] @@ -319,3 +329,83 @@ def test_course_enabled_setter_enable_valid_and_reset(self): self.client.post(api_url, {'enabled': False, 'reset': True}, format='json') self.assertEqual(units.count(), 0) + + +class TestApiViewsWithoutPermissions(APITestCase): + """API Endpoint View tests without permissions""" + def setUp(self): + can_change_summaries_settings.return_value = False + self.access_mock = patch('ai_aside.platform_imports.can_change_summaries_settings', + can_change_summaries_settings) + self.access_mock.start() + + def tearDown(self): + self.access_mock.stop() + + def test_course_configurable_403(self): + course_id = course_keys[0] + + api_url = reverse('api-course-configurable', kwargs={'course_id': course_id}) + response = self.client.get(api_url) + + self.assertEqual(response.status_code, 403) + + def test_get_course_settings_403(self): + course_id = course_keys[0] + + api_url = reverse('api-course-settings', kwargs={'course_id': course_id}) + response = self.client.get(api_url) + + self.assertEqual(response.status_code, 403) + + def test_post_course_settings_403(self): + course_id = course_keys[0] + + api_url = reverse('api-course-settings', kwargs={'course_id': course_id}) + response = self.client.post(api_url, {'enabled': 'true'}, format='json') + + self.assertEqual(response.status_code, 403) + + def test_delete_course_settings_403(self): + course_id = course_keys[0] + + api_url = reverse('api-course-settings', kwargs={'course_id': course_id}) + response = self.client.delete(api_url) + + self.assertEqual(response.status_code, 403) + + def test_get_unit_settings_403(self): + course_id = course_keys[0] + unit_id = unit_keys[0] + + api_url = reverse('api-unit-settings', kwargs={ + 'course_id': course_id, + 'unit_id': unit_id, + }) + response = self.client.get(api_url) + + self.assertEqual(response.status_code, 403) + + def test_post_unit_settings_403(self): + course_id = course_keys[0] + unit_id = unit_keys[0] + + api_url = reverse('api-unit-settings', kwargs={ + 'course_id': course_id, + 'unit_id': unit_id, + }) + response = self.client.post(api_url, {'enabled': True}, format='json') + + self.assertEqual(response.status_code, 403) + + def test_delete_unit_settings_403(self): + course_id = course_keys[0] + unit_id = unit_keys[0] + + api_url = reverse('api-unit-settings', kwargs={ + 'course_id': course_id, + 'unit_id': unit_id, + }) + response = self.client.delete(api_url) + + self.assertEqual(response.status_code, 403)