Skip to content
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

limit ldap_dn columns to 4000 #31683

Merged
merged 5 commits into from
Mar 30, 2022
Merged

limit ldap_dn columns to 4000 #31683

merged 5 commits into from
Mar 30, 2022

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Mar 23, 2022

fixes #31681, satisfies #31679

@blizzz blizzz added this to the Nextcloud 24 milestone Mar 23, 2022
come-nc
come-nc reviewed Mar 24, 2022
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

You also need to change the check in AbstractMapping::map
And I do not think the migration step for existing data is correct. Incomplete dn in mapping is just broken mapping lines which will appear as remnants for users and ghost for groups.
These lines should be deleted I suppose? Or the migration should just fail and we consider this an invalid previous state.

Realistically the case should not appear as maximum dn length was 255 not long ago and I would not expect anyone using >4000 dn.

Ok so actually maybe it’s good to truncate the lines because it will allow the admin to use the remnant system to clean up users. And they can see what the dn was to know which entry is concerned.
So maybe leave it like this but log which entries have been truncated?

apps/user_ldap/lib/Migration/Version1141Date20220323143801.php Show resolved Hide resolved
apps/user_ldap/lib/Migration/Version1141Date20220323143801.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Migration/Version1141Date20220323143801.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Migration/Version1141Date20220323143801.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Migration/Version1141Date20220323143801.php Show resolved Hide resolved
@come-nc come-nc mentioned this pull request Mar 24, 2022
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz
Copy link
Member Author

blizzz commented Mar 28, 2022

I got confirmation that the first commits unblocks update with Oracle for good.

@blizzz blizzz requested a review from come-nc March 28, 2022 13:28
@blizzz blizzz mentioned this pull request Mar 28, 2022
Merged
@come-nc
Copy link
Contributor

come-nc commented Mar 30, 2022

I do not understand the checkers failure, was there a composer update or something?

@come-nc
Copy link
Contributor

come-nc commented Mar 30, 2022

/rebase

@come-nc
Copy link
Contributor

come-nc commented Mar 30, 2022

/rebase

blizzz and others added 5 commits March 30, 2022 14:43
@blizzz @come-nc
limit ldap_dn columns to 4000
2f49f4a 
- enlarging string cols from 255 to more than 4000 fails on default
  Oracle installations for invalid datatype conversion

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @come-nc
shrink ldap_dn column to 4000
52b57eb 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@come-nc
Adapt max length limit in AbstactMapping to match database
6c7ba01 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Use the new octetLength function to filter lines to migrate
d07208b 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Build autoloaders
07ab258 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc merged commit 2bf0eff into master Mar 30, 2022
@come-nc come-nc deleted the fix/31681/oracle-str-col branch March 30, 2022 16:28
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@PVince81 PVince81 mentioned this pull request Apr 8, 2022
Closed
8 tasks
@come-nc
Copy link
Contributor

come-nc commented Dec 13, 2022

/backport to stable22

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 13, 2022
Merged
@come-nc
Copy link
Contributor

come-nc commented Dec 13, 2022

/backport to stable23

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@come-nc come-nc come-nc approved these changes

Assignees
No one assigned
Labels
bug feature: ldap
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.

[Bug]: reduce ldap_dn field to 4000 bytes
3 participants
@blizzz @come-nc @nickvergessen