Check if the owner of a share exists #7348

rullzer · 2017-11-30T11:09:50Z

If the owner does not exist we should behave properly.

We can't delete the share at this point as a back end might just be temp
offline.

To test:

Setup NC with LDAP
Share a folder test from user1 to user2
Delete user1 from LDAP

DO NOT NOTIFY NEXTCLOUD

Login as user2
navigate to test

Before:

If you enter test you will not see any content and warnings will be thrown
Weird sync behavior

Now:

Warning that the storage is temporaly unavailable
Sync client skips the folder because it is not available

blizzz

tested

codecov · 2017-11-30T12:14:04Z

Codecov Report

Merging #7348 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master    #7348      +/-   ##
============================================
- Coverage     50.91%   50.91%   -0.01%     
- Complexity    24699    24701       +2     
============================================
  Files          1586     1586              
  Lines         94116    94118       +2     
  Branches       1361     1361              
============================================
+ Hits          47920    47921       +1     
- Misses        46196    46197       +1

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Connector/Sabre/ObjectTree.php	`56.97% <50%> (-0.17%)`	`34 <0> (+2)`

schiessle · 2017-11-30T14:54:05Z

I remember that I discussed this issue with @blizzz a few days/weeks ago because I saw a similar issue on another instance. If I remember correctly our conclusion was that we should still show the folder but in a "temporarily not available" state, same as we do for external storage. If we just don't show it at all this will trigger the sync client to delete it and if the user back-end comes back the sync client will have to download everything again. Which can be really painful for large shared folders.

MorrisJobke · 2017-11-30T15:37:40Z

I remember that I discussed this issue with @blizzz a few days/weeks ago because I saw a similar issue on another instance. If I remember correctly our conclusion was that we should still show the folder but in a "temporarily not available" state, same as we do for external storage. If we just don't show it at all this will trigger the sync client to delete it and if the user back-end comes back the sync client will have to download everything again. Which can be really painful for large shared folders.

AFAIK this is done via a StorageNotAvailableException right, @icewind1991 ?

icewind1991 · 2017-11-30T15:55:43Z

AFAIK this is done via a StorageNotAvailableException right, @icewind1991 ?

Yes.

rullzer · 2017-11-30T18:30:02Z

But where to throw it :P??

rullzer · 2017-11-30T20:37:51Z

@icewind1991 please see my two commits. I think they make it somewhat more sane. As the FailedStorage was not handle properly by the DAV app. Now it behaves better IMO.

@schiessle The making red of the storage is actually handled by the files_external javascript.

rullzer · 2017-11-30T20:39:37Z

Ok, so my initial commit did not make much sense. As LDAP will also find out that the user is deleted. And still return it.

Now the share will still show. But if you try to enter it you get a 'storage not available' notification. This is probably saner.

brunt82 · 2017-12-01T08:31:07Z

One short question: how can the client request the server to get the current folder structure, if it is unable to log in, because the user backend is not available?

Do you have any use case where one user is able to login but another user not because it is temporarly disabled? Can the user logged in by using a redis-session although the LDAP server is unavailable?

blizzz · 2017-12-01T09:38:59Z

@brunt82 no, LDAP is available. This case is about that a user disappeared from LDAP who was sharing a file or folder to another one, who logged in. And sees the shares, but has no way to access them and experiences confusing behaviour.

code changed

brunt82 · 2017-12-01T10:00:10Z

Yes, I know. But you're discussing about an use case, which is unimportant (or maybe does not exist <- which was my question): it is difficult for me to imagine an use case where a LDAP user object is not available by a mistake.

Let us assume the LDAP is not available. In this case all users are not able to login. So the clients cannot get the folder structure. But my question was: Does the redis-sessions still work, although the LDAP is not available? If yes, the clients may delete the folders, which are hidden by the server.

A LDAP user will not be deleted by a mistake. And as I understood the issue, it is differed between an user object which was not found and a disabled object. So what other use cases do you know where an user object is not available for some time?

rullzer · 2017-12-01T12:22:17Z

Tests fixed

icewind1991 · 2017-12-01T15:57:41Z

apps/files_sharing/lib/SharedStorage.php

 	 * @return Cache
 	 */
 	public function getCache($path = '', $storage = null) {
+		$this->init();


this causes a major performance hit.

Shared storage is left uninitialized for cache only work for a reason

This is already the case.

https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/SharedStorage.php#L369
https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/Cache.php#L72
https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/SharedStorage.php#L458
https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/SharedStorage.php#L459

This just moves it earlier so the the cache of the storage is also set to FailedCache.

Will need to fix that then...

Fair enough then let me drop this commit from here. As the first one is the most important one anyway I think

We have to double check. Since getting the info of the root returns a generic entry. But actually the stroage is not available. Else we get very weird sync and web behavior. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Commit dropped

LukasReschke

🐘

rullzer added 3. to review Waiting for reviews backport-request bug feature: ldap feature: sharing labels Nov 30, 2017

rullzer added this to the Nextcloud 13 milestone Nov 30, 2017

rullzer requested review from LukasReschke, MorrisJobke, blizzz, icewind1991, nickvergessen and schiessle November 30, 2017 11:09

blizzz previously approved these changes Nov 30, 2017

View reviewed changes

rullzer force-pushed the filterout_shares_of_nonexisting_users branch from 35a9e9c to 1fefe18 Compare November 30, 2017 20:38

rullzer force-pushed the filterout_shares_of_nonexisting_users branch from 0903076 to 7deaf59 Compare December 1, 2017 11:40

icewind1991 previously requested changes Dec 1, 2017

View reviewed changes

rullzer added 2 commits December 4, 2017 15:23

A failed storage is a not available storage

c8a29ec

We have to double check. Since getting the info of the root returns a generic entry. But actually the stroage is not available. Else we get very weird sync and web behavior. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix tests

d2fe30d

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the filterout_shares_of_nonexisting_users branch from 7deaf59 to d2fe30d Compare December 4, 2017 14:23

icewind1991 approved these changes Dec 5, 2017

View reviewed changes

LukasReschke approved these changes Dec 6, 2017

View reviewed changes

rullzer merged commit 1287da8 into master Dec 6, 2017

rullzer deleted the filterout_shares_of_nonexisting_users branch December 6, 2017 08:14

rullzer mentioned this pull request Dec 6, 2017

[stable12] Check if owner of share exists #7407

Merged

MorrisJobke removed the backport-request label Dec 6, 2017

brunt82 mentioned this pull request Jul 30, 2018

Unable to unshare link if ldap user does not exist anymore #10446

Closed

Uh oh!

Check if the owner of a share exists #7348

Check if the owner of a share exists #7348

Uh oh!

Conversation

rullzer commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schiessle commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MorrisJobke commented Nov 30, 2017

Uh oh!

icewind1991 commented Nov 30, 2017

Uh oh!

rullzer commented Nov 30, 2017

Uh oh!

rullzer commented Nov 30, 2017

Uh oh!

rullzer commented Nov 30, 2017

Uh oh!

brunt82 commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blizzz commented Dec 1, 2017

Uh oh!

brunt82 commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Dec 1, 2017

Uh oh!

icewind1991 Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

rullzer Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

icewind1991 Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

icewind1991 Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

rullzer Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

LukasReschke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

rullzer commented Nov 30, 2017 •

edited

Loading

codecov bot commented Nov 30, 2017 •

edited

Loading

schiessle commented Nov 30, 2017 •

edited

Loading

brunt82 commented Dec 1, 2017 •

edited

Loading

brunt82 commented Dec 1, 2017 •

edited

Loading