Add support for sending the password for a share by Nextcloud Talk #10238

danxuliu · 2018-07-13T16:01:02Z

When a file is shared by email the file can be accessed by anyone as long as the token of the share is known, so to limit that access the share can be protected with a password. When a password was set it was also sent by email to the sharee. This pull request (and its Talk counterpart) adds another option: sending the password with Nextcloud Talk.

Now when the sharer protects the share with a password no email is sent. Instead, when the receiver tries to open the share the public share authentication page is shown, which now contains a Request password button. When that button is clicked an embedded Nextcloud UI is shown and a conversation is started between the sharer and the user trying to open the file, which allows the sharer to verify the identity of the user before telling her the password and thus granting her access to the file.

Note that it is the sharer who decides whether the receiver of an email share can call her or not; you will not be bothered with password requests if you do not choose to ;-)

This pull request updates the server parts needed to provide that feature, which are basically those related to shares; the handling of rooms and the Talk UI is isolated from the server and done in the Talk app. The "bridge" between the public share authentication page and the Talk UI is the share controller, that simply appends a Talk template when sending the password by Talk is enabled for the share. That template just loads some JavaScript code that, when run on the browser, adjusts the page generated by the server to inject the Talk UI as needed.

Regardind the share menu, a new item was added to the menu, Password protect by Talk, that behaves like the previous Password protect; sending the password by mail and sending it by Talk are mutually exclusive actions, so when one of the checkboxes is enabled the other one is automatically disabled (radio buttons may have been better, but it would be strange to have radio buttons mixed with the rest of checkboxes in the menu).

When switching from sending by Talk to sending by email the previous password is reused; however, when switching from sending by email to sending by Talk a new password must be provided, and the share will not be updated until that is done.

For reviewers, note that in ShareAPIController the ServerContainer is injected and then the AppManager is got from the container, instead of directly injecting the AppManager; this is on purpose, as the ServerContainer will be necessary for another pull request in the works ;-)

Also note that when a password is set in the list of shares the loading icon is misplaced; that happens too in current master and is not related to this pull request.

nickvergessen

Hmm

nickvergessen · 2018-07-13T16:18:07Z

core/templates/publicshareauth.php

@@ -25,3 +25,7 @@ class="svg icon-confirm input-button-inline" value="" disabled="disabled" />
 		</p>
 	</fieldset>
 </form>
+
+<?php if ($_['sendPasswordByTalk']): ?>
+	<?php echo (new \OCP\Template('spreed', 'publicshareauth'))->fetchPage($_['sendPasswordByTalk']); ?>


Cross app reference

Should also be possible to add an event in the relevant controller.
Talk could then load a JS script which creates the UI instead?

I do not know if the template system provides any event to add more content to a page. Even if all the markup was generated using JavaScript in the browser at the very least the page would need to be extended with the links in the header to load the scripts.

Well basically it should work like OCA\Files::loadAdditionalScripts and the first run wizard

Mmm, I think that would not work here. The reason is that, besides the scripts themselves, the handler of the event has to add some HTML elements too (in order to provide the share token in a data property). As far as I know currently it is not possible to add arbitrary elements to be rendered.

I think that the template system would have to be extended to add another template class that allows rendering subtemplates (like the PublicTemplateResponse and the IMenuAction, but generic). Or am I missing something?

As far as I know currently it is not possible to add arbitrary elements to be rendered

you can do this quite easily?
$('form.searchbox').after($('<div>').attr('id', 'notifications'));

Yes, adding an element from the Talk script is no problem (that is what it is done already for the sidebar). The problem is injecting the share token, which should be done when the template is rendered, not when the JavaScript is executed.

OK, that's it, I'm officially dumb xD I have just realized that I can simply add a hidden input field in the publicshareauth template with the share token, just like it is done in the public share template; there is no real need to inject it only when Talk is used.

MorrisJobke · 2018-07-13T16:36:55Z

For reviewers, note that in ShareAPIController the ServerContainer is injected and then the AppManager is got from the container, instead of directly injecting the AppManager; this is on purpose, as the ServerContainer will be necessary for another pull request in the works ;-)

Not really nice IMO 🙈

apps/files_sharing/lib/Controller/ShareAPIController.php

apps/files_sharing/lib/Controller/ShareController.php

core/Migrations/Version14000Date20180710092004.php

core/js/sharedialogshareelistview.js

nickvergessen · 2018-07-16T09:28:44Z

core/templates/publicshareauth.php

@@ -25,3 +25,7 @@ class="svg icon-confirm input-button-inline" value="" disabled="disabled" />
 		</p>
 	</fieldset>
 </form>
+
+<?php if ($_['sendPasswordByTalk']): ?>
+	<?php echo (new \OCP\Template('spreed', 'publicshareauth'))->fetchPage($_['sendPasswordByTalk']); ?>


Should also be possible to add an event in the relevant controller.
Talk could then load a JS script which creates the UI instead?

rullzer

Looks good. 🐘
I'd say lets merge it and iron out tiny bugs later.

nickvergessen

Fine by me too
No direct code call anymore

No direct code call anymore

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

When a password was set for a mail share an e-mail was sent to the recipient with the password. Now the e-mail is no longer sent if the password is meant to be sent by Talk. However, before the e-mail was not sent when the share was updated but the password was not changed. Now an e-mail is sent in that case too if switching from a password sent by Talk to a password sent by mail. On the other hand, when switching from a password sent by mail to a password sent by Talk it is mandatory to change the password; otherwise the recipient would already have access to the share without having to call the sharer to verify her identity. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

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

Before the public share authentication page is rendered now an event to load additional scripts is dispatched. Thanks to this any app can load its own scripts that, when run on the browser, adjust as needed the page generated by the server. Note, however, that during the handling of the event apps are only able to add scripts or styles to be loaded; they can not render arbitrary content on the page, or change how the content is rendered by the original template; all those changes have to be done by the scripts at run-time. This implies that the scripts of the apps can use only those parameters, like the token of the share, added to the page when it is generated by the "publicshareauth" template. Due to this, and given that the event is being introduced to be used by Talk to inject the UI needed to request the password for a share, the token of the share is now provided in the generated page, just like done in the public share page. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

danxuliu · 2018-07-24T12:17:17Z

I have rebased onto master to fix the merge conflicts. Note that now this pull request adds a migration with a previous date than another one already in master, but given that the version is bumped too according to @nickvergessen it should not be a problem.

Until now the password to be sent by mail was set by enabling a checkbox in the share menu and then entering the password in an input field shown below. Now, when Talk is enabled, another item is added to the share menu to do the same for a password to be sent by Talk. Sending the password by mail and sending it by Talk are mutually exclusive actions, so when one of the checkboxes is enabled the other one is automatically disabled. Note that the icon set for the field, "icon-passwordtalk", does not currently exist; it simply mimics the "icon-passwordmail" (which does not exist either) used for the field of the password protect by mail to get the right padding in the menu. Signed-off-by: Daniel Calviño Sánchez <[email protected]>

MorrisJobke · 2018-07-24T14:31:28Z

For me there is now no such sharing option anymore :/

MorrisJobke · 2018-07-24T14:56:40Z

Server side looks good, but the talk app seems to be a bit broken:

danxuliu · 2018-07-24T15:04:06Z

the talk app seems to be a bit broken

Known issue unrelated to this pull request; it was broken by #9982

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

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

danxuliu mentioned this pull request Jul 13, 2018

Add support for sending the password for a share by Nextcloud Talk nextcloud/spreed#1049

Merged

danxuliu requested review from schiessle, nickvergessen and MorrisJobke July 13, 2018 16:01

nickvergessen reviewed Jul 13, 2018

View reviewed changes

danxuliu force-pushed the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch 2 times, most recently from 20d46ac to 02d6e55 Compare July 16, 2018 07:00

nickvergessen previously requested changes Jul 16, 2018

View reviewed changes

danxuliu force-pushed the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch from 02d6e55 to 7a29e67 Compare July 17, 2018 17:30

rullzer approved these changes Jul 18, 2018

View reviewed changes

danxuliu force-pushed the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch 2 times, most recently from 381f5db to 8a6514c Compare July 18, 2018 16:18

nickvergessen reviewed Jul 19, 2018

View reviewed changes

MorrisJobke mentioned this pull request Jul 24, 2018

14.0.0 Beta 1 #10370

Merged

21 tasks

danxuliu added 6 commits July 24, 2018 13:56

Add "password_by_talk" column to "share" table in the database

49fd17f

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

Add "sendPasswordByTalk" property to shares

88600f4

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

Store "sendPasswordByTalk" property of mail shares in the database

dd0c5e2

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

Add support for sending the password by Talk to ShareAPIController

7849630

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

danxuliu force-pushed the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch from 8a6514c to cd6d99c Compare July 24, 2018 12:06

danxuliu force-pushed the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch from cd6d99c to 9110935 Compare July 24, 2018 12:50

MorrisJobke approved these changes Jul 24, 2018

View reviewed changes

MorrisJobke merged commit 9b9d729 into master Jul 24, 2018

MorrisJobke deleted the add-support-for-sending-the-password-for-a-share-by-nextcloud-talk branch July 24, 2018 15:07

danxuliu mentioned this pull request Jul 25, 2018

Add support for room shares #10255

Merged

This was referenced Oct 1, 2018

nextcloud 14.01 link sharing:: password setting vs. Safari 12 #11517

Closed

Enforce Password & expiration date on mail sharing #11547

Closed

danxuliu mentioned this pull request Oct 26, 2018

Add support for sending the password for a link share by Nextcloud Talk #11875

Merged

nextcloud-bot mentioned this pull request Nov 16, 2018

App support for Nextcloud 15 #12480

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sending the password for a share by Nextcloud Talk #10238

Add support for sending the password for a share by Nextcloud Talk #10238

danxuliu commented Jul 13, 2018 •

edited

Loading

nickvergessen left a comment

nickvergessen Jul 13, 2018

nickvergessen Jul 16, 2018

danxuliu Jul 16, 2018

nickvergessen Jul 16, 2018

danxuliu Jul 16, 2018

nickvergessen Jul 17, 2018

danxuliu Jul 17, 2018

danxuliu Jul 17, 2018

MorrisJobke commented Jul 13, 2018

nickvergessen Jul 16, 2018

rullzer left a comment

nickvergessen left a comment

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

danxuliu commented Jul 24, 2018

Add support for sending the password for a share by Nextcloud Talk #10238

Add support for sending the password for a share by Nextcloud Talk #10238

Conversation

danxuliu commented Jul 13, 2018 • edited Loading

nickvergessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Jul 13, 2018

Choose a reason for hiding this comment

rullzer left a comment

Choose a reason for hiding this comment

nickvergessen left a comment

Choose a reason for hiding this comment

danxuliu commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

MorrisJobke commented Jul 24, 2018

danxuliu commented Jul 24, 2018

danxuliu commented Jul 13, 2018 •

edited

Loading