diff --git a/apps/admin/src/containers/UserDecisionsContainer.tsx b/apps/admin/src/containers/UserDecisionsContainer.tsx index 321422c4..7e248b25 100644 --- a/apps/admin/src/containers/UserDecisionsContainer.tsx +++ b/apps/admin/src/containers/UserDecisionsContainer.tsx @@ -50,7 +50,7 @@ export function UserDecisionsContainer({ onSettled, onDeleted, }: UserDecisionsContainerProps) { - const { session } = useSession(); + const { session, signOut } = useSession(); const [openAction, setOpenAction] = useState(null); const [reason, setReason] = useState(''); const [role, setRole] = useState(user.role); @@ -69,6 +69,7 @@ export function UserDecisionsContainer({ close(); onDeleted(); }, + close, ); const member = session.status === 'signedIn' ? session.member : null; const allowed = member ? allowedUserActions(member, user) : []; @@ -161,6 +162,17 @@ export function UserDecisionsContainer({ confirmLabel={texts.delete.confirm} isBusy={actions.isBusy} /> + void signOut()} + title={texts.signInAgain.title} + description={texts.signInAgain.description} + cancelLabel={messages.confirm.cancel} + closeLabel={messages.confirm.close} + confirmLabel={texts.signInAgain.confirm} + confirmVariant="primary" + /> ); } diff --git a/apps/admin/src/hooks/useUserActions.ts b/apps/admin/src/hooks/useUserActions.ts index 16fa96c3..9175c1d8 100644 --- a/apps/admin/src/hooks/useUserActions.ts +++ b/apps/admin/src/hooks/useUserActions.ts @@ -1,5 +1,6 @@ import { getSessionClients } from '@/lib/auth/clients'; import { downloadTextFile } from '@/lib/download'; +import { isRecentSignInRequired } from '@/lib/recent-sign-in'; import { messages } from '@/messages/fr'; import { ApiRequestError } from '@widoo/api-client'; import { @@ -33,10 +34,18 @@ function failureTitleOf(error: unknown): string { * The actions of the team on one account (A-06), each told in a toast. `onSettled` is called when * the action is done, and also after a conflict (another member got there first, or the account * no longer allows it): the screen then reads the account again. `onDeleted` replaces it for the - * deletion, which leaves no card to read. + * deletion, which leaves no card to read. A sign-in too old for a sensitive action is no failure to + * tell in a toast: `isSignInRequired` says the person must sign in again, and `onSignInRequired` + * closes the window of the action. */ -export function useUserActions(userId: string, onSettled: () => void, onDeleted: () => void) { +export function useUserActions( + userId: string, + onSettled: () => void, + onDeleted: () => void, + onSignInRequired: () => void, +) { const [isBusy, setIsBusy] = useState(false); + const [isSignInRequired, setIsSignInRequired] = useState(false); const { api } = getSessionClients(); const send = async (action: () => Promise, successTitle: string, done = onSettled) => { @@ -46,6 +55,11 @@ export function useUserActions(userId: string, onSettled: () => void, onDeleted: showToast('success', { title: successTitle }); done(); } catch (error) { + if (isRecentSignInRequired(error)) { + onSignInRequired(); + setIsSignInRequired(true); + return; + } showToast('error', { title: failureTitleOf(error) }); if (error instanceof ApiRequestError && error.status === 409) { onSettled(); @@ -57,6 +71,8 @@ export function useUserActions(userId: string, onSettled: () => void, onDeleted: return { isBusy, + isSignInRequired, + dismissSignInRequired: () => setIsSignInRequired(false), changeRole: (role: UserRole) => send(() => api.changeUserRole(userId, { role }), texts.roleChanged), suspend: (reason: string) => send(() => api.suspendUser(userId, { reason }), texts.suspended), diff --git a/apps/admin/src/lib/recent-sign-in.test.ts b/apps/admin/src/lib/recent-sign-in.test.ts new file mode 100644 index 00000000..640ff263 --- /dev/null +++ b/apps/admin/src/lib/recent-sign-in.test.ts @@ -0,0 +1,18 @@ +import { ApiRequestError } from '@widoo/api-client'; +import { describe, expect, it } from 'vitest'; +import { isRecentSignInRequired } from './recent-sign-in'; + +const refusal = (code: 'recent_sign_in_required' | 'forbidden') => + new ApiRequestError('http', 403, { code, message: 'Refused' }); + +describe('isRecentSignInRequired', () => { + it('recognises the refusal of an old sign-in, and no other 403', () => { + expect(isRecentSignInRequired(refusal('recent_sign_in_required'))).toBe(true); + expect(isRecentSignInRequired(refusal('forbidden'))).toBe(false); + }); + + it('is false for anything that is not an answer of the API', () => { + expect(isRecentSignInRequired(new Error('network'))).toBe(false); + expect(isRecentSignInRequired(undefined)).toBe(false); + }); +}); diff --git a/apps/admin/src/lib/recent-sign-in.ts b/apps/admin/src/lib/recent-sign-in.ts new file mode 100644 index 00000000..e7f09575 --- /dev/null +++ b/apps/admin/src/lib/recent-sign-in.ts @@ -0,0 +1,10 @@ +import { ApiRequestError } from '@widoo/api-client'; + +/** + * The API refuses a sensitive action (a role, a suspension or a deletion) to a sign-in that is + * too old (403 `recent_sign_in_required`, wiki Admin-Web › Sécurité): the person signs in again, + * with the code of their authenticator app, and starts the action over. + */ +export function isRecentSignInRequired(error: unknown): boolean { + return error instanceof ApiRequestError && error.code === 'recent_sign_in_required'; +} diff --git a/apps/admin/src/messages/fr.ts b/apps/admin/src/messages/fr.ts index 817af395..206dd146 100644 --- a/apps/admin/src/messages/fr.ts +++ b/apps/admin/src/messages/fr.ts @@ -867,6 +867,12 @@ export const messages = { delete: 'Supprimer le compte', }, dialogs: { + signInAgain: { + title: 'Reconnexion nécessaire', + description: + 'Pour votre sécurité, cette action demande une connexion récente avec le code de votre application d’authentification. Reconnectez-vous : vous reviendrez sur cette page, puis vous recommencerez l’action.', + confirm: 'Se reconnecter', + }, reason: 'Motif', reasonHint: "Gardé dans le journal et sur le compte, visible par l'équipe.", role: { diff --git a/apps/api/.env.example b/apps/api/.env.example index b0d01b25..6725fd5f 100644 --- a/apps/api/.env.example +++ b/apps/api/.env.example @@ -18,6 +18,9 @@ CORS_ORIGINS=http://localhost:5173 TRUST_PROXY= # Requests per minute and per client IP, all routes together. RATE_LIMIT_MAX=1000 +# Minutes before a sensitive action of the admin (role, suspension, deletion of an account) asks +# to confirm the sign-in again. 10 on staging and production; shorter only to try it locally. +ADMIN_RECENT_SIGN_IN_MINUTES=10 # Firebase project whose ID tokens the API accepts; staging and production each have their own. # No key file: firebase-admin uses Application Default Credentials (the Cloud Run service # account). A demo-* project only exists in the Auth emulator and is refused in production. diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 40d6a54d..ac3a085e 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -141,7 +141,10 @@ export async function buildApp(config: Config, options: BuildAppOptions = {}) { app.addHook('onClose', async () => { await sql.end({ timeout: 5 }); }); - registerAuth(app, tokenVerifier, { isSecondFactorRequired: !config.usesAuthEmulator }); + registerAuth(app, tokenVerifier, { + isSecondFactorRequired: !config.usesAuthEmulator, + recentSignInMaxMs: config.adminRecentSignInMinutes * 60_000, + }); if (!config.isProduction) { await registerDocs(app); diff --git a/apps/api/src/auth/plugin.ts b/apps/api/src/auth/plugin.ts index af62fc18..22237ff7 100644 --- a/apps/api/src/auth/plugin.ts +++ b/apps/api/src/auth/plugin.ts @@ -19,6 +19,12 @@ declare module 'fastify' { * session opened without the code of an authenticator app (403 `mfa_required`, D-087). */ requireAdminSection: (section: AdminSection) => AuthHook; + /** + * Added to the route of a sensitive action of the admin, after `requireAdminSection`: refuses + * a session whose last sign-in, with its second factor, is older than the delay of + * `AuthOptions.recentSignInMaxMs` (403 `recent_sign_in_required`, wiki Admin-Web › Sécurité). + */ + requireRecentSignIn: AuthHook; /** * Optional authentication (wiki API › conventions): without an `Authorization` header the * caller stays anonymous; with one, the same checks as `requireAuth`, 401 included. @@ -45,6 +51,8 @@ export type AuthOptions = { * session without a second factor in. Always true on staging and production. */ isSecondFactorRequired: boolean; + /** How recent the sign-in must be for a sensitive action of the admin, in milliseconds. */ + recentSignInMaxMs: number; }; /** @@ -128,4 +136,18 @@ export function registerAuth( throw httpError(403, 'Second factor required', 'mfa_required'); } }); + // `auth_time` is renewed by a new sign-in, with its second factor, and kept by token refreshes. + // With the local Auth emulator, which has no second factor, nothing is asked, as above. + app.decorate('requireRecentSignIn', async (request: FastifyRequest) => { + const identity = request.identity; + if (!identity) { + throw httpError(401, 'Missing bearer token'); + } + if ( + options.isSecondFactorRequired && + Date.now() - identity.authTime.getTime() > options.recentSignInMaxMs + ) { + throw httpError(403, 'Recent sign-in required', 'recent_sign_in_required'); + } + }); } diff --git a/apps/api/src/config.test.ts b/apps/api/src/config.test.ts index 425b65da..e61d6296 100644 --- a/apps/api/src/config.test.ts +++ b/apps/api/src/config.test.ts @@ -17,6 +17,7 @@ describe('loadConfig', () => { corsOrigins: [], trustedProxies: [], rateLimitMax: 1000, + adminRecentSignInMinutes: 10, firebaseProjectId: undefined, usesAuthEmulator: false, sentryDsn: undefined, @@ -165,4 +166,12 @@ describe('loadConfig', () => { }); expect(load).toThrow(/demo- project/); }); + + it('reads the delay of the sign-in asked before a sensitive action of the admin', () => { + const load = (minutes?: string) => + loadConfig({ DATABASE_URL: databaseUrl, ADMIN_RECENT_SIGN_IN_MINUTES: minutes }); + expect(load().adminRecentSignInMinutes).toBe(10); + expect(load('5').adminRecentSignInMinutes).toBe(5); + expect(() => load('0')).toThrow(/ADMIN_RECENT_SIGN_IN_MINUTES/); + }); }); diff --git a/apps/api/src/config.ts b/apps/api/src/config.ts index f06d7214..b1e36c13 100644 --- a/apps/api/src/config.ts +++ b/apps/api/src/config.ts @@ -32,6 +32,11 @@ const Env = z.object({ .transform((proxies) => proxies.filter(Boolean)) .pipe(z.array(Proxy)), RATE_LIMIT_MAX: z.coerce.number().int().positive().default(1000), + /** + * Minutes after which a sensitive action of the admin (role, suspension or deletion of an + * account) asks to confirm the sign-in again. 10, as the wiki says; shorter only to try it. + */ + ADMIN_RECENT_SIGN_IN_MINUTES: z.coerce.number().int().min(1).max(720).default(10), /** * Required by the server (`buildApp`), not by the database scripts. Google Cloud project ID * rules: 6 to 30 lowercase letters, digits and hyphens. @@ -171,6 +176,7 @@ export const Config = ProductionEnv.transform((env) => ({ corsOrigins: env.CORS_ORIGINS, trustedProxies: env.TRUST_PROXY, rateLimitMax: env.RATE_LIMIT_MAX, + adminRecentSignInMinutes: env.ADMIN_RECENT_SIGN_IN_MINUTES, firebaseProjectId: env.FIREBASE_PROJECT_ID, /** * The local Auth emulator signs the tokens (never in production, refused above). It has no diff --git a/apps/api/src/routes/admin-users.test.ts b/apps/api/src/routes/admin-users.test.ts index 4c1ab76a..b4df677b 100644 --- a/apps/api/src/routes/admin-users.test.ts +++ b/apps/api/src/routes/admin-users.test.ts @@ -41,8 +41,8 @@ afterAll(async () => { }); /** An account of this file, signed in with a second factor; `firstName` carries the marker. */ -async function signIn(role: UserRole, name = 'Membre') { - const { token, uid } = verifier.issue({ secondFactor: 'totp' }); +async function signIn(role: UserRole, name = 'Membre', authTime: Date | undefined = undefined) { + const { token, uid } = verifier.issue({ secondFactor: 'totp', ...(authTime && { authTime }) }); createdUids.push(uid); const [user] = await app.db .insert(users) @@ -413,3 +413,47 @@ describe('certification, export and deletion', () => { expect(await journalOf(target.id)).toEqual(['user.delete']); }); }); + +describe('sensitive actions after a while without a sign-in', () => { + // The last sign-in with the second factor, 11 minutes ago: the delay is 10. + const elevenMinutesAgo = () => new Date(Date.now() - 11 * 60_000); + + it('refuses the role, the suspension and the deletion, and writes nothing', async () => { + const admin = await signIn('admin', 'Admin', elevenMinutesAgo()); + const target = await signIn('user'); + const calls = [ + send('PATCH', admin.headers, `/users/${target.id}/role`, { role: 'editor' }), + send('POST', admin.headers, `/users/${target.id}/suspend`, { reason: 'Test' }), + send('DELETE', admin.headers, `/users/${target.id}`), + ]; + for (const response of await Promise.all(calls)) { + expect(response.statusCode).toBe(403); + expect(ApiError.parse(response.json()).code).toBe('recent_sign_in_required'); + } + expect(await journalOf(target.id)).toEqual([]); + const [unchanged] = await app.db.select().from(users).where(eq(users.id, target.id)); + expect(unchanged).toMatchObject({ role: 'user', suspendedAt: null, deletedAt: null }); + }); + + it('lets the same action through once the sign-in is recent', async () => { + const admin = await signIn('admin', 'Admin'); + const target = await signIn('user'); + const response = await send('PATCH', admin.headers, `/users/${target.id}/role`, { + role: 'editor', + }); + expect(response.statusCode).toBe(200); + }); + + it('does not ask for it to read, nor to lift a suspension', async () => { + const recent = await signIn('admin', 'Admin'); + const stale = await signIn('admin', 'Admin', elevenMinutesAgo()); + const target = await signIn('user'); + await send('POST', recent.headers, `/users/${target.id}/suspend`, { reason: 'Test' }); + + expect((await get(stale.headers, `/users/${target.id}`)).statusCode).toBe(200); + const lifted = await send('POST', stale.headers, `/users/${target.id}/reactivate`, { + reason: 'Test', + }); + expect(lifted.statusCode).toBe(200); + }); +}); diff --git a/apps/api/src/routes/admin-users.ts b/apps/api/src/routes/admin-users.ts index 9fad03ca..577bba6e 100644 --- a/apps/api/src/routes/admin-users.ts +++ b/apps/api/src/routes/admin-users.ts @@ -87,6 +87,7 @@ export const adminUsersRoutes: FastifyPluginAsyncZod = async (app) => { app.patch( '/admin/users/:id/role', { + onRequest: app.requireRecentSignIn, schema: { tags, summary: 'Change the role of an account (admin only, never one’s own)', @@ -103,6 +104,7 @@ export const adminUsersRoutes: FastifyPluginAsyncZod = async (app) => { app.post( '/admin/users/:id/suspend', { + onRequest: app.requireRecentSignIn, schema: { tags, summary: 'Suspend an account', @@ -206,6 +208,7 @@ export const adminUsersRoutes: FastifyPluginAsyncZod = async (app) => { app.delete( '/admin/users/:id', { + onRequest: app.requireRecentSignIn, schema: { tags, summary: 'Delete an account, with the rule of the app', diff --git a/packages/shared/src/schemas/error.ts b/packages/shared/src/schemas/error.ts index 11d09f60..94db069a 100644 --- a/packages/shared/src/schemas/error.ts +++ b/packages/shared/src/schemas/error.ts @@ -14,6 +14,8 @@ export const apiErrorCodes = [ 'route_removed', /** 403 on `/admin/*`: the session was opened without a second factor (D-087). */ 'mfa_required', + /** 403 on a sensitive action of the admin: the last sign-in is too old, confirm it again. */ + 'recent_sign_in_required', /** 403 everywhere: the account was suspended by the team (wiki Admin-Web › A-06). */ 'account_suspended', /** 409 on a moderation decision: another member of the team decided first (D-087). */