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 paths returned by \OC\Files\Storage\Shared hooks #23648

Closed
wants to merge 1 commit into from

Conversation

oparoz
Copy link
Contributor

@oparoz oparoz commented Mar 30, 2016

Fixes #23620

All targets in hooks should look like this:

'target' => $this->getMountPoint() . '/' . $path

  • Add a test since this was missed...

@rullzer @PVince81

@oparoz oparoz self-assigned this Mar 30, 2016
@oparoz oparoz added this to the 9.1-current milestone Mar 30, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @schiesbn, @icewind1991 and @MTGap to be potential reviewers

@PVince81
Copy link
Contributor

also @nickvergessen @icewind1991 @rullzer

@PVince81
Copy link
Contributor

Just to be sure, I suggest changing it to ltrim($path, '/') . $path just in case.

@PVince81
Copy link
Contributor

@icewind1991 @georgehrke what do you think ?

@icewind1991
Copy link
Contributor

I expect the mountpoint to already have a leading slash

@oparoz
Copy link
Contributor Author

oparoz commented May 30, 2016

I would except the same, but it wasn't the case back in April and unless it's been fixed in all the Mountpoint classes in the mean time, it should still be a problem.

@oparoz
Copy link
Contributor Author

oparoz commented May 30, 2016

Maybe this PR would fix things: #24564

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2016

Steps to reproduce on stable8.2:

  1. Create two users "admin" and "user1"
  2. Login as "admin"
  3. Create a folder "test"
  4. Share "test" with "user1"
  5. Login as "user1"
  6. Create a file "test/test.txt" with the text editor and put some contents
  7. Close editor
  8. Set a breakpoint here https://github.com/owncloud/core/blob/v8.2.2/apps/files_sharing/lib/sharedstorage.php#L294
  9. Reopen the file "test.txt" with the text editor
  10. Evaluate $info.

My result on stable8.2:

$info                            = (array[2]);
  $info['target']                = (string[13]) '/testtest.txt';
  $info['source']                = (string[26]) '/admin/files/test/test.txt';

Indeed there's a problem there.

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2016

@icewind1991 said a "leading slash". But this PR is about a trailing slash in the mount point.

I think this PR is acceptable. Will double check stable9 and master.

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2016

Same issue on stable9.

On master/9.1 the code has changed a lot, and it looks like there is no more "file_get_contents" or "file_put_contents" hooks any more for the shared storage ?! Something to investigate separately, @icewind1991 expect some new tickets soon.

@oparoz I'll close your PR. Can you send PRs only to stable9 and stable8.2 ?

@PVince81
Copy link
Contributor

PVince81 commented Jun 1, 2016

Raised #24937 for the disappearance of these hooks on master
CC @rullzer

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2/3 of the hooks of OC\Files\Storage\Shared can point to unexisting files
5 participants