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

do not explode when getting permissions from a FailedStorage #11383

Merged
merged 1 commit into from
Sep 26, 2018

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Sep 26, 2018

for instance if a user of an external user backend is not available
currently, the whole Files UI would be frozen.

for instance if a user of an external user backend is not available
currently, the whole Files UI would be frozen.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/files-unavailable-deleted-user branch from c806a40 to c526cdf Compare September 26, 2018 08:48
@MorrisJobke
Copy link
Member

CI failure unrelated.

Beside that: should we somehow clean this up? Or is this already done in the background job that does the cleanup of orphaned shares?

@MorrisJobke MorrisJobke merged commit 89fdd08 into master Sep 26, 2018
@MorrisJobke MorrisJobke deleted the fix/noid/files-unavailable-deleted-user branch September 26, 2018 10:18
@blizzz
Copy link
Member Author

blizzz commented Sep 26, 2018

Beside that: should we somehow clean this up? Or is this already done in the background job that does the cleanup of orphaned shares?

I don't think it is done. Normally, when properly deleting a user, the entries also vanish. It is also an edge case here, because it comes with another condition that a username was removed (LDAP → clear mappings) and not repopulated. It's rather a consideration to change the behaviour there and also run the whole standard deletion process. As a fundamental change that's nothing to backport.

It's still OK to catch this to ensure the Files view is working. Still, should not be an apology for strange behaving user backends or admins.

@blizzz
Copy link
Member Author

blizzz commented Sep 27, 2018

For the record, we have had just another case that went into the same trap, but was not related to missing users. Worse, it did not generate any log output. But it is fixed with that check as well.

This makes me wonder whether the original change actually does what is is supposed to do @rullzer #11041 in apps/files_sharing/lib/Cache.php

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

Successfully merging this pull request may close these issues.

3 participants