Merge pull request #45 from edx/clintonb/catalog-api

Updated Catalog API

Makefile

@@ -52,7 +52,7 @@ quality:
 	pep8 --config=.pep8 acceptance_tests course_discovery *.py
 	pylint --rcfile=pylintrc acceptance_tests course_discovery *.py
 
-validate: test quality
+validate: quality test
 
 migrate:
 	python manage.py migrate --noinput

course_discovery/apps/api/filters.py

+from dry_rest_permissions.generics import DRYPermissionFiltersBase
+from guardian.shortcuts import get_objects_for_user
+
+
+class PermissionsFilter(DRYPermissionFiltersBase):
+    def filter_list_queryset(self, request, queryset, view):
+        """ Filters the list queryset, returning only the objects accessible by the user. """
+        perm = queryset.model.get_permission('view')
+        return get_objects_for_user(request.user, perm)

course_discovery/apps/api/serializers.py

@@ -6,11 +6,9 @@ from course_discovery.apps.course_metadata.models import Course
 
 
 class CatalogSerializer(serializers.ModelSerializer):
-    url = serializers.HyperlinkedIdentityField(view_name='api:v1:catalog-detail', lookup_field='id')
-
     class Meta(object):
         model = Catalog
-        fields = ('id', 'name', 'query', 'url',)
+        fields = ('id', 'name', 'query', 'courses_count',)
 
 
 class CourseSerializer(serializers.ModelSerializer):

course_discovery/apps/api/tests/test_serializers.py

-from django.core.urlresolvers import reverse
-from django.test import TestCase, RequestFactory
+from django.test import TestCase
 
 from course_discovery.apps.api.serializers import CatalogSerializer, CourseSerializer, ContainedCoursesSerializer
 from course_discovery.apps.catalogs.tests.factories import CatalogFactory
@@ -8,16 +7,15 @@ from course_discovery.apps.course_metadata.tests.factories import CourseFactory
 
 class CatalogSerializerTests(TestCase):
     def test_data(self):
-        catalog = CatalogFactory()
-        path = reverse('api:v1:catalog-detail', kwargs={'id': catalog.id})
-        request = RequestFactory().get(path)
-        serializer = CatalogSerializer(catalog, context={'request': request})
+        catalog = CatalogFactory(query='*:*')  # We intentionally use a query for all Courses.
+        courses = CourseFactory.create_batch(10)
+        serializer = CatalogSerializer(catalog)
 
         expected = {
             'id': catalog.id,
             'name': catalog.name,
             'query': catalog.query,
-            'url': request.build_absolute_uri(),
+            'courses_count': len(courses)
         }
         self.assertDictEqual(serializer.data, expected)
 
@@ -25,9 +23,7 @@ class CatalogSerializerTests(TestCase):
 class CourseSerializerTests(TestCase):
     def test_data(self):
         course = CourseFactory()
-        path = reverse('api:v1:course-detail', kwargs={'key': course.key})
-        request = RequestFactory().get(path)
-        serializer = CourseSerializer(course, context={'request': request})
+        serializer = CourseSerializer(course)
 
         expected = {
             'key': course.key,

course_discovery/apps/api/v1/tests/test_views/mixins.py

+# pylint: disable=redefined-builtin
+
+import json
+
+import responses
+from django.conf import settings
+from rest_framework.test import APIRequestFactory
+
+from course_discovery.apps.api.serializers import CatalogSerializer, CourseSerializer
+
+
+class SerializationMixin(object):
+    def _get_request(self, format=None):
+        query_data = {}
+        if format:
+            query_data['format'] = format
+        return APIRequestFactory().get('/', query_data)
+
+    def _serialize_object(self, serializer, obj, many=False, format=None):
+        return serializer(obj, many=many, context={'request': self._get_request(format)}).data
+
+    def serialize_catalog(self, catalog, many=False, format=None):
+        return self._serialize_object(CatalogSerializer, catalog, many, format)
+
+    def serialize_course(self, course, many=False, format=None):
+        return self._serialize_object(CourseSerializer, course, many, format)
+
+
+class OAuth2Mixin(object):
+    def generate_oauth2_token_header(self, user):
+        """ Generates a Bearer authorization header to simulate OAuth2 authentication. """
+        return 'Bearer {token}'.format(token=user.username)
+
+    def mock_user_info_response(self, user, status=200):
+        """ Mock the user info endpoint response of the OAuth2 provider. """
+
+        data = {
+            'family_name': user.last_name,
+            'preferred_username': user.username,
+            'given_name': user.first_name,
+            'email': user.email,
+        }
+
+        responses.add(
+            responses.GET,
+            settings.EDX_DRF_EXTENSIONS['OAUTH2_USER_INFO_URL'],
+            body=json.dumps(data),
+            content_type='application/json',
+            status=status
+        )

course_discovery/apps/api/v1/tests/test_views.py → course_discovery/apps/api/v1/tests/test_views/test_catalogs.py

-# pylint: disable=redefined-builtin
-import json
+# pylint: disable=redefined-builtin,no-member
 import urllib
 
 import ddt
 import responses
-from django.conf import settings
 from rest_framework.reverse import reverse
-from rest_framework.test import APITestCase, APIRequestFactory
+from rest_framework.test import APITestCase
 
-from course_discovery.apps.api.serializers import CatalogSerializer, CourseSerializer
 from course_discovery.apps.api.tests.jwt_utils import generate_jwt_header_for_user
+from course_discovery.apps.api.v1.tests.test_views.mixins import SerializationMixin, OAuth2Mixin
 from course_discovery.apps.catalogs.models import Catalog
 from course_discovery.apps.catalogs.tests.factories import CatalogFactory
-from course_discovery.apps.core.tests.factories import UserFactory, USER_PASSWORD
+from course_discovery.apps.core.tests.factories import UserFactory
 from course_discovery.apps.core.tests.mixins import ElasticsearchTestMixin
 from course_discovery.apps.course_metadata.tests.factories import CourseFactory
 
 
-class OAuth2Mixin(object):
-    def generate_oauth2_token_header(self, user):
-        """ Generates a Bearer authorization header to simulate OAuth2 authentication. """
-        return 'Bearer {token}'.format(token=user.username)
-
-    def mock_user_info_response(self, user, status=200):
-        """ Mock the user info endpoint response of the OAuth2 provider. """
-
-        data = {
-            'family_name': user.last_name,
-            'preferred_username': user.username,
-            'given_name': user.first_name,
-            'email': user.email,
-        }
-
-        responses.add(
-            responses.GET,
-            settings.EDX_DRF_EXTENSIONS['OAUTH2_USER_INFO_URL'],
-            body=json.dumps(data),
-            content_type='application/json',
-            status=status
-        )
-
-
-class SerializationMixin(object):
-    def _get_request(self, format=None):
-        query_data = {}
-        if format:
-            query_data['format'] = format
-        return APIRequestFactory().get('/', query_data)
-
-    def _serialize_object(self, serializer, obj, many=False, format=None):
-        return serializer(obj, many=many, context={'request': self._get_request(format)}).data
-
-    def serialize_catalog(self, catalog, many=False, format=None):
-        return self._serialize_object(CatalogSerializer, catalog, many, format)
-
-    def serialize_course(self, course, many=False, format=None):
-        return self._serialize_object(CourseSerializer, course, many, format)
-
-
 @ddt.ddt
 class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixin, APITestCase):
-    """ Tests for the catalog resource.
-
-    Read-only (GET) endpoints should NOT require authentication.
-    """
+    """ Tests for the catalog resource. """
 
     def setUp(self):
         super(CatalogViewSetTests, self).setUp()
         self.user = UserFactory(is_staff=True, is_superuser=True)
-        self.client.login(username=self.user.username, password=USER_PASSWORD)
+        self.client.force_authenticate(self.user)
         self.catalog = CatalogFactory(query='title:abc*')
         self.course = CourseFactory(key='a/b/c', title='ABC Test Course')
         self.refresh_index()
 
+    def assert_catalog_created(self, **headers):
+        name = 'The Kitchen Sink'
+        query = '*.*'
+        data = {
+            'name': name,
+            'query': query
+        }
+
+        response = self.client.post(reverse('api:v1:catalog-list'), data, format='json', **headers)
+        self.assertEqual(response.status_code, 201)
+
+        catalog = Catalog.objects.latest()
+        self.assertDictEqual(response.data, self.serialize_catalog(catalog))
+        self.assertEqual(catalog.name, name)
+        self.assertEqual(catalog.query, query)
+
+    def grant_catalog_permission_to_user(self, user, action):
+        """ Grant the user access to view `self.catalog`. """
+        perm = '{action}_catalog'.format(action=action)
+        user.add_obj_perm(perm, self.catalog)
+        self.assertTrue(user.has_perm('catalogs.' + perm, self.catalog))
+
     def test_create_without_authentication(self):
         """ Verify authentication is required when creating, updating, or deleting a catalog. """
         self.client.logout()
@@ -91,22 +67,6 @@ class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixi
         response = getattr(self.client, http_method)(url, {}, format='json')
         self.assertEqual(response.status_code, 403)
 
-    def assert_catalog_created(self, **headers):
-        name = 'The Kitchen Sink'
-        query = '*.*'
-        data = {
-            'name': name,
-            'query': query
-        }
-
-        response = self.client.post(reverse('api:v1:catalog-list'), data, format='json', **headers)
-        self.assertEqual(response.status_code, 201)
-
-        catalog = Catalog.objects.latest()
-        self.assertDictEqual(response.data, self.serialize_catalog(catalog))
-        self.assertEqual(catalog.name, name)
-        self.assertEqual(catalog.query, query)
-
     def test_create_with_session_authentication(self):
         """ Verify the endpoint creates a new catalog when the client is authenticated via session authentication. """
         self.assert_catalog_created()
@@ -199,42 +159,57 @@ class CatalogViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixi
         self.assertEqual(catalog.name, name)
         self.assertEqual(catalog.query, query)
 
+    def test_retrieve_permissions(self):
+        """ Verify only users with the correct permissions can create, read, or modify a Catalog. """
+        # Use an unprivileged user
+        user = UserFactory(is_staff=False, is_superuser=False)
+        self.client.force_authenticate(user)
+        url = reverse('api:v1:catalog-detail', kwargs={'id': self.catalog.id})
 
-@ddt.ddt
-class CourseViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixin, APITestCase):
-    def setUp(self):
-        super(CourseViewSetTests, self).setUp()
-        self.user = UserFactory(is_staff=True, is_superuser=True)
-        self.client.login(username=self.user.username, password=USER_PASSWORD)
+        # A user with no permissions should NOT be able to view a Catalog.
+        self.assertFalse(user.has_perm('catalogs.view_catalog', self.catalog))
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 403)
 
-    @ddt.data('json', 'api')
-    def test_list(self, format):
-        """ Verify the endpoint returns a list of all courses. """
-        courses = CourseFactory.create_batch(10)
-        courses.sort(key=lambda course: course.key.lower())
-        url = reverse('api:v1:course-list')
-        limit = 3
+        # The permitted user should be able to view the Catalog.
+        self.grant_catalog_permission_to_user(user, 'view')
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
 
-        response = self.client.get(url, {'format': format, 'limit': limit})
+    def test_list_permissions(self):
+        """ Verify only catalogs accessible to the user are returned in the list view. """
+        user = UserFactory(is_staff=False, is_superuser=False)
+        self.client.force_authenticate(user)
+        url = reverse('api:v1:catalog-list')
+
+        # An user with no permissions should not see any catalogs
+        response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
-        self.assertListEqual(response.data['results'], self.serialize_course(courses[:limit], many=True, format=format))
+        self.assertListEqual(response.data['results'], [])
 
-        response.render()
+        # The client should be able to see permissions for which it has access
+        self.grant_catalog_permission_to_user(user, 'view')
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        self.assertListEqual(response.data['results'], self.serialize_catalog([self.catalog], many=True))
 
-    def test_retrieve(self):
-        """ Verify the endpoint returns a single course. """
-        self.assert_retrieve_success()
+    def test_write_permissions(self):
+        """ Verify only authorized users can update or delete Catalogs. """
+        url = reverse('api:v1:catalog-detail', kwargs={'id': self.catalog.id})
+        user = UserFactory(is_staff=False, is_superuser=False)
+        self.client.force_authenticate(user)
 
-    def assert_retrieve_success(self, **headers):
-        """ Asserts the endpoint returns details for a single course. """
-        course = CourseFactory()
-        url = reverse('api:v1:course-detail', kwargs={'key': course.key})
-        response = self.client.get(url, format='json', **headers)
+        # Unprivileged users cannot modify Catalogs
+        response = self.client.put(url)
+        self.assertEqual(response.status_code, 403)
+        response = self.client.delete(url)
+        self.assertEqual(response.status_code, 403)
+
+        # With the right permissions, the user can perform the specified actions
+        self.grant_catalog_permission_to_user(user, 'change')
+        response = self.client.patch(url, {'query': '*:*'})
         self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.data, self.serialize_course(course))
 
-    @responses.activate
-    def test_retrieve_with_oauth2_authentication(self):
-        self.client.logout()
-        self.mock_user_info_response(self.user)
-        self.assert_retrieve_success(HTTP_AUTHORIZATION=self.generate_oauth2_token_header(self.user))
+        self.grant_catalog_permission_to_user(user, 'delete')
+        response = self.client.delete(url)
+        self.assertEqual(response.status_code, 204)

course_discovery/apps/api/v1/tests/test_views/test_courses.py

+# pylint: disable=redefined-builtin
+
+import ddt
+import responses
+from rest_framework.reverse import reverse
+from rest_framework.test import APITestCase
+
+from course_discovery.apps.api.v1.tests.test_views.mixins import SerializationMixin, OAuth2Mixin
+from course_discovery.apps.core.tests.factories import UserFactory, USER_PASSWORD
+from course_discovery.apps.core.tests.mixins import ElasticsearchTestMixin
+from course_discovery.apps.course_metadata.tests.factories import CourseFactory
+
+
+@ddt.ddt
+class CourseViewSetTests(ElasticsearchTestMixin, SerializationMixin, OAuth2Mixin, APITestCase):
+    def setUp(self):
+        super(CourseViewSetTests, self).setUp()
+        self.user = UserFactory(is_staff=True, is_superuser=True)
+        self.client.login(username=self.user.username, password=USER_PASSWORD)
+
+    @ddt.data('json', 'api')
+    def test_list(self, format):
+        """ Verify the endpoint returns a list of all courses. """
+        courses = CourseFactory.create_batch(10)
+        courses.sort(key=lambda course: course.key.lower())
+        url = reverse('api:v1:course-list')
+        limit = 3
+
+        response = self.client.get(url, {'format': format, 'limit': limit})
+        self.assertEqual(response.status_code, 200)
+        self.assertListEqual(response.data['results'], self.serialize_course(courses[:limit], many=True, format=format))
+
+        response.render()
+
+    def test_retrieve(self):
+        """ Verify the endpoint returns a single course. """
+        self.assert_retrieve_success()
+
+    def assert_retrieve_success(self, **headers):
+        """ Asserts the endpoint returns details for a single course. """
+        course = CourseFactory()
+        url = reverse('api:v1:course-detail', kwargs={'key': course.key})
+        response = self.client.get(url, format='json', **headers)
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, self.serialize_course(course))
+
+    @responses.activate
+    def test_retrieve_with_oauth2_authentication(self):
+        self.client.logout()
+        self.mock_user_info_response(self.user)
+        self.assert_retrieve_success(HTTP_AUTHORIZATION=self.generate_oauth2_token_header(self.user))

course_discovery/apps/api/v1/views.py

 import logging
 
 from django.db.models.functions import Lower
+from dry_rest_permissions.generics import DRYPermissions
 from rest_framework import viewsets
 from rest_framework.decorators import detail_route
 from rest_framework.permissions import IsAuthenticated
 from rest_framework.response import Response
 
+from course_discovery.apps.api.filters import PermissionsFilter
 from course_discovery.apps.api.serializers import CatalogSerializer, CourseSerializer, ContainedCoursesSerializer
 from course_discovery.apps.catalogs.models import Catalog
 from course_discovery.apps.course_metadata.constants import COURSE_ID_REGEX
@@ -18,7 +20,9 @@ logger = logging.getLogger(__name__)
 class CatalogViewSet(viewsets.ModelViewSet):
     """ Catalog resource. """
 
+    filter_backends = (PermissionsFilter,)
     lookup_field = 'id'
+    permission_classes = (DRYPermissions,)
     queryset = Catalog.objects.all()
     serializer_class = CatalogSerializer
 

course_discovery/apps/catalogs/admin.py

 from django.contrib import admin
+from guardian.admin import GuardedModelAdmin
 
 from course_discovery.apps.catalogs.models import Catalog
 
 
 @admin.register(Catalog)
-class CatalogAdmin(admin.ModelAdmin):
+class CatalogAdmin(GuardedModelAdmin):
     list_display = ('name',)
     readonly_fields = ('created', 'modified',)
 

course_discovery/apps/catalogs/migrations/0002_auto_20160327_2101.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('catalogs', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.AlterModelOptions(
+            name='catalog',
+            options={'ordering': ('-modified', '-created'), 'permissions': (('view_catalog', 'Can view catalog'),), 'get_latest_by': 'modified'},
+        ),
+    ]

course_discovery/apps/catalogs/models.py

@@ -3,10 +3,11 @@ from django.utils.translation import ugettext_lazy as _
 from django_extensions.db.models import TimeStampedModel
 from haystack.query import SearchQuerySet
 
+from course_discovery.apps.core.mixins import ModelPermissionsMixin
 from course_discovery.apps.course_metadata.models import Course
 
 
-class Catalog(TimeStampedModel):
+class Catalog(ModelPermissionsMixin, TimeStampedModel):
     name = models.CharField(max_length=255, null=False, blank=False, help_text=_('Catalog name'))
     query = models.TextField(null=False, blank=False, help_text=_('Query to retrieve catalog contents'))
 
@@ -31,6 +32,10 @@ class Catalog(TimeStampedModel):
         results = self._get_query_results().load_all()
         return [result.object for result in results]
 
+    @property
+    def courses_count(self):
+        return self._get_query_results().count()
+
     def contains(self, course_ids):  # pylint: disable=unused-argument
         """ Determines if the given courses are contained in this catalog.
 
@@ -47,3 +52,9 @@ class Catalog(TimeStampedModel):
             contains[result.get_stored_fields()['key']] = True
 
         return contains
+
+    class Meta(TimeStampedModel.Meta):
+        abstract = False
+        permissions = (
+            ('view_catalog', 'Can view catalog'),
+        )

course_discovery/apps/catalogs/tests/factories.py

@@ -9,4 +9,4 @@ class CatalogFactory(factory.DjangoModelFactory):
         model = Catalog
 
     name = FuzzyText(prefix='catalog-name-')
-    query = '{"query": {"match_all": {}}}'
+    query = '*:*'

course_discovery/apps/catalogs/tests/test_models.py

@@ -34,3 +34,12 @@ class CatalogTests(ElasticsearchTestMixin, TestCase):
             self.catalog.contains([self.course.key, uncontained_course.key]),
             {self.course.key: True, uncontained_course.key: False}
         )
+
+    def test_courses_count(self):
+        """ Verify the method returns the number of courses contained in the Catalog. """
+        self.assertEqual(self.catalog.courses_count, 1)
+
+        # Create a new course that should NOT be contained in the catalog, and one that should
+        CourseFactory()
+        CourseFactory(title='ABCDEF')
+        self.assertEqual(self.catalog.courses_count, 2)

course_discovery/apps/core/mixins.py

+from dry_rest_permissions.generics import allow_staff_or_superuser, authenticated_users
+
+
+class ModelPermissionsMixin:
+    """ Adds DRY permissions to a model.
+
+    Inheriting models should have the default add, change, and delete permissions, as well as the
+    custom "view" permission.
+    """
+    @classmethod
+    def get_permission(cls, action):
+        """
+        Returns a permission name for the given class and action.
+
+        Arguments:
+            action (str): Action tied to the desired permission (e.g. add, change, delete).
+
+        Returns:
+            str: Permission
+        """
+        kwargs = {
+            'app_label': cls._meta.app_label,
+            'model_name': cls._meta.model_name,
+            'action': action
+        }
+        return '{app_label}.{action}_{model_name}'.format(**kwargs)
+
+    @staticmethod
+    @authenticated_users
+    def has_read_permission(_request):
+        return True
+
+    @staticmethod
+    @authenticated_users
+    @allow_staff_or_superuser
+    def has_write_permission(_request):
+        # This is only here to get past the global has_permission check. The object permissions will determine
+        # if a specific instance can be updated.
+        return True
+
+    @classmethod
+    def has_create_permission(cls, request):
+        user = request.user
+        perm = cls.get_permission('add')
+        return user.is_staff or user.is_superuser or user.has_perm(perm)
+
+    @authenticated_users
+    @allow_staff_or_superuser
+    def has_object_destroy_permission(self, request):
+        perm = self.get_permission('delete')
+        return request.user.has_perm(perm, self)
+
+    @authenticated_users
+    @allow_staff_or_superuser
+    def has_object_read_permission(self, request):
+        perm = self.get_permission('view')
+        return request.user.has_perm(perm, self)
+
+    @authenticated_users
+    @allow_staff_or_superuser
+    def has_object_update_permission(self, request):
+        perm = self.get_permission('change')
+        return request.user.has_perm(perm, self)

course_discovery/apps/core/models.py

@@ -3,9 +3,10 @@
 from django.contrib.auth.models import AbstractUser
 from django.db import models
 from django.utils.translation import ugettext_lazy as _
+from guardian.mixins import GuardianUserMixin
 
 
-class User(AbstractUser):
+class User(GuardianUserMixin, AbstractUser):
     """Custom user model for use with OpenID Connect."""
     full_name = models.CharField(_('Full Name'), max_length=255, blank=True, null=True)
 

course_discovery/settings/base.py

@@ -36,6 +36,8 @@ THIRD_PARTY_APPS = (
     'sortedm2m',
     'simple_history',
     'haystack',
+    'guardian',
+    'dry_rest_permissions',
 )
 
 PROJECT_APPS = (
@@ -166,8 +168,13 @@ AUTH_USER_MODEL = 'core.User'
 AUTHENTICATION_BACKENDS = (
     'auth_backends.backends.EdXOpenIdConnect',
     'django.contrib.auth.backends.ModelBackend',
+    'guardian.backends.ObjectPermissionBackend',
 )
 
+# Guardian settings
+ANONYMOUS_USER_NAME = None  # Do not allow anonymous user access
+GUARDIAN_MONKEY_PATCH = False  # Use the mixin on the User model instead of monkey-patching.
+
 ENABLE_AUTO_AUTH = False
 AUTO_AUTH_USERNAME_PREFIX = 'auto_auth_'
 

requirements/base.txt

 django==1.8.7
 django-extensions==1.5.9
+django-guardian==1.4.2
 django-haystack==2.4.1
 django-simple-history==1.8.1
 django-sortedm2m==1.1.1
@@ -7,6 +8,7 @@ django-waffle==0.11
 djangorestframework==3.3.1
 djangorestframework-jwt==1.7.2
 django-rest-swagger[reST]==0.3.4
+dry-rest-permissions==0.1.6
 edx-auth-backends==0.1.3
 edx-drf-extensions==0.2.0
 edx-rest-api-client==1.5.0