From 7cc854f0ed9c21361d06fd5314dc34ce81532d59 Mon Sep 17 00:00:00 2001
From: janmax <j.michal@stud.uni-goettingen.de>
Date: Mon, 27 Nov 2017 21:34:34 +0100
Subject: [PATCH] Fixed viewset permissions and added logging to permission
base
* IT waS A FUCKIng tyPO
* ran isort and flake8 and fixed reported issues
---
backend/core/models.py | 3 +-
backend/core/permissions.py | 31 +++++++++----
backend/core/tests/data_factories.py | 1 +
backend/core/tests/test_access_rights.py | 45 +++++++++++++++++--
backend/core/tests/test_auth.py | 4 +-
backend/core/tests/test_examlist.py | 3 +-
.../core/tests/test_tutor_api_endpoints.py | 5 +--
backend/core/urls.py | 2 +-
backend/core/views.py | 8 +---
backend/grady/settings/default.py | 15 +++----
10 files changed, 80 insertions(+), 37 deletions(-)
diff --git a/backend/core/models.py b/backend/core/models.py
index b7c7f3eb..04d8d10d 100644
--- a/backend/core/models.py
+++ b/backend/core/models.py
@@ -7,8 +7,7 @@ database.
'''
from collections import OrderedDict
-from random import randrange, sample
-from string import ascii_lowercase
+from random import randrange
from typing import Dict, Union
from django.contrib.auth import get_user_model
diff --git a/backend/core/permissions.py b/backend/core/permissions.py
index 564a93c6..e9b87940 100644
--- a/backend/core/permissions.py
+++ b/backend/core/permissions.py
@@ -1,37 +1,50 @@
+import logging
+
from django.http import HttpRequest
from django.views import View
from rest_framework import permissions
from core.models import Reviewer, Student, Tutor
+log = logging.getLogger(__name__)
+
class IsUserGenericPermission(permissions.BasePermission):
""" Generic class that encapsulates how to identify someone
as a member of a user Group """
def has_permission(self, request: HttpRequest, view: View) -> bool:
- """ required by BasePermission. Check if user is instance of model"""
- assert self.model is not None, (
- "'%s' has to include a `model` attribute"
+ """ required by BasePermission. Check if user is instance of any
+ of the models provided in class' models attribute """
+ log.warn("Checking permission of request %s on view %s for user %s",
+ request, view, request.user)
+
+ assert self.models is not None, (
+ "'%s' has to include a `models` attribute"
% self.__class__.__name__
)
user = request.user
- return user.is_authenticated() and isinstance(
- user.get_associated_user(), self.model
- )
+ is_authorized = user.is_authenticated() and any(isinstance(
+ user.get_associated_user(), models) for models in self.models)
+
+ if not is_authorized:
+ log.warn('User %s has no permission to view %s',
+ user.username, view.__class__.__name__)
+
+ return is_authorized
class IsStudent(IsUserGenericPermission):
""" Has student permissions """
- model = Student
+ models = (Student,)
class IsReviewer(IsUserGenericPermission):
""" Has reviewer permissions """
- model = Reviewer
+ models = (Reviewer,)
class IsTutor(IsUserGenericPermission):
""" Has tutor permissions """
- model = Tutor
+ models = (Tutor,)
diff --git a/backend/core/tests/data_factories.py b/backend/core/tests/data_factories.py
index 25e5ca71..f2f8b4f5 100644
--- a/backend/core/tests/data_factories.py
+++ b/backend/core/tests/data_factories.py
@@ -7,6 +7,7 @@ from core.models import (ExamType, Feedback, Reviewer, Student, Submission,
# These methods are meant to be used to provide data to insert into the test
# database
+
def make_user(username='user01',
password='p',
fullname='us er01',
diff --git a/backend/core/tests/test_access_rights.py b/backend/core/tests/test_access_rights.py
index 51987f53..6e172626 100644
--- a/backend/core/tests/test_access_rights.py
+++ b/backend/core/tests/test_access_rights.py
@@ -3,7 +3,8 @@ from rest_framework import status
from rest_framework.test import (APIRequestFactory, APITestCase,
force_authenticate)
-from core.views import StudentSelfApiViewSet, TutorApiViewSet
+from core.views import (ExamApiViewSet, StudentReviewerApiViewSet,
+ StudentSelfApiViewSet, TutorApiViewSet)
from util.factories import GradyUserFactory
@@ -44,7 +45,8 @@ class AccessRightsOfStudentAPIViewTests(APITestCase):
class AccessRightsOfTutorAPIViewTests(APITestCase):
- """ Tests to ensure that only Reviewers have access to the TutorList information"""
+ """ Tests to ensure that only Reviewers have access to the TutorList
+ information """
@classmethod
def setUpTestData(cls):
cls.factory = APIRequestFactory()
@@ -78,7 +80,8 @@ class AccessRightsOfTutorAPIViewTests(APITestCase):
class AccessRightsOfStudentReviewerAPIViewTest(APITestCase):
- """ Tests to ensure that only Reviewers have access to the StudentReviewerApi endpoint information"""
+ """ Tests to ensure that only Reviewers have access to the
+ StudentReviewerApi endpoint information"""
@classmethod
def setUpTestData(cls):
@@ -90,7 +93,7 @@ class AccessRightsOfStudentReviewerAPIViewTest(APITestCase):
self.tutor = self.user_factory.make_tutor()
self.reviewer = self.user_factory.make_reviewer()
self.request = self.factory.get(reverse('student-list'))
- self.view = TutorApiViewSet.as_view({'get': 'list'})
+ self.view = StudentReviewerApiViewSet.as_view({'get': 'list'})
def test_unauthenticated_access_denied(self):
response = self.view(self.request)
@@ -110,3 +113,37 @@ class AccessRightsOfStudentReviewerAPIViewTest(APITestCase):
force_authenticate(self.request, user=self.reviewer.user)
response = self.view(self.request)
self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+
+class AccessRightsOfExamTypeAPIViewTest(APITestCase):
+ """ Tests who can access the exam list. The rational here is, that this
+ list contains information about what number of points was necessary to pass
+ the exam. There is no reason why anyone should see this information except
+ for their own module. """
+
+ @classmethod
+ def setUpTestData(cls):
+ cls.factory = APIRequestFactory()
+ cls.user_factory = GradyUserFactory()
+
+ def setUp(self):
+ self.student = self.user_factory.make_student()
+ self.tutor = self.user_factory.make_tutor()
+ self.reviewer = self.user_factory.make_reviewer()
+ self.request = self.factory.get(reverse('examtype-list'))
+ self.view = ExamApiViewSet.as_view({'get': 'list'})
+
+ def test_student_has_no_access(self):
+ force_authenticate(self.request, user=self.student.user)
+ response = self.view(self.request)
+ self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+
+ def test_tutor_has_no_access(self):
+ force_authenticate(self.request, user=self.tutor.user)
+ response = self.view(self.request)
+ self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+
+ def test_reviewer_has_access(self):
+ force_authenticate(self.request, user=self.reviewer.user)
+ response = self.view(self.request)
+ self.assertEqual(response.status_code, status.HTTP_200_OK)
diff --git a/backend/core/tests/test_auth.py b/backend/core/tests/test_auth.py
index 85505585..d9af01e8 100644
--- a/backend/core/tests/test_auth.py
+++ b/backend/core/tests/test_auth.py
@@ -4,10 +4,12 @@ from core.models import UserAccount
class AuthTests(APITestCase):
+
@classmethod
def setUpTestData(cls):
cls.credentials = {'username': 'user', 'password': 'p'}
- cls.user = UserAccount.objects.create(username=cls.credentials['username'])
+ cls.user = UserAccount.objects.create(
+ username=cls.credentials['username'])
cls.user.set_password(cls.credentials['password'])
cls.user.save()
cls.client = APIClient()
diff --git a/backend/core/tests/test_examlist.py b/backend/core/tests/test_examlist.py
index 67b3c84d..9443c9a5 100644
--- a/backend/core/tests/test_examlist.py
+++ b/backend/core/tests/test_examlist.py
@@ -19,7 +19,8 @@ class ExamListTest(APITestCase):
def setUp(self):
self.request = self.factory.get(reverse('examtype-list'))
- force_authenticate(self.request, self.user_factory.make_student().user)
+ force_authenticate(self.request,
+ self.user_factory.make_reviewer().user)
self.view = ExamApiViewSet.as_view({'get': 'list'})
self.response = self.view(self.request)
diff --git a/backend/core/tests/test_tutor_api_endpoints.py b/backend/core/tests/test_tutor_api_endpoints.py
index 6f65aade..deef555a 100644
--- a/backend/core/tests/test_tutor_api_endpoints.py
+++ b/backend/core/tests/test_tutor_api_endpoints.py
@@ -4,15 +4,12 @@
* POST /tutor/:username/:email create a new tutor and email password
* GET /tutorlist list of all tutors with their scores
"""
-import logging as log
-from unittest import skip
-
from rest_framework import status
from rest_framework.reverse import reverse
from rest_framework.test import (APIClient, APIRequestFactory, APITestCase,
force_authenticate)
-from core.models import Feedback, Reviewer, Tutor
+from core.models import Feedback, Tutor
from core.views import TutorApiViewSet
from util.factories import GradyUserFactory
diff --git a/backend/core/urls.py b/backend/core/urls.py
index a5d9818d..490d9f23 100644
--- a/backend/core/urls.py
+++ b/backend/core/urls.py
@@ -7,10 +7,10 @@ from core import views
# Create a router and register our viewsets with it.
router = DefaultRouter()
+router.register(r'student', views.StudentReviewerApiViewSet)
router.register(r'examtype', views.ExamApiViewSet)
router.register(r'submissiontype', views.SubmissionTypeApiView)
router.register(r'tutor', views.TutorApiViewSet)
-router.register(r'student', views.StudentReviewerApiViewSet)
router.register(r'student-page', views.StudentSelfApiViewSet, base_name='student_page')
urlpatterns = [
diff --git a/backend/core/views.py b/backend/core/views.py
index 084a4a3e..a0ebac92 100644
--- a/backend/core/views.py
+++ b/backend/core/views.py
@@ -1,8 +1,6 @@
""" All API views that are used to retrieve data from the database. They
can be categorized by the permissions they require. All views require a
user to be authenticated and most are only accessible by one user group """
-import logging
-
from rest_framework import mixins, viewsets
from core.models import ExamType, Student, SubmissionType, Tutor
@@ -11,8 +9,6 @@ from core.serializers import (ExamSerializer, StudentSerializer,
StudentSerializerForListView,
SubmissionTypeSerializer, TutorSerializer)
-log = logging.getLogger(__name__)
-
class StudentSelfApiViewSet(viewsets.ReadOnlyModelViewSet):
""" Gets all data that belongs to one student """
@@ -28,7 +24,7 @@ class StudentSelfApiViewSet(viewsets.ReadOnlyModelViewSet):
class ExamApiViewSet(viewsets.ReadOnlyModelViewSet):
""" Gets a list of an individual exam by Id if provided """
- permissions_classes = (IsReviewer,)
+ permission_classes = (IsReviewer,)
queryset = ExamType.objects.all()
serializer_class = ExamSerializer
@@ -39,7 +35,7 @@ class TutorApiViewSet(mixins.RetrieveModelMixin,
mixins.ListModelMixin,
viewsets.GenericViewSet):
""" Api endpoint for creating, listing, viewing or deleteing tutors """
- permissions_classes = (IsReviewer,)
+ permission_classes = (IsReviewer,)
queryset = Tutor.objects.all()
serializer_class = TutorSerializer
lookup_field = 'user__username'
diff --git a/backend/grady/settings/default.py b/backend/grady/settings/default.py
index a4a5fd8e..f839d3ef 100644
--- a/backend/grady/settings/default.py
+++ b/backend/grady/settings/default.py
@@ -40,9 +40,9 @@ INSTALLED_APPS = [
'django.contrib.messages',
'django.contrib.staticfiles',
'django_extensions',
- 'core',
'rest_framework',
'corsheaders',
+ 'core',
]
MIDDLEWARE = [
@@ -119,8 +119,8 @@ GRAPH_MODELS = {
'group_models': True,
}
-LOGIN_REDIRECT_URL = '/'
-LOGIN_URL = '/'
+LOGIN_REDIRECT_URL = '/'
+LOGIN_URL = '/'
MESSAGE_TAGS = {
@@ -131,9 +131,6 @@ MESSAGE_TAGS = {
messages.ERROR: 'alert-danger',
}
-COMPRESS_ENABLED = False
-COMPRESS_OFFLINE = True
-
STATICFILES_FINDERS = (
'django.contrib.staticfiles.finders.FileSystemFinder',
'django.contrib.staticfiles.finders.AppDirectoriesFinder',
@@ -190,16 +187,16 @@ LOGGING = {
'django': {
'level': 'INFO',
'class': 'logging.StreamHandler',
- 'formatter': 'django.server'
+ 'formatter': 'core'
},
- 'mail_admins': { # TODO: configuration
+ 'mail_admins': { # TODO: configuration
'level': 'ERROR',
'class': 'django.utils.log.AdminEmailHandler',
}
},
'loggers': {
'django': {
- 'handlers': ['django'],
+ 'handlers': [],
},
'django.request': {
'handlers': ['django'],
--
GitLab