Auth broken after upgrade to v6 (with DatabaseUserProvider)

[ThomHurks]

• Laravel Version: 5.8.5
• Adldap2-Laravel Version: 6.0.1
• PHP Version: 7.3.3
• LDAP Type: OpenLDAP

Description:

I followed the upgrade guide to upgrade v6. After testing I found out that authentication is now broken. Upon examining the logs I found the following:

• The following query is executed:

select * from `users` where `objectguid` is null or `email` = my_email limit 1

• Then the following query is executed:

update `users` set `username` = foo, `first_name` = Foo, `last_name` = Bar, `email` = my_email, `password` = some_hash, `users`.`updated_at` = 2019-03-20 00:00:00 where `id` = 1

• An error occurs:

Integrity constraint violation: 1062 Duplicate entry 'foo' for key 'users_username_unique' 

The reason is of course that the first query returns the first row in the users table (id=1) because all objectguid fields are null after just upgrading an existing users database. The following part of the query seems wrong:

where `objectguid` is null or

as that part will always cause the wrong user to be returned.

[stevebauman]

Hi @ThomHurks,

The query will return a user with either the given object guid or the given email.

The exception is showing a duplicate username key for your users database table - but the first query is showing that you have email configured as your login username?

[ThomHurks]

@stevebauman Thanks for your reply.
In my application some users (internal) login using OpenLDAP and others (external) using local DB, so the setup we have is that everyone logs in using their email address, and then in the case of OpenLDAP users we use the email address to discover their LDAP account and then bind using the uid, which is their username.

What is happening here is that even though the user binds correctly, the object guid is null. However all objectguid fields in the database are null since we just migrated, so simply the first row gets selected because where `objectguid` is null is true for every row in the table and so it tries to sync the LDAP attributes to that first row, which causes the index to complain that the username already exists. It's trying to update the wrong row.

To me it definitely seems like this behaviour is wrong. What it should do in my opinion is check if the object guid exists and is not null, and then try to query it in the database. In case the object guid does not exist or is null, just query on the username_column field alone or give an error. Querying on an object guid of null causes the wrong row to be updated, which in my case fails because of my unique constraint on username, but others may not have such table constraints and it could corrupt their users table.

Now the second issue is; why is the object guid always null for me? Well, according to the OpenLDAP schema, the object guid is the entryuuid attribute, which I can verify does not exist in our OpenLDAP entries. I'm not sure if this is specific to our OpenLDAP setup or if other companies have that too, but in either case we can't use the entryuuid as a unique ID. I don't think you can rely on uidnumber either, so in that case you should let the user specify what attribute can be considered as the unique ID. In our case I think we can just use the uid, as I don't think we ever change that once an LDAP account has been created. This would require a new configuration option for ADLDAP2-Laravel.

Regarding my third paragraph, I think the code should be something like:

$query->where(
    Resolver::getDatabaseUsernameColumn(),
    '=',
    $this->user->getFirstAttribute(Resolver::getLdapDiscoveryAttribute())
);
$guid = $this->user->getConvertedGuid();
if ($guid !== null) { // Or maybe use !empty($guid)?
    $query->orWhere(
        Resolver::getDatabaseIdColumn(),
         '=',
        $guid
    );
}
$userCount = $query->count();
if ($userCount === 0) {
    return null;
} elseif ($userCount === 1) {
    return $query->first();
} else {
    throw new ModelNotFoundException('Multiple matching users were found in the database.');
}

Also note that I changed the use of only ->first() to check the ->count() first, because if there are multiple matching users in the database, you probably don't want to simply select the first one and use that user, but just throw a hard error because the user is ambiguous. In my case multiple users would have matched and it would have thrown an error instead of selecting the first one because of using ->first().

[stevebauman]

Hi @ThomHurks, I completely understand, thanks for spending the time to thoroughly go through this with me! I really appreciate it.

I'll try to knock out all of your issues / questions here:

What is happening here is that even though the user binds correctly, the object guid is null. However all objectguid fields in the database are null since we just migrated, so simply the first row gets selected because where objectguid is null is true for every row in the table and so it tries to sync the LDAP attributes to that first row, which causes the index to complain that the username already exists. It's trying to update the wrong row.

The query that is supposed to be executed is supposed to contain your LDAP users GUID and not null:

Adldap2-Laravel/src/Commands/Import.php

Lines 83 to 94 in c52c007

	// We'll try to locate the user by their object guid,
	// otherwise we'll locate them by their username.
	return $query->where(
	Resolver::getDatabaseIdColumn(),
	'=',
	$this->user->getConvertedGuid()
	)->orWhere(
	Resolver::getDatabaseUsernameColumn(),
	'=',
	$this->user->getFirstAttribute(Resolver::getLdapDiscoveryAttribute())
	)->first();
	}

Otherwise you're right, the query is wrong if it returns null. There needs to be an exception put in place to prevent the query being ran if a GUID can't be retrieved from the users LDAP model.

I don't think you can rely on uidnumber either, so in that case you should let the user specify what attribute can be considered as the unique ID. In our case I think we can just use the uid, as I don't think we ever change that once an LDAP account has been created.

In the code pasted above from the Import job, the $this>user->getConvertedGuid() is supposed to return your OpenLDAP users entryuuid - though I understand your OpenLDAP instance doesn't have this attribute (which is strange since I thought this was standard for OpenLDAP?). You can actually customize what attribute on an LDAP model contains your users GUID via the schema configuration option in your connection via the config/ldap.php file:

// app/Schemas/OpenLDAP.php

namespace App\Schemas;

use Adldap\Schemas\OpenLDAP as BaseSchema;

class OpenLDAP extends BaseSchema
{
    /**
     * {@inheritdoc}
     */
    public function objectGuid()
    {
        return 'uid'; // The LDAP attribute name that contains your users object GUID
    }
}

// config/ldap.php

'schema' => \App\Schemas\OpenLDAP::class,

Once you put the above in place, the getConvertedGuid() method will return your users uid.

This is definitely where the issue lies - and once you put the above in place you won't have issues with the query generated that locates the user in your database. In the meantime I'm going to patch the Import job to throw an exception when an object GUID can't be located to prevent this from occurring.

Are you able to try the above and see if it resolves your issue?

[ThomHurks]

@stevebauman Thanks! I override the OpenLDAP schema now as you suggest, and now I can log in fine. Great!

What do you think about my suggestion to also throw when more than one matching user is located in the database? That case shouldn't happen if the user has unique constraints on both the guid_column and the username_column and if the query doesn't try to find null values, but I don't think you can always assume this to be the case. Might be better to just check and throw to prevent possible weird issues for some users.

[stevebauman]

Awesome @ThomHurks! Glad you were able to resolve the issue.

What do you think about my suggestion to also throw when more than one matching user is located in the database?

I think you're right, there should be some protection in place to prevent this from happening just in case. I'll close out this issue once that protection is in place.

Thanks again 😄

[ThomHurks]

Very nice, thanks @stevebauman!

[stevebauman]

Ok I added exceptions when the users Object GUID or username cannot be retrieved (9fe1443) - this covers the scenario if one of them returns null so the wrong user model isn't returned. I also added the ability to customize the import scope if needed so you can ensure the proper users are imported / synchronized (d41c4a4, 0042dfa).

New release will be out shortly!

[ThomHurks]

Thanks @stevebauman!