joscha
returning SignupResult.USER_CREATED doesnt send verification email
When a user is signed up using UsernamePasswordAuthenticator, and we want to sign in the user without verifying their email first, then you return SignupResult.USER_CREATED instead of USER_CREATED_UNVERIFIED like so:
protected com.feth.play.module.pa.providers.password.UsernamePasswordAuthProvider.SignupResult signupUser(final MyUsernamePasswordAuthUser user) {
final User u = User.findByUsernamePasswordIdentity(user);
if (u != null) {
if (u.emailValidated) {
// This user exists, has its email validated and is active
return SignupResult.USER_EXISTS;
} else {
// this user exists, is active but has not yet validated its
// email
return SignupResult.USER_EXISTS_UNVERIFIED;
}
}
// The user either does not exist or is inactive - create a new one
@SuppressWarnings("unused")
final User newUser = User.create(user);
// Usually the email should be verified before allowing login, however
// if you return
return SignupResult.USER_CREATED;
// then the user gets logged in directly
//return SignupResult.USER_CREATED_UNVERIFIED;
}
The problem is that the super method does not send the verification email when USER_CREATED is returned and since we dont have access to the context from here, we cant send it ourselves.
I suggest either sending the verification email when both USER_CREATED && USER_CREATED_UNVERIFIED is returned, OR pass the context down to this method so we can handle it ourselves if we want to.
I prefer passing down the context. It would also be useful if the method: sendVerifyEmailMailing(final Context ctx, final US user) be made protected as well so we can call it from our subclass.
Returning USER_CREATED means that there is no verification, e.g. the email is not needed - why would you want the user to verify his/her email address, if you allow that person to log in without that verification anyways?
Passing the context down would be an option, making sendVerifyEmailMailing protected is a bad idea I think because then there is virtually no API any more, but just a shell of a provider that can be used to do virtually anything.
Are you aware that you can write your very own login provider completely decoupled from any provider code that is implemented and shipped with Play! Authenticate already? If you have very special needs regarding the flow in your application that might be the easier way to go.
Your right about making my own provider. I didn't take this approach as I just needed a small correction with the default one.
In my use case, I still want users to verify their emails, however, I want them to be able to have a seamless workflow when they signup and not be bothered with verifying until later on. I still wanted to send out an email when a user signs up which makes passing down the Context useful.
After your response, I will be making my own provider to do exactly what I want, although passing the context down could be a good change if you're willing to make a breaking change. Otherwise, I have what I need to correct my code.
On Sun, Mar 3, 2013 at 3:30 PM, Joscha Feth [email protected]:
Returning USER_CREATED means that there is no verification, e.g. the email is not needed - why would you want the user to verify his/her email address, if you allow that person to log in without that verification anyways? Passing the context down would be an option, making sendVerifyEmailMailingprotected is a bad idea I think because then there is virtually no API any more, but just a shell of a provider that can be used to do virtually anything.
Are you aware that you can write your very own login provider completely decoupled from any provider code that is implemented and shipped with Play! Authenticate already? If you have very special needs regarding the flow in your application that might be the easier way to go.
— Reply to this email directly or view it on GitHubhttps://github.com/joscha/play-authenticate/issues/61#issuecomment-14354526 .
To be honest I'd rather not make a breaking change if it's not really needed, because lots of people just upgrade to the latest version without reading the release notes :-O and then it won't work any more. However I think this can be changed in a backwards compatible manner by overloading with a protected method and then calling the one that is implemented in the subclass without context, so if you are willing to provide a pull request that does just that, I'd consider merging it - if you are going to implement your own provider though, I'd recommend to let the interface be stable :)