diff --git a/lms/djangoapps/ccx/custom_exception.py b/lms/djangoapps/ccx/custom_exception.py new file mode 100644 index 0000000000..c1855c378d --- /dev/null +++ b/lms/djangoapps/ccx/custom_exception.py @@ -0,0 +1,17 @@ +""" +This file has custom exceptions for ccx +""" + + +class CCXUserValidationException(Exception): + """ + Custom Exception for validation of users in CCX + """ + pass + + +class CCXLocatorValidationException(Exception): + """ + Custom Exception to validate CCX locator + """ + pass diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index e2556a51ca..764cce6b71 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -29,17 +29,11 @@ from student.roles import CourseCcxCoachRole from lms.djangoapps.ccx.models import CustomCourseForEdX from lms.djangoapps.ccx.overrides import get_override_for_ccx +from lms.djangoapps.ccx.custom_exception import CCXUserValidationException log = logging.getLogger("edx.ccx") -class CCXUserValidationException(Exception): - """ - Custom Exception for validation of users in CCX - """ - pass - - def get_ccx_from_ccx_locator(course_id): """ helper function to allow querying ccx fields from templates """ ccx_id = getattr(course_id, 'ccx', None) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 8af5c1d208..178568dd8e 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -39,6 +39,7 @@ from student import auth from student.models import CourseEnrollmentAllowed from student.roles import ( CourseBetaTesterRole, + CourseCcxCoachRole, CourseInstructorRole, CourseStaffRole, GlobalStaff, @@ -62,9 +63,41 @@ from courseware.access_response import ( ) from courseware.access_utils import adjust_start_date, check_start_date, debug, ACCESS_GRANTED, ACCESS_DENIED +from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException +from lms.djangoapps.ccx.models import CustomCourseForEdX + log = logging.getLogger(__name__) +def has_ccx_coach_role(user, course_key): + """ + Check if user is a coach on this ccx. + + Arguments: + user (User): the user whose descriptor access we are checking. + course_key (CCXLocator): Key to CCX. + + Returns: + bool: whether user is a coach on this ccx or not. + """ + if hasattr(course_key, 'ccx'): + ccx_id = course_key.ccx + role = CourseCcxCoachRole(course_key) + + if role.has_user(user): + list_ccx = CustomCourseForEdX.objects.filter( + course_id=course_key.to_course_locator(), + coach=user + ) + if list_ccx.exists(): + coach_ccx = list_ccx[0] + return str(coach_ccx.id) == ccx_id + else: + raise CCXLocatorValidationException("Invalid CCX key. To verify that " + "user is a coach on CCX, you must provide key to CCX") + return False + + def has_access(user, action, obj, course_key=None): """ Check whether a user has the access to do action on obj. Handles any magic diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 3667369f7a..b32cce269e 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -8,6 +8,9 @@ import itertools import pytz from django.contrib.auth.models import User +from ccx_keys.locator import CCXLocator +from django.http import Http404 +from django.test.client import RequestFactory from django.core.urlresolvers import reverse from django.test import TestCase from mock import Mock, patch @@ -24,9 +27,14 @@ from courseware.tests.factories import ( StaffFactory, UserFactory, ) +import courseware.views as views from courseware.tests.helpers import LoginEnrollmentTestCase +from edxmako.tests import mako_middleware_process_request from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from student.models import CourseEnrollment +from student.roles import CourseCcxCoachRole from student.tests.factories import ( + AdminFactory, AnonymousUserFactory, CourseEnrollmentAllowedFactory, CourseEnrollmentFactory, @@ -37,7 +45,11 @@ from xmodule.course_module import ( CATALOG_VISIBILITY_NONE, ) from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ( + ModuleStoreTestCase, + SharedModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE +) from util.milestones_helpers import ( set_prerequisite_courses, @@ -45,9 +57,98 @@ from util.milestones_helpers import ( seed_milestone_relationship_types, ) +from lms.djangoapps.ccx.models import CustomCourseForEdX + # pylint: disable=protected-access +class CoachAccessTestCaseCCX(SharedModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test if user is coach on ccx. + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + @classmethod + def setUpClass(cls): + """ + Set up course for tests + """ + super(CoachAccessTestCaseCCX, cls).setUpClass() + cls.course = CourseFactory.create() + + def setUp(self): + """ + Set up tests + """ + super(CoachAccessTestCaseCCX, self).setUp() + + # Create ccx coach account + self.coach = AdminFactory.create(password="test") + self.client.login(username=self.coach.username, password="test") + + # assign role to coach + role = CourseCcxCoachRole(self.course.id) + role.add_users(self.coach) + self.request_factory = RequestFactory() + + def make_ccx(self): + """ + create ccx + """ + ccx = CustomCourseForEdX( + course_id=self.course.id, + coach=self.coach, + display_name="Test CCX" + ) + ccx.save() + + ccx_locator = CCXLocator.from_course_locator(self.course.id, unicode(ccx.id)) + role = CourseCcxCoachRole(ccx_locator) + role.add_users(self.coach) + CourseEnrollment.enroll(self.coach, ccx_locator) + return ccx_locator + + def test_has_ccx_coach_role(self): + """ + Assert that user has coach access on ccx. + """ + ccx_locator = self.make_ccx() + + # user have access as coach on ccx + self.assertTrue(access.has_ccx_coach_role(self.coach, ccx_locator)) + + # user dont have access as coach on ccx + self.setup_user() + self.assertFalse(access.has_ccx_coach_role(self.user, ccx_locator)) + + def test_access_student_progress_ccx(self): + """ + Assert that only a coach can see progress of student. + """ + ccx_locator = self.make_ccx() + student = UserFactory() + + # Enroll user + CourseEnrollment.enroll(student, ccx_locator) + + # Test for access of a coach + request = self.request_factory.get(reverse('about_course', args=[unicode(ccx_locator)])) + request.user = self.coach + mako_middleware_process_request(request) + resp = views.progress(request, course_id=unicode(ccx_locator), student_id=student.id) + self.assertEqual(resp.status_code, 200) + + # Assert access of a student + request = self.request_factory.get(reverse('about_course', args=[unicode(ccx_locator)])) + request.user = student + mako_middleware_process_request(request) + + with self.assertRaises(Http404) as context: + views.progress(request, course_id=unicode(ccx_locator), student_id=self.coach.id) + + self.assertIsNotNone(context.exception) + + @attr('shard_1') @ddt.ddt class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 9d1685935a..18ec8a05c5 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -33,7 +33,7 @@ from rest_framework import status import newrelic.agent from courseware import grades -from courseware.access import has_access, _adjust_start_date_for_beta_testers +from courseware.access import has_access, has_ccx_coach_role, _adjust_start_date_for_beta_testers from courseware.access_response import StartDateError from courseware.access_utils import in_preview_mode from courseware.courses import ( @@ -97,6 +97,7 @@ from util.views import ensure_valid_course_key from eventtracking import tracker import analytics from courseware.url_helpers import get_redirect_url +from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException from lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.user_api.preferences.api import get_user_preference @@ -918,7 +919,7 @@ def course_about(request, course_id): def progress(request, course_id, student_id=None): """ Display the progress page. """ - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) + course_key = CourseKey.from_string(course_id) with modulestore().bulk_operations(course_key): return _progress(request, course_key, student_id) @@ -940,13 +941,19 @@ def _progress(request, course_key, student_id): return redirect(reverse('course_survey', args=[unicode(course.id)])) staff_access = bool(has_access(request.user, 'staff', course)) + try: + coach_access = has_ccx_coach_role(request.user, course_key) + except CCXLocatorValidationException: + coach_access = False + + has_access_on_students_profiles = staff_access or coach_access if student_id is None or student_id == request.user.id: # always allowed to see your own profile student = request.user else: # Requesting access to a different student's profile - if not staff_access: + if not has_access_on_students_profiles: raise Http404 try: student = User.objects.get(id=student_id)