From 065f894d1b797476b78bc3762d4230140aead828 Mon Sep 17 00:00:00 2001 From: 0x29a Date: Wed, 19 Apr 2023 15:29:06 +0200 Subject: [PATCH] fix: missing advance_settings_access template variable Co-authored-by: Farhaan Bukhsh --- .../tests/test_course_settings.py | 55 ++++++++++++++++--- cms/djangoapps/contentstore/views/course.py | 21 +++---- cms/templates/settings.html | 3 + cms/templates/settings_graders.html | 3 + cms/templates/widgets/header.html | 3 +- common/djangoapps/student/auth.py | 16 +++++- 6 files changed, 77 insertions(+), 24 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index 6a1977f375..605e0d2133 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -106,7 +106,12 @@ class CourseAdvanceSettingViewTest(CourseTestCase, MilestonesTestCaseMixin): super().setUp() self.fullcourse = CourseFactory.create() self.course_setting_url = get_url(self.course.id, 'advanced_settings_handler') - self.non_staff_client, _ = self.create_non_staff_authed_user_client() + + self.non_staff_client, self.nonstaff = self.create_non_staff_authed_user_client() + # "nonstaff" means "non Django staff" here. We assign this user to course staff + # role to check that even so they won't have advanced settings access when explicitly + # restricted. + CourseStaffRole(self.course.id).add_users(self.nonstaff) @override_settings(FEATURES={'DISABLE_MOBILE_COURSE_AVAILABLE': True}) def test_mobile_field_available(self): @@ -145,16 +150,50 @@ class CourseAdvanceSettingViewTest(CourseTestCase, MilestonesTestCaseMixin): self.assertEqual('discussion_blackouts' in response, fields_visible) self.assertEqual('discussion_topics' in response, fields_visible) - @override_settings(FEATURES={'DISABLE_ADVANCED_SETTINGS': True}) - def test_disable_advanced_settings_feature(self): + @ddt.data(False, True) + def test_disable_advanced_settings_feature(self, disable_advanced_settings): """ - If this feature is enabled, only staff should be able to access the advanced settings page. + If this feature is enabled, only Django Staff/Superuser should be able to access the "Advanced Settings" page. + For non-staff users the "Advanced Settings" tab link should not be visible. """ - response = self.non_staff_client.get_html(self.course_setting_url) - self.assertEqual(response.status_code, 403) + advanced_settings_link_html = f"Advanced Settings".encode('utf-8') - response = self.client.get_html(self.course_setting_url) - self.assertEqual(response.status_code, 200) + with override_settings(FEATURES={'DISABLE_ADVANCED_SETTINGS': disable_advanced_settings}): + for handler in ( + 'import_handler', + 'export_handler', + 'course_team_handler', + 'course_info_handler', + 'assets_handler', + 'tabs_handler', + 'settings_handler', + 'grading_handler', + 'textbooks_list_handler', + ): + # Test that non-staff users don't see the "Advanced Settings" tab link. + response = self.non_staff_client.get_html( + get_url(self.course.id, handler) + ) + self.assertEqual(response.status_code, 200) + if disable_advanced_settings: + self.assertNotIn(advanced_settings_link_html, response.content) + else: + self.assertIn(advanced_settings_link_html, response.content) + + # Test that staff users see the "Advanced Settings" tab link. + response = self.client.get_html( + get_url(self.course.id, handler) + ) + self.assertEqual(response.status_code, 200) + self.assertIn(advanced_settings_link_html, response.content) + + # Test that non-staff users can't access the "Advanced Settings" page. + response = self.non_staff_client.get_html(self.course_setting_url) + self.assertEqual(response.status_code, 403 if disable_advanced_settings else 200) + + # Test that staff users can access the "Advanced Settings" page. + response = self.client.get_html(self.course_setting_url) + self.assertEqual(response.status_code, 200) @ddt.ddt diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 0cc4a5281c..e15904d640 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -43,7 +43,12 @@ from common.djangoapps.course_action_state.managers import CourseActionStateItem from common.djangoapps.course_action_state.models import CourseRerunState, CourseRerunUIStateManager from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.edxmako.shortcuts import render_to_response -from common.djangoapps.student.auth import has_course_author_access, has_studio_read_access, has_studio_write_access +from common.djangoapps.student.auth import ( + has_course_author_access, + has_studio_read_access, + has_studio_write_access, + has_studio_advanced_settings_access +) from common.djangoapps.student.roles import ( CourseInstructorRole, CourseStaffRole, @@ -149,17 +154,6 @@ class AccessListFallback(Exception): pass # lint-amnesty, pylint: disable=unnecessary-pass -def has_advanced_settings_access(user): - """ - If DISABLE_ADVANCED_SETTINGS feature is enabled, only global staff can access "Advanced Settings". - """ - return ( - not settings.FEATURES.get('DISABLE_ADVANCED_SETTINGS', False) - or user.is_staff - or user.is_superuser - ) - - def get_course_and_check_access(course_key, user, depth=0): """ Function used to calculate and return the locator and course block @@ -765,7 +759,6 @@ def course_index(request, course_key): 'frontend_app_publisher_url': frontend_app_publisher_url, 'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_block.id), 'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id), - 'advance_settings_access': has_advanced_settings_access(request.user), 'proctoring_errors': proctoring_errors, }) @@ -1437,7 +1430,7 @@ def advanced_settings_handler(request, course_key_string): json: update the Course's settings. The payload is a json rep of the metadata dicts. """ - if not has_advanced_settings_access(request.user): + if not has_studio_advanced_settings_access(request.user): raise PermissionDenied() course_key = CourseKey.from_string(course_key_string) diff --git a/cms/templates/settings.html b/cms/templates/settings.html index c3ed1afb99..b7ef368a09 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -7,6 +7,7 @@ <%namespace name='static' file='static_content.html'/> <%! from django.utils.translation import gettext as _ + from common.djangoapps.student.auth import has_studio_advanced_settings_access from cms.djangoapps.contentstore import utils from lms.djangoapps.certificates.api import can_show_certificate_available_date_field from openedx.core.djangolib.js_utils import ( @@ -721,7 +722,9 @@ CMS.URL.UPLOAD_ASSET = '${upload_asset_url | n, js_escaped_string}'
  • ${_("Grading")}
  • ${_("Course Team")}
  • ${_("Group Configurations")}
    • + % if has_studio_advanced_settings_access(request.user):
  • ${_("Advanced Settings")}
    • + % endif % if mfe_proctored_exam_settings_url:
  • ${_("Proctored Exam Settings")}
    • % endif diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index a923c4875d..0bd6483f44 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -11,6 +11,7 @@ import json from cms.djangoapps.contentstore import utils from django.utils.translation import gettext as _ + from common.djangoapps.student.auth import has_studio_advanced_settings_access from cms.djangoapps.models.settings.encoder import CourseSettingsEncoder from openedx.core.djangolib.js_utils import ( dump_js_escaped_json, js_escaped_string @@ -168,7 +169,9 @@
  • ${_("Details & Schedule")}
  • ${_("Course Team")}
  • ${_("Group Configurations")}
    • + % if has_studio_advanced_settings_access(request.user):
  • ${_("Advanced Settings")}
    • + % endif % if mfe_proctored_exam_settings_url:
  • ${_("Proctored Exam Settings")}
    • % endif diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 23678ec395..48b818da95 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -7,6 +7,7 @@ from django.urls import reverse from django.utils.translation import gettext as _ from urllib.parse import quote_plus + from common.djangoapps.student.auth import has_studio_advanced_settings_access from cms.djangoapps.contentstore import toggles from cms.djangoapps.contentstore.utils import get_pages_and_resources_url from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND @@ -120,7 +121,7 @@ ${_("Proctored Exam Settings")} % endif - % if advance_settings_access: + % if has_studio_advanced_settings_access(request.user):
  • ${_("Advanced Settings")}
    • diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 3091dbc454..f8e45ce6ba 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -125,9 +125,23 @@ def has_course_author_access(user, course_key): return has_studio_write_access(user, course_key) +def has_studio_advanced_settings_access(user): + """ + If DISABLE_ADVANCED_SETTINGS feature is enabled, only Django Superuser + or Django Staff can access "Advanced Settings". + + By default, this feature is disabled. + """ + return ( + not settings.FEATURES.get('DISABLE_ADVANCED_SETTINGS', False) + or user.is_staff + or user.is_superuser + ) + + def has_studio_read_access(user, course_key): """ - Return True iff user is allowed to view this course/library in studio. + Return True if user is allowed to view this course/library in studio. Will also return True if user has write access in studio (has_course_author_access) There is currently no such thing as read-only course access in studio, but