The Wayback Machine - https://web.archive.org/web/20210904015926/https://github.com/laravel/laravel/pull/5663
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

[8.x] Sanctum #5663

Merged
merged 4 commits into from Aug 11, 2021
Merged

[8.x] Sanctum #5663

merged 4 commits into from Aug 11, 2021

Conversation

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Aug 10, 2021

This PR serves as a proof of concept for what it would look like to just make Sanctum the default API authentication stack in a new Laravel application.

Motivations: The token driver is not robust and is no longer documented as of the Laravel 8.x release many months ago. As in, there is literally no documentation on the website about how to use it because I simply don't think it is a good idea to ever use it.

@amaelftah
Copy link
Contributor

@amaelftah amaelftah commented Aug 10, 2021

this is so cool please merge .... as since I started using laravel from 2016 i never used the default API driver

@elhadiabdallah
Copy link

@elhadiabdallah elhadiabdallah commented Aug 10, 2021

this is so cool

@GrahamCampbell GrahamCampbell changed the title Sanctum [8.x] Sanctum Aug 10, 2021

'api' => [
'driver' => 'token',
'provider' => 'users',
'hash' => false,
],
Comment on lines -43 to -48

This comment has been minimized.

@mfn

mfn Aug 11, 2021
Contributor

There's a comment above saying this; maybe the token mention should be removed there too? 🤔
image

@taylorotwell taylorotwell merged commit 226d1bf into 8.x Aug 11, 2021
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@taylorotwell taylorotwell deleted the sanctum branch Aug 11, 2021
@ldiebold
Copy link

@ldiebold ldiebold commented Aug 21, 2021

Just started a new projec. Was about to remove the default token based authentication...

Was thrilled to see sanctum already setup! Love this ❤️

@qunabu
Copy link

@qunabu qunabu commented Aug 26, 2021

With this PR merged all our packages failed on integration for actingAs (eg $response = $this->actingAs($billable)->json('GET', 'api/payments/');) method tests with testbench throwing the error

InvalidArgumentException: Auth guard [api] is not defined.

Solution to that was to add api guard to config from boot method in package ServiceProvider eg

public function boot() {
        $config = $this->app->make('config');
        $config->set('auth.guards', array_merge(
            [
                'api' => [
                    'driver' => 'passport',
                    'provider' => 'users',
                ],
            ],
            $config->get('auth.guards', [])
        ));
}

I'm posting this since finding the solution at this time was impossible with searching issues/forums/etc.

@driesvints
Copy link
Member

@driesvints driesvints commented Aug 26, 2021

@qunabu this is a starter skeleton package. You don't need to adopt changes made in existing apps. Just re-add the api guard to your auth config.

@qunabu
Copy link

@qunabu qunabu commented Aug 26, 2021

My comment refers only for Package Development and CI integration testing with testbench. The above is one possible solution, but you can also change this just for tests environment with

protected function getEnvironmentSetUp($app) {
    $app['config']->set('auth.guards.api', [
                    'driver' => 'passport',
                    'provider' => 'users',
                ]);
}

More context

I'm developing app with independent packages. If in you package you have in composer.json

"laravel/framework": "^8", or "laravel/framework": "^8.6",

and have been using api guard, code that used to work is no longer working because defaults laravel no longer have api guard as default. Integration API tests testbench is fetching one from composer, that's why you need to amend package code testing to be working.

With no change in code our CI/CD 2 days ago throwed an error
InvalidArgumentException: Auth guard [api] is not defined. for the code that used to work.

Like you @driesvints mentioned for the standard app development this is not a case at all, just packages.

PS. I'm going to change api guard with sanctum sooner then later

@crynobone
Copy link
Member

@crynobone crynobone commented Aug 26, 2021

Hi @qunabu

I intentionally sync the Laravel changes and release them as breaking changes to ensure that every packages depending on api knows that starting from last week release new created Laravel app doesn't come with api auth guard.

What you need to do as a package developer:

1. My package explicitly use api

You need to update getEnvironmentSetUp($app) to re-set the configuration and inform potential users that they need to set auth.guards.api configuration manually.

2. api is only used for packages tests, it shouldn't affect end-user

You just need to update getEnvironmentSetup($app)

@me-shaon
Copy link

@me-shaon me-shaon commented Aug 27, 2021

@qunabu I completely agree with you. We're facing the same problem and our packages CI/CD are failing for no reason.

@crynobone I think you are missing the point here. this change in this PR will only affect the new projects created by this skeleton. But your breaking change on a patch version on your package will affect existing projects completely out of the blue. This is not a professional move. I think you should adopt this change to the next major version of your package.

@qunabu
Copy link

@qunabu qunabu commented Aug 27, 2021

@crynobone

I intentionally sync the Laravel changes and release them as breaking changes to ensure that every packages depending on api knows that starting from last week release new created Laravel app doesn't come with api auth guard

I couldn't find this information can you point me out where is it.

Thanks for clarifying.

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