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

[v1.0] Extensibility of Actions using Authentication User Providers #17

Closed
stevebauman opened this issue Sep 8, 2020 · 14 comments
Closed

Comments

@stevebauman
Copy link
Contributor

stevebauman commented Sep 8, 2020

I know this is extremely early since this just got released -- but I'm wondering if there's any plans on adding the ability to override the included actions, or for the actions to utilize other authentication providers besides eloquent?

The current RedirectIfTwoFactorAuthenticatable action uses a getModel() method that is not currently inside the Stateful guard contract:

$request, $this->guard->getProvider()->getModel()

This action (RedirectIfTwoFactorAuthenticatable) is also responsible for validating the users credentials -- but isn't the authentication UserProvider responsible for this?

https://github.com/laravel/framework/blob/c87794fc354941729d1f0c4607693c0b8d2cfda2/src/Illuminate/Contracts/Auth/UserProvider.php#L48

The other concern is that the AttemptToAuthenticate action only allows customization of the username passed into the $guard->attempt() method, while Laravel UI allows you to customize the whole array of credentials passed in:

AttemptToAuthenticate Action in Fortify:

public function handle($request, $next)
{
if ($this->guard->attempt(
$request->only(Fortify::username(), 'password'),
$request->filled('remember'))
) {
return $next($request);
}

AuthenticatesUsers Trait in Laravel UI:
https://github.com/laravel/ui/blob/6ed3b97576fc99049ba576de4cfab5cef4771ab3/auth-backend/AuthenticatesUsers.php#L93-L96

Please don't take these questions in any sort of negative manor -- I'm immensely grateful for the insane amount of free work that was put into this! ❤️

If there is interest in this possibility, I can work on a PR to add this extensibility and you can look at it to see if it's something you like / don't like. If it's denied, no hard feelings 👍

Related: #16, DirectoryTree/LdapRecord-Laravel#196

@taylorotwell
Copy link
Member

Fortify 1.1.0 has been released with Laravel\Fortify\Fortify::loginThrough method... this method should return the authentication pipeline array you wish to use - allowing full customization. Could add this to the boot method of your JetstreamServiceProvider:

Fortify::loginThrough(function ($request) {
    return [
        //
    ];
});

@stevebauman
Copy link
Contributor Author

Awesome thanks @taylorotwell! Let me verify this right now on my install and see if I can now use my own auth guard 👍

@taylorotwell
Copy link
Member

This is sort of the "bazooka" approach and gives the most flexibility. I think there is probably still room for some more granular customization hooks that don't require customizing the pipeline.

@taylorotwell
Copy link
Member

taylorotwell commented Sep 10, 2020

OK I have tagged v1.2.0 with a more granular approach that I believe will solve your original use case a little easier. The loginThrough method I noted earlier still works though if you prefer a wider customization.

There is a new authenticateUsing(fn) method which receives the request and should retrieve the authenticatable user (however you want) using the data from the request and return the user instance or, if there is not a user matching those credentials, you should return null or false. Note that you are responsible for validating the password, etc.

This custom callback will be utilized by both RedirectIfTwoFactorAuthenticatable and AttemptToAuthenticate.

Fortify::authenticateUsing(function ($request) {
    $user = User::where('email', $request->email)->first();

    if (! $user || ! Hash::check($request->password, $user->password)) {
        return;
    }

    return $user;
});

@taylorotwell
Copy link
Member

@stevebauman when you have time, i would be curious to know if the new authenticateUsing method noted above solves your use case.

@stevebauman
Copy link
Contributor Author

stevebauman commented Sep 10, 2020

Perfect, the Fortify::authenticateUsing callback is exactly what I need. Here's how I'm now able to login using my own authentication provider:

// app/Providers/AppServiceProvider.php

public function boot()
{
    Fortify::authenticateUsing(function ($request) {
        Auth::attempt([
            // "mail" is an LDAP attribute
            'mail' => $request->email,
            'password' => $request->password
        ]);

        return Auth::user();
    });
}

This is the only change I need to make. I didn't need to configure anything at all in Jetstream. I did a complete new Jetstream installation, installed LdapRecord-Laravel, configured it, then added this callback.

Excellent! 👍 🎉

@taylorotwell
Copy link
Member

taylorotwell commented Sep 10, 2020

@stevebauman cool. my only suggestion is not to use Auth::attempt there. I would maybe use Auth::validate instead, which accepts the same arguments.

Reason being is that when authenticateUsing return a user instance then Fortify will call Auth::login for you. So you're sort of logging in twice here. Using Auth::attempt there is also not a good idea if you're using two-factor authentication. Not sure if you are. Because here you would have logged them in before they confirmed their two factor token.

@stevebauman
Copy link
Contributor Author

stevebauman commented Sep 10, 2020

@taylorotwell Ah okay understood - thanks! Here's what I've updated it to:

// app/Providers/AppServiceProvider.php

public function boot()
{
    Fortify::authenticateUsing(function ($request) {
        $validated = Auth::validate($credentials = [
            'mail' => $request->email,
            'password' => $request->password
        ]);

        return $validated ? Auth::getProvider()->retrieveByCredentials($credentials) : null;
    });
}

In my case, I need to all retrieveByCredentials() on the provider to retrieve the user instance from the LDAP directory.

@taylorotwell
Copy link
Member

Yeah, that looks good!

@ninjaparade
Copy link

image

@mikeburton220
Copy link

mikeburton220 commented Oct 16, 2020

Yeah, that looks good!

Hi Taylor, is there a way to utilize "remember" using Auth::validate with authenticateUsing, maybe it should return an array, instead of just the User model since it's going to call Auth::login ? [User, $remember]

@stevebauman
Copy link
Contributor Author

stevebauman commented Oct 16, 2020

Hi @mikeburton220,

The filled status of the login requests remember field will still be passed into the guard->login() method, as shown here:

protected function handleUsingCustomCallback($request, $next)
{
$user = call_user_func(Fortify::$authenticateUsingCallback, $request);
if (! $user) {
return $this->throwFailedAuthenticationException($request);
}
$this->guard->login($user, $request->filled('remember'));
return $next($request);
}

Simply send the remember input value with the login request, and you're good to go 👍

@fahmiegerton
Copy link

fahmiegerton commented Feb 1, 2022

OK I have tagged v1.2.0 with a more granular approach that I believe will solve your original use case a little easier. The loginThrough method I noted earlier still works though if you prefer a wider customization.

There is a new authenticateUsing(fn) method which receives the request and should retrieve the authenticatable user (however you want) using the data from the request and return the user instance or, if there is not a user matching those credentials, you should return null or false. Note that you are responsible for validating the password, etc.

This custom callback will be utilized by both RedirectIfTwoFactorAuthenticatable and AttemptToAuthenticate.

Fortify::authenticateUsing(function ($request) {
    $user = User::where('email', $request->email)->first();

    if (! $user || ! Hash::check($request->password, $user->password)) {
        return;
    }

    return $user;
});

I still got this problem, I just want to add additional where('is_active', true) but got
image

if I didn't checked the remember me box, it will be fine, otherwise it doesn't work :(.

@ToneZen
Copy link

ToneZen commented Oct 12, 2022

@stevebauman @mikeburton220
'remember' doesn't work at all after adding 'remember: true' to the login request parameter

This is my JS login request code

await axios.get('/sanctum/csrf-cookie').then( () => {
    // Login...
    axios.post('/api/login', {email, password, remember: true} ).then(response => {
        if (response.data) {
            window.open(redirectToAfterLogin_url, '_self');
        }
    }).catch((err) => {
        setError(err.response.data.errors[Object.keys(err.response.data.errors)[0]]);
    })
});

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

No branches or pull requests

6 participants