• 
      
    Fish Trophy

    lehoangm got a fish trophy!

    Fish Trophy

    [OAuth2Provider][WIP] Enabling OAuth2 authorization on Web API

    Review Request #7997 — Created Feb. 25, 2016 and discarded

    Information

    Djblets
    master

    Reviewers

    Prior to this commit, there is no mechanism to authorize resources using OAuth2
    protocol. In this commit, OAuth2 authorization mechanism is added to
    the current authorization mechanism for web API resources.
    Services now can follow OAuth2 protocol, send a request with appropriate
    authorization header to work with Djblet's resources. External services can
    request a token with specify scope through OAuth2 protocol. When using Djblet's
    API resource, the scope of a token is checked to make sure that the
    external service has the right permission the resource.

    This commit also adding unit tests for all current backend authentication:
    the base authentication class, the basic authentication via username and
    password, the api_token authentication, and the newly added oauth2
    authentication backend.

    Add test_base_auth_backend to test WebAPIAuthBackend
    Add test_basic_auth_backend to test WebAPIBasicAuthBackend
    Add test_api_auth_backend to test WebAPITokenAuthBackend
    Add test_oauth2_auth_backend to test OAuth2TokenAuthBackend

    There should be one more for ResourceOAuth2TokenMixin

    Description From Last Updated

    undefined name 'webapi_token'

    reviewbotreviewbot

    Col: 24 W292 no newline at end of file

    reviewbotreviewbot

    Col: 1 W391 blank line at end of file

    reviewbotreviewbot

    Col: 1 W293 blank line contains whitespace

    reviewbotreviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    This should further elaborate (in a second paragraph) which fields (and types) would be required by this backend (looks like …

    chipx86chipx86

    Description must be on the next line. The type should be django.http.HttpRequest. (Some of our older classes don't provide the …

    chipx86chipx86

    What if it's 3? That'd be a different error.

    chipx86chipx86

    The type should be django.http.HttpRequest.

    chipx86chipx86

    Must just be tuple. You can document the None in the description.

    chipx86chipx86

    Must be aligned, or it'll result in syntax errors in the docs.

    chipx86chipx86

    Should be: result = super(OAuth2TokenAuthBackend, self).login_with_credentials( request, **credentials)

    chipx86chipx86

    call_method_view should go on the previous line, with arguments on the second.

    chipx86chipx86

    Like with the backend, this should document the fields and types required.

    chipx86chipx86

    This can be one statement.

    chipx86chipx86

    Try: return super(ResourceOAuth2TokenMixin, self).call_method_view( request, method, view, *args, **kwargs)

    chipx86chipx86

    This needs to have Args and Returns sections.

    chipx86chipx86

    I don't see how this code works. resources_policy should be a list of strings, which shouldn't have .get().

    chipx86chipx86

    Col: 1 W293 blank line contains whitespace

    reviewbotreviewbot

    Col: 1 W391 blank line at end of file

    reviewbotreviewbot

    I don't know about this. The unit tests are supposed to be testing the new auth backend, but they're really …

    chipx86chipx86

    Missing period, and """ on the next line. Plus a blank line below. Same below.

    chipx86chipx86

    "Return". Missing period. Same below.

    chipx86chipx86

    Make sure to use the same format as other classes. Same below.

    chipx86chipx86

    Missing type. Same below.

    chipx86chipx86

    "Object" isn't a valid Python type.

    chipx86chipx86

    Must inherit from object. However, we should use a real User, not a mock.

    chipx86chipx86

    Can't align this way, or it'll break doc generation. Make sure to use the same format used by other classes.

    chipx86chipx86

    Make sure to follow the same exact format other tests use for docstrings.

    chipx86chipx86

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    Make sure to follow the correct format for these.

    chipx86chipx86

    Should have a trailing comma.

    chipx86chipx86

    validate_credentials should be on the previous line.

    chipx86chipx86

    Col: 80 E501 line too long (83 > 79 characters)

    reviewbotreviewbot

    'BaseWebAPIToken' imported but unused

    reviewbotreviewbot

    Col: 19 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 17 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 18 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 20 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 17 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 19 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 18 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 20 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 63 E231 missing whitespace after ','

    reviewbotreviewbot

    Col: 80 E501 line too long (80 > 79 characters)

    reviewbotreviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    'BaseWebAPIToken' imported but unused

    reviewbotreviewbot

    Col: 17 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 19 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 18 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 20 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 17 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 19 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 18 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot

    Col: 20 E251 unexpected spaces around keyword / parameter equals

    reviewbotreviewbot
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
      
      
    2. Show all issues
       undefined name 'webapi_token'
      
    3. Show all issues
      Col: 24
       W292 no newline at end of file
      
    4. 
        
    LE
    LE
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/markdown/tests.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/avatars/services/base.py
          djblets/avatars/tests.py
          djblets/avatars/registry.py
          djblets/webapi/tests/test_base_auth_backend.py
      
      Ignored Files:
          djblets/static/djblets/css/defs.less
          djblets/static/djblets/css/mixins/markdown.less
          djblets/static/djblets/css/config-forms.less
          .gitignore
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/markdown/tests.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/avatars/services/base.py
          djblets/avatars/tests.py
          djblets/avatars/registry.py
          djblets/webapi/tests/test_base_auth_backend.py
      
      Ignored Files:
          djblets/static/djblets/css/defs.less
          djblets/static/djblets/css/mixins/markdown.less
          djblets/static/djblets/css/config-forms.less
          .gitignore
      
      
    2. Show all issues
      Col: 1
       W391 blank line at end of file
      
    3. Show all issues
      Col: 1
       W293 blank line contains whitespace
      
    4. Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    5. 
        
    LE
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/tests/test_base_auth_backend.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/tests/test_base_auth_backend.py
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
      
      
    2. Show all issues
      Col: 1
       W293 blank line contains whitespace
      
    3. Show all issues
      Col: 1
       W391 blank line at end of file
      
    4. Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    5. 
        
    chipx86
    1. 
        
    2. Show all issues

      This should further elaborate (in a second paragraph) which fields (and types) would be required by this backend (looks like it's token and user, basically?).

    3. Show all issues

      Description must be on the next line.

      The type should be django.http.HttpRequest. (Some of our older classes don't provide the full path, but new ones should.)

    4. djblets/webapi/auth/backends/oauth2_tokens.py (Diff revision 3)
       
       
       
       
      Show all issues

      What if it's 3? That'd be a different error.

    5. Show all issues

      The type should be django.http.HttpRequest.

    6. Show all issues

      Must just be tuple. You can document the None in the description.

    7. Show all issues

      Must be aligned, or it'll result in syntax errors in the docs.

    8. Show all issues

      Should be:

      result = super(OAuth2TokenAuthBackend, self).login_with_credentials(
          request, **credentials)
      
    9. Show all issues

      call_method_view should go on the previous line, with arguments on the second.

    10. Show all issues

      Like with the backend, this should document the fields and types required.

    11. djblets/webapi/resources/mixins/oauth2_tokens.py (Diff revision 3)
       
       
       
       
       
       
      Show all issues

      This can be one statement.

    12. Show all issues

      Try:

      return super(ResourceOAuth2TokenMixin, self).call_method_view(
          request, method, view, *args, **kwargs)
      
    13. Show all issues

      This needs to have Args and Returns sections.

      1. I am just copying and pasting here. I dont understand what _get_queryset does. Can you elaborate more on this

    14. Show all issues

      I don't see how this code works. resources_policy should be a list of strings, which shouldn't have .get().

    15. Show all issues

      I don't know about this. The unit tests are supposed to be testing the new auth backend, but they're really just testing this fake backend, right? If so, the new test backend isn't getting any real coverage, making the tests useless.

      1. My is that if the real backend functions as expected according to the interface then the base implementaion would work.
        Also this make sure that there is no error on the base implementation so that somehow with an error in thel rea backend, everything goes undetected.

        Also note that in the real backend, I also added an "integration test" to combine the base and the real backend

    16. Show all issues

      Missing period, and """ on the next line.

      Plus a blank line below.

      Same below.

    17. Show all issues

      "Return".

      Missing period.

      Same below.

    18. Show all issues

      Make sure to use the same format as other classes.

      Same below.

    19. Show all issues

      Missing type.

      Same below.

    20. Show all issues

      "Object" isn't a valid Python type.

    21. Show all issues

      Must inherit from object.

      However, we should use a real User, not a mock.

    22. djblets/webapi/tests/test_base_auth_backend.py (Diff revision 3)
       
       
       
       
      Show all issues

      Can't align this way, or it'll break doc generation.

      Make sure to use the same format used by other classes.

    23. djblets/webapi/tests/test_base_auth_backend.py (Diff revision 3)
       
       
       
       
       
      Show all issues

      Make sure to follow the same exact format other tests use for docstrings.

    24. djblets/webapi/tests/test_base_auth_backend.py (Diff revision 3)
       
       
       
       
       
       
       
       
      Show all issues

      Make sure to follow the correct format for these.

    25. Show all issues

      Should have a trailing comma.

    26. Show all issues

      validate_credentials should be on the previous line.

    27. 
        
    LE
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
          djblets/webapi/tests/test_oauth2_auth_backend.py
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/tests/test_basic_auth_backend.py
          djblets/webapi/tests/test_api_auth_backend.py
          djblets/webapi/tests/test_base_auth_backend.py
          djblets/webapi/auth/backends/api_tokens.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/webapi/resources/mixins/api_tokens.py
          djblets/webapi/auth/backends/oauth2_tokens.py
          djblets/webapi/tests/test_oauth2_auth_backend.py
          djblets/webapi/resources/mixins/oauth2_tokens.py
          djblets/webapi/tests/test_basic_auth_backend.py
          djblets/webapi/tests/test_api_auth_backend.py
          djblets/webapi/tests/test_base_auth_backend.py
          djblets/webapi/auth/backends/api_tokens.py
      
      
    2. Show all issues
      Col: 80
       E501 line too long (83 > 79 characters)
      
    3. Show all issues
       'BaseWebAPIToken' imported but unused
      
    4. Show all issues
      Col: 19
       E251 unexpected spaces around keyword / parameter equals
      
    5. Show all issues
      Col: 17
       E251 unexpected spaces around keyword / parameter equals
      
    6. Show all issues
      Col: 18
       E251 unexpected spaces around keyword / parameter equals
      
    7. Show all issues
      Col: 20
       E251 unexpected spaces around keyword / parameter equals
      
    8. Show all issues
      Col: 17
       E251 unexpected spaces around keyword / parameter equals
      
    9. Show all issues
      Col: 19
       E251 unexpected spaces around keyword / parameter equals
      
    10. Show all issues
      Col: 18
       E251 unexpected spaces around keyword / parameter equals
      
    11. Show all issues
      Col: 20
       E251 unexpected spaces around keyword / parameter equals
      
    12. Show all issues
      Col: 63
       E231 missing whitespace after ','
      
    13. Show all issues
      Col: 80
       E501 line too long (80 > 79 characters)
      
    14. Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    15. Show all issues
       'BaseWebAPIToken' imported but unused
      
    16. Show all issues
      Col: 17
       E251 unexpected spaces around keyword / parameter equals
      
    17. Show all issues
      Col: 19
       E251 unexpected spaces around keyword / parameter equals
      
    18. Show all issues
      Col: 18
       E251 unexpected spaces around keyword / parameter equals
      
    19. Show all issues
      Col: 20
       E251 unexpected spaces around keyword / parameter equals
      
    20. Show all issues
      Col: 17
       E251 unexpected spaces around keyword / parameter equals
      
    21. Show all issues
      Col: 19
       E251 unexpected spaces around keyword / parameter equals
      
    22. Show all issues
      Col: 18
       E251 unexpected spaces around keyword / parameter equals
      
    23. Show all issues
      Col: 20
       E251 unexpected spaces around keyword / parameter equals
      
    24. 
        
    brennie
    Review request changed
    Status:
    Discarded