Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One-step user registration #260

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

tabdulradi
Copy link

This pull request creates another registration controller, that works in parallel with the old one, without breaking it.
It allows users to register in one step, but marked as non-verified, later they can verify their account by following a link send in a verification email.

These are changes in the code:

  • Moves old registration controller inside "registration" package
  • Creates a new registration controller that creates the User in one step
  • Creates an action for email verification
  • Add views for the new actions/emails
  • Adds methods for new views, in Mailer and TemplatePlugin (without breaking backward compatibilty)
  • Adds a state field in Identity trait, to hold the state of email verification
  • Adds configuration options to choose the registration method
  • Changes "SecuredAction" to prevent users with non-verified email

I have to note that this feature was implemented by one of our trainees, then reviewed by me.
You can see her original changes here

Todo:
We are planning to change the SecuredAction (or extend it) to take additional parameters, to allow non-verified users in specific pages, or even allow (recent) non-verified users only.

- Moves old registration controller inside "registration" package
- Creates a new registration controller that creates the User in one step
- Creates an action for email verification
- Add views for the new actions/emails
- Adds methods for new views, in Mailer and TemplatePlugin (without breaking backward compatibilty)
- Adds a state field in Identity trait, to hold the state of email verification
- Adds configuration options to choose the registration method
@jaliss
Copy link
Owner

jaliss commented Aug 14, 2013

Hi @tabdulradi

Thanks for your contribution. I did a quick look and would need to see this in more detail but here are my first comments:

  1. The registration flow leaks user information (error saying the email is taken). The default flow prevents that.
  2. Adding Active SocialUser might to be needed. Since the change in SecuredAction line 86 does a filter on the result, it really would be the same if your UserService.find method would return None for inactive users.
  3. As of your planned changes to SecuredAction please consider using an Authorization object as an alternative. That would allow your users to access certain pages: in the implementation of that object you could check if the user is active or not. If you make UserService.find return your own model that should be easy. Eg:
 case class WithActiveState extends Authorization {
       def isAuthorized(user: Identity): Boolean {
             user match {
                   case myUser: MyUserClass => myUser.isActive
                   case _ => // did not get a MyUserClass instance, log/handle error. Should not happen.
             }
        }
  }

@tabdulradi
Copy link
Author

Thanks for your reply,

  1. I think we can remove the validation on email field. Then take a decision, either we should send a welcome/validation email, or send instructions email. We will work on it by next week.
  2. UserAwareAction uses UserService.find method too, so returning None for inactive users will prevent us from allowing Inactive users to access specific pages in the future.
  3. Thanks for the note. We will take this into consideration.

@tabdulradi
Copy link
Author

What do you think now?

@tmbo
Copy link

tmbo commented Nov 12, 2013

@jaliss are you going to integrate this feature into securesocial in the near future?

@paiou
Copy link

paiou commented Feb 15, 2014

Hi @jaliss .
Are you interested in integrating this feature into SecureSocial? I need it for my project so I'm currently updating it to the latest code in master, and doing some small improvements. Shall I create a new pull request when I'm done?
Thanks for your hard work on this great framework.

@jakob85
Copy link

jakob85 commented Feb 22, 2014

Great work all of you! +1

@zfy0701
Copy link

zfy0701 commented Jul 15, 2014

+1 for the great works!

Is there any conclusion this would eventually be pulled or not?

@jaliss I understand that saying the email is taken might leak some user information, but it seems really few sites didn't do that. I feel this feature is essential to have, and it's developer's call to use it or not.

by the way, it seems one of the user in list in securesocial's user list, carambla, is using one step registration
https://carambla.com/register

@biokys
Copy link
Contributor

biokys commented Nov 3, 2014

Hi, what about merge? Will you resolve conflicts?

@olmaga
Copy link

olmaga commented Jan 1, 2015

+1 from my side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants