Add support for room shares #10255

danxuliu · 2018-07-16T04:40:55Z

This pull request, with its Talk counterpart, makes possible to share a file or folder into a Talk room.

Due to the current architecture of the sharing system it is not enough to provide an IShareProvider to add a new type of share; the controllers for the sharing API also need to be modified to support each type of share. Due to this, and to keep the server isolated from the Talk details (for example, whether a file can be shared with certain type of room or not, or how to generate the display name for a room share), the controllers just delegate those checks or actions to helper classes defined in Talk.

Besides that, each type of share needs to be referenced throughout the server code in order to be taken into account for certain operations, like providing the share type in DAV properties or transferring the ownership of files and shares to a different user.

While implementing this I have found some issues in the existing sharing code (like some types of sharing not being audited, a strange behaviour moving received shares into other received shares, or some shares being wrongly deleted when transferring ownership due to their name); those issues will be fixed in other pull requests.

Also note that this pull request implements room shares, but not search for room collaborators (so even if room shares are implemented you can not test them from the UI); this is implemented in #10256

nickvergessen · 2018-07-16T08:53:32Z

core/js/share.js

@@ -12,6 +12,7 @@ OC.Share = _.extend(OC.Share || {}, {
 	SHARE_TYPE_CIRCLE:7,
 	SHARE_TYPE_GUEST:8,
 	SHARE_TYPE_REMOTE_GROUP:9,
+	SHARE_TYPE_ROOM:10,


Can we use TALK instead of ROOM
it reflects a bit more what the value is about, also as we stop using the term room in the UI it's better for other devs

I thought about using TALK, but in the end I opted for ROOM because I think it is more accurate, as it describes exactly the element that you are sharing with, and is consistent with the names of the rest of share types; I find TALK too abstract, and maybe in the future there will be other types of shares that can be done through Talk too. Opinions?

nickvergessen · 2018-07-16T09:09:35Z

lib/private/Share20/ProviderFactory.php

+	 * @return RoomShareProvider
+	 */
+	protected function getRoomShareProvider() {
+		if ($this->roomShareProvider === null) {


We could do an event here, so there is less Talk code in the server repo?

If we add an event here I think that it would make sense to move all the other providers to that generic system too, but if we do that then it would be something for a separate pull request.

danxuliu · 2018-07-24T09:23:23Z

Phan complains that:

apps/files_sharing/lib/Controller/DeletedShareAPIController.php:145 PhanUndeclaredClassMethod Call to method formatShare from undeclared class \OCA\Spreed\Share\Helper\DeletedShareAPIController
apps/files_sharing/lib/Controller/ShareAPIController.php:244 PhanUndeclaredClassMethod Call to method formatShare from undeclared class \OCA\Spreed\Share\Helper\ShareAPIController
apps/files_sharing/lib/Controller/ShareAPIController.php:920 PhanUndeclaredClassMethod Call to method canAccessShare from undeclared class \OCA\Spreed\Share\Helper\ShareAPIController

Those classes are used only when they exist (otherwise an exception is thrown, which is then caught and the code goes on happily); should I add a Phan exception or just change the declared return type to object?

MorrisJobke · 2018-07-24T09:37:37Z

should I add a Phan exception

This ;)

danxuliu · 2018-07-24T10:23:27Z

Oops, now that share notes were merged the unit tests need update; I will take care of that.

MorrisJobke · 2018-07-25T07:32:40Z

Oops, now that share notes were merged the unit tests need update; I will take care of that.

Looks like another round of rebase is needed 🙈

danxuliu · 2018-07-25T08:02:52Z

Looks like another round of rebase is needed 🙈

Done; it was only a matter of adjusting the ShareAPIController constructor due to the AppManager being already passed in #10238.

MorrisJobke · 2018-07-26T06:53:21Z

What is the status here? Either in the beta 1 or in 15.

nickvergessen · 2018-07-26T06:59:47Z

I fear to rsh in a workflow direction we might not want in the end
we had a long discussion at lunch yesterday how files + talk could work together and how people think stuff should work
at some point we also need to allow people to upload stuff in chat
which would create a folder for each room, and then things get very complicated pretty fast
also this approach does nto work for guests
so not sure if we should do this step, because it's not revertable

@schiessle mind to comment your opinion?

MorrisJobke · 2018-07-26T07:02:23Z

We should think also about the UX for where to store the files that are uploaded to rooms directly as well.

@nickvergessen mentioned to put them in one folder per room. Also where to do the guest uploads.

IMO this should be put into the app data directory. I think the deck app does this as well. Because those files are not related to the users file system, but to the app itself. cc @juliushaertl

danxuliu · 2018-07-26T11:17:17Z

I say merge this for Nextcloud 14.

This pull request simply makes possible to provide room shares. But it does not force us to provide room shares. As long as the RoomShareProvider and the helpers for the controllers are not included in Talk there is no support for room shares.

If this pull request is merged and it is later decided that room shares is not a good approach to share files in Talk there is no problem. The pull request is reverted for 15 and done. There would have been no provider, so no room shares would have been ever created, no database cleanup needed, no removed feature, nothing; just some code gone.

However, if this pull request is not merged and it is later decided that room shares is the way to go then there would be no chance to add them before 15. Instead, if the pull request is merged then there would be no problem in adding the support for Talk 4.1, 4.2 or something like that.

The only reason that this pull request is needed is because the sharing API is not yet abstract enough for apps to provide new share types without touching the server.

Regarding uploading files to chats, my idea is based in room shares (obviously :-P ). All the files in chats would end inside the Talk folder (configurable name, like the share folder). When the user uploads a file in a chat the file would be uploaded to that folder (or maybe a Sent subfolder) and automatically shared with the room; the other participants in the room would be able to access that file from their own Talk folder (or maybe a Received subfolder).

In any case this would simply need to add a new mount provider in Talk based on OCA\Files_Sharing\MountProvider and OCA\Files_Sharing\SharedMount. Besides the benefit of reusing code it also has the benefit of behaving in a consistent way with other shares, although slightly adapted to this specific use case.

In the Talk UI then it would be possible somehow, maybe in the sidebar, to get a view of all the files uploaded/shared in a specific conversation (which using room shares would be trivial, just search all the files shared with the room and done, even if they were moved to another folder).

Also, in the same way that the Files app provides a special view for shares, it could also provide a special view that groups the files by folders with the name of the conversation. Or simply upload and receive them in a subfolder of Talk with the name of the conversation. I guess that as long as the folder id is not modified the clients would be clever enough to just rename them when syncing again if the conversation was renamed since the last sync.

Finally, regarding guest users, my idea was to create a public shared link for a file when it was shared with a public room. However I have not added it yet because currently it is not possible to create more than one share link for a file; I felt that it would be better to wait until that is supported so a read-only share link could be created for guests of the room without preventing the user to also create an editable link by herself.

To allow guest users to also upload files in the chat my idea was to somehow connect a public shared folder with a room, although I have to admit that I have not thought about it yet.

In any case, even if you see a lot of problems with all that, this pull request does not force us to implement anything of it. Again, it makes possible to do it in a later version of Talk, but if those ideas end being scrapped then there is no problem if this pull request was merged; just revert it and everything will be fine.

schiessle · 2018-07-31T08:27:55Z

Where will guests upload files?

Maybe even more important question for the first iteration: How can a guest access files shared to the room? Will we create public links for guests? How does the guest receives the link?

danxuliu · 2018-07-31T11:25:37Z

Maybe even more important question for the first iteration: How can a guest access files shared to the room?

For the first iteration my idea was that guests could not access files shared with the room. Having said that...

Will we create public links for guests?

I guess that we could simply create a token for shares with a public room so they can be downloaded through a public link (like mail shares or link shares). But this may need some changes in the server.

Alternatively we could automatically create a public link share when a file was shared with a public room. However it would have the problem mentioned above about not being able to create more than one link share for a file, and I think that the other approach would be much cleaner.

How does the guest receives the link?

When a file is shared with a room a new message will appear in the room (it is not implemented yet, but it only requires changes in Talk). The message will be parsed by the Web UI and it will show the appropriate URL to download the file depending on whether the participant is a registered user or a guest.

schiessle · 2018-08-01T15:24:21Z

I guess that we could simply create a token for shares with a public room so they can be downloaded through a public link (like mail shares or link shares). But this may need some changes in the server.
Alternatively we could automatically create a public link share when a file was shared with a public room. However it would have the problem mentioned above about not being able to create more than one link share for a file

As it will not be a link share but a "room share" your own share provider you should be freed from the limitation that there is only one public link allowed per file. We do the same for mail shares. Technically it is exactly the same as a link share but because it is a different share type the limitation doesn't kick in.

Btw, this was the initial, super simple idea to just create such a link and post it to the chat so that everyone can access it. Your implementation is far more advanced, with a better user experience for internal users but with the drawback that it doesn't work for guests.

danxuliu · 2018-08-01T17:58:24Z

Your implementation is far more advanced, with a better user experience for internal users but with the drawback that it doesn't work for guests.

Not any longer ;-) I have added tokens to room shares, so now room shares for public rooms can also be accessed by guests.

This type represents shares with a Nextcloud Talk conversation. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The RoomShareProvider is provided by the Talk app, so it is necessary to check whether the app is available or not, and also whether the class itself exists or not (just in case an older version of the app that did not have support yet for room shares is being used). Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Room shares are implemented in an external app (Nextcloud Talk), so in order to keep the share manager as isolated as possible from room share specifics all the validity checks are done in the provider of room shares. However, due to the code structure it is necessary to explicitly check for room shares in "generalCreateChecks" to prevent an exception from being thrown. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

In some cases, the ShareAPIController requires explicit handling of each type of share (for example, to format a share for a DataResponse). Room shares are implemented in an external app (Nextcloud Talk), so in order to keep the controller as isolated as possible from room share specifics all that explicit handling is done in a helper class provided by the Talk app. In other cases it is just enough to call the share manager specifying a room share type; note that the share manager is guarded against share types for which there is no provider, so it is not necessary to explicitly check that before passing room shares to the share manager. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

In some cases, the DeletedShareAPIController requires explicit handling of each type of share (for example, to format a share for a DataResponse). Room shares are implemented in an external app (Nextcloud Talk), so in order to keep the controller as isolated as possible from room share specifics all that explicit handling is done in a helper class provided by the Talk app. In other cases it is just enough to call the share manager specifying a room share type; note that the share manager is guarded against share types for which there is no provider, so it is not necessary to explicitly check that before passing room shares to the share manager. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The test just ensures that the controller will gracefully reject the creation instead of failing miserably; the integration tests when Talk is enabled are in the Talk repository. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The MountProvider for shares creates mount points for the files shared with the user, which makes possible to use the received shared files and folders as regular files and folders. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

A user can move her own shares into a received share. When that happens she is effectively handing over the ownership of the file, so the share needs to be updated to reflect the new owner. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Like done for other types of shares, room shares are now explicitly described as such in the UI. The avatar used is the image provided in the "shareWithAvatar" property of the share. If none is given then the avatar is the first letter of the display name of the room share with a coloured background seeded from the room token. If the display name of the room is empty then no letter is shown in the avatar; no special handling is done in that case. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Like done with group shares, received room shares are described as such in the UI. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

The DeletedShareAPIController and ShareAPIController helpers for room shares are defined in Talk, so the classes do not exist when Talk is not installed. Due to this when the object returned by "getRoomShareHelper" is used Phan complains that the class is not declared. This is not a problem, though, because when the class is not available "getRoomShareHelper" throws an exception, which is then caught where that method was called. Therefore now those warnings from Phan are suppressed (it would be better to use "@phan-suppress-next-line" instead, but it is not yet available in our Phan version). Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Tokens will be used to give access to a share to guests in public rooms. Although the token itself is created in the provider of room shares and no changes are needed for that, due to the code structure it is necessary to explicitly call the provider from the manager when getting a room share by token. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

rullzer

Ok lets do this 🐘

danxuliu added enhancement 3. to review Waiting for reviews feature: sharing labels Jul 16, 2018

danxuliu added this to the Nextcloud 14 milestone Jul 16, 2018

This was referenced Jul 16, 2018

Add support for room shares nextcloud/spreed#1050

Merged

Include rooms in the list of candidates to share with #10256

Merged

danxuliu requested review from rullzer, nickvergessen, MorrisJobke and schiessle July 16, 2018 04:46

danxuliu force-pushed the add-support-for-room-shares branch 2 times, most recently from 0006684 to d9d2cc2 Compare July 16, 2018 08:04

nickvergessen reviewed Jul 16, 2018

View reviewed changes

danxuliu force-pushed the add-support-for-room-shares branch from d9d2cc2 to aa5c1d1 Compare July 16, 2018 16:45

danxuliu force-pushed the add-support-for-room-shares branch from aa5c1d1 to db94579 Compare July 23, 2018 20:13

MorrisJobke mentioned this pull request Jul 24, 2018

14.0.0 Beta 1 #10370

Merged

21 tasks

danxuliu force-pushed the add-support-for-room-shares branch from 83aab19 to cd65e3e Compare July 24, 2018 11:31

danxuliu force-pushed the add-support-for-room-shares branch from cd65e3e to a693e11 Compare July 25, 2018 07:59

MorrisJobke modified the milestones: Nextcloud 14, Nextcloud 15 Jul 26, 2018

rullzer mentioned this pull request Aug 2, 2018

Nextcloud 14 Beta 3 #10502

Closed

58 tasks

danxuliu added 16 commits August 8, 2018 14:25

Add type for room shares

7292a98

This type represents shares with a Nextcloud Talk conversation. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Add comment with IDs of internal share types

857bb45

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Add room shares to DAV and recent files "share-types" property

523fdb6

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Transfer room shares too with the "files:transfer-ownership" command

30d8e3e

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Log sharing and unsharing with a room in the auditing app

0fab46c

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Add custom handling for room shares to the resharer information

e1561f0

Like done with group shares, received room shares are described as such in the UI. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu force-pushed the add-support-for-room-shares branch from a0c1f49 to 4b7fa4a Compare August 8, 2018 12:51

rullzer approved these changes Aug 9, 2018

View reviewed changes

schiessle approved these changes Aug 9, 2018

View reviewed changes

schiessle merged commit 6aa6d21 into master Aug 9, 2018

hanzei modified the milestones: Nextcloud 15, Nextcloud 14 Aug 9, 2018

nextcloud-bot mentioned this pull request Aug 16, 2018

Add AppLogger #10725

Closed

nextcloud-bot mentioned this pull request Oct 9, 2018

Dav endpoint: add share note #11702

Closed

juliusknorr mentioned this pull request Dec 8, 2020

Add deck share provider support #24605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for room shares #10255

Add support for room shares #10255

danxuliu commented Jul 16, 2018 •

edited

Loading

nickvergessen Jul 16, 2018

danxuliu Jul 16, 2018

nickvergessen Jul 16, 2018

danxuliu Jul 16, 2018

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 25, 2018

danxuliu commented Jul 25, 2018

MorrisJobke commented Jul 26, 2018

nickvergessen commented Jul 26, 2018

MorrisJobke commented Jul 26, 2018

danxuliu commented Jul 26, 2018

schiessle commented Jul 31, 2018

danxuliu commented Jul 31, 2018

schiessle commented Aug 1, 2018

danxuliu commented Aug 1, 2018

rullzer left a comment

Add support for room shares #10255

Add support for room shares #10255

Conversation

danxuliu commented Jul 16, 2018 • edited Loading

nickvergessen Jul 16, 2018

Choose a reason for hiding this comment

danxuliu Jul 16, 2018

Choose a reason for hiding this comment

nickvergessen Jul 16, 2018

Choose a reason for hiding this comment

danxuliu Jul 16, 2018

Choose a reason for hiding this comment

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 25, 2018

danxuliu commented Jul 25, 2018

MorrisJobke commented Jul 26, 2018

nickvergessen commented Jul 26, 2018

MorrisJobke commented Jul 26, 2018

danxuliu commented Jul 26, 2018

schiessle commented Jul 31, 2018

danxuliu commented Jul 31, 2018

schiessle commented Aug 1, 2018

danxuliu commented Aug 1, 2018

rullzer left a comment

Choose a reason for hiding this comment

danxuliu commented Jul 16, 2018 •

edited

Loading