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 552: Fixed issue with detaching groups from users #556

Closed DijkeMark closed 10 months ago

DijkeMark commented 1 year ago

closes #552

Bug-552:

Sorry, just noticed I put the wrong issue number in the commit message!

stevebauman commented 1 year ago

Thanks for the PR @DijkeMark!

Unfortunately this only resolves half of the issue. I've updated the PR with an additional assertion that still fails after detaching a user from a group. The underlying LdapObject Eloquent model for the LDAP user still contains a reference of the group inside of it's memberof attribute, so when querying for $group->members(), the user is still returned:

$user->groups()->detach($group);

$this->assertFalse($user->groups()->exists($group)); // Works ✅
$this->assertFalse($group->members()->exists($user)); // Doesn't work ❌

We should try to resolve this as well, but I'm a bit stumped at the moment on how we can do this easily in the emulator. When a user is detached from a group, we need to remove their DN's from their underlying LdapObject Eloquent model's attributes (memberof and member).

cheesegrits commented 1 year ago

@stevebauman if you check the PR I submitted before realizing there already was one ...

https://github.com/DirectoryTree/LdapRecord-Laravel/pull/560/commits/46826d62db42cb33d97a67ad23cc8ff56f753c00

... I'm doing this in the test ...

$user->removeAttribute('memberof', $group->getDn());

... to fake the 'memberof' removal. As it was fake added in the first place, can only fake remove it.

But ... WAIT, there's more! ...

I have another PR lined up I could submit in the next day or two which adds handling 'memberof' in the emulator, behind a config feature flag. See the issue I just posted ...

https://github.com/DirectoryTree/LdapRecord-Laravel/issues/561

stevebauman commented 10 months ago

Closed in favour of https://github.com/DirectoryTree/LdapRecord-Laravel/pull/560. They both solve the same issue -- though $model->hasAttribute('...') is less computationally intensive since attributesToArray() will prepare all attributes for JSON serialization.

Thanks again for your PR and time, I really appreciate it! ❤️