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

fixed information on resend password and recovery, related #856 #942

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

Conversation

schmunk42
Copy link
Contributor

Q A
Is bugfix? yes
New feature? yes
Breaks BC? no
Fixed issues related #856

Currently you can enter any e-mail address into recovery or resend password and you'll also get a success message even if the mail does not exists (eg. you've made a typo).

This PR adds a warning flash in these cases and outputs only success flashes when appropriate.

@thiagotalma
Copy link
Member

The possibility of "verifying" the existence of an email is not a very safe option.

I'm not sure this should be done.

The author of #856 has already reached the same conclusion.

@schmunk42
Copy link
Contributor Author

But always showing a success message is also not a appropriate solution. Any better ideas?

@philippfrenzel
Copy link
Contributor

Hi, shouldn't be the recovery form be validated on the existance of the user mail before sending?

@thiagotalma
Copy link
Member

I agree that having an unconditional success message is a problem.
What do you think of showing a message saying that the process was successful CASE EMAIL IS CORRECT.

Consider the following situation: a malicious person has a list of users leaked from a site and can test the existence of users with the same email and use the same password to access all accounts found.

I have no doubt that it is a major security breach.
The remedy against this would be a control to mitigate brute-force attacks, but that would not be feasible to be implemented here in that extension.

@schmunk42
Copy link
Contributor Author

Consider the following situation: a malicious person has a list of users leaked from a site and can test the existence of users with the same email and use the same password to access all accounts found.

But you do not need the password resend form for that, just use the login.

The remedy against this would be a control to mitigate brute-force attacks, but that would not be feasible to be implemented here in that extension.

I don't like them, but what's about an (optional) captcha?

@bscheshirwork
Copy link
Contributor

How about change only message?

add "if possible"/"if email correct"

@schmunk42
Copy link
Contributor Author

Consider the following situation: a malicious person has a list of users leaked from a site and can test the existence of users with the same email and use the same password to access all accounts found

But ... he can also try to login directly, what would be the difference here?
Don't like almost all web-sites disclose the information that an account exists? But yes, actually there should the tools to mitigate brute-force, also for the above.

add "if possible"/"if email correct"

It would be technically more correct, but makes no difference on the usability side.

@bscheshirwork
Copy link
Contributor

but makes no difference on the usability side.

I myself ran into the problem of misunderstanding when I tried to recover the password of a non-existent record (after the rollback of migrations)

I try it again and again. I can see "message send" and don't see email))) I check my smtp settings and go find error in code) And I forgot about it and repeat this after year)))

@thiagotalma
Copy link
Member

The solution could be here, but ......

@schmunk42
Copy link
Contributor Author

Does anyone know how usuario handles this? CC: @thyseus @tonydspaniard

@tonydspaniard
Copy link

tonydspaniard commented Jan 11, 2018

@schmunk42 https://github.com/2amigos/yii2-usuario/blob/master/src/User/Service/PasswordRecoveryService.php#L43

In yii-usuario If we don't find the email, we throw an error. Also, we send a token that requires verification in order to modify it. Only the owner of the email is allowed to modify the password: https://github.com/2amigos/yii2-usuario/blob/master/src/User/Controller/RecoveryController.php#L136

@schmunk42
Copy link
Contributor Author

@tonydspaniard Thanks for your feedback.

Are there measurements in usuario to prevent brute-force attacks or disclosing too much information, such if a user exists, like captchas, etc...?

@tonydspaniard
Copy link

tonydspaniard commented Jan 14, 2018

@schmunk42

Are there measurements in usuario to prevent brute-force attacks or disclosing too much information, such if a user exists, like captchas, etc...?

Yes sir, we use Google Recaptcha and also included Google 2Auth Authenticator.

@schmunk42
Copy link
Contributor Author

Any updates on this? I stumbled upon it in a project which requires this change.

@thiagotalma
Copy link
Member

It would not be good to implement this change without there being a control to avoid brute force.

@schmunk42
Copy link
Contributor Author

@thiagotalma Please make a proposal how that should look like.

  • always showing success is IMHO not an option
  • not showing a message on failure is the same like in the PR

A property to turn on/off messages completely?

@bscheshirwork
Copy link
Contributor

How about my opinion?

complex single message like "A message has been sent to your email address. It contains a confirmation link that you must click to complete registration. Of course, if you sure about registration on this email"

@schmunk42
Copy link
Contributor Author

How about adding an option extendedUserFlashMessages which can be set to true to enable this feature?

@bscheshirwork
Copy link
Contributor

How about adding an option extendedUserFlashMessages which can be set to true to enable this feature?

I will forget to enable this feature ;)

@bscheshirwork
Copy link
Contributor

So... I can see this in Nvidia resend form

something like
... "Email will be send (if associated with nvidia user) "...

@schmunk42
Copy link
Contributor Author

schmunk42 commented May 31, 2018 via email

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.

5 participants