DirectoryTree / LdapRecord-Laravel

Multi-domain LDAP Authentication & Management for Laravel.
https://ldaprecord.com/docs/laravel/v3
MIT License
492 stars 51 forks source link

[Feature] Sync attributes after authentication | Customize DatabaseUserProvider #530

Closed SamuelWei closed 1 year ago

SamuelWei commented 1 year ago

Describe the feature you'd like:

It would be great if there was a way to further customize the attribute synchronization behavior, in particular their timing. I currently have a custom user synchronizer implemented, however in some cases this is not enough. In restrictive LDAP scenarios, only the authenticated user himself can retrieve his own attributes. Therefore, at the time of the current synchronization, these are not available.

The desired functionality would be to adapt the existing DatabaseUserProvider, provide an alternative one or a simple way to future customize its behavior.

What would be needed is that the function retrieveByCredentials only retrieves the record from the DB if the user with the ID has already been found there, or otherwise just creates a model (not saving it yet) that is still missing almost all attributes except for the ID.

Later in the method validateCredentials the attribute mapping should be done after the authentication. This should be customizable so that possibly the userobject can be reloaded here. (If you customize authenticateUsing according to the documentation, the bind with the user credentials is preserved and the user object is retrieved as the logged in user). This should be done before $user->save();. In case the attribute mapping is failing, it should be possible to abort the login.

Another good extension would be to allow another callback directly after the save, since some relations or attributes may only be retrieved after the creation of an entry stored in the database. In case the attribute mapping is failing, it should be possible to abort the login.

SamuelWei commented 1 year ago

I now build my own DatabaseUserProvider using DirectoryTree/LdapRecord, but this would still be a good feature for this project

stevebauman commented 1 year ago

Hi @SamuelWei, apologies for the late reply.

provide an alternative one or a simple way to future customize its behavior.

All of the included providers can be customized and extended, as they are created using the app() Laravel helper:

https://github.com/DirectoryTree/LdapRecord-Laravel/blob/43e5e0f0837caa5f8503ae0f044966c15e0528ed/src/LdapAuthServiceProvider.php#L77-L85

Inside of your AppServiceProvider, you can bind the existing DatabaseUserProvider class with your own implementation, and override anything you'd like. I don't use private methods or properties anywhere in my repositories. I try to make everything configurable or overridable.

Here's an example:

<?php

namespace App\Providers;

use App\Ldap\MyDatabaseUserProvider;
use LdapRecord\Laravel\Auth\DatabaseUserProvider;

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application dependencies.
     *
     * @return void
     */
    public function register()
    {
        $this->app->bind(DatabaseUserProvider::class, MyDatabaseUserProvider::class);
    }
}

This is however quite a lot of customization you're looking for, and I haven't received any other requests for these features, and I also don't personally have a need for them, so I can't justify putting time into this. If you can submit a PR with these features, I'd be happy to work with you on getting it implemented. I hope that's understandable!

SamuelWei commented 1 year ago

Thank you for your detailed answer. I solved it in the meantime by mostly implementing the features I needed using DirectoryTree/LdapRecord but still having DirectoryTree/LdapRecord-Laravel in my packages due to the great testing features.

If anyone finds the issue in the future, you can have a look at my PR: https://github.com/THM-Health/PILOS/pull/340/files