Open matjazpotocnik opened 7 years ago
As a matter of security we validate both the session and the code. (We do the same with the core ProcessForgotPassword module). Maybe down the road we can provide an option for relaxing that further (ignoring the session), but for the short term at least I think better to stick with the most secure route.
Ryan, please consider adding a checkbox in the module configuration that would let us disable the session. It's increasingly a usability issue: people now routinely begin a task on a device, then finish on another. This means that they can start the sign-up on their computers, then click on the confirmation email on their phones –or vice versa. This breaks the session and they are unable to finish the registration process. I know because I've received reports from multiple users who did just that, and it's hard to justify (and explain) that, in the name of security, really they should stop using multiple devices for what we consider a single task, a continuous process.
I find also that I'm in incognito / private mode in my browser, the email confirmation also fails. In summary, these are two basic, increasingly common scenarios that break the process, and the user is left with a cryptic message that doesn't help clarify what went wrong, and why.
A checkbox like "Use sessions for enhanced security" could be enabled by default. Ticking it off would be a conscious decision on the developer's part to sacrifice some security for increased usability.
@jacmaes is so right about this. I wanted to write almost the same. I also want to register via an app on a mobile device and then send the activation mail to the user. When he clicks on this link the app should open and confirm the user, but how the module works right now, the session would not exist and so the user can not confirm his account or login.
Please let us disable the session.
i also came across this issue. by design, the registration data is only saved in the session before confirmation, so there's no easy way around, am i right? from my practice with small an medium sites, it's a pretty common task that users don't get the registration emails for one reason or another, then the support or site-owner gets contacted and needs a quick way to activate the user - send the mail again to trigger it out from greylisting spamfilters or activate the user directly in his control panel. so saving the registration-data as $user with a waiting-status or something else would be a big benefit for this really helpful module. sadly i'm also not a captain hook :-(
just for the case someone else is looking for a quick and dirty solution (for module version 0.0.2):
in ___processRegisterForm after sending the confirmation mail, i create the user and save the confirmation code in a field "user_confirmcode". so in LoginRegister.module Line 641 add: `
$users = $this->wire('users');
$values = $session->getFor($this, '');
$error = '';
if(!$this->allowName($values['register_name'], $error)) {
throw new WireException('Unable to complete registration - ' . $error);
}
$user = $users->newUser();
// populate values to new user
foreach($values as $key => $value) {
if(strpos($key, 'register_') !== 0) continue;
$key = str_replace('register_', '', $key);
if($key == 'roles') continue; // don't allow this, just in case
if($key != 'name' && !$user->template->fieldgroup->hasField($key)) continue;
$user->set($key, $value);
}
$user->set('user_confirmcode',$code);
// maybe here it's useful to not add the roles from the module settings, so the user is only guest. for me it's ok
foreach($this->registerRoles as $roleName) {
$role = $this->getRole($roleName);
if($role->id && !$role->hasPermission('page-edit')) {
$user->addRole($role);
}
}
try {
$this->createUserReady($user);
$user->save();
} catch(\Exception $e) {
throw new WireException('Unable to create user');
}
`
in processConfirmation() i look for a user with matching confirmation code, line 794 (with the above added): `
if($users->get('template=user, user_confirmcode='.$code)) {
// remove the value in field user_confirmcode here or do whatever makes the user a real user
return true;
}
`
in ___processLoginForm on Line 402 i check if field user_confirmcode is empty, if not, i give the message that the account isn't activated yet.
this is not a really secure solution, i know, but it works for my simple needs for only a newsletter-subscription with no post- or edit-permissions involved.
Even if I register and the confirm link is opened in the same browser I still see the "Register for an account" message and no new account is created. Not sure what's going on, but so far I can't get this working at all :)
I don't have time to figure out why, but register=1
was making it through when clicking the email confirmation link. My quick fix was to change the order of the:
} else if($input->get('register_confirm') && $this->allowFeature('register')) {
and
} else if($input->get('register') && $this->allowFeature('register')) {
so that if register_confirm
is provided, it will be processed first thereby preventing the register form from showing again.
Obviously the real reason needs to be uncovered though.
Hi all, just wondering if there has been any update on this? I too have run into a use case where users are complaining the link doesn't work if they click the link on their mobile are initiating registration on a desktop.
Just doesn't work for me at all. I just always get "Unable to complete confirmation, please re-register and try again". I don't mind the feature, but it has to work. This is using the same browser, same window. Copying and pasting the code in the email.
@TomS- I had the same experience as you - see my "fix" (https://github.com/ryancramerdesign/LoginRegister/issues/2#issuecomment-385845379) until Ryan responds.
@adrianbj this is also when copying and pasting the code inside the email not just clicking the link. Does this also apply? Where am I applying these changes?
Thanks Adrian.
this is also when copying and pasting the code inside the email not just clicking the link. Does this also apply?
Honestly not sure although I think it should - please let us know if it also works for that scenario.
Where am I applying these changes?
and
Just change the order of those two else if
condition blocks.
@adrianbj Nope :( doesn't seem to be working for me. I still get the error: "Unable to complete confirmation, please re-register and try again".
It doesn't take me to the register page like you have mentioned, it does take me to the login page. It just fails to authenticate. I'll look into this.
Here's a copy of the version I am using - note that I have also changed many other things as well. LoginRegister.module.txt
Obviously remove the .txt
extension :)
Thanks @adrianbj I'll have a look at it
Edit: Issue is $session->getFor($this, 'confirm_code')
returns nothing. Actually $session->getFor($this, '')
returns nothing. $sessions are not being set.
@adrianbj
It seems that sessions are not actually being set. I've checked everything and I can't seem to figure out why. They set normally in template files. Have you ever come across this? I haven't had any issue with anything else.
Obviously this issue is related to: https://github.com/ryancramerdesign/LoginRegister/issues/15
@ryancramerdesign - this module is proving very unreliable in its current state. If you don't have time to maintain it, do you think it's time to take it down?
When you fill the registration form in one browser and then you click the confirmation link send via email in another browser, the registration fails: Unable to complete confirmation, please re-register and try again. I know the reason for that, I guess registration data would have be saved to DB instead...