From 48ad7effb1df704737351310d7c4dfcfd2521865 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Fri, 10 Sep 2021 22:13:26 +0530 Subject: [PATCH] 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 | 49 +++++- common/djangoapps/student/tests/test_roles.py | 19 ++- 14 files changed, 459 insertions(+), 51 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 70232c47e9..c76837912d 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 @@ Test view handler for rerun (and eventually create) 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 @@ class TestCourseListing(ModuleStoreTestCase): # 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 @@ class TestCourseListing(ModuleStoreTestCase): ) 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 @@ class TestCourseListing(ModuleStoreTestCase): 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 8353fb332c..52e1c13db9 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -43,10 +43,8 @@ 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 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, @@ -108,12 +106,13 @@ from ..utils import ( 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 ) @@ -564,7 +563,7 @@ def course_listing(request): '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), @@ -850,9 +849,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')) @@ -860,6 +856,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'): @@ -916,6 +916,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 4ed4ba1414..6926b45f93 100644 --- a/cms/djangoapps/contentstore/views/helpers.py +++ b/cms/djangoapps/contentstore/views/helpers.py @@ -10,12 +10,14 @@ from django.http import HttpResponse 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 5a85555cdb..b065e3e4cd 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -19,6 +19,9 @@ from opaque_keys.edx.keys import CourseKey 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 @@ from common.djangoapps.student.auth import ( 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 e62c565095..0addf47194 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -23,7 +23,7 @@ from common.djangoapps.student.roles import LibraryUserRole 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 @@ class UnitTestLibraries(CourseTestCase): @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 @@ class UnitTestLibraries(CourseTestCase): "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 @@ class UnitTestLibraries(CourseTestCase): 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 90683660a6..45ff82ec49 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -6,9 +6,12 @@ django admin page for the course creators table 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 @@ from cms.djangoapps.course_creators.models import ( 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 @@ class CourseCreatorAdmin(admin.ModelAdmin): 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 0000000000..171e7ee531 --- /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 dc8191202f..9940ee026c 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.dispatch import Signal, receiver from django.utils import timezone 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"]) @@ -47,6 +47,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}]" @@ -59,6 +65,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) @@ -69,7 +76,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). @@ -79,7 +88,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 @@ -100,4 +110,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 0a5ac8c202..5ea37b37fc 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -14,10 +14,11 @@ from cms.djangoapps.course_creators.views import ( 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 from common.djangoapps.student.tests.factories import UserFactory @@ -40,6 +41,7 @@ class CourseCreatorView(TestCase): password='foo', ) self.admin.is_staff = True + self.org = "Edx" def test_staff_permission_required(self): """ @@ -84,6 +86,14 @@ class CourseCreatorView(TestCase): 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 80f0817067..199bae1c41 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -5,7 +5,7 @@ Methods for interacting programmatically with the user creator table. 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 421af61b80..3091dbc454 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -19,6 +19,7 @@ from common.djangoapps.student.roles import ( 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 052eb30862..1fcd9887ea 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -218,6 +218,12 @@ class RoleBase(AccessRole): ) 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 @@ class OrgInstructorRole(OrgRole): 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 a9d1406e4a..78257750cd 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -3,16 +3,28 @@ Tests authz.py """ from unittest import mock -import pytest +import pytest from ccx_keys.locator import CCXLocator from django.contrib.auth.models import AnonymousUser 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.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 @@ -254,3 +266,34 @@ class CourseGroupTest(TestCase): 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 520c6699c9..6c344352a1 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -14,14 +14,12 @@ from common.djangoapps.student.roles import ( CourseRole, CourseStaffRole, GlobalStaff, + OrgContentCreatorRole, OrgInstructorRole, OrgStaffRole, RoleCache ) -from common.djangoapps.student.tests.factories import AnonymousUserFactory -from common.djangoapps.student.tests.factories import InstructorFactory -from common.djangoapps.student.tests.factories import StaffFactory -from common.djangoapps.student.tests.factories import UserFactory +from common.djangoapps.student.tests.factories import AnonymousUserFactory, InstructorFactory, StaffFactory, UserFactory class RolesTestCase(TestCase): @@ -38,6 +36,7 @@ class RolesTestCase(TestCase): 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) @@ -142,6 +141,18 @@ class RolesTestCase(TestCase): 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