-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix: update re-share if shared-by user has been revoked #43025
base: master
Are you sure you want to change the base?
fix: update re-share if shared-by user has been revoked #43025
Conversation
Another effected case by this issue:
|
c298bf1
to
0828727
Compare
@artonge I've updated as you requested, please check again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comments to clarify what each if
group does?
I am also wondering if we could find this shares with the following query:
SELECT
f.fileid,
f.path,
f.storage,
s.id as share_id,
s.uid_owner as share_owner,
s.uid_initiator as share_initiator,
s.share_with as share_recipient
FROM
oc_filecache f
JOIN oc_share s ON f.fileid = s.file_source
AND s.uid_initiator NOT IN (
SELECT
user_id
FROM
oc_mounts m
WHERE
f.storage = m.storage_id
)
If so, then we could have a background job to remove them every hour or so like https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/DeleteOrphanedSharesJob.php, which might be easier than the current solution.
@artonge It only works if the shared-by user refreshes his files list after his share has revoked. So, I don't think this query would do the job. |
007d650
to
df7160f
Compare
df7160f
to
73fb85b
Compare
7e64905
to
cf9b02a
Compare
0d42361
to
032be69
Compare
@come-nc Could you please review again? All checks have passed now. |
|
||
public function __construct( | ||
IDBConnection $connection, | ||
IRootFolder $rootFolder | ||
IRootFolder $rootFolder, | ||
IUserMountCache $userMountCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IUserMountCache $userMountCache | |
IUserMountCache $userMountCache, |
lib/private/Share20/Manager.php
Outdated
|
||
foreach ($sharesInFolder as $shares) { | ||
foreach ($shares as $child) { | ||
$this->deleteShare($child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For subfiles you did not check that the user still has access to it, I think?
lib/private/Share20/Manager.php
Outdated
// If the user has another shares, we don't delete the shares by this user | ||
if ($share->getShareType() === IShare::TYPE_USER) { | ||
$groupShares = $this->getSharedWith($share->getSharedWith(), IShare::TYPE_GROUP, $node, -1, 0); | ||
|
||
if (count($groupShares) !== 0) { | ||
return; | ||
} | ||
|
||
// Check shares of parent folders | ||
try { | ||
$parentNode = $node->getParent(); | ||
while ($parentNode) { | ||
$groupShares = $this->getSharedWith($share->getSharedWith(), IShare::TYPE_GROUP, $parentNode, -1, 0); | ||
$userShares = $this->getSharedWith($share->getSharedWith(), IShare::TYPE_USER, $parentNode, -1, 0); | ||
|
||
if (count($groupShares) !== 0 || count($userShares) !== 0) { | ||
return; | ||
} | ||
|
||
$parentNode = $parentNode->getParent(); | ||
} | ||
} catch (NotFoundException) { | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing all this to check if the user still has access to the file or not, does it not make more sense to directly check if the user has access to the file? We must have some tooling for that, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@come-nc I cannot find another way to check this logic. Could you provide some suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get the user folder of the user and use getFirstNodeById
would work I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note that this would only work after deleting the share)
@luka-nextcloud After talking with Robin, actually the ownership transfer is supposed to update the owner of the share so there should be no problem in the first place? |
@come-nc Does it mean that these changes are enough for this issue? |
No, the second diff looks like it will cause trouble by arbitrary changing sharedBy of shares. I lost track of what you are trying to fix, can you explain how to reproduce your problem? Why do you remove the exception handling removing invalid shares in OwnershipTransferService? |
|
UserC already loses access to Folder1 after step 3, right? |
No, UserC didn't lose access to Folder1 after step3. This is the issue we are trying to fix.
No error. |
@come-nc Reproduce steps: Solution: |
a03da0a
to
e1450ee
Compare
e1450ee
to
522cf26
Compare
lib/private/Share20/Manager.php
Outdated
foreach ($shares as $child) { | ||
if ($this->hasNodeAccess($user->getUID(), $child->getNode())) { | ||
continue; | ||
} | ||
$this->deleteShare($child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is generalCreateCheck method not directly run on $child
instead?
I do not understand the logic in hasNodeAccess, it searches for other shares of the same node but only directly to the user?
Also, the code is duplicated between user shares and group shares, please first compute an array of target users and then loop on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@come-nc Ok, I have checked again. The logic in hasNodeAccess
was not really correct. It missed some cases. So, I removed that function and updated approach for deleteReshare
. First collect related user ids, then collect the re-share by those users, finally do generalCreateCheck
on those re-share records.
Signed-off-by: Luka Trovic <luka@nextcloud.com>
522cf26
to
b23eafb
Compare
foreach ($userIds as $userId) { | ||
foreach ($shareTypes as $shareType) { | ||
$provider = $this->factory->getProviderForType($shareType); | ||
$shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you pass false
for $reshares
flag here but true
in the call to getSharesInFolder
below?
return null; | ||
} | ||
foreach ($mounts as $mount) { | ||
// Only the mount of owner has the internal path value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@icewind1991 Could you provide a suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check if $mount
is a HomeMountPoint
and then use $mount->getUser()
Summary
TODO
Checklist