Summary

Centralize a method for checking if a user can access an attachment's review UI.

Review Request #14123 — Created Aug. 26, 2024 and submitted Sept. 16, 2024, 8:56 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change is motivated by a bug we have in Power Pack. When an unlicensed

user uploads a PDF (or any other file attachment that requires a Power

Pack license to review), the user can see the "Review" file action and

can click on the file attachment as if they have access to the review UI.

Navigating to the review UI leads to a 404, and reloading the review

request page properly shows the file attachment as un-reviewable.
This happens because the file attachment thumbnail view would assume that the

file attachment was reviewable if it had a review URL. However even though

an attachment has a review URL, it doesn't mean the user can access that

review URL.
To address this, we add a new can_access_review_ui method on the

File Attachment model, which given a user will return whether the user can

access the review UI for the file attachment. We already had two functions in

the codebase that achieved this, one public and one internal. We remove the

internal one and deprecate the public one, instead point to the new method on

the File Attachment model. And we update the file attachment thumbnail

view to check for this boolean before displaying the attachment as reviewable.

Testing Done


Ran unit tests.
Tested uploading a PDF document as an unlicensed user, saw that I couldn't

  review it.
Tested uploading an image file attachment as an unlicensed user, saw that

  I could review it.
Did the same tests as a licensed user.
Made sure you can't navigate to a review UI that you can't access using

  the "previous" and "next" file attachment thumbnails.

Commits

Summary	ID
Centralize a method for checking if a user can access an attachment's review UI. This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.	c73e9e77001cc59d29769c4f7921276c777f27de

Summary

Centralize a method for checking if a user can access an attachment's review UI.

This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.

c73e9e77001cc59d29769c4f7921276c777f27de

Issues

Description	From	Last Updated
I'd suggest maybe naming this is_review_ui_accessible_by(), to keep with the is_accessible_by() methods.	chipx86	Aug. 27, 2024, 3:39 p.m.
Typo in the second paragraph of the description: "hapens"	chipx86	Aug. 27, 2024, 3:39 p.m.
We should import this at the top of the module.	chipx86	Aug. 27, 2024, 3:43 p.m.
This is missing a -> None. Same below.	chipx86	Aug. 27, 2024, 3:44 p.m.
We sort these from newest to oldest.	chipx86	Aug. 27, 2024, 3:45 p.m.
Typical docs we use is "The HTTP request from the client", just to make it clear.	chipx86	Aug. 27, 2024, 3:45 p.m.
This needs a Version Added.	chipx86	Aug. 27, 2024, 3:46 p.m.
Let's use single quotes here.	chipx86	Aug. 27, 2024, 3:48 p.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 27, 2024, 4:09 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

I'd suggest maybe naming this is_review_ui_accessible_by(), to keep with the is_accessible_by() methods.

The issue has been resolved. Show all issues

Typo in the second paragraph of the description: "hapens"

reviewboard/attachments/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should import this at the top of the module.
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This is missing a -> None. Same below.
```

reviewboard/reviews/ui/base.py (Diff revision 1)

The issue has been resolved. Show all issues

We sort these from newest to oldest.

reviewboard/reviews/ui/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Typical docs we use is "The HTTP request from the client", just to make it clear.
```
reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
This needs a Version Added.
```
reviewboard/webapi/resources/base_review_request_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use single quotes here.
```

Change Summary:


Renamed FileAttachment.can_access_review_ui to FileAttachment.is_review_ui_accessible_by.
Typing and doc fixes.

Description:

		This change is motivated by a bug we have in Power Pack. When an unlicensed
		user uploads a PDF (or any other file attachment that requires a Power
		Pack license to review), the user can see the "Review" file action and
		can click on the file attachment as if they have access to the review UI.
		Navigating to the review UI leads to a 404, and reloading the review
		request page properly shows the file attachment as un-reviewable.

~		This hapens because the file attachment thumbnail view would assume that the
	~	This happens because the file attachment thumbnail view would assume that the
		file attachment was reviewable if it had a review URL. However even though
		an attachment has a review URL, it doesn't mean the user can access that
		review URL.

		To address this, we add a new `can_access_review_ui` method on the
		File Attachment model, which given a user will return whether the user can
		access the review UI for the file attachment. We already had two functions in
		the codebase that achieved this, one public and one internal. We remove the
		internal one and deprecate the public one, instead point to the new method on
		the File Attachment model. And we update the file attachment thumbnail
		view to check for this boolean before displaying the attachment as reviewable.

Commits:

	Summary	ID
	Centralize a method for checking if a user can access an attachment's review UI. This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.	cfc197bf962fe9f898e9b4a40d0e184183435bde
	Centralize a method for checking if a user can access an attachment's review UI. This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.	1047272feb5d73d323639c084a70f4add4b75b86

Diff:

Revision 2 (+344 -188)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts
	reviewboard/static/rb/js/reviews/views/fileAttachmentThumbnailView.ts
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	1 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/builtin_fields.py (Diff revision 2)
The issue has been resolved. Show all issues
```
line too long (85 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Centralize a method for checking if a user can access an attachment's review UI. This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.	1047272feb5d73d323639c084a70f4add4b75b86
	Centralize a method for checking if a user can access an attachment's review UI. This change is motivated by a bug we have in Power Pack. When an unlicensed user uploads a PDF (or any other file attachment that requires a Power Pack license to review), the user can see the "Review" file action and can click on the file attachment as if they have access to the review UI. Navigating to the review UI leads to a 404, and reloading the review request page properly shows the file attachment as un-reviewable. This hapens because the file attachment thumbnail view would assume that the file attachment was reviewable if it had a review URL. However even though an attachment has a review URL, it doesn't mean the user can access that review URL. To address this, we add a new `can_access_review_ui` method on the File Attachment model, which given a user will return whether the user can access the review UI for the file attachment. We already had two functions in the codebase that achieved this, one public and one internal. We remove the internal one and deprecate the public one, instead point to the new method on the File Attachment model. And we update the file attachment thumbnail view to check for this boolean before displaying the attachment as reviewable.	c73e9e77001cc59d29769c4f7921276c777f27de

Diff:

Revision 3 (+346 -188)

Show changes

	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/static/rb/js/common/resources/models/fileAttachmentModel.ts
	reviewboard/static/rb/js/reviews/views/fileAttachmentThumbnailView.ts
	reviewboard/webapi/resources/base_review_request_file_attachment.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (c6174e1)