Skip to content

Support LDAP dns longer than 255 characters #29523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Dec 9, 2021
Merged

Support LDAP dns longer than 255 characters #29523

merged 10 commits into from
Dec 9, 2021

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Nov 2, 2021

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
@come-nc
Copy link
Contributor Author

come-nc commented Nov 2, 2021

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
Copy link
Contributor Author

come-nc commented Nov 2, 2021

Related bug ticket: #2213

@come-nc come-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-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 8, 2021
@come-nc come-nc requested a review from nickvergessen November 8, 2021 09:04
@blizzz
Copy link
Member

blizzz commented Nov 16, 2021

/rebase

blizzz
blizzz requested changes Nov 16, 2021
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

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.

@blizzz
Copy link
Member

blizzz commented Nov 16, 2021

@nickvergessen what do you think about its backportability?

@blizzz
Copy link
Member

blizzz commented Nov 16, 2021

…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
@come-nc
Copy link
Contributor Author

come-nc commented Nov 18, 2021

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
Copy link
Contributor Author

come-nc commented Nov 18, 2021

…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-nc and 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-nc force-pushed the fix/support-ldap-long-dns branch from fd43e4e to a359047 Compare November 23, 2021 08:20
@come-nc come-nc requested review from a team and icewind1991 and removed request for a team December 9, 2021 11:41
CarlSchwan
CarlSchwan reviewed Dec 9, 2021
View reviewed changes
apps/user_ldap/lib/Migration/Version1130Date20211102154716.php
@@ -0,0 +1,146 @@
<?php
Copy link
Member

@CarlSchwan CarlSchwan Dec 9, 2021

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 master Dec 9, 2021
@skjnldsv skjnldsv deleted the fix/support-ldap-long-dns branch December 9, 2021 16:18
@skjnldsv
Copy link
Member

skjnldsv commented Dec 9, 2021

/backport to stable23

@skjnldsv
Copy link
Member

skjnldsv commented Dec 9, 2021

/backport to stable22

@skjnldsv
Copy link
Member

skjnldsv commented Dec 9, 2021

/backport to stable21

@skjnldsv
Copy link
Member

skjnldsv commented Dec 9, 2021

/backport to stable20

@CarlSchwan
Copy link
Member

CarlSchwan commented Dec 9, 2021

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-nc come-nc mentioned this pull request Dec 9, 2021
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Dec 9, 2021

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

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Dec 9, 2021

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

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Dec 9, 2021

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

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Dec 9, 2021

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

@skjnldsv
Copy link
Member

skjnldsv commented Dec 10, 2021

@come-nc please do the backports manually :(

This was referenced Dec 13, 2021
Merged
Merged
Merged
Merged
@come-nc come-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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews feature: 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

6 participants

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