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

The "web" guard should be a configuration #20

Closed
filhocodes opened this issue Jan 10, 2020 · 12 comments
Closed

The "web" guard should be a configuration #20

filhocodes opened this issue Jan 10, 2020 · 12 comments

Comments

@filhocodes
Copy link

The cookies are handled by the SessionGuard, and you can have many as you want. I think most cases will be like me where we have one Guard for a UserProvider and a second Guard for a different UserProvider (like table users and customers).

Anyways, I think this value should not be hardcoded:
https://github.com/laravel/airlock/blob/master/src/Guard.php#L46
https://github.com/laravel/airlock/blob/master/src/Guard.php#L77

@driesvints driesvints changed the title Suggestion: The "web" guard should be a configuration The "web" guard should be a configuration Jan 10, 2020
@mbougarne
Copy link

Yes, something like this config('airlock.guard', 'web') The same thing with making request to /airlock/csrf-cookie it will be good if we have it configurable, we can change the PREFIX 'web' or 'api'.

@mansouralex
Copy link

@mbougarne does this help in having a 2nd guard by setting config('airlock.guard', 'web') on run time?

@filhocodes
Copy link
Author

IMO it should not be an Airlock configuration, but an configuration to the Airlock guard.

By example, in the config/auth.php you can configure a TokenGuard to hash the tokens by changing the 'hash' => false, into 'hash' => true, or event use a column different than api_token by adding a storage_key value. Right now the guard is configured right here https://github.com/laravel/airlock/blob/master/src/AirlockServiceProvider.php#L22-L25, and I think this configuration should be flexible.

The problem is that the "airlock" driver is not a "full Guard", but an extension of the RequestGuard, that is just a callable (the Laravel\Airlock\Guard in this case). This callable does not receive any other configuration except the current request and the user provider, and the second is just ignored in Airlock in favor of using the provider from another guard. Airlock is also a "singleton" guard in the sense that the User model is statically defined in the Airlock class, so you can't create multiple guards for the driver "airlock" because would it be always the same User (except with some shenanigans on runtime).

So in order to be able to configure the Airlock guard, and even to allow multiple guards of the Airlock driver, it would need a refactoring in RequestGuard on the laravel/framework to pass extra config into the callable, or a refactoring in Laravel\Airlock\Guard to be a "full custom guard".

And on that note, setting up Airlock to use "configuration by guard" instead of "configuration in airlock.php" would (at least in my mind) require a lot of other changes like, one example, also removing the Airlock::userModel and Airlock::$personalAccessTokenModel to be retrieved from the guard, either by it's provider or by configurations on it. In the end this would change the current scope of Airlock.

That is kinda why this is an issue and not a direct PR on my part, I don't want to assume what Airlock aims to be (a simple authentication solution or a full flexible one)

The one thing I'm certain is that this line https://github.com/laravel/airlock/blob/master/src/AirlockServiceProvider.php#L24 can be removed with the current state. Airlock is not using a provider for itself, and even the Laravel Docs example for Request Guard has a configuration without a provider

@driesvints
Copy link
Member

Airlock is meant for SPA's and apis and not meant for web based apps. If you need authentication for web based apps, consider using Passport: https://github.com/laravel/passport

@mansouralex
Copy link

@driesvints Passport also doesn't support multi UserProvider, even in SPA you had some use-cases where you need that....

@driesvints
Copy link
Member

@mansouralex right, sorry I misread this.

@callmez

This comment has been minimized.

@Wedrix
Copy link

Wedrix commented Mar 8, 2020

The way I'm solving it now is to define a guard_map config that maps specified route patterns to the appropriate guards. Then in guards.php I fetch the appropriate guard based on the request path. Seems to work for spa but don't know how it affects tokens.

@dellow
Copy link
Contributor

dellow commented Mar 10, 2020

Also using multiple guards would really like to see 'web' guard as a config option.

@driesvints
Copy link
Member

Pr was merged and will be in the next release

@wrabit
Copy link

wrabit commented Mar 29, 2020

Taylor removed the config a day later, but kept the functionality there, I guess it's not certain this feature will remain or how it will be implemented, for now I am setting the sanctum.guard value in the route service provider.

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

No branches or pull requests

8 participants