Skip to content

Commit

Permalink
feat: Add user_dn_hash field to improve User::getFromDBbyDn performan…
Browse files Browse the repository at this point in the history 
…ce (glpi-project#16096)

* feat(user): optimize User::getFromDBbyDn() by adding user_dn_hash

fix(auth-ldap): unset user_dn_hash for sql insert

feat(composer.json): add ext-hash to required PHP extensions

Revert "feat(composer.json): add ext-hash to required PHP extensions"

This reverts commit 370b4f4.

chore: Update ComposerRequireChecker config to whitelist hash function

Revert "chore: Update ComposerRequireChecker config to whitelist hash function"

This reverts commit 8e3cdc9.

fix: use md5 algo and drop virtual key usage

* Apply suggestions from code review

Co-authored-by: Cédric Anne <[email protected]>

* refactor(user): Update user_dn_hash directly in pre_addInDB/pre_updateInDB

* test(user): Ensure user_dn_hash is emptied when user_dn is set to empty or null

---------

Co-authored-by: Cédric Anne <[email protected]>
  • Loading branch information
@ccailly @cedric-anne @anthonymontebrun
2 people authored and anthonymontebrun committed Jul 11, 2024
1 parent 9aad0a1 commit 0a25d81
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 6 deletions.
59 changes: 59 additions & 0 deletions install/migrations/update_10.0.11_to_10.0.12/user.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

/**
* ---------------------------------------------------------------------
*
* GLPI - Gestionnaire Libre de Parc Informatique
*
* http://glpi-project.org
*
* @copyright 2015-2023 Teclib' and contributors.
* @copyright 2003-2014 by the INDEPNET Development Team.
* @licence https://www.gnu.org/licenses/gpl-3.0.html
*
* ---------------------------------------------------------------------
*
* LICENSE
*
* This file is part of GLPI.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*
* ---------------------------------------------------------------------
*/

/**
* @var \DBmysql $DB
* @var \Migration $migration
*/

// Add user_dn_hash field
$migration->addField('glpi_users', 'user_dn_hash', 'varchar(32)', [
'after' => 'user_dn',
]);

$migration->addPostQuery($DB->buildUpdate(
'glpi_users',
[
'user_dn_hash' => new \QueryExpression('MD5(`user_dn`)'),
],
[
'NOT' => [
'user_dn' => null
]
]
));

// Add user_dn_hash index
$migration->addKey('glpi_users', 'user_dn_hash');
4 changes: 3 additions & 1 deletion install/mysql/glpi-empty.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7610,6 +7610,7 @@ CREATE TABLE `glpi_users` (
`password_forget_token` char(40) DEFAULT NULL,
`password_forget_token_date` timestamp NULL DEFAULT NULL,
`user_dn` text,
`user_dn_hash` varchar(32),
`registration_number` varchar(255) DEFAULT NULL,
`show_count_on_tabs` tinyint DEFAULT NULL,
`refresh_views` int DEFAULT NULL,
Expand Down Expand Up @@ -7686,7 +7687,8 @@ CREATE TABLE `glpi_users` (
KEY `groups_id` (`groups_id`),
KEY `users_id_supervisor` (`users_id_supervisor`),
KEY `auths_id` (`auths_id`),
KEY `default_requesttypes_id` (`default_requesttypes_id`)
KEY `default_requesttypes_id` (`default_requesttypes_id`),
KEY `user_dn_hash` (`user_dn_hash`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci ROW_FORMAT=DYNAMIC;


Expand Down
27 changes: 22 additions & 5 deletions src/User.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -550,15 +550,20 @@ public function getFromDBbySyncField($value)
/**
* Retrieve a user from the database using it's dn.
*
* @since 0.84
*
* @param string $user_dn dn of the user
*
* @return boolean
*/
public function getFromDBbyDn($user_dn)
{
return $this->getFromDBByCrit(['user_dn' => $user_dn]);
/**
* We use the 'user_dn_hash' field instead of 'user_dn' for performance reasons.
* The 'user_dn_hash' field is a hashed version of the 'user_dn' field
* and is indexed in the database, making it faster to search.
*/
return $this->getFromDBByCrit([
'user_dn_hash' => md5($user_dn)
]);
}

/**
Expand Down Expand Up @@ -833,6 +838,14 @@ public function computeCloneName(
);
}

public function pre_addInDB()
{
// Hash user_dn if set
if (isset($this->input['user_dn']) && is_string($this->input['user_dn']) && strlen($this->input['user_dn']) > 0) {
$this->input['user_dn_hash'] = md5($this->input['user_dn']);
}
}

public function post_addItem()
{

Expand Down Expand Up @@ -1187,8 +1200,6 @@ public function post_updateItem($history = true)
}
}



/**
* Apply rules to determine dynamic rights of the user.
*
Expand Down Expand Up @@ -3483,6 +3494,12 @@ public function pre_updateInDB()
unset($this->oldvalues['comment']);
}
}

// Hash user_dn if is updated
if (in_array('user_dn', $this->updates)) {
$this->updates[] = 'user_dn_hash';
$this->fields['user_dn_hash'] = is_string($this->input['user_dn']) && strlen($this->input['user_dn']) > 0 ? md5($this->input['user_dn']) : null;
}
}

public function getSpecificMassiveActions($checkitem = null)
Expand Down
1 change: 1 addition & 0 deletions tests/LDAP/AuthLdap.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,7 @@ public function testLdapAuth()
unset($dup['id']);
unset($dup['date_creation']);
unset($dup['date_mod']);
unset($dup['user_dn_hash']);
$aid = $dup['auths_id'];
$dup['auths_id'] = $aid + 1;

Expand Down
85 changes: 85 additions & 0 deletions tests/functional/User.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1293,4 +1293,89 @@ public function testUserPreferences()
$this->variable($user->fields['show_count_on_tabs'])->isNull();
$this->variable($user->fields['itil_layout'])->isEqualTo($itil_layout_2);
}

/**
* Test that user_dn_hash is correctly set on user creation and update
*
* @return void
*/
public function testUserDnIsHashedOnAddAndUpdate(): void
{
// Create user whithout dn and check that user_dn_hash is not set
$user = $this->createItem('User', [
'name' => __FUNCTION__,
]);
$this->variable($user->fields['user_dn'])->isNull();
$this->variable($user->fields['user_dn_hash'])->isNull();

// Create user with dn and check that user_dn_hash is set
$dn = 'user=' . __FUNCTION__ . '_created,dc=test,dc=glpi-project,dc=org';
$user = $this->createItem('User', [
'name' => __FUNCTION__ . '_created',
'user_dn' => $dn
]);
$this->string($user->fields['user_dn_hash'])->isEqualTo(md5($dn));

// Update user dn and check that user_dn_hash is updated
$dn = 'user=' . __FUNCTION__ . '_updated,dc=test,dc=glpi-project,dc=org';
$this->updateItem('User', $user->getID(), [
'user_dn' => $dn
]);
$user->getFromDB($user->getID());
$this->string($user->fields['user_dn_hash'])->isEqualTo(md5($dn));

// Set user_dn to empty and check that user_dn_hash is set to null
$this->updateItem('User', $user->getID(), [
'user_dn' => ''
]);
$user->getFromDB($user->getID());
$this->variable($user->fields['user_dn_hash'])->isNull();

// Set user_dn to null and check that user_dn_hash is set to null
$this->updateItem('User', $user->getID(), [
'user_dn' => null
]);
$user->getFromDB($user->getID());
$this->variable($user->fields['user_dn_hash'])->isNull();
}

/**
* Test that user_dn_hash is correctly used in getFromDBbyDn method
*
* @return void
*/
public function testUserDnHashIsUsedInGetFromDBbyDn(): void
{
global $DB;

$retrievedUser = new \User();

// Get a user with a bad dn
$this->boolean($retrievedUser->getFromDBbyDn(__FUNCTION__))
->isFalse();
$this->boolean($retrievedUser->isNewItem())->isTrue();

// Create a user with a dn
$dn = 'user=' . __FUNCTION__ . ',dc=test,dc=glpi-project,dc=org';
$user = $this->createItem('User', [
'name' => __FUNCTION__,
'user_dn' => $dn
]);

// Retrieve the user using getFromDBbyDn method
$this->boolean($retrievedUser->getFromDBbyDn($dn))->isTrue();
$this->boolean($retrievedUser->isNewItem())->isFalse();

// Unset user_dn to check that user_dn_hash is used
$DB->update(
\User::getTable(),
['user_dn' => ''],
['id' => $user->getID()]
);

// Retrieve the user using getFromDBbyDn and check if user_dn_hash is used
$this->boolean($retrievedUser->getFromDBbyDn($dn))->isTrue();
$this->boolean($retrievedUser->isNewItem())->isFalse();
$this->string($retrievedUser->fields['user_dn'])->isEmpty();
}
}

0 comments on commit 0a25d81

Please sign in to comment.