Summary

Adds OpenID authentication support

Review Request #4641 — Created Sept. 24, 2013 and discarded April 23, 2014, 3:34 p.m.

Information

Owner

puiterwijk

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

4666

Reviewers

Groups

reviewboard

People

sgallagh

Description

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

Testing Done

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

Files

Issues

Description	From	Last Updated
We should only have one form. I want this to be super slick. If you like, we can polish the …	chipx86	Oct. 16, 2013, 11:47 p.m.
This UI doesn't feel right. How does it look with multiple identities? "Disconnect" is too big compared to the text. …	chipx86	Oct. 16, 2013, 11:47 p.m.
The field should be wider. Going with the comment about a list, it might be nicer to have a list …	chipx86	Oct. 16, 2013, 11:47 p.m.
"Identities" should have a capital "I".	chipx86	Oct. 16, 2013, 11:47 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
'DNS' imported but unused	reviewbot	Sept. 24, 2013, 4:51 a.m.
'S3BotoStorage' imported but unused	reviewbot	Sept. 24, 2013, 4:51 a.m.
'CouchDBStorage' imported but unused	reviewbot	Sept. 24, 2013, 4:51 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (106 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 107 W291 trailing whitespace	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (112 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (111 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 24, 2013, 5:06 a.m.
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	Sept. 24, 2013, 5:07 a.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Sept. 24, 2013, 5:07 a.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Sept. 24, 2013, 5:07 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 24, 2013, 4:51 a.m.
Col: 80 E501 line too long (88 > 79 characters)	reviewbot	Sept. 24, 2013, 5:07 a.m.
'DNS' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
'S3BotoStorage' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
'CouchDBStorage' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:03 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:03 a.m.
Col: 55 E502 the backslash is redundant between brackets	reviewbot	Sept. 24, 2013, 5:08 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 24, 2013, 5:07 a.m.
Should only be two blank lines between classes.	chipx86	Sept. 25, 2013, 2:38 a.m.
Won't be needed if it's a dependency.	chipx86	Sept. 25, 2013, 2:39 a.m.
'DNS' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
'S3BotoStorage' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
'CouchDBStorage' imported but unused	reviewbot	Sept. 24, 2013, 5:07 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:07 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 24, 2013, 5:07 a.m.
Blank line between these.	chipx86	Sept. 25, 2013, 2:59 a.m.
There are inconsistencies in these lines. If you're going to have parens, please put the ( on the previous line …	chipx86	Sept. 25, 2013, 2:42 a.m.
Alignment issue.	chipx86	Sept. 25, 2013, 2:42 a.m.
These all seem like things that should just go directly in settings.	chipx86	Sept. 25, 2013, 2:48 a.m.
Blank line before this.	chipx86	Sept. 25, 2013, 2:48 a.m.
There's far too much copy/paste here. You should instead compose this in three steps. Set it to all the stuff …	chipx86	Sept. 25, 2013, 2:49 a.m.
Alphabetical order.	chipx86	Sept. 25, 2013, 2:50 a.m.
Can you leave this unchanged?	chipx86	Sept. 25, 2013, 2:51 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 24, 2013, 5:07 a.m.
This should just be a requirement in setup.py, and then you can depend on it absolutely.	chipx86	Sept. 25, 2013, 2:51 a.m.
For all the template tags in this, make sure to properly indent (within the {% .. %}) by one space, …	chipx86	Sept. 25, 2013, 2:58 a.m.
Please use single quotes inside the template tag, for consistency with the first parameter, and to fix styling (since it's …	chipx86	Sept. 25, 2013, 2:58 a.m.
There are inconsistencies in indentation of HTML tags.	chipx86	Sept. 25, 2013, 2:55 a.m.
Alphabetical order.	chipx86	Sept. 25, 2013, 2:55 a.m.
As above, you should be able to depend on this being here. 'print' will cause confusing failures in many WSGI …	chipx86	Sept. 25, 2013, 2:55 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 3:19 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 3:19 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 25, 2013, 3:20 a.m.
Col: 72 W291 trailing whitespace	reviewbot	Sept. 25, 2013, 3:20 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	Sept. 25, 2013, 3:21 a.m.
Col: 36 W291 trailing whitespace	reviewbot	Sept. 25, 2013, 3:21 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 3:26 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 3:26 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 25, 2013, 3:26 a.m.
Col: 36 E128 continuation line under-indented for visual indent	reviewbot	Sept. 25, 2013, 9:54 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 9:54 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 25, 2013, 9:54 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 25, 2013, 9:54 a.m.
Col: 36 E128 continuation line under-indented for visual indent	reviewbot	Sept. 26, 2013, 1:18 a.m.
I don't think we need the comment here.	chipx86	Oct. 16, 2013, 11:47 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Sept. 26, 2013, 1:18 a.m.
Col: 35 E241 multiple spaces after ':'	reviewbot	Sept. 26, 2013, 1:18 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 26, 2013, 1:19 a.m.
Why is the social.apps.django_app.default here, and not the list above? django_evolution should always be the last entry.	chipx86	Oct. 16, 2013, 11:47 p.m.
These should probably be before the settings_local import, so that they can be overridden if needed. The URLs need to …	chipx86	Oct. 16, 2013, 11:47 p.m.
Blank line before this.	chipx86	Oct. 16, 2013, 11:47 p.m.
Missing {% trans %} for "Disconnect".	chipx86	Oct. 16, 2013, 11:47 p.m.
This is too long a line. Should be more like: <td> <input ... /> <input ... /> </td>	chipx86	Oct. 16, 2013, 11:47 p.m.
No need for a separate urlpatterns here. It should be able to go into one of the above groups.	chipx86	Oct. 16, 2013, 11:47 p.m.

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

reviewboard/accounts/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (99 > 79 characters)
```
reviewboard/admin/checks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (83 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 26

 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 35

 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (87 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (106 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 107

 W291 trailing whitespace
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (112 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (111 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (80 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (87 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (94 > 79 characters)
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (96 > 79 characters)
```
reviewboard/urls.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

 E501 line too long (88 > 79 characters)
```

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

reviewboard/admin/checks.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'DNS' imported but unused
```
reviewboard/admin/checks.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'S3BotoStorage' imported but unused
```
reviewboard/admin/checks.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'CouchDBStorage' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
'from settings_local import *' used; unable to detect undefined names
```

Change Summary:

Fixed some PEP8 issues

Diff:

Revision 2 (+190 -2)

Show changes

	reviewboard/settings.py
	reviewboard/urls.py
	reviewboard/accounts/forms.py
	reviewboard/accounts/views.py
	reviewboard/admin/checks.py
	reviewboard/admin/siteconfig.py
	reviewboard/templates/accounts/login.html
	reviewboard/templates/accounts/prefs.html

reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 26

 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 35

 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 55

 E502 the backslash is redundant between brackets
```

reviewboard/admin/checks.py (Diff revision 2)
The issue has been dropped. Show all issues
```
'DNS' imported but unused
```
reviewboard/admin/checks.py (Diff revision 2)
The issue has been dropped. Show all issues
```
'S3BotoStorage' imported but unused
```
reviewboard/admin/checks.py (Diff revision 2)
The issue has been dropped. Show all issues
```
'CouchDBStorage' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
'from settings_local import *' used; unable to detect undefined names
```

Change Summary:

Fixed one more PEP8 issue introduced by the last fix

Diff:

Revision 3 (+190 -2)

Show changes

	reviewboard/settings.py
	reviewboard/urls.py
	reviewboard/accounts/forms.py
	reviewboard/accounts/views.py
	reviewboard/admin/checks.py
	reviewboard/admin/siteconfig.py
	reviewboard/templates/accounts/login.html
	reviewboard/templates/accounts/prefs.html

reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 26

 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 35

 E241 multiple spaces after ':'
```

reviewboard/admin/checks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'DNS' imported but unused
```
reviewboard/admin/checks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'S3BotoStorage' imported but unused
```
reviewboard/admin/checks.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'CouchDBStorage' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'from settings_local import *' used; unable to detect undefined names
```

People:: sgallagh

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?

puiterwijk Sept. 25, 2013, 3:09 a.m.

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

reviewboard/accounts/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should only be two blank lines between classes.
```

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