Fixed permission failure with status message update
Review Request #7922 — Created Jan. 30, 2016 and discarded
When users with the "can_change_status" permission try to edit the close message, a permission error occurs. This is because when checking for permission, users with no modify permission are all excluded.
Code is changed to include the case of "can_change_status" permission.
Added test case to cover case where user with "can_change_status" permission but no modify permission should get permission error when trying to edit changenum; the case where user with "can_change_status" permission but no modify permission should get permission error when trying to add extra data; the case where user with "can_edit_reviewrequest" permission but no status mutation permission should get permission error when trying to modify status.
Manual testing and Review Board Unit Testing
| Description | From | Last Updated |
|---|---|---|
|
Col: 14 E127 continuation line over-indented for visual indent |
 reviewbot
|
|
|
Col: 35 W291 trailing whitespace |
reviewbot
|
|
|
Col: 74 W291 trailing whitespace |
reviewbot
|
|
|
Col: 9 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
This worries me. The logic here says: "If you're modifying changenum or extra_fields and you don't have modify permissions, AND … |
 mike_conley
|
|
|
local variable 'rsp' is assigned to but never used |
reviewbot
|
|
|
Col: 25 E231 missing whitespace after ':' |
reviewbot
|
|
|
This doesn't need parens around the first two conditions. (A and B) and C is equivalent to A and B … |
 brennie
|
|
|
So this change actually has a bigger impact than it first appears. Old logic was: "Return an error if we're … |
 chipx86
|
|
|
The """ should go on the next line. |
brennie
|
|
|
You only embed this right into the payload, so it may aswell go there instead of being pulled out as … |
brennie
|
|
|
Can you format this as: self.api_put(get_review_request_item_url(r.display_id), { 'changenum': changenum, }, expected_status=403) |
brennie
|
SH
- Groups:
-
Just adding a note here so that we don't forget - can you please run the automated nose tests and update the Testing Done section with their result?
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request.py
-
Can you edit your description and remove the first line? It is already in the summary.
Your description should mention the problem (i.e., the logic error) and how it was fixed.
See this guide.
SH
- Description:
-
~ Fixed permission failure with status message update
~ Users with the "can_change_status" permission be allowed to edit the close message ~ When users with the "can_change_status" permission try to edit the close message, a permission error occurs. This is because when checking for permission, users with no modify permission are all excluded.
~ Code is changed to include the case of "can_change_status" permission. - Small fix to change from "or" to "and" in logic
-
Sorry this took so long, but I wanted to take a good long look at this! Great work on this - but I have a small concern. See below!
-
This worries me.
The logic here says:
"If you're modifying changenum or extra_fields and you don't have modify permissions, AND on top of that, if you don't have status mutable permissions, show a no access error".
What I believe that leaves, however, is the case where a user doesn't have status mutation permissions, but didn't modify changenum or extra_fields.
In that case, I believe we'll slip through this block, and attempt to write to the status of the review request. I think that's a security / permission problem.
I think we should write some tests for this to be sure, because I'm mostly just assuming this via inspection and reading the PUT docs on the ReviewRequest WebAPI resource.
Here's where tests should be written: https://github.com/reviewboard/reviewboard/blob/master/reviewboard/webapi/tests/test_review_request.py
SH
- Description:
-
When users with the "can_change_status" permission try to edit the close message, a permission error occurs. This is because when checking for permission, users with no modify permission are all excluded.
~ Code is changed to include the case of "can_change_status" permission. ~ Code is changed to include the case of "can_change_status" permission. + Added test case to cover case where user with "can_change_status" permission but no modify permission should get permission error when trying to edit changenum.
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py -
-
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py
-
-
This doesn't need parens around the first two conditions.
(A and B) and Cis equivalent toA and B and C. -
-
You only embed this right into the payload, so it may aswell go there instead of being pulled out as a var.
-
Can you format this as:
self.api_put(get_review_request_item_url(r.display_id), { 'changenum': changenum, }, expected_status=403)
-
-
So this change actually has a bigger impact than it first appears.
Old logic was: "Return an error if we're mutating a field without permission, or we're changing status without permission."
New logic is: "Return an error only if we are disallowed both from modifying and from updating a status."
This means that if you have permission to change status but not to modify, you can get past this point. Even if you're not changing status. This means that, further down, you're allowed to set
extra_data, and at the very least, you're allowed to trigger asave(which will update the timestamp and trigger signals, triggering webhooks, possibly triggering extension behavior).Let's give another hypothetical where you're allowed to edit but not change status (say we let an extension block that off, so it must be automated). We'd now let you through here, and into the block below where status is being changed. The old code would have caught this (status change requested and no permission == error).
So that
orwas pretty important. What we probably need to do, to fix that bug and these situations, is to:- Calculate these permission checks once, and store in a variable (so we don't have any expensive calculations happening more than once).
- Wrap the entire status checking block with the status permission check.
- Wrap the whole changenum block (actually from
changed_fieldsthrough the save) in the other status check. - Just to completely sanity-check, before saving, assert that we were allowed to get there based on one of those permission checks being
True.
SH
- Description:
-
When users with the "can_change_status" permission try to edit the close message, a permission error occurs. This is because when checking for permission, users with no modify permission are all excluded.
Code is changed to include the case of "can_change_status" permission. ~ Added test case to cover case where user with "can_change_status" permission but no modify permission should get permission error when trying to edit changenum. ~ Added test case to cover case where user with "can_change_status" permission but no modify permission should get permission error when trying to edit changenum; the case where user with "can_change_status" permission but no modify permission should get permission error when trying to add extra data; the case where user with "can_edit_reviewrequest" permission but no status mutation permission should get permission error when trying to modify status.
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request.py