Ldaprecord-laravel: [Feature] ImportLdapGroups command

[aanderse]

Describe the feature you'd like:

I feel like this might be common enough of a pattern that implementing a command similar to the ImportLdapUsers would be valuable. It certainly would be valuable to me. I'm currently writing some quick and dirty code to achieve the results I'm looking for (--delete, --delete-missing, and --restore). An official command to make this easier would be welcome.

[stevebauman]

Hi @aanderse!

I agree with this -- though I'm not sure what the best implementation would be.

We could allow the ImportLdapUsers command to be extendable, so developers could extend the build in command, define their model and sync attributes inside, or we could have these options available via command options, such as php artisan ldap:group-import --model="App\Group".

I'm leaning towards extending a built in command, as it would offer greater flexibility.

What are your thoughts?

[aanderse]

Right now the ImportLdapUsers command is fairly easily understandable. As an outsider you give it a read, along with the documentation, and you understand it pretty well :+1: My only suggestion would be that if we go the route of extending we take extra effort to keep the code understandable... but you seem to excel at that, so I think we're good :smile:

[stevebauman]

Awesome, I'm glad it's easy to understand and work with, that really is a big priority to me. Thanks a lot for your kind words! I really appreciate it 😄

I'm going to start working on this now. I'll keep this issue open, create a PR once complete and tag this issue. As always I'd appreciate any and all feedback you have on it!

[stevebauman]

Hi @aanderse -- I've been working out an API, and I wanted to get your thoughts, opinions (and most importantly) criticism on it.

What I'm thinking of is a command that will generate an Import class, which you can then define configuration and a model inside,

For example, to generate a new LDAP import, you would run:

php artisan make:ldap-import ImportGroups

Which will generate a class that extends LdapRecord\Laravel\Import:

namespace App\Ldap\Imports;

use LdapRecord\Laravel\Import;
use LdapRecord\Models\ActiveDirectory\Group;

class ImportGroups extends Import
{
    protected $model = Group::class;

    public function configuration()
    {
        return [
             'sync_attributes' => ['...'],
        ];
    }
}

Then in your application, you could execute the import by running:

use App\Group;
use App\Ldap\Imports\ImportGroups;

$importedGroups = ImportGroups::run(Group::class);

What are your thoughts on this API? Do you think anything should be added / changed? Thanks for your time!

[aanderse]

@stevebauman looking good. Again, I'll mention how you write very extensible classes which make customization very easy. I think what you have proposed will work well for a general purpose ldap importer. Thanks for your work on this. If you want me to test drive any code before you push to master just let me know.

:tada:

[stevebauman]

Okay we're all set! If you update your application to use "directorytree/ldaprecord-laravel": "dev-master" in your composer.json file, you can give it a shot. Here's how it works:

1. First, run the below artisan command:

php artisan make:ldap-import ImportGroups --model="LdapRecord\Models\ActiveDirectory\Group"

1. It will generate an import class in your app/Ldap/Imports directory:

<?php

namespace App\Ldap\Imports;

use LdapRecord\Laravel\Import as BaseImport;
use LdapRecord\Models\ActiveDirectory\Group;

class ImportGroups extends BaseImport
{
    /**
     * The LdapRecord model to use for the import.
     *
     * @var string
     */
    protected $model = Group::class;

    /**
     * The configuration for the import.
     *
     * @return array
     */
    protected function config()
    {
        return [
            'sync_attributes' => [
                // ...
            ],
        ];
    }
}

1. Execute the import:

use App\Group;
use App\Ldap\Imports\ImportGroups;

$importedGroups = ImportGroups::into(Group::class);

If you prefer, you can actually remove the config() method in the import, and synchronize attributes your own way, using the syncUsing method:

use App\Group;
use App\Ldap\Imports\ImportGroups;

$import = new ImportGroups(Group::class);

$import->syncUsing(function ($ldap, $database)) {
    $database->name = $ldap->getFirstAttribute('cn');
});

$importedGroups = $import->execute();

You can also apply LDAP query constraints for the import by overriding the applyConstraints() method:

<?php

namespace App\Ldap\Imports;

use LdapRecord\Laravel\Import as BaseImport;
use LdapRecord\Query\Model\Builder;
use LdapRecord\Models\ActiveDirectory\Group;

class ImportGroups extends BaseImport
{
    protected $model = Group::class;

    protected function config()
    {
        return [
            'sync_attributes' => [
                // ...
            ],
        ];
    }

    protect function applyConstraints(Builder $query)
    {
        $query
                ->select('cn')
                ->whereMemberOf('cn=Office Groups,ou=groups,dc=local,dc=com');
    }
}

Give it a shot and let me know your thoughts!

[aanderse]

This looks great. I'm wondering how best to implement the functionality provided by the --delete, --delete-missing, and --restore flags. It looks like I could mostly copy/paste the logic from the existing ImportLdapUsers command, but I just wanted to make sure I'm heading down the right path here first.

Thanks again @stevebauman! Your efforts are much appreciated!

[stevebauman]

Shoot you're right! Those features are a must and need to be implemented as well. Thanks for the reminder, let me update this and I'll get back to you 😄

Happy to help @aanderse!

[stevebauman]

Hey @aanderse!

I've completely rebuilt my original concept, I wasn't happy with the API. I think you'll like this much more 👍

Here's a basic import:

use App\Group;
use LdapRecord\Laravel\Import;
use LdapRecord\Models\ActiveDirectory\Group as LdapGroup;

$imported = (new Import(LdapGroup::class))
    ->into(Group::class)
    ->syncAttributes(['name' => 'cn'])
    ->execute();

You can now also add raw filters, limit the attributes returned in the LDAP query, disable logging, and trash missing objects:

$imported = (new Import(LdapGroup::class))
    ->into(Group::class)
    ->syncAttributes(['name' => 'cn'])
    ->applyFilter('(cn=John Doe)')
    ->limitAttributes(['cn'])
    ->trashMissing()
    ->disableLogging()
    ->execute();

And you can also use your own closure for syncing attributes in your own way:

$imported = (new Import(LdapGroup::class))
    ->into(Group::class)
    ->using(function ($database, $ldap) {
        $database->name = $ldap->getFirstAttribute('cn');

        $database->save();
    })
    ->applyFilter('(cn=John Doe)')
    ->limitAttributes(['cn'])
    ->trashMissing()
    ->disableLogging()
    ->execute();

I have also added event callbacks, where you can execute a closure during each specific event:

$import = new Import(LdapGroup::class);

$import->registerEventCallback('imported', function ($eloquent, $ldap) {
    // Operations to run for each object successfully imported.
});

$import->registerEventCallback('failed', function ($eloquent, $ldap) {
    // Operations to run for each object that failed to import.
});

$imported = $import->into(Group::class)
    ->syncAttributes(['name' => 'cn'])
    ->execute();

Full list of events:

• importing
• imported
• deleting.missing
• deleted.missing
• starting
• failed
• completed

I would love some feedback if you have some time you'd like to spend on checking it out! 😄

[aanderse]

Thanks for this! Had some shifting priorities so hoping to get to this before the weekend... Will let you know.

[aanderse]

Sorry @stevebauman ... this still isn't back on my priority list yet, unfortunately. I believe in the next few weeks this will jump back to the top of the queue, though. I'll try to get to it as soon as I can.

[stevebauman]

No problem @aanderse! Thanks for your reply. Take your time, there's no rush here. 👍

I'm thinking of putting this into v2 so I could use the importer in other places and not worry about breaking BC...

[aanderse]

@stevebauman I'm trying to pick this up again... :smile: Which branch should I be looking at?

[stevebauman]

Hey @aanderse! I've done some significant work on imports over the past couple weeks, so unfortunately the implementations I've shown you above have changed. Sorry!

I'm now comfortable with where it's sitting, so no more changes should be coming to it 👍

The Importer now simply uses Laravel events (instead of it's own pseudo event system), and I've also updated a lot of the method names.

Here's an example:

use App\Group as EloquentGroup;
use LdapRecord\Laravel\Import\Importer;
use LdapRecord\Models\ActiveDirectory\Group as LdapGroup;

$imported = (new Importer)
    ->setLdapModel(LdapGroup::class)
    ->setEloquentModel(EloquentGroup::class)
    ->setSyncAttributes(['name' => 'cn'])
    ->execute();

You can download the most recent update by targeting the dev-master branch in your composer.json:

"directorytree/ldaprecord-laravel": "dev-master"

Let me know if you encounter any issues or have any feedback! 👍

[aanderse]

@stevebauman thanks! I'm playing around with this a bit and it is looking good so far. Can I bother you to add an enableRestores option to match the enableSoftDeletes method?

[stevebauman]

Ah yes great catch @aanderse! Absolutely 👍 I'll add that in tonight

[stevebauman]

Ok added! I'll have tests completed for it tomorrow 👍

[aanderse]

Thank you very much. It looks like the ImportLdapUsers class is crashing when I try to import ActiveDirectory\Group objects though :thinking:

> php artisan import:roles --filter "(|(cn=GROUP_1)(cn=GROUP_2)(cn=GROUP_3))"


   Error 

  Call to a member function createProgressBar() on null

  at vendor/directorytree/ldaprecord-laravel/src/Commands/ImportLdapUsers.php:149
    145▕      */
    146▕     protected function registerEventListeners()
    147▕     {
    148▕         Event::listen(Started::class, function (Started $event) {
  ➜ 149▕             $this->progress = $this->output->createProgressBar($event->objects->count());
    150▕         });
    151▕ 
    152▕         Event::listen(Imported::class, function () {
    153▕             if ($this->progress) {

      +4 vendor frames 
  5   app/Console/Commands/ImportRoles.php:85
      LdapRecord\Laravel\Import\Importer::execute()

      +13 vendor frames 
  19  artisan:37
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

Maybe this line shouldn't be called until the command is going to be executed? I think laravel creates command objects even if you don't use them... That would explain a completely unused command throwing an error. Let me know if you need more details.

[aanderse]

Confirmed... If I move $this->registerEventListeners(); from this line down to public function handle() there is no crash. Though you would only want this to run once... so maybe somewhere else, or a guard against running multiple times would be in order.

[stevebauman]

Thanks again @aanderse! I thought the listeners would be registered properly there, but it forgot that commands are constructed differently in the artisan CLI 😅

Just fixed!

[aanderse]

This is working great. Thank you very much for implementing @stevebauman :tada:

[stevebauman]

Awesome! Great to hear @aanderse! Always happy to help 😄