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: files_versions store #33305

Closed
wants to merge 2 commits into from
Closed

Fix: files_versions store #33305

wants to merge 2 commits into from

Conversation

LinneyS
Copy link

@LinneyS LinneyS commented Jul 21, 2022

In version 7.5.2 of Nextcloud ONLYOFFICE integration app we have changed file saving (ONLYOFFICE/onlyoffice-nextcloud@1468eff):
we call \OC_Util::setupFS with the owner of the file
https://github.com/ONLYOFFICE/onlyoffice-nextcloud/blob/f77c15953d9956a74f45cd0a5acef215d495a2ab/controller/callbackcontroller.php#L498

and call \OC_User::setUserId with the author of the changes
https://github.com/ONLYOFFICE/onlyoffice-nextcloud/blob/f77c15953d9956a74f45cd0a5acef215d495a2ab/controller/callbackcontroller.php#L468

In general, the save is successful. But an error occurs when editing a shared file from a group folder or from external storage. And in the logs there is an error from the File Versions application
ONLYOFFICE/onlyoffice-nextcloud#660

This fix makes the save work.

Hi @juliushaertl @PVince81
can you help?

Signed-off-by: Sergey Linnik sergey.linnik@onlyoffice.com

Thanks

fix ONLYOFFICE/onlyoffice-nextcloud#660

Signed-off-by: Sergey Linnik <sergey.linnik@onlyoffice.com>
@CarlSchwan CarlSchwan added bug 3. to review Waiting for reviews labels Jul 21, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 25 milestone Jul 21, 2022
@juliushaertl
Copy link
Member

juliushaertl commented Jul 22, 2022

I think this is just hiding the symptoms and would stop the version storage in that case. The issue seems to be more that the versions backend is using the filesystem owner to construct the files view and that is a user that might not have access to the file.

Minimal reproduction case similar to the onlyoffice codebase:

  • have a groupfolder that only admin can access
  • Create a file in that groupfolder (Readme.md) and share that file with user2
  • Call the following script without a user session
<?php
	require_once __DIR__ . '/../lib/versioncheck.php';
	require_once __DIR__ . '/../lib/base.php';
	OC_Util::setupFS('admin');
	OC_User::setUserId('user2');

	$userFolder = \OC::$server->getRootFolder()->getUserFolder('admin');
	$file = $userFolder->get('/groupfolder/Readme.md');
	$file->putContent('adf' . time());

Not sure where to hook in there properly to solve this, as the different user ids seem rather something our codebase is not really ready for.

@LinneyS Can you elaborate a bit more on the use case using a different user for getting the file (setupFs) and overwriting the current user?

Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Sergey Linnik <sergey.linnik@onlyoffice.com>
@LinneyS
Copy link
Author

LinneyS commented Jul 25, 2022

Hi @juliushaertl
In version 7.4.0 we tried to add permission extension. For example, when granting read access, we add the ability to expand access to "comment only". In this case, when using ONLYOFFICE, the file is opened in a special mode where you cannot edit the entire file, but you can add a comment to the text. For Nextcloud, it will still be like saving a file from a user with the "read" access right.

@juliushaertl juliushaertl self-assigned this Aug 1, 2022
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
Comment on lines 194 to +199

$eventDispatcher = \OC::$server->get(IEventDispatcher::class);
$fileInfo = $files_view->getFileInfo($filename);
if ($fileInfo === false) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$eventDispatcher = \OC::$server->get(IEventDispatcher::class);
$fileInfo = $files_view->getFileInfo($filename);
if ($fileInfo === false) {
return false;
}
$fileInfo = $files_view->getFileInfo($filename);
if ($fileInfo === false) {
$uid = \OC\Files\Filesystem::getView()->getMount('')->getStorage()->getOwner('');
$files_view = new View('/'.$uid .'/files');
$fileInfo = $files_view->getFileInfo($filename);
}
if ($fileInfo === false) {
logger('files_versions')->error('Not storing version as the file could not be found for the owner or user: ' . $filename);
return false;
}

@juliushaertl
Copy link
Member

I think we can make this pick up the previous user from the setupFs call with the change mentioned in the comment, but am still unsure if that is a proper way or if the usage of setupFs and a different current user should actually be avoided. Would be nice to also get some insight from @icewind1991 here.

@LinneyS Can you maybe point to where author of the changes is actually exposed? For the version itself we don't store the user at all, so I'd be curious why exactly you set the different user, where the information is used further on.

@icewind1991
Copy link
Member

I think #33804 is the best approach atm.

In the long term this should be moved over to the newer fs hooks that provide the Node instead of just the path.

This was referenced Sep 6, 2022
@PVince81
Copy link
Member

#33804 is merged

should we close this PR as obsolete then ?

@vinicius73 vinicius73 closed this Sep 12, 2022
@LinneyS LinneyS deleted the patch-1 branch September 15, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants