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

Attempting to use with LdapRecord #527

Closed
snazy2000 opened this issue Jun 10, 2020 · 4 comments
Closed

Attempting to use with LdapRecord #527

snazy2000 opened this issue Jun 10, 2020 · 4 comments

Comments

@snazy2000
Copy link

I'm trying to use Bouncer with LdapRecord package, I'm overriding the user model with App\User within the AppServiceProvider in the boot function.

However when Bouncer loads it first loads the LdapRecord user model and I get

"Call to undefined method LdapRecord\Query\Model\ActiveDirectoryBuilder::getTable()"

Which is coming from

public static function setUsersModel($model)
    {
        static::$models[User::class] = $model;

        static::$tables['users'] = static::user()->getTable();
    }

Guessing its because its not a eloquent model.

I tried putting the

Bouncer::useUserModel(\App\User::class); 

within the register function so it kicks it off first which works but then the standard one overrides that.

I added an override flag against the setUsersModel

public static function setUsersModel($model, $override = false)
    {
        if (!isset(static::$models[User::class]) || $override == true)
        {
            static::$models[User::class] = $model;

            static::$tables['users'] = static::user()->getTable();
        }
    }

Then the useUserModel is setup like this

public function useUserModel($model)
    {
        Models::setUsersModel($model, true);

        return $this;
    }

I then had the useUserModel within the register function and it works as expected because its not trying to load something it shouldn't.

I'm not sure this is the best way to do this which is why I'm asking for any other options / feedback before I create a PR. The changes above wouldn't have any effect on previous cases as it still would override if required in the boot for other models, its just in this case it doesn't work because the LdapRecord model doesn't contain getTable.

Another possible option would be to have a config file that the model is read from instead of looking it up against the default guard which still allow an override if that config is not found (so it doesn't break all current implemented things)

@stevebauman
Copy link

Just ran into this as well -- and I agree on the use of an override flag (whether it be a static variable somewhere, or be added to the useUserModel method.

I'll see if I can write a PR to patch this.

@stevebauman
Copy link

stevebauman commented Jun 15, 2020

To elaborate further for @JosephSilber, the LdapRecord auth provider is registered like so:

'ldap' => [
    'driver' => 'ldap',
    'model' => LdapRecord\Models\ActiveDirectory\User::class,
    'database' => [
        'model' => App\User::class,
        'sync_passwords' => false,
        'sync_attributes' => [
            'name' => 'cn',
            'email' => 'mail',
        ],
    ],
],

So Bouncer is calling getTable() on an incorrect model instance -- even when configured manually via Bouncer::setUserModel().

@hbjydev
Copy link

hbjydev commented Jun 15, 2020

+1 to this, it's caused a halt in one of my company's projects

stevebauman added a commit to stevebauman/bouncer that referenced this issue Jun 15, 2020
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`.
@ankorinek
Copy link

The PR seems to not fully mitigate this issue. It defaults to App\User as the model when it should be App\Models\User as set in the ldapconfig.

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

No branches or pull requests

4 participants