[OAuth2Provider] Add dependencies and urls to provide OAuth2 support for external services

Review Request #7897 — Created Jan. 20, 2016 and discarded

Information

Review Board
master

Reviewers

Added into list of installed apps package django-tool-kit which provides
end-points and basic logic to manage client applications. All the end-points
for OAuth2 authorization with external services is moved to /oauth2/.
All the tool to manage client applications is a part of preference menu

Added package django-tool-kit into setup.py so that it will get install
with ReviewBoard during setup time. Even though the lastest version is 0.10,
the chosen version is 0.9 in order to be compatible with Django 1.6.

Note: please contact lehoangminh@live.com when you try to land the feature into master. I am more than happy to support the final integration

Manually tested the urls are setup correctly
Reran python setup.py develop

Description From Last Updated

The first import should be: from __future__ import unicode_literals

brenniebrennie

Docstrings should be of the form: """Single line summary. Multi-line description. """

brenniebrennie

Blank line between these.

brenniebrennie

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 9 W292 no newline at end of file

reviewbotreviewbot

Unnecessary.

brenniebrennie

We're only importing one set of views here. I think we should do: from oauth2_provider import views

brenniebrennie

"Review Board" "OAuth" Missing a period.

brenniebrennie

Single quotes. Same below.

brenniebrennie

"OAuth" Also missing a period.

brenniebrennie

Use patterns()

brenniebrennie

No blank line here.

brenniebrennie

Col: 2 W292 no newline at end of file

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Missing a trailing comma.

brenniebrennie

Please keep these imports in alphabetical order.

brenniebrennie

Unnecessary comment.

brenniebrennie

This isn't the right place for this. account/ URLs should go in reviewboard/accounts/urls.py. There, you can leave off the account/ …

brenniebrennie

Col: 34 W292 no newline at end of file

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Alphabetize these.

brenniebrennie

This import doesn't need the as ... clause. When you do from x import y as y it is the …

brenniebrennie

url(r'^preferences/oauth2/', include(apps_urlpatterns, namespace='oauth2_provider'))

brenniebrennie

Why are we including this with a namespace?

brenniebrennie

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Should use url().

chipx86chipx86

I might be wrong, but I think since we're just doing an include, you can put this up above where …

chipx86chipx86

Alignment issue here.

chipx86chipx86

No space after """. I'd also change this to "An OAuth2 client application registered by a user."

chipx86chipx86

"URL" and "users".

chipx86chipx86

Let's name this oauth2_apps_urlpatterns.

chipx86chipx86

Let's put the view on its own line, for consistency.

chipx86chipx86

Blank line between these.

chipx86chipx86

This should probably go before the reviewboard imports. Ours should be last. Also, reviewboard.oauth2 should be in alphabetical order.

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Can you document this?

chipx86chipx86

Should be listed in alphabetical order.

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

No space after """.

chipx86chipx86

Blank line between these.

chipx86chipx86

Is this covered in a future change? (I'll find out in a moment I'm sure). If so, we'll want to …

chipx86chipx86

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/oauth2/models.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E265 block comment should start with '# '
    
  3. reviewboard/oauth2/models.py (Diff revision 1)
     
     
    Show all issues
    Col: 9
     W292 no newline at end of file
    
  4. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 2
     W292 no newline at end of file
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'django_reset' imported but unused
    
  6. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  7. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  9. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 34
     W292 no newline at end of file
    
  10. 
      
brennie
  1. 
      
  2. reviewboard/oauth2/models.py (Diff revision 1)
     
     
    Show all issues

    The first import should be:

    from __future__ import unicode_literals
    
  3. reviewboard/oauth2/models.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    Docstrings should be of the form:

    """Single line summary.
    
    Multi-line description.
    """
    
  4. reviewboard/oauth2/models.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between these.

  5. reviewboard/oauth2/models.py (Diff revision 1)
     
     
    Show all issues

    Unnecessary.

  6. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    We're only importing one set of views here. I think we should do:

    from oauth2_provider import views
    
  7. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    "Review Board"
    "OAuth"

    Missing a period.

  8. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    Single quotes. Same below.

  9. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    "OAuth"

    Also missing a period.

  10. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    Use patterns()

  11. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Show all issues

    No blank line here.

  12. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues

    Missing a trailing comma.

  13. reviewboard/urls.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Please keep these imports in alphabetical order.

  14. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues

    Unnecessary comment.

  15. reviewboard/urls.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues

    This isn't the right place for this. account/ URLs should go in reviewboard/accounts/urls.py. There, you can leave off the account/ prefix (because all URLs there will have it automatically prepended because of how they are included).

    1. I agree with the suggestion, so I will move them into reviewboard/accounts/urls.py but note that we will end up with multiple places that dealing with OAuth2

  16. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 3)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
brennie
  1. So I think you may be heading in the wrong direction here. You will probably want to expose setting OAuth2 configuration per-user via the API. You should ask Christian about this to double check.

    1. I will ask Christian to clarify more

    2. Clarification:
      oauth2_provider package provide me some oauth2 end points and some forms to manage list of applications.
      Right now I am just put those end points and forms in a more proper place
      Questions:
      Do we plan to provide api support to manage oauth2 applications?
      Should I rewrite the management api or try to wrap around and use as much as possible what oauth2_provider have for me ?

    3. I discussed with Christian and it seems that there is no point adding API to manage OAuth2 client apps. Hence we are sticking to the form method for now

  2. reviewboard/accounts/urls.py (Diff revision 3)
     
     
    Show all issues

    Alphabetize these.

  3. reviewboard/accounts/urls.py (Diff revision 3)
     
     
    Show all issues

    This import doesn't need the as ... clause. When you do from x import y as y it is the same as doing from x import y.

  4. reviewboard/accounts/urls.py (Diff revision 3)
     
     
     
    Show all issues

    url(r'^preferences/oauth2/',
        include(apps_urlpatterns, namespace='oauth2_provider'))
    
  5. reviewboard/accounts/urls.py (Diff revision 3)
     
     
    Show all issues

    Why are we including this with a namespace?

    1. oauth2_provider looking for that name space in the template system. Without this name space the current default template of oauth2_provider with throw errors.
      Let's me add a TODO to remove this if needed since I am not very sure we still need this name space when we create our own template for OAuth2 forms.

    2. Removed URL

  6. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 4)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
chipx86
  1. 
      
  2. reviewboard/accounts/urls.py (Diff revision 6)
     
     
    Show all issues

    Should use url().

  3. reviewboard/accounts/urls.py (Diff revision 6)
     
     
     
    Show all issues

    I might be wrong, but I think since we're just doing an include, you can put this up above where we define the "preferences/" URL without having to worry about that view prefix argument to patterns causing problems.

    1. I dont understand what you mean here. I just want oauth2 to be under preferences and under oauth2

    2. I removed this to move all the management into the My Account page

  4. reviewboard/accounts/urls.py (Diff revision 6)
     
     
     
    Show all issues

    Alignment issue here.

    1. I dont see any alignment issue here...

    2. I removed this to move all the management into the My Account page

  5. reviewboard/oauth2/models.py (Diff revision 6)
     
     
    Show all issues

    No space after """.

    I'd also change this to "An OAuth2 client application registered by a user."

  6. reviewboard/oauth2/urls.py (Diff revision 6)
     
     
    Show all issues

    "URL" and "users".

  7. reviewboard/oauth2/urls.py (Diff revision 6)
     
     
    Show all issues

    Let's name this oauth2_apps_urlpatterns.

  8. reviewboard/oauth2/urls.py (Diff revision 6)
     
     
    Show all issues

    Let's put the view on its own line, for consistency.

  9. reviewboard/oauth2/urls.py (Diff revision 6)
     
     
     
    Show all issues

    Blank line between these.

  10. reviewboard/settings.py (Diff revision 6)
     
     
    Show all issues

    This should probably go before the reviewboard imports. Ours should be last.

    Also, reviewboard.oauth2 should be in alphabetical order.

  11. reviewboard/settings.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Can you document this?

    1. What document and where should I do it ?

    2. Just need to put comments above saying what this is for, and maybe explain that scopes are computing dynamically based on the API resources. Something like that. Just so people know what this structure is for when looking through the file.

  12. setup.py (Diff revision 6)
     
     
    Show all issues

    Should be listed in alphabetical order.

  13. 
      
LE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/settings.py
        reviewboard/oauth2/models.py
        reviewboard/accounts/urls.py
        reviewboard/oauth2/urls.py
        reviewboard/urls.py
        setup.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 7)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
    Show all issues
     'PIPELINE_JS' imported but unused
    
  6. 
      
LE
chipx86
  1. 
      
  2. reviewboard/oauth2/models.py (Diff revision 8)
     
     
    Show all issues

    No space after """.

  3. reviewboard/oauth2/models.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line between these.

  4. reviewboard/oauth2/models.py (Diff revision 8)
     
     
    Show all issues

    Is this covered in a future change? (I'll find out in a moment I'm sure). If so, we'll want to make sure one change has all the fields needed for a given model.

    1. Apparently after not having restriction on what localsite and what scope that an allication can be authenticated for, then we dont even need to change the original application class. Is it be better to remove this and use what from OAuth2 provider instead?

    2. Hmm... I think we probably want to have our own, because this may change in the future, and it'd be hard to update from another model. We should leave a comment (outside of the docstring) saying that we're subclassing this in order to have a model we can extend in the future.

      Now that said, looking into this more, we actually can't subclass from Application. If you look at the code in oauth2_provider/models.py, you'll see that Application is a subclass (and therefore a working model in the database) of AbstractApplication. We're meant to be subclassing the latter. That's probably why you needed the app_label, which we also don't want (it'll result in conflicts between the two models).

    3. Even though, subclassing AbstractApplication creates unused table, after Slack discussion we agree that an extra table is not an issue so we should subclass the AbstractApplication

      app_label can be removed, look at https://docs.djangoproject.com/en/dev/topics/auth/customizing/#auth-custom-user for more info

  5. 
      
LE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/oauth2/urls.py
        reviewboard/oauth2/models.py
        reviewboard/settings.py
        setup.py
        reviewboard/urls.py
    
    Ignored Files:
        reviewboard/oauth2/__init__.py
    
    
  2. reviewboard/settings.py (Diff revision 9)
     
     
    Show all issues
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 9)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. 
      
LE
brennie
Review request changed

Status: Discarded

Loading...