Extend pending shares list in frontend to include remote shares #27751

PVince81 · 2021-07-01T15:32:11Z

And adjust the accept/decline actions to use the right endpoint for remote shares.
Note: the backend API was already available.

Fixes #23769

Also makes the following scenario work:

On instance A share a folder "test" with a group "group1" on instance B
On instance B, add a new user "new-user" to the group "group1"
Login as "new-user"
Go to "Pending shares" and accept the received remote share

In this scenario no notification is generated for the newly added user, and I don't think we want to.
This PR still makes the pending remote share visible in the correct place.

Tests

Single user remote share

list
accept
reject
re-accept => not possible, once rejected it's gone, by design
re-reject => not possible, once rejected it's gone, by design

Group remote share

PVince81 · 2021-07-01T15:35:22Z

bug: adjust endpoint target for rejecting share: needs extra "/pending" suffix

PVince81 · 2021-07-01T15:41:21Z

bug: some mess happening after accepting a group share, it seems the share entries have duplicated in the DB but now for the current user

I wonder if that existing backend part is really reliable or whether it contains yet to be discovered bugs 🤔

PVince81 · 2021-07-01T15:49:57Z

it looks like "oc_share_external" for groups works in a similar fashion like "oc_share": for a group share there's a main entry and sub-entries.

This is how it looks like after rejecting the group share "new-group-share-reject":

+----+--------+------------+--------------------------------+-----------+-----------------+----------+-------------------------+-------+-------------+-----------------------------------------------------+----------------------------------+----------+
| id | parent | share_type | remote                         | remote_id | share_token     | password | name                    | owner | user        | mountpoint                                          | mountpoint_hash                  | accepted |
+----+--------+------------+--------------------------------+-----------+-----------------+----------+-------------------------+-------+-------------+-----------------------------------------------------+----------------------------------+----------+
|  6 |     -1 |          1 | https://vvortex-release.local/ | 32        | 5HRRJLdWNSZzSmE |          | /new-group-share-accept | admin | named-users | {{TemporaryMountPointName#/new-group-share-accept}} | 3bea95b0013a55aa8d1b0fc5bd9198c9 |        0 |
|  7 |     -1 |          1 | https://vvortex-release.local/ | 33        | ynZpKQYrw5jcN9O |          | /new-group-share-reject | admin | named-users | {{TemporaryMountPointName#/new-group-share-reject}} | 3ef4d6505bba0fc9dd07a6877f387731 |        0 |
|  8 |      7 |          1 | https://vvortex-release.local/ | 33        | ynZpKQYrw5jcN9O |          | /new-group-share-reject | admin | evert       | {{TemporaryMountPointName#/new-group-share-reject}} | 3ef4d6505bba0fc9dd07a6877f387731 |        0 |
+----+--------+------------+--------------------------------+-----------+-----------------+----------+-------------------------+-------+-------------+-----------------------------------------------------+----------------------------------+----------+

The trouble is that all these entries are returned by the "/pending" endpoint as is, so I now see the "new-group-share-reject" entry twice in the UI. But the accept/reject actions only operate on the share id of the group share entry, the other ones return 404.

It seems that this listing endpoint is missing the logic that transforms it to the point of view of the current user. This logic should be in the backend like for regular shares. :-/

PVince81 · 2021-07-01T15:51:59Z

yup, confirmed, the complex aggregation logic is missing here: https://github.com/nextcloud/server/blob/enh/23769/accept-decline-fed-sharing-ui/apps/files_sharing/lib/External/Manager.php#L644

apps/files_sharing/js/sharedfilelist.js

PVince81 · 2021-07-02T08:07:41Z

I've fixed the backend now and extended the unit tests, it was a bit more complicated than expected.

I've used the old style queries to save some time. At some point we should move to the query builder.

Up next: retesting through the frontend and checking for bugs

PVince81 · 2021-07-02T08:09:09Z

apps/files_sharing/lib/External/Manager.php

+				return !in_array($share['id'], $toRemove, true);
+			});
+
+			if (!is_null($accepted)) {


note: I had to move this out of the SQL query because when fetching the group share entry is never "accepted" but the sub-entry might be, and we need to have the non-accepted group share present to delete it from the results with the above filter

PVince81 · 2021-07-02T13:25:37Z

fixed postgresql tests by adding more casting for the returned DB values

PVince81 · 2021-07-02T14:16:55Z

BUG: can't re-accept after rejecting group share once: this is because the share entry returned by the API is actually the sub-share, and this one can't be re-inserted...
- fix
- add unit test for this scenario

PVince81 · 2021-07-02T14:36:10Z

make sure we only fetch a single share when processing the mount => fetchShare is never called when mounting the filesystem for an accepted share, so no scalability issues

PVince81 · 2021-07-02T14:37:19Z

remove "reject" action for pending group share as it cannot be discarded

PVince81 · 2021-07-02T14:55:38Z

prevent sidebar to open/load for remote shares as we have no data ? (click the avatar of owner)

PVince81 · 2021-07-02T15:44:10Z

bug: only remove the "reject" action for received group shares not user shares

PVince81 · 2021-07-02T15:49:25Z

I've fixed the mess in the backend and now it is possible to work with the sub-share entry when re-accepting or rejecting.

check what happens when operating on the group share id whenever a sub-share id already exists... (the UI operates on the sub-share)
- from the code this will likely fail as it will attempt to reinsert the already existing entry

=> solved now by operating on sub-share when it exists: #27751

PVince81 · 2021-07-02T15:59:33Z

~~backport/split the code that adds missing remote group share handling for status icon ?~~ later if relevant
~~split the code that enhances fileactions ?~~ later if relevant

And adjust the accept/decline actions to use the right endpoint for remote shares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Fix pending shares endpoint to consider user-specific sub-entries for group shares whenever a share was accepted or declined. Added unit test for adding remote group shares. Fixed "removeUserShares" to not send a remote request as we never send remote requests for group shares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

In the list of pending shares, the option for rejecting the share has been removed. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Instead of just returning false, also log the exception to make debugging database issues easier. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

When accepting a group share, a sub-share entry is created which also has a different id. When accepting or rejecting the sub-share, simply update the "accepted" flag instead of trying to re-insert the entry. Adjust getShare to also properly validate group share membership when called on a sub-share id. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Only remove reject share for remote group shares Also fix share indicator to appear for remote group shares as well. Fix pending remote share icon to be the one of a share. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Accepting and declining can now be done repeatedly on both the parent group share and sub-share with the same effects. Added unit tests to cover these cases, and also when the same operation is repeated. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Use query builder with proper matching for finding the group names. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

When declining a remote group share through the dialog that appears when notifications are off, the mount point is now correctly saved when re-accepting. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

apps/files_sharing/lib/External/Manager.php

apps/files_sharing/tests/External/ManagerTest.php

Signed-off-by: Vincent Petry <vincent@nextcloud.com> Co-authored-by: Carl Schwan <carl@carlschwan.eu>

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 · 2021-07-27T11:36:49Z

resolved conflicts and fixed route problems and missing imports for LoggerInterface

it works again.

however, while going through the code with @artonge we found some additional issues to fix

When deleting a user, we should only delete the direct remote user shares or the remote group based subshares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 · 2021-07-27T12:35:47Z

I've now fixed user deletion and also added a new method for group deletion, mostly for the sake of tests: f6f2f63

User deletion should now be properly limited to its share type and group subshares, and so is group deletion.

Note: I didn't add a hook yet for group deletion as it seems it will be more complicated and I didn't want to dig further in code unrelated to the task at hand.

PVince81 · 2021-07-27T12:36:01Z

@artonge can you review again ?

a lot has changed since, please re-review

PVince81 · 2021-07-28T09:53:13Z

thanks a lot everyone! what a journey...

PVince81 · 2021-07-28T09:53:21Z

/backport to stable22

PVince81 added enhancement 3. to review Waiting for reviews labels Jul 1, 2021

PVince81 added this to the Nextcloud 23 milestone Jul 1, 2021

PVince81 requested review from juliushaertl, skjnldsv and szaimen July 1, 2021 15:32

PVince81 self-assigned this Jul 1, 2021

PVince81 mentioned this pull request Jul 1, 2021

Cannot accept or decline federated share after dismissing notification #23769

Closed

PVince81 marked this pull request as draft July 1, 2021 15:52

szaimen removed their request for review July 1, 2021 16:56

juliushaertl reviewed Jul 2, 2021

View reviewed changes

apps/files_sharing/js/sharedfilelist.js Outdated Show resolved Hide resolved

PVince81 force-pushed the enh/23769/accept-decline-fed-sharing-ui branch from 32bc04a to 7d2b91d Compare July 2, 2021 08:06

PVince81 commented Jul 2, 2021

View reviewed changes

PVince81 force-pushed the enh/23769/accept-decline-fed-sharing-ui branch from 7d2b91d to 2c59f6c Compare July 2, 2021 13:25

PVince81 added 2. developing Work in progress feature: federation and removed 3. to review Waiting for reviews labels Jul 2, 2021

PVince81 added 11 commits July 27, 2021 12:19

Extend pending shares list to include remote shares

53aafff

And adjust the accept/decline actions to use the right endpoint for remote shares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Remove "Reject share" for pending remote shares

46b8cf4

In the list of pending shares, the option for rejecting the share has been removed. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Add logging to external shares manager

cff8ae7

Instead of just returning false, also log the exception to make debugging database issues easier. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Pending remote group share fixes

f3f2faa

Only remove reject share for remote group shares Also fix share indicator to appear for remote group shares as well. Fix pending remote share icon to be the one of a share. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Disable default actions in pending file list

c156321

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Fix external share manager with multiple user groups

e8f4a52

Use query builder with proper matching for finding the group names. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Use mimetype instead of type when defaulting in remote shares

3deffc3

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

Fix remote group share decline+accept code path

e67e90a

When declining a remote group share through the dialog that appears when notifications are off, the mount point is now correctly saved when re-accepting. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 commented Jul 27, 2021

View reviewed changes

apps/files_sharing/lib/External/Manager.php Outdated Show resolved Hide resolved

PVince81 commented Jul 27, 2021

View reviewed changes

apps/files_sharing/tests/External/ManagerTest.php Outdated Show resolved Hide resolved

PVince81 and others added 2 commits July 27, 2021 13:04

Replace ILogger with LoggerInterface in remote share manager

dac676c

Signed-off-by: Vincent Petry <vincent@nextcloud.com> Co-authored-by: Carl Schwan <carl@carlschwan.eu>

Fix route path for pending remote shares

1125832

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

PVince81 force-pushed the enh/23769/accept-decline-fed-sharing-ui branch from 927782d to 1125832 Compare July 27, 2021 11:34

Fix remote share deletion when deleting user

f6f2f63

When deleting a user, we should only delete the direct remote user shares or the remote group based subshares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>

artonge approved these changes Jul 28, 2021

View reviewed changes

juliushaertl approved these changes Jul 28, 2021

View reviewed changes

juliushaertl merged commit a71294e into master Jul 28, 2021

juliushaertl deleted the enh/23769/accept-decline-fed-sharing-ui branch July 28, 2021 08:38

backportbot-nextcloud bot added the backport-request label Jul 28, 2021

backportbot-nextcloud bot mentioned this pull request Jul 28, 2021

[stable22] Extend pending shares list in frontend to include remote shares #28209

Merged

backportbot-nextcloud bot removed the backport-request label Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend pending shares list in frontend to include remote shares #27751

Extend pending shares list in frontend to include remote shares #27751

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 1, 2021

PVince81 commented Jul 1, 2021

PVince81 commented Jul 2, 2021

PVince81 Jul 2, 2021

PVince81 commented Jul 2, 2021

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 27, 2021

PVince81 commented Jul 27, 2021

PVince81 commented Jul 27, 2021

PVince81 commented Jul 28, 2021

PVince81 commented Jul 28, 2021

Extend pending shares list in frontend to include remote shares #27751

Extend pending shares list in frontend to include remote shares #27751

Conversation

PVince81 commented Jul 1, 2021 • edited Loading

Tests

Single user remote share

Group remote share

PVince81 commented Jul 1, 2021 • edited Loading

PVince81 commented Jul 1, 2021 • edited Loading

PVince81 commented Jul 1, 2021

PVince81 commented Jul 1, 2021

PVince81 commented Jul 2, 2021

PVince81 Jul 2, 2021

Choose a reason for hiding this comment

PVince81 commented Jul 2, 2021

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 2, 2021 • edited Loading

PVince81 commented Jul 27, 2021

PVince81 commented Jul 27, 2021

PVince81 commented Jul 27, 2021

PVince81 commented Jul 28, 2021

PVince81 commented Jul 28, 2021

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 1, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading

PVince81 commented Jul 2, 2021 •

edited

Loading