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

This change helps user to switch between master keys #27512

Merged
merged 1 commit into from May 8, 2017

Conversation

Projects
None yet
4 participants
@sharidas
Member

sharidas commented Mar 27, 2017

and user specific keys.

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

Description

This change helps us to make master key as default for encryption instead of default configuration we had.

Related Issue

fixes #26847

Motivation and Context

How Has This Been Tested?

Testing yet to be done. This has only UI change.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 27, 2017

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @tomneedham to be potential reviewers.

mention-bot commented Mar 27, 2017

@sharidas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @tomneedham to be potential reviewers.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 27, 2017

Member

I would like to know if the UI part looks ok?

Member

sharidas commented Mar 27, 2017

I would like to know if the UI part looks ok?

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 27, 2017

Member

@felixheidecke Any suggestion?

Member

sharidas commented Mar 27, 2017

@felixheidecke Any suggestion?

@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Mar 27, 2017

Contributor

@sharidas Could you provide a few screenshots? Would be the fastest way to get feedback ;-)

Contributor

felixheidecke commented Mar 27, 2017

@sharidas Could you provide a few screenshots? Would be the fastest way to get feedback ;-)

Show outdated Hide outdated settings/js/admin.js
$('#confirm-encryption-warning').addClass("hidden");
}
if ($('#enableEncryption').prop("checked") == true) {

This comment has been minimized.

@PVince81

PVince81 Mar 27, 2017

Member

always use strict equality ===.

please setup jshint/jslint, it will show you warnings for this

@PVince81

PVince81 Mar 27, 2017

Member

always use strict equality ===.

please setup jshint/jslint, it will show you warnings for this

Show outdated Hide outdated settings/js/admin.js
@@ -41,6 +41,61 @@ $(document).ready(function(){
$('#encryptionAPI div#EncryptionWarning').toggleClass('hidden');
});
function selectEncryptionBehavior() {
$('#confirm-encryption-warning').addClass("hidden");

This comment has been minimized.

@PVince81

PVince81 Mar 27, 2017

Member

this code is difficult to read

I suggest to have only a single method, like "onChange" which then toggles the options based on the conditions.
For example:

$('#encryptionAPI').toggleClass(!userSpecificEncryption, 'hidden')

This way you only need a single row instead of having it appear three times in different functions

@PVince81

PVince81 Mar 27, 2017

Member

this code is difficult to read

I suggest to have only a single method, like "onChange" which then toggles the options based on the conditions.
For example:

$('#encryptionAPI').toggleClass(!userSpecificEncryption, 'hidden')

This way you only need a single row instead of having it appear three times in different functions

Show outdated Hide outdated settings/js/admin.js
});
$('#reconfirm-encryption-type').click(function (event) {
console.log("I am clicked the value selected is = ", $('#keyTypeId option:selected').val());

This comment has been minimized.

@PVince81

PVince81 Mar 27, 2017

Member

remove logging

@PVince81

PVince81 Mar 27, 2017

Member

remove logging

Show outdated Hide outdated settings/js/admin.js
console.log("I am clicked the value selected is = ", $('#keyTypeId option:selected').val());
if ($('#keyTypeId option:selected').val() == 'masterkey') {
OC.AppConfig.setValue('encryption', 'useMasterKey', '1');
location.reload();

This comment has been minimized.

@PVince81

PVince81 Mar 27, 2017

Member

the function OC.AppConfig.setValue() does an ajax call, so if you reload the page directly after that there is a slight risk of race condition in which the value isn't completely saved. Usually when reloading the page the browser will abort all ajax connections.

only reload the page once the ajax call finished

@PVince81

PVince81 Mar 27, 2017

Member

the function OC.AppConfig.setValue() does an ajax call, so if you reload the page directly after that there is a slight risk of race condition in which the value isn't completely saved. Usually when reloading the page the browser will abort all ajax connections.

only reload the page once the ajax call finished

Show outdated Hide outdated settings/templates/panels/admin/encryption.php
<select id="keyTypeId" name="keyType">
<option value="nokey">Please select an encryption option</option>
<option value="masterkey" <?php echo \OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', 0) !== 0 ? 'selected' : ''; ?>>Master Key</option>
<option value="customkey">User-specific key</option>

This comment has been minimized.

@PVince81

PVince81 Mar 27, 2017

Member

text translation missing, please recheck all the texts and make sure you're using the translation function

@PVince81

PVince81 Mar 27, 2017

Member

text translation missing, please recheck all the texts and make sure you're using the translation function

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 30, 2017

Member

@PVince81 I have updated the patch set with the changes mentioned below:

  1. Replaced the drop texts with the translation function
  2. Used ajaxStop function after the setValue's used.
  3. Removed console.log which was there in the code.
  4. Used === for equality check.
  5. Wrote a single function which gets called for the onChange event of the drop down selection.
  6. Tried adding some comments, to make the code readable.

Let me know if the code looks better, else I can refactor again.

Member

sharidas commented Mar 30, 2017

@PVince81 I have updated the patch set with the changes mentioned below:

  1. Replaced the drop texts with the translation function
  2. Used ajaxStop function after the setValue's used.
  3. Removed console.log which was there in the code.
  4. Used === for equality check.
  5. Wrote a single function which gets called for the onChange event of the drop down selection.
  6. Tried adding some comments, to make the code readable.

Let me know if the code looks better, else I can refactor again.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 31, 2017

Member

@felixheidecke Below are the screenshots:
a) The initial page when user clicks on the encryption ( after encryption app is enabled and user re-logins back to the UI)
encryptionpage
b) The drop down select has 3 options. Of them default is what user sees in encryption page, unless user selects any of the other two and configures.
c) The second option in the drop down is : "Master Key". When user selects that from the drop down the following page appears on screen:
masterkeyselection
c a) When master key is selected the user is asked with a warning message. This warning message helps user understand that once selected it cannot be reverted.
c b ) If user clicks "Enable Selection" of master key, then the page reloads and you see a screen as shown below:
masterkeyselectedpage
d) If user selects the 3rd option which is: "User-specific key" this is how the screen appears
userspecifickey
d a) Once the user clicks "Enable server-side encryption" the screen appears as shown below:
enable_server_side_encrypt
d b) Once the user clicks "Enable encryption" this is how the screen appears:
enableencryption
d c) After entering the pasword for recovery key the screen appears as shown below:
afterrecoverykey
e) Once the selection of master key or user specific key is made I have made a provision to disable the select ( drop down).
Let me know if the screenshot helps you :)

Member

sharidas commented Mar 31, 2017

@felixheidecke Below are the screenshots:
a) The initial page when user clicks on the encryption ( after encryption app is enabled and user re-logins back to the UI)
encryptionpage
b) The drop down select has 3 options. Of them default is what user sees in encryption page, unless user selects any of the other two and configures.
c) The second option in the drop down is : "Master Key". When user selects that from the drop down the following page appears on screen:
masterkeyselection
c a) When master key is selected the user is asked with a warning message. This warning message helps user understand that once selected it cannot be reverted.
c b ) If user clicks "Enable Selection" of master key, then the page reloads and you see a screen as shown below:
masterkeyselectedpage
d) If user selects the 3rd option which is: "User-specific key" this is how the screen appears
userspecifickey
d a) Once the user clicks "Enable server-side encryption" the screen appears as shown below:
enable_server_side_encrypt
d b) Once the user clicks "Enable encryption" this is how the screen appears:
enableencryption
d c) After entering the pasword for recovery key the screen appears as shown below:
afterrecoverykey
e) Once the selection of master key or user specific key is made I have made a provision to disable the select ( drop down).
Let me know if the screenshot helps you :)

Show outdated Hide outdated settings/js/admin.js
OC.AppConfig.setValue('core', 'encryption_enabled', 'yes');
OC.AppConfig.setValue("encryption", "encryptHomeStorage", '0');
OC.AppConfig.setValue('encryption', 'useMasterKey', '1');
$(document).ajaxStop(function () {

This comment has been minimized.

@PVince81

PVince81 Mar 31, 2017

Member

No, this will break in some cases.

Not only this code is doing ajax calls. There are background processes like the notifications app, the heartbeat, etc that will trigger ajax calls.

A better approach is to use promises:

  • adjust setValue and other methods to return the result of $.post or other ajax calls
  • use $.when(call1, call2, call3).then(function() { ... }) to find out when all the three calls have finished.
@PVince81

PVince81 Mar 31, 2017

Member

No, this will break in some cases.

Not only this code is doing ajax calls. There are background processes like the notifications app, the heartbeat, etc that will trigger ajax calls.

A better approach is to use promises:

  • adjust setValue and other methods to return the result of $.post or other ajax calls
  • use $.when(call1, call2, call3).then(function() { ... }) to find out when all the three calls have finished.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 31, 2017

Member

The flow is wrong, "Enable server-side encryption" must always be the first entry.
Then after that the selection of encryption type.

Also, remember that this checkbox "Enable server-side encryption" is about enabling the encryption subsystem/engine in core, not enabling a specific module.

The code with the encryption type selection needs to be part of the "Default encryption module" app (aka plugin). Keep in mind that other developers can write their own "Custom encryption module" (aka plugin) in which case the new dropdown is not relevant at all.

The encryption module code is in the "apps/encryption" folder. Please move the dropdown logic there.

Member

PVince81 commented Mar 31, 2017

The flow is wrong, "Enable server-side encryption" must always be the first entry.
Then after that the selection of encryption type.

Also, remember that this checkbox "Enable server-side encryption" is about enabling the encryption subsystem/engine in core, not enabling a specific module.

The code with the encryption type selection needs to be part of the "Default encryption module" app (aka plugin). Keep in mind that other developers can write their own "Custom encryption module" (aka plugin) in which case the new dropdown is not relevant at all.

The encryption module code is in the "apps/encryption" folder. Please move the dropdown logic there.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 31, 2017

Member

Flow as follows:

  1. Admin enables the app "default encryption module"
  2. Admin goes to admin page
  3. Admin clicks "Enable server-side encryption" then "Enable encryption"
  4. Admin scrolls down to the "Encryption" section (let's rename it to "Default encryption module")
  5. Admin selects encryption type (which is the only visible item at this stage)
  6. If admin picked "user-keys": now display all old fields
    • if admin picked "master-key": no further fields

The dropdown needs a button "Select this mode".
Once clicked, the dropdown must disappear and be replaced with a label: "Encryption type: xyz" which cannot be changed any more.

Member

PVince81 commented Mar 31, 2017

Flow as follows:

  1. Admin enables the app "default encryption module"
  2. Admin goes to admin page
  3. Admin clicks "Enable server-side encryption" then "Enable encryption"
  4. Admin scrolls down to the "Encryption" section (let's rename it to "Default encryption module")
  5. Admin selects encryption type (which is the only visible item at this stage)
  6. If admin picked "user-keys": now display all old fields
    • if admin picked "master-key": no further fields

The dropdown needs a button "Select this mode".
Once clicked, the dropdown must disappear and be replaced with a label: "Encryption type: xyz" which cannot be changed any more.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Mar 31, 2017

Member

@PVince81 Thanks for the review comments. I have updated the PR and below are the changes made in the updated PR:

  1. Made the work flow as follows:
    After enabling the encryption app and after re-login, admin when navigates to "encryption" page
    a) "Encryption" has been renamed to "Default encryption module"
    b) Once user selects either of the options: "Master Key" or "User-specific Key", a "select this mode" button appears
    c) Once the button is clicked based on the respective mode, the drop down and the button is not visible to the user. And a label appers "Encryption: master key" if master was selected or "Encryption: User specific key" if user specific key was selected.
    d) If user had opted for the master key then page reload happens
    e) The logic for the user specific key remains the same.

Let me know how the current PR looks?

Member

sharidas commented Mar 31, 2017

@PVince81 Thanks for the review comments. I have updated the PR and below are the changes made in the updated PR:

  1. Made the work flow as follows:
    After enabling the encryption app and after re-login, admin when navigates to "encryption" page
    a) "Encryption" has been renamed to "Default encryption module"
    b) Once user selects either of the options: "Master Key" or "User-specific Key", a "select this mode" button appears
    c) Once the button is clicked based on the respective mode, the drop down and the button is not visible to the user. And a label appers "Encryption: master key" if master was selected or "Encryption: User specific key" if user specific key was selected.
    d) If user had opted for the master key then page reload happens
    e) The logic for the user specific key remains the same.

Let me know how the current PR looks?

@PVince81 PVince81 added this to the 10.0.1 milestone Apr 3, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 3, 2017

Member
  • BUG: Getting this message after "enable encryption" then "enable default encryption module app":
    "Encryption App is enabled but your keys are not initialized, please log-out and log-in again"

This message shouldn't appear until an encryption type has been selected, because this message is only relevant for user-key mode encryption.

  • TODO: change the button text to "Permanently select this mode"
  • BUG: do not display anything under the dropdown until a mode is selected. So selecting "user-key" should not show anything either. Once the selection is made, the dropdown disappears forever and the rest of the form can appear.
Member

PVince81 commented Apr 3, 2017

  • BUG: Getting this message after "enable encryption" then "enable default encryption module app":
    "Encryption App is enabled but your keys are not initialized, please log-out and log-in again"

This message shouldn't appear until an encryption type has been selected, because this message is only relevant for user-key mode encryption.

  • TODO: change the button text to "Permanently select this mode"
  • BUG: do not display anything under the dropdown until a mode is selected. So selecting "user-key" should not show anything either. Once the selection is made, the dropdown disappears forever and the rest of the form can appear.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 3, 2017

Member

Enabling the modes worked fine, nice!

Member

PVince81 commented Apr 3, 2017

Enabling the modes worked fine, nice!

Show outdated Hide outdated apps/encryption/js/settings-admin.js
encryptionTypeSelection($("#keyTypeId :selected").val(), "static");
OC.AppConfig.getValue("encryption", "useMasterKey", function ($value) {

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

note: a way to prevent reading these values with ajax would be to append them into the template on the PHP side then read them out with JS.

@PVince81

PVince81 Apr 3, 2017

Member

note: a way to prevent reading these values with ajax would be to append them into the template on the PHP side then read them out with JS.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
//Action to be taken when "Select this mode" button is selected.
$("#select-mode,#keyTypeId").addClass("hidden");
if($("#keyTypeId :selected").val() === "masterkey") {
$.when(OC.AppConfig.setValue("encryption", "encryptHomeStorage", '0'),

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

this cannot work properly, you didn't adjust OC.AppConfig.setValue to actually return the ajax object

@PVince81

PVince81 Apr 3, 2017

Member

this cannot work properly, you didn't adjust OC.AppConfig.setValue to actually return the ajax object

This comment has been minimized.

@PVince81

PVince81 Apr 3, 2017

Member

well, the two "setValue" calls do work, but the final callback to reload the page is never called

@PVince81

PVince81 Apr 3, 2017

Member

well, the two "setValue" calls do work, but the final callback to reload the page is never called

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Apr 4, 2017

Member

@PVince81 I have updated the PR for review by making the changes as follows:

  1. Fixed bug : Getting this message after "enable encryption" then "enable default encryption module app": "Encryption App is enabled but your keys are not initialized, please log-out and log-in again"
  2. Changed button text to "Permanently select this mode"
  3. Once the user clicks the "Permanently select this mode", the rest of the form is displayed and the drop down is not displayed for ever.
  4. For the getValue thing, I have moved the call to the PHP template and the JS code reads from there.
  5. I have made adjustments to call the 2 setValue and passed the promise objects to when. I did a verification using firebug. When page reload is called, the setValues are seen to be completed their job. Verified in the network response section of firebug.

Hope the updated PR looks fine.

Member

sharidas commented Apr 4, 2017

@PVince81 I have updated the PR for review by making the changes as follows:

  1. Fixed bug : Getting this message after "enable encryption" then "enable default encryption module app": "Encryption App is enabled but your keys are not initialized, please log-out and log-in again"
  2. Changed button text to "Permanently select this mode"
  3. Once the user clicks the "Permanently select this mode", the rest of the form is displayed and the drop down is not displayed for ever.
  4. For the getValue thing, I have moved the call to the PHP template and the JS code reads from there.
  5. I have made adjustments to call the 2 setValue and passed the promise objects to when. I did a verification using firebug. When page reload is called, the setValues are seen to be completed their job. Verified in the network response section of firebug.

Hope the updated PR looks fine.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
//If user selects "Master Key" from the drop down
$("#select-mode").removeClass("hidden");
$("#encryptHomeStorageSetting, #encryptionSetRecoveryKey").addClass("hidden");

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

hide these permanently before any selection is made and only unhide if the other type is selected

@PVince81

PVince81 Apr 4, 2017

Member

hide these permanently before any selection is made and only unhide if the other type is selected

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

what about this comment ? this code isn't needed as you need to hide these fields only initially.

@PVince81

PVince81 Apr 4, 2017

Member

what about this comment ? this code isn't needed as you need to hide these fields only initially.

This comment has been minimized.

@PVince81

PVince81 May 4, 2017

Member

@sharidas any update here ?

@PVince81

PVince81 May 4, 2017

Member

@sharidas any update here ?

This comment has been minimized.

@sharidas

sharidas May 4, 2017

Member

looking into.

@sharidas

sharidas May 4, 2017

Member

looking into.

This comment has been minimized.

@sharidas

sharidas May 4, 2017

Member

I have removed this line. As its not required. Its toggled from hide only when user specific keys are permanently selected.

@sharidas

sharidas May 4, 2017

Member

I have removed this line. As its not required. Its toggled from hide only when user specific keys are permanently selected.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
if(state === "static") {
$("#select-mode, #keyTypeId").addClass("hidden");
$("#encryptHomeStorage, #encryptionSetRecoveryKey").addClass("hidden");
if($("#encryptionType").text().trim().length === 0) {

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

why $("#encryptionType").text() and not simply the value of encryptionType above ?

@PVince81

PVince81 Apr 4, 2017

Member

why $("#encryptionType").text() and not simply the value of encryptionType above ?

Show outdated Hide outdated apps/encryption/js/settings-admin.js
//This function is to arrange the display of the encryption page
//in the settings.
function encryptionTypeSelection(encryptionType, state=undefined) {

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

add a proper JSDoc block, see in other JS files how it looks like, for example: https://github.com/owncloud/core/blob/stable9.1/core/js/js.js#L181

and explain what "state" means

@PVince81

PVince81 Apr 4, 2017

Member

add a proper JSDoc block, see in other JS files how it looks like, for example: https://github.com/owncloud/core/blob/stable9.1/core/js/js.js#L181

and explain what "state" means

Show outdated Hide outdated apps/encryption/js/settings-admin.js
}
} else {
//If user selects "Please sect an encryption option" from the drop down

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

s/sect/select/

@PVince81

PVince81 Apr 4, 2017

Member

s/sect/select/

Show outdated Hide outdated apps/encryption/js/settings-admin.js
function adjustAjaxCallMasterKey () {
return $.ajax({
success: function() {
return OC.AppConfig.setValue('encryption', 'useMasterKey', '1');

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

currently setValue never returns anything, please check the method declaration here: https://github.com/owncloud/core/blob/master/core/js/config.js#L40

That's where you need to add a return to make it return the ajax object.

@PVince81

PVince81 Apr 4, 2017

Member

currently setValue never returns anything, please check the method declaration here: https://github.com/owncloud/core/blob/master/core/js/config.js#L40

That's where you need to add a return to make it return the ajax object.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
var masterAjaxObj = adjustAjaxCallMasterKey();
var userSpecificAjaxObj = adjustAjaxCallUserKey();
$.when(masterAjaxObj, userSpecificAjaxObj).done(function (masterKeyObj, userSpecificObj) {
location.reload();

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

have you tested this ? did it work ?

from reading the code above this cannot work.
try adding console.log('test') and see if something is logged after setting the master key.

@PVince81

PVince81 Apr 4, 2017

Member

have you tested this ? did it work ?

from reading the code above this cannot work.
try adding console.log('test') and see if something is logged after setting the master key.

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

Ok, so it seems to reload the page in my local test, but not for the right reason.

You made empty $.ajax calls that do nothing, so their promises/deferred will automatically be in the resolved state.
This is still wrong because it might happen before setValue finished.
Please use the approach I asked for above.

@PVince81

PVince81 Apr 4, 2017

Member

Ok, so it seems to reload the page in my local test, but not for the right reason.

You made empty $.ajax calls that do nothing, so their promises/deferred will automatically be in the resolved state.
This is still wrong because it might happen before setValue finished.
Please use the approach I asked for above.

Show outdated Hide outdated apps/encryption/lib/Session.php
@@ -60,7 +60,10 @@ public function setStatus($status) {
public function getStatus() {
$status = $this->session->get('encryptionInitialized');
if (is_null($status)) {
$status = self::NOT_INITIALIZED;
if(\OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', '0') !== '0'
or \OC::$server->getAppConfig()->getValue('encryption', 'encryptHomeStorage', '') !== '') {

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

Ok, so you're using the value "encryptHomeStorage" to find out whether user-key encryption is enabled, is that correct ?

@PVince81

PVince81 Apr 4, 2017

Member

Ok, so you're using the value "encryptHomeStorage" to find out whether user-key encryption is enabled, is that correct ?

Show outdated Hide outdated apps/encryption/templates/settings-admin.php
<label id="encryptionType">
<?php
$masterKey = \OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', '0');
$homeEncryption = \OC::$server->getAppConfig()->getValue('encryption', 'encryptHomeStorage', 'empty');

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

In the above logic you used '' as empty value, which is correct.
Why not use '' as well instead of 'empty' for consistency ?

Developers reading both pieces of code might get confused and think that "empty" is a different value than "".

@PVince81

PVince81 Apr 4, 2017

Member

In the above logic you used '' as empty value, which is correct.
Why not use '' as well instead of 'empty' for consistency ?

Developers reading both pieces of code might get confused and think that "empty" is a different value than "".

Show outdated Hide outdated apps/encryption/templates/settings-admin.php
<p id="encryptionKeySelection">
<select id="keyTypeId" name="keyType">
<option value="nokey"><?php p($l->t("Please select an encryption option"))?></option>
<option value="masterkey" <?php echo \OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', '0') !== '0' ? 'selected="selected"' : ''; ?>><?php p($l->t("Master Key"))?></option>

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

Use print_unescaped instead of echo

@PVince81

PVince81 Apr 4, 2017

Member

Use print_unescaped instead of echo

Show outdated Hide outdated apps/encryption/templates/settings-admin.php
<option value="customkey"><?php p($l->t("User-specific key"))?></option>
</select>
<button id="select-mode" type="button" class="hidden"><?php p($l->t("Permanently select this mode"));?></button>
<label id="masterKeyVal" class="hidden"><?php echo \OC::$server->getAppConfig()->getValue("encryption", "useMasterKey", "empty"); ?> </label>

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

In the past we usually used hidden input fields to make server values accessible to the JS code.
Might be better than using label.

Another better approach is using data attributes on a specific element: <div id="encryptionSettingsContainer" data-master-key="true">

@PVince81

PVince81 Apr 4, 2017

Member

In the past we usually used hidden input fields to make server values accessible to the JS code.
Might be better than using label.

Another better approach is using data attributes on a specific element: <div id="encryptionSettingsContainer" data-master-key="true">

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 4, 2017

Member
  • BUG: when enabling master key, the files are not encrypted at all (check any newly uploaded file under "data/admin/files/").

When I switch to master with the oc_appconfig settings from this branch, files also do not get encrypted.

However if I setup encryption from scratch on master with master key, it works.
So maybe something else is missing in the settings. Maybe compare the oc_appconfig values from both branches.

Member

PVince81 commented Apr 4, 2017

  • BUG: when enabling master key, the files are not encrypted at all (check any newly uploaded file under "data/admin/files/").

When I switch to master with the oc_appconfig settings from this branch, files also do not get encrypted.

However if I setup encryption from scratch on master with master key, it works.
So maybe something else is missing in the settings. Maybe compare the oc_appconfig values from both branches.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Apr 4, 2017

Member

@PVince81 I have updated the PR for review with the changes mentioned below:
a) I have added return before the https://github.com/owncloud/core/blob/master/core/js/config.js#L40.
b) Instead of using labels with hidden, div is being used with proper attributes.
c) Changed the code flow for User Specific Key. Now oc_appconfig will have userSpecificKey in "encryption" app. This will be set only when user selects User Specific Key mode in the UI.
d) Replaced echo with print_unescaped where mentioned.
e) Removed 'empty' and used '' for consistency.
f) Verified with Master Key and User Specific Key the data is getting encrypted. Verified by uploading a text file after relogin. And then cat the file inside data folder. I was able to see message:
"
HBEGIN:oc_encryption_module:OC_DEFAULT_MODULE:cipher:AES-256-CTR:signed:true:HEND
"
at the beginning of the line.

Let me know if more changes are required.

Member

sharidas commented Apr 4, 2017

@PVince81 I have updated the PR for review with the changes mentioned below:
a) I have added return before the https://github.com/owncloud/core/blob/master/core/js/config.js#L40.
b) Instead of using labels with hidden, div is being used with proper attributes.
c) Changed the code flow for User Specific Key. Now oc_appconfig will have userSpecificKey in "encryption" app. This will be set only when user selects User Specific Key mode in the UI.
d) Replaced echo with print_unescaped where mentioned.
e) Removed 'empty' and used '' for consistency.
f) Verified with Master Key and User Specific Key the data is getting encrypted. Verified by uploading a text file after relogin. And then cat the file inside data folder. I was able to see message:
"
HBEGIN:oc_encryption_module:OC_DEFAULT_MODULE:cipher:AES-256-CTR:signed:true:HEND
"
at the beginning of the line.

Let me know if more changes are required.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
location.reload();
});
} else if($("#keyTypeId :selected").val() === "customkey") {
OC.AppConfig.setValue("encryption", "userSpecificKey", '1');

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

so master key reloads the page but not this one ? any particular reason ?

@PVince81

PVince81 Apr 4, 2017

Member

so master key reloads the page but not this one ? any particular reason ?

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

also, since you added a new key it is not guaranteed that upgrading from 9.1 with encryption already enabled will work because this key will be missing. Need to find a way to solve this.

@PVince81

PVince81 Apr 4, 2017

Member

also, since you added a new key it is not guaranteed that upgrading from 9.1 with encryption already enabled will work because this key will be missing. Need to find a way to solve this.

This comment has been minimized.

@sharidas

sharidas Apr 4, 2017

Member

my mistake.

@sharidas

sharidas Apr 4, 2017

Member

my mistake.

@@ -37,7 +37,7 @@ OC.AppConfig={
OC.AppConfig.getCall('getValue',{app:app,key:key,defaultValue:defaultValue},callback);
},
setValue:function(app,key,value){
OC.AppConfig.postCall('setValue',{app:app,key:key,value:value});
return OC.AppConfig.postCall('setValue',{app:app,key:key,value:value});

This comment has been minimized.

@PVince81

PVince81 Apr 4, 2017

Member

postCall doesn't return anything, please fix accordingly

@PVince81

PVince81 Apr 4, 2017

Member

postCall doesn't return anything, please fix accordingly

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Apr 4, 2017

Member

@PVince81 I have updated the PR with the change to return from the postCall. I have verified the same using debugger: http://pasteboard.co/1MeQdMvX.png. I have also added the reload. Made sure that before the reload ( the second reload ) happens, the ajax request is completed. I have verified the data uploaded after relogin ( for both master key and user specific key) they found to be encrypted.

Member

sharidas commented Apr 4, 2017

@PVince81 I have updated the PR with the change to return from the postCall. I have verified the same using debugger: http://pasteboard.co/1MeQdMvX.png. I have also added the reload. Made sure that before the reload ( the second reload ) happens, the ajax request is completed. I have verified the data uploaded after relogin ( for both master key and user specific key) they found to be encrypted.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 6, 2017

Member

Works fine now.

Now to some additional missing bits:

  • TODO: add occ command to select encryption type and remove "occ encrpyiton:enable-master-key"
  • TODO: update documentation (raise ticket in doc repo) to update the web UI flow and CLI flow (mark the PR as pending until this PR here is merged)

I think later on we'll mark "Master key" in as "recommended", but not in this PR because there are still many features that do not work well with master key, so I'm not sure.

Member

PVince81 commented Apr 6, 2017

Works fine now.

Now to some additional missing bits:

  • TODO: add occ command to select encryption type and remove "occ encrpyiton:enable-master-key"
  • TODO: update documentation (raise ticket in doc repo) to update the web UI flow and CLI flow (mark the PR as pending until this PR here is merged)

I think later on we'll mark "Master key" in as "recommended", but not in this PR because there are still many features that do not work well with master key, so I'm not sure.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 6, 2017

Member
  • TODO: adjust tests/integration/run.sh to use the new commands to enable either user-key or master-key mode

Note: CI for these tests only run on the master branch so you'll need to run them locally.

For example:

% cd tests/integration
% sudo -uwwwrun OC_TEST_ENCRYPTION_ENABLED=1 OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED=1 ./run.sh features/webdav-related.feature
Member

PVince81 commented Apr 6, 2017

  • TODO: adjust tests/integration/run.sh to use the new commands to enable either user-key or master-key mode

Note: CI for these tests only run on the master branch so you'll need to run them locally.

For example:

% cd tests/integration
% sudo -uwwwrun OC_TEST_ENCRYPTION_ENABLED=1 OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED=1 ./run.sh features/webdav-related.feature

@PVince81 PVince81 removed the 3 - To Review label Apr 6, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 26, 2017

Member

@sharidas would be good to finish this, see my points above

Member

PVince81 commented Apr 26, 2017

@sharidas would be good to finish this, see my points above

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Apr 27, 2017

Member

@PVince81 I have updated the patch. I have removed "encrpyiton:enable-master-key" and created "encryption:select-encryption-type". It would accept either "masterkey" or "user-keys" ( without quotes). And confirmation question is also taken care. Apart from that I have also looked into run.sh. I have tried to update by removing "encryption:enable-master-key" and using "encryption:select-encryption-type". Tried testing it from the command line by enabling the app, enabling encryption, selecting either of the type {masterkey|user-keys}. Logged out from the UI. Uploaded files. Verified the header of the files. And they were encrypted.

Member

sharidas commented Apr 27, 2017

@PVince81 I have updated the patch. I have removed "encrpyiton:enable-master-key" and created "encryption:select-encryption-type". It would accept either "masterkey" or "user-keys" ( without quotes). And confirmation question is also taken care. Apart from that I have also looked into run.sh. I have tried to update by removing "encryption:enable-master-key" and using "encryption:select-encryption-type". Tried testing it from the command line by enabling the app, enabling encryption, selecting either of the type {masterkey|user-keys}. Logged out from the UI. Uploaded files. Verified the header of the files. And they were encrypted.

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas Apr 27, 2017

Member

Umm. PhantomJS error!! Tried to shake the jenkins by pushing again. But its failing again :(

Member

sharidas commented Apr 27, 2017

Umm. PhantomJS error!! Tried to shake the jenkins by pushing again. But its failing again :(

@sharidas

This comment has been minimized.

Show comment
Hide comment
@sharidas

sharidas May 3, 2017

Member

Encryption Key: Master Key

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.

Encryption Key: User-specific keys

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
  • TEST: Verified that after creating recovery password, the user password can be updated by applying the admin recovery password in UI.

Command line:

Encryption Key : User-specific keys ( ./occ encryption:select-encryption-type user-keys or ./occ encryption:select-encryption-type user-keys -y )

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
  • TEST: Verified that after creating recovery password, the user password can be updated by applying the admin recovery password in UI.

Encryption Key : Master key ( ./occ encryption:select-encryption-type masterkey or ./occ encryption:select-encryption-type masterkey -y )

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
Member

sharidas commented May 3, 2017

Encryption Key: Master Key

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.

Encryption Key: User-specific keys

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
  • TEST: Verified that after creating recovery password, the user password can be updated by applying the admin recovery password in UI.

Command line:

Encryption Key : User-specific keys ( ./occ encryption:select-encryption-type user-keys or ./occ encryption:select-encryption-type user-keys -y )

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
  • TEST: Verified that after creating recovery password, the user password can be updated by applying the admin recovery password in UI.

Encryption Key : Master key ( ./occ encryption:select-encryption-type masterkey or ./occ encryption:select-encryption-type masterkey -y )

  • TEST: Upload a file and verify its encrypted using cat command.
  • TEST: Create a file and verify its encrypted using cat command.
  • TEST: Modify the existing file, i.e, welcome.txt. Verified that file is encrypted after modification using cat command.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 4, 2017

Member
  • add spinner while encryption is being enabled. It took a few seconds to enable master key and it makes the UI look broken (check how other spinners are displayed for example in the share panel, there's a CSS class for it)
  • the text "Encryption: master key" that appears after going to that page again after enabling, the text must not display a hand cursor as it's not clickable

Otherwise this looked good in my quick local tests.

Member

PVince81 commented May 4, 2017

  • add spinner while encryption is being enabled. It took a few seconds to enable master key and it makes the UI look broken (check how other spinners are displayed for example in the share panel, there's a CSS class for it)
  • the text "Encryption: master key" that appears after going to that page again after enabling, the text must not display a hand cursor as it's not clickable

Otherwise this looked good in my quick local tests.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
$("#select-mode").click(function () {
//Action to be taken when "Select this mode" button is selected.
$("#loadSpinner").toggleClass("hidden");
$("#loadSpinner").addClass("loading");

This comment has been minimized.

@sharidas

sharidas May 5, 2017

Member

Does this spinner look ok?

@sharidas

sharidas May 5, 2017

Member

Does this spinner look ok?

This comment has been minimized.

@PVince81

PVince81 May 5, 2017

Member

Argh, global ids again ? What if another section also has a spinenr called "loadSpinner" ?

Please use class names instead.
The only id that's acceptable here is the parent element #encryptionKeySelection, so use a selector like #encryptionKeySelection .loading when toggling or something.

And when toggling please force the value to "true" as you might not know what its previous state is.

@PVince81

PVince81 May 5, 2017

Member

Argh, global ids again ? What if another section also has a spinenr called "loadSpinner" ?

Please use class names instead.
The only id that's acceptable here is the parent element #encryptionKeySelection, so use a selector like #encryptionKeySelection .loading when toggling or something.

And when toggling please force the value to "true" as you might not know what its previous state is.

Show outdated Hide outdated apps/encryption/js/settings-admin.js
$("#select-mode").click(function () {
//Action to be taken when "Select this mode" button is selected.
$("#encryptionKeySelection").toggleClass("loading", true);

This comment has been minimized.

@sharidas

sharidas May 5, 2017

Member

This looks better, right? I have tested the same.

@sharidas

sharidas May 5, 2017

Member

This looks better, right? I have tested the same.

This comment has been minimized.

@PVince81

PVince81 May 5, 2017

Member

Did you test it ?

The spinner only shortly appears at the very right of the row and disappears very quickly.

Also looking at the code it looks like it sets the class on the whole container, which looks wrong.
The spinner should have its own div element.

@PVince81

PVince81 May 5, 2017

Member

Did you test it ?

The spinner only shortly appears at the very right of the row and disappears very quickly.

Also looking at the code it looks like it sets the class on the whole container, which looks wrong.
The spinner should have its own div element.

//Action to be taken when "Select this mode" button is selected.
var $loadSpinner = $('#encryptionKeySelection').find('div.hidden').first();
$loadSpinner.toggleClass('hidden',false);
$loadSpinner.toggleClass('loading',true);

This comment has been minimized.

@sharidas

sharidas May 5, 2017

Member

Made the modification by having the div for spinner. And now atleast the spinner doesn't vanish quickly.

@sharidas

sharidas May 5, 2017

Member

Made the modification by having the div for spinner. And now atleast the spinner doesn't vanish quickly.

This change helps user to switch between master and user specific keys.
Signed-off-by: Sujith H <sharidasan@owncloud.com>
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 8, 2017

Member

Looks good now 👍

Member

PVince81 commented May 8, 2017

Looks good now 👍

@PVince81 PVince81 merged commit 4e5211b into master May 8, 2017

4 checks passed

Scrutinizer 3 new issues, 4 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the encryption-task-26847 branch May 8, 2017

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