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.

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

Fix migration issue caused during upgrade to version 10 #15

Merged
merged 1 commit into from Sep 18, 2017

Conversation

sharidas
Copy link
Contributor

This change will help users to fix the migration
issue caused during upgrade to version 10. If
user had opted user specific key in the older versions
then no drop down will appear in the version 10.
Same applies if user had opted for the masterkey
in older version, no drop down will appear in version
10.

Signed-off-by: Sujith H sharidasan@owncloud.com

@sharidas
Copy link
Contributor Author

sharidas commented Sep 13, 2017

This patch addresses the issue : owncloud/core#28985

The test matrix is as follows:

  • When encryption is enabled with userspecifickey in version 8.2.11 and then upgraded to 10.0.3. Result is : No drop down list for masterkey or userspecific key selection. Only options for recovery key is visible in UI.
  • when encryption is enabled with masterkey in version 8.2.11 and then upgraded to version 10.0.3. Result is: No drop down list for masterkey or userspecific key selection.
  • When encryption is enabled with userspecifickey in version 9.1.6 and then upgraded to 10.0.2. Result is : No drop down list for masterkey or userspecific key selection. Only options for recovery key is visible in UI.
  • when encryption is enabled with masterkey in version 9.1.6 and then upgraded to version 10.0.2. Result is: No drop down list for masterkey or userspecific key selection.

@sharidas sharidas force-pushed the fix-migration-issue branch 3 times, most recently from ae2ce20 to ef232cb Compare September 13, 2017 14:33
Copy link
Contributor

@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.

Fantastic. Can you use version_compare instead ?

$installedVersion = \OC::$server->getConfig()->getSystemValue('version', '0.0.0');

if ($installedVersion !== '0.0.0') {
$maxVersion = explode('.', $installedVersion)[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

public function run(IOutput $out) {
$installedVersion = \OC::$server->getConfig()->getSystemValue('version', '0.0.0');

if (version_compare('10.0.0', $installedVersion) === 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks better.

@PVince81
Copy link
Contributor

@sharidas when updating from 10.0.2 the logic should also operate, since 10.0.2 currently still shows the dropdown. When I upgrade from 10.0.2 with working encryption but dropdown shown to 10.0.3, then dropdown should disappear. It should be possible by running the same logic.

public function run(IOutput $out) {
$installedVersion = \OC::$server->getConfig()->getSystemValue('version', '0.0.0');

if (version_compare('10.0.0', $installedVersion) !== 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified with the change by upgrading from 10.0.2 to 10.0.3. In both the user specific key or master key when enabled the later version doesn't show the dropdown list now.

Copy link
Contributor

Choose a reason for hiding this comment

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

what you wrote here is basically $version !== '10.0.0' not $version <= 10.0.3

The reason it works is probably because the check in your "if" block would likely work correctly even without a version check. I'd rather have either the correct logic with version check, or no version check at all.

Furthermore if someone upgrades from 10.0.0 the logic below will not run at all.
Please recheck the documentation of version_compare

This change will help users to fix the migration
issue caused during upgrade to version 10. If
user had opted user specific key in the older versions
then no drop down will appear in the version 10.
Same applies if user had opted for the masterkey
in older version, no drop down will appear in version
10.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
public function run(IOutput $out) {
$installedVersion = \OC::$server->getConfig()->getSystemValue('version', '0.0.0');

if (version_compare('10.0.3', $installedVersion, '>=') === true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this is the only modification I made. This is equivalent to 10.0.3 >= $installeddVersion.

Since we are heading towards 10.0.3, this should be good enough. The UI changed from 10.0.2.
So till version < 10.0.2, the upgrade should work as it did. Or lets say if userSpecificKey is added to appconfig for versions less than 10.0.2, then, nothing would prohibit the code flow.

Now once the upgrade reaches 10.0.2 which is still less than 10.0.3, goes inside the if condition. And then tries to check if the condition is met with the inner if condition ( the one with lots of &&). And adds the userSpecificKey if the condition is true and UI will not have dropdown list.

Lets say if version >= 10.0.2, then the inner if condition may/may not be hit. Because if the encryption is enabled then either of the types would be selected. If not then again falls back to userSpecificKey. This logic applies till, including version 10.0.3.

Though technically the version check is redundant imho. Because unless explicitly selected for masterkey or enabled masterkey via command line => its userSpecificKey. Which sticks with the inner if condition.

@sharidas sharidas added this to the planned milestone Sep 15, 2017
Copy link
Contributor

@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.

👍

@PVince81 PVince81 merged commit 5f969f9 into master Sep 18, 2017
@PVince81 PVince81 deleted the fix-migration-issue branch September 18, 2017 13:56
@PVince81
Copy link
Contributor

@sharidas please backport to stable10 for 10.0.4

@sharidas
Copy link
Contributor Author

Backport : owncloud/core#29049

@felixboehm felixboehm modified the milestones: planned, development Oct 17, 2017
@patrickjahns patrickjahns removed this from the QA milestone Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants