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 a method to get all users with access to a file in OCP #38946

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jun 22, 2023

  • Resolves: #

Summary

In a lot of places we need the list of users having access to a file, this is especially often the case in activity providers in applications, to notify all users of an event.
Currently the implementation is done differently each time, this is an attempt to bring a method in OCP that allows to get the list of users having access to a file and under which path.
For comments for example, this fixes showing the activity event to users having access to the file through a groupfolder.

TODO

  • See if this is the right implementation
  • Use it in all places that needs this feature
  • Use it in applications where it makes sense, especially activity
  • See how groupfolder ACLs could be integrated in there to avoid returning users which actually do not have access to the file

Checklist

@come-nc come-nc added the 2. developing Work in progress label Jun 22, 2023
@come-nc come-nc self-assigned this Jun 22, 2023
lib/public/Files/Config/IUserMountCache.php Outdated Show resolved Hide resolved
*/
public function getNodesByUserForFileId(int $fileId): array {
$mounts = $this->getMountsForFileId($id);
$rootFolder = Server::get(IRootFolder::class);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can this be added to the DI or is there a problem with that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense to move this method to the RootFolder instead

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jun 22, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jun 23, 2023
@come-nc come-nc force-pushed the fix/refactor-user-access-to-file-list branch from 3ba5f1d to 287a5a8 Compare June 27, 2023 09:46
@come-nc
Copy link
Contributor Author

come-nc commented Jun 27, 2023

Screenshot_20230627_114800
Works nicely

@come-nc
Copy link
Contributor Author

come-nc commented Jun 27, 2023

This already takes into account groupfolder ACLs, because $userFolder->getById($fileId) will return an empty array in this case.
What’s missing is a check for this to avoid returning an empty array for a user (in this case the user should not appear).

Also, the check if (isset($result[$mount->getUser()->getUID()])) { is problematic because it means for a user it will return only the first mount giving access to a file, but it should actually merge them.

That said, in the usecase of activity, which is the only one I found so far, we do not need to merge, we only want to get one path for each file. So I’m tempted to change the method to actually return only one path for each user who has access, as a string. This will make calling code a lot clearer.
The only usecase which needs more info is info:file command, to my knowledge.

@come-nc
Copy link
Contributor Author

come-nc commented Jun 27, 2023

So in the end I splitted between getReadablePathByUserForFileId and getReadableNodesByUserForFileId.
The first one stops at first path found for each user, and is useful for activity listeners.
The second one is used by info:file command only for now, but I still think it’s nice to have it in OCP.

I would like to use getReadablePathByUserForFileId as well in https://github.com/nextcloud/activity/blob/master/lib/FilesHooks.php , but there IShareHelper::getPathsForAccessList is used, which returns paths for local users as well as remote users it seems, and I failed to figure out how to replace only the part for local users.
I failed to find the code filling $accessList['remotes'] so far.

But for comments and systemtags it works nicely with groupfolders now, even with ACLs. Let’s hope I did not break something else.

@come-nc
Copy link
Contributor Author

come-nc commented Jun 27, 2023

I failed to figure out how to replace only the part for local users. I failed to find the code filling $accessList['remotes'] so far.

It’s

public function getAccessList($nodes, $currentAccess) {

I missed it because I was searching for 'remotes', but in ShareHelper::getPathsForAccessList remote is turned into remotes actually.

(Thanks Robin for the pointer)

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the lib/public changes

@come-nc come-nc force-pushed the fix/refactor-user-access-to-file-list branch from 80e469c to a95769a Compare September 11, 2023 13:34
@come-nc
Copy link
Contributor Author

come-nc commented Sep 11, 2023

Rebased on master and fixed psalm errors

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall but I would like to ensure this is somewhat performant

}

$userFolder = $rootFolder->getUserFolder($uid);
$result[$uid] = array_merge($result[$uid], $userFolder->getById($fileId));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that there should be a more efficient way to do this here since we already know the mount.

Perhaps extracting some of the logic from RootFolder::getByIdInPath to create a RootFolder::getByIdInMounts or similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #40482 (comment) for some alternate discussion around the same problem

@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@come-nc come-nc modified the milestones: Nextcloud 29, Nextcloud 30 Mar 12, 2024
@marcelklehr
Copy link
Member

It seems that IUserMountCache#getMountsForFileId() does not include shares because they are not listed in oc_mounts. For shares we would need something like $shareManager->getAccessList in addition to the mounts. And for groupfolders we would need an additional piece altogether.

Something like this might work, but hopefully there's a less hacky way:

// Assuming you have the file ID
$fileId = 123; // replace with your actual file ID

// Get the root folder
$rootFolder = \OC::$server->getRootFolder();

// Get the file by ID
$file = $rootFolder->getById($fileId);

if ($file) {
    $filePath = $file->getPath();

    // Now we need to find the group folder ID associated with this path
    // Assuming the group folders are stored in a specific path, e.g., "/__groupfolders/<groupfolder_id>/..."
    preg_match('/\/__groupfolders\/(\d+)\//', $filePath, $matches);

    if (isset($matches[1])) {
        $groupFolderId = $matches[1];
    } else {
        throw new Exception('Group folder ID not found');
    }
} else {
    throw new Exception('File not found');
}

@icewind1991
Copy link
Member

It seems that IUserMountCache#getMountsForFileId() does not include shares because they are not listed in oc_mounts

Shares should be listen in oc_mounts (once the recipient of the share has setup their filesystem)

@marcelklehr
Copy link
Member

Aha! That explains a lot. I also used the wrong array concatenation method (:see_no_evil: ) so, groupfolders are afterall taken care of by this method, so: sorry for the noise, all good.

This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@marcelklehr
Copy link
Member

Sad times.

@skjnldsv
Copy link
Member

@marcelklehr if womeone wanna take over, or OP wanna work again on it, they can still re-open :)

@skjnldsv skjnldsv deleted the fix/refactor-user-access-to-file-list branch August 16, 2024 07:19
@come-nc
Copy link
Contributor Author

come-nc commented Sep 14, 2024

Reopening because we need to figure this out at some point, it’s causing trouble for groupfolders. The question is whether we can do that without making the experience worse in other places.

@come-nc come-nc restored the fix/refactor-user-access-to-file-list branch September 14, 2024 11:35
@come-nc come-nc reopened this Sep 14, 2024
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…erForFileId

Optimises further and avoid duplicate code for all activity listeners

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/refactor-user-access-to-file-list branch from a95769a to c1c40ff Compare September 14, 2024 11:41
@come-nc
Copy link
Contributor Author

come-nc commented Sep 14, 2024

Rebased

@skjnldsv skjnldsv requested a review from icewind1991 November 15, 2024 13:12
@skjnldsv skjnldsv marked this pull request as draft November 15, 2024 13:12
@marcelklehr
Copy link
Member

Since there is no other method than this currently, and we already use it in some apps, I vote to get this in, even if it's not super performant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants