DirectoryTree / LdapRecord-Laravel

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

[Bug] Detaching a member from a group does not work with Directory Emulator #552

Closed DijkeMark closed 10 months ago

DijkeMark commented 1 year ago

Environment:

Describe the bug: I am working on an application where read/write data to an ActiveDirectory (actually there are multiple off them). To be able to locally develop, I write the data to a .sqlite file.

The application is supposed to attach and detach users from groups, but this only partially works. I am able to attach an user to a group, but detaching the same user from that specific group does not work. The user stays linked to the group. This is something I can confirm in the .sqlite file as the 'member' record doesn't get removed.

See the following example: I first add the data to attach.

// Create new user
$user = new User([
    'dn' => 'cn=012345jd@test.nl,dc=local,dc=com',
    'cn' => '012345jd',
    'sn' => 'John Doe',
    'userprincipalname' => '01234jod@test.nl',
    'displayname' => 'John',
]);
$user->save();

// create new group
$group = new Group([
    'dn' => 'cn=oc-1337,dc=local,dc=com',
    'cn' => 'oc-1337',
    'displayname' => 'Test Group'
]);
$group->save();

This works as expected. I see both records in the .sqlite.

Then I attach the group to the user:

// Retrieve both user and group and attach them
$user2 = User::find('cn=012345jd@test.nl,dc=local,dc=com');
$group2 = Group::find('cn=oc-1337,dc=local,dc=com');

dump($user2->groups()->get()->toArray()); // This will show an empty collection
$user2->groups()->attach($group2);
dump($user2->groups()->get()->toArray()); // This will now show 1 group is attached

So far so good. I now have a 'member' record in the .sqlite file.

Now we try to remove the user from the group:

// Detach the user from the group. We try it from both directions, but both don't work
$user2->groups()->detach($group2);
$group2->members()->detach($user2);

$user3 = User::find('cn=012345jd@test.nl,dc=local,dc=com');
dump($user3->groups()->get()->toArray()); // This will still show 1 group is attached, record is still in the .sqlite file

Other: This is done with the default LdapRecord\Models\ActiveDirectory\Group and LdapRecord\Models\ActiveDirectory\User classes. This problem happens to me, even in a fresh/clean laravel project, so it shouldn't be impacted by anything else.

The original project still uses the v2 of this package, but in my example project, I used both version 2. and 3. and both had the same problem.

The Directory Emulator is used as shown in the documentation.

stevebauman commented 1 year ago

Hi @DijkeMark,

Is this issue regarding the Directory Emulator feature? You mention storage in SQLite, so I'm not sure if this is your own implementation or not.

DijkeMark commented 1 year ago

My apologies, I should have specified that. I am indeed using the Directory Emulator. There is no custom implementation.

stevebauman commented 1 year ago

No worries @DijkeMark!

This is a bug -- I can replicate this in the test environment. If you're able to find a resolution then a PR is always welcome, otherwise I'll be taking a look at this tomorrow 👍

DijkeMark commented 1 year ago

I was wondering if you had any luck finding the problem? I had a look myself, but I couldn't find the problem myself (for now).

stevebauman commented 1 year ago

Hi @DijkeMark,

Yes I've found the issue, but haven't been able to found a solution yet.

The issue is that the underlying relationship attach/detach methods call the model methods addAttribute()/removeAttribute() and not query builder methods. I.e.:

$model->addAttribute('...');
$model->removeAttribute('...');

Currently, only query builder methods are emulated with the DirectoryEmulator.

We basically need to figure a way to either override or listen for these methods (or the relationship attach/detach methods) to then remove the attributes from the underlying Eloquent database model that stores the LDAP object's attributes in the DirectoryEmulator.

DijkeMark commented 1 year ago

Hm, okay. That's interesting.

Does the 'attach' work differently then the 'detach'? As the attach functionality is working, but only the detach is not. So based on this and your explanation I would think, that perhaps something within the detach part is not wired the same as the attach.

Or am I misunderstanding you?

Thanks for replying and trying to fix it. :)

DesideriusE commented 1 year ago

I think the problem is on this line: https://github.com/DirectoryTree/LdapRecord-Laravel/blob/master/src/Testing/Emulated/EmulatesModelQueries.php#L92

It seems to work if I change } elseif (Arr::exists($model->{$attribute} ?? [], $attribute)) { in to } elseif (Arr::exists($model->attributesToArray() ?? [], $attribute)) {.

DijkeMark commented 1 year ago

I have create the pull request https://github.com/DirectoryTree/LdapRecord-Laravel/pull/556 with the proposed fix from @DesideriusE

DijkeMark commented 1 year ago

@stevebauman I was wondering was remains to be done for the PR to be merged?

I see that the scrutinizer check fails on

No releases available for package "pecl.php.net/ldap"
install failed

I suspect only you can fix this? Is this something that is needed for this PR?

stevebauman commented 10 months ago

Sorry for the late reply on this @DijkeMark, this has been resolved in the latest release. Group memberships are now emulated properly. Thanks for your patience!

https://github.com/DirectoryTree/LdapRecord-Laravel/releases/v3.0.8

Please run composer update and you're all set 👍

DijkeMark commented 10 months ago

@stevebauman Thanks for replying with the fix. Unfortunately I do not get the fix to work in my own project.

When I look at the fix from @cheesegrits (https://github.com/DirectoryTree/LdapRecord-Laravel/pull/560/files#diff-0e5035306d66fb5baf7dc9604180204ff56462ec5059062afe22ac89827dd963), I see that a manual $user->removeAttribute('memberof', $group->getDn()); is done.

But isn't your PR (https://github.com/DirectoryTree/LdapRecord-Laravel/pull/584) supposed to fix this, so we dont have to manually remove the virtual attributes?

Because, when I remove this line from the test, it fails. Even with your emulation fix. Or did I misunderstand both fixes? Because I expected the detach to work now, without having to manually remove virtual attributes.

stevebauman commented 10 months ago

Hi @DijkeMark,

For the memberof attribute to update after detaching a user from a group, you have to $user->refresh() the model. This is true when using a real LDAP server as well, as the virtual attribute needs to be reloaded from the server.

Let me know if you still encounter this issue after including the refresh of the model 👍

DijkeMark commented 10 months ago

I tried your suggestion, but unfortunately it fails at the refresh, it seems because I am using a different connection. Since it seems like a separate issue, I created a new issue for this (https://github.com/DirectoryTree/LdapRecord-Laravel/issues/590).