From e75aa069294f7793da9ec82f8abf4680d4bc9436 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Fri, 27 Sep 2024 20:57:44 +0700 Subject: [PATCH] Allow multiple project invites for one user (#1043) * Allow users to accept multiple invitations Now that we use invitation links for both projects and orgs, it will start to become rather common for someone to receive an org invitation and a project invitation within the same few minutes. So we need to allow users to click on invitation links even if they already have a Lexbox account. * Forbid changing email when accepting invite This prevents stealing someone else's invite by snooping on their email. * Don't error if same invite accepted twice If a user clicks the same invitation link twice, he should not get an error message, he should just be taken to his home page. He's already a member of the project, so trying to join the project should not error but just succeed silently. * Handle edge case of registering existing email This can only happen if an admin creates the account while the user was filling out the registration page, so this probably will never happen. --- Taskfile.yml | 6 ++ .../LexBoxApi/Controllers/LoginController.cs | 11 ++- .../LexBoxApi/Controllers/UserController.cs | 79 +++++++++++++++++-- backend/LexBoxApi/Services/EmailService.cs | 5 +- .../lib/components/Users/CreateUser.svelte | 6 +- frontend/src/lib/i18n/locales/en.json | 1 + .../acceptInvitation/+page.svelte | 2 +- 7 files changed, 97 insertions(+), 13 deletions(-) diff --git a/Taskfile.yml b/Taskfile.yml index 15f470b02..e477c0233 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -51,6 +51,12 @@ tasks: - wget -c -O {{.DATA_DIR}}/sena-3.zip 'https://drive.google.com/uc?export=download&id=1I-hwc0RHoQqW774gbS5qR-GHa1E7BlsS' - wget -c -O {{.DATA_DIR}}/empty.zip 'https://drive.google.com/uc?export=download&id=1p73u-AGdSwNkg_5KEv9-4iLRuN-1V-LD' - wget -c -O {{.DATA_DIR}}/elawa.zip 'https://drive.usercontent.google.com/download?export=download&id=1Jk-eSDho8ATBMS-Kmfatwi-MWQth26ro&confirm=t' + setup-local-env: + cmds: + - echo "HONEYCOMB_API_KEY=__REPLACE__" > deployment/local-dev/local.env + - echo "#OTEL_SDK_DISABLED=true" >> deployment/local-dev/local.env + - echo "GOOGLE_OAUTH_CLIENT_ID=__REPLACE__.apps.googleusercontent.com" >> deployment/local-dev/local.env + - echo "GOOGLE_OAUTH_CLIENT_SECRET=__REPLACE__" >> deployment/local-dev/local.env # k8s up: diff --git a/backend/LexBoxApi/Controllers/LoginController.cs b/backend/LexBoxApi/Controllers/LoginController.cs index f7371c07e..3d4f6d21b 100644 --- a/backend/LexBoxApi/Controllers/LoginController.cs +++ b/backend/LexBoxApi/Controllers/LoginController.cs @@ -51,7 +51,16 @@ public async Task LoginRedirect( await HttpContext.SignInAsync(User, new AuthenticationProperties { IsPersistent = true }); - return Redirect(returnTo); + var destination = ValidateRedirectUrl(returnTo); + return Redirect(destination); + } + + private string ValidateRedirectUrl(string url) + { + // Redirect URLs must be relative, to avoid phishing attacks where user is redirected to + // a lookalike site. So we strip off the host if there is one. + var uri = new Uri(url, UriKind.RelativeOrAbsolute); + return uri.IsAbsoluteUri ? uri.PathAndQuery : uri.ToString(); } [HttpGet("google")] diff --git a/backend/LexBoxApi/Controllers/UserController.cs b/backend/LexBoxApi/Controllers/UserController.cs index 59f56bdee..ac48f3f52 100644 --- a/backend/LexBoxApi/Controllers/UserController.cs +++ b/backend/LexBoxApi/Controllers/UserController.cs @@ -82,6 +82,42 @@ await HttpContext.SignInAsync(user.GetPrincipal("Registration"), return Ok(user); } + [HttpGet("acceptInvitation")] + [RequireAudience(LexboxAudience.RegisterAccount, true)] + [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status401Unauthorized)] + public async Task HandleInviteLink() + { + var user = _loggedInContext.User; + if (user.Email is null) + { + // Malformed JWT, exit early + return Redirect("/login"); + } + var dbUser = await _lexBoxDbContext.Users + .Where(u => u.Email == user.Email) + .Include(u => u.Projects) + .Include(u => u.Organizations) + .FirstOrDefaultAsync(); + if (dbUser is null) + { + // Go to frontend to fill out registration page + var queryString = QueryString.Create("email", user.Email); + var returnTo = new UriBuilder { Path = "/acceptInvitation", Query = queryString.Value }.Uri.PathAndQuery; + return Redirect(returnTo); + } + else + { + // No need to re-register users that already exist + UpdateUserMemberships(user, dbUser); + await _lexBoxDbContext.SaveChangesAsync(); + var loginUser = new LexAuthUser(dbUser); + await HttpContext.SignInAsync(loginUser.GetPrincipal("Invitation"), + new AuthenticationProperties { IsPersistent = true }); + return Redirect("/"); + } + } + [HttpPost("acceptInvitation")] [RequireAudience(LexboxAudience.RegisterAccount, true)] [ProducesResponseType(StatusCodes.Status200OK)] @@ -99,18 +135,29 @@ public async Task> AcceptEmailInvitation(RegisterAccou } var jwtUser = _loggedInContext.User; + if (jwtUser.Email != accountInput.Email) + { + // Changing email address in invite links is not allowed; this prevents someone from trying to reuse a JWT belonging to somebody else + ModelState.AddModelError(r => r.Email, "email address mismatch in invitation link"); + return ValidationProblem(ModelState); + } - var hasExistingUser = await _lexBoxDbContext.Users.FilterByEmailOrUsername(accountInput.Email).AnyAsync(); - acceptActivity?.AddTag("app.email_available", !hasExistingUser); - if (hasExistingUser) + var userEntity = await _lexBoxDbContext.Users.FindByEmailOrUsername(accountInput.Email); + acceptActivity?.AddTag("app.email_available", userEntity is null); + if (userEntity is null) + { + userEntity = CreateUserEntity(accountInput, jwtUser); + _lexBoxDbContext.Users.Add(userEntity); + } + else { + // Multiple invitations accepted by the same account should no longer go through this method, so return an error if the account already exists + // That can only happen if an admin created the user's account while the user was still on the registration page: very unlikely ModelState.AddModelError(r => r.Email, "email already in use"); return ValidationProblem(ModelState); } - var userEntity = CreateUserEntity(accountInput, jwtUser); acceptActivity?.AddTag("app.user.id", userEntity.Id); - _lexBoxDbContext.Users.Add(userEntity); await _lexBoxDbContext.SaveChangesAsync(); var user = new LexAuthUser(userEntity); @@ -139,16 +186,32 @@ private User CreateUserEntity(RegisterAccountInput input, LexAuthUser? jwtUser, Locked = false, CanCreateProjects = false }; + UpdateUserMemberships(jwtUser, userEntity); + return userEntity; + } + private void UpdateUserMemberships(LexAuthUser? jwtUser, User userEntity) + { // This audience check is redundant now because of [RequireAudience(LexboxAudience.RegisterAccount, true)], but let's leave it in for safety if (jwtUser?.Audience == LexboxAudience.RegisterAccount && jwtUser.Projects.Length > 0) { - userEntity.Projects = jwtUser.Projects.Select(p => new ProjectUsers { Role = p.Role, ProjectId = p.ProjectId }).ToList(); + foreach (var p in jwtUser.Projects) + { + if (!userEntity.Projects.Exists(proj => proj.ProjectId == p.ProjectId)) + { + userEntity.Projects.Add(new ProjectUsers { Role = p.Role, ProjectId = p.ProjectId }); + } + } } if (jwtUser?.Audience == LexboxAudience.RegisterAccount && jwtUser.Orgs.Length > 0) { - userEntity.Organizations = jwtUser.Orgs.Select(o => new OrgMember { Role = o.Role, OrgId = o.OrgId }).ToList(); + foreach (var o in jwtUser.Orgs) + { + if (!userEntity.Organizations.Exists(org => org.OrgId == o.OrgId)) + { + userEntity.Organizations.Add(new OrgMember { Role = o.Role, OrgId = o.OrgId }); + } + } } - return userEntity; } [HttpPost("sendVerificationEmail")] diff --git a/backend/LexBoxApi/Services/EmailService.cs b/backend/LexBoxApi/Services/EmailService.cs index 2e8ce7b52..4f6b93723 100644 --- a/backend/LexBoxApi/Services/EmailService.cs +++ b/backend/LexBoxApi/Services/EmailService.cs @@ -166,8 +166,9 @@ private async Task SendInvitationEmail( var httpContext = httpContextAccessor.HttpContext; ArgumentNullException.ThrowIfNull(httpContext); - var queryString = QueryString.Create("email", emailAddress); - var returnTo = new UriBuilder { Path = "/acceptInvitation", Query = queryString.Value }.Uri.PathAndQuery; + var returnTo = _linkGenerator.GetUriByAction(httpContext, + nameof(LexBoxApi.Controllers.UserController.HandleInviteLink), + "User"); var registerLink = _linkGenerator.GetUriByAction(httpContext, "LoginRedirect", "Login", diff --git a/frontend/src/lib/components/Users/CreateUser.svelte b/frontend/src/lib/components/Users/CreateUser.svelte index 17a50e944..45adf4775 100644 --- a/frontend/src/lib/components/Users/CreateUser.svelte +++ b/frontend/src/lib/components/Users/CreateUser.svelte @@ -7,8 +7,10 @@ import { createEventDispatcher, onMount } from 'svelte'; import { usernameRe } from '$lib/user'; import { z } from 'zod'; + import type { StringifyValues } from '$lib/type.utils'; export let allowUsernames = false; + export let errorOnChangingEmail = ''; export let skipTurnstile = false; export let submitButtonText = $t('register.button_register'); export let handleSubmit: (password: string, passwordStrength: number, name: string, email: string, locale: string, turnstileToken: string) => Promise; @@ -24,6 +26,7 @@ email: string; }; let turnstileToken = ''; + let urlValues = {} as StringifyValues; function validateAsEmail(value: string): boolean { return !allowUsernames || value.includes('@'); @@ -36,6 +39,7 @@ name: z.string().trim().min(1, $t('register.name_missing')), email: z.string().trim() .min(1, $t('project_page.add_user.empty_user_field')) + .refine((value) => !errorOnChangingEmail || !urlValues.email || value == urlValues.email, errorOnChangingEmail) .refine((value) => !validateAsEmail(value) || isEmail(value), $t('form.invalid_email')) .refine((value) => validateAsEmail(value) || usernameRe.test(value), $t('register.invalid_username')), password: passwordFormRules($t), @@ -64,7 +68,7 @@ throw new Error('Unknown error, no error from server, but also no user.'); }); onMount(() => { // query params not available during SSR - const urlValues = getSearchParamValues(); + urlValues = getSearchParamValues(); form.update((form) => { if (urlValues.name) form.name = urlValues.name; if (urlValues.email) form.email = urlValues.email; diff --git a/frontend/src/lib/i18n/locales/en.json b/frontend/src/lib/i18n/locales/en.json index 80f974dba..0f36d14c8 100644 --- a/frontend/src/lib/i18n/locales/en.json +++ b/frontend/src/lib/i18n/locales/en.json @@ -554,6 +554,7 @@ If you don't see a dialog or already closed it, click the button below:", "account_exists_email": "An account with this email already exists", "account_exists_login": "An account with this login/username already exists", "invalid_username": "Invalid login/username. Only letters, numbers, and underscore (_) characters are allowed.", + "changing_email_not_allowed": "You can't change your email when accepting an invite. You can change your email after your account is registered. Be aware this will invalidate any other invites you have received, so accept them first before changing your email.", "button_register": "Register", "label_email": "Email", "label_email_or_username": "Email or login/username", diff --git a/frontend/src/routes/(unauthenticated)/acceptInvitation/+page.svelte b/frontend/src/routes/(unauthenticated)/acceptInvitation/+page.svelte index dd8af1c28..385ca2dea 100644 --- a/frontend/src/routes/(unauthenticated)/acceptInvitation/+page.svelte +++ b/frontend/src/routes/(unauthenticated)/acceptInvitation/+page.svelte @@ -11,5 +11,5 @@ - +