Allow users to reject their own blocklist submission (#13622)

* Allow users to reject their own blocklist submission

* _signoff to _approve

* add to test to verify Reject button is there for own submissions

src/olympia/blocklist/admin.py

@@ -144,17 +144,18 @@ class BlockSubmissionAdmin(admin.ModelAdmin):
         we need to return true if the user has sign-off permission instead.
         We can override that permissive behavior with `strict=True`."""
         change_perm = super().has_change_permission(request, obj=obj)
-        signoff_perm = self.has_signoff_permission(request, obj=obj)
-        either_perm = change_perm or (signoff_perm and not strict)
+        approve_perm = self.has_signoff_approve_permission(request, obj=obj)
+        either_perm = change_perm or (approve_perm and not strict)
         return either_perm and (not obj or self.is_pending_signoff(obj))
 
     def has_view_permission(self, request, obj=None):
         return (
             super().has_view_permission(request, obj) or
-            self.has_signoff_permission(request, obj))
+            self.has_signoff_approve_permission(request, obj) or
+            self.has_signoff_reject_permission(request, obj))
 
-    def has_signoff_permission(self, request, obj=None):
-        """ This controls whether the sign-off approve and reject actions are
+    def has_signoff_approve_permission(self, request, obj=None):
+        """ This controls whether the sign-off approve action is
         available on the change form.  `BlockSubmission.can_user_signoff`
         confirms the current user, who will signoff, is different from the user
         who submitted the guids (unless settings.DEBUG is True when the check
@@ -164,6 +165,16 @@ class BlockSubmissionAdmin(admin.ModelAdmin):
         has_perm = request.user.has_perm("%s.%s" % (opts.app_label, codename))
         return has_perm and (not obj or obj.can_user_signoff(request.user))
 
+    def has_signoff_reject_permission(self, request, obj=None):
+        """ This controls whether the sign-off reject action is
+        available on the change form.  Users can reject their own submission
+        regardless."""
+        opts = self.opts
+        codename = auth.get_permission_codename('signoff', opts)
+        has_perm = request.user.has_perm("%s.%s" % (opts.app_label, codename))
+        is_own_submission = obj and obj.updated_by == request.user
+        return has_perm or is_own_submission
+
     def get_fieldsets(self, request, obj):
         input_guids = (
             'Input Guids', {
@@ -356,8 +367,10 @@ class BlockSubmissionAdmin(admin.ModelAdmin):
     def change_view(self, request, object_id, form_url='', extra_context=None):
         extra_context = extra_context or {}
         obj = self.model.objects.filter(id=object_id).latest()
-        extra_context['has_signoff_permission'] = self.has_signoff_permission(
-            request, obj)
+        extra_context['has_signoff_approve_permission'] = (
+            self.has_signoff_approve_permission(request, obj))
+        extra_context['has_signoff_reject_permission'] = (
+            self.has_signoff_reject_permission(request, obj))
         extra_context['can_change_object'] = (
             obj.action == BlockSubmission.ACTION_ADDCHANGE and
             self.has_change_permission(request, obj, strict=True))
@@ -387,15 +400,16 @@ class BlockSubmissionAdmin(admin.ModelAdmin):
 
     def save_model(self, request, obj, form, change):
         if change and self.is_pending_signoff(obj):
-            is_signoff = '_signoff' in request.POST
+            is_approve = '_approve' in request.POST
             is_reject = '_reject' in request.POST
-            if ((is_signoff or is_reject) and
-                    not self.has_signoff_permission(request, obj)):
-                raise PermissionDenied
-            if is_signoff:
+            if is_approve:
+                if not self.has_signoff_approve_permission(request, obj):
+                    raise PermissionDenied
                 obj.signoff_state = BlockSubmission.SIGNOFF_APPROVED
                 obj.signoff_by = request.user
             elif is_reject:
+                if not self.has_signoff_reject_permission(request, obj):
+                    raise PermissionDenied
                 obj.signoff_state = BlockSubmission.SIGNOFF_REJECTED
             elif not self.has_change_permission(request, obj, strict=True):
                 # users without full change permission should only do signoff
@@ -411,9 +425,9 @@ class BlockSubmissionAdmin(admin.ModelAdmin):
 
     def log_change(self, request, obj, message):
         log_entry = None
-        is_signoff = '_signoff' in request.POST
+        is_approve = '_approve' in request.POST
         is_reject = '_reject' in request.POST
-        if is_signoff:
+        if is_approve:
             signoff_msg = 'Sign-off Approval'
         elif is_reject:
             signoff_msg = 'Sign-off Rejection'

src/olympia/blocklist/templates/blocklist/block_submission_change_form.html

@@ -10,9 +10,11 @@
     {% if can_change_object and change %}
         <input type="submit" value="{% trans 'Update' %}" class="default" name="_save">
     {% endif %}
-    {% if has_signoff_permission and is_pending_signoff %}
+    {% if has_signoff_reject_permission and is_pending_signoff %}
       <input type="submit" value="{% trans 'Reject Submission' %}" name="_reject">
-      <input type="submit" value="{% trans 'Approve Submission' %}" name="_signoff">
+    {% endif %}
+    {% if has_signoff_approve_permission and is_pending_signoff %}
+      <input type="submit" value="{% trans 'Approve Submission' %}" name="_approve">
     {% endif %}
   </div>
 {% endblock %}

src/olympia/blocklist/tests/test_admin.py

@@ -908,7 +908,7 @@ class TestBlockSubmissionAdmin(TestCase):
                 'max_version': '99',  # should be ignored
                 'url': 'new.url',  # should be ignored
                 'reason': 'a reason',  # should be ignored
-                '_signoff': 'Approve Submission',
+                '_approve': 'Approve Submission',
             },
             follow=True)
         assert response.status_code == 200
@@ -1047,7 +1047,7 @@ class TestBlockSubmissionAdmin(TestCase):
                 'max_version': '99',  # should be ignored
                 'url': 'new.url',  # could be updated with this permission
                 'reason': 'a reason',  # could be updated with this permission
-                '_signoff': 'Approve Submission',
+                '_approve': 'Approve Submission',
             },
             follow=True)
         assert response.status_code == 403
@@ -1059,8 +1059,68 @@ class TestBlockSubmissionAdmin(TestCase):
         assert mbs.url != 'new.url'
         assert mbs.reason != 'a reason'
 
-    def test_cannot_reject_with_only_block_create_permission(self):
-        pass
+    def test_can_only_reject_your_own_with_only_block_create_permission(self):
+        addon = addon_factory(guid='guid@', name='Danger Danger')
+        submission = BlockSubmission.objects.create(
+            input_guids='guid@\ninvalid@',
+            updated_by=user_factory())
+        assert submission.to_block == [
+            {'guid': 'guid@',
+             'id': None,
+             'average_daily_users': addon.average_daily_users}]
+
+        user = user_factory()
+        self.grant_permission(user, 'Admin:Tools')
+        self.grant_permission(user, 'Blocklist:Create')
+        self.client.login(email=user.email)
+        change_url = reverse(
+            'admin:blocklist_blocksubmission_change', args=(submission.id,))
+        response = self.client.post(
+            change_url, {
+                'input_guids': 'guid2@\nfoo@baa',  # should be ignored
+                'min_version': '1',  # should be ignored
+                'max_version': '99',  # should be ignored
+                'url': 'new.url',  # could be updated with this permission
+                'reason': 'a reason',  # could be updated with this permission
+                '_reject': 'Reject Submission',
+            },
+            follow=True)
+        assert response.status_code == 403
+        submission = submission.reload()
+        # It wasn't signed off
+        assert not submission.signoff_by
+        assert submission.signoff_state == BlockSubmission.SIGNOFF_PENDING
+        # And the details weren't updated either
+        assert submission.url != 'new.url'
+        assert submission.reason != 'a reason'
+
+        # except if it's your own submission
+        submission.update(updated_by=user)
+        response = self.client.get(change_url, follow=True)
+        assert response.status_code == 200
+        doc = pq(response.content)
+        buttons = doc('.submit-row input')
+        assert buttons[0].attrib['value'] == 'Update'
+        assert buttons[1].attrib['value'] == 'Reject Submission'
+        assert len(buttons) == 2
+        assert b'Approve Submission' not in response.content
+
+        response = self.client.post(
+            change_url, {
+                'input_guids': 'guid2@\nfoo@baa',  # should be ignored
+                'min_version': '1',  # should be ignored
+                'max_version': '99',  # should be ignored
+                'url': 'new.url',  # could be updated with this permission
+                'reason': 'a reason',  # could be updated with this permission
+                '_reject': 'Reject Submission',
+            },
+            follow=True)
+        assert response.status_code == 200
+        submission = submission.reload()
+        assert submission.signoff_state == BlockSubmission.SIGNOFF_REJECTED
+        assert not submission.signoff_by
+        assert submission.url == 'new.url'
+        assert submission.reason == 'a reason'
 
     def test_signed_off_view(self):
         addon = addon_factory(guid='guid@', name='Danger Danger')