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

Topic invitations - User to accept invitation, no auto accept. #112

Closed
34 tasks done
tiblu opened this issue Nov 6, 2018 · 56 comments
Closed
34 tasks done

Topic invitations - User to accept invitation, no auto accept. #112

tiblu opened this issue Nov 6, 2018 · 56 comments
Assignees
Labels
critical App down / data leak / core functionality not usable. UX UX related issue. Needs input from UX specialists.

Comments

@tiblu
Copy link
Member

tiblu commented Nov 6, 2018

Overview

When User is invited to a Topic, he gets added automatically.
This may cause issues in the future where spammers may use this do distribute their content.

TODO

  • UX: Accept flow visuals
  • UX: New invitation visuals
  • UX: Invite expired screen - https://projects.invisionapp.com/d/main?origin=v7#/console/17224475/391689912/preview
  • UX: Invite revoked screen - implemented with generic error messaging - New Crowdin updates citizenos-fe#112 (comment)
  • FE: Update invite visuals - New Crowdin updates citizenos-fe#112 (comment)
  • FE: Use /invites API instead of /members/users - New Crowdin updates citizenos-fe#112 (comment)
  • FE: /invites/:inviteId -> /invites/users/:inviteId so that there is a clear distinction between User and Group invites.
  • FE: Rename invite translation keys to reflect User invites
  • FE: Show pending invites in the UI - New Crowdin updates citizenos-fe#112 (comment)
  • FE: Ability to revoke invite - New Crowdin updates citizenos-fe#112 (comment)
  • FE: Delete invite modal - translations, verify layout
  • UX: Log in - TOS - invite - flow. Current implementation will skip TOS!
  • UX: BUG - TOS & PP links not working!
  • API: VERSIONING - Backward comp - Partners (RAA.ee) use the auto-accept! Not needed, as the old API will remain with deprecation warning in the docs.
  • API: DB model - Invites
  • API: DB model - TopicInvites (extends Invites)
  • API: Create invites - POST /topics/:topicId/invites - auth: Topic admin
  • API: Create invites - emails!
  • API: Read invite - GET /topics/:topicId/invites/:inviteId - auth: knowing the inviteId (uuidv4)
  • API: Accept an invite - POST /api/users/:userId/topics/:topicId/invites/:inviteId/accept - auth: knowing the inviteId (uuidv4) && user.id === invite.userId
  • API: Delete (revoke) invites - DELETE/topics/:topicId/invites/:inviteId - auth: Topic admin - New Crowdin updates citizenos-fe#112 (comment)
  • API: Read/list - handle deleted and expired invites, TESTS!
  • API: /topics/:topicId/invites -> /topics/:topicId/invites/users/ so that there is a clear distinction between User and Group invites.
  • FE: Activity links to invite or topic depending on the viewer
  • API: BUG - multiple accepts of the same invite create several activities
  • FE: Localization - change activity messages to reflect "create invite" (ACTIVITY_FEED.ACTIVITY_USER_INVITE_TOPIC_USE, (ACTIVITY_FEED.ACTIVITY_USER_ACCEPT_INVITE_TOPIC)
  • FE: Activity icons - make sure icons are shown in mobile and desktop
  • FE: BUG - double create User events! - https://github.com/citizenos/citizenos-api/blob/master/libs/passport/index.js#L107
  • API: Create invites - activity!
  • API: Accept an invite - activity!
  • API: DB migration for TopicInvites!
  • BUG: Sometimes clicking invite from activity feed still shows the red error bar for expiry while it should show the modal.
  • Add localization context to Crowdin
  • API: When user accepts the invite, the invite is not marked as deleted thus UI shows pending invite and added member.

Related to:

@tiblu tiblu added the UX UX related issue. Needs input from UX specialists. label Apr 8, 2019
@tiblu tiblu added the critical App down / data leak / core functionality not usable. label Apr 8, 2019
@loorm loorm removed their assignment May 20, 2019
@loorm
Copy link
Member

loorm commented May 20, 2019

Triage #4 - Topics added to Group will remain auto accept. Possibly, invites coming from "friends", will also be auto-accepted (TBD).

@loorm
Copy link
Member

loorm commented Jun 3, 2019

@kevincrepin, this is likely to go into dev within the next weeks. Can you please have a look at this? UX tasks are in the description. When done, please remove yourself as an assigned person and assign Ilmar instead. Thank you!

@tiblu
Copy link
Member Author

tiblu commented Sep 3, 2019

Questions:

  • @kevincrepin @loorm Any plans to show list invitations in the Citizen OS UI? IF so, then where? Let's say I have not read my email and I log in, would we show all invitations in the UI somewhere? From what I understand, these screens both come into play when a User clicks a invite link in e-mail OR join link wherever it's shared?
  • @loorm From technical complexity, it would be easier if the shown Topic title on these screens would be the one that was at the time the invite was sent BUT that would create a loophole where a malicious actor MAY create a Topic with sensible title, send out invites and then change it to whatever he/she wants (spam).
  • @loorm IF we're showing latest info in the invite UI, we need to create an access token for the Topic information. That token cannot live forever. How long do you think it makes sense for an invitation to be valid? I would think if you haven't reacted 14 days, it should expire. The inviter can always send a new invite. We MAY want to implement expired invitation flow where you can re-request access, but that would create significant amount of UX and BE work. BUT, overall the feature of requesting an access makes sense to me. You MAY want to request access to edit a public topic?
  • @loorm Do want to avoid double invites from different people or not for now? It would be technically easier if we would not, BUT MAY make sense when it comes to UX.
  • @loorm To confirm - the Topic member list will only contain members that have accepted the invitation, we will NOT show a list of people that have pending invites?
  • @loorm Do we want to change the invitations to Group to the same model in this task or we're doing it as a separate one?

@tiblu
Copy link
Member Author

tiblu commented Sep 3, 2019

Implementation details

image

Information that is displayed and otherwise would not be available to the User:

  • Topic title
  • Topic inviters profile info
  • Topic permission

To provide this information we could:

  • Option 1: Generate a JWT token that would allow partial reading of Topic information from the UI. BUT:
    • Expiry - how long should the token be valid. We cannot allow it to be valid forever.
    • Scope - the token would contain scope, that is list of fields allowed to be read with this token. For example: scope: ["topic.title", "topic.creator.name", ....]. The implementation should ideally be generic so that we could use it for other entities also.
  • Option 2: Let the GET /api/invite/view fetch the info and pass it on to UI in a JWT BUT:
    • Assumes the GET /api/invite/view becomes a restricted endpoint requiring a token which disables malicious modification of parameters to expose private data.
    • Depending how much info is stored in the JWT, we MAY run into GET request length limits.

@loorm
Copy link
Member

loorm commented Sep 4, 2019

  • @loorm From technical complexity, it would be easier if the shown Topic title on these screens would be the one that was at the time the invite was sent BUT that would create a loophole where a malicious actor MAY create a Topic with sensible title, send out invites and then change it to whatever he/she wants (spam).

@tiblu I see the attack vector you've indentified. I'd like to test how Google Docs, for example, does this. My current thinking is, that we release invites with static topic title at this time and put down the access token idea into backlog for future development. But my thinking may change, if I test this on a few other services andsee, that they are all doing tokens. Will have time to test this later today.

@loorm
Copy link
Member

loorm commented Sep 4, 2019

  • @loorm IF we're showing latest info in the invite UI, we need to create an access token for the Topic information. That token cannot live forever. How long do you think it makes sense for an invitation to be valid? I would think if you haven't reacted 14 days, it should expire. The inviter can always send a new invite. We MAY want to implement expired invitation flow where you can re-request access, but that would create significant amount of UX and BE work. BUT, overall the feature of requesting an access makes sense to me. You MAY want to request access to edit a public topic?

I think invites should expire regardless of whether we go with token access or static invites. Everything dies. Invites should too :) 14 days sounds as good a lifetime as any. We should clearly communicate this to the user, though, both when sending and on the invite itself.

@loorm
Copy link
Member

loorm commented Sep 4, 2019

  • @kevincrepin @loorm Any plans to show list invitations in the Citizen OS UI? IF so, then where? Let's say I have not read my email and I log in, would we show all invitations in the UI somewhere? From what I understand, these screens both come into play when a User clicks a invite link in e-mail OR join link wherever it's shared?

@kevincrepin I'll leave this to you. I think it's much better UX, if invites are show in UI, too, but that's an unqualified opinion. WDYT?

@loorm
Copy link
Member

loorm commented Sep 4, 2019

  • @loorm Do want to avoid double invites from different people or not for now? It would be technically easier if we would not, BUT MAY make sense when it comes to UX.
  • @loorm To confirm - the Topic member list will only contain members that have accepted the invitation, we will NOT show a list of people that have pending invites?

We should avoid double invites to the same e-mail. When attempted, there should be a clear message, indicating that the invite failed, because this e-mail has already been invited.

What's the estimated extra work time, if we do indicate invited users in the Topic members list?

@tiblu
Copy link
Member Author

tiblu commented Sep 4, 2019

  • @loorm Do want to avoid double invites from different people or not for now? It would be technically easier if we would not, BUT MAY make sense when it comes to UX.
  • @loorm To confirm - the Topic member list will only contain members that have accepted the invitation, we will NOT show a list of people that have pending invites?

We should avoid double invites to the same e-mail. When attempted, there should be a clear message, indicating that the invite failed, because this e-mail has already been invited.

@loorm Now that I think about it, it MAY be better to allow double invites. Let's say a person I hardly know invites me to a Topic, in my eyes, he/she has not the mojo to convince me to join. But as a second invite I get from a close friend, who certainly has the mojo to make ma participate. Also, the more invites, the more people think that the person can contribute.

What's the estimated extra work time, if we do indicate invited users in the Topic members list?

@loorm Not sure, but I'll just say 3 days extra.

@tiblu
Copy link
Member Author

tiblu commented Sep 4, 2019

  • @loorm From technical complexity, it would be easier if the shown Topic title on these screens would be the one that was at the time the invite was sent BUT that would create a loophole where a malicious actor MAY create a Topic with sensible title, send out invites and then change it to whatever he/she wants (spam).

@tiblu I see the attack vector you've indentified. I'd like to test how Google Docs, for example, does this. My current thinking is, that we release invites with static topic title at this time and put down the access token idea into backlog for future development. But my thinking may change, if I test this on a few other services andsee, that they are all doing tokens. Will have time to test this later today.

@loorm Google Drive has kind of hybrid solution where in the e-mail content and title you have the outdated info and in the attachment you have up-to-date info.

I have never received a spam invite from Google, so they MAY tackle the issue some other way.

TBH, we have no way of providing fresh info in the e-mail itself, but when a User opens up the invite landing page in the Citizen OS, we should show fresh. It is possible, but I have to figure out what extra effort it takes.

image

@kevincrepin
Copy link
Collaborator

  • @kevincrepin @loorm Any plans to show list invitations in the Citizen OS UI? IF so, then where? Let's say I have not read my email and I log in, would we show all invitations in the UI somewhere? From what I understand, these screens both come into play when a User clicks a invite link in e-mail OR join link wherever it's shared?

@kevincrepin I'll leave this to you. I think it's much better UX, if invites are show in UI, too, but that's an unqualified opinion. WDYT?

@loorm @tiblu
Quick and simple solution right now: it's part of the activity feed. There's also filtering possibilities in there, so invites can be part of the filter.

Slightly more elaborate: invites section within the activity feed but not as part of the filter since it might be hard to find there and invites are of such nature that we want the user to quickly find them. So maybe the feed exists of 2 "tabs" (for example): notifications & invites.

For both these solutions we can create an icon that shows if there's an invitation instead of the general activity feed. This icon would show if there's a new (unread) invitation within the feed.

Bigger picture solution: notifications and/or invites could be more visible in the future dashboard, so that users wouldn't even have to click on anything to see them. Or a dynamic notification that shows up, for example on the top of the dashboard, similar to "success" or "error" notifications.

@tiblu
Copy link
Member Author

tiblu commented Sep 4, 2019

@loorm

1. Invites by e-mail
Member invites are personalized right? That is, when I open up an invite from my inbox, that is sent to [email protected], when I click the invite and open the Citizen OS App, I MAY be logged in as a different User [email protected]. The case is not that rare, for example I have all my Citizen OS e-mail redirected to another e-mail and I usually use that other e-mail as my primary Citizen OS login.
I propose that the invite really is personalized, I should be able to accept this invite as [email protected] and ONLY as [email protected]. That would mean we would ask User to switch account and log in as [email protected] to accept the invite. That OFC means we need UX designs for such flows.
We need to make sure that [email protected] is the one actually accepting the invite, requiring to log in is a way to guarantee it.
And just to make it clear - User MUST log in to accept an invite.

2. Join links
Join links are not personalized, but still require User to log in, there is no way to know which User gets the Topic access.
As join links are not personalized and they are not actually invites themselves, the UX texts need to be different. The "A person invites you" is not accurate.

@loorm
Copy link
Member

loorm commented Sep 5, 2019

  • @loorm Do we want to change the invitations to Group to the same model in this task or we're doing it as a separate one?

@tiblu Yes, no, yes. Yes - we want to change to the same model. No, not in this task. Yes, let's split it off as a separate task. Can you create the task, please?

@loorm
Copy link
Member

loorm commented Sep 5, 2019

  • @loorm Do want to avoid double invites from different people or not for now? It would be technically easier if we would not, BUT MAY make sense when it comes to UX.
  • @loorm To confirm - the Topic member list will only contain members that have accepted the invitation, we will NOT show a list of people that have pending invites?

We should avoid double invites to the same e-mail. When attempted, there should be a clear message, indicating that the invite failed, because this e-mail has already been invited.

@loorm Now that I think about it, it MAY be better to allow double invites. Let's say a person I hardly know invites me to a Topic, in my eyes, he/she has not the mojo to convince me to join. But as a second invite I get from a close friend, who certainly has the mojo to make ma participate. Also, the more invites, the more people think that the person can contribute.

@tiblu I see your point. In my mind, the attack vector of sending a thousand "double" invites outweighs the benefit of +1-ing invites. However, if we can prevent double invites to the same e-mail by the same user, we get your benefits and solve a majority of the spam risk. I don't know if that makes any difference, but I'm reluctant to make our system send out e-mails indiscriminately.

@loorm
Copy link
Member

loorm commented Sep 5, 2019

  • @loorm From technical complexity, it would be easier if the shown Topic title on these screens would be the one that was at the time the invite was sent BUT that would create a loophole where a malicious actor MAY create a Topic with sensible title, send out invites and then change it to whatever he/she wants (spam).

@tiblu I see the attack vector you've indentified. I'd like to test how Google Docs, for example, does this. My current thinking is, that we release invites with static topic title at this time and put down the access token idea into backlog for future development. But my thinking may change, if I test this on a few other services andsee, that they are all doing tokens. Will have time to test this later today.

@loorm Google Drive has kind of hybrid solution where in the e-mail content and title you have the outdated info and in the attachment you have up-to-date info.

I have never received a spam invite from Google, so they MAY tackle the issue some other way.

TBH, we have no way of providing fresh info in the e-mail itself, but when a User opens up the invite landing page in the Citizen OS, we should show fresh. It is possible, but I have to figure out what extra effort it takes.

image

@tiblu It's settled then, yes? Static info in the e-mail, up-to-date info, once you follow the link and enter our system.

@tiblu
Copy link
Member Author

tiblu commented Sep 9, 2019

@loorm Yes, it's settled then - static in the e-mail, up-to-date when you follow the link.

@tiblu
Copy link
Member Author

tiblu commented Sep 10, 2019

Dynamic (fresh) content in e-mails

It is possible to add dynamic content in the e-mails using Amp for Emails - https://amp.dev/about/email/.

Examples of what Citizen OS could do:

  • Topic invites would always have up-to-date info in the email - title, member count, etc.
  • You could accept an invite without opening the browser.
  • You could vote (NOT sign) without opening the browser.

BUT, you need to consider:

  • The email client of the client needs to support the AMP for Emails - Outlook, Gmail, Yahoo and other major providers have support already. TODO: Where to find a comprehensive list of clients that support AMP?
  • Your e-mail delivery system needs to suppport AMP (text/x-amp-html) - Mailgun does
  • You do need to make a backward compatible version of the email also.
  • The e-mail will be heavier that is more kilobytes to send over the network. May be a problem on slower connections.
  • OFC added development cost of creating separate AMP emails and testing them.

@loorm Just to keep you in the loop, maybe knowing that there is new technology helps to think of new cool solutions in Citizen OS.

Sources:

tiblu referenced this issue in citizenos/citizenos-api Nov 13, 2019
tiblu referenced this issue Nov 18, 2019
@tiblu
Copy link
Member Author

tiblu commented Nov 19, 2019

Pre release testing

  • Accept flow for a unauthenticated User
  • Accept flow for a authenticated User who was invited
  • Accept flow for a authenticated User, but not the one invited
  • Invite expiry
  • Other errors (topic deleted, network error)
  • Activity feed - the invite creator view and actions
  • Activity feed - the invitees view and actions
  • Emails

tiblu referenced this issue in citizenos/citizenos-api Nov 19, 2019
tiblu referenced this issue in citizenos/citizenos-api Nov 20, 2019
…ite after accepting, update existing member level if invite level is higher than current - citizenos/citizenos-fe#112
tiblu referenced this issue in citizenos/citizenos-api Nov 20, 2019
@tiblu
Copy link
Member Author

tiblu commented Nov 20, 2019

Done!

Development time: 97.67 hours

@loorm
Copy link
Member

loorm commented Nov 21, 2019

Tested. Not working.

Topic was a private topic called "Spring Push for Design Perfection". I am admin with my gmail account. I invited my other account. I see the pending invitation in the user list (see screenshot).

Invites

The invite action also shows up in activity feed (see screenshot).

Invites_2

Then, as you can see in the linked screencast (sry for the WeTransfer, but I didn't know how else to attach mp4 video), clicking on the invitation link in the email opens the correct dialog window, but that's where it stops. I'm not getting access to the topic.

https://wetransfer.com/downloads/16ccf9cbd41ad498b1d444ceef04a35b20191121125641/27a8dd27e7aad5f86f64ded925f9c46e20191121125641/9707c9

@loorm
Copy link
Member

loorm commented Nov 21, 2019

UPDATE: I tried the scenario in the video several times and each time nothing happened. However, on other occasions (different topics, different rights), the invite flow worked and my other user was successfully added. So it only seems to fail under certain circumstances, that are not clear as of now.

The only thing I did outside of expected primary flow was to click the "Read more about Citizen OS" at the bottom of the dialog, BEFORE clicking the "Log in as ..." big blue button.

@tiblu
Copy link
Member Author

tiblu commented Nov 21, 2019

@loorm Great spotting! Fixed!

The bug appeared only if tried to accept the invite by logging in with username/password.
Bug itself was related to user/password login not redirecting to the specified redirectSuccess url, but just reloading current page. Bug seems to be an old one, not related to the release.

While at it, also removed level none from the permissions dropdown menus in the "invite" dialog. Does not make sense to invite a person with none access.

@loorm
Copy link
Member

loorm commented Nov 22, 2019

Tested. Working.

@KatiVellak
Copy link

Legally reviewed, during the discussions legal input provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical App down / data leak / core functionality not usable. UX UX related issue. Needs input from UX specialists.
Projects
None yet
Development

No branches or pull requests

5 participants