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

Add support for files with no permissions #7047

Merged
merged 6 commits into from
Nov 20, 2017

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Nov 2, 2017

Until now it was safe to assume that every file was readable by its owner, but with the introduction of end to end encryption that is no longer the case. Therefore, the server now must explicitly return whether a file is readable or not, and the web UI has to be prepared to handle files with no permissions (which is simply a matter of fixing some code that assumed that the file would always have at least the read permission).

This is a requisite for proper handling of encrypted folders in #6670.

@danxuliu danxuliu added the 2. developing Work in progress label Nov 2, 2017
@danxuliu danxuliu added this to the Nextcloud 13 milestone Nov 2, 2017
Until now it was safe to assume that every file was readable by its
owner, so there was no need to return whether the file was readable or
not. However, with the introduction of end to end encryption that is no
longer the case, and it is now necessary to explicitly provide that
information.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Now a file gets its directory permissions only if it contained no
permissions (they were undefined or null), but not if its permissions
were set to "NONE".

Besides that, now file actions that do not require any permission on the
file to be performed can be used on files that have no permissions.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The Details and the Favorite actions do not require any permission on
the files to be performed.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The first parameter of "apply" must be the object to act as "this", and
the Promise callback gets the parameters provided in the "resolve".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Now that these tests are executed they are revealed to be partially
obsolete; they were fixed to match the current parsing behaviour.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Now that the permissions returned by the server specify whether a file
is readable or not the frontend no longer needs to assume that every
file is readable.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the add-support-for-files-with-no-permissions branch from ddb7217 to 3e844d3 Compare November 2, 2017 18:37
@codecov
Copy link

codecov bot commented Nov 2, 2017

Codecov Report

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

@@             Coverage Diff              @@
##             master    #7047      +/-   ##
============================================
+ Coverage     50.61%   50.62%   +<.01%     
- Complexity    24297    24298       +1     
============================================
  Files          1577     1577              
  Lines         92922    92926       +4     
  Branches       1359     1359              
============================================
+ Hits          47036    47045       +9     
+ Misses        45886    45881       -5
Impacted Files Coverage Δ Complexity Δ
core/js/js.js 61.83% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/Connector/Sabre/Node.php 71.55% <100%> (+0.53%) 43 <0> (+1) ⬆️
core/js/files/client.js 84.52% <100%> (+1.65%) 0 <0> (ø) ⬇️

@danxuliu danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 2, 2017
@LukasReschke
Copy link
Member

I'd love to have some thoughts of @icewind1991 or/and @rullzer on this. Is this the best approach? Or should we have a dedicated flag for the E2E? :)

@rullzer
Copy link
Member

rullzer commented Nov 7, 2017

Mmmm I'm not sure this is right. Because the file is still readable by the owner. It is just encrypted.

If I'm not mistaken the folder that is encrypted has the encrypted flag set. @schiessle

So the js should check for that...

@danxuliu
Copy link
Member Author

danxuliu commented Nov 8, 2017

Mmmm I'm not sure this is right. Because the file is still readable by the owner. It is just encrypted.

It is not readable, at least not from the point of view of the web UI (or any other insecure client). When installed in the server, the E2E encryption app blocks all the permissions in responses to PROPFIND requests, no matter the actual permissions of the file. So if an encrypted file can not be updated, deleted, shared... it should not be seen as readable either.

But encryption aside this pull request simply makes the readable permission more consistent with the other permissions; the other permissions are explicitly returned by the server if they are set for a file, but readable was never returned and the web UI simply assumed that it was always set. Now the server explicitly returns the readable permission if it is set for a file and the web UI honours the returned permissions.

If I'm not mistaken the folder that is encrypted has the encrypted flag set.

So the js should check for that...

The reason to use the permissions instead of the encrypted flag was to avoid introducing specific cases and to use the general code instead (after the needed fixes, I mean ;-) ).

@rullzer
Copy link
Member

rullzer commented Nov 9, 2017

Ok so if the app actually sets the permissions to non readable for files. Then yes lets just properly display this.

@blizzz
Copy link
Member

blizzz commented Nov 10, 2017

@rullzer is this a +1?

@schiessle
Copy link
Member

@rullzer can we merge this?

As explained by @danxuliu correctly. The end-to-end encryption app has a SabraDAV plugin which removes the read permission for all clients which don't support e2e encryption.

@schiessle schiessle merged commit f347e2e into master Nov 20, 2017
@schiessle schiessle deleted the add-support-for-files-with-no-permissions branch November 20, 2017 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants