Prevent multiple accounts from sharing a sync folder #8849

TheOneRing · 2021-07-28T10:43:29Z

Since 838c072 we allow that multiple accounts share the same sync folder.

In combination with virtual files it leads to data loss
Security risk by allowing mirroring to an external server
We don't warn that we mirror to an external server

The text was updated successfully, but these errors were encountered:

Fixes: #8849

pmaier1 · 2021-08-03T08:24:20Z

Write down reasoning why to drop the feature => deprecation note
Alternatives?
Drop with 2.9

Fixes: #8849

jnweiger · 2021-08-26T00:07:05Z

Tested with testpilotcloud-client-2.9.0-beta3 and owncloud-client 2.9.0-beta3 on Fedora-33

connect testpilotcloud to [email protected] without using vfs
rename the Photos folder to demo-Photos
wait for the sync to finish
connect owncloud to [email protected] with using vfs and re-using the folder ~/testpilotcloud as syncroot.
wait for the sync to finish.

No error messages occur, both accounts on the server now have the same files and folders. admin's Photos folder appears at user demo. Demo's demo-Photos folder appars at user admin.

Expected behaviour, sharing of sync root folders should not be possible when using VFS.

However, when configuring the second account with the same client, sharing of the sync root folder is not possible, as expected.

TheOneRing · 2021-08-26T09:53:02Z

This should prevented in https://github.com/owncloud/client/blob/master/src/gui/folderman.cpp#L1332
@erikjv could you have a look?

erikjv · 2021-08-26T10:05:27Z

@jnweiger can you trigger the message "The folder %1 is used in a folder sync connection!" at all?

Also: can you check what the exact name is of the .sync files? Is there a ._sync* in there?

jnweiger · 2021-08-26T11:42:30Z

It is named .sync_journal.db (without the underscore after the dot.)

Hmm, when I try a second connection with the same client using the same sync root folder, I get a slightly different message:

Not sure, how to actually provoke the message you quote.

erikjv · 2021-08-26T14:13:30Z

Never mind, I think I already found the issue.

... and not the parent directory of the path. Fixes: #8849

The dir() method will return the *parent* directory of the file/directory in the QFileInfo. So if "~/ownCloud" was passed in as the path, the check for .sync_*.db would be done on "~/". Because we're migrating from ._sync_*.db to .sync_*.db, it will now also check for the old filename. Fixes: #8849

jnweiger · 2021-08-30T16:43:52Z

Retested in an update scenario on Linux Mint:

testpilotcloud 2.9.0-beta3 and owncloud 2.9.0-beta3 configured a remote connection to different servers using the shared ~/ownCloud4 sync folder.
the servers start sharing data in a confusing way. To be expected. This is before @erikjv 's fix came in.
stop both clients. Upgrade both to 2.9.0-rc1 to bring in that fix.
start testpilotcloud client. Wait for sync to complete
start owncloud client -- it hangs frozen.
killall owncloud, to be really sure there is nothing old.
Try inf foreground from the shell as follows, window frame is drawn without contents, Place for tray icon is created without content, we get only these few lines of output then it hangs:

$ owncloud --debug -s --logflush --logfile -
08-30 17:20:41:171 [ debug default ]    [ OCC::Logger::setLogRules ]:   "gui.*.debug=true\nsync.*.debug=true\nsync.httplogger=true"
08-30 17:20:41:171 [ debug default ]    [ OCC::Logger::setLogRules ]:   "gui.*.debug=true\nsync.httplogger=true\nsync.*.debug=true\nsync.httplogger=true"
08-30 17:20:41:197 [ info gui.application ]:    ################## "ownCloud" locale: "en_US" version: "ownCloud 2.9.0rc1 (build 5070) 49cfb1 Aug 27 2021 17:26:42 Libraries Qt 5.12.10, OpenSSL 1.1.1f  31 Mar 2020 Using virtual files plugin: off linuxmint-5.4.0-81-generic"
08-30 17:20:41:197 [ info gui.application ]:    Arguments: ("owncloud", "--debug", "-s", "--logflush", "--logfile", "-")
08-30 17:20:41:198 [ info gui.application ]:    Using "en_US" translation
08-30 17:20:41:198 [ info gui.application ]:    Adding extra plugin search path: "/opt/ownCloud/ownCloud/bin/../lib/x86_64-linux-gnu/plugins"
08-30 17:20:41:200 [ info gui.application ]:    VFS suffix plugin is available
08-30 17:20:41:200 [ info gui.socketapi ]:      server started, listening at  "/run/user/1000/ownCloud/socket"
08-30 17:20:41:258 [ info gui.account.manager ]:        Restored:  0  unknown certs.
08-30 17:20:41:258 [ info gui.account.manager ]:        Restored:  0  unknown certs.
08-30 17:20:41:258 [ info gui.account.manager ]:        Restored:  0  unknown certs.
08-30 17:20:41:259 [ info gui.account.manager ]:        Restored:  1  unknown certs.
08-30 17:20:41:283 [ debug gui.activity ]       [ OCC::ActivitySettings::setNotificationRefreshInterval ]:      Starting Notification refresh timer with  300  sec interval
08-30 17:20:41:298 [ debug gui.application ]    [ OCC::GeneralSettings::loadLanguageNamesIntoDropdown ]:        Warning: could not find native language name for locale "TW"
08-30 17:20:41:394 [ debug gui.updater ]        [ OCC::Updater::getSystemInfo ]:        Sys Info size:  83
08-30 17:20:41:394 [ debug gui.updater ]        [ OCC::Updater::create ]:       QUrl("https://updates.owncloud.com/client/?client=RGlzdHJpYnV0b3IgSUQ6CUxpbnV4bWludApEZXNjcmlwdGlvbjoJTGludXggTWludCAyMC4yClJlbGVhc2U6CTIwLjIKQ29kZW5hbWU6CXVtYQo%3D&version=2.9.0.5070&platform=linux&oem=ownCloud&buildArch=x86_64&currentArch=x86_64&versionsuffix=rc1")
08-30 17:20:41:470 [ info gui.application ]:    Tray menu workarounds: noabouttoshow: false fakedoubleclick: false showhide: false manualvisibility: false
08-30 17:20:41:479 [ info gui.folder.manager ]: Setup folders from settings file
08-30 17:20:41:479 [ info plugins ]:    Created VFS instance from plugin "owncloudsync_vfs_suffix"
08-30 17:20:41:479 [ debug gui.folder ] [ OCC::Folder::checkLocalPath ]:        Checked local path ok
08-30 17:20:41:479 [ info sync.configfile ]:    Adding system ignore list to csync: "/etc/ownCloud/sync-exclude.lst"

jnweiger · 2021-08-30T17:16:27Z

I'd venture a guess:

Beta3 already uses the new naming scheme, but did not detect the collision correctly.
When rc1 gets installed, the client tries an exclusive flock() on its sqlite db, while the same file is already flock()ed exclusively by the alread running client?
this situation is probably unrealistic, as we don't expect beta3 out in the wild.

TODO:

retest upgrade with shared sync root on a fresh system
retest on windows

jnweiger · 2021-08-31T08:33:52Z

Retested the upgrade scenario in Fedora 33

install both testpilot cand owncloud client in verison 2.8.2,
connect both with different accounts to servers, using the same e.g. ~/testpilotcloud3 sync folder
wait for (chaotic) cross sync to finish. quit both, upgrade both to 2.9.0 rc1
both continue happily to sync. BAD
The new name .sync_journal.db is not used. We retain the two differently named .sync_....db files. BAD

testpilotcloud-logdir.zip
ownCloud-logdir.zip

jnweiger · 2021-08-31T11:01:23Z

Regarding charotic syncing:

Using VFS on Linux (Placeholder mode) in one client, and physical mode in the other client, bad things happen.
- placeholder files are uploaded to the server (seen in 2.9.0-rc1)

jnweiger · 2021-08-31T14:54:37Z

Retested the upgrade scenario in Windows 10

install both testpilot cand owncloud client in verison 2.8.2,
connect both with different accounts to servers, using the same sync folder
wait for (chaotic) cross sync to finish. quit both, upgrade both to 2.9.0 rc1
same as on Fedora33 above, both clients happily sync.

A third sync folder connetcion (fresh, not from an upgrade) cannot be added to the same sync folder:

jnweiger · 2021-09-01T09:58:20Z

I am not sure what the expected behaviour should be:

a) Detect the folder sharing and prevent using that with 2.9.0 and later.
b) Allow it as a legacy configuration so to not disrupt the user too much. But disallow the creation of the same.

Currently we do b) -- this has two disadvantages

the user might experience some misbehaviour
the user is not aware that the same sharing cannot b recreated with 2.9.0

To make b) nice and safe, I suggest to at least have a pop up a warning dialog, e.g. like this:

"Warning: Legacy shared sync folder detected: Folder testpilotcloud3 is connected to [email protected] and [email protected] -- this can lead to bad side effects including data loss, especially when using VFS. Please resolve this as soon as possible. [Remove Account] [Continue for now]"

Maybe (also) a pemanent red warning....

TheOneRing · 2021-09-06T07:41:16Z

At the moment the behavior is ok, we didn't plan to break existing setups we just want that never to happen again.

TheOneRing added this to the 2.9.0 milestone Jul 28, 2021

TheOneRing added a commit that referenced this issue Jul 29, 2021

Prevent using a sync folder for multiple accounts if vfs is used

ba26b0f

Fixes: #8849

TheOneRing added a commit that referenced this issue Jul 30, 2021

Prevent using a sync folder for multiple accounts if vfs is used

21b4dbb

Fixes: #8849

TheOneRing changed the title ~~Prevent multiple accounts from sharing a sync folder in cas of VFS~~ Prevent multiple accounts from sharing a sync folder Aug 3, 2021

TheOneRing added a commit that referenced this issue Aug 3, 2021

Prevent using a sync folder for multiple accounts

3d58364

Fixes: #8849

TheOneRing linked a pull request Aug 5, 2021 that will close this issue

Prevent sync root sharing #8853

Merged

TheOneRing added a commit that referenced this issue Aug 6, 2021

Prevent using a sync folder for multiple accounts

990ace4

Fixes: #8849

TheOneRing added a commit that referenced this issue Aug 9, 2021

Prevent using a sync folder for multiple accounts

dcf7105

Fixes: #8849

TheOneRing closed this as completed in 975b0f2 Aug 10, 2021

jnweiger mentioned this issue Aug 23, 2021

Testplan Release Desktop 2.9 #8870

Closed

20 tasks

jnweiger reopened this Aug 26, 2021

jnweiger mentioned this issue Aug 26, 2021

Release Desktop 2.9 #8869

Closed

39 tasks

erikjv self-assigned this Aug 26, 2021

erikjv added a commit that referenced this issue Aug 26, 2021

Check the correct path for .sync_*.db and ._sync_*.db

19365ba

... and not the parent directory of the path. Fixes: #8849

erikjv added a commit that referenced this issue Aug 26, 2021

Check the correct path for .sync_*.db and ._sync_*.db

b7ff732

... and not the parent directory of the path. Fixes: #8849

TheOneRing closed this as completed in aaf4a70 Aug 27, 2021

jnweiger reopened this Aug 30, 2021

TheOneRing closed this as completed Sep 6, 2021

TheOneRing mentioned this issue Sep 6, 2021

Warn on unsupported legacy configurations #8977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent multiple accounts from sharing a sync folder #8849

Prevent multiple accounts from sharing a sync folder #8849

TheOneRing commented Jul 28, 2021 •

edited

Loading

pmaier1 commented Aug 3, 2021

jnweiger commented Aug 26, 2021 •

edited

Loading

TheOneRing commented Aug 26, 2021

erikjv commented Aug 26, 2021 •

edited

Loading

jnweiger commented Aug 26, 2021 •

edited

Loading

erikjv commented Aug 26, 2021

jnweiger commented Aug 30, 2021

jnweiger commented Aug 30, 2021 •

edited

Loading

jnweiger commented Aug 31, 2021 •

edited

Loading

jnweiger commented Aug 31, 2021

jnweiger commented Aug 31, 2021

jnweiger commented Sep 1, 2021 •

edited

Loading

TheOneRing commented Sep 6, 2021

Prevent multiple accounts from sharing a sync folder #8849

Prevent multiple accounts from sharing a sync folder #8849

Comments

TheOneRing commented Jul 28, 2021 • edited Loading

pmaier1 commented Aug 3, 2021

jnweiger commented Aug 26, 2021 • edited Loading

TheOneRing commented Aug 26, 2021

erikjv commented Aug 26, 2021 • edited Loading

jnweiger commented Aug 26, 2021 • edited Loading

erikjv commented Aug 26, 2021

jnweiger commented Aug 30, 2021

jnweiger commented Aug 30, 2021 • edited Loading

jnweiger commented Aug 31, 2021 • edited Loading

jnweiger commented Aug 31, 2021

jnweiger commented Aug 31, 2021

jnweiger commented Sep 1, 2021 • edited Loading

TheOneRing commented Sep 6, 2021

TheOneRing commented Jul 28, 2021 •

edited

Loading

jnweiger commented Aug 26, 2021 •

edited

Loading

erikjv commented Aug 26, 2021 •

edited

Loading

jnweiger commented Aug 26, 2021 •

edited

Loading

jnweiger commented Aug 30, 2021 •

edited

Loading

jnweiger commented Aug 31, 2021 •

edited

Loading

jnweiger commented Sep 1, 2021 •

edited

Loading