Skip to content

Support LDAP dns longer than 255 characters - #29523

Merged
skjnldsv merged 10 commits into
masterfrom
fix/support-ldap-long-dns
Dec 9, 2021
Merged

Support LDAP dns longer than 255 characters#29523
skjnldsv merged 10 commits into
masterfrom
fix/support-ldap-long-dns

Conversation

@come-nc

@come-nccome-nc commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

Adds an ldap_full_dn column to store the dn, and only store a sha256 hash in the ldap_dn which is shorter and can be indexed without trouble.
Migration still needs to be implemented.

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

nickvergessen
nickvergessen requested changes Nov 2, 2021
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1010Date20200630192842.php Outdated
@come-nc

come-nc commented Nov 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Note to self: we should look into which kind of hash fits the best what we need here. There are uuid v5, and other hash algorithm, unicity and performance should be compared to select the best fit.

@come-nc

come-nc commented Nov 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Related bug ticket: #2213

@come-nccome-nc self-assigned this Nov 4, 2021
@come-nc
come-nc marked this pull request as ready for review November 8, 2021 09:04
@come-nccome-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 8, 2021
@blizzz

blizzz commented Nov 16, 2021

Copy link
Copy Markdown
Member

/rebase

blizzz
blizzz requested changes Nov 16, 2021
View reviewed changes

@blizzzblizzz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the first Migration class, the final state of the table should be reflected for new installations. Not necessary to run the while migration history on a new install. So you can add the columns and alter the index in Version1010Date20200630192842.

The new Version1130Date20211102154716 is of course required and looks good as is, with the one remark I left there.

Comment threadapps/user_ldap/lib/Mapping/AbstractMapping.php Outdated
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php
@blizzz

blizzz commented Nov 16, 2021

Copy link
Copy Markdown
Member

@nickvergessen what do you think about its backportability?

@blizzz

blizzz commented Nov 16, 2021

Copy link
Copy Markdown
Member

…while at changing DB it might be worth to add a simple index on directory_uuid, too, since this also may be used as sole WHERE column in queries.

nickvergessen
nickvergessen reviewed Nov 18, 2021
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php Outdated
@come-nc

come-nc commented Nov 18, 2021

Copy link
Copy Markdown
ContributorAuthor

In the first Migration class, the final state of the table should be reflected for new installations. Not necessary to run the while migration history on a new install. So you can add the columns and alter the index in Version1010Date20200630192842.

Done in bb6906d

@come-nc

come-nc commented Nov 18, 2021

Copy link
Copy Markdown
ContributorAuthor

…while at changing DB it might be worth to add a simple index on directory_uuid, too, since this also may be used as sole WHERE column in queries.

Done in c1ff024
Used a unique index as this column should be unique as well, tell me if this needs to be changed to something else.

come-ncand others added 10 commits November 23, 2021 09:19
@come-nc
Support LDAP dns longer than 255 characters
662e324
Adds an ldap_full_dn column to store the dn, and only store a sha256
hash in the ldap_dn which is shorter and can be indexed without
trouble.
Migration still needs to be implemented.
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Change column names to ldap_dn and ldap_dn_hash and add migration
31a503b
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@nickvergessen@come-nc
Fix variable names
9a1df9d
Signed-off-by: Joas Schilling <coding@schilljs.com>
@come-nc
Fixed migration step for user_ldap
ce5192a
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Put back length check to have a clear error
d4c49a3
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fixes in migration step
c4d8bd9
We cannot set ldap_dn_hash column as notnull because it is empty for
existing users before postSchemaChange is called
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add the columns and alter the index in Version1010Date20200630192842
14f0020
This is to ensure new installations do not need to go through migration
history.
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Make sure that hash function returns a string
df25a6d
The documentation says it can return false, and even if that is highly
unlikely for sha256, better safe than sorry.
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add an index for directory_uuid as well
5143249
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Use clearer names for variables
a359047
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
come-ncforce-pushed the fix/support-ldap-long-dns branch from fd43e4e to a359047CompareNovember 23, 2021 08:20
@come-nc
come-nc requested review from a team and icewind1991 and removed request for a teamDecember 9, 2021 11:41
CarlSchwan
CarlSchwan reviewed Dec 9, 2021
View reviewed changes
Comment threadapps/user_ldap/lib/Migration/Version1130Date20211102154716.php
@@ -0,0 +1,146 @@
<?php

@CarlSchwanCarlSchwanDec 9, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright

@skjnldsv
skjnldsv merged commit 2e869fd into masterDec 9, 2021
@skjnldsv
skjnldsv deleted the fix/support-ldap-long-dns branch December 9, 2021 16:18
@skjnldsv

skjnldsv commented Dec 9, 2021

Copy link
Copy Markdown
Member

/backport to stable23

@skjnldsv

skjnldsv commented Dec 9, 2021

Copy link
Copy Markdown
Member

/backport to stable22

@skjnldsv

skjnldsv commented Dec 9, 2021

Copy link
Copy Markdown
Member

/backport to stable21

@skjnldsv

skjnldsv commented Dec 9, 2021

Copy link
Copy Markdown
Member

/backport to stable20

@CarlSchwan

CarlSchwan commented Dec 9, 2021

Copy link
Copy Markdown
Member

Sorry for remembering that 2 minutes too late, but I think this is missing a version bump. Otherwise the migration won't run in our development instances.

@come-nccome-nc mentioned this pull request Dec 9, 2021
Merged
@backportbot-nextcloud

backportbot-nextcloudBot commented Dec 9, 2021

Copy link
Copy Markdown

The backport to stable20 failed. Please do this backport manually.

@backportbot-nextcloud

backportbot-nextcloudBot commented Dec 9, 2021

Copy link
Copy Markdown

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud

backportbot-nextcloudBot commented Dec 9, 2021

Copy link
Copy Markdown

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud

backportbot-nextcloudBot commented Dec 9, 2021

Copy link
Copy Markdown

The backport to stable21 failed. Please do this backport manually.

@skjnldsv

skjnldsv commented Dec 10, 2021

Copy link
Copy Markdown
Member

@come-nc please do the backports manually :(

This was referenced Dec 13, 2021
Merged
Merged
Merged
Merged
@come-nccome-nc linked an issue Feb 28, 2022 that may be closed by this pull request
LDAP entries with DN > 255 characters are unsupported #2213
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsvskjnldsvskjnldsv approved these changes
@CarlSchwanCarlSchwanCarlSchwan approved these changes
@blizzzblizzzAwaiting requested review from blizzz
@nickvergessennickvergessenAwaiting requested review from nickvergessen
@icewind1991icewind1991Awaiting requested review from icewind1991

Assignees

@come-nccome-nc

Labels

3. to reviewWaiting for reviewsfeature: ldap

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LDAP entries with DN > 255 characters are unsupported

5 participants

@come-nc@blizzz@skjnldsv@CarlSchwan@nickvergessen