DirectoryTree / LdapRecord-Laravel

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

orWhereNotHas / orFilter + whereNotHas unexpected behaviour #663

Closed frankmichel closed 4 weeks ago

frankmichel commented 1 month ago

Environment:

Describe the bug:

In my PHPUnit tests after creating exactly 1 Ldap user I am running the following query on my Ldap user model:

use LdapRecord\Models\ActiveDirectory\User;

$query = User::query();

$query->orFilter(function ($query) {
    $query->where('msExchRecipientTypeDetails', 1);
    $query->whereNotHas('msExchRecipientTypeDetails');
});

dd($query->get()); // returns 3 user objects

This query returns 3 User objects:

If I run the query isolated like this:

$query = User::query();
$query->where('msExchRecipientTypeDetails', 1);

dd($query->get()); // returns 0 user objects

and like this:

$query = User::query();
$query->whereNotHas('msExchRecipientTypeDetails');

dd($query->get()); // returns 1 user object

both results are as expected.

Only if I combine the two (I have tried several ways) the result is wrong and non-users are returned.

It seems like the $objectClasses constraint from the user model is not being applied.

Is this a bug or am I doing something wrong in the query? Thanks for your time looking into this!

stevebauman commented 1 month ago

Hey @frankmichel, thanks for the report.

That's strange, as the LDAP filter appears to be correct? 🤔

use LdapRecord\Models\ActiveDirectory\User;

$query = User::query();

$query->orFilter(function ($query) {
    $query->where('msExchRecipientTypeDetails', 1);
    $query->whereNotHas('msExchRecipientTypeDetails');
});

dd($query->getUnescapedFilter());
(&
    (|(msExchRecipientTypeDetails=1)(!(msExchRecipientTypeDetails=*)))
    (objectclass=top)
    (objectclass=person)
    (objectclass=organizationalperson)
    (objectclass=user)
    (!(objectclass=computer))
)
frankmichel commented 1 month ago

Hi @stevebauman, thanks for the quick reaction.

When running my unit tests with exactly your code sample, I get the following exception:

Call to undefined method LdapRecord\Laravel\Testing\Emulated\ActiveDirectoryBuilder::getUnescapedFilter()

frankmichel commented 1 month ago

@stevebauman I ran it with getUnescapedQuery

Here's my result:

(&
    (|(msExchRecipientTypeDetails=1)(msExchRecipientTypeDetails=*))
    (objectclass=top)
    (objectclass=person)
    (objectclass=organizationalperson)
    (objectclass=user)
    (!(objectclass=computer))
)

Looks like it's missing the ! for the "NotHas" part.

stevebauman commented 1 month ago

That's strange, I'm not able to reproduce that query filter 🤔

You can see the exclamation mark:

Screenshot 2024-05-30 at 10 45 27 AM
frankmichel commented 1 month ago

@stevebauman

I have created a new test with nothing but the code above. However, to make it work like my actual test, I have added this line on top:

DirectoryEmulator::setup('default');

And it seems like adding this on top, is changing the output of the query.

DirectoryEmulator::setup('default');

$query = User::query();

$query->orFilter(function ($query) {
    $query->where('msExchRecipientTypeDetails', 1);
    $query->whereNotHas('msExchRecipientTypeDetails');
});

dd($query->getUnescapedQuery());

Result: (&(|(msExchRecipientTypeDetails=1)(msExchRecipientTypeDetails=*))(objectclass=top)(objectclass=person)(objectclass=organizationalperson)(objectclass=user)(!(objectclass=computer)))

Without the DirectoryEmulator it is correct:

(&(|(msExchRecipientTypeDetails=1)(!(msExchRecipientTypeDetails=*)))(objectclass=top)(objectclass=person)(objectclass=organizationalperson)(objectclass=user)(!(objectclass=computer)))

stevebauman commented 1 month ago

Ah, definitely a bug there! Though to clarify -- is this issue you're experiencing during unit/integration testing with the Directory Emulator, or is it with a live Active Directory connection?

stevebauman commented 1 month ago

Ohh I see, you mentioned this:

In my PHPUnit tests after creating exactly 1 Ldap user I am running the following query on my Ldap user model:

Ok let me patch this! 👍

stevebauman commented 1 month ago

Apologies for the spam @frankmichel -- do you have a test case already that I can use to ensure I've resolved this bug?

frankmichel commented 1 month ago

No worries! I appreciate your quick response and help in this.

Unfortunately I don't have a working test case ready, since I ran into this issue before finishing my tests.

stevebauman commented 1 month ago

What users did you create in your test case to get this result mentioned in your OP?

use LdapRecord\Models\ActiveDirectory\User;

$query = User::query();

$query->orFilter(function ($query) {
    $query->where('msExchRecipientTypeDetails', 1);
    $query->whereNotHas('msExchRecipientTypeDetails');
});

dd($query->get()); // returns 3 user objects

Here's my test I created in this repo, which passes:

DirectoryEmulator::setup('default');

$userWithMatchingAttribute = User::create([
    'cn' => 'John',
    'msExchRecipientTypeDetails' => [1],
]);

$userWithoutAttribute = User::create([
    'cn' => 'Jane',
]);

$userWithoutMatchingAttribute = User::create([
    'cn' => 'Bob',
    'msExchRecipientTypeDetails' => [2],
]);

$results = User::query()->orFilter(function ($query) {
    $query->where('msExchRecipientTypeDetails', 1);
    $query->whereNotHas('msExchRecipientTypeDetails');
})->get();

$this->assertCount(2, $results);
$this->assertTrue($results->contains($userWithoutAttribute));
$this->assertTrue($results->contains($userWithMatchingAttribute));
$this->assertFalse($results->contains($userWithoutMatchingAttribute));
frankmichel commented 1 month ago

Hi @stevebauman, sorry for the late response. I was busy with another project.

Your test runs green in my local environment, too. However, the getUnescapedQuery is still different from the one you posted on top of this thread if you log it in your test.

I have pinpointed the problem in my test to the fact, that before creating any Ldap Users I am also creating some Organization Units like this:

$customers = OrganizationalUnit::create([
    'OU' => 'Customers',
]);

$customer = (new OrganizationalUnit)->inside($customers);
$customer->fill(['OU' => 'Customer']);
$customer->save();

$users = (new OrganizationalUnit)->inside($customer);
$users->fill(['OU' => 'Users']);
$users->save();

So If I add this between the DirectoryEmulator setup and the creation of the first user, the test will fail. It will see the OU's as users. Maybe I am doing something wrong in the creation of the OU's? Or it's still the filter problem.

PS: This only seems to be an issue with the DirectoryEmulator.

stevebauman commented 4 weeks ago

Hey @frankmichel, apologies for the delay on resolving this issue.

I've just created a patch for this, as well as a test case to ensure this is resolved: https://github.com/DirectoryTree/LdapRecord-Laravel/pull/665/commits/c46ebb342348f4a26a701e854b7dd6d099b24281

Run composer update and you should be all set. Let me know if you run into further issues!

Thanks for our detailed report 🙏

frankmichel commented 3 weeks ago

Thank you, @stevebauman !