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

Add support for room shares #1050

Merged
merged 43 commits into from
Aug 24, 2018
Merged

Add support for room shares #1050

merged 43 commits into from
Aug 24, 2018

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Jul 16, 2018

Talk part of nextcloud/server#10255

First of all sorry for the massive size of this pull request. If it is any comfort to you it is basically a big specification of integration tests with some code in between :-P

This pull request, with its server counterpart, makes possible to share a file or folder into a Talk room. The integration test will need to be launched again once the server part is merged.

Room shares are heavily based on (read, copy-pasted from :-P ) group shares, although with some differences. In the same way that a user must be a participant of a room to send a message to it a user must be a participant of a room to share a file with it (in the case of group shares enforcing that is enabled in the settings). Similarly, when a user leaves a room she has no longer access to the shares in the room; for consistency, her own shares are also removed.

This Talk part adds a new IShareProvider, RoomShareProvider, which provides the core operations (create, update, delete...) for each share type. To try to make it more manageable for review each operation is added incrementally in its own commit, followed by commits that add the integration tests that verify that operation.

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.

Note that this pull request just adds the core implementation of room shares. Other features, like sending a message in the chat to inform the rest of the users that a file was shared, show the name of the participants as the display name of the share when the room has no name set, or making possible to share files from the Talk UI itself will be added in other pull requests. In any case, all those changes are Talk specific; no changes are needed in the server for them.

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 #1051

@nickvergessen
Copy link
Member

In the same way that a user must be a participant of a room to send a message to it a user must be a participant of a room to share a file with it (in the case of group shares enforcing that is enabled in the settings).

Just to be sure I understood this correctly: you must only be able to share to rooms you are part of

/** @var Room $room */
$room = $event->getSubject();

$users = $room->getParticipants()['users'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, the event is missing the information who is disconnecting?!
Let's fix that instead of doing such crazy stuff in here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I tried to fix the event data, but ended with the current approach. I have no idea why... but I hope that I had a good reason for that hack :-P

* @param IShare $share
* @return array
*/
public function formatShare(IShare $share): array {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shared with DeletedShareAPIController extract into a base class?

* @param int $itemSource
* @param string $target
* @param int $permissions
* @param DateTime|null $expirationDate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\DateTime

* @return IShare
*/
private function createShareObject($data) {
$share = new Share($this->rootFolder, $this->userManager);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCP\Share\IManager::newShare() instead of direct access to a private class

@danxuliu danxuliu force-pushed the add-support-for-room-shares branch 2 times, most recently from 9d32bfa to d2872a9 Compare July 18, 2018 15:32
@nickvergessen nickvergessen modified the milestones: Next Major, backlog Jul 27, 2018
@danxuliu danxuliu force-pushed the add-support-for-room-shares branch 2 times, most recently from 370728b to 7ecb121 Compare August 8, 2018 16:00
The methods of the helper class are called from the main
\OCA\Files_Sharing\Controller\ShareAPIController class to perform
actions or checks specific to room shares, thus keeping the main
ShareAPIController of the server isolated from the Talk details.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The methods of the helper class are called from the main
\OCA\Files_Sharing\Controller\DeletedShareAPIController class to perform
actions or checks specific to room shares, thus keeping the main
DeletedShareAPIController of the server isolated from the Talk details.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
This forces the users and groups to be created again from scratch as
needed for each new scenario, thus preventing actions performed in a
previous scenario, like creating a new folder, from affecting the
following scenarios.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
In order to keep the share manager as isolated as possible from room
share specifics the validity checks are done in RoomShareManager.

Getting room shares by path was also implemented, as it is needed to
prevent creating again a share for the same path and room.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Note that these tests require some "getShareXXX" methods to be
implemented in the provider, as even if they are not explicitly used
they are required by the code paths to create shares.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
danxuliu and others added 24 commits August 24, 2018 16:23
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When an expierd share is got that share is deleted. Due to this these
tests could not be added until now that deleting shares is implemented.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When a user is removed from a room by any means all the shares owned and
received by that user are removed.

When a room is deleted all the shares in that room are deleted.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Besides the SharesPlugin for DAV, these tests exercise getting room
shares in a folder (as that plugin is the only place where that is
used).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Room shares have no token, so it is not possible to get a room share by
token.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
These tests do not check anything new in the RoomShareProvider, but on
other code paths.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
These tests do not check anything new in the RoomShareProvider, but on
other code paths.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Tokens allow guest users to download the files shared with a public
room; although every room share has a token only those with public rooms
can be accessed using the token.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Only shares with public rooms can be accessed using the token, so there
is no point in returning the token for shares with other types of rooms.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants