diff --git a/cms/djangoapps/api/v1/serializers/course_runs.py b/cms/djangoapps/api/v1/serializers/course_runs.py index cbd4d09e2181..6bbbce96dd42 100644 --- a/cms/djangoapps/api/v1/serializers/course_runs.py +++ b/cms/djangoapps/api/v1/serializers/course_runs.py @@ -5,6 +5,7 @@ from django.db import transaction from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from rest_framework import serializers from rest_framework.fields import empty @@ -203,3 +204,35 @@ def update(self, instance, validated_data): course_run = get_course_and_check_access(new_course_run_key, user) self.update_team(course_run, team) return course_run + + +class CourseCloneSerializer(serializers.Serializer): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring + source_course_id = serializers.CharField() + destination_course_id = serializers.CharField() + + def validate(self, attrs): + source_course_id = attrs.get('source_course_id') + destination_course_id = attrs.get('destination_course_id') + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + + # Check if the source course exists + if not store.has_course(source_key): + raise serializers.ValidationError('Source course does not exist.') + + # Check if the destination course already exists + if store.has_course(dest_key): + raise serializers.ValidationError('Destination course already exists.') + return attrs + + def create(self, validated_data): + source_course_id = validated_data.get('source_course_id') + destination_course_id = validated_data.get('destination_course_id') + user_id = self.context['request'].user.id + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + with store.default_store('split'): + new_course = store.clone_course(source_key, dest_key, user_id) + return new_course diff --git a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py index 49589a473878..8366ef72941e 100644 --- a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py +++ b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py @@ -402,3 +402,54 @@ def test_rerun_invalid_number(self): assert response.data == {'non_field_errors': [ 'Invalid key supplied. Ensure there are no special characters in the Course Number.' ]} + + def test_clone_course(self): + course = CourseFactory() + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': str(course.id), + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 201 + self.assertEqual(response.data, {"message": "Course cloned successfully."}) + + def test_clone_course_with_missing_source_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'source_course_id': ['This field is required.']}) + + def test_clone_course_with_missing_dest_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:source+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'destination_course_id': ['This field is required.']}) + + def test_clone_course_with_nonexistent_source_course(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:nonexistent+source+course_id', + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Source course does not exist.' + + def test_clone_course_with_existing_dest_course(self): + url = reverse('api:v1:course_run-clone') + course = CourseFactory() + existing_dest_course = CourseFactory() + data = { + 'source_course_id': str(course.id), + 'destination_course_id': str(existing_dest_course.id), + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Destination course already exists.' diff --git a/cms/djangoapps/api/v1/views/course_runs.py b/cms/djangoapps/api/v1/views/course_runs.py index a0415d4e06dc..fb1671ebef04 100644 --- a/cms/djangoapps/api/v1/views/course_runs.py +++ b/cms/djangoapps/api/v1/views/course_runs.py @@ -13,6 +13,7 @@ from cms.djangoapps.contentstore.views.course import _accessible_courses_iter, get_course_and_check_access from ..serializers.course_runs import ( + CourseCloneSerializer, CourseRunCreateSerializer, CourseRunImageSerializer, CourseRunRerunSerializer, @@ -93,3 +94,10 @@ def rerun(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=miss new_course_run = serializer.save() serializer = self.get_serializer(new_course_run) return Response(serializer.data, status=status.HTTP_201_CREATED) + + @action(detail=False, methods=['post']) + def clone(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=missing-function-docstring, unused-argument + serializer = CourseCloneSerializer(data=request.data, context=self.get_serializer_context()) + serializer.is_valid(raise_exception=True) + serializer.save() + return Response({"message": "Course cloned successfully."}, status=status.HTTP_201_CREATED) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 1a4b709622e6..e392a32f3ad6 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1372,7 +1372,8 @@ def get_course_grading(course_key): 'grading_url': reverse_course_url('grading_handler', course_key), 'is_credit_course': is_credit_course(course_key), 'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_key), - 'course_assignment_lists': dict(course_assignment_lists) + 'course_assignment_lists': dict(course_assignment_lists), + 'default_grade_designations': settings.DEFAULT_GRADE_DESIGNATIONS } return grading_context diff --git a/cms/envs/common.py b/cms/envs/common.py index d5e561aa9cf9..10ec4b3aa084 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -531,6 +531,17 @@ # .. toggle_creation_date: 2023-03-31 # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32015 'DISABLE_ADVANCED_SETTINGS': False, + + # .. toggle_name: FEATURES['ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the legacy MD5 hashing algorithm to generate anonymous user id + # instead of the newer SHAKE128 hashing algorithm + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-08-08 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' + 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE @@ -2366,6 +2377,14 @@ # Rate limit for regrading tasks that a grading policy change can kick off POLICY_CHANGE_TASK_RATE_LIMIT = '900/h' +# .. setting_name: DEFAULT_GRADE_DESIGNATIONS +# .. setting_default: ['A', 'B', 'C', 'D'] +# .. setting_description: The default 'pass' grade cutoff designations to be used. The failure grade +# is always 'F' and should not be included in this list. +# .. setting_warning: The DEFAULT_GRADE_DESIGNATIONS list must have more than one designation, +# or else ['A', 'B', 'C', 'D'] will be used as the default grade designations. +DEFAULT_GRADE_DESIGNATIONS = ['A', 'B', 'C', 'D'] + ############## Settings for CourseGraph ############################ # .. setting_name: COURSEGRAPH_JOB_QUEUE diff --git a/cms/static/js/factories/settings_graders.js b/cms/static/js/factories/settings_graders.js index dc75029e0f26..57e811b7ed3f 100644 --- a/cms/static/js/factories/settings_graders.js +++ b/cms/static/js/factories/settings_graders.js @@ -3,7 +3,7 @@ define([ ], function($, GradingView, CourseGradingPolicyModel) { 'use strict'; - return function(courseDetails, gradingUrl, courseAssignmentLists) { + return function(courseDetails, gradingUrl, gradeDesignations, courseAssignmentLists) { var model, editor; $('form :input') @@ -19,7 +19,8 @@ define([ editor = new GradingView({ el: $('.settings-grading'), model: model, - courseAssignmentLists: courseAssignmentLists + courseAssignmentLists: courseAssignmentLists, + gradeDesignations: gradeDesignations }); editor.render(); }; diff --git a/cms/static/js/views/settings/grading.js b/cms/static/js/views/settings/grading.js index ac4d170352a2..3d383ea457af 100644 --- a/cms/static/js/views/settings/grading.js +++ b/cms/static/js/views/settings/grading.js @@ -34,6 +34,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { $('#course_grade_cutoff-tpl').text() ); this.setupCutoffs(); + this.setupGradeDesignations(options.gradeDesignations); this.listenTo(this.model, 'invalid', this.handleValidationError); this.listenTo(this.model, 'change', this.showNotificationBar); @@ -318,7 +319,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { addNewGrade: function(e) { e.preventDefault(); var gradeLength = this.descendingCutoffs.length; // cutoffs doesn't include fail/f so this is only the passing grades - if (gradeLength > 3) { + if (gradeLength > this.GRADES.length - 1) { // TODO shouldn't we disable the button return; } @@ -399,7 +400,9 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { this.descendingCutoffs = _.sortBy(this.descendingCutoffs, function(gradeEle) { return -gradeEle.cutoff; }); }, - revertView: function() { + setupGradeDesignations: function(gradeDesignations) { + if (Array.isArray(gradeDesignations) && gradeDesignations.length > 1) { this.GRADES = gradeDesignations; } + },revertView: function() { var self = this; this.model.fetch({ success: function() { diff --git a/cms/static/sass/views/_settings.scss b/cms/static/sass/views/_settings.scss index cc62fd436f0d..c571af4b8cb8 100644 --- a/cms/static/sass/views/_settings.scss +++ b/cms/static/sass/views/_settings.scss @@ -777,23 +777,23 @@ height: 17px; } - &:nth-child(1) { + &:nth-child(5n+1) { background: #4fe696; } - &:nth-child(2) { + &:nth-child(5n+2) { background: #ffdf7e; } - &:nth-child(3) { + &:nth-child(5n+3) { background: #ffb657; } - &:nth-child(4) { + &:nth-child(5n+4) { background: #ef54a1; } - &:nth-child(5), + &:nth-child(5n+5), &.bar-fail { background: #fb336c; } diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index c3b6f8f73a2a..1c8f0019bf3e 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -36,6 +36,7 @@ { is_credit_course: ${is_credit_course | n, dump_js_escaped_json} } ), "${grading_url | n, js_escaped_string}", + ${default_grade_designations | n, dump_js_escaped_json}, ${course_assignment_lists | n, dump_js_escaped_json}, ); }); diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index abb98dcaffdc..fb2999eca98d 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -93,9 +93,16 @@ def get_user_permissions(user, course_key, org=None): return all_perms if course_key and user_has_role(user, CourseInstructorRole(course_key)): return all_perms - # Limited Course Staff does not have access to Studio. + # HACK: Limited Staff should not have studio read access. However, since many LMS views depend on the + # `has_course_author_access` check and `course_author_access_required` decorator, we have to allow write access + # until the permissions become more granular. For example, there could be STUDIO_VIEW_COHORTS and + # STUDIO_EDIT_COHORTS specifically for the cohorts endpoint, which is used to display the "Cohorts" tab of the + # Instructor Dashboard. + # The permissions matrix from the RBAC project (https://github.com/openedx/platform-roadmap/issues/246) shows that + # the LMS and Studio permissions will be separated as a part of this project. Once this is done (and this code is + # not removed during its implementation), we can replace the Limited Staff permissions with more granular ones. if course_key and user_has_role(user, CourseLimitedStaffRole(course_key)): - return STUDIO_NO_PERMISSIONS + return STUDIO_EDIT_CONTENT # Staff have all permissions except EDIT_ROLES: if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))): return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 2a4a9deb3664..cb806fed4467 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -154,12 +154,21 @@ def anonymous_id_for_user(user, course_id): # function: Rotate at will, since the hashes are stored and # will not change. # include the secret key as a salt, and to make the ids unique across different LMS installs. - hasher = hashlib.shake_128() + legacy_hash_enabled = settings.FEATURES.get('ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID', False) + if legacy_hash_enabled: + # Use legacy MD5 algorithm if flag enabled + hasher = hashlib.md5() + else: + hasher = hashlib.shake_128() hasher.update(settings.SECRET_KEY.encode('utf8')) hasher.update(str(user.id).encode('utf8')) if course_id: hasher.update(str(course_id).encode('utf-8')) - anonymous_user_id = hasher.hexdigest(16) + + if legacy_hash_enabled: + anonymous_user_id = hasher.hexdigest() + else: + anonymous_user_id = hasher.hexdigest(16) # pylint: disable=too-many-function-args try: AnonymousUserId.objects.create( diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index c2fd4564e420..ff6be033f9f5 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -7,6 +7,7 @@ import logging from abc import ABCMeta, abstractmethod from collections import defaultdict +from contextlib import contextmanager from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from opaque_keys.edx.django.models import CourseKeyField @@ -44,6 +45,17 @@ def register_access_role(cls): return cls +@contextmanager +def strict_role_checking(): + """ + Context manager that temporarily disables role inheritance. + """ + OLD_ACCESS_ROLES_INHERITANCE = ACCESS_ROLES_INHERITANCE.copy() + ACCESS_ROLES_INHERITANCE.clear() + yield + ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE) + + class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring CACHE_NAMESPACE = "student.roles.BulkRoleCache" CACHE_KEY = 'roles_by_user' @@ -78,7 +90,7 @@ def __init__(self, user): ) @staticmethod - def _get_roles(role): + def get_roles(role): """ Return the roles that should have the same permissions as the specified role. """ @@ -90,7 +102,7 @@ def has_role(self, role, course_id, org): or a role that inherits from the specified role, course_id and org. """ return any( - access_role.role in self._get_roles(role) and + access_role.role in self.get_roles(role) and access_role.course_id == course_id and access_role.org == org for access_role in self._roles @@ -136,7 +148,7 @@ class GlobalStaff(AccessRole): The global staff role """ def has_user(self, user): - return bool(user and user.is_staff) + return bool(user and (user.is_superuser or user.is_staff)) def add_users(self, *users): for user in users: @@ -286,6 +298,14 @@ class CourseLimitedStaffRole(CourseStaffRole): BASE_ROLE = CourseStaffRole.ROLE +@register_access_role +class eSHEInstructorRole(CourseStaffRole): + """A Staff member of a course without access to Studio.""" + + ROLE = 'eshe_instructor' + BASE_ROLE = CourseStaffRole.ROLE + + @register_access_role class CourseInstructorRole(CourseRole): """A course Instructor""" @@ -463,11 +483,11 @@ def remove_courses(self, *course_keys): def courses_with_role(self): """ - Return a django QuerySet for all of the courses with this user x role. You can access + Return a django QuerySet for all of the courses with this user x (or derived from x) role. You can access any of these properties on each result record: * user (will be self.user--thus uninteresting) * org * course_id * role (will be self.role--thus uninteresting) """ - return CourseAccessRole.objects.filter(role=self.role, user=self.user) + return CourseAccessRole.objects.filter(role__in=RoleCache.get_roles(self.role), user=self.user) diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index a7a3694d3489..1c79780e88c1 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -285,14 +285,14 @@ def test_remove_user_from_course_group_permission_denied(self): with pytest.raises(PermissionDenied): remove_users(self.staff, CourseStaffRole(self.course_key), another_staff) - def test_no_limited_staff_read_or_write_access(self): + def test_limited_staff_no_studio_read_access(self): """ - Test that course limited staff have no read or write access. + Verifies that course limited staff have no read, but have write access. """ add_users(self.global_admin, CourseLimitedStaffRole(self.course_key), self.limited_staff) assert not has_studio_read_access(self.limited_staff, self.course_key) - assert not has_studio_write_access(self.limited_staff, self.course_key) + assert has_studio_write_access(self.limited_staff, self.course_key) class CourseOrgGroupTest(TestCase): diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index dff86047d2ff..b97ebb0e26cf 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -15,6 +15,7 @@ CourseStaffRole, CourseFinanceAdminRole, CourseSalesAdminRole, + eSHEInstructorRole, LibraryUserRole, CourseDataResearcherRole, GlobalStaff, @@ -167,6 +168,7 @@ class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-clas ROLES = ( (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), (CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')), + (eSHEInstructorRole(IN_KEY), ('eshe_instructor', IN_KEY, 'edX')), (CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')), (OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')), (CourseFinanceAdminRole(IN_KEY), ('finance_admin', IN_KEY, 'edX')), diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index b41ad2f856d6..8f952831f12b 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -15,6 +15,7 @@ from django.test import TestCase, override_settings from django.test.client import Client from django.urls import reverse +from edx_toggles.toggles.testutils import override_waffle_switch from markupsafe import escape from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import CourseLocator @@ -33,6 +34,7 @@ user_by_anonymous_id ) from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory +from common.djangoapps.student.toggles import REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT from common.djangoapps.student.views import complete_course_mode_info from common.djangoapps.util.model_utils import USER_SETTINGS_CHANGED_EVENT_NAME from common.djangoapps.util.testing import EventTestMixin @@ -893,6 +895,7 @@ def test_change_enrollment_modes(self): @skip_unless_lms +@ddt.ddt class ChangeEnrollmentViewTest(ModuleStoreTestCase): """Tests the student.views.change_enrollment view""" @@ -913,6 +916,17 @@ def _enroll_through_view(self, course): ) return response + @ddt.data( + (True, 'courseware'), + (False, None), + ) + @ddt.unpack + def test_enrollment_url(self, waffle_flag_enabled, returned_view): + with override_waffle_switch(REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT, waffle_flag_enabled): + response = self._enroll_through_view(self.course) + data = reverse(returned_view, args=[str(self.course.id)]) if returned_view else '' + assert response.content.decode('utf8') == data + def test_enroll_as_default(self): """Tests that a student can successfully enroll through this view""" response = self._enroll_through_view(self.course) @@ -1050,6 +1064,17 @@ def test_anonymous_id_secret_key_changes_result_in_diff_values_for_same_new_user assert anonymous_id != new_anonymous_id assert self.user == user_by_anonymous_id(new_anonymous_id) + def test_enable_legacy_hash_flag(self): + """Test that different anonymous id returned if ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID enabled.""" + CourseEnrollment.enroll(self.user, self.course.id) + anonymous_id = anonymous_id_for_user(self.user, self.course.id) + with patch.dict(settings.FEATURES, ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID=True): + # Recreate user object to clear cached anonymous id. + self.user = User.objects.get(pk=self.user.id) + AnonymousUserId.objects.filter(user=self.user).filter(course_id=self.course.id).delete() + new_anonymous_id = anonymous_id_for_user(self.user, self.course.id) + assert anonymous_id != new_anonymous_id + @skip_unless_lms @patch('openedx.core.djangoapps.programs.utils.get_programs') diff --git a/common/djangoapps/student/toggles.py b/common/djangoapps/student/toggles.py index 46e9b0408fbf..507f323a2a53 100644 --- a/common/djangoapps/student/toggles.py +++ b/common/djangoapps/student/toggles.py @@ -1,7 +1,7 @@ """ Toggles for Dashboard page. """ -from edx_toggles.toggles import WaffleFlag +from edx_toggles.toggles import WaffleFlag, WaffleSwitch # Namespace for student waffle flags. WAFFLE_FLAG_NAMESPACE = 'student' @@ -75,3 +75,21 @@ def should_show_2u_recommendations(): def should_send_enrollment_email(): return ENROLLMENT_CONFIRMATION_EMAIL.is_enabled() + + +# Waffle flag to enable control redirecting after enrolment. +# .. toggle_name: student.redirect_to_courseware_after_enrollment +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Redirect to courseware after enrollment instead of dashboard. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2023-02-06 +# .. toggle_target_removal_date: None +# .. toggle_warning: None +REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT = WaffleSwitch( + f'{WAFFLE_FLAG_NAMESPACE}.redirect_to_courseware_after_enrollment', __name__ +) + + +def should_redirect_to_courseware_after_enrollment(): + return REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT.is_enabled() diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index d131575193b4..cb3df1627fd9 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -38,6 +38,7 @@ from rest_framework.decorators import api_view, authentication_classes, permission_classes from rest_framework.permissions import IsAuthenticated +from common.djangoapps.student.toggles import should_redirect_to_courseware_after_enrollment from common.djangoapps.track import views as track_views from lms.djangoapps.bulk_email.models import Optout from common.djangoapps.course_modes.models import CourseMode @@ -400,8 +401,10 @@ def change_enrollment(request, check_access=True): reverse("course_modes_choose", kwargs={'course_id': str(course_id)}) ) - # Otherwise, there is only one mode available (the default) - return HttpResponse() + if should_redirect_to_courseware_after_enrollment(): + return HttpResponse(reverse('courseware', args=[str(course_id)])) + else: + return HttpResponse() elif action == "unenroll": if configuration_helpers.get_value( "DISABLE_UNENROLLMENT", diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index af5159764c92..1ea265e868f2 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -366,13 +366,12 @@ class OAuth2ProviderConfig(ProviderConfig): .. no_pii: """ - # We are keying the provider config by backend_name here as suggested in the python social - # auth documentation. In order to reuse a backend for a second provider, a subclass can be - # created with seperate name. + # We are keying the provider config by backend_name and site_id to support configuration per site. + # In order to reuse a backend for a second provider, a subclass can be created with seperate name. # example: # class SecondOpenIDProvider(OpenIDAuth): # name = "second-openId-provider" - KEY_FIELDS = ('backend_name',) + KEY_FIELDS = ('site_id', 'backend_name') prefix = 'oa2' backend_name = models.CharField( max_length=50, blank=False, db_index=True, @@ -401,6 +400,29 @@ class Meta: verbose_name = "Provider Configuration (OAuth)" verbose_name_plural = verbose_name + @classmethod + def current(cls, *args): + """ + Get the current config model for the provider according to the given backend and the current + site. + """ + site_id = Site.objects.get_current(get_current_request()).id + return super(OAuth2ProviderConfig, cls).current(site_id, *args) + + @property + def provider_id(self): + """ + Unique string key identifying this provider. Must be URL and css class friendly. + Ignoring site_id as the config is filtered using current method which fetches the configuration for the current + site_id. + """ + assert self.prefix is not None + return "-".join((self.prefix, ) + tuple( + str(getattr(self, field)) + for field in self.KEY_FIELDS + if field != 'site_id' + )) + def clean(self): """ Standardize and validate fields """ super().clean() diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 9135ea556bea..fde2bb9cbc0c 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -854,7 +854,7 @@ def user_details_force_sync(auth_entry, strategy, details, user=None, *args, **k This step is controlled by the `sync_learner_profile_data` flag on the provider's configuration. """ current_provider = provider.Registry.get_from_pipeline({'backend': strategy.request.backend.name, 'kwargs': kwargs}) - if user and current_provider.sync_learner_profile_data: + if user and current_provider and current_provider.sync_learner_profile_data: # Keep track of which incoming values get applied. changed = {} @@ -931,7 +931,7 @@ def set_id_verification_status(auth_entry, strategy, details, user=None, *args, Use the user's authentication with the provider, if configured, as evidence of their identity being verified. """ current_provider = provider.Registry.get_from_pipeline({'backend': strategy.request.backend.name, 'kwargs': kwargs}) - if user and current_provider.enable_sso_id_verification: + if user and current_provider and current_provider.enable_sso_id_verification: # Get previous valid, non expired verification attempts for this SSO Provider and user verifications = SSOVerification.objects.filter( user=user, diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index 3fa8f80f4d1a..28e95c16b8f9 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -11,7 +11,9 @@ from common.djangoapps.third_party_auth import provider from common.djangoapps.third_party_auth.tests import testutil from common.djangoapps.third_party_auth.tests.utils import skip_unless_thirdpartyauth -from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration +from openedx.core.djangoapps.site_configuration.tests.test_util import ( + with_site_configuration, with_site_configuration_context +) SITE_DOMAIN_A = 'professionalx.example.com' SITE_DOMAIN_B = 'somethingelse.example.com' @@ -114,13 +116,13 @@ def test_providers_displayed_for_login(self): assert no_log_in_provider.provider_id not in provider_ids assert normal_provider.provider_id in provider_ids - def test_tpa_hint_provider_displayed_for_login(self): + def test_tpa_hint_exp_hidden_provider_displayed_for_login(self): """ - Tests to ensure that an enabled-but-not-visible provider is presented + Test to ensure that an explicitly enabled-but-not-visible provider is presented for use in the UI when the "tpa_hint" parameter is specified + A hidden provider should be accessible with tpa_hint (this is the main case) """ - # A hidden provider should be accessible with tpa_hint (this is the main case) hidden_provider = self.configure_google_provider(visible=False, enabled=True) provider_ids = [ idp.provider_id @@ -128,8 +130,14 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert hidden_provider.provider_id in provider_ids - # New providers are hidden (ie, not flagged as 'visible') by default - # The tpa_hint parameter should work for these providers as well + def test_tpa_hint_hidden_provider_displayed_for_login(self): + """ + Tests to ensure that an implicitly enabled-but-not-visible provider is presented + for use in the UI when the "tpa_hint" parameter is specified. + New providers are hidden (ie, not flagged as 'visible') by default + The tpa_hint parameter should work for these providers as well. + """ + implicitly_hidden_provider = self.configure_linkedin_provider(enabled=True) provider_ids = [ idp.provider_id @@ -137,7 +145,10 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert implicitly_hidden_provider.provider_id in provider_ids - # Disabled providers should not be matched in tpa_hint scenarios + def test_tpa_hint_disabled_hidden_provider_displayed_for_login(self): + """ + Disabled providers should not be matched in tpa_hint scenarios + """ disabled_provider = self.configure_twitter_provider(visible=True, enabled=False) provider_ids = [ idp.provider_id @@ -145,7 +156,10 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert disabled_provider.provider_id not in provider_ids - # Providers not utilized for learner authentication should not match tpa_hint + def test_tpa_hint_no_log_hidden_provider_displayed_for_login(self): + """ + Providers not utilized for learner authentication should not match tpa_hint + """ no_log_in_provider = self.configure_lti_provider() provider_ids = [ idp.provider_id @@ -153,6 +167,30 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert no_log_in_provider.provider_id not in provider_ids + def test_get_current_site_oauth_provider(self): + """ + Verify that correct provider for current site is returned even if same backend is used for multiple sites. + """ + site_a = Site.objects.get_or_create(domain=SITE_DOMAIN_A, name=SITE_DOMAIN_A)[0] + site_b = Site.objects.get_or_create(domain=SITE_DOMAIN_B, name=SITE_DOMAIN_B)[0] + site_a_provider = self.configure_google_provider(visible=True, enabled=True, site=site_a) + site_b_provider = self.configure_google_provider(visible=True, enabled=True, site=site_b) + with with_site_configuration_context(domain=SITE_DOMAIN_A): + assert site_a_provider.enabled_for_current_site is True + + # Registry.displayed_for_login gets providers enabled for current site + provider_ids = provider.Registry.displayed_for_login() + # Google oauth provider for current site should be displayed + assert site_a_provider in provider_ids + assert site_b_provider not in provider_ids + + # Similarly, the other site should only see its own providers + with with_site_configuration_context(domain=SITE_DOMAIN_B): + assert site_b_provider.enabled_for_current_site is True + provider_ids = provider.Registry.displayed_for_login() + assert site_b_provider in provider_ids + assert site_a_provider not in provider_ids + def test_provider_enabled_for_current_site(self): """ Verify that enabled_for_current_site returns True when the provider matches the current site. @@ -201,7 +239,7 @@ def test_oauth2_enabled_only_for_supplied_backend(self): def test_get_returns_none_if_provider_id_is_none(self): assert provider.Registry.get(None) is None - def test_get_returns_none_if_provider_not_enabled(self): + def test_get_returns_none_if_provider_not_enabled_change(self): linkedin_provider_id = "oa2-linkedin-oauth2" # At this point there should be no configuration entries at all so no providers should be enabled assert provider.Registry.enabled() == [] @@ -209,6 +247,12 @@ def test_get_returns_none_if_provider_not_enabled(self): # Now explicitly disabled this provider: self.configure_linkedin_provider(enabled=False) assert provider.Registry.get(linkedin_provider_id) is None + + def test_get_returns_provider_if_provider_enabled(self): + """ + Test to ensure that Registry gets enabled providers. + """ + linkedin_provider_id = "oa2-linkedin-oauth2" self.configure_linkedin_provider(enabled=True) assert provider.Registry.get(linkedin_provider_id).provider_id == linkedin_provider_id diff --git a/common/djangoapps/third_party_auth/views.py b/common/djangoapps/third_party_auth/views.py index c6a406c5301c..d24ba8cfd7db 100644 --- a/common/djangoapps/third_party_auth/views.py +++ b/common/djangoapps/third_party_auth/views.py @@ -47,7 +47,7 @@ def inactive_user_view(request): if third_party_auth.is_enabled() and pipeline.running(request): running_pipeline = pipeline.get(request) third_party_provider = provider.Registry.get_from_pipeline(running_pipeline) - if third_party_provider.skip_email_verification and not activated: + if third_party_provider and third_party_provider.skip_email_verification and not activated: user.is_active = True user.save() activated = True diff --git a/lms/djangoapps/course_wiki/middleware.py b/lms/djangoapps/course_wiki/middleware.py index 54884a23f252..37bef0b74740 100644 --- a/lms/djangoapps/course_wiki/middleware.py +++ b/lms/djangoapps/course_wiki/middleware.py @@ -15,6 +15,8 @@ from openedx.features.enterprise_support.api import get_enterprise_consent_url from common.djangoapps.student.models import CourseEnrollment +from xmodule.modulestore.django import modulestore + class WikiAccessMiddleware(MiddlewareMixin): """ @@ -55,6 +57,20 @@ def process_view(self, request, view_func, view_args, view_kwargs): # lint-amne course_id = course_id_from_url(request.path) wiki_path = request.path.partition('/wiki/')[2] + # if no wiki_path, can't get wiki_slug, so no point trying to look up + # course_id by wiki_slug + if not course_id and wiki_path: + # wiki path always starts with wiki_slug + wiki_slug = wiki_path.split('/')[0] + + course_ids = modulestore().get_courses_for_wiki(wiki_slug) + # the above can return multiple courses, and to avoid ambiguity and + # avoid pointing to wrong courses, we only set course_id if we've + # got an exact match, i.e. only one course was returned for a + # wiki_slug + if len(course_ids) == 1: + course_id = course_ids[0] + if course_id: # This is a /courses/org/name/run/wiki request course_path = f"/courses/{str(course_id)}" diff --git a/lms/djangoapps/course_wiki/tests/test_middleware.py b/lms/djangoapps/course_wiki/tests/test_middleware.py index d9fa62608cb1..264a24f5a297 100644 --- a/lms/djangoapps/course_wiki/tests/test_middleware.py +++ b/lms/djangoapps/course_wiki/tests/test_middleware.py @@ -34,3 +34,10 @@ def test_url_tranform(self): response = self.client.get('/courses/course-v1:edx+math101+2014/wiki/math101/') self.assertContains(response, '/courses/course-v1:edx+math101+2014/wiki/math101/_edit/') self.assertContains(response, '/courses/course-v1:edx+math101+2014/wiki/math101/_settings/') + + def test_finds_course_by_wiki_slug(self): + """Test that finds course by wiki slug, if course id is not present in the url.""" + response = self.client.get('/wiki/math101/') + request = response.wsgi_request + self.assertTrue(hasattr(request, 'course')) + self.assertEqual(request.course.id, self.course_math101.id) diff --git a/lms/djangoapps/courseware/rules.py b/lms/djangoapps/courseware/rules.py index 8202418f6f4c..07cbbab9022c 100644 --- a/lms/djangoapps/courseware/rules.py +++ b/lms/djangoapps/courseware/rules.py @@ -18,7 +18,7 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.enrollments.api import is_enrollment_valid_for_proctoring from common.djangoapps.student.models import CourseAccessRole -from common.djangoapps.student.roles import CourseRole, OrgRole +from common.djangoapps.student.roles import CourseRole, OrgRole, strict_role_checking from xmodule.course_block import CourseBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.error_block import ErrorBlock # lint-amnesty, pylint: disable=wrong-import-order @@ -47,10 +47,14 @@ class HasAccessRule(Rule): """ A rule that calls `has_access` to determine whether it passes """ - def __init__(self, action): + def __init__(self, action, strict=False): self.action = action + self.strict = strict def check(self, user, instance=None): + if self.strict: + with strict_role_checking(): + return has_access(user, self.action, instance) return has_access(user, self.action, instance) def query(self, user): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index fdc90a46a513..0b3038fad891 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -276,6 +276,7 @@ def courses(request): """ courses_list = [] course_discovery_meanings = getattr(settings, 'COURSE_DISCOVERY_MEANINGS', {}) + set_default_filter = settings.FEATURES.get('ENABLE_COURSE_DISCOVERY_DEFAULT_LANGUAGE_FILTER') if not settings.FEATURES.get('ENABLE_COURSE_DISCOVERY'): courses_list = get_courses(request.user) @@ -293,6 +294,7 @@ def courses(request): { 'courses': courses_list, 'course_discovery_meanings': course_discovery_meanings, + 'set_default_filter': set_default_filter, 'programs_list': programs_list, } ) diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 9255d113f038..343b6923ccee 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -19,6 +19,7 @@ CourseInstructorRole, CourseLimitedStaffRole, CourseStaffRole, + eSHEInstructorRole, ) from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params from openedx.core.djangoapps.django_comment_common.models import Role @@ -30,6 +31,7 @@ 'instructor': CourseInstructorRole, 'staff': CourseStaffRole, 'limited_staff': CourseLimitedStaffRole, + 'eshe_instructor': eSHEInstructorRole, 'ccx_coach': CourseCcxCoachRole, 'data_researcher': CourseDataResearcherRole, } diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 20426761e66b..0d8353653b93 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -29,6 +29,8 @@ RESCORE_EXAMS = 'instructor.rescore_exams' VIEW_REGISTRATION = 'instructor.view_registration' VIEW_DASHBOARD = 'instructor.dashboard' +VIEW_ENROLLMENTS = 'instructor.view_enrollments' +VIEW_FORUM_MEMBERS = 'instructor.view_forum_members' perms[ALLOW_STUDENT_TO_BYPASS_ENTRANCE_EXAM] = HasAccessRule('staff') @@ -44,7 +46,13 @@ # only global staff or those with the data_researcher role can access the data download tab # HasAccessRule('staff') also includes course staff perms[CAN_RESEARCH] = is_staff | HasRolesRule('data_researcher') -perms[CAN_ENROLL] = HasAccessRule('staff') +# eshe_instructor implicitly gets staff access, but shouldn't be able to enroll +perms[CAN_ENROLL] = ( + # can enroll if a user is an eshe_instructor and has an explicit staff role + (HasRolesRule('eshe_instructor') & HasAccessRule('staff', strict=True)) | + # can enroll if a user is just staff + (~HasRolesRule('eshe_instructor') & HasAccessRule('staff')) +) perms[CAN_BETATEST] = HasAccessRule('instructor') perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher') @@ -60,3 +68,5 @@ 'instructor', 'data_researcher' ) | HasAccessRule('staff') | HasAccessRule('instructor') +perms[VIEW_ENROLLMENTS] = HasAccessRule('staff') +perms[VIEW_FORUM_MEMBERS] = HasAccessRule('staff') diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index bca16977a98c..6572379b6eb9 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -26,9 +26,11 @@ from common.djangoapps.student.tests.factories import UserFactory from common.test.utils import XssTestMixin from lms.djangoapps.courseware.courses import get_studio_url +from lms.djangoapps.courseware.masquerade import CourseMasquerade from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase +from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK from lms.djangoapps.instructor.toggles import DATA_DOWNLOAD_V2 from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info @@ -37,6 +39,7 @@ ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND, OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG ) +from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -119,6 +122,11 @@ def has_instructor_tab(user, course): staff = StaffFactory(course_key=self.course.id) assert has_instructor_tab(staff, self.course) + masquerade_staff = StaffFactory(course_key=self.course.id) + masquerade = CourseMasquerade(self.course.id, role='student') + masquerade_staff.masquerade_settings = {self.course.id: masquerade} + assert not has_instructor_tab(masquerade_staff, self.course) + student = UserFactory.create() assert not has_instructor_tab(student, self.course) @@ -308,6 +316,56 @@ def test_membership_reason_field_visibility(self, enbale_reason_field): else: self.assertNotContains(response, reason_field) + def test_membership_tab_for_eshe_instructor(self): + """ + Verify that eSHE instructors don't have access to membership tab and + work correctly with other roles. + """ + + membership_section = ( + '