Add an API for managing OAuth applications

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

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

Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E702 multiple statements on one line (semicolon)

reviewbotreviewbot

E305 expected 2 blank lines after class or function definition, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (5)

reviewbotreviewbot

F811 redefinition of unused 'test_get_filtered' from line 77

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

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

daviddavid

Module docstring?

daviddavid

Swap these two lines.

daviddavid

Swap these two lines.

daviddavid

You can enforce the actual values by doing: 'type': (Application.GRANT_AUTHORIZATION_CODE, Application.GRANT_CLIENT_CREDENTIALS, Application.GRANT_IMPLICIT, Application.GRANT_PASSWORD)

daviddavid

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

daviddavid

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

chipx86chipx86

Needs a space before "stylesheet"

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

Can you put GRANT_PASSWORD on its own line?

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing a period. Should also include a bit more information on what this is. In general, the more explicit the ...

chipx86chipx86

Can you break the strings at newlines?

chipx86chipx86

Can you break at the newlines?

chipx86chipx86

Local Site

chipx86chipx86

Local Site

chipx86chipx86

Rather than having all this here, let's add permission checks to Application. There should be an is_accessible_by for this and ...

chipx86chipx86

Should use local_site.is_mutable_by(request.user).

chipx86chipx86

This can fit on one line.

chipx86chipx86

, optional

chipx86chipx86

, optional

chipx86chipx86

local_site.is_mutable_by(request.user)

chipx86chipx86

This is already provided by the base class.

chipx86chipx86

We need a @webapi_request_fields that documents username=.

chipx86chipx86

These are already provided by the base class.

chipx86chipx86

No blank line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

local_site.is_mutable_by(request.user).

chipx86chipx86

How about "You don't have permission to set this field."? We may very well want special permissions for modifying OAuth2 ...

chipx86chipx86

values_list would go on the next line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

resources should go before tests.

chipx86chipx86

Missing a docstring.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing a docstring.

chipx86chipx86

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

chipx86chipx86

Can you put a blank line before/after the api_get? It's helpful to have a block of setup code, a block ...

chipx86chipx86

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

E131 continuation line unaligned for hanging indent

reviewbotreviewbot

F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Blank line between these two.

daviddavid

i before s

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

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
david
  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. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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