Storage repair iterates over all LDAP users during upgrade #26774

Closed
sbraz opened this Issue Dec 6, 2016 · 6 comments

Projects

None yet

3 participants

@sbraz
sbraz commented Dec 6, 2016

As discussed with @DeepDiver1975, while upgrading from 9.1.0 to 9.1.2 for an instance which uses LDAP, the code iterates over all users matched by the LDAP filter, not only those who connected once.

@DeepDiver1975 DeepDiver1975 added this to the 9.1.4 milestone Dec 6, 2016
@PVince81
Collaborator
PVince81 commented Dec 6, 2016

I just realized a flaw of this: if ($this->config->getAppValue('core', 'repairlegacystoragesdone') === 'yes').

This repair was supposed to run only for installs that were created before OC 8.1 or so, then never run again thanks to this flag.

However this flag isn't set for new OC >= 8.1 new installs, so it would run at least for the very first upgrade. This is wrong.

One idea would be to kill that repair step completely in OC >= 8.1.

@PVince81
Collaborator
PVince81 commented Dec 6, 2016

Quick workaround:
insert into oc_appconfig values ('core', 'repairlegacystoragesdone', 'yes');

@PVince81
Collaborator
PVince81 commented Dec 6, 2016

From what I see that class was introduced in OC 8.0, so it's the upgrade from OC 7 to OC 8 that needed this repair. If the repair has been done already then there is no need to do it in later versions.

@PVince81
Collaborator
PVince81 commented Dec 6, 2016

One commonly observed problem is that on some setups people ignored the warnings given by this repair step in case of unrepairable situations. Then the duplicate storages still exist up to current versions. (the duplicates could only have been created in OC < 8.0).

But with that reasoning, if the duplicates could not be fixed automatically, the repair step itself can still be removed from the codebase as it wouldn't be useful anyway in such situations.

However, the legacy storage fallback would still make sure that OC still works as before. There are discussions about removing the fallback as well, which is a much more risky change: #26325

@PVince81 PVince81 self-assigned this Dec 6, 2016
@PVince81 PVince81 referenced this issue Dec 6, 2016
Merged

Remove obsolete RepairLegacyStorages repair step #26779

3 of 11 tasks complete
@PVince81
Collaborator
PVince81 commented Dec 6, 2016

PR to remove the repair step completely #26779

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