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

Add support for shared file IDs on SMB storage #21049

Closed
wants to merge 2 commits into from

Conversation

ddean4040
Copy link

@ddean4040 ddean4040 commented May 19, 2020

Add a checkbox in external storage config so an admin can mark that external SMB storage is shared among users. If checked, Nextcloud file IDs on that storage will be the same for all users, even when using log-in credentials for authentication. Includes a test to verify the ID structure.

This creates WOPI URLs that are the same across users, so collaborative editing works as expected for documents on SMB shares using log-in credentials as the authentication method.

Signed-off-by: David ddean@njstatelib.org

Fix #21049

Signed-off-by: David <ddean@njstatelib.org>
@ddean4040 ddean4040 closed this May 19, 2020
Signed-off-by: David <ddean@njstatelib.org>
@ddean4040 ddean4040 reopened this May 19, 2020
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Cool 👍

Should be possible for the new test case to use a local variable.

@@ -96,6 +96,19 @@ public function testStorageId() {
$this->instance = null;
}

public function testSharedStorageId() {
$this->instance = new SMB([
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
$this->instance = new SMB([
$instance = new SMB([

'root' => 'someroot',
'shared' => true
]);
$this->assertEquals('smb::testhost//someshare//someroot/', $this->instance->getId());
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
$this->assertEquals('smb::testhost//someshare//someroot/', $this->instance->getId());
$this->assertEquals('smb::testhost//someshare//someroot/', $instance->getId());

'shared' => true
]);
$this->assertEquals('smb::testhost//someshare//someroot/', $this->instance->getId());
$this->instance = null;
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
$this->instance = null;

@kesselb kesselb added this to the Nextcloud 20 milestone May 19, 2020
@icewind1991
Copy link
Member

While I agree that the functionality this enables is very nice to have, having them share file ids means that everything in the smb share has to be exactly the same between all users accessing the storage, including all metadata like permissions.

Simply adding this option will inevitably lead to users enabling the option where this is not the case (even if you add some warning explaining the limitations) which will lead to hard to debug unexpected behaviour.

So while I would like to have a way to get this functionality, simply adding an option to drop the username from the storageid isn't the right way to go imo.

@ddean4040
Copy link
Author

@icewind1991 - Is there another way you can think of to get common file IDs with log-in credentials? I'm new to Nextcloud and this was just the first way that came to mind, but I'd be happy with any fix.

We're using this option for just the situation you described - to mount group shares from a Windows file server where everyone in the group has the same permissions. We assign each external storage to its user group in Nextcloud, matching the native permissions.

Without something like this we would have to disable co-editing, since users could work on the same document from an SMB share at the same time without being aware of each other, and one user's changes would be lost. Before this we also had a very bloated filecache table because of duplicate entries for each user for every file on external storage.

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@juliusknorr
Copy link
Member

We could of course just do this only in cases where global credentials are used, so when we can be sure that every user has the same access level to the files.

@icewind1991
Copy link
Member

in that case it should already be shared, since the storage id includes the username, you only get different file ids if the username used to connect to smb is different

This was referenced Dec 14, 2020
This was referenced Dec 28, 2020
@ddean4040
Copy link
Author

Hi! I keep getting pings about this and I don't know if I'm supposed to do something or what.

What we ran into is that, when you set up an external storage entry using log-in credentials under Administration, the file ID scheme makes Nextcloud think users accessing a shared file are working on different files.

There were LOTS of duplicate filecache entries, and collaborative editing didn't work as expected. Instead of working together on a document, staff thought they were working alone but were unknowingly overwriting each others' changes.

If we switch to global credentials, we essentially lose auditing on our file server, which some staff still access natively via SMB as a mapped drive. The log-in credentials option makes this hybrid access seamless, but only with some kind of fix to collapse the file IDs.

@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Jan 6, 2021
This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@blizzz blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 2, 2021
@skjnldsv
Copy link
Member

Hi! I keep getting pings about this and I don't know if I'm supposed to do something or what.

What we ran into is that, when you set up an external storage entry using log-in credentials under Administration, the file ID scheme makes Nextcloud think users accessing a shared file are working on different files.

There were LOTS of duplicate filecache entries, and collaborative editing didn't work as expected. Instead of working together on a document, staff thought they were working alone but were unknowingly overwriting each others' changes.

If we switch to global credentials, we essentially lose auditing on our file server, which some staff still access natively via SMB as a mapped drive. The log-in credentials option makes this hybrid access seamless, but only with some kind of fix to collapse the file IDs.

There are some comments, you need to address them :)
As there is no feedback since a while I will close this PR. If you are still willing to get this in, please address the potential comments and rebase to latest master. Then, feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants