Fish Trophy

lehoangm got a 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.  undefined name 'webapi_token'
    
  3. 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. Col: 1
     W391 blank line at end of file
    
  3. Col: 1
     W293 blank line contains whitespace
    
  4. 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. Col: 1
     W293 blank line contains whitespace
    
  3. Col: 1
     W391 blank line at end of file
    
  4. Col: 80
     E501 line too long (81 > 79 characters)
    
  5. 
      
chipx86
  1. 
      
  2. 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. 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)
     
     
     
     

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

  5. The type should be django.http.HttpRequest.

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

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

  8. Should be:

    result = super(OAuth2TokenAuthBackend, self).login_with_credentials(
        request, **credentials)
    
  9. call_method_view should go on the previous line, with arguments on the second.

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

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

    This can be one statement.

  12. Try:

    return super(ResourceOAuth2TokenMixin, self).call_method_view(
        request, method, view, *args, **kwargs)
    
  13. 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. I don't see how this code works. resources_policy should be a list of strings, which shouldn't have .get().

  15. 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. Missing period, and """ on the next line.

    Plus a blank line below.

    Same below.

  17. "Return".

    Missing period.

    Same below.

  18. Make sure to use the same format as other classes.

    Same below.

  19. Missing type.

    Same below.

  20. "Object" isn't a valid Python type.

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

    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)
     
     
     
     
     

    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)
     
     
     
     
     
     
     
     

    Make sure to follow the correct format for these.

  25. Should have a trailing comma.

  26. 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. Col: 80
     E501 line too long (83 > 79 characters)
    
  3.  'BaseWebAPIToken' imported but unused
    
  4. Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  5. Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  6. Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  7. Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  8. Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  9. Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  10. Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  11. Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  12. Col: 63
     E231 missing whitespace after ','
    
  13. Col: 80
     E501 line too long (80 > 79 characters)
    
  14. Col: 80
     E501 line too long (81 > 79 characters)
    
  15.  'BaseWebAPIToken' imported but unused
    
  16. Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  17. Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  18. Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  19. Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  20. Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  21. Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  22. Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  23. Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  24. 
      
brennie
Review request changed

Status: Discarded

Loading...