Check OAuth token in @webapi_check_local_site

Review Request #9079 - Created July 17, 2017 and submitted

Barret Rennie
Review Board
release-3.0.x
9078
b60473b...
reviewboard

This patch updates @webapi_check_local_site to ensure that if the user
is using an OAuth2 access token that it is valid for the current Local
Site.

Web API test mixins classes have been added so that this logic can be
tested for all resources.

Ran unit tests.

  • 0
  • 0
  • 11
  • 2
  • 13
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

David Trowbridge
  1. 
      
  2. reviewboard/webapi/tests/mixins.py (Diff revision 1)
     
     

    Why is this commented out? Same below.

  3. 
      
Barret Rennie
Review request changed
Barret Rennie
David Trowbridge
  1. 
      
  2. It looks like you posted the wrong change here?

  3. 
      
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/webapi/decorators.py (Diff revision 4)
     
     

    Here we compare directly against None, whereas below (in the next conditional), we just check truthiness. Is this correct? Or can we just check truthiness in both?

  3. reviewboard/webapi/decorators.py (Diff revision 4)
     
     

    The other logging statements are out of date. These should now have a request=request argument, and the three initial request.* args should be removed. The logging handler will take care to log these details.

  4. reviewboard/webapi/tests/mixins.py (Diff revision 4)
     
     
     

    Swap these.

  5. reviewboard/webapi/tests/mixins.py (Diff revision 4)
     
     

    This method should really be purely about setting things up to work with a Local Site. I don't think we want to complicate it by then amking the usage of a Local Site optional. It seems that we only have the one basic test using this (and then the GET equivalent used the GET version of this test). Can we just change up the logic in those to use the non-site versions of this, and to perform the tests needed?

    This applies to the equivalent versions of this.

  6. reviewboard/webapi/tests/mixins.py (Diff revision 4)
     
     
     

    I don't really understand this. "Meant for a local site on the root"?

    1. A local-site restricted token was used on the root, ie. /api/ instead of s/foo/api/

    2. Hmm, okay. It's the "with access to a local site ... meant for a local site on the root" that's confusing. It's still not completely clear to me what this is testing for, from the docstring alone. Is it the token meant for a local site that the user has access to, being used on /api/? Might be better to explicitly say /api/ instead of "root", and to maybe just get rid of the "with access to a local site" bit?

  7. reviewboard/webapi/tests/mixins.py (Diff revision 4)
     
     
     
     
     

    The two Local Site-related assignments are redundant.

  8. These would be better as creates + IntegrityError checks. It'll keep query counts down (otherwise, we're doing 2 in most cases now).

  9. 
      
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Review request changed
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (f738b53)
Loading...