Fixed permission failure with status message update

Review Request #7922 — Created Jan. 30, 2016 and discarded

Information

Review Board
release-2.0.x

Reviewers

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

reviewbotreviewbot

Col: 35 W291 trailing whitespace

reviewbotreviewbot

Col: 74 W291 trailing whitespace

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

This worries me. The logic here says: "If you're modifying changenum or extra_fields and you don't have modify permissions, AND …

mike_conleymike_conley

local variable 'rsp' is assigned to but never used

reviewbotreviewbot

Col: 25 E231 missing whitespace after ':'

reviewbotreviewbot

This doesn't need parens around the first two conditions. (A and B) and C is equivalent to A and B …

brenniebrennie

So this change actually has a bigger impact than it first appears. Old logic was: "Return an error if we're …

chipx86chipx86

The """ should go on the next line.

brenniebrennie

You only embed this right into the payload, so it may aswell go there instead of being pulled out as …

brenniebrennie

Can you format this as: self.api_put(get_review_request_item_url(r.display_id), { 'changenum': changenum, }, expected_status=403)

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
SH
mike_conley
  1. 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?

  2. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
  2. Col: 14
     E127 continuation line over-indented for visual indent
    
  3. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
  2. Col: 35
     W291 trailing whitespace
    
  3. Col: 74
     W291 trailing whitespace
    
  4. Col: 9
     E128 continuation line under-indented for visual indent
    
  5. 
      
SH
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
    
    
  2. 
      
brennie
  1. 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.

  2. 
      
SH
mike_conley
  1. 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!

  2. 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

  3. 
      
SH
reviewbot
  1. 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
    
    
  2.  local variable 'rsp' is assigned to but never used
    
  3. Col: 25
     E231 missing whitespace after ':'
    
  4. 
      
SH
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 6)
     
     
     
     

    This doesn't need parens around the first two conditions.
    (A and B) and C is equivalent to A and B and C.

  3. The """ should go on the next line.

  4. You only embed this right into the payload, so it may aswell go there instead of being pulled out as a var.

  5. reviewboard/webapi/tests/test_review_request.py (Diff revision 6)
     
     
     
     
     

    Can you format this as:

    self.api_put(get_review_request_item_url(r.display_id),
                 {
                     'changenum': changenum,
                 },
                 expected_status=403)
    
  6. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revision 6)
     
     
     
     

    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 a save (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 or was pretty important. What we probably need to do, to fix that bug and these situations, is to:

    1. Calculate these permission checks once, and store in a variable (so we don't have any expensive calculations happening more than once).
    2. Wrap the entire status checking block with the status permission check.
    3. Wrap the whole changenum block (actually from changed_fields through the save) in the other status check.
    4. Just to completely sanity-check, before saving, assert that we were allowed to get there based on one of those permission checks being True.
    1. I just realized with the code further down, we do bail before saving, but only if a changenum is specified.

    2. Oh, and we shoud probably have unit tests for each one of those possible conditions, just to be super safe. Check that we don't so much as save the review request unless we're really allowed to.

  3. 
      
SH
reviewbot
  1. 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
    
    
  2. 
      
SH
reviewbot
  1. 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
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...