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

Registering and confirming registration #2

Open
matjazpotocnik opened this Issue Sep 23, 2017 · 17 comments

Comments

Projects
None yet
8 participants
@matjazpotocnik
Copy link

matjazpotocnik commented Sep 23, 2017

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

@ryancramerdesign

This comment has been minimized.

Copy link
Owner

ryancramerdesign commented Sep 24, 2017

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.

@jacmaes

This comment has been minimized.

Copy link

jacmaes commented Oct 4, 2017

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.

@jmartsch

This comment has been minimized.

Copy link

jmartsch commented Oct 30, 2017

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

@fluctusz

This comment has been minimized.

Copy link

fluctusz commented Nov 29, 2017

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 :-(

@fluctusz

This comment has been minimized.

Copy link

fluctusz commented Nov 29, 2017

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.

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented May 1, 2018

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

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented May 2, 2018

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.

@mrjcgoodwin

This comment has been minimized.

Copy link

mrjcgoodwin commented Aug 7, 2018

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.

@TomS-

This comment has been minimized.

Copy link

TomS- commented Nov 15, 2018

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.

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented Nov 15, 2018

@TomS- I had the same experience as you - see my "fix" (#2 (comment)) until Ryan responds.

@TomS-

This comment has been minimized.

Copy link

TomS- commented Nov 15, 2018

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

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented Nov 15, 2018

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?

} else if($input->get('register_confirm') && $this->allowFeature('register')) {

and

} else if($input->get('register') && $this->allowFeature('register')) {

Just change the order of those two else if condition blocks.

@TomS-

This comment has been minimized.

Copy link

TomS- commented Nov 15, 2018

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

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented Nov 15, 2018

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

@TomS-

This comment has been minimized.

Copy link

TomS- commented Nov 15, 2018

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.

@TomS-

This comment has been minimized.

Copy link

TomS- commented Nov 15, 2018

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

@adrianbj

This comment has been minimized.

Copy link

adrianbj commented Jan 24, 2019

Obviously this issue is related to: #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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.