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

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

Minh Le Hoang
Review Board
master
7900
reviewboard
brennie, chipx86, david

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

  • 0
  • 0
  • 34
  • 36
  • 70
Description From Last Updated
Review Bot
  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)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  3. reviewboard/oauth2/models.py (Diff revision 1)
     
     
    Col: 9
     W292 no newline at end of file
    
  4. reviewboard/oauth2/urls.py (Diff revision 1)
     
     
    Col: 2
     W292 no newline at end of file
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
     'django_reset' imported but unused
    
  6. reviewboard/settings.py (Diff revision 1)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  7. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_CSS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_JS' imported but unused
    
  9. reviewboard/urls.py (Diff revision 1)
     
     
    Col: 34
     W292 no newline at end of file
    
  10. 
      
Barret Rennie
  1. 
      
  2. reviewboard/oauth2/models.py (Diff revision 1)
     
     

    The first import should be:

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

    Docstrings should be of the form:

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

    Blank line between these.

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

    Unnecessary.

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

    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)
     
     

    "Review Board"
    "OAuth"

    Missing a period.

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

    Single quotes. Same below.

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

    "OAuth"

    Also missing a period.

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

    Use patterns()

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

    No blank line here.

  12. reviewboard/settings.py (Diff revision 1)
     
     

    Missing a trailing comma.

  13. reviewboard/urls.py (Diff revision 1)
     
     
     
     
     
     
     
     

    Please keep these imports in alphabetical order.

  14. reviewboard/urls.py (Diff revision 1)
     
     

    Unnecessary comment.

  15. reviewboard/urls.py (Diff revision 1)
     
     
     
     
     
     
     
     
     

    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. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 3)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Barret Rennie
  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)
     
     

    Alphabetize these.

  3. reviewboard/accounts/urls.py (Diff revision 3)
     
     

    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)
     
     
     

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

    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. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 4)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 5)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 5)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 6)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 6)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Christian Hammond
  1. 
      
  2. reviewboard/accounts/urls.py (Diff revision 6)
     
     

    Should use url().

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

    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)
     
     
     

    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)
     
     

    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)
     
     

    "URL" and "users".

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

    Let's name this oauth2_apps_urlpatterns.

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

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

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

    Blank line between these.

  10. reviewboard/settings.py (Diff revision 6)
     
     

    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)
     
     
     
     
     

    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)
     
     

    Should be listed in alphabetical order.

  13. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 7)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 7)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 8)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_CSS' imported but unused
    
  5. reviewboard/settings.py (Diff revision 8)
     
     
     'PIPELINE_JS' imported but unused
    
  6. 
      
Minh Le Hoang
Christian Hammond
  1. 
      
  2. reviewboard/oauth2/models.py (Diff revision 8)
     
     

    No space after """.

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

    Blank line between these.

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

    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. 
      
Minh Le Hoang
Review Bot
  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)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 9)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. 
      
Minh Le Hoang
Barret Rennie
Review request changed

Status: Discarded

Loading...