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

isLockedOut() in Member.php call LoginAttempt::getByEmail but it passes to it the unique_identifier_field instead $this->Email #9516

Conversation

alessandromarotta
Copy link

The public function isLockedOut() in Member.php call LoginAttempt::getByEmail but serves to it the unique_identifier_field.

This PR could allow to extensions to patch the use of uniqueidentifierfield (otherwise it would be necessary to extends the Member Class to override the isLockedOut function, with a lot of problems)

The public function isLockedOut() in Member.php call LoginAttempt::getByEmail but serves to it the unique_identifier_field.

This PR could allow to extensions to patch the use of uniqueidentifierfield (otherwise it would be necessary to extends the Member Class to override the isLockedOut function, with a lot of problems)
@alessandromarotta alessandromarotta changed the title Update Member.php isLockedOut() in Member.php call LoginAttempt::getByEmail but it passes to it the unique_identifier_field instead $this->Email May 17, 2020
@dhensby
Copy link
Contributor

dhensby commented May 17, 2020

See #9507

@kinglozzer
Copy link
Member

If email isn’t the unique identifier field, then you could have multiple accounts with the same email address (or, my bigger concern, null). If the email address can be null, one person getting themselves locked out could potentially result in everyone being locked out

@dhensby
Copy link
Contributor

dhensby commented May 18, 2020

This function is explicitly about getting loginattempts by email; not about getting it by unquie identifier.

The MemberAuthentictor only pushes the Email and doesn't try to discern the unique identifier of a member, so it's all a bit moot.

If potentially null emails are a problem, then we could filter that out when looking up or creating loginattempts.

I think if developers are taking the step to use a different identifier for authentication then I think they would be taking a lot greater steps than just assuming all this stuff works without a decent amount of effort.

Login attempt tracking won't work if the submitted form data doesn't contain an Email field, so if you aren't logging in with Email and you want login tracking, you're going to have to do a decent amount of customisation.

@alessandromarotta
Copy link
Author

alessandromarotta commented May 18, 2020

I think that the email should always be present and required for the registration of a user, as it is essential for password recovery. So I think it is correct to record (and get) the login attempts by email address, as it is the only field certainly present and real.

@dhensby
Copy link
Contributor

dhensby commented May 18, 2020

Just because email addresses are provided during registration doesn't mean they'll be present during login, though. If you login with a username that doesn't exist, how do you track an email against it?

@robbieaverill robbieaverill merged commit 7184703 into silverstripe:4 Oct 2, 2020
@robbieaverill
Copy link
Contributor

Thanks @alessandromarotta

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

Successfully merging this pull request may close these issues.

None yet

5 participants