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

[feature request] allow user/API to set file_target for a share #12548

Closed
landryb opened this issue Nov 20, 2018 · 18 comments
Closed

[feature request] allow user/API to set file_target for a share #12548

landryb opened this issue Nov 20, 2018 · 18 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement feature: dav feature: sharing

Comments

@landryb
Copy link

landryb commented Nov 20, 2018

Right now, the file_target field in the oc_share table is more or less internal, only updated (afaict after looking at the code) by changing mount points for shared mounts. I havent found many calls to setTarget and updateFileTarget is internal to files_sharing app.

It would be interesting to have a way to control this value via the API (as a user via the UI, it would be an advanced feature) via a new sharePath parameter as it would allow the admin/person sharing to 'place' the shared file in a specific subdir in the remote user hierarchy. Right now i can do it by setting the value directly in the database but that feels hackish.

Of course that implies access controls (ie can you 'write' in the given remote subdir, etc..) but as an admin who has to share billions of files to ~1k users (different subset of files for each users of course, but lots of overlap, ie some files will be shared to 10+ users, and i need to do it programatically), i plan to (abuse? misuse?) use this "feature" to "present" them a hierarchy i would control in a group folder shared with them. Avoiding thousands of files at the root of their homedir...

So far i've tested the following scenarios, with a file shared from user A to user B:

  • setting file_target to /path/file, /path being a dir local to the B user account -> file is displayed to B under /path as expected
  • setting file_target to /groupfolder/file, /groupfolder being a group folder seen as an external mount (created with the groupfolder API) -> file is displayed to B under /groupfolder as expected, but not to user C (who also has access to the shared groupfolder) - would need to set an extra share for each user in that group ?
  • setting file_target to /nonexistent/file, file is displayed to B at the root of his homedir, and file_target is fixed in the db (ie comes back to /file), so this 'broken' case (ie trying to share a file in a wrong/nonexistent dir) is gracefully handled

same tests when sharing a folder produced similar results.

@nextcloud-bot nextcloud-bot added enhancement feature: sharing stale Ticket or PR with no recent activity and removed stale Ticket or PR with no recent activity labels Nov 20, 2018
@nextcloud-bot
Copy link
Member

GitMate.io thinks possibly related issues are #4159 (Shared API ), #1762 ([Feature] send a file to user (not sharing)), #3617 (Setting Favorite API), #10230 (Feature/shares overview), and #10132 (Allow restoring of deleted user shares).

@nextcloud-bot nextcloud-bot added feature: dav stale Ticket or PR with no recent activity and removed stale Ticket or PR with no recent activity labels Nov 20, 2018
@landryb
Copy link
Author

landryb commented Nov 20, 2018

It also works if i set file_target in a group share, ie file foo shared with group G1 (which has access to /groupfolder), file_target set it to /groupfolder/foo => it is presented to all users of group G1 under shared folder /groupfolder/

@JLueke
Copy link

JLueke commented Nov 23, 2018

It works with those changes for share creation via API:
mhq-services@a9ae3d9

Still requires a fix for the indentation and tests, but I have troubles with the setup for the testing enviroment:

phpunit --configuration phpunit-autotest.xml

return following Error:

PHP Fatal error: Uncaught Exception: Not installed in /var/data/lib/base.php:277

If I set "installed" to true in the config.php, it fails for the missing Database tables.

@landryb
Copy link
Author

landryb commented Nov 28, 2018

It works with those changes for share creation via API:
mhq-services@a9ae3d9

Nice ! how are you handling the case where subdir doesnt exist in the remote user homedir ? the shared file/dir ends up in the homedir root i guess ?

@JLueke
Copy link

JLueke commented Nov 29, 2018

if the subdir doesn't exist, it will be moved automatically (already in implemented in Nextcloud ) to the root or the configured "'share_folder" (can be set in config.php for default target for all shares).

@landryb
Copy link
Author

landryb commented Nov 30, 2018

Great ! Will make sure to test it as i plan to extensively use that feature.

@DrMurx
Copy link

DrMurx commented Apr 4, 2019

@landryb How did your tests go? Did you stumble over any issues?

@landryb
Copy link
Author

landryb commented Apr 4, 2019

sadly i've been unable to test it yet :/

@JLueke
Copy link

JLueke commented Apr 4, 2019

Well, we have it still running without any issues.

@JLueke
Copy link

JLueke commented Apr 4, 2019

It also works if i set file_target in a group share, ie file foo shared with group G1 (which has access to /groupfolder), file_target set it to /groupfolder/foo => it is presented to all users of group G1 under shared folder /groupfolder/

But this one doesn't work as far as I know, since Nextcloud doesn't show shares in other shares. Since we generate the shares most of the time via the API, we always try to optimize the displayed shares for the user:

If he has already access to the parent folder with the same rights, then we ignore/remove the share for the subfolder. If the parentfolder share is removed we recover the share for the subfolder.

@landryb
Copy link
Author

landryb commented Jun 3, 2019

Fwiw, i've (finally!) tested mhq-services@a9ae3d9 locally on stable15 branch and it works fine, would be nice to get that into the next release as its an api change..

@landryb
Copy link
Author

landryb commented Jun 3, 2019

After testing it a bit more, there's some codepaths where the modified file_target is lost, ie a share to a group will have an entry with the proper share type, but some sub-entries (with type 2) are created for each group user, referencing the 'parent' groupshare via the parent field, but those entries lose the full path and only have the share name as file_target.

@landryb
Copy link
Author

landryb commented Jun 3, 2019

Digging more in the code, https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/SharedMount.php#L110 is where the new sub-entries get the 'wrong' file target without subdirs.

But i agree maybe i'm asking too much, as i want the shares to appear in subdirectories, and that doesnt seem supported at all in the current code layout, sadly.

@landryb
Copy link
Author

landryb commented Jun 3, 2019

I think i understand whats' wrong.. and it's a bit complicated. I'm trying to set file_target to point at a subdir of another existing groupshare, and verifyMountPoint fails to check that '/share/foo/bar' exists as the dir doesnt seem to exist (https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/SharedMount.php#L97 returns false) from the pov of the recipientView object - which is weird, as it's shown in the recipient folder list...

@JLueke
Copy link

JLueke commented Jun 3, 2019

After testing it a bit more, there's some codepaths where the modified file_target is lost, ie a share to a group will have an entry with the proper share type, but some sub-entries (with type 2) are created for each group user, referencing the 'parent' groupshare via the parent field, but those entries lose the full path and only have the share name as file_target.

Right, there was a problem, totally forgot about it. But yeah, thats the reason why we don't use group shares directly. Instead we create single shares for each member of the group.

@landryb
Copy link
Author

landryb commented Jun 4, 2019

My (horrible) workaround so far is to hack VerifyMountPoint and avoid calling $this->updateFileTarget if the share target is a path with subdirs, then directly return the share target instead of $newMountPoint.

                if ($newMountPoint !== $share->getTarget()) {
-                       $this->updateFileTarget($newMountPoint, $share);
+                       if (strpos($share->getTarget(),'/',1)) {
+                               return $share->getTarget();
+                       } else {
+                               $this->updateFileTarget($newMountPoint, $share);
+                       }
                }
                return $newMountPoint;

but that feels soooo fragile..

landryb added a commit to landryb/server that referenced this issue Jul 11, 2019
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue Jan 30, 2020
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue Mar 16, 2020
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue May 12, 2020
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
@skjnldsv skjnldsv added the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Aug 20, 2020
landryb added a commit to landryb/server that referenced this issue Oct 8, 2020
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
@szaimen
Copy link
Contributor

szaimen commented Jun 9, 2021

Please use the groupfolder app for this. Thanks!
https://apps.nextcloud.com/apps/groupfolders

@szaimen szaimen closed this as completed Jun 9, 2021
@landryb
Copy link
Author

landryb commented Jun 9, 2021

we .. already use groupfolders, but that cant scale for the usecase we have, and that isnt the same feature as setting the file_target. But i'll keep on carrying my local patch, whatever.

landryb added a commit to landryb/server that referenced this issue Jan 24, 2022
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue Feb 16, 2022
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue Mar 24, 2022
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
landryb added a commit to landryb/server that referenced this issue Mar 5, 2024
…th a / (nextcloud#12548)

needed by craig/autoshare#15 when we explicitely set file_target
to present file in a subdir.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap enhancement feature: dav feature: sharing
Projects
None yet
Development

No branches or pull requests

6 participants