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

[6.x] Implement new password rule and password confirmation #30214

Merged
merged 4 commits into from
Oct 8, 2019
Merged

[6.x] Implement new password rule and password confirmation #30214

merged 4 commits into from
Oct 8, 2019

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Oct 8, 2019

These changes add a new password validation rule and the new password confirmation functionality.

The validation rule offers to validate a password input field and checks if the password matches the currently authed user's password. You can also pass a guard name as a parameter.

The password confirmation functionality behaves exactly like the Github confirmation screen, allowing you to set a password.confirm middleware on sensitive routes you want password protected. When a user enters a valid password, the time will be added to the session and its session will stay valid for a default of three hours (can be configured in the auth.php config file here: laravel/laravel#5129). Underneath the hood it makes use of the intended url to redirect the user to wherever they were going when the password confirmation was done.

Screen Shot 2019-10-08 at 13 24 35

Some remarks:

  • I went back and forth on some other naming like "reauthenticate" and "reconfirm password" but eventually settled for what was displayed in the UI (like Github)
  • The auth.password_timeout config option might need a better name but couldn't think of one
  • The "guest" method name of the Redirector class might need a rename because I noticed that this is actually a bad naming for what it actually does.

Feel free to provide feedback.

Kudos to @freekmurze, @mpociot, @christophrumpel and all the others that helped out with validating this idea. I first started working on this but later discovered this article below by @browner12 which inspired me a bit for this PR. Thanks!

laravel/ui PR: laravel/ui#34
laravel/laravel PR: laravel/laravel#5129

@taylorotwell taylorotwell merged commit 8b82aa3 into laravel:6.x Oct 8, 2019
@driesvints driesvints deleted the password-confirmation-rule branch October 8, 2019 12:24
@arcanedev-maroc
Copy link
Contributor

Thanks @driesvints 👍

@freekmurze
Copy link
Contributor

I very much like the functionality, but I find the name is a bit confusing.

If someone now says to you "I have a problem confirming a password", does that problem concern:

  • a problem with the second time you need to type your password when initially setting your password
  • a problem with this middlemare.

I think a name like Reauthorise would be better, Reconfirm could also be considered.

@browner12
Copy link
Contributor

Glad I could be an inspiration! Happy to see this in the core.

Couple of comments/questions:

  • 3 hours seems excessive for the default. Obviously this is a subjective value, but I would petition for more around the 1 hour mark.
  • Was the timer "reset" specifically left out or are you open to adding this functionality? The reset means whenever you visit an elevated security page, your timeout gets reset, so you don't need to keep re-authenticating.
  • Did you give any thought to allowing this to work on POST routes? One way I had considered doing this was adding all POST data as a hidden input on the re-authentication view. It's definitely a tricky question. If we don't want to support POST routes right away, we should add a disclaimer to the newly written docs.

My last point is a little more overarching. I think the way we handle injecting functionality into Controllers with built in features (auth, password resets, elevated security) is a little off. Having the Traits is great, but the methods of the Traits are too all inclusive and therefore (IMO) restrictive. A trait method should not encompass an entire route Controller method. I think it would be better if the methods were more "single responsibility", and then the programmer would use them to compose their own Controllers. This gives the programmer the freedom to make the flow of the Controller to work however they like, while also having the consistency of the methods in the Traits. This also prevents us from having to add a bunch of properties to customize every single use case. Maybe I'll bring this up elsewhere, because this is a concern I have with all of these features.

@driesvints
Copy link
Member Author

3 hours seems excessive for the default. Obviously this is a subjective value, but I would petition for more around the 1 hour mark.

Github also does "a few hours", hence the reason why I decided on this value.

Was the timer "reset" specifically left out or are you open to adding this functionality? The reset means whenever you visit an elevated security page, your timeout gets reset, so you don't need to keep re-authenticating.

Left out intentionally but I don't mind adding that either way. Not sure how @taylorotwell feels about that.

Did you give any thought to allowing this to work on POST routes? One way I had considered doing this was adding all POST data as a hidden input on the re-authentication view. It's definitely a tricky question. If we don't want to support POST routes right away, we should add a disclaimer to the newly written docs.

This is gonna be a bit tricky indeed. Not sure how to exactly solve this. In any case, the current middleware will still protect post routes, they'll just fail if people attempt to execute them manually.

I think the way we handle injecting functionality into Controllers with built in features (auth, password resets, elevated security) is a little off.

This is more of a discussion for the ideas repo.

@arcanedev-maroc
Copy link
Contributor

Hi @driesvints,

What if the user has an account without a password (Socialite) ?

What is the best approach ?

  • Resetting the password
  • Or skip the password confirm

@driesvints
Copy link
Member Author

@arcanedev-maroc this would only work with a password based flow.

@ajoy39
Copy link
Contributor

ajoy39 commented Oct 22, 2019

This is causing problems for some users, the Auth::routes() helper tries to register a route for this but the ConfirmPasswordController is a userland file that doesn't get added to existing applications on upgrade. Is there some way to add the file on upgrade? this is a really useful feature but as it stands it's also a breaking change for what should be a minor release

@potsky
Copy link

potsky commented Nov 27, 2019

Just a two hours headhach to share with you :-)

The password confirmation did not work anymore yesterday on a project. The culprit was https://github.com/Intervention/validation. It redefines the password validator and given that the password confirmation use it here and here, password confirmation does not work anymore if the password does not pass thru the package rules.

It is a convenient method to use this validator to check user password but given that the validator is not used when creating a new fresh user, we can reach this kind of asymmetrical problem.

I finally removed the package and implement manually the wanted validators I needed.

Thank you @driesvints for this new feature 💕 Really useful for hot transactions 🔥

@driesvints
Copy link
Member Author

driesvints commented Nov 28, 2019

Hey @potsky sorry to hear that. We can't unfortunately keep into account every third party library that might implement its own password rule, sorry. I think it's best that that library either renames its validator or creates a new one.

@juliobitencourt
Copy link

juliobitencourt commented Apr 9, 2020

This is a great feature @driesvints 🚀

I miss a way to check if the user is in the 3 hours timespan. I accomplished this by checking the auth.password_confirmed_at key in the session.

public function confirmedPassword()
{
    if (session('auth.password_confirmed_at') === null) {
        return false;
    }

    return session('auth.password_confirmed_at') > time();
}```

I'm I missing some point here?

@driesvints
Copy link
Member Author

Basically the logic here: https://github.com/laravel/framework/blob/7.x/src/Illuminate/Auth/Middleware/RequirePassword.php#L71-L73

@juliobitencourt
Copy link

Basically the logic here: https://github.com/laravel/framework/blob/7.x/src/Illuminate/Auth/Middleware/RequirePassword.php#L71-L73

This is not the ideas repo, but what do you think about this being added to the request session as a method?

@driesvints
Copy link
Member Author

Why do you need this?

@juliobitencourt
Copy link

Basically to show some sensitive data with partial views.

@driesvints
Copy link
Member Author

We'd have to store the timeout inside the session store for that and that's not wanted. We could do it somewhere else maybe but I'm not sure where. For now just re-use the logic.

@juliobitencourt
Copy link

juliobitencourt commented Apr 9, 2020

Thanks @driesvints I'll go with the logic re-used in a method in the User model.

public function confirmedPassword()
{
    $confirmedAt = time() - (session('auth.password_confirmed_at') ?? 0);

    return $confirmedAt < config('auth.password_timeout', 10800);
}

@emargareten
Copy link
Contributor

@driesvints

Did you give any thought to allowing this to work on POST routes? One way I had considered doing this was adding all POST data as a hidden input on the re-authentication view. It's definitely a tricky question. If we don't want to support POST routes right away, we should add a disclaimer to the newly written docs.

This is gonna be a bit tricky indeed. Not sure how to exactly solve this. In any case, the current middleware will still protect post routes, they'll just fail if people attempt to execute them manually.

Is there a possibility to have the entire request instance saved to the session and use that one after successful password confirmation instead of just saving the intended url?

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

Successfully merging this pull request may close these issues.

10 participants