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

feat: auth guard with multiple dynamic auth models #605

Merged
merged 12 commits into from
Jun 29, 2020

Conversation

0xb4lint
Copy link
Contributor

@0xb4lint 0xb4lint commented Jun 16, 2020

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

This PR handles the Guard interface and extracts the actual guard specified with, after the guard is extracted it loads the matching auth model (through the guard provider) from config and sets the return type (e.g. User, Admin, Customer...).

auth('admin')->user();
auth()->guard('admin')->user();
Auth::guard('admin')->user();

There are 2 new files added for tests app/Admin.php and config/auth.php.

(I've also added LoadsAuthModel trait, since getDefaultAuthModel() is implemented multiple times.)

Some example errors before this PR:

// Method Tests\\Features\\ReturnTypes\\Helpers\\AuthExtension::testAuthGuardUser() should return App\\User|null but returns Illuminate\\Contracts\\Auth\\Authenticatable|null.
public function testAuthGuardUser(): ?User
{
    return auth()->guard('web')->user();
}

// Method Tests\\Features\\ReturnTypes\\Helpers\\AuthExtension::testAuthGuardParameterUser() should return App\\User|null but returns Illuminate\\Contracts\\Auth\\Authenticatable|null.
public function testAuthGuardParameterUser(): ?User
{
    return auth('web')->user();
}

// Method Tests\\Features\\ReturnTypes\\AuthExtension::testGuardAdminUser() should return App\\Admin|null but returns Illuminate\\Contracts\\Auth\\Authenticatable|null.
public function testGuardAdminUser(): ?Admin
{
    return Auth::guard('admin')->user();
}

// Method Tests\\Features\\ReturnTypes\\Helpers\\AuthExtension::testAuthGuardAdminUser() should return App\\Admin|null but returns Illuminate\\Contracts\\Auth\\Authenticatable|null.
public function testAuthGuardAdminUser(): ?Admin
{
    return auth()->guard('admin')->user();
}

@szepeviktor
Copy link
Collaborator

Thank you.
Seem like everything is 🍏

@0xb4lint
Copy link
Contributor Author

@szepeviktor @canvural I've finished a feature that dynamically resolves the actual guard (from nodeKey) and loads model from config for the return type.

Like these (note the ?Admin typehint):

// config/auth.php
// 'guards' => [
//     ...
//     'admin' => [
//         'driver' => 'session',
//         'provider' => 'admins',
//     ],
// ],
// 'providers' => [
//     ...
//    'admins' => [
//         'driver' => 'eloquent',
//         'model' => App\Admin::class,
//     ],
// ],

public function testGuardAdminUser(): ?Admin
{
    return Auth::guard('admin')->user();
}

public function testAuthGuardAdminParameterUser(): ?Admin
{
    return auth('admin')->user();
}

Should I wait for this merge or add the commit for this PR?

@0xb4lint
Copy link
Contributor Author

@szepeviktor @canvural I've finished editing this PR. Added all the stuff for the dynamic guard return type feature. :)

@0xb4lint 0xb4lint changed the title Auth guard Auth guard with multiple dynamic auth models Jun 21, 2020
@0xb4lint
Copy link
Contributor Author

@canvural Have you had a chance to look at this yet?

@canvural
Copy link
Collaborator

Sorry no. I'm on holiday. I'll take a look next week 👍

@0xb4lint
Copy link
Contributor Author

All right, have fun! 😉

Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I left some comments. Other than that, it looks good!

src/ReturnTypes/GuardExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/GuardExtension.php Outdated Show resolved Hide resolved
tests/Application/app/Admin.php Outdated Show resolved Hide resolved
@0xb4lint
Copy link
Contributor Author

@canvural thanks for your feedback! I've fixed your comments, please check it.

@0xb4lint 0xb4lint requested a review from canvural June 29, 2020 20:36
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good 👍

@canvural canvural changed the title Auth guard with multiple dynamic auth models feat: auth guard with multiple dynamic auth models Jun 29, 2020
@canvural canvural merged commit 63a3934 into larastan:master Jun 29, 2020
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.

None yet

3 participants