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

Properly check for a set guest name when obtaining the file #805

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

juliusknorr
Copy link
Member

To reproduce, put a file on external storage and share it with another user that doesn't have access to the external storage. This makes sure that the editor is not falsely overwritten with the owner.

@juliusknorr juliusknorr added bug Something isn't working 3. to review Ready to be reviewed labels Jan 27, 2020
@juliusknorr juliusknorr requested a review from rullzer January 27, 2020 13:45
rullzer
rullzer previously approved these changes Jan 27, 2020
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

makes sense

@juliusknorr juliusknorr added 2. developing Work in progress and removed 3. to review Ready to be reviewed labels Jan 29, 2020
@juliusknorr juliusknorr dismissed rullzer’s stale review January 29, 2020 06:28

This currently brings in other issues :/ Needs more investigation

@juliusknorr
Copy link
Member Author

juliusknorr commented Jan 29, 2020

@rullzer I pushed two more commits, as with the earlier fix, the filesystem was actually setup for the owner instead of the editor which caused the OC\Files\View::file_put_contents to fail to write a new file as the file exists check failed because it operated on the file obtained with the editor user scope earlier. Therefore I also moved the filesystem setup to the beginning of the putFile controller method, to avoid any further discrepancies in which user is used for filesystem access.

I think for a code cleanup we could try moving the whole Wopi token handling for which user is acting in the anonymous request to a middleware to make sure the preconditions are properly set for all WopiController methods. I'll look into that in a follow up.

@juliusknorr juliusknorr added 3. to review Ready to be reviewed and removed 2. developing Work in progress labels Jan 29, 2020
@juliusknorr juliusknorr added this to the 3.6.0 milestone Jan 29, 2020
Copy link

@lgnch lgnch left a comment

Choose a reason for hiding this comment

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

Changes tested. This has resolved the saving issues in #780. tests involved individual edits and saves to same file with users editing and exiting the document. Then saves where tested on the fly with users editing at the same time. Both worked without error on this commit.

@rullzer
Copy link
Member

rullzer commented Jan 30, 2020

@rullzer I pushed two more commits, as with the earlier fix, the filesystem was actually setup for the owner instead of the editor which caused the OC\Files\View::file_put_contents to fail to write a new file as the file exists check failed because it operated on the file obtained with the editor user scope earlier. Therefore I also moved the filesystem setup to the beginning of the putFile controller method, to avoid any further discrepancies in which user is used for filesystem access.

I think for a code cleanup we could try moving the whole Wopi token handling for which user is acting in the anonymous request to a middleware to make sure the preconditions are properly set for all WopiController methods. I'll look into that in a follow up.

yes. That would make a lot of sense indeed.

@juliusknorr juliusknorr merged commit f2b2f2e into master Jan 30, 2020
@juliusknorr juliusknorr deleted the bugfix/noid/only-use-owner-if-guest branch January 30, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants