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

Show user invite link when inviting user #19625

Closed
Findus23 opened this issue Aug 12, 2022 · 19 comments · Fixed by #19707
Closed

Show user invite link when inviting user #19625

Findus23 opened this issue Aug 12, 2022 · 19 comments · Fixed by #19707
Assignees
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently.
Milestone

Comments

@Findus23
Copy link
Member

At the moment the only way to invite a user is by letting Matomo send an E-Mail to them. But a ton of people have broken E-Mail setups. Either their mail server isn't properly set up with DKIM, etc. and all mails are rejected as SPAM or they don't really have a mail server or they are using some wonky SMTP setup via gmail (which can break for all kinds of Google changes).
Before that didn't really matter as unless you need E-Mail reports, you are only missing out on some things (mostly password reset and security notifications).
But now with the invite feature Matomo becomes completely unusable unless your Matomo instance is able to successfully and reliably send E-Mails. And I am sure that will trip up and frustrate a lot of newer Matomo uses.

One simple solution would be to display the user invite link that is sent to the new user also to the admin when doing the invite. This also allows the admin to also send the link via other ways than E-Mail and allows people to easily circumvent the invite feature (assuming it works when already signed in).

@Findus23 Findus23 added the c: Usability For issues that let users achieve a defined goal more effectively or efficiently. label Aug 12, 2022
@Findus23 Findus23 added this to the For Prioritization milestone Aug 12, 2022
@Findus23 Findus23 added the Needs priority decision This issue may need to be added to the current milestone by Product Manager label Aug 12, 2022
@MatomoForumNotifications

This issue has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/4-11-0-cant-set-new-user-passwords-but-user-invite-emails-not-sent/46954/2

@sgiehl
Copy link
Member

sgiehl commented Aug 16, 2022

I would suggest to maybe add a new config flag for this. Having it enabled will show the invite links instead of sending an email at all. Having the possibility to show an invite link on all instances might otherwise make it a bit less secure I guess.

@jane-twizel jane-twizel removed the Needs priority decision This issue may need to be added to the current milestone by Product Manager label Aug 16, 2022
@jane-twizel
Copy link

@Javi-Ormaechea will look at this and come back with a design solution by end of tomorrow. @justinvelluppillai he will be in touch with questions.

@Javi-Ormaechea Javi-Ormaechea self-assigned this Aug 17, 2022
@Javi-Ormaechea
Copy link

A solution was created for this issue. Please get in touch with me to get file, thanks! 🙂

@peterhashair
Copy link
Contributor

peterhashair commented Aug 25, 2022

ah, I finished the UI, but I found some tech difficulty. Because once the token is generated (which saved hash string into the database), it can’t be reviewed again (one-way trip), only in the target emails. The only way to get the token again is to regenerate a new one.

discuss with @justinvelluppillai and @bx80, maybe we can merge the send invite and copy link to one button. Or add a new table structure to allow multiple tokens for one user ( which requires quite a few changes). Or save an un hashed token into the database.

@sgiehl
Copy link
Member

sgiehl commented Aug 25, 2022

Personally I would suggest to give the admin the possibility to either send an email or to show the invite link. Maybe we could also add a config option that allows configuring which options are available. As viewing the invite link is a bit less secure, some people might want to disable it maybe.

Also I think it's totally fine if the token can only be seen once (if the UI explains that clearly).
In case the user clicks the "resend" link, we could again give the possibility to either send a mail or view the invite link. But we need to ensure the UI makes it clear, that previously sent token will get invalid.

@peterhashair
Copy link
Contributor

@sgiehl that makes sense, @justinvelluppillai @bx80 I was wondering about a simple solution. once the admin creates or resends invites, it will show a green notification on the top, I was wondering if just add a copy link button in the notification.

@bx80
Copy link
Contributor

bx80 commented Aug 29, 2022

@peterhashair Maybe it would be good to discuss updating the original UI design with @Javi-Ormaechea?

@justinvelluppillai
Copy link
Contributor

yes good idea - @peterhashair can you please reach out to @Javi-Ormaechea to discuss the changed requirements so he can advise on UI?

@peterhashair
Copy link
Contributor

Hey @tsteur, just checking security here is that ok to store an invite token into the database without hashing it? Because we need to copy the invite link after the token has been generated.

@tsteur
Copy link
Member

tsteur commented Sep 6, 2022

Good question @peterhashair . Ideally we would hash it and don't store it in plain text.

Is there a chance we could change it to "Generate a link" and mention that this will basically invalidate any previously sent link in an email? Then we wouldn't need to store this in plain text. Or maybe we could have two tokens? One from the email and one generated when clicking on "generate a link" or something?

@peterhashair
Copy link
Contributor

thanks @tsteur another database field makes more sense.

@peterhashair
Copy link
Contributor

As I go, I found the copy link won't always work, see screenshots, it won't work in Safari, etc, and also our UI test environment will show errors as well. I guess the reason is a web page could silently copy rm -rf / or a decompression bomb image to your clipboard.

  • Or we could make the copy link to generate a link, then show the generated link in UI.
  • Adding permission, but I can see some browsers support this, and some are not.
  • Or we could do like the image shows, shows an error notification then, the user could resolve the permission for this.

image

@peterhashair
Copy link
Contributor

Sorry to bother, @tsteur I got a security question here because we using javascript code navigator.clipboard.writeText to copy the invite js link. Then somehow if the client land on a malicious site after that, the site has code navigator.clipboard.readText which will read your invite link, would that be a security concern?

@tsteur
Copy link
Member

tsteur commented Sep 19, 2022

@peterhashair that could be an issue indeed if another site could read that invite link. Is it possible to use an alternative like document.execCommand('copy'); or is there an issue with it?

@peterhashair
Copy link
Contributor

peterhashair commented Sep 22, 2022

@tsteur I think this function is deprecated, personally, I think copying the password or token link is not a very good practice. Maybe We can either show the link as a string like AWS did or we could remove copy link, but add create a user back next to invite user, invite user will auto disabled when the email is not enabled in config.

image

@tsteur
Copy link
Member

tsteur commented Sep 22, 2022

Interesting, then we might at some point also need to adjust other places where we are using this currently.

Note that I'm just seeing this clipboard API only works on HTTPS meaning we might still need to fallback to execCommand when someone is on HTTP.

I've just reproduced this and the browser does seem to ask you for permission when you want to readText. To lower the chances of an issue, could we maybe clear the value from clipboard after a while? although people might close the tab right after copying the link. Are there any ideas/ thoughts how we could make this more secure? Can we check if browser supports execCommand('copy') and use it as preferred choice and only fallback to clipboard if it's not? (don't know if we can find out if it's supported or not).

@peterhashair
Copy link
Contributor

@tsteur execCommand('copy') would be ideal, but I tried different ways, but seems like there is no way we can detect whether the browser support execCommand('copy') unless we make a list and keeps updating it, any suggestions?

@tsteur
Copy link
Member

tsteur commented Sep 25, 2022

@peterhashair Maybe something like https://github.com/sudodoki/copy-to-clipboard/blob/master/index.js#L79-L83 could work (see below)? And it could be also be combined with if (document.queryCommandEnabled) {document.queryCommandEnabled("copy")}? If not available or if unsuccessful we could fallback to writeText?

if (document.queryCommandEnabled) { var isAvailable = document.queryCommandEnabled("copy")}
if (isAvailable && !document.execCommand) { ... }
try {
    var successful = document.execCommand("copy");
    if (!successful) {
      throw new Error("copy command was unsuccessful");
    }
    success = true;
  } catch (err) {
   // fallback
}

If this doesn't work then we may have to use writeText by default on HTTPS (and execCommand on HTTP)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Usability For issues that let users achieve a defined goal more effectively or efficiently.
Projects
None yet
9 participants