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

fix: Pass the mountpoint target user to storages without owner #44294

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Mar 19, 2024

Storages that do not have a dedicated owner (e.g. groupfolders, external
storages) currently always assume the current session user as the owner.
This leads to several issues when there is no user session but a node is
obtained through a user folder.

In order to have the correct user available we need to pass the user
that is used to setup a mountpoint along to the storage layer as we
generally assume that an owner is available for those.

Steps to see the issue:

Text

  • Setup a local global external storage
  • As a user with access to it
    • Have text enabled
    • Create a share link with edit permissions
    • Access the share link in a private browser window
    • Try to save the file which fails because file hooks like the version backend or files metadata extractors try to operate with the storage owner

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Is it not possible to set the user in the session?
I’m not sure I understand how this works not having a session, apart from public share.

Other places like https://github.com/nextcloud/server/blob/master/apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php#L56 (Just know this one because I had a conflict about this line in another PR) expects to have a user in session, won’t that cause troubles?

@juliusknorr
Copy link
Member Author

Yeah, when debugging further I noticed that setting the session user temporarily would be more sensible for the onlyoffice bug. I came up with #44295 for that. For public share links we though still need this as the node does not hold information about the share (or share owner) which is used to construct the user folder

$userFolder = $this->rootFolder->getUserFolder($this->sharedBy);
but is lost afterwards before this PR.

@come-nc
Copy link
Contributor

come-nc commented Mar 19, 2024

Should something like https://github.com/nextcloud/server/blob/master/apps/dav/lib/Storage/PublicOwnerWrapper.php be used for public links?

@juliusknorr
Copy link
Member Author

That's a great hint, wasn't aware of that wrapper, let me see if that can be adopted

@juliusknorr
Copy link
Member Author

Is it not possible to set the user in the session?
I’m not sure I understand how this works not having a session, apart from public share.

Collabora requests don't have a session itself. Using the current session user might lead to wrong assumptions further down, the mountpoint target user is the more correct one. The larger problem is that the storage API doesn't really cover the use case of having a storage that has "shared" ownership, but at least the mount owner is more correct then just always assuming the logged in user. The nodes API is flexible so that a user folder for a different user can be requested where the current session user would actually be incorrect.

Should something like https://github.com/nextcloud/server/blob/master/apps/dav/lib/Storage/PublicOwnerWrapper.php be used for public links?

I checked this but think setting the owner on the storage itself seems cleaner to me as it is passed along when building the storage class instance.

* This can be used for storages that do not have a dedicated owner, where we want to
* pass the user that we setup the mountpoint for along to the storage layer
*
* @param string|null $user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param string|null $user
* @param string|null $user Owner user id

Storages that do not have a dedicated owner (e.g. groupfolders, external
storages) currently always assume the current session user as the owner.
This leads to several issues when there is no user session but a node is
obtained through a user folder.

In order to have the correct user available we need to pass the user
that is used to setup a mountpoint along to the storage layer as we
generally assume that an owner is available for those.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the fix/storage-mount-owner branch from 41b0fef to 4910e7e Compare April 9, 2024 08:04
@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 9, 2024
@juliusknorr juliusknorr merged commit e0fcf6b into master Apr 9, 2024
160 checks passed
@juliusknorr juliusknorr deleted the fix/storage-mount-owner branch April 9, 2024 11:18
* @return void
* @since 29.0.0
*/
public function setOwner(?string $user): void;
Copy link
Member

Choose a reason for hiding this comment

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

juliusknorr added a commit to nextcloud/documentation that referenced this pull request Jul 19, 2024
nextcloud/server#44294

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

/backport to stable29

@juliusknorr
Copy link
Member Author

/backport to stable28

@juliusknorr
Copy link
Member Author

/backport to stable28

Copy link

backportbot bot commented Jul 22, 2024

The backport to stable28 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable28
git pull origin stable28

# Create the new backport branch
git checkout -b backport/44294/stable28

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 4910e7e2

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/44294/stable28

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/44294/stable28."}


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants