Merge pull request #597 from opensafely-core/permissions-3

Refactor permissions/policies for reviewing files

airlock/business_logic.py

@@ -1973,39 +1973,15 @@ def submit_request(self, request: ReleaseRequest, user: User):
         self.set_status(request, RequestStatus.SUBMITTED, user)
         self._dal.start_new_turn(request.id)
 
-    def _verify_permission_to_review_file(
-        self, release_request: ReleaseRequest, relpath: UrlPath, user: User
-    ):
-        if self.STATUS_OWNERS[release_request.status] != RequestStatusOwner.REVIEWER:
-            raise exceptions.ApprovalPermissionDenied(
-                f"cannot review file from request in state {release_request.status.name}"
-            )
-
-        if user.username == release_request.author:
-            raise exceptions.ApprovalPermissionDenied(
-                "cannot review files in your own request"
-            )
-
-        if not user.output_checker:
-            raise exceptions.ApprovalPermissionDenied(
-                "only an output checker can review a file"
-            )
-
-        if relpath not in release_request.output_files():
-            raise exceptions.ApprovalPermissionDenied(
-                "file is not an output file on this request"
-            )
-
     def approve_file(
         self,
         release_request: ReleaseRequest,
         request_file: RequestFile,
         user: User,
     ):
         """ "Approve a file"""
-
-        self._verify_permission_to_review_file(
-            release_request, request_file.relpath, user
+        permissions.check_user_can_review_file(
+            user, release_request, request_file.relpath
         )
 
         audit = AuditEvent.from_request(
@@ -2027,9 +2003,8 @@ def request_changes_to_file(
         user: User,
     ):
         """Request changes to a file"""
-
-        self._verify_permission_to_review_file(
-            release_request, request_file.relpath, user
+        permissions.check_user_can_review_file(
+            user, release_request, request_file.relpath
         )
 
         audit = AuditEvent.from_request(
@@ -2049,12 +2024,7 @@ def reset_review_file(
     ):
         """Reset a file to have no review from this user"""
 
-        self._verify_permission_to_review_file(release_request, relpath, user)
-
-        if user.username in release_request.submitted_reviews:
-            raise exceptions.ApprovalPermissionDenied(
-                "cannot reset file from submitted review"
-            )
+        permissions.check_user_can_reset_file_review(user, release_request, relpath)
 
         audit = AuditEvent.from_request(
             request=release_request,
@@ -2071,30 +2041,7 @@ def review_request(self, release_request: ReleaseRequest, user: User):
 
         Marking the request as either PARTIALLY_REVIEWED or REVIEWED, depending on whether this is the first or second review.
         """
-        if self.STATUS_OWNERS[release_request.status] != RequestStatusOwner.REVIEWER:
-            raise exceptions.RequestPermissionDenied(
-                f"Cannot review request in state {release_request.status.name}"
-            )
-
-        if user.username == release_request.author:
-            raise exceptions.RequestPermissionDenied(
-                "You cannot review your own request"
-            )
-
-        if not user.output_checker:
-            raise exceptions.RequestPermissionDenied(
-                "Only an output checker can review a request"
-            )
-
-        if not release_request.all_files_reviewed_by_reviewer(user):
-            raise exceptions.RequestReviewDenied(
-                "You must review all files to submit your review"
-            )
-
-        if user.username in release_request.submitted_reviews:
-            raise exceptions.RequestReviewDenied(
-                "You have already submitted your review of this request"
-            )
+        permissions.check_user_can_submit_review(user, release_request)
 
         self._dal.record_review(release_request.id, user.username)
 
@@ -2141,12 +2088,12 @@ def mark_file_undecided(
     ):
         """Change an existing changes-requested file in a returned request to undecided before re-submitting"""
         if release_request.status != RequestStatus.RETURNED:
-            raise exceptions.ApprovalPermissionDenied(
+            raise exceptions.RequestReviewDenied(
                 f"cannot change file review to {RequestFileVote.UNDECIDED.name} from request in state {release_request.status.name}"
             )
 
         if review.status != RequestFileVote.CHANGES_REQUESTED:
-            raise exceptions.ApprovalPermissionDenied(
+            raise exceptions.RequestReviewDenied(
                 f"cannot change file review from {review.status.name} to {RequestFileVote.UNDECIDED.name} from request in state {release_request.status.name}"
             )
 

airlock/exceptions.py

@@ -31,7 +31,4 @@ class IncompleteContextOrControls(RequestPermissionDenied): ...
 class RequestReviewDenied(APIException): ...
 
 
-class ApprovalPermissionDenied(APIException): ...
-
-
 class ManifestFileError(APIException): ...

airlock/permissions.py

@@ -163,7 +163,6 @@ def user_can_update_file_on_request(
 ):  # pragma: no cover; not currently used
     try:
         check_user_can_update_file_on_request(user, request, workspace, relpath)
-
     except exceptions.RequestPermissionDenied:
         return False
     return True
@@ -186,3 +185,63 @@ def user_can_withdraw_file_from_request(
     except exceptions.RequestPermissionDenied:
         return False
     return True
+
+
+def check_user_can_review_file(user: User, request: "ReleaseRequest", relpath: UrlPath):
+    try:
+        check_user_can_review_request(user, request)
+    except exceptions.RequestPermissionDenied as exc:
+        raise exceptions.RequestReviewDenied(str(exc))
+    policies.check_can_review_file_on_request(request, relpath)
+
+
+def user_can_review_file(
+    user: User, request: "ReleaseRequest", relpath: UrlPath
+):  # pragma: no cover; not currently used
+    try:
+        check_user_can_review_file(user, request, relpath)
+    except exceptions.RequestReviewDenied:
+        return False
+    return True
+
+
+def check_user_can_reset_file_review(
+    user: User, request: "ReleaseRequest", relpath: UrlPath
+):
+    check_user_can_review_file(user, request, relpath)
+    if user.username in request.submitted_reviews:
+        raise exceptions.RequestReviewDenied("cannot reset file from submitted review")
+
+
+def user_can_reset_file_review(
+    user: User, request: "ReleaseRequest", relpath: UrlPath
+):  # pragma: no cover; not currently used
+    try:
+        check_user_can_reset_file_review(user, request, relpath)
+    except exceptions.RequestReviewDenied:
+        return False
+    return True
+
+
+def check_user_can_submit_review(user: User, request: "ReleaseRequest"):
+    policies.check_can_review_request(request)
+    check_user_can_review_request(user, request)
+    if not request.all_files_reviewed_by_reviewer(user):
+        raise exceptions.RequestReviewDenied(
+            "You must review all files to submit your review"
+        )
+
+    if user.username in request.submitted_reviews:
+        raise exceptions.RequestReviewDenied(
+            "You have already submitted your review of this request"
+        )
+
+
+def user_can_submit_review(
+    user: User, request: "ReleaseRequest"
+):  # pragma: no cover; not currently used
+    try:
+        check_user_can_submit_review(user, request)
+    except exceptions.RequestReviewDenied:
+        return False
+    return True

airlock/policies.py

@@ -68,3 +68,24 @@ def check_can_update_file_on_request(workspace: "Workspace", relpath: UrlPath):
         raise exceptions.RequestPermissionDenied(
             "Cannot update file in request if it is not updated on disk"
         )
+
+
+def check_can_review_request(request: "ReleaseRequest"):
+    """
+    This request is in a reviewable state
+    """
+    if not request.is_under_review():
+        raise exceptions.RequestReviewDenied(
+            f"cannot review request in state {request.status.name}"
+        )
+
+
+def check_can_review_file_on_request(request: "ReleaseRequest", relpath: UrlPath):
+    """
+    This file is reviewable; i.e. it is on the request, and it is an output file.
+    """
+    check_can_review_request(request)
+    if relpath not in request.output_files():
+        raise exceptions.RequestReviewDenied(
+            "file is not an output file on this request"
+        )

airlock/views/request.py

@@ -509,7 +509,7 @@ def file_approve(request, request_id, path: str):
 
     try:
         bll.approve_file(release_request, request_file, request.user)
-    except exceptions.ApprovalPermissionDenied as exc:
+    except exceptions.RequestReviewDenied as exc:
         raise PermissionDenied(str(exc))
 
     return redirect(release_request.get_url(path))
@@ -527,7 +527,7 @@ def file_request_changes(request, request_id, path: str):
 
     try:
         bll.request_changes_to_file(release_request, request_file, request.user)
-    except exceptions.ApprovalPermissionDenied as exc:
+    except exceptions.RequestReviewDenied as exc:
         raise PermissionDenied(str(exc))
 
     return redirect(release_request.get_url(path))
@@ -545,7 +545,7 @@ def file_reset_review(request, request_id, path: str):
 
     try:
         bll.reset_review_file(release_request, relpath, request.user)
-    except exceptions.ApprovalPermissionDenied as exc:
+    except exceptions.RequestReviewDenied as exc:
         raise PermissionDenied(str(exc))
     except exceptions.FileReviewNotFound:
         raise Http404()

tests/unit/test_business_logic.py

@@ -2760,7 +2760,7 @@ def test_approve_file_not_submitted(bll):
     bll.add_file_to_request(release_request, path, author)
     request_file = release_request.get_request_file_from_output_path(path)
 
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.approve_file(release_request, request_file, checker)
 
     rfile = _get_request_file(release_request, path)
@@ -2783,7 +2783,7 @@ def test_approve_file_not_your_own(bll):
     )
     request_file = release_request.get_request_file_from_output_path(path)
 
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.approve_file(release_request, request_file, author)
 
     rfile = _get_request_file(release_request, path)
@@ -2807,7 +2807,7 @@ def test_approve_file_not_checker(bll):
     )
     request_file = release_request.get_request_file_from_output_path(path)
 
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.approve_file(release_request, request_file, author2)
 
     rfile = _get_request_file(release_request, path)
@@ -2830,7 +2830,7 @@ def test_approve_file_not_part_of_request(bll):
     bad_path = Path("path/file2.txt")
     bad_request_file = factories.create_request_file_bad_path(request_file, bad_path)
 
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.approve_file(release_request, bad_request_file, checker)
 
     rfile = _get_request_file(release_request, path)
@@ -2851,7 +2851,7 @@ def test_approve_supporting_file(bll):
     checker = factories.create_user(output_checker=True)
     request_file = release_request.get_request_file_from_output_path(path)
 
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.approve_file(release_request, request_file, checker)
 
     rfile = _get_request_file(release_request, path)
@@ -3111,7 +3111,7 @@ def test_reset_review_file_after_review_submitted(bll):
 
     # After submitting a review, the user can't reset it
     rfile = _get_request_file(release_request, path)
-    with pytest.raises(exceptions.ApprovalPermissionDenied):
+    with pytest.raises(exceptions.RequestReviewDenied):
         bll.reset_review_file(release_request, path, checker)
     assert rfile.get_file_vote_for_user(checker) is RequestFileVote.APPROVED
 
@@ -3230,7 +3230,7 @@ def test_mark_file_undecided_permission_errors(
     if allowed:
         bll.mark_file_undecided(release_request, review, path, checkers[0])
     else:
-        with pytest.raises(exceptions.ApprovalPermissionDenied):
+        with pytest.raises(exceptions.RequestReviewDenied):
             bll.mark_file_undecided(release_request, review, path, checkers[0])
 
 
@@ -3287,8 +3287,8 @@ def test_review_request_non_submitted_status(bll):
         ],
     )
     with pytest.raises(
-        exceptions.RequestPermissionDenied,
-        match="Cannot review request in state WITHDRAWN",
+        exceptions.RequestReviewDenied,
+        match="cannot review request in state WITHDRAWN",
     ):
         bll.review_request(release_request, checker)
 
@@ -3304,7 +3304,7 @@ def test_review_request_non_output_checker(bll):
     )
     with pytest.raises(
         exceptions.RequestPermissionDenied,
-        match="Only an output checker can review a request",
+        match="You do not have permission to review this request",
     ):
         bll.review_request(release_request, user)