[Bug] Laravel Nova - Call to undefined method ...\Model\ActiveDirectoryBuilder::getKeyType() on Migrate

[scramatte]

Hello,

I've rollback all migration from a project. And I when I try to migrate back everything I'got the following error ... Any Idea ?

php artisan migrate
Migrating: 2018_01_01_000000_create_action_events_table

   BadMethodCallException 

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

  at vendor/directorytree/ldaprecord/src/Query/Builder.php:1967
    1963▕         if (substr($method, 0, 5) === 'where') {
    1964▕             return $this->dynamicWhere($method, $parameters);
    1965▕         }
    1966▕ 
  ➜ 1967▕         throw new BadMethodCallException(sprintf(
    1968▕             'Call to undefined method %s::%s()',
    1969▕             static::class,
    1970▕             $method
    1971▕         ));

      +31 vendor frames 
  32  artisan:37

[stevebauman]

Hi @scramatte,

Looks like you’re running something in your migration that is causing the exception. I’m going to need that code to be able to help in any way. Thanks!

[scramatte]

This migration is from "Laravel Nova" itself

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Schema\Builder;
use Illuminate\Support\Facades\Schema;
use Laravel\Nova\Util;

class CreateActionEventsTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('action_events', function (Blueprint $table) {
            $table->id();
            $table->char('batch_id', 36);
            $table->foreignIdFor(Util::userModel(), 'user_id')->index();
            $table->string('name');
            $table->morphs('actionable');
            $table->morphs('target');
            $table->string('model_type');

            if (Builder::$defaultMorphKeyType === 'uuid') {
                $table->uuid('model_id')->nullable();
            } else {
                $table->unsignedBigInteger('model_id')->nullable();
            }

            $table->text('fields');
            $table->string('status', 25)->default('running');
            $table->text('exception');
            $table->timestamps();

            $table->index(['batch_id', 'model_type', 'model_id']);
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::dropIfExists('action_events');
    }
}

[scramatte]

My project use laravel 9.2 + nova 4

[scramatte]

I suspect the following line

$table->foreignIdFor(Util::userModel(), 'user_id')->index();

[scramatte]

If you disable "ldap" auth.php provider and set back temporarily the default eloquent one you can migrate everything without issue.

[stevebauman]

Looks like you're using an LdapRecord model in place of a Laravel Eloquent model. This won't work (as you've seen from the undefined method call). You'll have to replace the Util::userModel() method with a Laravel Eloquent model or change what Util::userModel() returns -- if that's possible (I haven't used Laravel Nova so I'm unaware of its configurability).

[scramatte]

I think that It's not posible to custom this. So may we can add some method into LdapRecord class that return eloquent related model? In my case I've got following config

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

The problem is that Util class is from Nova too and it looks that is not posible to override it easily. They read model directly from config file

  /**
     * Get the user model for Laravel Nova.
     *
     * @return class-string<\Illuminate\Database\Eloquent\Model>
     */
    public static function userModel()
    {
        $guard = config('nova.guard') ?: config('auth.defaults.guard');
        $provider = config("auth.guards.{$guard}.provider");
        return config("auth.providers.{$provider}.model");
    }

[stevebauman]

Hmm, it looks like they've hard-coded the config path for the user model... You may have to ask the Laravel Nova team this question, as I do not have a license to be able to source dive this with you to see if there is an alternate way to override this method.

[stevebauman]

I've made a tweet to one of the creators of Laravel Nova, hopefully we can get something merged to resolve this:

https://twitter.com/SteveTheBauman/status/1517492302935216129

[crynobone]

1. Run php artisan vendor:publish --tag=nova-migrations
2. Add Laravel\Nova\Nova::ignoreMigrations() to App\Providers\NovaServiceProvider
3. Edit generated CreateActionEventsTable migration.
4. Run php artisan migrate

[stevebauman]

Hi @crynobone, thanks for the reply! However, isn't the Nova::userModel() method used in other places other than the just the migration? Won't users still receive this issue in other areas of Nova?

[crynobone]

@stevebauman I'm also hesitant to introduce such changes fearing having to deal with regression issues or making Nova harder to maintain in multitenancy, Octane, or other environments.

[stevebauman]

I totally understand -- though not all auth providers are created equally and have identical config pathing to their Eloquent model.

If Nova will not support other authentication providers, then a notice should at least be placed to indicate such so users are aware.

No fault to anyone, but I want to ensure that awareness is there so bug reports are not filed on either of our ends 👍

[lightbreaker]

I don't use Nova, but the same problem - and the same cause - occur with spatie permissions package and an internal company package. Although I could probably make the adjustments to the internal package, I see the cause on the side of LdapRecord and accordingly as a bug of LdapRecord .

In my eyes the main problem lies in the inconsistency between the configured model and the provided auth/user model.

Part one of why I see this as a problem is the expectations I have when I look at the configuration. Here for a hypothetical user provider, the comments are my expectations.

'users' => [
        'driver' => 'generic',                                // this is the key for the provider
        'model' =>GenericProvider\User::class,   // this is the model I get from the provider
        '...' => [],                                                // everything else is configuration for the provider
    ],

Here is an example config for this package:

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

For the plain_authproviderthe expectation is met. TheAuth::user()is an instance ofLdapRecord\Models\ActiveDirectory\User::class`.

But for the database_auth provider the expectation it not met. Auth::user() magically is an instance of App\User::class. Without reading the documentation, nobody would expect that - or atleast I wouldn't.

Part two only exists because I can't rely on the expectations of part one. I now have to define for our package somewhere for each user provider which model is provided. And this only because I want to support LdapRecord.

In my eyes a better solution would be if the configuration of LdapRecord would look something like this - so model and database.model are swapped.

'alternate_config' => [
        'driver' => 'ldap',
        'model' => App\User::class,          // this is now the provided model
        'rules' => [],
        'database' => [
            'model' => LdapRecord\Models\ActiveDirectory\User::class,      // this is now the ldap model
            'sync_passwords' => false,
            'sync_attributes' => [
                'name' => 'cn',
                'email' => 'mail',
            ],
        ],
    ],

I know this would be a breaking change, but otherwise it would be likely a breaking in Nova, spatie permissions and our internal company package. Maybe it can be considered for v3.

[stevebauman]

Hi @lightbreaker,

In my eyes the main problem lies in the inconsistency between the configured model and the provided auth/user model.

Package developers need to consider variations of auth configuration. Not all auth providers are identical. Otherwise they really should state they are only compatible with Eloquent. All package providers need to do is offer a way to override either:

• The model instance they retrieve; or
• The configuration path they retrieve the model at

This issue exists for any auth provider that does not use identical configuration to Eloquent.

But for the database_auth provider the expectation it not met. Auth::user() magically is an instance of App\User::class. Without reading the documentation, nobody would expect that - or atleast I wouldn't.

If you've configured a database provider, the database.model will be returned. If you have configured a plain provider, the model will be returned.

I would argue -- of course you have to read the documentation to understand this. I wouldn't expect anyone to jump into Laravel for the first time and understand the auth.php configuration.

In my eyes a better solution would be if the configuration of LdapRecord would look something like this - so model and database.model are swapped.

With your suggested configuration, how would we configure a plain LDAP auth provider? Would we still supply a database array? If so, that would certainly confuse me, as we're not using a database at all at that point.

[stevebauman]

@lightbreaker For Spatie Permission, this could easily be resolved by allowing developers to control how their models are resolved via a resolver callback -- without any breaking changes. Here's an example.

Current:

// laravel-permission/helpers.php

function getModelForGuard(string $guard)
{
    return collect(config('auth.guards'))
        ->map(function ($guard) {
            if (! isset($guard['provider'])) {
                return;
            }

            return config("auth.providers.{$guard['provider']}.model"); // <-- Hard-coded model path.
        })->get($guard);
}

New:

function getModelForGuard(string $guard)
{
    return ModelResolver::resolve($guard);
}

class ModelResolver
{
    protected static $resolver;

    public static function resolve(string $guard)
    {
        return collect(config('auth.guards'))
                ->map(function ($guard) {
                    if (! isset($guard['provider'])) {
                        return;
                    }

                    return value(static::$resolver ?? config("auth.providers.{$guard['provider']}.model"), $guard);
                })->get($guard);
    }

    public static function resolveUsing(Closure $callback)
    {
        static::$resolver = $callback;
    }
}

// In your application...

ModelResolver::resolveUsing(function ($guard) {
    return config("auth.providers.{$guard['provider']}.database.model");
});

[lightbreaker]

In my eyes the main problem lies in the inconsistency between the configured model and the provided auth/user model.

Package developers need to consider variations of auth configuration. Not all auth providers are identical. Otherwise they really should state they are only compatible with Eloquent. All package providers need to do is offer a way to override either:

* The model instance they retrieve; or

* The configuration path they retrieve the model at

This issue exists for any auth provider that does not use identical configuration to Eloquent.

This issue exists for any package in an ecosystem that not comply with conventions. An in that case the package should be responsible to get compatible with the other packages. As of the default eloquent config, Nova as a first party system/package and spatie permissions as the to go package for permissions are all using the same convention, I see it as the convention for the whole Laravel ecosystem, like the generic provider I defined in my post.

But for the database_auth provider the expectation it not met. Auth::user() magically is an instance of App\User::class. Without reading the documentation, nobody would expect that - or atleast I wouldn't.

If you've configured a database provider, the database.model will be returned. If you have configured a plain provider, the model will be returned.

Why is that this way? Why isn't in both cases model returned?

I would argue -- of course you have to read the documentation to understand this. I wouldn't expect anyone to jump into Laravel for the first time and understand the auth.php configuration.

Sure I read the documentation as I implement the package. But thats not granted for a dev looking at it in a couple of month and may not even currently working at the project.

In my eyes a better solution would be if the configuration of LdapRecord would look something like this - so model and database.model are swapped.

With your suggested configuration, how would we configure a plain LDAP auth provider? Would we still supply a database array? If so, that would certainly confuse me, as we're not using a database at all at that point.

The config for plain auth would remain untouched.

ModelResolver::resolveUsing(function ($guard) {
    return config("auth.providers.{$guard['provider']}.database.model");
});

For more than one provider and multiple guards this quickly gets messy. And you are loosing the single source of truth

[stevebauman]

Why is that this way? Why isn't in both cases model returned?

Because LdapRecord-Laravel provides two authentication mechanisms. You can either authenticate with a database, or without. If you are authenticating without a database, then we cannot return an Eloquent model instance. You can read more about this in the docs here:

https://ldaprecord.com/docs/laravel/v2/auth

Some developers do not need a database for their application. Plain authentication covers this use-case. For developers who need a database and have to attach data to their users, can use Synchronized Database authentication.

The config for plain auth would remain untouched.

The configuration you've shared replaces the root model option to reference an Eloquent model and swaps the database.model with an LdapRecord model. They are not interchangeable.

For more than one provider and multiple guards this quickly gets messy. And you are loosing the single source of truth

Then determine what driver the guard is using and alter the config path. This is for the developer to implement, not Spatie:

// app/Providers/AppServiceProvider.php

ModelResolver::resolveUsing(function ($guard) {
    $provider = config("auth.providers.{$guard['provider']}");

    return $provider['driver'] === 'ldap'
        ? $provider['database.model']
        : $provider['model'];
});

More flexibility in general is better, not worse. We must trust developers and hand them more freedom, not restrict them in fear that they will implement incorrectly.

[lightbreaker]

The configuration you've shared replaces the root model option to reference an Eloquent model and swaps the database.model with an LdapRecord model. They are not interchangeable.

From an auth perspective they are. They both implement Illuminate\Contracts\Auth\Authenticatable. And Auth::user() is returning a LdapRecord model for plain auth and an Eloquent model for database auth.

// app/Providers/AppServiceProvider.php

ModelResolver::resolveUsing(function ($guard) {
    $provider = config("auth.providers.{$guard['provider']}");

    return $provider['driver'] === 'ldap'
        ? $provider['database.model']
        : $provider['model'];
});

More flexibility in general is better, not worse. We must trust developers and hand them more freedom, not restrict them in fear that they will implement incorrectly.

I don't see any added flexibility with this approach, but added configuration overhead.

---

In summary, I really just wanted to note that there are several valid reasons that the problem of incompatibility of the configuration with, for example, Nova or Spatie permission, should be solved by LdapRecord. There is something like a convention supported by Nova,Spatie permission as well as the default Eloquent user provider. Would this be adapted by LdapRecord, then:

• neither Nova nor Spatie permission nor others who rely on this convention would have to make adjustments.
• developers using LdapRecord would not have to write extra configuration or extra code.
• the object returned by Auth::user() is an instance of the class specified in 'model'

The upcoming major release would be a good time to adjust the configuration.

I personally just like the "convention over configuration" concept

[stevebauman]

From an auth perspective they are. They both implement Illuminate\Contracts\Auth\Authenticatable. And Auth::user() is returning a LdapRecord model for plain auth and an Eloquent model for database auth.

Sure, but that configuration would cause confusion. I.e. "Why is the LdapRecord model stored in the database array, and the database model stored in the ldap driver array?"

I don't see any added flexibility with this approach, but added configuration overhead.

Ok, although this is an extremely common approach used throughout Laravel and it's own first-party ecosystem.

LdapRecord-Laravel is not primarily an Eloquent authentication driver. It is primarily an LDAP authentication driver, with Eloquent compatibility.

The upcoming major release would be a good time to adjust the configuration.

I personally just like the "convention over configuration" concept

Check out the Laravel-Auth0 repository. It also has a unique auth driver configuration. This driver would also be incompatible with Nova -- even though it could be compatible since it implements the Authenticatable interface.

FYI -- Nova used to work with LdapRecord-Laravel. Now it does not due to their update which hard-coded the config path.

[Perzonallica]

Hi @scramatte !

Have you found any solution for this? We just purchased Nova and of course it still doesn't work out of the box.

[scramatte]

Hi @Perzonallica ,

To get it running I've done the following :

1. First create an app/helpers.php. file and copy or just append function bellow:

<?php

if (! function_exists('getModelForGuard')) {
    /**
     * @param string $guard
     *
     * @return string|null
     */
    function getModelForGuard(string $guard)
    {
        return collect(config('auth.guards'))
            ->map(function ($guard) {
                if (! isset($guard['provider'])) {
                    return;
                }

                return config("auth.providers.{$guard['provider']}.database.model");
            })->get($guard);
    }
}

2. After that install the following composer plugin:

https://github.com/funkjedi/composer-include-files

Modify your composer.json to load plugin above. Be careful as normally "extra" section already exists into composer.json you need to edit existing one and append the plugin as bellow:

...
    "extra": {
        "laravel": {
            "dont-discover": []
        },
        "include_files": [
            "app/helpers.php"
        ]
    },
...

Execute composer dump-autoload

Everything should works properly with this patch Hope that helps

Regards

[Perzonallica]

Thanks @scramatte for your time to describing this!

In the meantime I managed to make it work with creating a different auth guard for Nova.

[scramatte]

Can you publish this here to get reference just in case somebody needs it

Thank you

Enviado desde mi iPhone

El 14 dic 2022, a las 17:01, Perzonallica @.***> escribió:



Thanks @scramattehttps://github.com/scramatte for your time to describing this!

In the meantime I managed to make it work with creating a different auth guard for Nova.

— Reply to this email directly, view it on GitHubhttps://github.com/DirectoryTree/LdapRecord-Laravel/issues/423#issuecomment-1351689335, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AABOCXQAW5QLUO5W4KDO2DTWNHVM5ANCNFSM5UCI2KGQ. You are receiving this because you were mentioned.Message ID: @.***>

[Perzonallica]

@scramatte Sure, at the end it turned out to be a pretty easy solution for us. All we had to do is to create another guard in config/auth.php. We named is as admin and the provider is the default users:

    'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'ldap',
        ],
        'admin' => [
            'driver' => 'session',
            'provider' => 'users'
        ],
    ],

Then in the providers part we just configured it:

    'providers' => [
        'ldap' => [
            'driver' => 'ldap',
            'model' => LdapRecord\Models\ActiveDirectory\User::class,
            'rules' => [],
        ],
        'users' => [
            'driver' => 'eloquent',
            'model' => App\Models\User::class,
        ],
    ],

And the last thing is to put this line in to the .env file: NOVA_GUARD=admin

After this Nova should work with the eloquent driver.

[scramatte]

Hi,

I've spoken to fast, I've still have the issue on migration ... But I might discover something:

If you add method. getKeyType that just return null, into ./Query/Model/ActiveDirectoryBuilder.php It looks that migrations works. I don't know if this will cause issue at another level...

...
    public function getKeyType() {
        return null;
    }
...