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

Fix duplicated UUID migration issue #30981

Merged
merged 8 commits into from
Feb 10, 2022
Merged

Fix duplicated UUID migration issue #30981

merged 8 commits into from
Feb 10, 2022

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Feb 2, 2022 •
edited
Loading

solves #30936

strategy based on #30936 (comment)

  • invalidate duplicated UUIDs in migration step
  • make it possible to re-run migration step for broken setup
  • occ command to let Nextcloud try to update UUIDs to current value¹
  • add a warning in admin overview if invalidated UUIDs are present
  • testing

¹ could be a manually triggerable repair step, but it would run once, be less flexible and this way it MAY be also easier for some LDAP server migration scenarios when an update was needed anyways (but only works of DN stays identical).

@blizzz blizzz added this to the Nextcloud 24 milestone Feb 2, 2022
@solracsf solracsf linked an issue Feb 3, 2022 that may be closed by this pull request
[Bug]: [user_ldap] Migration from 1.13.0 to 1.13.1 broken #30936
Closed
8 tasks
@come-nc come-nc requested a review from a team February 7, 2022 15:12
@come-nc
Copy link
Contributor

come-nc commented Feb 7, 2022

occ command looks good to me now, I tested simple cases where one uuid is wrong, or giving it a non-mapped dn to test the error. Should be tested some more.

@come-nc
Copy link
Contributor

come-nc commented Feb 8, 2022

Testing the migration with duplicated UUIDs give me:

Updating <user_ldap> ...
InvalidArgumentException: Only strings, Literals and Parameters are allowed
Update failed

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 8, 2022
@blizzz blizzz marked this pull request as ready for review February 8, 2022 21:11
@blizzz
Copy link
Member Author

blizzz commented Feb 8, 2022

Added the checkup warning and did some basic testing with the occ command, which was working as expected.

Minor annoyance: we have no means to clean up (i.e. remove) old gone groups. That's not a blocker and can be worked around with custom instructions if necessary.

@blizzz blizzz force-pushed the fix/30936/ldap-dup-uuid-migration branch from 7bc9118 to 3e72b4e Compare February 9, 2022 15:56
@blizzz
Copy link
Member Author

blizzz commented Feb 9, 2022 •
edited
Loading

Rebased and squashed the fixups. My testing was successful so far.

PVince81
PVince81 reviewed Feb 9, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

I flew over the code and didn't find anything notable apart from my minor comments.

I hope others can chime in to review the more complex code more in detail,

apps/user_ldap/lib/Command/UpdateUUID.php
use Symfony\Component\Console\Output\OutputInterface;
use function sprintf;

class UuidUpdateReport {
Copy link
Member

Choose a reason for hiding this comment

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

two classes per file, is that still legal or not recommended ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Legal it is. I believe it is also not the sole case in our code bases, but given, I would not know for certain where to look at first. It is an internal helper class that just contains public properties, in order to not use an arbitrary array with with arbitrary keys.

apps/user_ldap/lib/Jobs/CleanUp.php Show resolved Hide resolved
@blizzz
Copy link
Member Author

blizzz commented Feb 9, 2022

last fixups were to fix unit tests

come-nc
come-nc approved these changes Feb 10, 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.

👍

PVince81
PVince81 approved these changes Feb 10, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

there's a DCO error though

@PVince81
Copy link
Member

/backport to stable23

@PVince81
Copy link
Member

/backport to stable22

@PVince81
Copy link
Member

/backport to stable21

@PVince81
Copy link
Member

/backport to stable20

@nickvergessen @come-nc
Don't provide favorite activity settings
1ab0ed8 
Since mails and notifications are only available for actions of other users
it does not make sense to allow changing this.
It also prevents the common misunderstanding with
"file was changed inside a favorited folder"

Signed-off-by: Joas Schilling <coding@schilljs.com>
@PVince81
Copy link
Member

down to 20 because the original fix was put there as well...

nickvergessen and others added 7 commits February 10, 2022 10:14
@nickvergessen @come-nc
Temporarily disable samba-non-native as it times out too often
de01da7 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@blizzz @come-nc
invalidated duplicated UUIDs prior to migration change
71fb449 
- in a proper setup there are no duplicated UUIDs
- not all setups are proper
- log warning to admin

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @come-nc
allow to re-run migration step when backup DB is already created
601ce05 
- to heal incomplete states from broken upgrades

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @come-nc
add occ command to update UUIDs (incomplete)
45d4c0b 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@come-nc
Fix ldap:update-uuid
9f2db6e 
Generators cannot be iterated with while or returned by an other
 generator, using foreach instead.
And a few other problems.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix psalm errors in apps/user_ldap/lib/Jobs/CleanUp.php
bcfe99c 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzz @come-nc
Setup warning for invalid LDAP user or group UUIDs.
b28aa52 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@come-nc come-nc force-pushed the fix/30936/ldap-dup-uuid-migration branch from 47c3da0 to b28aa52 Compare February 10, 2022 09:16
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 10, 2022
@blizzz blizzz merged commit c605ef1 into master Feb 10, 2022
@blizzz blizzz deleted the fix/30936/ldap-dup-uuid-migration branch February 10, 2022 10:53
@backportbot-nextcloud
Copy link

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

@backportbot-nextcloud
Copy link

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

@backportbot-nextcloud
Copy link

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

@backportbot-nextcloud
Copy link

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

@come-nc
Copy link
Contributor

come-nc commented Feb 10, 2022

I messed up my git rebase -i and duplicated two commits from Joas.
Not sure how that happenned but it causes no trouble on master because the commits were already on master.
For instance 1ab0ed8 is a duplicate of 5f5142f

I will create the backport by hand for 23 and backport from there.

This was referenced Feb 10, 2022
Closed
Merged
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 27, 2023
@ChristophWurst
Copy link
Member

Command is not documented in admin manual 😿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

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

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: ldap pending documentation This pull request needs an associated documentation update regression
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.

[Bug]: [user_ldap] Migration from 1.13.0 to 1.13.1 broken
6 participants
@blizzz @come-nc @PVince81 @ChristophWurst @skjnldsv @nickvergessen