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

Added a "Forgotten password" maker #359

Merged
merged 1 commit into from Jan 18, 2020

Conversation

romaricdrigon
Copy link
Contributor

@romaricdrigon romaricdrigon commented Jan 31, 2019

Hello,

Having a password reset feature ("forgotten password") is a common feature of a web application. Maker bundle already has a User registration maker, it makes sense imo to add this too. Otherwise users are left either to code it manually (long), or to ditch Maker commands for FOSUserBundle.

I started implementing such a maker. It is heavily inspired from other Registration maker regarding style, and from FOSUserBundle process. Right now this is a work in progress, but I wanted to propose the idea & to discuss code style.

To do:

  • first working POC
  • add (minimal) view
  • add documentation describing the command
  • add tests - I got those are functional tests over the generated code?
  • squash & rebase everything

Points to be discussed:

  • is the controller right now looking good? I'm trying to have something minimalistic doing the job right, and easily modifiable. Not having too many services/helpers.
  • should the e-mail body be from a template, or does an heredoc looks sufficient to you?
  • which options would make sense for the command? for instance, redirecting or authenticating the user after having changed his password?
  • how to handle missing password setter/User class without an e-mail field? Display a warning in console?

Screenshots:

image

image

image

image

Thank you for this great bundle :)

@romaricdrigon romaricdrigon force-pushed the feat-forgotten-password branch 6 times, most recently from 593718a to 59805d7 Compare February 1, 2019 10:16
@romaricdrigon
Copy link
Contributor Author

Views were added, and the POC if fully functional (manually tested).
From now on I will stop amending my first commit, if people want to review/comment code.
I'm considering making the "request password" form (first screenshot) work only by e-mail address, as recommended here.

@sstok
Copy link

sstok commented Feb 1, 2019

I strongly suggest you read this 👍 https://paragonie.com/blog/2016/09/untangling-forget-me-knot-secure-account-recovery-made-simple

Almost all password reset systems are "horrible" insecure, tld;dr explanation: by using a single token (basically a random username) it's possible for an attack to guess the existing token by using a timing attack. When you provide a token, the database will use the index to look for an existing record, each character is checked against a binary-tree until it finds a record. When a character has a hit this means the time takes longer (because it continues the traversing of the tree), when there is no hit the time takes shorter because it bails out. Now using this technique an attack can use this information to try as many combinations as possible until all characters matches!

This is similar to a brute-force attack to guess a username/ID, but with a major difference. There is no password, and therefor access is directly granted.

By using the SplitToken you have selector (random username) and a verifier (random password in a hashed format), meaning that an attacker first needs to guess the selector, and then when there is a positive match, try to guess the verifier. BUT! Once you provide a wrong verifier for the selector, you simple revoke the token and no further tries are possible, even then guessing the verifier is nearly impossible as you compare the hashes in constant time (always takes the same amount of time) which leaks no timing information, making guessing nearly impossible.

I actually created a library that uses this technique https://github.com/rollerworks/split-token (MPLv2.0 licensed because it depends on HiddenString) but for most applications using the technique descried in the linked article is already a good step.

@romaricdrigon
Copy link
Contributor Author

@sstok thank you for the article, but I'm not sure implementing the suggestions from the article would be useful for most users The goal is to offer a basic feature, done right, for beginners. I'm pretty sure developers having tighter security requirements won't be using a maker command.
Hence I'm re-using a process from FOSUserBundle, which proved since a few years good enough for most, and follow some reasonable best practices (https://www.troyhunt.com/everything-you-ever-wanted-to-know/).

I like your approach, though. Did you consider make a "SplitTokenBundle"?

@gsylvestre
Copy link

This PR is a really good idea, and would be very usefull. I tried it and I love very much the generated code.

Some really small problems:

  • The generated twig files and folders do not follow coding stardards in their camelCased form (they should be snake_cased).
  • In the controller, method request(), is there any reason that you inject Twig_Environment instead of using the simpler approch of $this->renderView() ?
  • The suffix Action should be removed on controller's method checkEmailAction().

Also, I understand completely that you want to keep things simple and basic, and I agree. I just wonder if we could kinda incorporate some of @sstok suggestions by passing along the user email in the reset link. That would allow us to hash the reset token in the database, without adding much generated code and no other field in the entity. We even already use the UserPasswordEncoderInterface in the reset() method...

@gsylvestre
Copy link

In fact, maybe that those should be command options:

The reset password system need to store random tokens for your user. Do you want to store those:

  1. In your User entity
  2. In a separate entity

Would you like to hash the stored tokens (more secure)?

  1. Yes
  2. No

Would you like to add an extra "selector" field to protect against timed attack (more secure)?

  1. Yes
  2. No

Obviously, that is a lot of work to imlement, but that keep things simple for the end user.

@romaricdrigon
Copy link
Contributor Author

Thank you for the review @gsylvestre
I fixed the small problems, regarding \Twig_Environment at some point I wanted to render blocks independently, but I dropped this approach as it uses internal methods.

Regarding going further, I'm indecisive for now. I would like to get more feedbacks on this PR before going further regarding advanced features, about if and how it can integrates Maker bundle.

@sstok
Copy link

sstok commented Feb 7, 2019

FYI. I don't have any plans for a bundle to integrate SplitToken, in practice you can register as many SplitToken factories as you want.

The only thing that might be interesting is a FormType to handle this format native, I actually did this for Park-Manager . But that's something for later as it's still experimental.

@weaverryan
Copy link
Member

Sorry for the delay. I really want this - and it's on my list to review - but haven't had time yet. Thanks for getting it started!

@weaverryan
Copy link
Member

@gsylvestre I was interested in getting more info from your above comments:

In fact, maybe that those should be command options:

The reset password system need to store random tokens for your user. Do you want to store those:

  1. In your User entity
  2. In a separate entity

I don't think we should do this - that's a level of customization that you can do if you want to, but I don't see any advantage to adding this flexibility (or is there a security implication?)

Would you like to hash the stored tokens (more secure)?

  1. Yes
  2. No

I saw your other comment before this, but can you explain this a bit more? If we only had 1 field in the database, how is the token hashed and how does that improve security?

Would you like to add an extra "selector" field to protect against timed attack (more secure)?

  1. Yes
  2. No

Maybe ;). I do think this is something to keep in mind as we get closer - it's probably not that much more work to implement, if indeed this is an industry-standard way of making more secure reset passwords.

@weaverryan
Copy link
Member

Oh, I understand now about the hashed tokens: if we passed the email address in the reset URL, then we could look up the User by that email address, and then do a timing-safe check to see if the hash in the URL matches what we expect in the database.

Of course, then this also in theory allows for a timing attack on that "reset" URL, because you could use it to detect timing differences in return to determine if the email you entered is an email in the system :).

In general, I think we should be looking at other popular implementations (including FOSUserBundle, Laravel, Rails, whatever) and making sure we have the same safeguards at least as they do.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback started! I ran out of time to go through everything. But WOW! This is a crazy awesome and huge start! Let's work together and get this awesome command to the people :)

src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
src/Maker/MakeForgottenPassword.php Outdated Show resolved Hide resolved
@gsylvestre
Copy link

gsylvestre commented Mar 2, 2019

@weaverryan yes that's what I meant: if we hash the token in the database, we then need some kind of identifier to retrieve it. As you say, if we use the email, then an attacker can use timed attacks to find out if it's in the db... Really not a big concern imo, but still.

I was not proposing the "separate entity" option for security implication, not directly. But now that I have looked around and thought about it, I believe that we should always store the token in a new entity, instead of inside properties in the user class. See below...

Something like :

class PasswordResetToken 
{
    $id
    $selector //(crypto secure string, shorter)
    $token //(crypto secure string, longer)
    $requestedAt //(datetime)
    $user //(OneToOne unidir relation)
}

Pros:

  • We do not mess with User class by adding new properties and methods. It is often already clogged, and I feel it is intrusive.
  • We can then more freely add more fields and methods to our Token entity, like the generated "selector", or ie. if the developper want, he/she can add an expireAt field easily, or maybe use this entity for email confirmation purposes.
  • Indirectly more secure because we can now add the "selector" field
  • Until a password reset request is made, no token are generated. And no fields are sitting there being NULL and sad in the user table.
  • Better model structure, added flexibility

Cons:

  • Extra entity and database table are generated
  • more complicated?

In fact, is it really more complicated for the developper to understand what is going on? I believe even beginners will get this new entity easily.
And we are doing a really secure password reset. Why settle for less? We expect Symfony to provide top secure features.

BTW, Laravel is storing reset tokens in a new table, but use the email as the selector.

I can implement these changes next week, if the coding work is a problem.

@romaricdrigon
Copy link
Contributor Author

Thank you for your inputs both of you, and for your review @weaverryan, that's great to see things moving. Let's make this maker happen!

@gsylvestre I like that last idea you had, having a separate entity. It is not making things overly complex, but it both adds some security from what I got & code is a little bit cleaner in my opinion (it prevents User entity from growing too big...). I can work on it while fixing the other comments, but I would appreciate your review.

@sstok
Copy link

sstok commented Mar 4, 2019

Of course, then this also in theory allows for a timing attack on that "reset" URL, because you could use it to detect timing differences in return to determine if the email you entered is an email in the system :).

See the linked article for full details ;) https://paragonie.com/blog/2016/09/untangling-forget-me-knot-secure-account-recovery-made-simple

The main focus is that you have two factors for authenticating, a selector (like a username, email address or random token) and a verifier that acts like a password (with the same security requirements for verifying). Using only a random value (token) as-is (as the FOSUserBundle is doing) makes that your token is nothing more then a random id that can be guessed, I can't make it more clear than this 👍

@romaricdrigon
Copy link
Contributor Author

I pushed commits regarding the first review comments.
On top of that, I prefixed both form classes names by Password, I find PasswordRequestFormType and PasswordResettingFormType to be more explicit.

Now I will work on having a separate entity, and then on testing everything.

@stof
Copy link
Member

stof commented Mar 5, 2019

Note that if I were building FOSUserBundle today, I would probably use a separate table to store the reset tokens instead of putting things in the User class.

@sstok the issue of using the email address as the selector is that it allows using a timing attack to identify whether the email exists in the DB, allowing to leak the user base. That's why using a random selector instead is better if you try to protect the system against timing attacks.
Another option is to use the primary key of the user table...

@gsylvestre
Copy link

@stof, would you rather go with the random selector or the primary key?
I feel like the id would seem unsafe (for users and developpers to see the easily guessable user id in the url), even if it's mostly the same security level... It is one less field to generate on the other hand.

@romaricdrigon
Copy link
Contributor Author

I feel like having an extra "selector" field is not that bad, so for now I'm going with a selector separate from the ID.
One issue I'm having though: how to hash "token" without hardcoding a reference to the hash algorithm & without adding a dependency to an extra library?
For now, I'm considering getting the "UserPasswordEncoder", it feels a little bit hacky though.

@romaricdrigon romaricdrigon force-pushed the feat-forgotten-password branch 5 times, most recently from f8cffc9 to b2294a6 Compare March 12, 2019 14:37
@romaricdrigon romaricdrigon force-pushed the feat-forgotten-password branch 3 times, most recently from 7fb97e0 to e01c353 Compare September 20, 2019 07:41
@romaricdrigon
Copy link
Contributor Author

The PR is now good regarding feedback from @bocharsky-bw and @weaverryan !
I added a few tests regarding new features (retry TTL...), test are green on Travis / 7.3 and AppVeyor. I tested the Maker on a fresh Symfony project, I like the generated code, and I believe it will improve many applications :)

Also thank you @stof for your comments and @Pierstoval for your help.
There are still 2 comments open for discussion, which are either personal preference either to be done later I think.

@Pierstoval
Copy link

🚢 ❕

😄

@romaricdrigon
Copy link
Contributor Author

@bocharsky-bw and @weaverryan could you please check how I implemented modifications you requested?
I still hope it can get merged soon :)

@romaricdrigon romaricdrigon added Related Tests Pass Status: Waiting Feedback Needs feedback from the author and removed Status: Needs Work Additional work is needed labels Oct 25, 2019
@FBL-dev
Copy link

FBL-dev commented Nov 21, 2019

Do you know when your changes will be available? I would like to use them

@romaricdrigon
Copy link
Contributor Author

Do you know when your changes will be available? I would like to use them

@FBL-dev we had a chat over the feature today at SymfonyCon hack day, I rebased and fixed some small issues, hopefully @bocharsky-bw should review it soon, and I believe we are good for release :)

'security'
);
$dependencies->addClassDependency(
SwiftmailerBundle::class,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be the new Symfony Mailer component instead of Swiftmailer.

I tried to install this bundle, but even though I installed mail, the dependency was swiftmailer, this error kept popping up

Copy link
Contributor

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, we should use the Mailer component instead of Swiftmailer here, but I'm fine to merge this PR finally. Ideally, I think we should support both Mailer or Swiftmailer here. But this PR is really feature-rich so far and works great with Swiftmailer, so let's replace it with (or add integration of) the Mailer component in a separate PR, WDYT? I could help and take care of it when this is merged, just don't want to postpone further the merging of this awesome work that was made by the author so far. So, 👍 for me to merge it now

@romaricdrigon
Copy link
Contributor Author

@rbaarsma you have a point, but right now we don't quite have the infrastructure in MakerBundle to handle that (ie., have 2 different templates, switching according to installed dependency, test that...). I think SF5 is a topic we should treat "globally" in Maker bundle, maybe with some infrastructure refactor.

So I would prefer that PR to be merged, as @bocharsky-bw says, and then I will help with setting up that infrastructure + supporting Mailer. It does not degrade Maker as a whole, Maker won't work on SF5, but make:functional-test does not work too.

@romaricdrigon romaricdrigon added Status: Reviewed Has been reviewed by a maintainer and removed Status: Waiting Feedback Needs feedback from the author labels Nov 26, 2019
@weaverryan
Copy link
Member

Sorry for the slow merge on this. THANK you Romaric for your HUGE work here and also to Victor for helping to review and give feedback in my absence. I'm very excited about this feature :)

weaverryan added a commit that referenced this pull request Jan 18, 2020
This PR was merged into the 1.0-dev branch.

Discussion
----------

Added a "Forgotten password" maker

Hello,

Having a password reset feature ("forgotten password") is a common feature of a web application. Maker bundle already has a User registration maker, it makes sense imo to add this too. Otherwise users are left either to code it manually (long), or to ditch Maker commands for FOSUserBundle.

I started implementing such a maker. It is heavily inspired from other Registration maker regarding style, and from FOSUserBundle process. Right now this is a work in progress, but I wanted to propose the idea & to discuss code style.

To do:
 - [x] first working POC
 - [x] add (minimal) view
 - [x] add documentation describing the command
 - [x] add tests - I got those are functional tests over the generated code?
 - [x] squash & rebase everything

Points to be discussed:
 - is the controller right now looking good? I'm trying to have something minimalistic doing the job right, and easily modifiable. Not having too many services/helpers.
 - ~~should the e-mail body be from a template, or does an heredoc looks sufficient to you?~~
 - which options would make sense for the command? for instance, redirecting or authenticating the user after having changed his password?
 - how to handle missing password setter/User class without an e-mail field? Display a warning in console?

Screenshots:

![image](https://user-images.githubusercontent.com/919405/52117115-37911580-2613-11e9-8680-ba0a4a52901b.png)

![image](https://user-images.githubusercontent.com/919405/52117157-51caf380-2613-11e9-809b-b437ca24cecd.png)

![image](https://user-images.githubusercontent.com/919405/52117173-5ee7e280-2613-11e9-8d17-130d38a24750.png)

![image](https://user-images.githubusercontent.com/919405/52117218-7921c080-2613-11e9-9828-6e14b4424d53.png)

Thank you for this great bundle :)

Commits
-------

542591c Added a make:forgotten-password maker
@weaverryan weaverryan merged commit 542591c into symfony:master Jan 18, 2020
@romaricdrigon romaricdrigon deleted the feat-forgotten-password branch July 8, 2020 15:50
@darraghenright
Copy link

Hi all. I just came across this. I want to implement this forgotten password functionality and I see it is merged, but I cannot find any documentation for it, nor is it in the list of make: commands available in my project.

I'm using version 1.21.1 of the maker bundle.

@bocharsky-bw
Copy link
Contributor

@darraghenright see make:reset-password command, it should be there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Related Tests Pass Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet