From 43712839fddbb1038b196347b92a534b46c59659 Mon Sep 17 00:00:00 2001 From: fengmk2 Date: Wed, 12 Mar 2025 06:53:29 +0000 Subject: [PATCH] refactor(server): improve magic link login flow (#10736) --- .../src/__tests__/auth/controller.spec.ts | 95 +++++++++++++++++++ .../server/src/core/auth/controller.ts | 40 ++++++-- .../ios/App/App/Plugins/Auth/AuthPlugin.swift | 3 +- packages/frontend/apps/ios/src/app.tsx | 3 +- .../apps/ios/src/plugins/auth/definitions.ts | 1 + .../core/src/modules/cloud/impl/auth.ts | 8 +- .../core/src/modules/cloud/provider/auth.ts | 6 +- .../core/src/modules/cloud/services/auth.ts | 2 + .../core/src/modules/cloud/stores/auth.ts | 6 +- 9 files changed, 149 insertions(+), 15 deletions(-) diff --git a/packages/backend/server/src/__tests__/auth/controller.spec.ts b/packages/backend/server/src/__tests__/auth/controller.spec.ts index 7e9eb6a007..30623caf36 100644 --- a/packages/backend/server/src/__tests__/auth/controller.spec.ts +++ b/packages/backend/server/src/__tests__/auth/controller.spec.ts @@ -1,3 +1,5 @@ +import { randomUUID } from 'node:crypto'; + import { HttpStatus } from '@nestjs/common'; import { PrismaClient } from '@prisma/client'; import ava, { TestFn } from 'ava'; @@ -249,3 +251,96 @@ test('should be able to sign out multiple accounts in one session', async t => { session = await app.GET('/api/auth/session').expect(200); t.falsy(session.body.user); }); + +test('should be able to sign in with email and client nonce', async t => { + const { app, mailer } = t.context; + + const clientNonce = randomUUID(); + const u1 = await app.createUser(); + // @ts-expect-error mock + mailer.sendSignInMail.resolves({ rejected: [] }); + + const res = await app + .POST('/api/auth/sign-in') + .send({ email: u1.email, client_nonce: clientNonce }) + .expect(200); + + t.is(res.body.email, u1.email); + t.true(mailer.sendSignInMail.calledOnce); + + const [, { url: signInLink }] = mailer.sendSignInMail.firstCall.args; + const url = new URL(signInLink); + const email = url.searchParams.get('email'); + const token = url.searchParams.get('token'); + + await app + .POST('/api/auth/magic-link') + .send({ email, token, client_nonce: clientNonce }) + .expect(201); + + const session = await currentUser(app); + t.is(session?.id, u1.id); +}); + +test('should not be able to sign in with email and client nonce if invalid', async t => { + const { app, mailer } = t.context; + + const clientNonce = randomUUID(); + const u1 = await app.createUser(); + // @ts-expect-error mock + mailer.sendSignInMail.resolves({ rejected: [] }); + + const res = await app + .POST('/api/auth/sign-in') + .send({ email: u1.email, client_nonce: clientNonce }) + .expect(200); + + t.is(res.body.email, u1.email); + t.true(mailer.sendSignInMail.calledOnce); + + const [, { url: signInLink }] = mailer.sendSignInMail.firstCall.args; + const url = new URL(signInLink); + const email = url.searchParams.get('email'); + const token = url.searchParams.get('token'); + + // invalid client nonce + await app + .POST('/api/auth/magic-link') + .send({ email, token, client_nonce: randomUUID() }) + .expect(400) + .expect({ + status: 400, + code: 'Bad Request', + type: 'BAD_REQUEST', + name: 'INVALID_AUTH_STATE', + message: + 'Invalid auth state. You might start the auth progress from another device.', + }); + // no client nonce + await app + .POST('/api/auth/magic-link') + .send({ email, token }) + .expect(400) + .expect({ + status: 400, + code: 'Bad Request', + type: 'BAD_REQUEST', + name: 'INVALID_AUTH_STATE', + message: + 'Invalid auth state. You might start the auth progress from another device.', + }); + + const session = await currentUser(app); + t.falsy(session); +}); + +test('should not be able to sign in if token is invalid', async t => { + const { app } = t.context; + + const res = await app + .POST('/api/auth/magic-link') + .send({ email: 'u1@affine.pro', token: 'invalid' }) + .expect(400); + + t.is(res.body.message, 'An invalid email token provided.'); +}); diff --git a/packages/backend/server/src/core/auth/controller.ts b/packages/backend/server/src/core/auth/controller.ts index 34a4693c7b..a5b067fad9 100644 --- a/packages/backend/server/src/core/auth/controller.ts +++ b/packages/backend/server/src/core/auth/controller.ts @@ -21,6 +21,7 @@ import { EarlyAccessRequired, EmailTokenNotFound, InternalServerError, + InvalidAuthState, InvalidEmail, InvalidEmailToken, Runtime, @@ -45,11 +46,13 @@ interface SignInCredential { email: string; password?: string; callbackUrl?: string; + client_nonce?: string; } interface MagicLinkCredential { email: string; token: string; + client_nonce?: string; } const OTP_CACHE_KEY = (otp: string) => `magic-link-otp:${otp}`; @@ -140,7 +143,8 @@ export class AuthController { res, credential.email, credential.callbackUrl, - redirectUri + redirectUri, + credential.client_nonce ); } } @@ -162,7 +166,8 @@ export class AuthController { res: Response, email: string, callbackUrl = '/magic-link', - redirectUrl?: string + redirectUrl?: string, + clientNonce?: string ) { // send email magic link const user = await this.models.user.getUserByEmail(email); @@ -210,7 +215,11 @@ export class AuthController { const otp = this.crypto.otp(); // TODO(@forehalo): this is a temporary solution, we should not rely on cache to store the otp const cacheKey = OTP_CACHE_KEY(otp); - await this.cache.set(cacheKey, token, { ttl: ttlInSec * 1000 }); + await this.cache.set( + cacheKey, + { token, clientNonce }, + { ttl: ttlInSec * 1000 } + ); const magicLink = this.url.link(callbackUrl, { token: otp, @@ -266,24 +275,37 @@ export class AuthController { async magicLinkSignIn( @Req() req: Request, @Res() res: Response, - @Body() { email, token }: MagicLinkCredential + @Body() + { email, token: otp, client_nonce: clientNonce }: MagicLinkCredential ) { - if (!token || !email) { + if (!otp || !email) { throw new EmailTokenNotFound(); } validators.assertValidEmail(email); - const cacheKey = OTP_CACHE_KEY(token); - const cachedToken = await this.cache.get(cacheKey); + const cacheKey = OTP_CACHE_KEY(otp); + const cachedToken = await this.cache.get< + { token: string; clientNonce: string } | string + >(cacheKey); + let token: string | undefined; + // TODO(@fengmk2): this is a temporary compatible with cache token is string value, should be removed in 0.22 + if (typeof cachedToken === 'string') { + token = cachedToken; + } else if (cachedToken) { + token = cachedToken.token; + if (cachedToken.clientNonce && cachedToken.clientNonce !== clientNonce) { + throw new InvalidAuthState(); + } + } - if (!cachedToken) { + if (!token) { throw new InvalidEmailToken(); } const tokenRecord = await this.models.verificationToken.verify( TokenType.SignIn, - cachedToken, + token, { credential: email, } diff --git a/packages/frontend/apps/ios/App/App/Plugins/Auth/AuthPlugin.swift b/packages/frontend/apps/ios/App/App/Plugins/Auth/AuthPlugin.swift index af9fb945f3..ccc39c57a1 100644 --- a/packages/frontend/apps/ios/App/App/Plugins/Auth/AuthPlugin.swift +++ b/packages/frontend/apps/ios/App/App/Plugins/Auth/AuthPlugin.swift @@ -17,8 +17,9 @@ public class AuthPlugin: CAPPlugin, CAPBridgedPlugin { let endpoint = try call.getStringEnsure("endpoint") let email = try call.getStringEnsure("email") let token = try call.getStringEnsure("token") + let clientNonce = call.getString("clientNonce") - let (data, response) = try await self.fetch(endpoint, method: "POST", action: "/api/auth/magic-link", headers: [:], body: ["email": email, "token": token]) + let (data, response) = try await self.fetch(endpoint, method: "POST", action: "/api/auth/magic-link", headers: [:], body: ["email": email, "token": token, "client_nonce": clientNonce]) if response.statusCode >= 400 { if let textBody = String(data: data, encoding: .utf8) { diff --git a/packages/frontend/apps/ios/src/app.tsx b/packages/frontend/apps/ios/src/app.tsx index b34dd6d5f5..12546b68a5 100644 --- a/packages/frontend/apps/ios/src/app.tsx +++ b/packages/frontend/apps/ios/src/app.tsx @@ -169,11 +169,12 @@ framework.scope(ServerScope).override(AuthProvider, resolver => { const serverService = resolver.get(ServerService); const endpoint = serverService.server.baseUrl; return { - async signInMagicLink(email, linkToken) { + async signInMagicLink(email, linkToken, clientNonce) { const { token } = await Auth.signInMagicLink({ endpoint, email, token: linkToken, + clientNonce, }); await writeEndpointToken(endpoint, token); }, diff --git a/packages/frontend/apps/ios/src/plugins/auth/definitions.ts b/packages/frontend/apps/ios/src/plugins/auth/definitions.ts index 160b18b20a..8e5ffae4ad 100644 --- a/packages/frontend/apps/ios/src/plugins/auth/definitions.ts +++ b/packages/frontend/apps/ios/src/plugins/auth/definitions.ts @@ -3,6 +3,7 @@ export interface AuthPlugin { endpoint: string; email: string; token: string; + clientNonce?: string; }): Promise<{ token: string }>; signInOauth(options: { endpoint: string; diff --git a/packages/frontend/core/src/modules/cloud/impl/auth.ts b/packages/frontend/core/src/modules/cloud/impl/auth.ts index 2655e6f71a..6c46fbcb76 100644 --- a/packages/frontend/core/src/modules/cloud/impl/auth.ts +++ b/packages/frontend/core/src/modules/cloud/impl/auth.ts @@ -8,13 +8,17 @@ export function configureDefaultAuthProvider(framework: Framework) { framework.scope(ServerScope).override(AuthProvider, resolver => { const fetchService = resolver.get(FetchService); return { - async signInMagicLink(email: string, token: string) { + async signInMagicLink( + email: string, + token: string, + clientNonce?: string + ) { await fetchService.fetch('/api/auth/magic-link', { method: 'POST', headers: { 'Content-Type': 'application/json', }, - body: JSON.stringify({ email, token }), + body: JSON.stringify({ email, token, client_nonce: clientNonce }), }); }, diff --git a/packages/frontend/core/src/modules/cloud/provider/auth.ts b/packages/frontend/core/src/modules/cloud/provider/auth.ts index d326a11a69..f82f9e4940 100644 --- a/packages/frontend/core/src/modules/cloud/provider/auth.ts +++ b/packages/frontend/core/src/modules/cloud/provider/auth.ts @@ -1,7 +1,11 @@ import { createIdentifier } from '@toeverything/infra'; export interface AuthProvider { - signInMagicLink(email: string, token: string): Promise; + signInMagicLink( + email: string, + token: string, + clientNonce?: string + ): Promise; signInOauth( code: string, diff --git a/packages/frontend/core/src/modules/cloud/services/auth.ts b/packages/frontend/core/src/modules/cloud/services/auth.ts index 5a7c5a02d6..61e9b0dd71 100644 --- a/packages/frontend/core/src/modules/cloud/services/auth.ts +++ b/packages/frontend/core/src/modules/cloud/services/auth.ts @@ -79,6 +79,7 @@ export class AuthService extends Service { redirectUrl?: string // url to redirect to after signed-in ) { track.$.$.auth.signIn({ method: 'magic-link' }); + this.setClientNonce(); try { const scheme = this.urlService.getClientScheme(); const magicLinkUrlParams = new URLSearchParams(); @@ -95,6 +96,7 @@ export class AuthService extends Service { // we call it [callbackUrl] instead of [redirect_uri] // to make it clear the url is used to finish the sign-in process instead of redirect after signed-in callbackUrl: `/magic-link?${magicLinkUrlParams.toString()}`, + client_nonce: this.store.getClientNonce(), }), headers: { 'content-type': 'application/json', diff --git a/packages/frontend/core/src/modules/cloud/stores/auth.ts b/packages/frontend/core/src/modules/cloud/stores/auth.ts index c75c3b901d..a9774a6f2c 100644 --- a/packages/frontend/core/src/modules/cloud/stores/auth.ts +++ b/packages/frontend/core/src/modules/cloud/stores/auth.ts @@ -74,7 +74,11 @@ export class AuthStore extends Store { } async signInMagicLink(email: string, token: string) { - await this.authProvider.signInMagicLink(email, token); + await this.authProvider.signInMagicLink( + email, + token, + this.getClientNonce() + ); } async signInOauth(code: string, state: string, provider: string) {