[5.5] Crypto-based password resets #17499
Conversation
tillkruss
force-pushed
the
passwords
branch
2 times, most recently
from
c40fd20
to
3da8d64
Compare
|
👏 nice |
| return $this->tokens->exists($user, $token); | ||
| $key = $this->app['config']['app.key']; | ||
|
|
||
| if (Str::startsWith($key, 'base64:')) { |
There was a problem hiding this comment.
Does nowhere else in the framework do this base64 decoding of the app key? Seems like there should be...
There was a problem hiding this comment.
Checkout EncryptionServiceProvider and KeyGenerateCommand.
There was a problem hiding this comment.
Ah, well that kind of code duplication is getting a bit messy. Going off topic a bit but I've proposed some improvements to this laravel/ideas/issues/416
|
Awesome stuff! 👍 |
| @@ -35,15 +45,15 @@ class PasswordBroker implements PasswordBrokerContract | |||
| /** | |||
| * Create a new password broker instance. | |||
| * | |||
| * @param \Illuminate\Auth\Passwords\TokenRepositoryInterface $tokens | |||
| * @param \Illuminate\Contracts\Foundation\Application $app | |||
There was a problem hiding this comment.
We shouldn't inject the entire application here just to get the key. Just inject the key.
There was a problem hiding this comment.
That makes sense. Changed in c709b25.
|
Would it be possible to keep and deprecate the old DatabaseTokenRepository for a release, instead of completely removing it? Relevant: laravel/ideas#383 (Deprecating changes for 1 release instead of removing) |
|
@sisve the suggestion in that thread seems to allude towards deprecate things "that can be aliased" - I don't see any point in keeping the old system for one release in this instance - having it as upgrade notes should be suitable. |
|
As an alternative to deprecating it, the |
|
can you ELI5 what this means, and what the process kind of looks like? |
|
Is this somewhat simular as a JWT? The concept looks familiar. Looks good 👍 |
|
Yeah somewhat similar, here is an article on it. |
|
@garygreen Taylor has now commented that "we will attempt to deprecate removals for 1 release in the future". I see no reason that crypto-based password resets cannot live side-by-side. Isn't it, to over-simplify it, just two different kinds of token generators/validators; one that uses a repository, another that uses this new technology? Or, to turn the question around; why must the old functionality be removed when this is introduced? |
|
Honestly I think I may table this for now unless there is a severe security problem with our current approach. Trying to stabilize out these releases a bit more. |
|
Okay. If anyone wants this as a package, LMK. |
This implements the ability to generate signed (and tempoorary) URLs. These URLs may be easily verified as having been generated by your application and not modified by the end-user.
Same as #17052.
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.