From 3b6a86bb689253c08e83322605eaa6be4d1065e8 Mon Sep 17 00:00:00 2001 From: David Joy Date: Thu, 2 Apr 2020 14:57:33 -0400 Subject: [PATCH 1/5] fix: Improving access check in CoursewareInformation view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We weren’t using has_access to check user access, which meant we were missing out on a bunch of checks. This PR adds a local _check_access function to CoursewareInformation. Ideally we would add this into access.py, but we’re adding it here for now to avoid any unexpected regressions in editing more commonly used code. This should ultimately be folded into our access system properly. TNL-7053 --- .../core/djangoapps/courseware_api/views.py | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index a056d2c22e..1921323dd3 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -80,6 +80,27 @@ class CoursewareInformation(RetrieveAPIView): serializer_class = CourseInfoSerializer + def _check_access(self, user, overview): + if has_access(user, 'staff', overview): + return True + + # We can only trust has_access in its false case because it doesn't check everything we + # need to check. + if not has_access(user, 'load', overview): + return False + + has_public_access = allow_public_access(overview, [COURSE_VISIBILITY_PUBLIC]) + if user.is_anonymous and not has_public_access: + return False + + if not CourseEnrollment.is_enrolled(user, overview.id) and not has_public_access: + return False + + # if is_survey_required_and_unanswered(user, course): + # TODO: This. + + return True + def get_object(self): """ Return the requested course object, if the user has appropriate @@ -91,6 +112,7 @@ class CoursewareInformation(RetrieveAPIView): self.request.user.username, CourseKey.from_string(self.kwargs['course_key_string']), ) + if self.request.user.is_anonymous: mode = None is_active = False @@ -99,14 +121,14 @@ class CoursewareInformation(RetrieveAPIView): overview.effective_user, overview.id ) - overview.enrollment = {'mode': mode, 'is_active': is_active} - if not is_active: - user_has_access = allow_public_access(overview, [COURSE_VISIBILITY_PUBLIC]) - else: - user_has_access = True - overview.user_has_access = user_has_access - overview.user_has_staff_access = has_access(self.request.user, 'staff', overview).has_access + + overview.can_load_course = self._check_access(self.request.user, overview) + overview.is_staff = has_access(self.request.user, 'staff', overview).has_access + + overview.user_has_access = overview.can_load_course # TODO: TNL-7053 Legacy: Delete once ready to contract + overview.user_has_staff_access = overview.is_staff # TODO: TNL-7053 Legacy: Delete once ready to contract + return overview def get_serializer_context(self): From ff33cc54ddb451081594cbaa99dcbe352a3f13a8 Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Fri, 3 Apr 2020 09:46:47 -0400 Subject: [PATCH 2/5] fix lint error --- openedx/core/djangoapps/courseware_api/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 1921323dd3..df7d009fbd 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -126,8 +126,8 @@ class CoursewareInformation(RetrieveAPIView): overview.can_load_course = self._check_access(self.request.user, overview) overview.is_staff = has_access(self.request.user, 'staff', overview).has_access - overview.user_has_access = overview.can_load_course # TODO: TNL-7053 Legacy: Delete once ready to contract - overview.user_has_staff_access = overview.is_staff # TODO: TNL-7053 Legacy: Delete once ready to contract + overview.user_has_access = overview.can_load_course # TODO: TNL-7053 Legacy: Delete once ready to contract + overview.user_has_staff_access = overview.is_staff # TODO: TNL-7053 Legacy: Delete once ready to contract return overview From 4363b1ede0e9dec09aa12f49d41a531552b686cd Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Fri, 3 Apr 2020 09:55:28 -0400 Subject: [PATCH 3/5] Refactor public course check to only call if needed --- openedx/core/djangoapps/courseware_api/views.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index df7d009fbd..c7593bbfc7 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -89,12 +89,11 @@ class CoursewareInformation(RetrieveAPIView): if not has_access(user, 'load', overview): return False - has_public_access = allow_public_access(overview, [COURSE_VISIBILITY_PUBLIC]) - if user.is_anonymous and not has_public_access: - return False - - if not CourseEnrollment.is_enrolled(user, overview.id) and not has_public_access: - return False + # Anonymous or unenrolled users + if user.is_anonymous or not CourseEnrollment.is_enrolled(user, overview.id): + # do not have access if the course is not public + if not allow_public_access(overview, [COURSE_VISIBILITY_PUBLIC]): + return False # if is_survey_required_and_unanswered(user, course): # TODO: This. From 8b7ff1ac2ae5273854e61349e972df6dcd8646a0 Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Fri, 3 Apr 2020 10:21:06 -0400 Subject: [PATCH 4/5] Eliminate extra has staff access checks --- openedx/core/djangoapps/courseware_api/views.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index c7593bbfc7..c6cb01af77 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -80,8 +80,11 @@ class CoursewareInformation(RetrieveAPIView): serializer_class = CourseInfoSerializer - def _check_access(self, user, overview): - if has_access(user, 'staff', overview): + def _check_access(self, user, overview, is_staff=None): + if is_staff is None: + is_staff = has_access(user, 'staff', overview) + + if is_staff: return True # We can only trust has_access in its false case because it doesn't check everything we @@ -122,8 +125,8 @@ class CoursewareInformation(RetrieveAPIView): ) overview.enrollment = {'mode': mode, 'is_active': is_active} - overview.can_load_course = self._check_access(self.request.user, overview) overview.is_staff = has_access(self.request.user, 'staff', overview).has_access + overview.can_load_course = self._check_access(self.request.user, overview, overview.is_staff) overview.user_has_access = overview.can_load_course # TODO: TNL-7053 Legacy: Delete once ready to contract overview.user_has_staff_access = overview.is_staff # TODO: TNL-7053 Legacy: Delete once ready to contract From 1b84f815ccdcf37bc52264c57cd4d1804af1fd5d Mon Sep 17 00:00:00 2001 From: David Joy Date: Fri, 3 Apr 2020 11:50:29 -0400 Subject: [PATCH 5/5] Add is_staff and can_load_course to the CourseInfoSerializer. --- .../core/djangoapps/courseware_api/serializers.py | 8 ++++++-- openedx/core/djangoapps/courseware_api/views.py | 12 +++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/serializers.py b/openedx/core/djangoapps/courseware_api/serializers.py index a27ddbf17b..a5acda0fc0 100644 --- a/openedx/core/djangoapps/courseware_api/serializers.py +++ b/openedx/core/djangoapps/courseware_api/serializers.py @@ -83,11 +83,15 @@ class CourseInfoSerializer(serializers.Serializer): # pylint: disable=abstract- start_type = serializers.CharField() pacing = serializers.CharField() enrollment = serializers.DictField() - user_has_access = serializers.BooleanField() - user_has_staff_access = serializers.BooleanField() tabs = serializers.SerializerMethodField() verified_mode = serializers.SerializerMethodField() show_calculator = serializers.BooleanField() + is_staff = serializers.BooleanField() + can_load_courseware = serializers.BooleanField() + + # TODO: TNL-7053 Legacy: Delete these two once ready to contract + user_has_access = serializers.BooleanField() + user_has_staff_access = serializers.BooleanField() def __init__(self, *args, **kwargs): """ diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index c6cb01af77..d30e2d3846 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -80,10 +80,7 @@ class CoursewareInformation(RetrieveAPIView): serializer_class = CourseInfoSerializer - def _check_access(self, user, overview, is_staff=None): - if is_staff is None: - is_staff = has_access(user, 'staff', overview) - + def _check_access(self, user, overview, is_staff): if is_staff: return True @@ -126,10 +123,11 @@ class CoursewareInformation(RetrieveAPIView): overview.enrollment = {'mode': mode, 'is_active': is_active} overview.is_staff = has_access(self.request.user, 'staff', overview).has_access - overview.can_load_course = self._check_access(self.request.user, overview, overview.is_staff) + overview.can_load_courseware = self._check_access(self.request.user, overview, overview.is_staff) - overview.user_has_access = overview.can_load_course # TODO: TNL-7053 Legacy: Delete once ready to contract - overview.user_has_staff_access = overview.is_staff # TODO: TNL-7053 Legacy: Delete once ready to contract + # TODO: TNL-7053 Legacy: Delete these two once ready to contract + overview.user_has_access = overview.can_load_courseware + overview.user_has_staff_access = overview.is_staff return overview