@peterhashair opened this Pull Request on September 5th 2022 Contributor

Description:

Fixes: #19625

Add copy link after the token has been sent for the user who doesn't have an email set up from design.

  • As per the design, add a popup to resend invites.
  • As suggested, add an additional database field for the link token, and make 2 option works.
  • Add a new API endpoint.
  • Add 2 new Icon, arrow left and arrow right
  • As suggested, when over HTTPS using js WriteText otherwise executeCommand('copy'), there is still a security risk in WriteText, when failed, show the whole link.

This problem needs to update docs as well.

Review

@peterhashair commented on September 9th 2022 Contributor

@justinvelluppillai any suggestion on that one? Should I go back to @Javi-Ormaechea?

@peterhashair commented on September 13th 2022 Contributor

Notes: I believe once that merge we need to update the docs here https://matomo.org/faq/general/manage-users/

@justinvelluppillai commented on September 15th 2022 Member

@tsteur do you have any thoughts as security person on https://github.com/matomo-org/matomo/pull/19707#pullrequestreview-1107407965? We have included all security and UX/UI suggestions received so far, be good to know if there are any raised from @sgiehl that you'd like added here also.

@justinvelluppillai commented on September 15th 2022 Member

Meantime @peterhashair please proceed to address @sgiehl's other review comments, they all seem important.

@tsteur commented on September 15th 2022 Member

From what I see the password confirmation check is still there so this should be all good 👍

That way of storing two token actually also contains a "small" security risk, as we still only have one expiry time. So sending out an email or copying a new link would magically extend the expiration of the other token. Which might be unexpected and less secure.

It's tricky.

  • We could not extend the expire time when clicking copy link. This could be fine as long as we show information for how long that link is valid after copying it. It's actually currently also not clear that clicking "Resent invite" will extend the 7 days. At least the wording doesn't suggest that clearly.
  • Or we still extend the time but then make it more clear that any action will extend the allowed invitation time. We just need to be clear about it at least.

In terms of security we currently also only ask for a password when the user is initially invited. That makes it impossible for someone gaining access to an admin account (without knowing the password) to create a new user. But the feature to copy a new invite link currently does not ask for a password. So if there is an invited user, that has not yet accepted, an attacker would easily be able to take over that account by simply copying the invite link, without anyone noticing.

It's a hard one. I would say if we're in doubt / unsure we should decide pro security. Especially since the action may be executed rarely and it's not a standard flow I would recommend asking for password confirmation to copy the link.

fyi getting this error in the UI (not sure if it's known)

image

Unrelated: Not sure if it's important but the header still says "Add new user" when you're actually inviting a new user. It could give the indication that a user is added directly maybe. Not needed to be changed though, just an FYI that this is maybe not quite clear. Especially for existing people where they maybe don't read the buttons as they've been using them many times before.

image

I then couldn't find the feature to get the link but eventually found it by clicking on "Resend invite". I don't know what's been discussed so far and don't want to make anything more complicated. Was just thinking it could be useful if it said Not received the invite? Resend invite or copy activation link or something. Just a thought.

@peterhashair commented on September 17th 2022 Contributor

Update the PR for the following, just random thoughts unrelated. I would suggest, that we actually remove copy link, it may cause other issues later on (like copy function in js won't work). instead, we could give 2 actions which are create a user and invite a user. create a user goes to the old way, admin/super admin enters the password for the client. 'Invite User' only available when a client has email_enabled turned on.

  • Updated the resend and copy link, they both required a passwords confirmation to trigger the action, if the password was wrong, the popup will remain open until the correct password is entered

  • add invite action notes, any action will extend the invite link to x days

@peterhashair commented on September 21st 2022 Contributor

As @Javi-Ormaechea requested, update the wording. It seems like our UI tester deny clipboard access, so leave the screen as that copy denied, and in order to prevent random failure with the random token, hide the token link.

I believe there is a security risk that another site can access the clipboard which accesses the token link, the chance is rare. I think the clipboard is not meant to copy passwords etc.

I would suggest, giving up this PR, recreating an Issue that uses invite user for our cloud and those who have email setup, and create user as it was (admin enter password) for the user who doesn't have an email setup.

@peterhashair commented on September 29th 2022 Contributor

@justinvelluppillai should be fixed now.

@bx80 commented on September 30th 2022 Contributor

This is my first look at this PR, it does work :+1: and I'm aware there have already been a lot of points raised in the discussion above, I'll try to avoid rehashing previously covered comments.

a) If I want to create a new user and send them their invite link on slack, then I'd probably end up doing this:

1) Click 'Invite a new user' (goes to a screen labeled 'Add a new user')
2) Click 'Invite User'.
3) Enter my password, then 'Invitation is sent' is shown.
4) Go back to the user list.
5) Search for the newly invited user (what was their email again? Type it in...)
6) Click on an envelope row icon ("but I don't want to email, I want to copy...")
7) Click 'Copy link' on the popup.
8) Enter my password again (second time in 30 seconds :slightly_frowning_face: )
9) Finally the invite link is in the clipboard.

I did this a couple of times before I realized that the 'Resend Invite' link shown in step 3) would allow me to jump to step 7). So although I saw 'Resend Invite' link, it didn't register because I want to copy the link. I never wanted to send an invite in the first place, so wasn't intuitive.

Could we either rename this to 'Copy Invite Link' or duplicate the link so we have 'Resend invite' and 'Copy invite link'?

b) Once the 'copy link' button is used the buttons are disabled but it isn't obvious that they no longer work. If I accidentally copied something else into my clipboard and lost the copied link then clicking the 'Copy link' button again has no effect. Wouldn't it be better to hide the buttons altogether and/or replace them with a close button?

c) Minor cosmetic: The resend invite popup box seems to have a different font size from the normal popups? The password popup font is much smaller, it looks like part of a different application.

d) Minor point: It might be slightly clearer to say 'Link copied to the clipboard' rather than just 'Link copied'.

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

@peterhashair commented on September 30th 2022 Contributor

@bx80 I agree.

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

I guess this has come to the design question. @Javi-Ormaechea

Minor point: It might be slightly clearer to say 'Link copied to the clipboard' rather than just 'Link copied'.

@bx80 commented on September 30th 2022 Contributor

e) Having to enter the password twice in the space of 10 seconds isn't great, probably beyond the scope of this PR, but maybe we note this somewhere else for improvement, since we're adding password prompts to more places in the UI and risk increasing user frustration.

I guess this has come to the design question.

We could possibly look at a system level solution - same as sudo won't prompt for your password over and over if you entered it in the last 5 minutes. At the moment being tasked with adding 10 new users and copying their invites would make the admin type their password 20 times in under 5 minutes, it might be secure but it's the sort of UX that causes the admin to change their password to 12345 out of frustration :slightly_smiling_face: I can log a RFC issue on this if you like?

@justinvelluppillai commented on September 30th 2022 Member

@bx80 yes it'd be good to maybe create a separate issue for that. I agree it wasn't fun getting links or sending invites during testing 👍🏽

This Pull Request was closed on September 30th 2022
Powered by GitHub Issue Mirror