583797 - Implement new user registration

Status: RESOLVED FIXED

(Webtools Graveyard :: SSO (Legacy), defect, P2)

Description

[Fred Wenzel [:wenzel]]

Need some new user registration form, and probably a password reset. Can generously snag code for this from zamboni and/or django. We could also use the "django-registration" app, but we need to render via jinja2 using ``jingo.render()``.

Comment 1

[Fred Wenzel [:wenzel]]

btw. removed the previous templates here: http://github.com/mozilla/secret-squirrel/commit/7ed2562

Might be helpful when re-adding this functionality later.

Comment 2

[Austin King [:ozten]]

Attachment: Fix but not 100% integrated with cas_login logout, etc.

Providing a patch for review as I'm about to go offline for a few days :(

This patch can be applied with 
git apply --stat bug583797.patch1.diff

or you can look at the bug583797 branch on github.

I picked zamboni/app/users for the registration. I ran into lots of weird little issues and tweaks.
I didn't finish working through all the integration issues of login/logout.
Other notes:
Be sure to set your local email backend to dummy

Use local_settings.py to override

This includes an email confirmation setp... not sure we want that

Removed cas-provider from requirements/prod.txt (stumped me for a while)

To use SHA512, I had to 
alter table auth_user MODIFY password varchar(255);

Where I left off:
apps/cas_provider/views.py line 50 
auth_login(request, user) -- blows up as user has no backend...

Comment 3

[Fred Wenzel [:wenzel]]

Comment on attachment 463034 [details] [diff] [review]
Fix but not 100% integrated with cas_login logout, etc.

All right, I left some comments on the github branch. My general impression is that it is much more complicated to remove all the things we *don't* need from this huge patch than it would be to copy only the parts that are useful. One part of the patch was particularly interesting, the one that lets us use Django apps with Jinja2.

How about using the Django contrib auth app the way tuxedo does (you'll notice, it's just hooking up a few URLs and templates, otherwise it's all stock Django)? We could then pass it through the django/jinja2 code to make it work. I think we can implement the stronger password hash etc. unrelated from this.

What strikes me as a bad idea about the current patch is that while we pull in a lot of things we might need later (some L10n, etc.) we'll make this entirely unmaintainable if we import it in bulk. I think we should keep borrowing from Zamboni, but only to solve specific problems.

What do you think?

Comment 4

[Austin King [:ozten]]

(In reply to comment #3)

Thanks for the review. I agree. I checked out your github comments as well. I'll prepare a second attempt re-using 'django.contrib.auth'.

Comment 5

[Austin King [:ozten]]

Attachment: Second stab at adding registration

Second attempt at adding new user registration.
    
Taking a lighter approach this time. Reusing django.contrib.auth. 

Note: Email, First, and Last names are optional. No email verification step.
AFAIK there are no pre-made registration forms, so I didn't bring in the jinja_to_django helper functions.

You can also review this patch at http://github.com/mozilla/secret-squirrel/commits/583797_2 instead of the attachment.

Comment 6

[Austin King [:ozten]]

Attachment: Second stab at adding registration (updated with missing files)

Sigh. Attachment 464515 [details] [diff] was missing existing files. Attached is an updated patch for the second attempt.

Comment 7

[Fred Wenzel [:wenzel]]

Comment on attachment 464520 [details] [diff] [review]
Second stab at adding registration (updated with missing files)

This looks good. r+, with a few comments:
- sorry, I thought Django comes with a way to verify email addresses -- but apparently not. All they have is PW reset: http://github.com/fwenzel/tuxedo/blob/master/apps/users/urls.py
Do you think we need to add the verification step?
- You can probably put your code into the users app if you like.
- It's cool that you're using the messages framework to show the result. I am wondering if that (= showing messages if there are any) should be part of the base template instead?

Comment 8

[Fred Wenzel [:wenzel]]

Oh, and to request code reviews if you like you can just comment on a bug and point me to a branch on github, or send me a pull request there. If that's easier than generating a patch and attaching it here.

Comment 9

[Austin King [:ozten]]

(In reply to comment #7)

> sorry, I thought Django comes with a way to verify email addresses -- but
apparently not. All they have is PW reset:

We can always file a bug and add this later. We should think about it some more, but I'm leaning towards adding it.

Moved code into apps/users. The url is now /profile/register

Added a messages div to base.html when there are messages to be shown.

Sorry I didn't rebase my commits ;(