Handle encryption state in web interface #6670

schiessle · 2017-09-27T12:13:59Z

add a nice looking lock in front of a end-to-end encrypted folder
handle permissions correctly
hide '...' menu for encrypted folders

The easiest way to review is by setting "encrypted" in the filecache table to "1" for a folder and check if all the file actions are gone.

schiessle · 2017-09-27T12:14:58Z

@nextcloud/javascript could need some help from our Java Script experts to hide the complete "..." menu for end-to-end encrypted folders.

codecov · 2017-09-29T15:20:08Z

Codecov Report

Merging #6670 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #6670      +/-   ##
============================================
+ Coverage     50.84%   50.85%   +<.01%     
  Complexity    24547    24547              
============================================
  Files          1585     1585              
  Lines         93801    93812      +11     
  Branches       1354     1357       +3     
============================================
+ Hits          47697    47709      +12     
+ Misses        46104    46103       -1

Impacted Files	Coverage Δ	Complexity Δ
core/js/mimetypelist.js	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
core/js/mimetype.js	`91.48% <100%> (+0.37%)`	`0 <0> (ø)`	⬇️
core/js/share.js	`43.15% <100%> (+1.22%)`	`0 <0> (ø)`	⬇️
core/js/files/client.js	`84.89% <100%> (+0.23%)`	`0 <0> (ø)`	⬇️
lib/private/Server.php	`83.19% <0%> (-0.12%)`	`126% <0%> (ø)`
apps/files_trashbin/lib/Trashbin.php	`72.53% <0%> (+0.24%)`	`136% <0%> (ø)`	⬇️
lib/private/Security/CertificateManager.php	`92.07% <0%> (+0.99%)`	`39% <0%> (ø)`	⬇️

danxuliu

If you could add unit tests for not rendering the actions menu trigger on encrypted folders, rendering encrypted folder icon for encrypted folders and ensuring that the is-encrypted property is parsed as expected in a FileInfo that would be great :-)

Besides my usual nitpicking the code looks good ;-)

danxuliu · 2017-10-08T22:19:00Z

apps/files/js/fileactions.js

@@ -370,6 +370,9 @@
 		_renderMenuTrigger: function($tr, context) {
 			// remove previous
 			$tr.find('.action-menu').remove();
+			var isE2EEncrypted = $tr.data('e2eencrypted');
+
+			if (isE2EEncrypted) return;


I think that the most common style in our JavaScript code is

if (isE2EEncrypted) { return; }

danxuliu · 2017-10-08T22:20:48Z

core/js/files/client.js

@@ -304,6 +309,13 @@
 				data.hasPreview = true;
 			}

+			var isEncryptedProp = props['{' + Client.NS_NEXTCLOUD + '}is-encrypted'];
+			if (!_.isUndefined(isEncryptedProp)) {
+				data.isEncrypted = isEncryptedProp === '1';


Just to be sure, the server is guaranteed to always return '1', never 1, true or 'true', right?

yes, that's what's stored in the file cache table

danxuliu · 2017-10-08T22:22:14Z

core/js/mimetypelist.js

@@ -108,6 +108,7 @@ OC.MimeTypeList={
    "folder-public",
    "folder-shared",
    "folder-starred",
+    "folder-encrypted",


Please amend to keep the alphabetical order :-)

schiessle · 2017-10-10T09:41:45Z

@danxuliu thanks for the review... I fixed the small stuff. May I ask you to have a look at the test? This would be awesome because I'm really bad with JS tests... Thanks a lot!

icewind1991 · 2017-10-12T12:13:58Z

I can still enter an encrypted folder
Share icon should stay aligned when the dots aren't there

Code looks good otherwise

danxuliu · 2017-10-14T13:28:46Z

@schiessle

May I ask you to have a look at the test?

No problem :-) I will need some clarification on the expected behaviour, though. What actions should be available on encrypted folders? And why are those actions available and not others?

I have also realized that just not rendering the three dots button is not a proper solution. Right now an encrypted folder can be favourited by clicking on the star icon on the left. However, once the checkbox is placed where the favourite icon is now, the favourite action would be shown inside the three-dots menu, so the three-dots icon should be shown even for encrypted folders, although only with a single action.

So, in the same way that file actions are currently filtered based on permissions and mime types, an option could be to filter all actions on encrypted folders except those marked as allowed-on-encrypted (or something like that). I could implement that if needed... although probably not in the next days, sorry :-S

@icewind1991

I can still enter an encrypted folder

If you should not enter an encrypted folder I would expect the server to prevent that. Otherwise, even if fixed in the web UI, you could simply replicate the current web UI behaviour "manually" and get access to the folder. Maybe the server does not prevent it because there is no proper encryption in place and it would not allow that on a real case (assuming that you just used the trick of setting encrypted to 1 in the filecache table, but I do not know if that is what you did :-) )?

In any case, what should be the expected behaviour? Not allowing to click on the folder? Showing a warning/error saying that it is encrypted and that it can not be accessed? Showing a dialog that ask for a decryption key to be able to enter?

Share icon should stay aligned when the dots aren't there

If the filtering I described above is implemented I think that the alignment should be fixed in a different pull request, as it could be seen as a general case of "When there are no actions to be shown in the file actions menu the file actions trigger should not be shown, and the in-line actions should be kept aligned" (although that would not happen anyway once the pull request to change the placement of the checkbox is merged, so... I would say that this has low priority).

schiessle · 2017-10-17T14:23:30Z

@icewind1991 did you enabled the end2end encryption app on the server? If it is enabled access to the folder should be blocked

The share icon shouldn't be there at all, at least it isn't for me, again the e2e app needs to be enabled

schiessle · 2017-10-17T14:34:12Z

@danxuliu thanks a lot for the js tests! All operations on the encrypted folders should be removed. At the moment just the option to "star" the folder is still there which is fine.

As said to @icewind1991, in order to block access to the folder you need to have the "end to end encryption" app enabled. This contain a sabre dav plugin which blocks all the operations.

danxuliu · 2017-10-31T11:44:40Z

I have realized that there is no need to add another field to file actions to specify whether they are allowed on encrypted folders or not to filter the file actions by that field as I initially thought.

When installed in the server, the E2E encryption app blocks all the permissions in responses to PROPFIND requests, and file actions are currently filtered based on the permissions they need and the permissions that the user has on the file. For example, the download action needs read permissions, while the move action needs update permissions. Therefore, as encrypted files have no permissions (in the web UI) everything should simply work.

But it does not :-P The reason is that the JavaScript files client sets the read permission on all files, no matter the permissions returned by the server. Thus all the actions that need read permissions are enabled for encrypted files even it they should not.

Besides that, currently every action require a permission, even if they should not. For example the favorite action requires the read permission, but there is no need to be able to read the file to mark or unmark it as a favourite. That is probably due to the read permission being always on and the fact that some code that works with file permissions currently does not work as expected if the actions requires no permissions (as it would filter out the action even if it should not).

Finally, when selecting several files on the file list the delete action is hidden if any of the files can not be deleted. However, the Move or copy and the Download actions are always shown even if some of the selected files do not support that action.

With all of the above my proposed solution would be to fix the handling of permissions in the file list and then handle encrypted files just like any other file, as the available actions will be automatically adjusted based on the permissions of encrypted files. Does this sound sane?

schiessle · 2017-11-01T13:53:51Z

@danxuliu yes, the webdav plugin sets already the permission to '0' if you can make the javascript to keep this, this would probably work.

danxuliu · 2017-11-02T18:07:24Z

I have rewritten the commit history to squash the unit tests to their related commits, extract the fixing of whitespace errors to its own commit, remove the commit that hid the three dots menu (as the Favorite and Details action could be performed on encrypted folders), and add some missing details (setting the folder icon too on markFileAsShared).

This pull request now requires #7046 and #7047 to be merged first; this pull request should be rebased on master once that happens, as there are some minor conflicts.

Also note that there is still a bug in this code: when the Share tab is shown the file row loses part of its metadata, and if the Share tab is loaded again (if it is simply opened there is no problem) then the encrypted folder will appear as a regular folder. For reference, the problem happens when updating the file info model due to the sharesChanged event being triggered, but I have not found the cause yet :-(

schiessle · 2017-11-20T15:20:01Z

@danxuliu all dependencies are now merged, can you rebase this PR and take care of the merge conflicts? Thanks!

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

danxuliu · 2017-11-20T20:15:19Z

@danxuliu all dependencies are now merged, can you rebase this PR and take care of the merge conflicts? Thanks!

Done ;-)

I have also fixed the bug mentioned in my last comment; it was caused by elementToFile not setting the isEncrypted attribute.

When rebasing onto current master I dropped the commit that fixed the whitespace errors (they are now fixed in master) and split the commit to show the E2E folder icon in two, one to add the e2eencrypted attribute to the file list rows and another one just to show the E2E folder icon.

This pull request is now ready to be reviewed! :-)

schiessle · 2017-11-21T10:21:12Z

@danxuliu great. I will try it out! 😃

schiessle · 2017-11-21T11:09:38Z

tested it and it works really great! Thanks @danxuliu

@nickvergessen @icewind1991 @rullzer can you please review it? Thanks!

LukasReschke

🐘

schiessle added the 2. developing Work in progress label Sep 27, 2017

schiessle added this to the Nextcloud 13 milestone Sep 27, 2017

schiessle added the feature: encryption (server-side) label Sep 27, 2017

schiessle mentioned this pull request Sep 28, 2017

Overview: Initial development of the server part of end-to-end encryption nextcloud/end_to_end_encryption#2

Closed

14 tasks

schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 29, 2017

schiessle requested review from rullzer, icewind1991 and mjobst-necls September 29, 2017 15:30

schiessle force-pushed the handle-encryption-state-in-web-interface branch from 3f5e1aa to 81b9363 Compare September 29, 2017 15:35

schiessle requested review from blizzz and danxuliu October 6, 2017 09:15

danxuliu reviewed Oct 8, 2017

View reviewed changes

schiessle force-pushed the handle-encryption-state-in-web-interface branch from 81b9363 to f7fa1cd Compare October 10, 2017 09:39

danxuliu added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 14, 2017

schiessle force-pushed the handle-encryption-state-in-web-interface branch from d55a7a7 to efa876b Compare October 26, 2017 11:55

This was referenced Nov 2, 2017

Hide summary file actions when a selected file does not match #7046

Merged

Add support for files with no permissions #7047

Merged

danxuliu force-pushed the handle-encryption-state-in-web-interface branch from efa876b to 57c85eb Compare November 2, 2017 17:47

schiessle and others added 4 commits November 20, 2017 18:24

check for encryption state on propfind

fc456be

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

adjust permissions in web view for encrypted folders

670ac48

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Add data attribute to file list rows telling if the file is encrypted

37d8d3d

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

show e2e folder icon on encrypted folders

7bc28f1

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

danxuliu force-pushed the handle-encryption-state-in-web-interface branch from 57c85eb to 7bc28f1 Compare November 20, 2017 20:03

danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 20, 2017

rullzer approved these changes Dec 6, 2017

View reviewed changes

LukasReschke approved these changes Dec 6, 2017

View reviewed changes

tobiasKaminsky merged commit 430f60d into master Dec 6, 2017

tobiasKaminsky deleted the handle-encryption-state-in-web-interface branch December 6, 2017 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle encryption state in web interface #6670

Handle encryption state in web interface #6670

schiessle commented Sep 27, 2017 •

edited

Loading

schiessle commented Sep 27, 2017

codecov bot commented Sep 29, 2017 •

edited

Loading

danxuliu left a comment

danxuliu Oct 8, 2017

danxuliu Oct 8, 2017

schiessle Oct 10, 2017

danxuliu Oct 8, 2017

schiessle commented Oct 10, 2017

icewind1991 commented Oct 12, 2017

danxuliu commented Oct 14, 2017

schiessle commented Oct 17, 2017 •

edited

Loading

schiessle commented Oct 17, 2017 •

edited

Loading

danxuliu commented Oct 31, 2017

schiessle commented Nov 1, 2017

danxuliu commented Nov 2, 2017

schiessle commented Nov 20, 2017

danxuliu commented Nov 20, 2017

schiessle commented Nov 21, 2017

schiessle commented Nov 21, 2017

LukasReschke left a comment

Handle encryption state in web interface #6670

Handle encryption state in web interface #6670

Conversation

schiessle commented Sep 27, 2017 • edited Loading

schiessle commented Sep 27, 2017

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

danxuliu left a comment

Choose a reason for hiding this comment

danxuliu Oct 8, 2017

Choose a reason for hiding this comment

danxuliu Oct 8, 2017

Choose a reason for hiding this comment

schiessle Oct 10, 2017

Choose a reason for hiding this comment

danxuliu Oct 8, 2017

Choose a reason for hiding this comment

schiessle commented Oct 10, 2017

icewind1991 commented Oct 12, 2017

danxuliu commented Oct 14, 2017

schiessle commented Oct 17, 2017 • edited Loading

schiessle commented Oct 17, 2017 • edited Loading

danxuliu commented Oct 31, 2017

schiessle commented Nov 1, 2017

danxuliu commented Nov 2, 2017

schiessle commented Nov 20, 2017

danxuliu commented Nov 20, 2017

schiessle commented Nov 21, 2017

schiessle commented Nov 21, 2017

LukasReschke left a comment

Choose a reason for hiding this comment

schiessle commented Sep 27, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading

schiessle commented Oct 17, 2017 •

edited

Loading

schiessle commented Oct 17, 2017 •

edited

Loading