[5.4] Crypto-based password resets #17052
Conversation
tillkruss
force-pushed
the
passwords
branch
5 times, most recently
from
429b9a5 to
bde653e
Compare
GrahamCampbell
changed the title
tillkruss
changed the title
| @@ -44,9 +52,12 @@ public function via($notifiable) | |||
| */ | |||
| public function toMail($notifiable) | |||
| { | |||
| $email = $notifiable->getEmailForPasswordReset(); | |||
| $link = url("password/reset?email={$email}&expiration={$this->expiration}&token={$this->token}"); | |||
There was a problem hiding this comment.
The URL has always been hard-coded, because Laravel is assuming you're using the Router::auth() routes.
If you want to use a custom route, you can use your own notification by adding a sendPasswordResetNotification() method to your class. See CanResetPassword trait.
Personally, I'd prefer it if Laravel were using route names instead of the url() helper everywhere, but that's a separate PR I recon?
There was a problem hiding this comment.
@tillkruss, that's a PR i'd love to see
There was a problem hiding this comment.
@taylorotwell Are you at all into this idea? Switch all url() calls with route() calls?
There was a problem hiding this comment.
Comparing route() against url(), the latter is significantly faster.
| $this->assertEquals(PasswordBroker::PASSWORD_RESET, $broker->reset(['password' => 'password', 'token' => 'token'], $callback)); | ||
| $this->assertEquals(['user' => $user, 'password' => 'password'], $_SERVER['__password.reset.test']); | ||
| } | ||
| // public function testGetUserThrowsExceptionIfUserDoesntImplementCanResetPassword() |
There was a problem hiding this comment.
Please don't comment out tests.
tillkruss
force-pushed
the
passwords
branch
3 times, most recently
from
c887124 to
18700c9
Compare
|
@GrahamCampbell: Sure, just adjusted all the unit tests. |
GrahamCampbell
dismissed
their stale review
Pending review from Taylor now. :)
|
@tillkruss thanks for that, getting rid of |
|
@lucasmichot: Seemed like the most straight forward approach to me. Do you want make a branch using JWT to see what is looks like? |
| * Create a notification instance. | ||
| * | ||
| * @param string $token | ||
| * @return void | ||
| */ | ||
| public function __construct($token) | ||
| public function __construct($token, $expiration) |
There was a problem hiding this comment.
You can add also parameter to PHPDoc, it will be @param int $expiration
| * @param \Illuminate\Contracts\Auth\UserProvider $users | ||
| * @return void | ||
| */ | ||
| public function __construct(TokenRepositoryInterface $tokens, | ||
| UserProvider $users) | ||
| public function __construct($app, UserProvider $users, $expiration) |
There was a problem hiding this comment.
Maybe is also good idea to type hinting here $app parameter.
| $email, | ||
| $expiration, | ||
| $user->getKey(), | ||
| $user->updated_at->timestamp, |
There was a problem hiding this comment.
Not sure about using the updated_at timestamp in here, an unrelated change to the user model would break an in-progress password reset and cause confusion for the user as to why their reset token was invalid. The password is sufficient to make the token unique and uncrackable.
tillkruss
force-pushed
the
passwords
branch
2 times, most recently
from
bb10bea to
af70f3c
Compare
|
Does this block the ability to use the same reset link twice? It looks like it does with the |
|
Yes, the link expires when the password hash changes. |
|
Perfect 👍🏼
|
|
This will probably have to be re-targeted to 5.5. I just don't have time to look at it for this release cycle. |
|
Probably fine to keep this open though, and just retarget? |
As discussed with @taylorotwell, a proof-of-concept for crypto-based password resets.
This approach allows us to drop the
password_resetstable and remove theTokenRepositoryInterfaceandDatabaseTokenRepository. Seelaravel/laravelchanges.The email field has been removed from the reset form.
If the reset link is expired/invalid, or the user doesn't exists, then the password reset form is hidden and an appropriate warning message is displayed.