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

Set password_confirmed_at on login #208

Merged
merged 2 commits into from
Nov 2, 2021
Merged

Conversation

tontonsb
Copy link
Contributor

@tontonsb tontonsb commented Nov 2, 2021

The current behaviour is such that when an unauthenticated user is accessing a confirmation-protected route, they are first asked to log in and then instantly asked to confirm the password. That confuses users and also is redundant as they just entered the password.

I have made the most straightforward solution here — explicitly set the password_confirmed_at timestamp as the user logs in.

@taylorotwell taylorotwell merged commit 21fd6c0 into laravel:3.x Nov 2, 2021
@jansgescheit
Copy link

In my case, that's a breaking change. We use JWT and not the Session Store.

{
    "class": "RuntimeException",
    "message": "Session store not set on request.",
    "code": 0,
    "file": "/var/www/html/vendor/laravel/framework/src/Illuminate/Http/Request.php:502",
    "trace": [
        "/var/www/html/vendor/laravel/ui/auth-backend/AuthenticatesUsers.php:47",
        "..."
}

@arthurkirkosa
Copy link

I have the same problem :/

Added \Illuminate\Session\Middleware\StartSession::class to $middleware in Kernel.php while a better solution is found.

@tontonsb
Copy link
Contributor Author

tontonsb commented Nov 5, 2021

Sorry, I didn't consider this would cause issues with session disabled.

@jansgescheit @arthurkirkosa can you check if changing line 47 in vendor/laravel/ui/auth-backend/AuthenticatesUsers.php to this solves the issue?

        optional($request->getSession())->put('auth.password_confirmed_at', time());

Or we could do just explicit

        if ($request->hasSession())
            $request->session()->put('auth.password_confirmed_at', time());

@driesvints
Copy link
Member

@tontonsb I'll get that fixed.

@arthurkirkosa
Copy link

It works with both variants

@driesvints
Copy link
Member

Released v3.3.2 which should fix the issues here.

@jansgescheit
Copy link

@tontonsb 👍 for the explicit variant

kohenkatz added a commit to kohenkatz/ui that referenced this pull request Aug 25, 2022
It is useful to use the same `AuthenticatesUsers` trait for login via API as for login in the browser, because the trait provides things like throttling and validation.

The problem is similar to the discussion in laravel#208 - API routes may not have a session.

This PR adds `if ($request->hasSession()) {` the same as the comments in laravel#208 suggest.

This also replaces laravel#90 properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants