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

files_external: proper user context for sharing #16525

Merged
merged 7 commits into from
Aug 2, 2019

Conversation

juliusknorr
Copy link
Member

This will make sure we also have a proper user context for non-logged in users that access the instance though share links. Otherwise the file system setup doesn't work properly for share links, if either a $home or $user variable is configured for substitution.

Steps to reproduce:

  • Configure external storage mounted to / with $user contained in the path
  • Try to access file shared via link

@juliusknorr
Copy link
Member Author

/backport to stable16

@blizzz
Copy link
Member

blizzz commented Jul 24, 2019

That's a good catch already. Another use case are internal shares with placeholders in the config. They also don't resolve because of the wrong replacement it seems.

@scanom
Copy link

scanom commented Jul 24, 2019

Just tested the PR and it solves partially the issues referenced. Shared files with path containing $user variable work fine now, but opening them with onlyoffice still broken (download failed). As before, same file without $user in path opens fine.

@juliusknorr
Copy link
Member Author

That's a good catch already. Another use case are internal shares with placeholders in the config. They also don't resolve because of the wrong replacement it seems.

Hm, then I think it would be better if we pass though the user id that is used to setup the storage (as it seems that it was in stable15 and before). I'll have another look.

@juliusknorr juliusknorr changed the title files_external: proper user user context for sharing files_external: proper user context for sharing Jul 25, 2019
@juliusknorr
Copy link
Member Author

juliusknorr commented Jul 25, 2019

@blizzz I've pushed another commit so we reuse the user id that is used to setup the mountpoints from

public static function initMountPoints($user = '') {

@scanom I don't have a only office test setup, so maybe you can give that another try. At least for collabora this works just fine now.

@blizzz
Copy link
Member

blizzz commented Jul 25, 2019

@juliushaertl let me test it today

@blizzz blizzz self-requested a review July 25, 2019 08:33
@juliusknorr
Copy link
Member Author

TypeError: Argument 1 passed to OCA\Files_External\Config\UserContext::__construct() must implement interface OCP\IUserSession, instance of Mock_IRequest_d7497604 given, called in /drone/src/apps/files_external/tests/Config/UserPlaceholderHandlerTest.php on line 59

Seems the tests also need another look 🤔 Not sure why they don't run yet.

@scanom
Copy link

scanom commented Jul 25, 2019

@juliushaertl PR working flawlessly now, checked both Onlyoffice and Collabora.

@blizzz
Copy link
Member

blizzz commented Jul 25, 2019

Issue persists for me, but it seems the issue is deeper. The wrong storage is used when looking for the file id. Still debugging.

@blizzz
Copy link
Member

blizzz commented Jul 25, 2019

Issue persists for me, but it seems the issue is deeper. The wrong storage is used when looking for the file id. Still debugging.

There was an adjustment on the home handler needed, could have seen it way earlier. Stupid me.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke
Copy link
Member

CI fails :/

@juliusknorr
Copy link
Member Author

TypeError: Argument 1 passed to OCA\Files_External\Config\UserContext::__construct() must implement interface OCP\IUserSession, instance of Mock_IRequest_8a9960c5 given, called in /drone/src/apps/files_external/tests/Config/UserPlaceholderHandlerTest.php on line 59

Still not sure why that happens. Could it be that phpunit is not figuring out the proper type hints since it is in the constructor parent class (UserContext)?

@juliusknorr juliusknorr force-pushed the bugfix/external-user-substitution branch from 469f055 to 908dd01 Compare July 30, 2019 07:30
@juliusknorr
Copy link
Member Author

Tests should be fine now ... stupid typos.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

my commits got lost

and I think also something of yours, @juliushaertl, because I don't get the share owning user anymore. I don't recall what the change was here and I overwrote my local copy with this state :(

@juliusknorr
Copy link
Member Author

Hm, not sure what happened, because I had your commits locally as I tested them. Github still has them, let try to grab them

juliusknorr and others added 6 commits August 2, 2019 08:39
…ution of variables

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/external-user-substitution branch from ae2fcd7 to bd08902 Compare August 2, 2019 06:39
@juliusknorr
Copy link
Member Author

@blizzz Restored

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 2, 2019
@blizzz blizzz merged commit bc40916 into master Aug 2, 2019
@delete-merged-branch delete-merged-branch bot deleted the bugfix/external-user-substitution branch August 2, 2019 12:07
@backportbot-nextcloud
Copy link

backport to stable16 in #16637

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 feature: external storage feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants