Adds OpenID authentication support

Review Request #4641 — Created Sept. 24, 2013 and discarded

Information

Review Board
master

Reviewers

This adds OpenID support to ReviewBoard with the use of python-social-auth (previously django-social-auth).

Tested basic functions.
Tested registering with an OpenID and logging in with one.
Tested connecting/disconnecting identifiers from an existing user.


Description From Last Updated

We should only have one form. I want this to be super slick. If you like, we can polish the …

chipx86chipx86

This UI doesn't feel right. How does it look with multiple identities? "Disconnect" is too big compared to the text. …

chipx86chipx86

The field should be wider. Going with the comment about a list, it might be nicer to have a list …

chipx86chipx86

"Identities" should have a capital "I".

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

'DNS' imported but unused

reviewbotreviewbot

'S3BotoStorage' imported but unused

reviewbotreviewbot

'CouchDBStorage' imported but unused

reviewbotreviewbot

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 107 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'DNS' imported but unused

reviewbotreviewbot

'S3BotoStorage' imported but unused

reviewbotreviewbot

'CouchDBStorage' imported but unused

reviewbotreviewbot

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 55 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

Should only be two blank lines between classes.

chipx86chipx86

Won't be needed if it's a dependency.

chipx86chipx86

'DNS' imported but unused

reviewbotreviewbot

'S3BotoStorage' imported but unused

reviewbotreviewbot

'CouchDBStorage' imported but unused

reviewbotreviewbot

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

Blank line between these.

chipx86chipx86

There are inconsistencies in these lines. If you're going to have parens, please put the ( on the previous line …

chipx86chipx86

Alignment issue.

chipx86chipx86

These all seem like things that should just go directly in settings.

chipx86chipx86

Blank line before this.

chipx86chipx86

There's far too much copy/paste here. You should instead compose this in three steps. Set it to all the stuff …

chipx86chipx86

Alphabetical order.

chipx86chipx86

Can you leave this unchanged?

chipx86chipx86

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

reviewbotreviewbot

This should just be a requirement in setup.py, and then you can depend on it absolutely.

chipx86chipx86

For all the template tags in this, make sure to properly indent (within the {% .. %}) by one space, …

chipx86chipx86

Please use single quotes inside the template tag, for consistency with the first parameter, and to fix styling (since it's …

chipx86chipx86

There are inconsistencies in indentation of HTML tags.

chipx86chipx86

Alphabetical order.

chipx86chipx86

As above, you should be able to depend on this being here. 'print' will cause confusing failures in many WSGI …

chipx86chipx86

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 72 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 36 W291 trailing whitespace

reviewbotreviewbot

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 36 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Col: 36 E128 continuation line under-indented for visual indent

reviewbotreviewbot

I don't think we need the comment here.

chipx86chipx86

Col: 26 E241 multiple spaces after ':'

reviewbotreviewbot

Col: 35 E241 multiple spaces after ':'

reviewbotreviewbot

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

reviewbotreviewbot

Why is the social.apps.django_app.default here, and not the list above? django_evolution should always be the last entry.

chipx86chipx86

These should probably be before the settings_local import, so that they can be overridden if needed. The URLs need to …

chipx86chipx86

Blank line before this.

chipx86chipx86

Missing {% trans %} for "Disconnect".

chipx86chipx86

This is too long a line. Should be more like: <td> <input ... /> <input ... /> </td>

chipx86chipx86

No need for a separate urlpatterns here. It should be able to go into one of the above groups.

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/accounts/forms.py (Diff revision 1)
     
     

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

  3. reviewboard/admin/checks.py (Diff revision 1)
     
     

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

  4. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

    Col: 26
    E241 multiple spaces after ':'

  5. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

    Col: 35
    E241 multiple spaces after ':'

  6. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  7. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  8. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

    Col: 107
    W291 trailing whitespace

  9. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  10. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  11. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  12. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  13. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  14. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

  15. reviewboard/admin/siteconfig.py (Diff revision 1)
     
     

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

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

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

  17. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/checks.py (Diff revision 1)
     
     

    'DNS' imported but unused

  3. reviewboard/admin/checks.py (Diff revision 1)
     
     

    'S3BotoStorage' imported but unused

  4. reviewboard/admin/checks.py (Diff revision 1)
     
     

    'CouchDBStorage' imported but unused

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

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

  6. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 2)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 2)
     
     

    Col: 35
    E241 multiple spaces after ':'

  4. reviewboard/admin/siteconfig.py (Diff revision 2)
     
     

    Col: 55
    E502 the backslash is redundant between brackets

  5. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/checks.py (Diff revision 2)
     
     

    'DNS' imported but unused

  3. reviewboard/admin/checks.py (Diff revision 2)
     
     

    'S3BotoStorage' imported but unused

  4. reviewboard/admin/checks.py (Diff revision 2)
     
     

    'CouchDBStorage' imported but unused

  5. reviewboard/settings.py (Diff revision 2)
     
     

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

  6. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     

    Col: 35
    E241 multiple spaces after ':'

  4. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/checks.py
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/checks.py (Diff revision 3)
     
     

    'DNS' imported but unused

  3. reviewboard/admin/checks.py (Diff revision 3)
     
     

    'S3BotoStorage' imported but unused

  4. reviewboard/admin/checks.py (Diff revision 3)
     
     

    'CouchDBStorage' imported but unused

  5. reviewboard/settings.py (Diff revision 3)
     
     

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

  6. 
      
PU
chipx86
  1. Awesome! Is this complete and ready to ship, or is there more to do?

    Can you provide screenshots of everything? Login, registration (if affected), admin UI, etc?

    1. This is ready to ship, although it could be improved a bit.

      For example, currently there is nothing stopping a user from removing their OpenID if they don't have a password, thus rendering their account inaccesible (I tried fixing this, but request.user.password is always set to an empty string. If you know another way to circumvent this, please let me know).

  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/forms.py (Diff revision 3)
     
     
     
     
     
     

    Should only be two blank lines between classes.

  3. reviewboard/admin/checks.py (Diff revision 3)
     
     
     
     
     
     
     
     

    Won't be needed if it's a dependency.

  4. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     
     

    Blank line between these.

  5. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    There are inconsistencies in these lines. If you're going to have parens, please put the ( on the previous line instead of using \

  6. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     
     

    Alignment issue.

  7. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These all seem like things that should just go directly in settings.

  8. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     

    Blank line before this.

  9. reviewboard/admin/siteconfig.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    There's far too much copy/paste here. You should instead compose this in three steps. Set it to all the stuff before create_user, then conditionally add create_user, then add the rest.

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

    Alphabetical order.

  11. reviewboard/settings.py (Diff revision 3)
     
     
     
     
     

    Can you leave this unchanged?

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

    This should just be a requirement in setup.py, and then you can depend on it absolutely.

  13. reviewboard/templates/accounts/login.html (Diff revision 3)
     
     
     
     

    For all the template tags in this, make sure to properly indent (within the {% .. %}) by one space, relative to the parent template tag. So:

    {% block ... %}
    {% if ... %}
    {% box ... %}
    {% if messages %}

    etc. The {% should never be indented at all. Indentation happens within.

    Also, {{vars}}, not {{ vars }}.

    Same goes for any other HTML files.

  14. Please use single quotes inside the template tag, for consistency with the first parameter, and to fix styling (since it's in an action="")

  15. reviewboard/templates/accounts/login.html (Diff revision 3)
     
     
     

    There are inconsistencies in indentation of HTML tags.

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

    Alphabetical order.

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

    As above, you should be able to depend on this being here.

    'print' will cause confusing failures in many WSGI deployments.

  18. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 4)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 4)
     
     

    Col: 35
    E241 multiple spaces after ':'

  4. reviewboard/settings.py (Diff revision 4)
     
     

    Col: 72
    W291 trailing whitespace

  5. reviewboard/settings.py (Diff revision 4)
     
     

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

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

    Col: 36
    W291 trailing whitespace

  7. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/settings.py (Diff revision 4)
     
     

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

  3. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 5)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 5)
     
     

    Col: 35
    E241 multiple spaces after ':'

  4. reviewboard/settings.py (Diff revision 5)
     
     

    Col: 36
    E128 continuation line under-indented for visual indent

  5. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/settings.py (Diff revision 5)
     
     

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

  3. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 6)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 6)
     
     

    Col: 35
    E241 multiple spaces after ':'

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

    Col: 36
    E128 continuation line under-indented for visual indent

  5. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

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

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

  3. 
      
PU
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/admin/siteconfig.py (Diff revision 7)
     
     

    Col: 26
    E241 multiple spaces after ':'

  3. reviewboard/admin/siteconfig.py (Diff revision 7)
     
     

    Col: 35
    E241 multiple spaces after ':'

  4. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/settings.py
    reviewboard/accounts/views.py
    reviewboard/urls.py
    setup.py
    reviewboard/accounts/forms.py
    Ignored Files:
    reviewboard/templates/accounts/prefs.html
    reviewboard/templates/accounts/login.html

  2. reviewboard/settings.py (Diff revision 7)
     
     

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

  3. 
      
PU
chipx86
  1. 
      
  2. We should only have one form. I want this to be super slick. If you like, we can polish the UI, but we'll commit this to a branch first and merge it in when we have time to refactor the UI.

    I think something more like http://2.bp.blogspot.com/_EuCTzLdp3vE/SguAL9E94NI/AAAAAAAACfk/1hjbP6RH9h0/s1600-h/openID_1.jpg is what we want, though limited to only a couple providers by default: Google and GitHub. These are likely to be the most common providers, based on what we've seen.

    It'd be nice to allow that list to be customized, but let's start there.

    There could also be a separate OpenID button that allows any URL to be given (the URL you have here, for instance), but that wouldn't be on the form people see right off the bat.

    1. We can discuss options for this work more on IRC tomorrow when we're both around, if you like.

      Some of my other comments critique code related to this that I'm otherwise saying to replace,
      so consider that more just some general feedback on our code style.

  3. This UI doesn't feel right. How does it look with multiple identities?

    "Disconnect" is too big compared to the text. There should also be more spacing.

    What I'd suggest is more of a grid look, with the URL on each row and a "Disconnect" link on the right. Sort of like the issue summary table on a review request.

  4. The field should be wider.

    Going with the comment about a list, it might be nicer to have a list of current ones, and an Add field and link at the bottom that expands to a text field and a button.

    I'm assuming that there can be multiple identities.

  5. "Identities" should have a capital "I".

  6. reviewboard/accounts/views.py (Diff revision 7)
     
     
     

    I don't think we need the comment here.

  7. reviewboard/settings.py (Diff revision 7)
     
     
     
     

    Why is the social.apps.django_app.default here, and not the list above?

    django_evolution should always be the last entry.

  8. reviewboard/settings.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These should probably be before the settings_local import, so that they can be overridden if needed.

    The URLs need to take into account the SITE_ROOT. Actually, they shouldn't be hard-coded paths, either. Can these somehow take Django URL names?

  9. Blank line before this.

  10. Missing {% trans %} for "Disconnect".

  11. This is too long a line. Should be more like:

    <td>
    <input ... />
    <input ... />
    </td>

  12. reviewboard/urls.py (Diff revision 7)
     
     
     
     
     
     
     

    No need for a separate urlpatterns here. It should be able to go into one of the above groups.

  13. 
      
sgallagh
  1. What's the status of this review? Is it in shape to be included in ReviewBoard 2.0?

    1. What we decided to do was turn this into an extension. I spent some time making the RB2.0 extension support even nicer to allow for this, and have ported this project to a working extension. The only thing missing is a form on the My Account page, which is waiting for a rewrite of that page (which is blocked on some large, unrelated Django 1.5 porting issues I'm working on for django-evolution).

      The extension will be released prior to RB 2.0, and will work with the 2.0 release.

  2. 
      
PU
Review request changed

Status: Discarded

Change Summary:

Discarded in favor of https://reviews.reviewboard.org/r/5199/
Loading...