From e98540778a1804a7ba07c908d874e41a5b234288 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Fri, 10 Sep 2021 22:13:26 +0530 Subject: [PATCH 1/2] feat: grant course/library creation rights by organization (#26616) Current State (before this commit): Studio, as of today doesn't have a way to restrict a user to create a course in a particular organization. What Studio provides right now is a CourseCreator permission which gives an Admin the power to grant a user the permission to create a course. For example: If the Admin has given a user Spiderman the permission to create courses, Spiderman can now create courses in any organization i.e Marvel as well as DC. There is no way to restrict Spiderman from creating courses under DC. Purpose of this commit: The changes done here gives Admin the ability to restrict a user on an Organization level from creating courses via the Course Creators section of the Studio Django administration panel. For example: Now, the Admin can give the user Spiderman the privilege of creating courses only under Marvel organization. The moment Spiderman tries to create a course under some other organization(i.e DC), Studio will show an error message. This change is available to all Studio instances that enable the FEATURES['ENABLE_CREATOR_GROUP'] flag. Regardless of the flag, it will not affect any instances that choose not to use it. BB-3622 --- .../tests/test_course_create_rerun.py | 155 +++++++++++++++++- cms/djangoapps/contentstore/views/course.py | 28 +++- cms/djangoapps/contentstore/views/helpers.py | 18 +- cms/djangoapps/contentstore/views/library.py | 37 ++++- .../contentstore/views/tests/test_library.py | 14 +- cms/djangoapps/course_creators/admin.py | 79 ++++++++- ...002_add_org_support_for_course_creators.py | 24 +++ cms/djangoapps/course_creators/models.py | 19 ++- .../course_creators/tests/test_views.py | 12 +- cms/djangoapps/course_creators/views.py | 12 +- common/djangoapps/student/auth.py | 28 +++- common/djangoapps/student/roles.py | 16 ++ common/djangoapps/student/tests/test_authz.py | 51 +++++- common/djangoapps/student/tests/test_roles.py | 16 +- 14 files changed, 460 insertions(+), 49 deletions(-) create mode 100644 cms/djangoapps/course_creators/migrations/0002_add_org_support_for_course_creators.py diff --git a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py index 70232c47e981..c76837912daa 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -4,24 +4,36 @@ import datetime +from unittest import mock import ddt +from django.contrib.admin.sites import AdminSite +from django.http import HttpRequest from django.test import override_settings from django.test.client import RequestFactory from django.urls import reverse from opaque_keys.edx.keys import CourseKey from organizations.api import add_organization, get_course_organizations, get_organization_by_short_name from organizations.exceptions import InvalidOrganizationException - -from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json -from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole -from common.djangoapps.student.tests.factories import UserFactory +from organizations.models import Organization from xmodule.course_module import CourseFields from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json +from cms.djangoapps.course_creators.admin import CourseCreatorAdmin +from cms.djangoapps.course_creators.models import CourseCreator +from common.djangoapps.student.auth import update_org_role +from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, OrgContentCreatorRole +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory + + +def mock_render_to_string(template_name, context): + """Return a string that encodes template_name and context""" + return str((template_name, context)) + @ddt.ddt class TestCourseListing(ModuleStoreTestCase): @@ -37,6 +49,7 @@ def setUp(self): # create and log in a non-staff user self.user = UserFactory() self.factory = RequestFactory() + self.global_admin = AdminFactory() self.client = AjaxEnabledTestClient() self.client.login(username=self.user.username, password='test') self.course_create_rerun_url = reverse('course_handler') @@ -56,6 +69,12 @@ def setUp(self): ) self.source_course_key = source_course.id + self.course_creator_entry = CourseCreator(user=self.user) + self.course_creator_entry.save() + self.request = HttpRequest() + self.request.user = self.global_admin + self.creator_admin = CourseCreatorAdmin(self.course_creator_entry, AdminSite()) + for role in [CourseInstructorRole, CourseStaffRole]: role(self.source_course_key).add_users(self.user) @@ -180,3 +199,131 @@ def test_course_creation_for_known_organization(self, organizations_autocreate): course_orgs = get_course_organizations(new_course_key) self.assertEqual(len(course_orgs), 1) self.assertEqual(course_orgs[0]['short_name'], 'orgX') + + @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_course_creation_when_user_not_in_org(self, store): + """ + Tests course creation when user doesn't have the required role. + """ + with modulestore().default_store(store): + response = self.client.ajax_post(self.course_create_rerun_url, { + 'org': 'TestorgX', + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2021_T1' + }) + self.assertEqual(response.status_code, 403) + + @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_course_creation_when_user_in_org_with_creator_role(self, store): + """ + Tests course creation with user having the organization content creation role. + """ + add_organization({ + 'name': 'Test Organization', + 'short_name': self.source_course_key.org, + 'description': 'Testing Organization Description', + }) + update_org_role(self.global_admin, OrgContentCreatorRole, self.user, [self.source_course_key.org]) + with modulestore().default_store(store): + response = self.client.ajax_post(self.course_create_rerun_url, { + 'org': self.source_course_key.org, + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2021_T1' + }) + self.assertEqual(response.status_code, 200) + + @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + @mock.patch( + 'cms.djangoapps.course_creators.admin.render_to_string', + mock.Mock(side_effect=mock_render_to_string, autospec=True) + ) + def test_course_creation_with_all_org_checked(self, store): + """ + Tests course creation with user having permission to create course for all organization. + """ + add_organization({ + 'name': 'Test Organization', + 'short_name': self.source_course_key.org, + 'description': 'Testing Organization Description', + }) + self.course_creator_entry.all_organizations = True + self.course_creator_entry.state = CourseCreator.GRANTED + self.creator_admin.save_model(self.request, self.course_creator_entry, None, True) + with modulestore().default_store(store): + response = self.client.ajax_post(self.course_create_rerun_url, { + 'org': self.source_course_key.org, + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2021_T1' + }) + self.assertEqual(response.status_code, 200) + + @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + @mock.patch( + 'cms.djangoapps.course_creators.admin.render_to_string', + mock.Mock(side_effect=mock_render_to_string, autospec=True) + ) + def test_course_creation_with_permission_for_specific_organization(self, store): + """ + Tests course creation with user having permission to create course for specific organization. + """ + add_organization({ + 'name': 'Test Organization', + 'short_name': self.source_course_key.org, + 'description': 'Testing Organization Description', + }) + self.course_creator_entry.all_organizations = False + self.course_creator_entry.state = CourseCreator.GRANTED + self.creator_admin.save_model(self.request, self.course_creator_entry, None, True) + dc_org_object = Organization.objects.get(name='Test Organization') + self.course_creator_entry.organizations.add(dc_org_object) + with modulestore().default_store(store): + response = self.client.ajax_post(self.course_create_rerun_url, { + 'org': self.source_course_key.org, + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2021_T1' + }) + self.assertEqual(response.status_code, 200) + + @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + @mock.patch( + 'cms.djangoapps.course_creators.admin.render_to_string', + mock.Mock(side_effect=mock_render_to_string, autospec=True) + ) + def test_course_creation_without_permission_for_specific_organization(self, store): + """ + Tests course creation with user not having permission to create course for specific organization. + """ + add_organization({ + 'name': 'Test Organization', + 'short_name': self.source_course_key.org, + 'description': 'Testing Organization Description', + }) + add_organization({ + 'name': 'DC', + 'short_name': 'DC', + 'description': 'DC Comics', + }) + self.course_creator_entry.all_organizations = False + self.course_creator_entry.state = CourseCreator.GRANTED + self.creator_admin.save_model(self.request, self.course_creator_entry, None, True) + # User has been given the permission to create course under `DC` organization. + # When the user tries to create course under `Test Organization` it throws a 403. + dc_org_object = Organization.objects.get(name='DC') + self.course_creator_entry.organizations.add(dc_org_object) + with modulestore().default_store(store): + response = self.client.ajax_post(self.course_create_rerun_url, { + 'org': self.source_course_key.org, + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2021_T1' + }) + self.assertEqual(response.status_code, 403) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 57da0c543507..64328845bfd5 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -40,10 +40,8 @@ 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 import auth from common.djangoapps.student.auth import has_course_author_access, has_studio_read_access, has_studio_write_access from common.djangoapps.student.roles import ( - CourseCreatorRole, CourseInstructorRole, CourseStaffRole, GlobalStaff, @@ -105,12 +103,13 @@ reverse_usage_url ) from .component import ADVANCED_COMPONENT_TYPES +from .helpers import is_content_creator from .entrance_exam import create_entrance_exam, delete_entrance_exam, update_entrance_exam from .item import create_xblock_info from .library import ( LIBRARIES_ENABLED, LIBRARY_AUTHORING_MICROFRONTEND_URL, - get_library_creator_status, + user_can_create_library, should_redirect_to_library_authoring_mfe ) @@ -560,7 +559,7 @@ def format_in_process_course_view(uca): 'redirect_to_library_authoring_mfe': should_redirect_to_library_authoring_mfe(), 'library_authoring_mfe_url': LIBRARY_AUTHORING_MICROFRONTEND_URL, 'libraries': [_format_library_for_view(lib, request) for lib in libraries], - 'show_new_library_button': get_library_creator_status(user) and not should_redirect_to_library_authoring_mfe(), + 'show_new_library_button': user_can_create_library(user) and not should_redirect_to_library_authoring_mfe(), 'user': user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(user), @@ -849,9 +848,6 @@ def _create_or_rerun_course(request): Returns the destination course_key and overriding fields for the new course. Raises DuplicateCourseError and InvalidKeyError """ - if not auth.user_has_role(request.user, CourseCreatorRole()): - raise PermissionDenied() - try: org = request.json.get('org') course = request.json.get('number', request.json.get('course')) @@ -859,6 +855,10 @@ def _create_or_rerun_course(request): # force the start date for reruns and allow us to override start via the client start = request.json.get('start', CourseFields.start.default) run = request.json.get('run') + has_course_creator_role = is_content_creator(request.user, org) + + if not has_course_creator_role: + raise PermissionDenied() # allow/disable unicode characters in course_id according to settings if not settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID'): @@ -915,6 +915,20 @@ def _create_or_rerun_course(request): return JsonResponse({ "ErrMsg": _("Unable to create course '{name}'.\n\n{err}").format(name=display_name, err=str(error))} ) + except PermissionDenied as error: + log.info( + "User does not have the permission to create course in this organization" + "or course creation is disabled." + "User: '%s' Org: '%s' Course #: '%s'.", + request.user.id, + org, + course, + ) + return JsonResponse({ + 'error': _('User does not have the permission to create courses in this organization ' + 'or course creation is disabled')}, + status=403 + ) def create_new_course(user, org, number, run, fields): diff --git a/cms/djangoapps/contentstore/views/helpers.py b/cms/djangoapps/contentstore/views/helpers.py index 4ed4ba141495..6926b45f932a 100644 --- a/cms/djangoapps/contentstore/views/helpers.py +++ b/cms/djangoapps/contentstore/views/helpers.py @@ -10,12 +10,14 @@ from django.utils.translation import ugettext as _ from opaque_keys.edx.keys import UsageKey from xblock.core import XBlock +from xmodule.modulestore.django import modulestore +from xmodule.tabs import StaticTab from cms.djangoapps.models.settings.course_grading import CourseGradingModel from common.djangoapps.edxmako.shortcuts import render_to_string +from common.djangoapps.student import auth +from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole from openedx.core.toggles import ENTRANCE_EXAMS -from xmodule.modulestore.django import modulestore -from xmodule.tabs import StaticTab from ..utils import reverse_course_url, reverse_library_url, reverse_usage_url @@ -290,3 +292,15 @@ def is_item_in_course_tree(item): ancestor = ancestor.get_parent() return ancestor is not None + + +def is_content_creator(user, org): + """ + Check if the user has the role to create content. + + This function checks if the User has role to create content + or if the org is supplied, it checks for Org level course content + creator. + """ + return (auth.user_has_role(user, CourseCreatorRole()) or + auth.user_has_role(user, OrgContentCreatorRole(org=org))) diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 5a85555cdb69..b065e3e4cd89 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -19,6 +19,9 @@ from opaque_keys.edx.locator import LibraryLocator, LibraryUsageLocator from organizations.api import ensure_organization from organizations.exceptions import InvalidOrganizationException +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import DuplicateCourseError from cms.djangoapps.course_creators.views import get_course_creator_status from common.djangoapps.edxmako.shortcuts import render_to_response @@ -29,15 +32,17 @@ has_studio_read_access, has_studio_write_access ) -from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseStaffRole, + LibraryUserRole +) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest, expect_json -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import DuplicateCourseError from ..config.waffle import REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND from ..utils import add_instructor, reverse_library_url from .component import CONTAINER_TEMPLATES, get_component_templates +from .helpers import is_content_creator from .item import create_xblock_info from .user import user_with_role @@ -63,7 +68,7 @@ def should_redirect_to_library_authoring_mfe(): ) -def get_library_creator_status(user): +def user_can_create_library(user, org=None): """ Helper method for returning the library creation status for a particular user, taking into account the value LIBRARIES_ENABLED. @@ -74,7 +79,10 @@ def get_library_creator_status(user): elif user.is_staff: return True elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): - return get_course_creator_status(user) == 'granted' + has_course_creator_role = True + if org: + has_course_creator_role = is_content_creator(user, org) + return get_course_creator_status(user) == 'granted' and has_course_creator_role else: # EDUCATOR-1924: DISABLE_LIBRARY_CREATION overrides DISABLE_COURSE_CREATION, if present. disable_library_creation = settings.FEATURES.get('DISABLE_LIBRARY_CREATION', None) @@ -97,7 +105,7 @@ def library_handler(request, library_key_string=None): raise Http404 # Should never happen because we test the feature in urls.py also if request.method == 'POST': - if not get_library_creator_status(request.user): + if not user_can_create_library(request.user): return HttpResponseForbidden() if library_key_string is not None: @@ -188,6 +196,8 @@ def _create_library(request): library = request.json.get('number', None) if library is None: library = request.json['library'] + if not user_can_create_library(request.user, org): + raise PermissionDenied() store = modulestore() with store.default_store(ModuleStoreEnum.Type.split): new_lib = store.create_library( @@ -198,6 +208,19 @@ def _create_library(request): ) # Give the user admin ("Instructor") role for this library: add_instructor(new_lib.location.library_key, request.user, request.user) + except PermissionDenied as error: + log.info( + "User does not have the permission to create LIBRARY in this organization." + "User: '%s' Org: '%s' LIBRARY #: '%s'.", + request.user.id, + org, + library, + ) + return JsonResponse({ + 'error': _('User does not have the permission to create library in this organization ' + 'or course creation is disabled')}, + status=403 + ) except KeyError as error: log.exception("Unable to create library - missing required JSON key.") return JsonResponseBadRequest({ diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index e62c56509592..0addf47194db 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -23,7 +23,7 @@ from xmodule.modulestore.tests.factories import LibraryFactory from ..component import get_component_templates -from ..library import get_library_creator_status +from ..library import user_can_create_library LIBRARY_REST_URL = '/library/' # URL for GET/POST requests involving libraries @@ -54,23 +54,23 @@ def setUp(self): @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", False) def test_library_creator_status_libraries_not_enabled(self): _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(get_library_creator_status(nostaff_user), False) + self.assertEqual(user_can_create_library(nostaff_user), False) @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_is_staff_user(self): - self.assertEqual(get_library_creator_status(self.user), True) + self.assertEqual(user_can_create_library(self.user), True) @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_course_creator_role(self): _, nostaff_user = self.create_non_staff_authed_user_client() with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): grant_course_creator_status(self.user, nostaff_user) - self.assertEqual(get_library_creator_status(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user), True) @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_no_course_creator_role(self): _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(get_library_creator_status(nostaff_user), True) + self.assertEqual(user_can_create_library(nostaff_user), True) @ddt.data( (False, False, True), @@ -94,7 +94,7 @@ def test_library_creator_status_settings(self, disable_course, disable_library, "DISABLE_LIBRARY_CREATION": disable_library } ): - self.assertEqual(get_library_creator_status(nostaff_user), expected_status) + self.assertEqual(user_can_create_library(nostaff_user), expected_status) @mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': True}) @mock.patch("cms.djangoapps.contentstore.views.library.LIBRARIES_ENABLED", True) @@ -103,7 +103,7 @@ def test_library_creator_status_with_no_course_creator_role_and_disabled_nonstaf Ensure that `DISABLE_COURSE_CREATION` feature works with libraries as well. """ nostaff_client, nostaff_user = self.create_non_staff_authed_user_client() - self.assertFalse(get_library_creator_status(nostaff_user)) + self.assertFalse(user_can_create_library(nostaff_user)) # To be explicit, this user can GET, but not POST get_response = nostaff_client.get_json(LIBRARY_REST_URL) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 90683660a661..45ff82ec49cc 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -6,9 +6,12 @@ import logging from smtplib import SMTPException +from django import forms from django.conf import settings from django.contrib import admin +from django.core.exceptions import ValidationError from django.core.mail import send_mail +from django.db.models.signals import m2m_changed from django.dispatch import receiver from cms.djangoapps.course_creators.models import ( @@ -17,7 +20,7 @@ send_user_notification, update_creator_state ) -from cms.djangoapps.course_creators.views import update_course_creator_group +from cms.djangoapps.course_creators.views import update_course_creator_group, update_org_content_creator_role from common.djangoapps.edxmako.shortcuts import render_to_string log = logging.getLogger("studio.coursecreatoradmin") @@ -30,26 +33,59 @@ def get_email(obj): get_email.short_description = 'email' +class CourseCreatorForm(forms.ModelForm): + """ + Admin form for course creator + """ + class Meta: + model = CourseCreator + fields = '__all__' + + def clean(self): + """ + Validate the 'state', 'organizations' and 'all_orgs' field before saving. + """ + all_orgs = self.cleaned_data.get("all_organizations") + orgs = self.cleaned_data.get("organizations").exists() + state = self.cleaned_data.get("state") + is_all_org_selected_with_orgs = (orgs and all_orgs) + is_orgs_added_with_all_orgs_selected = (not orgs and not all_orgs) + is_state_granted = state == CourseCreator.GRANTED + if is_state_granted: + if is_all_org_selected_with_orgs: + raise ValidationError( + "The role can be granted either to ALL organizations or to " + "specific organizations but not both." + ) + if is_orgs_added_with_all_orgs_selected: + raise ValidationError( + "Specific organizations needs to be selected to grant this role," + "if it is not granted to all organiztions" + ) + + class CourseCreatorAdmin(admin.ModelAdmin): """ Admin for the course creator table. """ # Fields to display on the overview page. - list_display = ['username', get_email, 'state', 'state_changed', 'note'] + list_display = ['username', get_email, 'state', 'state_changed', 'note', 'all_organizations'] + filter_horizontal = ('organizations',) readonly_fields = ['username', 'state_changed'] # Controls the order on the edit form (without this, read-only fields appear at the end). fieldsets = ( (None, { - 'fields': ['username', 'state', 'state_changed', 'note'] + 'fields': ['username', 'state', 'state_changed', 'note', 'all_organizations', 'organizations'] }), ) # Fields that filtering support list_filter = ['state', 'state_changed'] # Fields that search supports. - search_fields = ['user__username', 'user__email', 'state', 'note'] + search_fields = ['user__username', 'user__email', 'state', 'note', 'organizations'] # Turn off the action bar (we have no bulk actions) actions = None + form = CourseCreatorForm def username(self, inst): """ @@ -75,22 +111,31 @@ def save_model(self, request, obj, form, change): obj.admin = request.user obj.save() + # This functions is overriden to update the m2m query + def save_related(self, request, form, formsets, change): + super().save_related(request, form, formsets, change) + state = form.instance.state + if state != CourseCreator.GRANTED: + form.instance.organizations.clear() + admin.site.register(CourseCreator, CourseCreatorAdmin) @receiver(update_creator_state, sender=CourseCreator) -def update_creator_group_callback(sender, **kwargs): # lint-amnesty, pylint: disable=unused-argument +def update_creator_group_callback(sender, **kwargs): # pylint: disable=unused-argument """ Callback for when the model's creator status has changed. """ user = kwargs['user'] updated_state = kwargs['state'] - update_course_creator_group(kwargs['caller'], user, updated_state == CourseCreator.GRANTED) + all_orgs = kwargs['all_organizations'] + create_role = all_orgs and (updated_state == CourseCreator.GRANTED) + update_course_creator_group(kwargs['caller'], user, create_role) @receiver(send_user_notification, sender=CourseCreator) -def send_user_notification_callback(sender, **kwargs): # lint-amnesty, pylint: disable=unused-argument +def send_user_notification_callback(sender, **kwargs): # pylint: disable=unused-argument """ Callback for notifying user about course creator status change. """ @@ -118,7 +163,7 @@ def send_user_notification_callback(sender, **kwargs): # lint-amnesty, pylint: @receiver(send_admin_notification, sender=CourseCreator) -def send_admin_notification_callback(sender, **kwargs): # lint-amnesty, pylint: disable=unused-argument +def send_admin_notification_callback(sender, **kwargs): # pylint: disable=unused-argument """ Callback for notifying admin of a user in the 'pending' state. """ @@ -141,3 +186,21 @@ def send_admin_notification_callback(sender, **kwargs): # lint-amnesty, pylint: ) except SMTPException: log.warning("Failure sending 'pending state' e-mail for %s to %s", user.email, studio_request_email) + + +@receiver(m2m_changed, sender=CourseCreator.organizations.through) +def course_creator_organizations_changed_callback(sender, **kwargs): # pylint: disable=unused-argument + """ + Callback for addition and removal of orgs field. + """ + instance = kwargs["instance"] + action = kwargs["action"] + orgs = list(instance.organizations.all().values_list('short_name', flat=True)) + updated_state = instance.state + is_granted = updated_state == CourseCreator.GRANTED + should_update_role = ( + (action in ["post_add", "post_remove"] and is_granted) or + (action == "post_clear" and not is_granted) + ) + if should_update_role: + update_org_content_creator_role(instance.admin, instance.user, orgs) diff --git a/cms/djangoapps/course_creators/migrations/0002_add_org_support_for_course_creators.py b/cms/djangoapps/course_creators/migrations/0002_add_org_support_for_course_creators.py new file mode 100644 index 000000000000..171e7ee53178 --- /dev/null +++ b/cms/djangoapps/course_creators/migrations/0002_add_org_support_for_course_creators.py @@ -0,0 +1,24 @@ +# Generated by Django 2.2.24 on 2021-06-21 09:50 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('organizations', '0003_historicalorganizationcourse'), + ('course_creators', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='coursecreator', + name='all_organizations', + field=models.BooleanField(default=True, help_text='Grant the user the permission to create courses in ALL organizations'), + ), + migrations.AddField( + model_name='coursecreator', + name='organizations', + field=models.ManyToManyField(blank=True, help_text='Organizations under which the user is allowed to create courses.', to='organizations.Organization'), + ), + ] diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 40f20db016ab..e827f34155e1 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -2,7 +2,6 @@ Table for storing information about whether or not Studio users have course creation privileges. """ - from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.db import models from django.db.models.signals import post_init, post_save @@ -10,9 +9,10 @@ from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ +from organizations.models import Organization # A signal that will be sent when users should be added or removed from the creator group -update_creator_state = Signal(providing_args=["caller", "user", "state"]) +update_creator_state = Signal(providing_args=["caller", "user", "state", "organizations"]) # A signal that will be sent when admin should be notified of a pending user request send_admin_notification = Signal(providing_args=["user"]) @@ -48,6 +48,12 @@ class CourseCreator(models.Model): help_text=_("Current course creator state")) note = models.CharField(max_length=512, blank=True, help_text=_("Optional notes about this user (for example, " "why course creation access was denied)")) + organizations = models.ManyToManyField(Organization, blank=True, + help_text=_("Organizations under which the user is allowed " + "to create courses.")) + all_organizations = models.BooleanField(default=True, + help_text=_("Grant the user the permission to create courses " + "in ALL organizations")) def __str__(self): return f"{self.user} | {self.state} [{self.state_changed}]" @@ -60,6 +66,7 @@ def post_init_callback(sender, **kwargs): # lint-amnesty, pylint: disable=unuse """ instance = kwargs['instance'] instance.orig_state = instance.state + instance.orig_all_organizations = instance.all_organizations @receiver(post_save, sender=CourseCreator) @@ -70,7 +77,9 @@ def post_save_callback(sender, **kwargs): instance = kwargs['instance'] # We only wish to modify the state_changed time if the state has been modified. We don't wish to # modify it for changes to the notes field. - if instance.state != instance.orig_state: + # We need to keep track of all_organization switch. If this switch is changed we are going to remove the + # Course Creator group. + if instance.state != instance.orig_state or instance.all_organizations != instance.orig_all_organizations: granted_state_change = instance.state == CourseCreator.GRANTED or instance.orig_state == CourseCreator.GRANTED # If either old or new state is 'granted', we must manipulate the course creator # group maintained by authz. That requires staff permissions (stored admin). @@ -80,7 +89,8 @@ def post_save_callback(sender, **kwargs): sender=sender, caller=instance.admin, user=instance.user, - state=instance.state + state=instance.state, + all_organizations=instance.all_organizations ) # If user has been denied access, granted access, or previously granted access has been @@ -101,4 +111,5 @@ def post_save_callback(sender, **kwargs): instance.state_changed = timezone.now() instance.orig_state = instance.state + instance.orig_all_organizations = instance.all_organizations instance.save() diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index 5d3846ca2865..76616731b43e 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -15,10 +15,11 @@ add_user_with_status_unrequested, get_course_creator_status, update_course_creator_group, + update_org_content_creator_role, user_requested_access ) from common.djangoapps.student import auth -from common.djangoapps.student.roles import CourseCreatorRole +from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole class CourseCreatorView(TestCase): @@ -32,6 +33,7 @@ def setUp(self): self.user = User.objects.create_user('test_user', 'test_user+courses@edx.org', 'foo') self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo') self.admin.is_staff = True + self.org = "Edx" def test_staff_permission_required(self): """ @@ -76,6 +78,14 @@ def test_update_creator_group(self): update_course_creator_group(self.admin, self.user, False) self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) + def test_update_org_content_creator_role(self): + with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) + update_org_content_creator_role(self.admin, self.user, [self.org]) + self.assertTrue(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) + update_org_content_creator_role(self.admin, self.user, []) + self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) + def test_user_requested_access(self): add_user_with_status_unrequested(self.user) self.assertEqual('unrequested', get_course_creator_status(self.user)) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 80f081706722..199bae1c41de 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -5,7 +5,7 @@ from cms.djangoapps.course_creators.models import CourseCreator from common.djangoapps.student import auth -from common.djangoapps.student.roles import CourseCreatorRole +from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole def add_user_with_status_unrequested(user): @@ -50,6 +50,16 @@ def update_course_creator_group(caller, user, add): auth.remove_users(caller, CourseCreatorRole(), user) +def update_org_content_creator_role(caller, user, orgs): + """ + Method for updating users for OrgContentCreatorRole, this method + takes care of creating and deleting the role as required. + + Caller must have staff permissions. + """ + auth.update_org_role(caller, OrgContentCreatorRole, user, orgs) + + def get_course_creator_status(user): """ Returns the status for a particular user, or None if user is not in the table. diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index 421af61b8018..3091dbc45406 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -19,6 +19,7 @@ CourseStaffRole, GlobalStaff, LibraryUserRole, + OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, OrgStaffRole @@ -49,7 +50,7 @@ def user_has_role(user, role): """ if not user.is_active: return False - # do cheapest check first even tho it's not the direct one + # Do cheapest check first even though it's not the direct one if GlobalStaff().has_user(user): return True # CourseCreator is odd b/c it can be disabled via config @@ -63,10 +64,11 @@ def user_has_role(user, role): if role.has_user(user): return True - # if not, then check inferred permissions + # If not, then check inferred permissions if (isinstance(role, (CourseStaffRole, CourseBetaTesterRole)) and CourseInstructorRole(role.course_key).has_user(user)): return True + return False @@ -160,6 +162,26 @@ def remove_users(caller, role, *users): role.remove_users(*users) +def update_org_role(caller, role, user, orgs): + """ + The caller requests updating the Org role for the user. Checks that the caller has + sufficient authority. + + :param caller: an user + :param role: an AccessRole class + :param user: an user for which org roles are updated + :param orgs: List of organization names to update the org role + """ + _check_caller_authority(caller, role()) + existing_org_roles = set(role().get_orgs_for_user(user)) + orgs_roles_to_create = list(set(orgs) - existing_org_roles) + org_roles_to_delete = list(existing_org_roles - set(orgs)) + for org in orgs_roles_to_create: + role(org=org).add_users(user) + for org in org_roles_to_delete: + role(org=org).remove_users(user) + + def _check_caller_authority(caller, role): """ Internal function to check whether the caller has authority to manipulate this role @@ -172,7 +194,7 @@ def _check_caller_authority(caller, role): if GlobalStaff().has_user(caller): return - if isinstance(role, (GlobalStaff, CourseCreatorRole)): # lint-amnesty, pylint: disable=no-else-raise + if isinstance(role, (GlobalStaff, CourseCreatorRole, OrgContentCreatorRole)): # lint-amnesty, pylint: disable=no-else-raise raise PermissionDenied elif isinstance(role, CourseRole): # instructors can change the roles w/in their course if not user_has_role(caller, CourseInstructorRole(role.course_key)): diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 052eb30862d6..1fcd9887ea07 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -218,6 +218,12 @@ def users_with_role(self): ) return entries + def get_orgs_for_user(self, user): + """ + Returns a list of org short names for the user with given role. + """ + return CourseAccessRole.objects.filter(user=user, role=self._role_name).values_list('org', flat=True) + class CourseRole(RoleBase): """ @@ -332,6 +338,16 @@ def __init__(self, *args, **kwargs): super().__init__('instructor', *args, **kwargs) +@register_access_role +class OrgContentCreatorRole(OrgRole): + """An organization content creator""" + + ROLE = "org_course_creator_group" + + def __init__(self, *args, **kwargs): + super().__init__(self.ROLE, *args, **kwargs) + + class OrgLibraryUserRole(OrgRole): """ A user who can view any libraries in an org and import content from them, but not edit them. diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index 4bd1587f5517..af59148ac53c 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -3,17 +3,29 @@ """ from unittest import mock -import pytest +import pytest from ccx_keys.locator import CCXLocator from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import PermissionDenied from django.test import TestCase from opaque_keys.edx.locator import CourseLocator -from common.djangoapps.student.auth import add_users, has_studio_read_access, has_studio_write_access, remove_users, user_has_role # lint-amnesty, pylint: disable=line-too-long -from common.djangoapps.student.roles import CourseCreatorRole, CourseInstructorRole, CourseStaffRole -from common.djangoapps.student.tests.factories import AdminFactory +from common.djangoapps.student.auth import ( + add_users, + has_studio_read_access, + has_studio_write_access, + remove_users, + update_org_role, + user_has_role +) +from common.djangoapps.student.roles import ( + CourseCreatorRole, + CourseInstructorRole, + CourseStaffRole, + OrgContentCreatorRole +) +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory class CreatorGroupTest(TestCase): @@ -244,3 +256,34 @@ def test_remove_user_from_course_group_permission_denied(self): add_users(self.global_admin, CourseStaffRole(self.course_key), self.creator, self.staff, another_staff) with pytest.raises(PermissionDenied): remove_users(self.staff, CourseStaffRole(self.course_key), another_staff) + + +class CourseOrgGroupTest(TestCase): + """ + Tests for Org Content Creator groups for a particular course. + """ + + def setUp(self): + """ Test case setup """ + super().setUp() + self.global_admin = AdminFactory() + self.user = UserFactory.create( + username='test', email='test+courses@edx.org', password='foo' + ) + self.org = 'mitx' + self.course_key = CourseLocator(self.org, '101', 'test') + + def test_update_org_role_permission_denied(self): + """ + Verifies PermissionDenied if caller of update_org_role is not instructor role. + """ + with pytest.raises(PermissionDenied): + update_org_role(self.user, OrgContentCreatorRole, self.user, [self.org]) + + def test_update_org_role_permission(self): + """ + Verifies if caller of update_org_role is GlobalAdmin. + """ + assert not user_has_role(self.user, OrgContentCreatorRole(self.org)) + update_org_role(self.global_admin, OrgContentCreatorRole, self.user, [self.org]) + assert user_has_role(self.user, OrgContentCreatorRole(self.org)) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index 059abcf2e821..3bd5cb06421c 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -15,11 +15,12 @@ CourseRole, CourseStaffRole, GlobalStaff, + OrgContentCreatorRole, OrgInstructorRole, OrgStaffRole, RoleCache ) -from common.djangoapps.student.tests.factories import AnonymousUserFactory +from common.djangoapps.student.tests.factories import AnonymousUserFactory, InstructorFactory, StaffFactory, UserFactory class RolesTestCase(TestCase): @@ -36,6 +37,7 @@ def setUp(self): self.global_staff = UserFactory(is_staff=True) self.course_staff = StaffFactory(course_key=self.course_key) self.course_instructor = InstructorFactory(course_key=self.course_key) + self.orgs = ["Marvel", "DC"] def test_global_staff(self): assert not GlobalStaff().has_user(self.student) @@ -140,6 +142,18 @@ def test_add_users_doesnt_add_duplicate_entry(self): role.remove_users(self.student) assert not role.has_user(self.student) + def test_get_orgs_for_user(self): + """ + Test get_orgs_for_user + """ + role = OrgContentCreatorRole(org=self.orgs[0]) + assert len(role.get_orgs_for_user(self.student)) == 0 + role.add_users(self.student) + assert len(role.get_orgs_for_user(self.student)) == 1 + role_second_org = OrgContentCreatorRole(org=self.orgs[1]) + role_second_org.add_users(self.student) + assert len(role.get_orgs_for_user(self.student)) == 2 + @ddt.ddt class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-class-docstring From 284785c2f5f9ee47dc0e8862e543f3a10d558c48 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Mon, 4 Oct 2021 13:30:37 +0530 Subject: [PATCH 2/2] fix: port bugfix for #28735 Signed-off-by: Farhaan Bukhsh --- cms/djangoapps/course_creators/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 45ff82ec49cc..a1d34b3897fc 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -82,7 +82,7 @@ class CourseCreatorAdmin(admin.ModelAdmin): # Fields that filtering support list_filter = ['state', 'state_changed'] # Fields that search supports. - search_fields = ['user__username', 'user__email', 'state', 'note', 'organizations'] + search_fields = ['user__username', 'user__email', 'state', 'note'] # Turn off the action bar (we have no bulk actions) actions = None form = CourseCreatorForm