Add support for sending the password for a link share by Nextcloud Talk #11875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
933efa0 to
b4a5b7b
Compare
Merged
2 tasks
danxuliu
added
enhancement
3. to review
danxuliu
requested review from
rullzer,
schiessle,
MorrisJobke and
nickvergessen
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
b4a5b7b to
9e5440f
Compare
rullzer
approved these changes
Oct 30, 2018
MorrisJobke
added
4. to release
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
9e5440f to
aa6fc9e
Compare
|
CI failure due to the newly added "hide_download" setting ;) |
MorrisJobke
added
2. developing
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
aa6fc9e to
5edc746
Compare
"Someone" forgot to run the PHP tests before pushing... :-P Fixed now, and as an extra I also added some missing (minor) tests for notes and hide download ;-) |
|
CI failure is a segfault in the memcache tests -> ignore it. |
|
I will wait with the merge of this after I merged #11844 |
MorrisJobke
added
4. to release
The update share tests only checked that the share returned by "update()" had the expected values. However, as "update()" returns the same share that was given as a parameter the tests were not really verifying that the values were updated in the database. In a similar way, the test that checked that a password was removed did not set a password first, so even if the database returned null it could be simply returning the default value for the share; a password must be set first to ensure that it is removed. Besides that, a typo was fixed too that made the checks on the original share instead of on the one returned by "update()"; right now it is the same share, so the change makes no difference, but it is how the check should be done anyway. 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>
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
5edc746 to
86ad85c
Compare
|
@danxuliu The failing acceptance tests in here are the ones you mentioned that they also fail on master, right? |
When Talk is enabled the menu for link shares now shows a checkbox to protect the password by Talk (that is, to show the "Request password by Talk" UI in the authentication page for the link share). Although in e-mail shares protecting the share with a password and protecting the password by Talk are mutually exclusive actions (as when the password is set it is sent to the sharee, so it must be set again when protecting it by Talk to be able to verify the identity of the sharee), in the case of link shares protecting the password by Talk is an additional step to protecting the share with a password (as just setting the password does not disclose it to anyone). As such, the checkbox is shown only when there is a password set for the link share (even if the field itself for the password is not shown, like when they are enforced in the settings). Note that the icon set for the field, "icon-passwordtalk", does not currently exist; it is the same used for e-mail shares, and it is needed simply to get the right padding in the menu. 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>
danxuliu
force-pushed
the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
from
93feac7 to
f57b07e
Compare
|
I have fixed the rendering by making the view to render again only when the password changes; it is not optimal, but at least it does not render again the whole view whenever a link share changed (and thus respects the changes made in #11844 ;-) ).
Yes, fixed in #12215 |
rullzer
deleted the
add-support-for-sending-the-password-for-a-link-share-by-nextcloud-talk
branch
|
Especially considering this added yet another entry to the menu, I would really love your feedback on #12178 ;) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requires nextcloud/spreed#1273
Sending the password by Nextcloud Talk for a link share behaves slightly different than when doing it for an e-mail share. When an e-mail share is protected with a password an e-mail is sent with the password. Due to this, when the password is protected by Talk a new password must be always set; otherwise the recipient would already have the password, and thus she would not need to request it to the sharer and protecting it by Talk would be pointless.
However, in the case of link shares, just setting the password does not disclose it to anyone; the sharer must explicitly send it to someone to be known. Therefore, when protecting the password of a link share by Talk it is not mandatory to set a different password than the one set before; protecting the password by Talk in this case is an additional step to protecting the share with a password. Due to this the checkbox to protect the password by Talk is shown only when there is a password set for the link share.
Reviewers: Although the Talk counterpart is still in development state the server part is ready for review and can be merged now even if the Talk part is not ready yet; the missing parts in Talk are just better naming of rooms and a more useful text in the notifications, but besides that it just works (or, at least, it should :-) ).