Don't apply persistent lock on received declined shares

[PVince81]

From #32250 (comment)

Steps

1. as user "sharer" create a folder called "parent" and inside that a folder called "subfolder"
2. from user "sharer" share a folder "parent" with the user "receiver"
3. as user "sharer" set a lock on the folder "parent"
4. as user "receiver" decline the share "parent" from the "Shared with you" page
5. as user "receiver" create a folder called "parent" and inside that a folder called "subfolder"

Expected result

• the folder "parent/subfolder" of the user "receiver" not locked

Actual result

• the folder "parent/subfolder" of the user "receiver" will be locked by sharer
• deleting that lock as "receiver" results in Unlock failed with status: 409
• deleting/renaming parent/subfolder" as "receiver" is not possible because its locked

Version

discovered on #32250

It is likely that whatever checks for locks is retrieving the full list of share mount points and does not filter it by state. Need to check what APIs are used there as there is already an existing way to only get visible mount points.

[PVince81]

• user1 creates "test"
• user1 shares "test" with user2
• user2 declines "test"
• user2 creates "test/x"
• get lock on "test/x" as user2

SELECT `id`, `owner`, `timeout`, `created_at`, `token`, `token`, `scope`, `depth`, `file_id`, `path`, `owner_account_id`
FROM `oc_persistent_locks` l
INNER JOIN `oc_filecache` f ON l.`file_id` = f.`fileid`
WHERE (
    (`storage` = 4) AND (`created_at` > (1544710587 - `timeout`)) AND (f.`path` = 'files/test/x')
) OR (
    (`depth` <> 0) AND (`path` = 'files')
) OR (
    (`depth` <> 0) AND (`path` = 'files/test')
);

This returns the lock for user1:

+----+-------+---------+------------+--------------------------------------+--------------------------------------+-------+-------+---------+------------+------------------+
| id | owner | timeout | created_at | token                                | token                                | scope | depth | file_id | path       | owner_account_id |
+----+-------+---------+------------+--------------------------------------+--------------------------------------+-------+-------+---------+------------+------------------+
|  1 | user1 |    1800 | 1544709686 | 9cb71e26-3f36-472a-b30f-6bd36cb8b3df | 9cb71e26-3f36-472a-b30f-6bd36cb8b3df |     2 |    -1 |      33 | files/test |                2 |
+----+-------+---------+------------+--------------------------------------+--------------------------------------+-------+-------+---------+------------+------------------+

Looks like the storage limit isn't applied correctly.

[PVince81]

looking at the formatted SQL above, it seems we also need to add the storage id in the two other "OR" clauses

[PVince81]

• reenable acceptance test after fixing

[ownclouders]

GitMate.io thinks possibly related issues are #33847 (Persistent lock: cannot unshare from self), #27072 (Cannot move received federated share mount), #17243 (Lock sharing operations when file is locked), #31651 (Adding persistent lock types to ILockingProvider), and #17181 (Properly lock share owner parent folders).

[PVince81]

You don't even need to decline the share.
It seems that any user who have the same folder structure will get the lock ⚠️

This is too bad, increasing to p2 as we can't release in this state.

[PVince81]

@DeepDiver1975 FYI

[PVince81]

We should change the query to:

SELECT `id`, `owner`, `timeout`, `created_at`, `token`, `token`, `scope`, `depth`, `file_id`, `path`, `owner_account_id`
FROM `oc_persistent_locks` l
INNER JOIN `oc_filecache` f ON l.`file_id` = f.`fileid`
WHERE (
    (`storage` = 4)
    AND (`created_at` > (1544710587 - `timeout`))
    AND (
        (f.`path` = 'files/test/x')
        OR ((`depth` <> 0) AND (`path` = 'files'))
        OR ((`depth` <> 0) AND (`path` = 'files/test'))
    )
);

[PVince81]

Additional adjustment to have only one depth check:

SELECT `id`, `owner`, `timeout`, `created_at`, `token`, `token`, `scope`, `depth`, `file_id`, `path`, `owner_account_id`
FROM `oc_persistent_locks` l
INNER JOIN `oc_filecache` f ON l.`file_id` = f.`fileid`
WHERE (
    (`storage` = 4)
    AND (`created_at` > (1544710587 - `timeout`))
    AND (
        (f.`path` = 'files/test/x')
        OR ((`depth` <> 0) AND (`path` in ('files', 'files/test')))
    )
);

[PVince81]

in the spirit of TDD, here's a PR with a unit test that covers depth 0 and 1 and also an unrelated storage, which proves the issue: #33888

[PVince81]

• "Values other than 0 or infinity MUST NOT be used with the Depth header on a LOCK method" from https://tools.ietf.org/html/rfc4918#section-9.10.3

let's enforce that so we don't burden ourselves with useless use cases...