-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add encryption version fix command #115
Conversation
|
c07c902
to
85207e0
Compare
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
============================================
+ Coverage 64.28% 69.76% +5.48%
- Complexity 598 619 +21
============================================
Files 33 34 +1
Lines 2212 2289 +77
============================================
+ Hits 1422 1597 +175
+ Misses 790 692 -98
Continue to review full report at Codecov.
|
85207e0
to
c96a10c
Compare
This error https://drone.owncloud.com/owncloud/encryption/599/150 is not reproducible in my local instance.
Digging into it... |
Using the unit test this is what I wanted to do:
But the problem here I find with Unit test
Acceptance test
|
c96a10c
to
81b8bc2
Compare
lib/Command/FixEncryptedVersion.php
Outdated
if ($oldEncryptedVersion !== null) { | ||
$encryptedVersion = $oldEncryptedVersion + $increment; | ||
} else { | ||
$encryptedVersion = $encryptedVersion + $increment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please recheck, from my understanding this will go as follows:
with $encryptedVersion = 6 at start
cycle 1: $increment=1, $encryptedVersion=6+1=7
cycle 2: $increment=2, $encryptedVersion=7+2=9
cycle 3: $increment=3, $encryptedVersion=9+3=12
...
if it doesn't do that (which might be confirmed with unit test), then rewrite the code to make it more clear what happens to the reader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it doesn't work like that, I have added php comment. Also adjusted the variable encryptedVersion
in this section to $newEncryptedVersion
, the oldEncryptedVersion
is renamed to $wrongEncryptedVersion
.
So basically its like this:
when the $wrongEncryptedVersion = 6 at start
cycle 1: $increment=1, $newEncryptedVersion = 6+1 = 7
cycle 2: $increment=2, $newEncryptedVersion = 6+2 = 8
cycle 3: $increment=3, $newEncryptedVersion = 6+3 = 9
Basically the $wrongEncryptedVersion
remains same and the $increment
is doing the increment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it was only a code readability issue, cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
b8d461a
to
c44077c
Compare
//Enable encryption app | ||
\OC_App::enable("encryption"); | ||
//Enable encryption | ||
\OC::$server->getConfig()->setAppValue('core', 'encryption_enabled', 'yes'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normally you should save appconfig values in a variable and set that variable back at the end of the test to avoid side effects
* set to a positive non zero number. | ||
*/ | ||
public function testEncryptedVersionZero() { | ||
$this->markTestSkipped("Skipping this test because the user login would fail in the CI. And once we have a db endpoints available in acceptance test, we would add the test over there"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please continue researching a bit
you should use $this->loginAsUser()
for a more accurate login simulation. see https://github.com/owncloud/core/blob/v10.1.1/tests/lib/TestCase.php#L368
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks better and more straightforward to read
please check my comment to unskip the unit tests, there should be a way
1bcf089
to
37bbca5
Compare
The output buffer issue https://drone.owncloud.com/owncloud/encryption/609/150 :( |
37bbca5
to
ebd1a20
Compare
not sure what this is and what I should do about it ? |
ebd1a20
to
fd1d53e
Compare
Ok :) Lets wait for the CI result. |
Will have to restart the CI again, because of unrelated failure https://drone.owncloud.com/owncloud/encryption/612/1494 :( |
The CI has passed. Would require re-review. |
looks good now. I'm missing a test where the correct value is lower than the encrypted version value and where decrementing would succeed. |
b92e86d
to
06c913f
Compare
Add a new command to fix the encryption version. This would help the admin to fix the issues related to encryption version mismatch for the files. Signed-off-by: Sujith H <sharidasan@owncloud.com>
This change helps to fix the errors caused while running the unit test without enabling the app. Signed-off-by: Sujith H <sharidasan@owncloud.com>
06c913f
to
4f2d98d
Compare
Ouch. Added the test at the end of the test file, which shows that the command would allow the files encrypted version reach the correct value by decrementing only. |
Re-review required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 please backport
Add a new command to fix the encryption
version. This would help the admin to
fix the issues related to encryption version
mismatch for the files.
Signed-off-by: Sujith H sharidasan@owncloud.com
Note: "backport" to core
stable10
is PR owncloud/core#35000related issue: https://github.com/owncloud/enterprise/issues/3194