Clarify access model in ReviewRequest.get_draft().

Review Request #14011 — Created July 3, 2024 and updated

Information

Review Board
release-7.x

Reviewers

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.

  • 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.
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
Description From Last Updated

over-indented Column: 17 Error code: E117

reviewbotreviewbot

For these calls, can we start passing in user= on these? Might be nice to transition all access check methods …

chipx86chipx86

The "for modify" kinda reads strangely.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. 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.

  3. Show all issues

    The "for modify" kinda reads strangely.

  4. 
      
david
Review request changed
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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.