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..a1d34b3897fc 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,18 +33,50 @@ 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 @@ -50,6 +85,7 @@ class CourseCreatorAdmin(admin.ModelAdmin): search_fields = ['user__username', 'user__email', 'state', 'note'] # 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