Add an API for managing OAuth applications

Review Request #9002 - Created June 8, 2017 and submitted

Barret Rennie
Review Board
release-3.0.x
8987
9013
reviewboard

OAuth applications can now be created and managed via the oauth-apps/
API endpoint. Application information is only available to (1) the user
who created the app, (2) site administrators, and (3) Local Site
administrators when the application is associated with a Local Site.

Ran unit tests.

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

flake8

Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/testing/testcase.py (Diff revision 3)
     
     
     

    ): should be on the previous line. Also, docstring?

  3. Module docstring?

  4. reviewboard/webapi/resources/oauth_app.py (Diff revision 3)
     
     
     

    Swap these two lines.

  5. reviewboard/webapi/resources/oauth_app.py (Diff revision 3)
     
     
     

    Swap these two lines.

  6. reviewboard/webapi/resources/oauth_app.py (Diff revision 3)
     
     
     
     
     
     
     
     

    You can enforce the actual values by doing:

    'type': (Application.GRANT_AUTHORIZATION_CODE,
             Application.GRANT_CLIENT_CREDENTIALS,
             Application.GRANT_IMPLICIT,
             Application.GRANT_PASSWORD)
    
  7. reviewboard/webapi/resources/oauth_app.py (Diff revision 3)
     
     
     
     
     
     
     

    This can also use 'type': (Application.CLIENT_CONFIDENTIAL, Application.CLIENT_PUBLIC)

  8. 
      
Barret Rennie
Barret Rennie
Barret Rennie
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/oauth/forms.py (Diff revision 7)
     
     

    Can this (and the other changes in this file) be rolled up into existing review requests?

  3. Needs a space before "stylesheet"

  4. reviewboard/testing/testcase.py (Diff revision 7)
     
     

    , optional

  5. reviewboard/testing/testcase.py (Diff revision 7)
     
     

    , optional

  6. reviewboard/testing/testcase.py (Diff revision 7)
     
     

    , optional

  7. reviewboard/testing/testcase.py (Diff revision 7)
     
     

    , optional

  8. Can you put GRANT_PASSWORD on its own line?

  9. I'd like to include more information on what this is, for those new to OAuth2.

  10. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     

    Can you break the string after the newlines, so it reads more like paragraphs?

  11. Missing a period.

    Should also include a bit more information on what this is. In general, the more explicit the descriptions/help text, the more time we save others and ourselves.

  12. Can you break the strings at newlines?

  13. Can you break at the newlines?

  14. Local Site

  15. Local Site

  16. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     
     
     
     
     

    Rather than having all this here, let's add permission checks to Application. There should be an is_accessible_by for this and is_mutable_by for the modify/delete checks (which are free to check is_accessible_by).

  17. Should use local_site.is_mutable_by(request.user).

  18. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     

    This can fit on one line.

  19. , optional

  20. , optional

  21. local_site.is_mutable_by(request.user)

  22. This is already provided by the base class.

  23. We need a @webapi_request_fields that documents username=.

  24. These are already provided by the base class.

  25. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     
     

    No blank line.

  26. I don't really love the complexity of building a temp function that does its own caching. The logic should probably change instead.

    How about:

    skip_authorization = parsed_request_fields.get('skip_authorization', False)
    is_owner = (username is not None and
                username != request.user.username)
    
    if skip_authorization or is_owner:
        # The user wants to set a fields only available to administrators.
        # Check for permission first.
        if not (request.user.is_authenticated() and
                (request.user.is_superuser or
                 (...))):
            # The user doesn't have permission to modify these fields, so
            # return error messages for each of the fields they attempted to
            # modify.
            if skip_authorization:
                errors['skip_authorization'].append(...)
    
            if is_owner:
                errors['user'].append(...)
        else:
            # Start handling those fields.
            if is_owner:
                ...
    

    This gets rid of the need to build a temp function that caches on itself, and it separates out the permission checking from the rest of the logic.

    The User lookups would go in the else:. If the user doesn't have permission to set these fields, we shouldn't be giving them any information on whether the user they've set is valid. They should just get denied up-front.

  27. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     
     
     
     

    Can you put this in parens? (the superuser or ...).

  28. reviewboard/webapi/resources/oauth_app.py (Diff revision 7)
     
     
     
     

    local_site.is_mutable_by(request.user).

  29. How about "You don't have permission to set this field."? We may very well want special permissions for modifying OAuth2 entries for organizations without granting full admin privileges down the road.

  30. values_list would go on the next line.

  31. We should default this to None up above, instead of setting it here and in the else: further down.

  32. Let's remove the "on this LocalSite." RBCommons users are just going to ask questions about it. From their point of view, their team's users are the entire set of users.

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

    How about instead of this, we have a not_owner_status_code and not_owner_error, defaulting to 403 and PERMISSION_DENIED, respectively. Then it's a little more immediately clear what the purpose is, and it's not hard-coded for one specific use case.

    These should also have doc comments.

  34. reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7)
     
     
     
     

    resources should go before tests.

  35. Missing a docstring.

  36. reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7)
     
     
     
     
     

    This sort of thing is repeated an aweful lot. How about giving _make_applications a filter argument?

  37. reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7)
     
     
     

    Can we use keyword arguments for all these? It'll make it more self-documenting.

  38. Missing a docstring.

  39. reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7)
     
     
     
     
     
     
     
     

    No need for these blank lines. They're all setting common state for the tests.

  40. reviewboard/webapi/tests/test_oauth_app.py (Diff revision 7)
     
     
     
     
     

    Can you put a blank line before/after the api_get?

    It's helpful to have a block of setup code, a block of test execution, and a block of assertions.

    Here and below.

  41. 
      
Barret Rennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
Barret Rennie
Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/webapi/resources/oauth_app.py (Diff revision 9)
     
     
     

    Blank line between these two.

  3. reviewboard/webapi/resources/oauth_app.py (Diff revision 9)
     
     
     

    i before s

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

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (2993ce0)
Loading...