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 sending the password for a link share by Nextcloud Talk #1273

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Oct 26, 2018

Requires #1267
Required by nextcloud/server#11875

Regarding Talk the differences between protecting the password of an e-mail share or a link share are minimal; despite that, this pull request is required by the server changes. The reason is that link shares do not have a value in their sharedWith field (when got with the OCS enpoint they do, although it is set by the ShareAPIController, internally they have none), so without those changes when the room for the link share was created the name of the room was set to null. When the call activity is generated the room name is expected to be a string, but as the name was null PHP complained, no proper HTTP response was sent and the UI broke.

TODO:

  • The room name should be fixed. Currently it is set to the e-mail address or the target of the share (depending on the type of share), and in the RoomController it is composed and returned as Password request by {roomName}.

    This was done to show the room name in the language of the sharer, but now that the name needs to be different depending on the share type it would require to get the share object to check its type every time a room is formatted.

    It would be better to set the name once (and update it when needed, for example if the file name of a link share changes) than adding those extra queries, for example, every time that the list of rooms is got.

  • The notification sent for a link share should probably include the name of the file in its message; Someone requested the password to access a share is too vague.

@danxuliu danxuliu added 2. developing enhancement feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents labels Oct 26, 2018
@danxuliu danxuliu added this to the 💚 Next Major milestone Oct 26, 2018
@danxuliu danxuliu force-pushed the add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk branch from bc3443f to 0a8dce9 Compare November 7, 2018 17:16
@danxuliu
Copy link
Member Author

danxuliu commented Nov 7, 2018

@nickvergessen Could you take over this to fix the pending issues? Thanks :-)

@nickvergessen nickvergessen self-assigned this Nov 7, 2018
@nickvergessen
Copy link
Member

I think it is good as is now

@danxuliu
Copy link
Member Author

danxuliu commented Nov 8, 2018

Mmm, when the file is added to the notification clicking on the notification to go to the call is no longer possible using the Notification app in the WebUI. This happens because the file template generates a link to the file, and the link for the full subject is used only when the subject has no links. In this case the link to the call is much more relevant than the link to the file, so... I am not sure what to do with that :-) Do as you wish ;-)

Regarding the room name I would prefer to keep the "namespaced" object types. I guess that they were changed to be able to use one text or the other in the RoomController without having to query the database for the share object every time that the room name is got. However, I would use share:password:mail and share:password:link instead of share_mail and share_link; once rooms in public share pages are introduced I expected to use something like share:public to differentiate them from share:password rooms.

However, I would rather keep the object type as share:password and add another field like objectData that in this case would include the type of the share, the name of the file and the e-mail address (when applicable) and compose the name based on those fields; this would make possible to provide more descriptive room names specifying both the e-mail and the file, just like in the notifications. Or, to keep things simple, just get the share from the database when the room name is formatted; after all those rooms exist only when there is a password request in progress, so it should not be too bad for the performance (I hope :-P ).

Another even simpler approach would be to use a generic text like Password request: XXX where XXX is the e-mail or the file name, instead of Password request by {email} and Password request for {file}; it would not have the more descriptive name with both the e-mail and file name, but after all there is not much space in the room list for long names, and this would make possible to keep the current share:password types while being fast and simple.

Besides all that, I have noticed that the file target of a share is not updated when the file name changes (I do not know if that is the expected behaviour or a bug in the server), so if the sharer renames the file after enabling the password request by Talk the room name will show the old file name (due to being based on the file target of the share); notifications are fine, as they get the file object everytime they are generated.

Also, the room name shown in the sidebar is the raw room name, that is, just the e-mail address or just the file name, without the formatting to add the Password request part. This is caused by using the name attribute instead of the displayName; for share:password rooms the attribute to use should be displayName and the edition should be always disabled. I will fix that in a different pull request, as this happens too in Talk 4.0 with e-mail shares.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@nickvergessen nickvergessen force-pushed the add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk branch from 481a9e6 to 0dbf1e0 Compare November 8, 2018 12:54
danxuliu and others added 3 commits November 8, 2018 20:59
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Until now only the e-mail shares had support for sending the password by
Talk. In Nextcloud 15 that feature was added to link shares too, so the
room name and the notification sent for "share:password" rooms has to be
adjusted accordingly.

The display name of "share:password" rooms is generated from the raw
name of the room (the e-mail for mail shares and the file name for link
shares) each time the room information is sent by the server, so the
display name was generalized to accomodate both types of raw names.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@danxuliu danxuliu force-pushed the add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk branch from 0dbf1e0 to ad22021 Compare November 8, 2018 21:16
@danxuliu
Copy link
Member Author

danxuliu commented Nov 8, 2018

I have squashed the room name fix into the original commit to add support for link shares, and I have added another commit to remove an unused attribute from PublicShareAuthController.

@nickvergessen No notification is sent when using an undefined type for the rich object parameter (last commit); the problem seems to be that when getting an undefined type an exception is thrown, so the validation of the parameter fails, and with this the validation of the notification, and thus the notification ends being ignored.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk branch from ad22021 to 876165b Compare November 9, 2018 12:31
@danxuliu
Copy link
Member Author

Tested and works 👍

@nickvergessen nickvergessen merged commit 95cce7d into master Nov 12, 2018
@nickvergessen nickvergessen deleted the add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk branch November 12, 2018 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement feature: talk-sidebar ⬅️ Sidebar integration of Talk into other apps like sharing and documents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants