Merge pull request #300 from edx/waheed/ecom-5469-secure-publisher-links

Added changed_by field in all publisher models and decorated all views with login_required.

course_discovery/apps/publisher/forms.py

@@ -37,6 +37,7 @@ class CourseForm(BaseCourseForm):
     class Meta:
         model = Course
         fields = '__all__'
+        exclude = ('changed_by',)
 
 
 class CourseRunForm(BaseCourseForm):
@@ -45,7 +46,7 @@ class CourseRunForm(BaseCourseForm):
     class Meta:
         model = CourseRun
         fields = '__all__'
-        exclude = ('state',)
+        exclude = ('state', 'changed_by',)
 
 
 class SeatForm(BaseCourseForm):
@@ -54,7 +55,7 @@ class SeatForm(BaseCourseForm):
     class Meta:
         model = Seat
         fields = '__all__'
-        exclude = ('currency',)
+        exclude = ('currency', 'changed_by',)
 
     def save(self, commit=True):
         seat = super(SeatForm, self).save(commit=False)

course_discovery/apps/publisher/migrations/0001_initial.py

@@ -44,7 +44,7 @@ class Migration(migrations.Migration):
                 'get_latest_by': 'modified',
                 'abstract': False,
             },
-            bases=(models.Model, course_discovery.apps.publisher.models.ChangedByMixin),
+            bases=(models.Model,),
         ),
         migrations.CreateModel(
             name='CourseRun',
@@ -86,7 +86,7 @@ class Migration(migrations.Migration):
                 'get_latest_by': 'modified',
                 'abstract': False,
             },
-            bases=(models.Model, course_discovery.apps.publisher.models.ChangedByMixin),
+            bases=(models.Model,),
         ),
         migrations.CreateModel(
             name='HistoricalCourse',
@@ -202,6 +202,6 @@ class Migration(migrations.Migration):
                 'get_latest_by': 'modified',
                 'abstract': False,
             },
-            bases=(models.Model, course_discovery.apps.publisher.models.ChangedByMixin),
+            bases=(models.Model,),
         ),
     ]

course_discovery/apps/publisher/migrations/0004_auto_20160810_0854.py

@@ -48,7 +48,7 @@ class Migration(migrations.Migration):
                 'ordering': ('-modified', '-created'),
                 'abstract': False,
             },
-            bases=(models.Model, course_discovery.apps.publisher.models.ChangedByMixin),
+            bases=(models.Model,),
         ),
         migrations.AddField(
             model_name='courserun',

course_discovery/apps/publisher/migrations/0007_auto_20160905_1020.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+from django.conf import settings
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('publisher', '0006_auto_20160902_0726'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='course',
+            name='changed_by',
+            field=models.ForeignKey(to=settings.AUTH_USER_MODEL, null=True, blank=True),
+        ),
+        migrations.AddField(
+            model_name='courserun',
+            name='changed_by',
+            field=models.ForeignKey(to=settings.AUTH_USER_MODEL, null=True, blank=True),
+        ),
+        migrations.AddField(
+            model_name='historicalcourse',
+            name='changed_by',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.DO_NOTHING, to=settings.AUTH_USER_MODEL, db_constraint=False, null=True, blank=True, related_name='+'),
+        ),
+        migrations.AddField(
+            model_name='historicalcourserun',
+            name='changed_by',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.DO_NOTHING, to=settings.AUTH_USER_MODEL, db_constraint=False, null=True, blank=True, related_name='+'),
+        ),
+        migrations.AddField(
+            model_name='historicalseat',
+            name='changed_by',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.DO_NOTHING, to=settings.AUTH_USER_MODEL, db_constraint=False, null=True, blank=True, related_name='+'),
+        ),
+        migrations.AddField(
+            model_name='historicalstate',
+            name='changed_by',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.DO_NOTHING, to=settings.AUTH_USER_MODEL, db_constraint=False, null=True, blank=True, related_name='+'),
+        ),
+        migrations.AddField(
+            model_name='seat',
+            name='changed_by',
+            field=models.ForeignKey(to=settings.AUTH_USER_MODEL, null=True, blank=True),
+        ),
+        migrations.AddField(
+            model_name='state',
+            name='changed_by',
+            field=models.ForeignKey(to=settings.AUTH_USER_MODEL, null=True, blank=True),
+        ),
+    ]

course_discovery/apps/publisher/mixins.py

-from django.http import HttpResponseForbidden
+from django.contrib.auth.decorators import login_required
+from django.http import HttpResponseForbidden, HttpResponseRedirect
+from django.utils.decorators import method_decorator
 
 from course_discovery.apps.publisher.models import Course, Seat
 
@@ -30,5 +32,33 @@ class ViewPermissionMixin(object):
         return super(ViewPermissionMixin, self).dispatch(request, *args, **kwargs)
 
 
+class LoginRequiredMixin(object):
+
+    @method_decorator(login_required)
+    def dispatch(self, request, *args, **kwargs):
+        return super(LoginRequiredMixin, self).dispatch(request, *args, **kwargs)
+
+
+class FormValidMixin(object):
+    change_state = False
+    assign_user_groups = False
+
+    def form_valid(self, form):
+        user = self.request.user
+        publisher_object = form.save(commit=False)
+        publisher_object.changed_by = user
+        publisher_object.save()
+
+        if self.assign_user_groups:
+            publisher_object.assign_user_groups(user)
+
+        if self.change_state:
+            publisher_object.change_state(user=user)
+
+        self.object = publisher_object
+
+        return HttpResponseRedirect(self.get_success_url())
+
+
 def check_view_permission(user, course):
     return user.is_staff or user.has_perm(Course.VIEW_PERMISSION, course)

course_discovery/apps/publisher/models.py

@@ -19,9 +19,12 @@ from course_discovery.apps.ietf_language_tags.models import LanguageTag
 logger = logging.getLogger(__name__)
 
 
-class ChangedByMixin(object):
+class ChangedByMixin(models.Model):
     changed_by = models.ForeignKey(User, null=True, blank=True)
 
+    class Meta:
+        abstract = True
+
 
 class State(TimeStampedModel, ChangedByMixin):
     """ Publisher Workflow State Model. """
@@ -203,7 +206,7 @@ class CourseRun(TimeStampedModel, ChangedByMixin):
     def __str__(self):
         return '{course}: {start_date}'.format(course=self.course.title, start_date=self.start)
 
-    def change_state(self, target=State.DRAFT):
+    def change_state(self, target=State.DRAFT, user=None):
         if target == State.NEEDS_REVIEW:
             self.state.needs_review()
         elif target == State.NEEDS_FINAL_APPROVAL:
@@ -215,6 +218,9 @@ class CourseRun(TimeStampedModel, ChangedByMixin):
         else:
             self.state.draft()
 
+        if user:
+            self.state.changed_by = user
+
         self.state.save()
 
     @property

course_discovery/apps/publisher/tests/test_views.py

@@ -28,6 +28,21 @@ class CreateUpdateCourseViewTests(TestCase):
         self.site = Site.objects.get(pk=settings.SITE_ID)
         self.client.login(username=self.user.username, password=USER_PASSWORD)
 
+    def test_course_form_without_login(self):
+        """ Verify that user can't access new course form page when not logged in. """
+        self.client.logout()
+        response = self.client.get(reverse('publisher:publisher_courses_new'))
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=reverse('publisher:publisher_courses_new')
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
     def test_create_course(self):
         """ Verify that we can create a new course. """
         # Create unique course number
@@ -56,6 +71,7 @@ class CreateUpdateCourseViewTests(TestCase):
         updated_course_title = 'Updated {}'.format(self.course.title)
         course_dict['title'] = updated_course_title
         self.assertNotEqual(self.course.title, updated_course_title)
+        self.assertNotEqual(self.course.changed_by, self.user)
         response = self.client.post(
             reverse('publisher:publisher_courses_edit', kwargs={'pk': self.course.id}),
             course_dict
@@ -71,6 +87,7 @@ class CreateUpdateCourseViewTests(TestCase):
         course = Course.objects.get(id=self.course.id)
         # Assert that course is updated.
         self.assertEqual(course.title, updated_course_title)
+        self.assertEqual(course.changed_by, self.user)
 
         # add new and check the comment on edit page.
         comment = CommentFactory(content_object=self.course, user=self.user, site=self.site)
@@ -166,6 +183,21 @@ class CreateUpdateCourseRunViewTests(TestCase):
         for key in key_list:
             data_dict.pop(key)
 
+    def test_courserun_form_without_login(self):
+        """ Verify that user can't access new course run form page when not logged in. """
+        self.client.logout()
+        response = self.client.get(reverse('publisher:publisher_course_runs_new'))
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=reverse('publisher:publisher_course_runs_new')
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
     def test_create_course_run(self):
         """ Verify that we can create a new course run. """
         lms_course_id = 'course-v1:testX+AS12131+2016_q4'
@@ -191,6 +223,7 @@ class CreateUpdateCourseRunViewTests(TestCase):
         updated_lms_course_id = 'course-v1:testX+AS121+2018_q1'
         self.course_run_dict['lms_course_id'] = updated_lms_course_id
         self.assertNotEqual(self.course_run.lms_course_id, updated_lms_course_id)
+        self.assertNotEqual(self.course_run.changed_by, self.user)
         response = self.client.post(
             reverse('publisher:publisher_course_runs_edit', kwargs={'pk': self.course_run.id}),
             self.course_run_dict
@@ -206,6 +239,7 @@ class CreateUpdateCourseRunViewTests(TestCase):
         course_run = CourseRun.objects.get(id=self.course_run.id)
         # Assert that course run is updated.
         self.assertEqual(course_run.lms_course_id, updated_lms_course_id)
+        self.assertEqual(course_run.changed_by, self.user)
 
         # add new and check the comment on edit page.
         comment = CommentFactory(content_object=self.course_run, user=self.user, site=self.site)
@@ -295,6 +329,21 @@ class SeatsCreateUpdateViewTests(TestCase):
         self.client.login(username=self.user.username, password=USER_PASSWORD)
         self.seat_edit_url = reverse('publisher:publisher_seats_edit', kwargs={'pk': self.seat.id})
 
+    def test_seat_form_without_login(self):
+        """ Verify that user can't access new seat form page when not logged in. """
+        self.client.logout()
+        response = self.client.get(reverse('publisher:publisher_seats_new'))
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=reverse('publisher:publisher_seats_new')
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
     def test_seat_view_page(self):
         """ Verify that we can open new seat page. """
         response = self.client.get(reverse('publisher:publisher_seats_new'))
@@ -323,6 +372,7 @@ class SeatsCreateUpdateViewTests(TestCase):
         self.seat_dict['price'] = updated_seat_price
         self.seat_dict['type'] = Seat.VERIFIED
         self.assertNotEqual(self.seat.price, updated_seat_price)
+        self.assertNotEqual(self.seat.changed_by, self.user)
         response = self.client.post(
             reverse('publisher:publisher_seats_edit', kwargs={'pk': self.seat.id}),
             self.seat_dict
@@ -338,6 +388,7 @@ class SeatsCreateUpdateViewTests(TestCase):
         seat = Seat.objects.get(id=self.seat.id)
         # Assert that seat is updated.
         self.assertEqual(seat.price, updated_seat_price)
+        self.assertEqual(seat.changed_by, self.user)
         self.assertEqual(seat.type, Seat.VERIFIED)
 
         self.seat_dict['type'] = Seat.HONOR
@@ -446,6 +497,21 @@ class CourseRunDetailTests(TestCase):
         self.wrapped_course_run = CourseRunWrapper(self.course_run)
         self.date_format = '%b %d, %Y, %H:%M:%S %p'
 
+    def test_page_without_login(self):
+        """ Verify that user can't access detail page when not logged in. """
+        self.client.logout()
+        response = self.client.get(reverse('publisher:publisher_course_run_detail', args=[self.course_run.id]))
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=reverse('publisher:publisher_course_run_detail', args=[self.course_run.id])
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
     def test_page_without_data_staff(self):
         """ Verify that staff user can access detail page without any data
         available for that course-run.
@@ -634,6 +700,21 @@ class ChangeStateViewTests(TestCase):
         self.page_url = reverse('publisher:publisher_course_run_detail', args=[self.course_run.id])
         self.change_state_url = reverse('publisher:publisher_change_state', args=[self.course_run.id])
 
+    def test_page_without_login(self):
+        """ Verify that user can't access change state endpoint when not logged in. """
+        self.client.logout()
+        response = self.client.post(self.change_state_url, data={'state': State.NEEDS_REVIEW})
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=self.change_state_url
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
     def test_change_state_with_staff(self):
         """ Verify that staff user can change workflow state from detail page. """
         response = self.client.get(self.page_url)
@@ -676,3 +757,34 @@ class ChangeStateViewTests(TestCase):
 
         # assert that state is changed to `NEEDS_REVIEW`
         self.assertContains(response, State.NEEDS_REVIEW.title().replace('_', ' '))
+
+
+class CourseRunListViewTests(TestCase):
+    """ Tests for the `CourseRunListView`. """
+
+    def setUp(self):
+        super(CourseRunListViewTests, self).setUp()
+        self.user = UserFactory(is_staff=True)
+        self.client.login(username=self.user.username, password=USER_PASSWORD)
+        self.page_url = reverse('publisher:publisher_course_runs')
+
+    def test_page_without_login(self):
+        """ Verify that user can't access course runs list page when not logged in. """
+        self.client.logout()
+        response = self.client.get(self.page_url)
+
+        self.assertRedirects(
+            response,
+            expected_url='{url}?next={next}'.format(
+                url=reverse('login'),
+                next=self.page_url
+            ),
+            status_code=302,
+            target_status_code=302
+        )
+
+    def test_page_with_login(self):
+        """ Verify that user can access course runs list page when logged in. """
+        response = self.client.get(self.page_url)
+
+        self.assertEqual(response.status_code, 200)

course_discovery/apps/publisher/views.py

@@ -13,7 +13,7 @@ from django_fsm import TransitionNotAllowed
 from guardian.shortcuts import get_objects_for_user
 
 from course_discovery.apps.publisher.forms import CourseForm, CourseRunForm, SeatForm
-from course_discovery.apps.publisher.mixins import ViewPermissionMixin, check_view_permission
+from course_discovery.apps.publisher import mixins
 from course_discovery.apps.publisher.models import Course, CourseRun, Seat
 from course_discovery.apps.publisher.wrappers import CourseRunWrapper
 
@@ -21,7 +21,7 @@ from course_discovery.apps.publisher.wrappers import CourseRunWrapper
 SEATS_HIDDEN_FIELDS = ['price', 'currency', 'upgrade_deadline', 'credit_provider', 'credit_hours']
 
 
-class CourseRunListView(ListView):
+class CourseRunListView(mixins.LoginRequiredMixin, ListView):
     """ Create Course View."""
     template_name = 'publisher/course_runs_list.html'
 
@@ -35,7 +35,7 @@ class CourseRunListView(ListView):
         return [CourseRunWrapper(course_run) for course_run in course_runs]
 
 
-class CourseRunDetailView(ViewPermissionMixin, DetailView):
+class CourseRunDetailView(mixins.LoginRequiredMixin, mixins.ViewPermissionMixin, DetailView):
     """ Course Run Detail View."""
     model = CourseRun
     template_name = 'publisher/course_run_detail.html'
@@ -48,23 +48,19 @@ class CourseRunDetailView(ViewPermissionMixin, DetailView):
 
 
 # pylint: disable=attribute-defined-outside-init
-class CreateCourseView(CreateView):
+class CreateCourseView(mixins.LoginRequiredMixin, mixins.FormValidMixin, CreateView):
     """ Create Course View."""
     model = Course
     form_class = CourseForm
     template_name = 'publisher/course_form.html'
     success_url = 'publisher:publisher_courses_edit'
-
-    def form_valid(self, form):
-        self.object = form.save()
-        self.object.assign_user_groups(self.request.user)
-        return HttpResponseRedirect(self.get_success_url())
+    assign_user_groups = True
 
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
 
-class UpdateCourseView(ViewPermissionMixin, UpdateView):
+class UpdateCourseView(mixins.LoginRequiredMixin, mixins.ViewPermissionMixin, mixins.FormValidMixin, UpdateView):
     """ Update Course View."""
     model = Course
     form_class = CourseForm
@@ -72,10 +68,6 @@ class UpdateCourseView(ViewPermissionMixin, UpdateView):
     template_name = 'publisher/course_form.html'
     success_url = 'publisher:publisher_courses_edit'
 
-    def form_valid(self, form):
-        self.object = form.save()
-        return HttpResponseRedirect(self.get_success_url())
-
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
@@ -85,28 +77,25 @@ class UpdateCourseView(ViewPermissionMixin, UpdateView):
         return context
 
 
-class CreateCourseRunView(CreateView):
+class CreateCourseRunView(mixins.LoginRequiredMixin, mixins.FormValidMixin, CreateView):
     """ Create Course Run View."""
     model = CourseRun
     form_class = CourseRunForm
     template_name = 'publisher/course_run_form.html'
     success_url = 'publisher:publisher_course_runs_edit'
 
-    def form_valid(self, form):
-        self.object = form.save()
-        return HttpResponseRedirect(self.get_success_url())
-
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
 
-class UpdateCourseRunView(ViewPermissionMixin, UpdateView):
+class UpdateCourseRunView(mixins.LoginRequiredMixin, mixins.ViewPermissionMixin, mixins.FormValidMixin, UpdateView):
     """ Update Course Run View."""
     model = CourseRun
     form_class = CourseRunForm
     permission_required = Course.VIEW_PERMISSION
     template_name = 'publisher/course_run_form.html'
     success_url = 'publisher:publisher_course_runs_edit'
+    change_state = True
 
     def get_context_data(self, **kwargs):
         context = super(UpdateCourseRunView, self).get_context_data(**kwargs)
@@ -116,16 +105,11 @@ class UpdateCourseRunView(ViewPermissionMixin, UpdateView):
         context['comment_object'] = self.object
         return context
 
-    def form_valid(self, form):
-        self.object = form.save()
-        self.object.change_state()
-        return HttpResponseRedirect(self.get_success_url())
-
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
 
-class CreateSeatView(CreateView):
+class CreateSeatView(mixins.LoginRequiredMixin, mixins.FormValidMixin, CreateView):
     """ Create Seat View."""
     model = Seat
     form_class = SeatForm
@@ -137,15 +121,11 @@ class CreateSeatView(CreateView):
         context['hidden_fields'] = SEATS_HIDDEN_FIELDS
         return context
 
-    def form_valid(self, form):
-        self.object = form.save()
-        return HttpResponseRedirect(self.get_success_url())
-
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
 
-class UpdateSeatView(ViewPermissionMixin, UpdateView):
+class UpdateSeatView(mixins.LoginRequiredMixin, mixins.ViewPermissionMixin, mixins.FormValidMixin, UpdateView):
     """ Update Seat View."""
     model = Seat
     form_class = SeatForm
@@ -159,15 +139,11 @@ class UpdateSeatView(ViewPermissionMixin, UpdateView):
         context['comment_object'] = self.object
         return context
 
-    def form_valid(self, form):
-        self.object = form.save()
-        return HttpResponseRedirect(self.get_success_url())
-
     def get_success_url(self):
         return reverse(self.success_url, kwargs={'pk': self.object.id})
 
 
-class ChangeStateView(View):
+class ChangeStateView(mixins.LoginRequiredMixin, View):
     """ Change Workflow State View"""
 
     def post(self, request, course_run_id):
@@ -175,10 +151,10 @@ class ChangeStateView(View):
         try:
             course_run = CourseRun.objects.get(id=course_run_id)
 
-            if not check_view_permission(request.user, course_run.course):
+            if not mixins.check_view_permission(request.user, course_run.course):
                 return HttpResponseForbidden()
 
-            course_run.change_state(target=state)
+            course_run.change_state(target=state, user=self.request.user)
             # pylint: disable=no-member
             messages.success(
                 request, _('Content moved to `{state}` successfully.').format(state=course_run.current_state)