Merge pull request #845 from chaotic-kingdoms/oppia-26-permssions-dec…

…orators

oppia-26: Extracted permissions checks to custom decorator and mixin

oppia/mixins/PermissionMixins.py

@@ -0,0 +1,33 @@
+from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin
+
+from oppia.models import Participant, Course
+
+
+class CanViewUserDetailsPermissionMixin(LoginRequiredMixin, UserPassesTestMixin):
+    """
+    Verify that the current user can view details of another user. For this, the other user pk is get from
+    the URL kwargs. If it is not defined with the SingleObjectMixin `pk_url_kwarg`, the specific kwarg
+    can be set under the CVB that uses this mixin on the `user_url_kwarg` property
+    """
+    user_url_kwarg = None
+
+    def test_func(self):
+        if self.user_url_kwarg is not None:
+            user_kwarg = self.user_url_kwarg
+        else:
+            user_kwarg = self.pk_url_kwarg if hasattr(self, 'pk_url_kwarg') else None
+        user_pk = int(self.kwargs[user_kwarg]) if user_kwarg in self.kwargs else None
+
+        if self.request.user.is_staff or (self.request.user.id == int(user_pk)):
+            # If the user is staff or is the user itself, always can see the details
+            return True
+
+        # The logged in user can see other user details if they are a teacher in a course attended by that user
+        courses_teached_by = Course.objects.filter(
+            coursecohort__cohort__participant__user__pk=user_pk,
+            coursecohort__cohort__participant__role=Participant.STUDENT) \
+            .filter(
+            coursecohort__cohort__participant__user=self.request.user,
+            coursecohort__cohort__participant__role=Participant.TEACHER) \
+
+        return courses_teached_by.exists()

oppia/models/main.py

@@ -177,6 +177,31 @@ def get_no_media(self):
     def get_no_trackers(self):
         return Tracker.objects.filter(course=self).count()
 
+    # ------------ Permissions management ----------------
+
+    def user_can_view_detail(self, user):
+        if user.is_staff:
+            return True
+        else:
+            try:
+                Course.objects.get(
+                    pk=self.pk,
+                    coursepermissions__course=self,
+                    coursepermissions__user=user,
+                    coursepermissions__role=CoursePermissions.MANAGER)
+                return True
+            except Course.DoesNotExist:
+                return False
+
+    def user_can_edit(self, user):
+        return self.user_can_view_detail(user)
+
+    def user_can_edit_gamification(self, user):
+        return self.user_can_edit(user) and \
+            self.status is not CourseStatus.ARCHIVED and \
+            self.status is not CourseStatus.NEW_DOWNLOADS_DISABLED and \
+            self.status is not CourseStatus.READ_ONLY
+
     @staticmethod
     def get_pre_test_score(course, user):
         try:

oppia/permissions.py

@@ -1,10 +1,11 @@
 # oppia/permissions.py
-
+import functools
 from itertools import chain
 
 from django.contrib.auth.models import User
 from django.core.exceptions import PermissionDenied
 from django.http import Http404, HttpResponseForbidden
+from django.shortcuts import get_object_or_404
 
 from oppia.models import Course, Participant, Cohort, CoursePermissions
 from oppia.utils.filters import CourseFilter
@@ -37,31 +38,6 @@ def can_edit_user(request, view_user_id):
     else:
         return False
 
-
-def get_user(request, view_user_id):
-    if request.user.is_staff or (request.user.id == int(view_user_id)):
-        try:
-            view_user = User.objects.get(pk=view_user_id)
-            return view_user
-        except User.DoesNotExist:
-            raise Http404()
-    try:
-        view_user = User.objects.get(pk=view_user_id)
-        courses = Course.objects.filter(
-            coursecohort__cohort__participant__user=view_user,
-            coursecohort__cohort__participant__role=Participant.STUDENT) \
-            .filter(
-                coursecohort__cohort__participant__user=request.user,
-                coursecohort__cohort__participant__role=Participant.TEACHER) \
-            .count()
-        if courses > 0:
-            return view_user
-        else:
-            raise PermissionDenied
-    except User.DoesNotExist:
-        raise PermissionDenied
-
-
 def get_user_courses(request, view_user):
 
     if request.user.is_staff or request.user == view_user:
@@ -192,23 +168,18 @@ def can_download_course(request, course_id):
     return course
 
 
-def can_view_course_detail(request, course_id):
-    if request.user.is_staff:
-        try:
-            course = Course.objects.get(pk=course_id)
-            return course
-        except Course.DoesNotExist:
-            raise Http404
-    else:
-        try:
-            course = Course.objects.get(
-                        pk=course_id,
-                        coursepermissions__course__id=course_id,
-                        coursepermissions__user=request.user,
-                        coursepermissions__role=CoursePermissions.MANAGER)
-            return course
-        except Course.DoesNotExist:
+def permission_view_course_detail(view_func):
+    """
+        this decorator ensures that only the users who have permission to
+        access a course can view it, raising a 403 otherwise
+    """
+    @functools.wraps(view_func)
+    def wrapper(request, *args, **kwargs):
+        course = get_object_or_404(Course, pk=kwargs['course_id'])
+        if not course.user_can_view_detail(request.user):
             raise PermissionDenied
+        return view_func(request, *args, **kwargs)
+    return wrapper
 
 
 def can_view_course_activity(request, course_id):

oppia/test.py

@@ -28,6 +28,12 @@ def get_view(self, route, user=None):
             self.client.force_login(user)
         return self.client.get(route)
 
+    def assert_response_status(self, user, url, status_code):
+        self.client.force_login(user)
+        response = self.client.get(url)
+        self.assertEqual(status_code, response.status_code)
+        return response
+
 
 class OppiaTransactionTestCase(TransactionTestCase):
 

oppia/views/activity.py

@@ -5,6 +5,7 @@
 from django.db.models import Sum
 from django.db.models.functions import TruncDay, TruncMonth, TruncYear
 from django.http import HttpResponse
+from django.utils.decorators import method_decorator
 from django.views.generic import TemplateView, DetailView, ListView
 
 from helpers.forms.dates import DateRangeIntervalForm
@@ -14,12 +15,12 @@
 from oppia.forms.activity_search import ActivitySearchForm
 from oppia.models import Points, Course
 from oppia.models import Tracker
-from oppia.permissions import can_view_course_detail, can_edit_course_gamification
+from oppia.permissions import can_edit_course_gamification, permission_view_course_detail
 from oppia.views.utils import generate_graph_data
 from profile import utils
 from summary.models import CourseDailyStats, UserCourseSummary
 
-
+@method_decorator(permission_view_course_detail, name='dispatch')
 class CourseActivityDetail(DateRangeFilterMixin, DetailView):
     template_name = 'course/detail.html'
     pk_url_kwarg = 'course_id'
@@ -29,10 +30,7 @@ class CourseActivityDetail(DateRangeFilterMixin, DetailView):
     daterange_form_initial = {'interval': 'days'}
 
     def get_context_data(self, **kwargs):
-
         context = super().get_context_data(**kwargs)
-        can_view_course_detail(self.request, self.object.id)
-
         start_date, end_date = self.get_daterange()
         interval = self.get_daterange_form().cleaned_data.get("interval")
         context['monthly'] = interval == 'months'
@@ -65,7 +63,7 @@ def get_activity(self, start_date, end_date, interval):
 
             return generate_graph_data(monthly_stats, True)
 
-
+@method_decorator(permission_view_course_detail, name='dispatch')
 class CourseActivityDetailList(DateRangeFilterMixin, SafePaginatorMixin, ListView):
     template_name = 'course/activity/list.html'
     paginate_by = 25
@@ -89,7 +87,7 @@ def get_queryset(self):
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        context['course'] = can_view_course_detail(self.request, self.get_course_id())
+        context['course'] = Course.objects.get(pk=self.get_course_id())
         context['advanced_search'] = self.filtered
 
         for tracker in context['page_obj'].object_list:
@@ -106,10 +104,12 @@ def get_context_data(self, **kwargs):
         return context
 
 
+
 class ExportCourseTrackers(TemplateView):
 
+    @method_decorator(permission_view_course_detail)
     def get(self, request, course_id):
-        course = can_view_course_detail(request, course_id)
+        course = Course.objects.get(pk=course_id)
 
         headers = ('Date',
                    'UserId',

oppia/views/course.py

@@ -2,7 +2,6 @@
 
 from django.conf import settings
 from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin
-from django.core.exceptions import PermissionDenied
 from django.http import HttpResponseRedirect, HttpResponse
 from django.urls import reverse, reverse_lazy
 from django.utils.translation import gettext_lazy as _
@@ -13,7 +12,7 @@
 from helpers.mixins.SafePaginatorMixin import SafePaginatorMixin
 from oppia.forms.upload import UploadCourseStep1Form, UploadCourseStep2Form
 from oppia.models import Category, CourseCategory, CoursePublishingLog, Course
-from oppia.permissions import can_edit_course, can_download_course, can_view_course_detail, can_view_courses_list, \
+from oppia.permissions import can_edit_course, can_download_course, can_view_courses_list, \
     can_upload, can_edit_course_gamification
 from oppia.uploader import handle_uploaded_file
 from oppia.utils.filters import CourseFilter
@@ -66,11 +65,7 @@ def get_context_data(self, **kwargs):
             .aggregated_stats('total_downloads')
 
         for course in course_list:
-            try:
-                access_detail = can_view_course_detail(self.request, course.id)
-                course.access_detail = access_detail is not None
-            except PermissionDenied:
-                course.access_detail = None
+            course.access_detail = course.user_can_view_detail(self.request.user)
             course.can_edit = can_edit_course(self.request, course.id)
             course.can_edit_gamification = can_edit_course_gamification(self.request, course.id)
             for stats in course_stats:

oppia/views/feedback.py

@@ -1,14 +1,15 @@
 from django.http import HttpResponseRedirect
 from django.urls import reverse
+from django.utils.decorators import method_decorator
 from django.views.generic import ListView, DetailView
 
 from helpers.mixins.AjaxTemplateResponseMixin import AjaxTemplateResponseMixin
 from helpers.mixins.ListItemUrlMixin import ListItemUrlMixin
 from oppia.models import Course, Activity
-from oppia.permissions import can_view_course_detail
+from oppia.permissions import permission_view_course_detail
 from quiz.models import QuizAttempt, Quiz, QuizProps
 
-
+@method_decorator(permission_view_course_detail, name='dispatch')
 class CourseFeedbackActivitiesList(ListView,
                                    ListItemUrlMixin,
                                    AjaxTemplateResponseMixin):
@@ -20,10 +21,7 @@ class CourseFeedbackActivitiesList(ListView,
 
     def get_queryset(self):
         course = self.kwargs['course_id']
-        # check permissions, get_user raises PermissionDenied
-        can_view_course_detail(self.request, course)
-        return Activity.objects.filter(section__course=course,
-                                       type=Activity.FEEDBACK)
+        return Activity.objects.filter(section__course=course, type=Activity.FEEDBACK)
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
@@ -42,10 +40,8 @@ def get(self, request, *args, **kwargs):
 
         return super().get(request, *args, **kwargs)
 
-
-class CourseFeedbackResponsesList(ListView,
-                                  ListItemUrlMixin,
-                                  AjaxTemplateResponseMixin):
+@method_decorator(permission_view_course_detail, name='dispatch')
+class CourseFeedbackResponsesList(ListView, ListItemUrlMixin, AjaxTemplateResponseMixin):
 
     model = QuizAttempt
     objects_url_name = 'feedback_response_detail'
@@ -54,10 +50,6 @@ class CourseFeedbackResponsesList(ListView,
     paginate_by = 15
 
     def get_queryset(self):
-        course = self.kwargs['course_id']
-        # check permissions, get_user raises PermissionDenied
-        can_view_course_detail(self.request, course)
-
         activity = Activity.objects.get(pk=self.kwargs['feedback_id'])
         quiz = Quiz.objects.filter(quizprops__name=QuizProps.DIGEST,
                                    quizprops__value=activity.digest).last()

profile/views/activity.py

@@ -9,8 +9,9 @@
 from helpers.mixins.DateRangeFilterMixin import DateRangeFilterMixin
 from helpers.mixins.SafePaginatorMixin import SafePaginatorMixin
 from oppia.forms.activity_search import ActivitySearchForm
+from oppia.mixins.PermissionMixins import CanViewUserDetailsPermissionMixin
 from oppia.models import Activity, Tracker
-from oppia.permissions import get_user, get_user_courses, can_view_course, can_view_course_activity
+from oppia.permissions import get_user_courses, can_view_course, can_view_course_activity
 from oppia.views import filter_trackers
 from quiz.models import Quiz, QuizAttempt, QuizProps
 from summary.models import UserCourseSummary
@@ -25,15 +26,14 @@ def get_tracker_activities(user, course_ids=[], course=None):
     return trackers.filter(user=user)
 
 
-class UserScorecard(DateRangeFilterMixin, DetailView):
+class UserScorecard(CanViewUserDetailsPermissionMixin, DateRangeFilterMixin, DetailView):
     template_name = 'profile/user-scorecard.html'
     context_object_name = 'view_user'
     pk_url_kwarg = 'user_id'
     model = User
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        get_user(self.request, self.object.pk)  # TODO: Change permissions check
         cohort_courses, other_courses, all_courses = get_user_courses(self.request, self.object)
 
         courses = []
@@ -73,16 +73,14 @@ def get_context_data(self, **kwargs):
         return context
 
 
-class UserCourseScorecard(DateRangeFilterMixin, DetailView):
+class UserCourseScorecard(CanViewUserDetailsPermissionMixin, DateRangeFilterMixin, DetailView):
     template_name = 'profile/user-course-scorecard.html'
     context_object_name = 'view_user'
     pk_url_kwarg = 'user_id'
     model = User
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-
-        get_user(self.request, self.object.pk)  # TODO: Change permissions check
         course = can_view_course(self.request, self.kwargs['course_id'])
 
         act_quizzes = Activity.objects \
@@ -200,30 +198,26 @@ def process_quiz_activity(view_user,
     return quiz, course_pretest, quizzes_attempted, quizzes_passed
 
 
-class UserActivityDetailList(DateRangeFilterMixin, SafePaginatorMixin, ListView):
+class UserActivityDetailList(CanViewUserDetailsPermissionMixin, DateRangeFilterMixin, SafePaginatorMixin, ListView):
     template_name = 'profile/activity/list.html'
     paginate_by = 25
     daterange_form_class = ActivitySearchForm
+    user_url_kwarg = 'user_id'
 
     def get_user_id(self):
-        return self.kwargs['user_id']
+        return self.kwargs[self.user_url_kwarg]
 
     def get_queryset(self):
         self.filtered = False
-        user = get_user(self.request, self.get_user_id())
-        trackers = Tracker.objects.filter(user=user).exclude(type__exact='')
-
+        trackers = Tracker.objects.filter(user__pk=self.get_user_id()).exclude(type__exact='')
         start_date, end_date = self.get_daterange()
-
-        print(start_date)
-        print(end_date)
         trackers = trackers.filter(tracker_date__gte=start_date, tracker_date__lte=end_date)
 
         return trackers.order_by('-tracker_date')
 
     def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
-        context['user'] = get_user(self.request, self.get_user_id())
+        context['user'] = User.objects.get(pk=self.get_user_id())
         context['advanced_search'] = self.filtered
 
         for tracker in context['page_obj'].object_list: