From 23d5a7b559a7fe463e2b648f0741f82d6cdccd3d Mon Sep 17 00:00:00 2001 From: Kristaps Fabians Geikins Date: Mon, 2 Sep 2024 10:40:53 +0300 Subject: [PATCH] fix(server): auto-verify on invited server registration (#2824) --- .../server/modules/auth/strategies/azureAd.ts | 48 ++++++------------- .../server/modules/auth/strategies/github.ts | 40 ++++++---------- .../server/modules/auth/strategies/google.ts | 40 ++++++---------- .../server/modules/auth/strategies/local.ts | 3 +- .../server/modules/auth/strategies/oidc.ts | 39 ++++++--------- .../auth/tests/helpers/registration.ts | 12 ++++- .../tests/integration/registration.spec.ts | 10 +++- .../server/modules/core/services/users.js | 2 +- 8 files changed, 79 insertions(+), 115 deletions(-) diff --git a/packages/server/modules/auth/strategies/azureAd.ts b/packages/server/modules/auth/strategies/azureAd.ts index 274833693..7a5c9f396 100644 --- a/packages/server/modules/auth/strategies/azureAd.ts +++ b/packages/server/modules/auth/strategies/azureAd.ts @@ -31,6 +31,7 @@ import { } from '@/modules/shared/helpers/envHelper' import type { Request } from 'express' import { ensureError, Optional } from '@speckle/shared' +import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' const azureAdStrategyBuilder: AuthStrategyBuilder = async ( app, @@ -128,31 +129,6 @@ const azureAdStrategyBuilder: AuthStrategyBuilder = async ( return next() } - // if the server is not invite only, go ahead and log the user in. - if (!serverInfo.inviteOnly) { - const myUser = await findOrCreateUser({ - user - }) - - // ID is used later for verifying access token - req.user = { - ...profile, - id: myUser.id, - email: myUser.email, - isNewUser: myUser.isNewUser - } - - // process invites - if (myUser.isNewUser) { - await finalizeInvitedServerRegistrationFactory({ - deleteServerOnlyInvites: deleteServerOnlyInvitesFactory({ db }), - updateAllInviteTargets: updateAllInviteTargetsFactory({ db }) - })(user.email, myUser.id) - } - - return next() - } - // if the server is invite only and we have no invite id, throw. if (serverInfo.inviteOnly && !req.session.token) { throw new UserInputError( @@ -160,18 +136,22 @@ const azureAdStrategyBuilder: AuthStrategyBuilder = async ( ) } - // validate the invite - const validInvite = await validateServerInviteFactory({ - findServerInvite: findServerInviteFactory({ db }) - })(user.email, req.session.token) + // 2. if you have an invite it must be valid, both for invite only and public servers + let invite: Optional = undefined + if (req.session.token) { + invite = await validateServerInviteFactory({ + findServerInvite: findServerInviteFactory({ db }) + })(user.email, req.session.token) + } // create the user const myUser = await findOrCreateUser({ user: { ...user, - role: validInvite - ? getResourceTypeRole(validInvite.resource, ServerInviteResourceType) - : undefined + role: invite + ? getResourceTypeRole(invite.resource, ServerInviteResourceType) + : undefined, + verified: !!invite } }) @@ -181,7 +161,7 @@ const azureAdStrategyBuilder: AuthStrategyBuilder = async ( id: myUser.id, email: myUser.email, isNewUser: myUser.isNewUser, - isInvite: !!validInvite + isInvite: !!invite } req.log = req.log.child({ userId: myUser.id }) @@ -193,7 +173,7 @@ const azureAdStrategyBuilder: AuthStrategyBuilder = async ( })(user.email, myUser.id) // Resolve redirect path - req.authRedirectPath = resolveAuthRedirectPathFactory()(validInvite) + req.authRedirectPath = resolveAuthRedirectPathFactory()(invite) // return to the auth flow return next() diff --git a/packages/server/modules/auth/strategies/github.ts b/packages/server/modules/auth/strategies/github.ts index c31a37933..65ca15a55 100644 --- a/packages/server/modules/auth/strategies/github.ts +++ b/packages/server/modules/auth/strategies/github.ts @@ -31,7 +31,8 @@ import { } from '@/modules/shared/helpers/envHelper' import type { Request } from 'express' import { get } from 'lodash' -import { ensureError } from '@speckle/shared' +import { ensureError, Optional } from '@speckle/shared' +import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' const githubStrategyBuilder: AuthStrategyBuilder = async ( app, @@ -102,21 +103,6 @@ const githubStrategyBuilder: AuthStrategyBuilder = async ( return done(null, myUser) } - // if the server is not invite only, go ahead and log the user in. - if (!serverInfo.inviteOnly) { - const myUser = await findOrCreateUser({ user }) - - // process invites - if (myUser.isNewUser) { - await finalizeInvitedServerRegistrationFactory({ - deleteServerOnlyInvites: deleteServerOnlyInvitesFactory({ db }), - updateAllInviteTargets: updateAllInviteTargetsFactory({ db }) - })(user.email, myUser.id) - } - - return done(null, myUser) - } - // if the server is invite only and we have no invite id, throw. if (serverInfo.inviteOnly && !req.session.token) { throw new UserInputError( @@ -124,18 +110,22 @@ const githubStrategyBuilder: AuthStrategyBuilder = async ( ) } - // validate the invite - const validInvite = await validateServerInviteFactory({ - findServerInvite: findServerInviteFactory({ db }) - })(user.email, req.session.token) + // validate the invite, if any + let invite: Optional = undefined + if (req.session.token) { + invite = await validateServerInviteFactory({ + findServerInvite: findServerInviteFactory({ db }) + })(user.email, req.session.token) + } // create the user const myUser = await findOrCreateUser({ user: { ...user, - role: validInvite - ? getResourceTypeRole(validInvite.resource, ServerInviteResourceType) - : undefined + role: invite + ? getResourceTypeRole(invite.resource, ServerInviteResourceType) + : undefined, + verified: !!invite } }) @@ -146,12 +136,12 @@ const githubStrategyBuilder: AuthStrategyBuilder = async ( })(user.email, myUser.id) // Resolve redirect path - req.authRedirectPath = resolveAuthRedirectPathFactory()(validInvite) + req.authRedirectPath = resolveAuthRedirectPathFactory()(invite) // return to the auth flow return done(null, { ...myUser, - isInvite: !!validInvite + isInvite: !!invite }) } catch (err) { const e = ensureError( diff --git a/packages/server/modules/auth/strategies/google.ts b/packages/server/modules/auth/strategies/google.ts index db9539bf1..d1ea989e4 100644 --- a/packages/server/modules/auth/strategies/google.ts +++ b/packages/server/modules/auth/strategies/google.ts @@ -26,7 +26,8 @@ import { getGoogleClientId, getGoogleClientSecret } from '@/modules/shared/helpers/envHelper' -import { ensureError } from '@speckle/shared' +import { ensureError, Optional } from '@speckle/shared' +import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' const googleStrategyBuilder: AuthStrategyBuilder = async ( app, @@ -85,21 +86,6 @@ const googleStrategyBuilder: AuthStrategyBuilder = async ( return done(null, myUser) } - // if the server is not invite only, go ahead and log the user in. - if (!serverInfo.inviteOnly) { - const myUser = await findOrCreateUser({ user }) - - // process invites - if (myUser.isNewUser) { - await finalizeInvitedServerRegistrationFactory({ - deleteServerOnlyInvites: deleteServerOnlyInvitesFactory({ db }), - updateAllInviteTargets: updateAllInviteTargetsFactory({ db }) - })(user.email, myUser.id) - } - - return done(null, myUser) - } - // if the server is invite only and we have no invite id, throw. if (serverInfo.inviteOnly && !req.session.token) { throw new UserInputError( @@ -107,18 +93,22 @@ const googleStrategyBuilder: AuthStrategyBuilder = async ( ) } - // validate the invite - const validInvite = await validateServerInviteFactory({ - findServerInvite: findServerInviteFactory({ db }) - })(user.email, req.session.token) + // validate the invite, if any + let invite: Optional = undefined + if (req.session.token) { + invite = await validateServerInviteFactory({ + findServerInvite: findServerInviteFactory({ db }) + })(user.email, req.session.token) + } // create the user const myUser = await findOrCreateUser({ user: { ...user, - role: validInvite - ? getResourceTypeRole(validInvite.resource, ServerInviteResourceType) - : undefined + role: invite + ? getResourceTypeRole(invite.resource, ServerInviteResourceType) + : undefined, + verified: !!invite } }) @@ -129,12 +119,12 @@ const googleStrategyBuilder: AuthStrategyBuilder = async ( })(user.email, myUser.id) // Resolve redirect path - req.authRedirectPath = resolveAuthRedirectPathFactory()(validInvite) + req.authRedirectPath = resolveAuthRedirectPathFactory()(invite) // return to the auth flow return done(null, { ...myUser, - isInvite: !!validInvite + isInvite: !!invite }) } catch (err) { const e = ensureError( diff --git a/packages/server/modules/auth/strategies/local.ts b/packages/server/modules/auth/strategies/local.ts index 1fd6258a2..18f78b35a 100644 --- a/packages/server/modules/auth/strategies/local.ts +++ b/packages/server/modules/auth/strategies/local.ts @@ -112,7 +112,8 @@ const localStrategyBuilder: AuthStrategyBuilder = async ( ...user, role: invite ? getResourceTypeRole(invite.resource, ServerInviteResourceType) - : undefined + : undefined, + verified: !!invite }) req.user = { id: userId, diff --git a/packages/server/modules/auth/strategies/oidc.ts b/packages/server/modules/auth/strategies/oidc.ts index c4658f13f..597f8708b 100644 --- a/packages/server/modules/auth/strategies/oidc.ts +++ b/packages/server/modules/auth/strategies/oidc.ts @@ -29,6 +29,7 @@ import { getResourceTypeRole } from '@/modules/serverinvites/helpers/core' import { AuthStrategyBuilder } from '@/modules/auth/helpers/types' import { get } from 'lodash' import { Optional } from '@speckle/shared' +import { ServerInviteRecord } from '@/modules/serverinvites/domain/types' const oidcStrategyBuilder: AuthStrategyBuilder = async ( app, @@ -98,39 +99,27 @@ const oidcStrategyBuilder: AuthStrategyBuilder = async ( return done(null, myUser) } - // if the server is not invite only, go ahead and log the user in. - if (!serverInfo.inviteOnly) { - const myUser = await findOrCreateUser({ - user - }) - - // process invites - if (myUser.isNewUser) { - await finalizeInvitedServerRegistrationFactory({ - deleteServerOnlyInvites: deleteServerOnlyInvitesFactory({ db }), - updateAllInviteTargets: updateAllInviteTargetsFactory({ db }) - })(user.email, myUser.id) - } - return done(null, myUser) - } - // if the server is invite only and we have no invite id, throw. if (serverInfo.inviteOnly && !token) { throw new Error('This server is invite only. Please provide an invite id.') } - // validate the invite - const validInvite = await validateServerInviteFactory({ - findServerInvite: findServerInviteFactory({ db }) - })(user.email, token) + // validate the invite, if any + let invite: Optional = undefined + if (token) { + invite = await validateServerInviteFactory({ + findServerInvite: findServerInviteFactory({ db }) + })(user.email, token) + } // create the user const myUser = await findOrCreateUser({ user: { ...user, - role: validInvite - ? getResourceTypeRole(validInvite.resource, ServerInviteResourceType) - : undefined + role: invite + ? getResourceTypeRole(invite.resource, ServerInviteResourceType) + : undefined, + verified: !!invite } }) @@ -140,12 +129,12 @@ const oidcStrategyBuilder: AuthStrategyBuilder = async ( })(user.email, myUser.id) // Resolve redirect path - req.authRedirectPath = resolveAuthRedirectPathFactory()(validInvite) + req.authRedirectPath = resolveAuthRedirectPathFactory()(invite) // return to the auth flow return done(null, { ...myUser, - isInvite: !!validInvite + isInvite: !!invite }) } catch (err) { logger.error(err) diff --git a/packages/server/modules/auth/tests/helpers/registration.ts b/packages/server/modules/auth/tests/helpers/registration.ts index 7f48b94fd..a3f348a51 100644 --- a/packages/server/modules/auth/tests/helpers/registration.ts +++ b/packages/server/modules/auth/tests/helpers/registration.ts @@ -137,7 +137,7 @@ export const localAuthRestApi = (params: { express: Express }) => { const authCheck = async (params: { token: string }) => { const query = - 'query LocalAuthRestApiAuthCheck { activeUser { id email name role } }' + 'query LocalAuthRestApiAuthCheck { activeUser { id email name role emails { id email verified } } }' const res = await request(express) .post('/graphql') .set('Authorization', `Bearer ${params.token}`) @@ -148,7 +148,15 @@ export const localAuthRestApi = (params: { express: Express }) => { } const body = res.body as { - data: { activeUser?: { id: string; name: string; email: string; role: string } } + data: { + activeUser?: { + id: string + name: string + email: string + role: string + emails: Array<{ id: string; email: string; verified: boolean }> + } + } errors?: { message: string; extensions: Record }[] } if (!body.data.activeUser) { diff --git a/packages/server/modules/auth/tests/integration/registration.spec.ts b/packages/server/modules/auth/tests/integration/registration.spec.ts index 7430006c3..052dd4cf9 100644 --- a/packages/server/modules/auth/tests/integration/registration.spec.ts +++ b/packages/server/modules/auth/tests/integration/registration.spec.ts @@ -88,7 +88,10 @@ describe('Server registration', () => { const params = generateRegistrationParams() params.challenge = challenge - await restApi.register(params) + const user = await restApi.register(params) + + // email remains unverified + expect(user.emails.every((e) => !e.verified)).to.be.true }) it('fails without challenge', async () => { @@ -202,7 +205,9 @@ describe('Server registration', () => { itEach( [{ stream: true }, { stream: false }], ({ stream }) => - `works with valid ${stream ? 'stream' : 'server'} invite token`, + `works with valid ${ + stream ? 'stream' : 'server' + } invite token and auto-verifies email`, async ({ stream }) => { const challenge = 'bababooey' const params = generateRegistrationParams() @@ -221,6 +226,7 @@ describe('Server registration', () => { const newUser = await restApi.register(params) expect(newUser.role).to.equal(Roles.Server.Admin) + expect(newUser.emails.every((e) => e.verified)).to.be.true } ) }) diff --git a/packages/server/modules/core/services/users.js b/packages/server/modules/core/services/users.js index ca3c24d48..0a55294b8 100644 --- a/packages/server/modules/core/services/users.js +++ b/packages/server/modules/core/services/users.js @@ -156,7 +156,7 @@ module.exports = { }, /** - * @param {{user: {email: string, name?: string, role?: import('@speckle/shared').ServerRoles}, bio?: string}} param0 + * @param {{user: {email: string, name?: string, role?: import('@speckle/shared').ServerRoles, bio?: string, verified?: boolean}}} param0 * @returns {Promise<{ * id: string, * email: string,