-
Notifications
You must be signed in to change notification settings - Fork 331
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
Fixes #527 - Use custom table resolver for compatibility with other auth drivers #528
Conversation
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`.
Can you detail how this works? |
For sure 👍 Since the user model is set in the bouncer/src/BouncerServiceProvider.php Lines 32 to 35 in c6a9a98
Which in-turn calls bouncer/src/BouncerServiceProvider.php Lines 119 to 121 in c6a9a98
This patch updates the When Bouncer requires the This means that when we call Hope this helps! |
Awesome, that's just what I needed! Thanks a lot, mate. I'll be using your patch for the time being, haha |
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 |
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
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 bouncer/src/Database/Models.php Lines 76 to 81 in c6a9a98
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 Bouncer should wait to call this function ( Hope that explains it thoroughly!
This isn't about a table on a User model, its about the configured model instance itself in the |
Can I get a bump on this? @JosephSilber |
Any follow up on this? |
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']);
} |
@JosephSilber Ok, I can do a couple things here:
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. |
That would be superb 👍 |
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());
} Tests Passing: https://github.com/stevebauman/bouncer-custom-guards-issue/actions |
@stevebauman thanks for that repo with the clear tests 👍 I think I now understand the issue. It's not that Bouncer is calling Think about it this way: if there were a way to restrict the string class-type being passed into Calling 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:
|
Bouncer is calling
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.
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! ❤️ |
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.
Of course! I wasn't criticizing your PR. Just trying to find an ideal solution.
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.
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. |
I implemented the second option above in 429262a. I also just tagged Thanks again @stevebauman for the detailed repo with the tests 👍 |
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 asIlluminate\Auth\AuthManager::$customCreators
.Once all of the applications service providers
boot()
, the user model will be overridden and its model will be used.