feat: Add user_dn_hash field to improve User::getFromDBbyDn performance #16096

ccailly · 2023-11-30T14:27:13Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	!30554

Description:

This PR introduces a new field user_dn_hash in the glpi_users table. This field is a virtual field that is generated by hashing the user_dn field. This change is aimed at improving the performance of queries that involve the user_dn field by using the hashed value instead.

Changes:

Added ext-hash to the required PHP extensions in composer.json and composer.lock to avoid soft dependency issues.
Created a new migration file user.php in install/migrations/update_10.0.10_to_10.0.11/ to add the user_dn_hash field and its index to the glpi_users table.
Updated glpi-empty.sql to include the user_dn_hash field and its index in the glpi_users table creation statement.
Updated the getFromDBbyDn method in User.php to use the user_dn_hash field for the database query.
Updated the test file AuthLdap.php to unset the user_dn_hash field in the dup array.

ccailly · 2023-11-30T14:29:09Z

Please note that there is currently an issue with the database migration tests from version 0.80 using MyISAM and virtual columns under MySQL.
MySQL and MyISAM do not seem to support adding indexes to a virtual column.

trasher · 2023-11-30T14:30:50Z

Are we sure a new PHP extension is really to be required? Anyway, it should not be in bugfixes; and not only in composer.json

ccailly · 2023-11-30T14:41:44Z

Are we sure a new PHP extension is really to be required? Anyway, it should not be in bugfixes; and not only in composer.json

ext-hash is an old extension that was included in php 7.4. However, if I don't add the extension to the composer.json, composer-require-checker tells me that the hash symbol is unknown (although it is a native PHP function). The error disappears by adding the dependency.

On reflection, this doesn't seem to be the right method. You can add the function to the whitelist in the .composer-require-checker.config.json file.

trasher · 2023-11-30T14:46:55Z

We could use md5 instead to avoid this but could it be exploited by malicious users ?

I'm not sure is can really be used for an exploit; we can also use sha1 which is a bit better maybe.

We rely on some hash features, but in main only (hash_hmac)

AdrienClairembault · 2023-11-30T14:47:37Z

Anyway, it should not be in bugfixes;

Regarding the bugfixes/main debate, this change is introduced because the current user_dn column can't be indexed and it block the ldap sync for some users with big databases (as it take too long to complete).

So IMO this is kinda a bugfix and it would be best to stay on the 10.0.11 target (as long as we can drop the new ext requirement).

trasher · 2023-11-30T14:51:38Z

ext-hash is an old extension that was included in php 7.4. However, if I don't add the extension to the composer.json, composer-require-checker tells me that the hash symbol is unknown (although it is a native PHP function). The error disappears by adding the dependency.

If it's part of php core since 7.4; then it's certainly OK to add it - I've not checked (and I do not really know how to)

trasher · 2023-11-30T14:53:27Z

ext-hash is an old extension that was included in php 7.4. However, if I don't add the extension to the composer.json, composer-require-checker tells me that the hash symbol is unknown (although it is a native PHP function). The error disappears by adding the dependency.

If it's part of php core since 7.4; then it's certainly OK to add it - I've not checked (and I do not really know how to)

Anyway, I do not know if we have to add a check GLPI side; composer.json will only be used on development, not from release archive.

AdrienClairembault · 2023-12-04T10:07:49Z

Regarding the issues:

1) Composer hash

It seems @cedric-anne already encountered the issue and fixed in directly in composer require checker : maglnet/ComposerRequireChecker#399

Maybe it was not fixed completely and we should open an issue on their side ?

2) Unsupported index on virtual fields for Mysql + Myisam

Should we fallback to a "real" field or can we make an exception for this ?
It would suck not being able to use a virtual fields because an obsolete engine do not support indexes on them.

trasher · 2023-12-04T10:13:36Z

It would suck not being able to use a virtual fields because an obsolete engine do not support indexes on them.

Switching to InnoDB is not yet mandatory, we cannot change this is a bugfixes version.

AdrienClairembault · 2023-12-04T10:19:26Z

It would suck not being able to use a virtual fields because an obsolete engine do not support indexes on them.

Switching to InnoDB is not yet mandatory, we cannot change this is a bugfixes version.

Yeah I expect that of course, but maybe the error should not be flagged as an issue ? It wont make anything worse for users on myisam while it will improve speed for anyone else.

(Can we still drop myisam for GLPI 10.1 tho ?)

src/User.php

cedric-anne · 2023-12-04T10:42:41Z

It would suck not being able to use a virtual fields because an obsolete engine do not support indexes on them.

Switching to InnoDB is not yet mandatory, we cannot change this is a bugfixes version.

Yeah I expect that of course, but maybe the error should not be flagged as an issue ? It wont make anything worse for users on myisam while it will improve speed for anyone else.

I think it would be easier to use a "concrete" field than fixing the CI to handle this case. But if you find an easy way to activate the index only if user uses InnoDB at the migration time, without changing too many things on the CI env, I guess it would be OK.

(Can we still drop myisam for GLPI 10.1 tho ?)

We should probably, and I would like to do it, but it is not yet done and not sure we would have time to do this.

AdrienClairembault

I guess we'll have to stick to a normal field then (sadly).

@ccailly can you rework the field into a concrete field ?

ccailly · 2023-12-19T09:29:02Z

@trasher I rebased on main as you asked yesterday but I don't see your issue in the PR thread anymore. Should I keep main on target or switch back to bugfixes?

trasher · 2023-12-19T10:18:18Z

keep in main for now, once approved we'll see where it will go with @cedric-anne and @orthagh

src/User.php

trasher · 2023-12-20T08:50:53Z

Please also add unit tests; this should not be too hard.

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

src/User.php

install/migrations/update_10.0.11_to_10.0.12/user.php

install/mysql/glpi-empty.sql

src/User.php

cedric-anne

You should add a test to ensure that user_dn_hash is correctly emptied when user_dn is changed to an empty value.

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

…eInDB

…ty or null

…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]>

ccailly requested a review from AdrienClairembault November 30, 2023 14:27

ccailly self-assigned this Nov 30, 2023

ccailly added the performance label Nov 30, 2023

ccailly added this to the 10.0.11 milestone Nov 30, 2023

cedric-anne reviewed Dec 4, 2023

View reviewed changes

src/User.php Outdated Show resolved Hide resolved

AdrienClairembault modified the milestones: 10.0.11, 10.0.12 Dec 5, 2023

AdrienClairembault requested changes Dec 5, 2023

View reviewed changes

ccailly force-pushed the fix/optimize-user-getFromDBbyDn-with-sha2-index branch 2 times, most recently from e1a2286 to 9e037be Compare December 18, 2023 15:11

cedric-anne requested review from AdrienClairembault and cedric-anne December 18, 2023 15:21

ccailly force-pushed the fix/optimize-user-getFromDBbyDn-with-sha2-index branch from 9e037be to 08268b5 Compare December 19, 2023 09:22

ccailly changed the base branch from 10.0/bugfixes to main December 19, 2023 09:23

orthagh reviewed Dec 20, 2023

View reviewed changes

src/User.php Show resolved Hide resolved

trasher added the Need unit tests label Dec 20, 2023

ccailly force-pushed the fix/optimize-user-getFromDBbyDn-with-sha2-index branch from 08268b5 to c7da5cf Compare December 20, 2023 11:20

ccailly changed the base branch from main to 10.0/bugfixes December 20, 2023 11:21

trasher approved these changes Dec 20, 2023

View reviewed changes

trasher removed the Need unit tests label Dec 20, 2023

trasher requested a review from orthagh December 20, 2023 13:26

orthagh approved these changes Dec 20, 2023

View reviewed changes

trasher force-pushed the fix/optimize-user-getFromDBbyDn-with-sha2-index branch from c7da5cf to 9fc18ac Compare December 21, 2023 08:59

cedric-anne requested changes Dec 21, 2023

View reviewed changes

src/User.php Outdated Show resolved Hide resolved

install/migrations/update_10.0.11_to_10.0.12/user.php Show resolved Hide resolved

install/mysql/glpi-empty.sql Show resolved Hide resolved

src/User.php Outdated Show resolved Hide resolved

cedric-anne reviewed Dec 21, 2023

View reviewed changes

src/User.php Outdated Show resolved Hide resolved

cedric-anne reviewed Dec 21, 2023

View reviewed changes

src/User.php Outdated Show resolved Hide resolved

cedric-anne requested changes Dec 21, 2023

View reviewed changes

ccailly and others added 3 commits December 21, 2023 16:08

Apply suggestions from code review

3618847

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

refactor(user): Update user_dn_hash directly in pre_addInDB/pre_updat…

70983fa

…eInDB

test(user): Ensure user_dn_hash is emptied when user_dn is set to emp…

c55200b

…ty or null

trasher merged commit 2ee311a into glpi-project:10.0/bugfixes Dec 22, 2023
12 checks passed

cedric-anne mentioned this pull request Mar 19, 2024

Fix user_dn_hash reconciliation when user DN contains special chars #16806

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add user_dn_hash field to improve User::getFromDBbyDn performance #16096

feat: Add user_dn_hash field to improve User::getFromDBbyDn performance #16096

ccailly commented Nov 30, 2023 •

edited by cedric-anne

Loading

ccailly commented Nov 30, 2023

trasher commented Nov 30, 2023 •

edited

Loading

ccailly commented Nov 30, 2023

trasher commented Nov 30, 2023

AdrienClairembault commented Nov 30, 2023

trasher commented Nov 30, 2023

trasher commented Nov 30, 2023

AdrienClairembault commented Dec 4, 2023

trasher commented Dec 4, 2023

AdrienClairembault commented Dec 4, 2023

cedric-anne commented Dec 4, 2023

AdrienClairembault left a comment

ccailly commented Dec 19, 2023

trasher commented Dec 19, 2023

trasher commented Dec 20, 2023

cedric-anne left a comment

feat: Add user_dn_hash field to improve User::getFromDBbyDn performance #16096

feat: Add user_dn_hash field to improve User::getFromDBbyDn performance #16096

Conversation

ccailly commented Nov 30, 2023 • edited by cedric-anne Loading

ccailly commented Nov 30, 2023

trasher commented Nov 30, 2023 • edited Loading

ccailly commented Nov 30, 2023

trasher commented Nov 30, 2023

AdrienClairembault commented Nov 30, 2023

trasher commented Nov 30, 2023

trasher commented Nov 30, 2023

AdrienClairembault commented Dec 4, 2023

trasher commented Dec 4, 2023

AdrienClairembault commented Dec 4, 2023

cedric-anne commented Dec 4, 2023

AdrienClairembault left a comment

Choose a reason for hiding this comment

ccailly commented Dec 19, 2023

trasher commented Dec 19, 2023

trasher commented Dec 20, 2023

cedric-anne left a comment

Choose a reason for hiding this comment

ccailly commented Nov 30, 2023 •

edited by cedric-anne

Loading

trasher commented Nov 30, 2023 •

edited

Loading