Fixes #527 - Use custom table resolver for compatibility with other auth drivers

[stevebauman]

This provides the ability to successfully customize the user model, as the database table is only resolved when needed via the table method. This pattern is used in the Laravel core, such as Illuminate\Auth\AuthManager::$customCreators.

Once all of the applications service providers boot(), the user model will be overridden and its model will be used.

[hbjydev]

Can you detail how this works?

[stevebauman]

Can you detail how this works?

For sure 👍

Since the user model is set in the BouncerServiceProvider::boot() method:

bouncer/src/BouncerServiceProvider.php

Lines 32 to 35 in c6a9a98

	public function boot()
	{
	$this->registerMorphs();
	$this->setUserModel();

Which in-turn calls Models::setUsersModel($this->getUserModel()):

bouncer/src/BouncerServiceProvider.php

Lines 119 to 121 in c6a9a98

	protected function setUserModel()
	{
	Models::setUsersModel($this->getUserModel());

This patch updates the Models class and delays the resolution (or in other words, the "retrieval") of the User model database table to after all service providers have booted.

When Bouncer requires the users database table (via the Models::table('users') method), it will first look to see if a custom table closure exists inside of the static $customTableResolvers array, and then call the resulting closure, thus delaying the call of static::user()->getTable() until it is needed after all application service providers have booted (which is where we override the Bouncer user model in our own application).

This means that when we call Bouncer::useUserModel(\App\User::class) in our AppServiceProvider::boot() method, our configured model will be properly used to retrieve the database table when Bouncer requires it, instead of Bouncers default resolved model through the config/auth.php.

Hope this helps!

[hbjydev]

Awesome, that's just what I needed! Thanks a lot, mate. I'll be using your patch for the time being, haha

[JosephSilber]

I'm still unclear why this needs to be a callback.

Is your problem that Bouncer's service provider runs before your code changes the table on your User model? If so, why not have that same code update it for Bouncer as well?

[stevebauman]

I'm still unclear why this needs to be a callback.

The applications default guard will always be retrieved when Bouncer boots. This default guard must be an Eloquent model located at a specific key here:

bouncer/src/BouncerServiceProvider.php

Lines 129 to 142 in c6a9a98

	protected function getUserModel()
	{
	$config = $this->app->make('config');
	if (is_null($guard = $config->get('auth.defaults.guard'))) {
	return null;
	}
	if (is_null($provider = $config->get("auth.guards.{$guard}.provider"))) {
	return null;
	}
	return $config->get("auth.providers.{$provider}.model");
	}

This means, Bouncer is not compatible with non-standard guard configurations or any other authentication provider (as shown in the linked LdapRecord-Laravel issue), since the model key must be an instance of an Eloquent model. This is due to this call here:

bouncer/src/Database/Models.php

Lines 76 to 81 in c6a9a98

	public static function setUsersModel($model)
	{
	static::$models[User::class] = $model;
	static::$tables['users'] = static::user()->getTable();
	}

Problem is, even when you manually override the user Eloquent model, Bouncer will always create the default guard model (regardless of its type) and call the above getTable() method on the resulting instance.

Bouncer should wait to call this function (getTable()) on the instance, so it is called on the overridden Eloquent model, and not the one Bouncer decides during boot, which is overridden later.

Hope that explains it thoroughly!

Is your problem that Bouncer's service provider runs before your code changes the table on your User model? If so, why not have that same code update it for Bouncer as well?

This isn't about a table on a User model, its about the configured model instance itself in the config/auth.php provider definition that is always retrieved by Bouncer, even if you've attempted to override the User model yourself, since Bouncer always boots before our AppServiceProvider boots.

[hbjydev]

Can I get a bump on this? @JosephSilber

[mackhankins]

Any follow up on this?

[JosephSilber]

I still don't understand why setting this directly from your service provider isn't enough.

public function boot()
{
    Models::setTables(['users' => 'my_users_table']);
}

[stevebauman]

@JosephSilber Ok, I can do a couple things here:

1. Connect via Zoom and we can discuss the issue easier, rather than over GitHub issues
2. I can create a repository with a failing test case due to how Bouncer is resolving the Eloquent model

Let me know what option you would find most suitable. I’m only looking to improve the current implementation so Bouncer can be used with non-standard auth guards, which would help users of my LdapRecord-Laravel package.

I’ve written above what I felt was a decent explanation, but I’d rather not write another lengthy explanation if I’m not explaining it well enough.

[JosephSilber]

I can create a repository with a failing test case due to how Bouncer is resolving the Eloquent model

That would be superb 👍

[stevebauman]

Great, here you go 👍:

https://github.com/stevebauman/bouncer-custom-guards-issue

Summary:

public function test_bouncer_service_provider_fails_boot_with_custom_auth_guard()
{
    // This occurs during Laravel's boot process, because we
    // have a non-standard auth configuration shown above.
    // I needed to exclude Bouncer from loading in our
    // composer.json file to duplicate this issue.
    $this->expectExceptionMessage("Call to undefined method LdapRecord\Query\Model\ActiveDirectoryBuilder::getTable()");

    $this->app->register(BouncerServiceProvider::class);
}

public function test_custom_bouncer_service_provider_works_with_custom_auth_guard_because_of_delayed_resolution_using_table_resolver()
{
    // Notice how this does not fail, and we can then override the
    // model instance after it has been successfully registered.
    $this->app->register(CustomBouncerServiceProvider::class);

    CustomBouncerModels::setUsersModel(EloquentModel::class);

    $this->assertInstanceOf(EloquentModel::class, CustomBouncerModels::user());
}

https://github.com/stevebauman/bouncer-custom-guards-issue/blob/a80522980602d87d4bd5afa4fb72e49b3424e54c/tests/BouncerTest.php#L27-L47

Tests Passing:

https://github.com/stevebauman/bouncer-custom-guards-issue/actions

[JosephSilber]

@stevebauman thanks for that repo with the clear tests 👍

I think I now understand the issue. It's not that Bouncer is calling getTable() too soon. It's the fact that setUsersModel is not called with an Eloquent model, which is wrong in the first place.

Think about it this way: if there were a way to restrict the string class-type being passed into setUsersModel via a type-hint, it would surely restrict it to an Eloquent model. This code would blow up even before any line of code within that method is executed.

Calling setUsersModel with a non-Eloquent model, and then later calling it again with the correct model, is just totally wrong.

This PR is not a fix. It's duct tape.

---

This issue stems from the fact that the LDAP package uses a non-standard config for its auth. That means Bouncer can't figure out which user model to use, so you have to tell it yourself.

I see 2 possible solutions:

1. Tell users of the LDAP package to not discover Bouncer's service provider, and instead register their own:

   use App\User;
   use Silber\Bouncer\BouncerServiceProvider as Base;
   
   class BouncerServiceProvider extends Base
   {
       protected function getUserModel()
       {
           return User::class;
       }
   }

2. Always check if the auth model is actually an Eloquent model before calling setUsersModel with it:

   protected function setUserModel()
   {
       $class = $this->getUserModel();
   
       if (is_subclass_of($class, Eloquent\Model::class)) {
           Models::setUsersModel($this->getUserModel());
       }
   }

[stevebauman]

I think I now understand the issue. It's not that Bouncer is calling getTable() too soon.

Bouncer is calling getTable() on a possible non-existing model before the developers application providers boot. This is also the case if a developer isn't using an Eloquent user provider as their default guard.

This PR is not a fix. It's duct tape.

Absolutely. However, I surely wasn't going to modify many parts of Bouncer while it is still in the release candidate phase. Making large modifications to the core would have definitely gotten this PR denied immediately due to the sheer overhead of consuming what has changed.

This issue stems from the fact that the LDAP package uses a non-standard config for its auth.

No, this is the case with any non-eloquent provider. Developers are free to use the configuration array in any way they choose. There is no "standard".

Anyway, I've invested too much time in this PR. Have a good one man! ❤️

[JosephSilber]

This is also the case if a developer isn't using an Eloquent user provider as their default guard.

Bouncer is designed to work with Eloquent (it's right there in the tagline "Eloquent roles and abilities"). I don't even know if Bouncer could possibly work with a non-Eloquent users model.

I surely wasn't going to modify many parts of Bouncer while it is still in the release candidate phase. Making large modifications to the core would have definitely gotten this PR denied immediately due to the sheer overhead of consuming what has changed.

Of course! I wasn't criticizing your PR. Just trying to find an ideal solution.

Developers are free to use the configuration array in any way they choose. There is no "standard".

I wasn't implying there's anything wrong with the way LDAP is set up, just that it's different than the standard way the config is structured out of the box.

Anyway, I've invested too much time in this PR.

Fair.

---

I'm still interested in helping others with this issue.

If anyone wants to chime in with comments on the 2 proposed solutions, I'm all ears.

[JosephSilber]

I implemented the second option above in 429262a.

I also just tagged v1.0.0-rc.10, which includes this fix.

---

Thanks again @stevebauman for the detailed repo with the tests 👍