Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

SSC: Handle "Accept invite" calls#62888

Merged
vdavid merged 11 commits into
mainfrom
dv/handle-accept-invite-calls
May 27, 2024
Merged

SSC: Handle "Accept invite" calls#62888
vdavid merged 11 commits into
mainfrom
dv/handle-accept-invite-calls

Conversation

@vdavid

@vdavid vdavid commented May 23, 2024

Copy link
Copy Markdown
Contributor

Before this PR, we had no "Accept invite" functionality.
A few days ago, @chrsmith added the email-sending feature in https://github.com/sourcegraph/self-serve-cody/pull/777, and I tweaked it a bit in https://github.com/sourcegraph/self-serve-cody/pull/781. The current PR wires up the rest of the flow, and lets the user actually use the invite.

Three changes happen in this PR:

  1. I extracted CodyAlert and CodyContainer because I wanted to reuse them on pages other than CodyManageTeamPage.
  2. I split the remaining styles of CodyManageTeamPage into InviteUsers and TeamMemberList.
  3. I added the new page at /cody/invites/accept. It makes a back-end call to POST /team/${teamId}/invites/${inviteId}/accept, and if it succeeds, redirects the user to /cody/team/manage, otherwise displays an error message.

For easier review, no. 1 and no. 2 are in one commit, and no. 3 is in a different commit.

There are a few problematic parts, but first, to have a common understanding of the process, I'll describe the user flow and how the user interacts with our system when using invites.

This is a common happy path:

  1. An admin goes to https://cody-services-dev.sgdev.dev/cody/subscription, clicks the "Create a Cody Pro team" button, and buys two seats.
  2. Same admin goes to https://cody-services-dev.sgdev.dev/cody/team/manage and invites a colleague. This action sends an email to the colleague
  3. The colleague receives the "You've been invited to join a Cody Pro team!" email and clicks the big "View invite" button.
  4. The colleague arrives at the https://cody-services-dev.sgdev.dev/cody/invites/accept page, through a link that looks like this: https://cody-services-dev.sgdev.dev/cody/invites/accept?inviteID=018f9b13-ddd1-7f1f-b030-29b6f8fc77dd&signin=OpenIDConnect&teamID=018f9b13-5bf7-77cc-b192-c2fcac17ad94 (has an inviteID, and a teamID argument).
  5. The page checks the validity of the inviteId+teamId combo.
  6. It's valid, so the user gets redirected to https://cody-services-dev.sgdev.dev/cody/team/manage and sees a list of the admin and themself.
  7. 💰

Problems I'm seeing:

  1. (larger problem) With this architecture, anyone can accept the invite. This is problematic in at least two ways:
    • If the user forwards the email to someone (maybe even through an autoforward), that someone can redeem the invite for them.
    • If the user is logged in with a different user on sourcegraph.com, their invite will be redeemed for the wrong user. This can be as easy as just clicking the wrong SSO icon when presented. To solve this, the admin can kick out and re-invite the colleague, but it's still awkward.
    • Suggestion 1: back end: In step no. 5, compare the logged-in SAMS user's email address to the email address of the invite (here) and fail if they don't match.
    • Suggestion 2: front end: If suggestion no. 1 doesn't work for some reason, we can add the invited email address to the link, like https://cody-services-dev.sgdev.dev/cody/invites/accept?inviteID=018f9b13-ddd1-7f1f-b030-29b6f8fc77dd&signin=OpenIDConnect&teamID=018f9b13-5bf7-77cc-b192-c2fcac17ad94&email=invited.person@company.com (notice the last part), and then check it against the logged in user's email address on the front end. Fail if they don't match.
  2. (smaller problem) The "View invite" button actually links https://cody-services-dev.sgdev.dev/cody/invites/accept, which straight away accepts the invite. The button text is misleading.
    CleanShot 2024-05-23 at 23 56 58@2x
    • Suggestion: Let's use the copy "Accept invite".
  3. (smaller problem) The name is missing from the email:
    • Suggestion: Let's just say "Hey, friend," or something. We won't know the name of most users.
    • Suggestion 2: I'd treat this as a nice-to-have because we don't even have a DB field for it, but in case the invitee already has a Sourcegraph account, use their first name.

That said, the current PR could be fine as-is; we just need to be mindful of these two caveats and fix them (or consciously ignore them) as a follow-up.

Test plan

I've tested it manually. The flow is not perfect (A "Bad gateway" response displays on the "Accept" page for a split-second AND a "Forbidden" response is returned on the "Manage" page), but it accepts the invite correctly and the user shows up in the list:

CleanShot 2024-05-23 at 23 51 29@2x

Same on the admin's side:

CleanShot 2024-05-23 at 23 52 21@2x

I'll fix these problems in a follow-up PR.

@cla-bot cla-bot Bot added the cla-signed label May 23, 2024
@vdavid vdavid marked this pull request as ready for review May 23, 2024 21:53
@vdavid vdavid requested a review from a team May 23, 2024 21:55
@chenkc805

Copy link
Copy Markdown
Contributor

Thanks for writing this up so detailed and clearly @vdavid .

  • What's the pros/cons between the front-end vs back-end suggestions? They end up giving the same functionality but I'm not sure if there are trade-offs. I guess the front-end solution is susceptible to tampering, but is that the only downside?
  • For "Suggestion: Let's use the copy "Accept invite".", let's make this change. Good call
  • "Suggestion: Let's just say "Hey, friend," or something. We won't know the name of most users." yeah let's do this. Getting the email or name is a nice to have.

Comment thread client/web/src/cody/invites/AcceptInvitePage.tsx Outdated
Comment thread client/web/src/cody/invites/AcceptInvitePage.tsx Outdated
Comment thread client/web/src/cody/invites/AcceptInvitePage.tsx Outdated
Comment thread client/web/src/cody/invites/AcceptInvitePage.tsx Outdated
Comment thread client/web/src/cody/team/InviteUsers.module.scss Outdated
Comment thread client/web/src/cody/team/TeamMemberList.module.scss Outdated
Comment thread client/web/src/routes.constants.ts Outdated
Comment thread client/web/src/routes.tsx Outdated

@taras-yemets taras-yemets left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the code LGTM. Left a few comments inline. Approving to unblock.

@vdavid vdavid force-pushed the dv/handle-accept-invite-calls branch from 739bdcd to 79f1730 Compare May 24, 2024 15:53
@vdavid vdavid enabled auto-merge (squash) May 27, 2024 11:54
@vdavid

vdavid commented May 27, 2024

Copy link
Copy Markdown
Contributor Author

@chenkc805, I've added the next actions to new ticket https://github.com/sourcegraph/self-serve-cody/issues/785. For the back-end/front-end question, the back-end solution is a clear winner so marked that one as the next action.

@vdavid vdavid merged commit 7f0080f into main May 27, 2024
@vdavid vdavid deleted the dv/handle-accept-invite-calls branch May 27, 2024 12:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants