Summary

Clarify access model in ReviewRequest.get_draft().

Review Request #14011 — Created July 3, 2024 and submitted Aug. 5, 2024, 9:08 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Our get_draft() method has two modes. If no arguments are given, it

will always return the existing draft. If a user was passed in, we'd

return the draft only if the review request was owned by that user.
In some places, we were fetching the draft using the no-args version and

then doing our own access control checks. We also had at least one bug

that I remember where we were passing the user in and it broke the

ability for admins to reassign review requests, because we weren't

fetching the draft for them in the publish operation. This inconsistency

was also making it so we were showing admins the draft data, but they

were unable to load draft diff fragments.
This change makes it so when passing a user in to get_draft(), we will

return the draft if that user has access to the draft, not just if it

matches the owner of the review request.

Testing Done


Ran js-tests.
Verified that as an admin I could now view all draft data on a

  review request owned by another user.
Audited calls to get_draft() to ensure that everything was using it in

  the correct way.

Commits

Summary	ID
Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	df00e2e7ac81a1d403d69784c4295af7a08eae08

Summary

Clarify access model in ReviewRequest.get_draft().

Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view *all* draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.

df00e2e7ac81a1d403d69784c4295af7a08eae08

Issues

Description	From	Last Updated
over-indented Column: 17 Error code: E117	reviewbot	July 3, 2024, 9:47 a.m.
For these calls, can we start passing in user= on these? Might be nice to transition all access check methods …	chipx86	July 15, 2024, 9:19 p.m.
**kwargs	chipx86	Aug. 5, 2024, 9:04 a.m.
The "for modify" kinda reads strangely.	chipx86	July 15, 2024, 9:20 p.m.
Is this something that we should document further with a "Version Changed"?	maubin	July 24, 2024, 8:40 a.m.
Missing type.	maubin	July 24, 2024, 8:41 a.m.
Missing args and *kwargs docs.	maubin	July 24, 2024, 8:42 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/review_request_draft.py (Diff revision 1)
The issue has been dropped. Show all issues
```
over-indented

Column: 17
Error code: E117
```

Commits:

	Summary	ID
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	6e67e712970193cd41cb001244874cc7d9a10b32
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	e22a963662c650219be81846b2e7edcbca8bedbd

Diff:

Revision 2 (+260 -74)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been resolved. Show all issues

For these calls, can we start passing in user= on these?

Might be nice to transition all access check methods to keyword-only arguments too, to make those calls very explicit and make it easier for us to alter them in the future if needed.

reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
The "for modify" kinda reads strangely.
```

Commits:

	Summary	ID
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	e22a963662c650219be81846b2e7edcbca8bedbd
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	f24a91f09a780554105bec98e1edfedcac75e6ca

Diff:

Revision 3 (+260 -74)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/models/review_request.py (Diff revision 3)

The issue has been resolved. Show all issues

Is this something that we should document further with a "Version Changed"?

reviewboard/reviews/models/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing type.
```
reviewboard/webapi/resources/review_request_draft.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing *args and **kwargs docs.
```

Commits:

	Summary	ID
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	f24a91f09a780554105bec98e1edfedcac75e6ca
	Clarify access model in ReviewRequest.get_draft(). Our `get_draft()` method has two modes. If no arguments are given, it will always return the existing draft. If a user was passed in, we'd return the draft only if the review request was owned by that user. In some places, we were fetching the draft using the no-args version and then doing our own access control checks. We also had at least one bug that I remember where we were passing the user in and it broke the ability for admins to reassign review requests, because we weren't fetching the draft for them in the publish operation. This inconsistency was also making it so we were showing admins the draft data, but they were unable to load draft diff fragments. This change makes it so when passing a user in to get_draft(), we will return the draft if that user has access to the draft, not just if it matches the owner of the review request. Testing Done: - Ran js-tests. - Verified that as an admin I could now view all draft data on a review request owned by another user. - Audited calls to get_draft() to ensure that everything was using it in the correct way.	df00e2e7ac81a1d403d69784c4295af7a08eae08

Diff:

Revision 4 (+290 -76)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

reviewboard/webapi/resources/review_request_draft.py (Diff revisions 2 - 4)
The issue has been resolved. Show all issues
```
**kwargs
```

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