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

File comments and versions in a SMB/CIFS mount are user specific #28249

Closed
AdamReece-WebBox opened this issue Jun 28, 2017 · 17 comments
Closed

Comments

@AdamReece-WebBox
Copy link

I've noticed that both comments and version history for files on SMB/CIFS mounts are user specific, which is of no use for a globally added (shared) mount. For example if a colleague leaves a comment describing a file, other colleagues should be able to see that comment.

The version history appears to be user specific because the older copies of files are stored within the users "black box" instead of on the SMB/CIFS mount. (Should they not be stored alongside the original file, but hidden from file listings?)

Steps to reproduce

  1. Select any file within a SMB/CIFS mount so the details pane is on the right.
  2. Add a comment to the file or modify the file.
  3. Log in as another user and find the file you just commented on or modified as the first user.

Expected behaviour

Comments and version history from other users should appear.

Actual behaviour

Comments and version history only show by the current user.

Server configuration

Operating system: Debian Linux 9 "Stretch"

Web server: Apache 2.4.25 (Debian)

Database: mysql Ver 15.1 Distrib 10.1.23-MariaDB, for debian-linux-gnu (x86_64) using readline 5.2

PHP version: PHP 7.0.19-1 (cli) (built: May 11 2017 14:04:47) ( NTS )

ownCloud version: ownCloud 10.0.1.5 (production)

Updated from an older ownCloud or fresh install: Fresh

Where did you install ownCloud from: Manual through shell. (Not using apt/yum.)

Signing status (ownCloud 9.0 and above): https://gist.github.com/AdamReece-WebBox/7a1e3eb17573daac4c9079dca6c6679b

The content of config/config.php: https://gist.github.com/AdamReece-WebBox/e1d79c950e4ea95a71b94841f4f8de39

List of activated apps:

Enabled:

  • calendar: 1.4.2
  • comments: 0.3.0
  • configreport: 0.1.1
  • contacts: 1.5.2
  • dav: 0.2.9
  • external: 1.2
  • federatedfilesharing: 0.3.0
  • federation: 0.1.0
  • files: 1.5.1
  • files_antivirus: 0.10.0.0
  • files_external: 0.7.0
  • files_external_ftp: 0.2.0
  • files_pdfviewer: 0.8.2
  • files_sharing: 0.10.0
  • files_texteditor: 2.2
  • files_trashbin: 0.9.0
  • files_versions: 1.3.0
  • files_videoplayer: 0.9.8
  • firstrunwizard: 1.1
  • market: 0.1.0
  • notifications: 0.3.0
  • provisioning_api: 0.5.0
  • systemtags: 0.3.0
  • templateeditor: 0.1
  • updatenotification: 0.2.1
  • user_external: 0.4
  • user_ldap: 0.9.1
    Disabled:
  • encryption
  • theme-example

Are you using external storage, if yes which one: SMB/CIFS

Are you using encryption: No

Are you using an external user-backend, if yes which one: LDAP

LDAP configuration (delete this part if not used)

+-------------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Configuration                 |                                                                                                                                                                         |
+-------------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| hasMemberOfFilterSupport      |                                                                                                                                                                         |
| hasPagedResultSupport         |                                                                                                                                                                         |
| homeFolderNamingRule          | attr:homeDirectory                                                                                                                                                      |
| lastJpegPhotoLookup           | 0                                                                                                                                                                       |
| ldapAgentName                 | ***REMOVED SENSITIVE VALUE*** (but this works OK)                                                                                                                       |
| ldapAgentPassword             | ***REMOVED SENSITIVE VALUE***                                                                                                                                           |
| ldapAttributesForGroupSearch  |                                                                                                                                                                         |
| ldapAttributesForUserSearch   |                                                                                                                                                                         |
| ldapBackupHost                | ldaps://172.29.x.x                                                                                                                                                      |
| ldapBackupPort                | 636                                                                                                                                                                     |
| ldapBase                      | ***REMOVED SENSITIVE VALUE*** (but this works OK)                                                                                                                       |
| ldapBaseGroups                | ***REMOVED SENSITIVE VALUE*** (but this works OK)                                                                                                                       |
| ldapBaseUsers                 | ***REMOVED SENSITIVE VALUE*** (but this works OK)                                                                                                                       |
| ldapCacheTTL                  | 600                                                                                                                                                                     |
| ldapConfigurationActive       | 1                                                                                                                                                                       |
| ldapDynamicGroupMemberURL     |                                                                                                                                                                         |
| ldapEmailAttribute            | mail                                                                                                                                                                    |
| ldapExperiencedAdmin          | 0                                                                                                                                                                       |
| ldapExpertUUIDGroupAttr       |                                                                                                                                                                         |
| ldapExpertUUIDUserAttr        |                                                                                                                                                                         |
| ldapExpertUsernameAttr        | uid                                                                                                                                                                     |
| ldapGroupDisplayName          | cn                                                                                                                                                                      |
| ldapGroupFilter               | (&(|(objectclass=posixGroup))(|(cn=Account Managers)(cn=Administrators)(cn=Developers)(cn=Staff)(cn=Users)))                                                            |
| ldapGroupFilterGroups         | Account Managers;Administrators;Developers;Staff;Users                                                                                                                  |
| ldapGroupFilterMode           | 0                                                                                                                                                                       |
| ldapGroupFilterObjectclass    | posixGroup                                                                                                                                                              |
| ldapGroupMemberAssocAttr      | memberUid                                                                                                                                                               |
| ldapHost                      | ldaps://***REMOVED SENSITIVE VALUE***                                                                                                                                   |
| ldapIgnoreNamingRules         |                                                                                                                                                                         |
| ldapLoginFilter               | (&(|(objectclass=inetOrgPerson)(objectclass=posixAccount)(objectclass=sambaSamAccount)(objectclass=shadowAccount))(|(uid=%uid)(|(mailPrimaryAddress=%uid)(mail=%uid)))) |
| ldapLoginFilterAttributes     |                                                                                                                                                                         |
| ldapLoginFilterEmail          | 1                                                                                                                                                                       |
| ldapLoginFilterMode           | 0                                                                                                                                                                       |
| ldapLoginFilterUsername       | 1                                                                                                                                                                       |
| ldapNestedGroups              | 0                                                                                                                                                                       |
| ldapOverrideMainServer        |                                                                                                                                                                         |
| ldapPagingSize                | 500                                                                                                                                                                     |
| ldapPort                      | 636                                                                                                                                                                     |
| ldapQuotaAttribute            |                                                                                                                                                                         |
| ldapQuotaDefault              |                                                                                                                                                                         |
| ldapTLS                       | 1                                                                                                                                                                       |
| ldapUserDisplayName           | displayname                                                                                                                                                             |
| ldapUserDisplayName2          |                                                                                                                                                                         |
| ldapUserFilter                | (|(objectclass=inetOrgPerson)(objectclass=posixAccount)(objectclass=sambaSamAccount)(objectclass=shadowAccount))                                                        |
| ldapUserFilterGroups          |                                                                                                                                                                         |
| ldapUserFilterMode            | 0                                                                                                                                                                       |
| ldapUserFilterObjectclass     | inetOrgPerson;posixAccount;sambaSamAccount;shadowAccount                                                                                                                |
| ldapUuidGroupAttribute        | auto                                                                                                                                                                    |
| ldapUuidUserAttribute         | auto                                                                                                                                                                    |
| turnOffCertCheck              | 0                                                                                                                                                                       |
| useMemberOfToDetectMembership | 1                                                                                                                                                                       |
+-------------------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Client configuration

Browser: Happens on any (Chrome/Edge/...)

Operating system: Happens on any (Windows/OSX/...)

Logs

Web server error log

Irrelevant, same as stack trace posted above.

ownCloud log (data/owncloud.log)

Irrelevant, same as stack trace posted above.

Browser log

Irrelevant, server side issue.

@PVince81
Copy link
Contributor

Comments are associated with a file id.

If you setup your SMB shares to be specific to individual users, then their view of the SMB shares will have their own file ids in oc_filecache.

The only way to have common metadata/comments/tags for all users on a SMB share is to set this one up as admin (system-wide mount) and give access to all users.

I suspect that you set up one external storage entry for each user individually, which won't work with this use case as every user gets their own storage. OC has no way to know that these files are exactly the same.

@AdamReece-WebBox
Copy link
Author

AdamReece-WebBox commented Jun 29, 2017

All SMB mounts are already system wide. These are defined in Settings > Admin > Storage. They also appear in Settings > Personal > Storage, but are not editable. (No users have any personal SMB's defined.)

@PVince81
Copy link
Contributor

@AdamReece-WebBox ok, so you did this correctly.

I did a quick local test and it works correctly for me for a system-wide SMB storage.

Does every user only see their individual comments ? Or is there a JS error happening somehow somewhere ?

Another thing you could check:

  1. given two users "user1" and "user2" who have access to this SMB storage "smbshare"
  2. login as "user1"
  3. explore the "smbshare" mount and create a file "test.txt"
  4. using the web browser, inspect the row in the table and find the attribute "data-id". That's the file id.
  5. post a comment "hello from user1"
  6. login as "user2"
  7. using the web browser, inspect the row in the table and find the attribute "data-id". That's the file id. It should be the same value as above. If not, then we have a weird bug.
  8. post a comment "hello from user2"
  9. select * from oc_comments where object_type='files' and object_id=$fileId, replace $fileId with the id you got above. You should see both comments.

@PVince81
Copy link
Contributor

but first, did you use "username and password" mode or "Log-in credentials, save in session" ?

I think the latter will create a separate storage entry per user, so it won't work... This is because SMB shares might return different contents based on who the user is, so OC cannot guess if the contents inside it is the same for all users.

@AdamReece-WebBox
Copy link
Author

AdamReece-WebBox commented Jun 29, 2017

Every user sees their own comments, but not anyone elses. I don't see any JS errors happening.

File attributes as me:

data-id="1168"
data-type="file"
data-size="93"
data-file="Test file.txt"
data-mime="text/plain"
data-mtime="1488529569000"
data-etag="5954d59b78f36"
data-permissions="11"
data-mounttype="external"
data-path="/Share/Misc"
data-share-permissions="3"
data-original-title=""

File attributes as another colleague:

data-id="1206"
data-type="file"
data-size="93"
data-file="Test file.txt"
data-mime="text/plain"
data-mtime="1488529569000"
data-etag="1488530775"
data-permissions="11"
data-mounttype="external"
data-path="/Share/Misc"
data-tags=""
data-share-permissions="3"
data-original-title=""

Looks like the file IDs are different then, even though we're browsing the same file in the same share.

Authentication is "Log-in credentials, save in session". This is very important to us because file I/O on shared files needs to happen in the context of the interactive user, not the web server's user.

For obvious good security reasons the web server's user does not have access to read/write/execute any of these shares. This is exactly the same reason as this issue I posted here, and was in fact suggested that SMB should be used to work around that.

@jvillafanez
Copy link
Member

If the credentials each user is using are different, the view each user will have will be different and ownCloud will consider the contents to be different.

You can use the "username + password" authentication so all the user will use the same credentials. You can create a specific account in your SMB server for this purpose. In this case comments, versions and any other metadata will be shared because each user will have the same view of the SMB server.

@AdamReece-WebBox
Copy link
Author

Thanks, but each user needs to authenticate as themselves rather than a common user, otherwise both ACLs and audit trails become useless.

I guess this would only work for us if OC kept track of files by their path+name instead of an arbitrary ID number, but that would be a major redesign and would have to track moves/renames.

OC probably isn't for us then.

@PVince81
Copy link
Contributor

OC would need to be enhanced to track different permissions for every user as it stores the permissions in oc_filecache. Would require extracting the permissions to a separate table.

@phil-davis
Copy link
Contributor

phil-davis commented Jun 29, 2017

An interesting design challenge. For security reasons I expect that sysadmins would prefer that each user connect with their own credentials to the SMB share. That means that the SMB share server can log whatever it wants accurately, and can police what files it shares to whom with what access.

ownCloud could add a concept like "meta-file-id" - for remote files the URL of the connection plus path down to the file and file name, for local files just re-use the file-id... Files with the same meta-file-id are understood to be ultimately the same file somewhere on some ultimate back end storage. Then store meta-data about the file (comments, tags, preview image, whatever) indexed under the "meta-file-id".

@AdamReece-WebBox
Copy link
Author

AdamReece-WebBox commented Jun 29, 2017

Is this actually a permissions issue? Sounds more like that the file IDs just don't map to the same real file for each user, likely down to each user seeing a different set of files due to permission rules. However this isn't permissions fault.

Considering the share is system wide perhaps OC should be keeping a system wide mapping of file IDs to file paths, regardless of who can/can't see them. Permissions can be applied after such mappings have taken place. This is the same as accessing files natively as different users -- some users don't have permission to read/write/execute certain files, but they still have the same INODE number or whatever no matter who the user is.

Example:

  • UserA can see a folder at path "/Assets/ClientX"
  • UserB can see a folder at path "/Assets/ClientY"
  • UserA and UserB can see a folder at path "/Assets/ClientZ"

Contents of "/Assets/ClientX" and "/Assets/ClientY" would likely differ, so by the time we reach "/Assets/ClientZ" for generating the user specific file listing the IDs would no longer match. If system wide shares used the same file ID to name mapping permissions wouldn't matter, because the shared mapping would be like "oh yes I've seen file "/Assets/ClientZ/Test.txt" already and it has ID 38523, but wait there Johnny Boy while we check if you have permission to read this as per your in-session authentication credentials".

@butonic
Copy link
Member

butonic commented Jun 29, 2017

INODEs are problematic:

  • they are reused, so you comments (tags and shares) from a deleted file might show up on a new file
  • they are only unique per device and we currently don't pass around the device id along with the fileid

Paths are problematic:

  • files may be renamed and there is no way to guarantee that oc will detect the move in time before a file / folder with the same name has been created

While oc does have a listener that can be attached to smb shares to get notified of file changes that still leaves a time gap and cannot guarantee detecting renames AFAIU.

We could store the device id and inode, along with the path. If the file is moved (or only the path changes) the inode stays the same but we cannot be sure if it was a rename or a delete and recreation. We might check the basename, mtime and filesize ... but that only leads to heuristics detecting moves which leaves end users wondering what happened when it doesn't work.

But assuming that rename detection and inode reuse (can be disabled for gpfs) can be fixed, then storing deviceid and inode along with the path should allow tying tags, shares and comments to them.

@jvillafanez
Copy link
Member

Accessing to something like smb://server/share/file by different accounts doesn't guarantee the file will be the same. Check https://ubuntuforums.org/showthread.php?t=1916212 to setup home directories, where accessing to smb://server/homes/ will return different content depending on the account used to connect.
I expect something similar to windows home directories.

In addition, I don't expect inodes to work with DFS or any other virtual FS accessible by SMB.

@PVince81
Copy link
Contributor

Accessing to something like smb://server/share/file by different accounts doesn't guarantee the file will be the same.

For this one idea I had was that we could have a checkbox in the external storage config for the admin: "I hereby declare that this SMB share delivers the same content for all users"

@AdamReece-WebBox
Copy link
Author

The "homes" share is a special case to Samba servers. This is not a standard SMB/CIFS feature. (OC claims to implement a SMB/CIFS client, not a Samba client.)

Windows since Vista by default shares user homes/profiles within the "Users" share, but you must still enter the user's folder within. The root contents of the "Users" share is not user specific with the exception of possibly hiding user folders they cannot read. For example as an administrator if I browse the "Users" share on a Windows 10 system I can see all user homes/profiles as sub-folder per user.

@AdamReece-WebBox
Copy link
Author

AdamReece-WebBox commented Jun 29, 2017

For this one idea I had was that we could have a checkbox in the external storage config for the admin: "I hereby declare that this SMB share delivers the same content for all users"

This should probably be the reverse as this behaviour is non-standard.

"The share differs per user. (E.g. Linux user home folders)"

@ownclouders
Copy link
Contributor

Hey, this issue has been closed because the label needs info is set and there were no updates for 14 days. Feel free to reopen this issue if you deem it appropriate.

@lock
Copy link

lock bot commented Aug 2, 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 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants