diff --git a/openapi/spec/paths/api@user@mfa@auth.yaml b/openapi/spec/paths/api@user@mfa@auth.yaml index d4f497c008c..63fa2f82b13 100644 --- a/openapi/spec/paths/api@user@mfa@auth.yaml +++ b/openapi/spec/paths/api@user@mfa@auth.yaml @@ -4,6 +4,12 @@ post: description: | Authenticate a user who has MFA enabled. This endpoint should be called after the default authUser endpoint, which generates an `X-MFA-Token` indicating that the user has already authenticated with a password. + + The token logs in once only. An unknown, expired or already used token answers `401`, the same + as a wrong code, and so does a code sent while another code for the same user is still being + checked. A token whose user no longer exists answers `404`. Wrong codes count toward an account + lockout of their own, which a correct password does not clear: check the `X-Account-Lockout` + header, which is 0 when there is no active lockout. tags: - cloud - mfa @@ -17,6 +23,12 @@ post: responses: '200': description: Success to authenticate. + headers: + X-Account-Lockout: + schema: + type: integer + description: Indicates the Unix timestamp in seconds of when the account lockout period ends. + example: 0 content: application/json: schema: @@ -29,5 +41,17 @@ post: $ref: ../components/responses/403.yaml '404': $ref: ../components/responses/404.yaml + '429': + description: Too Many Requests + headers: + X-Account-Lockout: + schema: + type: integer + description: Indicates the Unix timestamp in seconds of when the account lockout period ends. + example: 1711575401 + content: + application/json: + schema: + $ref: ../components/schemas/apiError.yaml '500': $ref: ../components/responses/500.yaml diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 3cdbaa6a816..ac1d8a34fa1 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -39,6 +39,11 @@ type Cache interface { // exactly once under concurrent requests, which Get+Set cannot do race-free. SetNX(ctx context.Context, key string, value any, ttl time.Duration) (bool, error) + // CompareAndDelete atomically deletes key only while it still holds value, and reports whether + // it did. It releases a reservation taken with SetNX under a value unique to its owner, so an + // owner whose reservation expired cannot delete the one another caller took after it. + CompareAndDelete(ctx context.Context, key, value string) (bool, error) + // HasAccountLockout reports whether the source is currently blocked from attempting to // log in to a user with the specified userID. It returns the absolute Unix timestamp // in seconds representing the end of the lockout while clock.Now() is before it, or 0 diff --git a/pkg/cache/cache_null.go b/pkg/cache/cache_null.go index 87d6edbd370..8f6f2229076 100644 --- a/pkg/cache/cache_null.go +++ b/pkg/cache/cache_null.go @@ -31,6 +31,10 @@ func (*nullCache) Delete(_ context.Context, _ string) error { return nil } +func (*nullCache) CompareAndDelete(_ context.Context, _, _ string) (bool, error) { + return true, nil +} + func (*nullCache) HasAccountLockout(_ context.Context, _, _ string) (int64, int, error) { return 0, 0, nil } diff --git a/pkg/cache/cache_redis.go b/pkg/cache/cache_redis.go index 498e00e824f..adabb212249 100644 --- a/pkg/cache/cache_redis.go +++ b/pkg/cache/cache_redis.go @@ -74,6 +74,22 @@ func (c *redisCache) SetNX(ctx context.Context, key string, value any, ttl time. return c.client.SetNX(ctx, key, value, ttl).Result() } +var compareAndDeleteScript = redis.NewScript(` +if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) +end +return 0 +`) + +func (c *redisCache) CompareAndDelete(ctx context.Context, key, value string) (bool, error) { + deleted, err := compareAndDeleteScript.Run(ctx, c.client, []string{key}, value).Int() + if err != nil { + return false, err + } + + return deleted == 1, nil +} + // Delete deletes cached value by given key. func (c *redisCache) Delete(ctx context.Context, key string) error { if err := c.cache.Get(ctx, key, nil); errors.Is(err, rediscache.ErrCacheMiss) { diff --git a/pkg/cache/cache_redis_test.go b/pkg/cache/cache_redis_test.go index 9c8a123bd44..d7b9af23aa7 100644 --- a/pkg/cache/cache_redis_test.go +++ b/pkg/cache/cache_redis_test.go @@ -66,3 +66,40 @@ func TestRedisCacheLockoutEndsAtAnnouncedDeadline(t *testing.T) { require.NoError(t, c.Get(ctx, "account-lockout=10.0.0.1:user", &storedDeadline)) assert.Equal(t, "1700000060", storedDeadline) } + +func TestRedisCacheCompareAndDeleteKeepsAnotherOwnersKey(t *testing.T) { + ctx := context.Background() + + redisContainer, err := redis.Run(ctx, "docker.io/valkey/valkey:9.1-alpine") + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, redisContainer.Terminate(ctx)) + }) + + uri, err := redisContainer.ConnectionString(ctx) + require.NoError(t, err) + + c, err := cache.NewRedisCache(uri, 0) + require.NoError(t, err) + + taken, err := c.SetNX(ctx, "guard", "owner", time.Minute) + require.NoError(t, err) + require.True(t, taken) + + deleted, err := c.CompareAndDelete(ctx, "guard", "someone-else") + require.NoError(t, err) + assert.False(t, deleted) + + taken, err = c.SetNX(ctx, "guard", "intruder", time.Minute) + require.NoError(t, err) + assert.False(t, taken) + + deleted, err = c.CompareAndDelete(ctx, "guard", "owner") + require.NoError(t, err) + assert.True(t, deleted) + + taken, err = c.SetNX(ctx, "guard", "next", time.Minute) + require.NoError(t, err) + assert.True(t, taken) +} diff --git a/pkg/cache/mocks/mock_cache.go b/pkg/cache/mocks/mock_cache.go index 37f60cfb529..11e494ca7b2 100644 --- a/pkg/cache/mocks/mock_cache.go +++ b/pkg/cache/mocks/mock_cache.go @@ -47,6 +47,78 @@ func (_m *MockCache) EXPECT() *MockCache_Expecter { return &MockCache_Expecter{mock: &_m.Mock} } +// CompareAndDelete provides a mock function for the type MockCache +func (_mock *MockCache) CompareAndDelete(ctx context.Context, key string, value string) (bool, error) { + ret := _mock.Called(ctx, key, value) + + if len(ret) == 0 { + panic("no return value specified for CompareAndDelete") + } + + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) (bool, error)); ok { + return returnFunc(ctx, key, value) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) bool); ok { + r0 = returnFunc(ctx, key, value) + } else { + r0 = ret.Get(0).(bool) + } + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, key, value) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockCache_CompareAndDelete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CompareAndDelete' +type MockCache_CompareAndDelete_Call struct { + *mock.Call +} + +// CompareAndDelete is a helper method to define mock.On call +// - ctx context.Context +// - key string +// - value string +func (_e *MockCache_Expecter) CompareAndDelete(ctx any, key any, value any) *MockCache_CompareAndDelete_Call { + return &MockCache_CompareAndDelete_Call{Call: _e.mock.On("CompareAndDelete", ctx, key, value)} +} + +func (_c *MockCache_CompareAndDelete_Call) Run(run func(ctx context.Context, key string, value string)) *MockCache_CompareAndDelete_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 string + if args[1] != nil { + arg1 = args[1].(string) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *MockCache_CompareAndDelete_Call) Return(b bool, err error) *MockCache_CompareAndDelete_Call { + _c.Call.Return(b, err) + return _c +} + +func (_c *MockCache_CompareAndDelete_Call) RunAndReturn(run func(ctx context.Context, key string, value string) (bool, error)) *MockCache_CompareAndDelete_Call { + _c.Call.Return(run) + return _c +} + // Delete provides a mock function for the type MockCache func (_mock *MockCache) Delete(ctx context.Context, key string) error { ret := _mock.Called(ctx, key) diff --git a/ui/apps/console/src/api/fetchInterceptors.ts b/ui/apps/console/src/api/fetchInterceptors.ts index faf35a67c64..b9a09744e38 100644 --- a/ui/apps/console/src/api/fetchInterceptors.ts +++ b/ui/apps/console/src/api/fetchInterceptors.ts @@ -70,11 +70,13 @@ client.interceptors.response.use((response) => { if (mfaToken) { useAuthStore.getState().setMfaToken(mfaToken); } else { - const isLoginRequest = response.url.includes("/api/login"); + const isSignInStep = + response.url.includes("/api/login") || + response.url.includes("/api/user/mfa/auth"); const isTokenLogin = new URLSearchParams(window.location.search).has( "token", ); - if (!isLoginRequest && !isTokenLogin) { + if (!isSignInStep && !isTokenLogin) { useAuthStore.getState().logout(); window.location.href = "/login"; } diff --git a/ui/apps/console/src/components/auth/SignInForm.tsx b/ui/apps/console/src/components/auth/SignInForm.tsx index a2b5a1304b2..b99b5e02616 100644 --- a/ui/apps/console/src/components/auth/SignInForm.tsx +++ b/ui/apps/console/src/components/auth/SignInForm.tsx @@ -1,9 +1,13 @@ -import { useState, useEffect, FormEvent } from "react"; +import { useState, FormEvent } from "react"; import { useForm } from "react-hook-form"; import { useNavigate } from "react-router-dom"; import { Button, Callout } from "@shellhub/design-system/primitives"; import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; +import { + lockoutEndFrom, + useLockoutCountdown, +} from "@/hooks/useLockoutCountdown"; import { landingAfterSignIn } from "@/utils/navigation"; import { FormInputField, @@ -14,52 +18,6 @@ import MfaCodeForm from "@/components/mfa/MfaCodeForm"; import AuthActions, { type AuthLink } from "@/components/auth/AuthActions"; import type { LoginFormValues } from "@/pages/setup/loginResolver"; -interface CountdownState { - display: string; - expired: boolean; - epoch: number | null; -} - -function useLoginCountdown(lockoutEndEpoch: number | null) { - const [state, setState] = useState({ - display: "", - expired: false, - epoch: null, - }); - - useEffect(() => { - if (lockoutEndEpoch === null) return; - - const interval = setInterval(() => { - const diff = lockoutEndEpoch - Date.now() / 1000; - if (diff <= 0) { - clearInterval(interval); - setState({ display: "", expired: true, epoch: lockoutEndEpoch }); - } else if (diff < 60) { - const s = Math.floor(diff); - setState({ - display: `${s} ${s === 1 ? "second" : "seconds"}`, - expired: false, - epoch: lockoutEndEpoch, - }); - } else { - const m = Math.floor(diff / 60); - setState({ - display: `${m} ${m === 1 ? "minute" : "minutes"}`, - expired: false, - epoch: lockoutEndEpoch, - }); - } - }, 1000); - - return () => clearInterval(interval); - }, [lockoutEndEpoch]); - - if (state.epoch !== lockoutEndEpoch) return { display: "", expired: false }; - - return { display: state.display, expired: state.expired }; -} - interface SignInFormProps { redirect: string; submitLabel?: string; @@ -88,7 +46,7 @@ export default function SignInForm({ const [secondFactor, setSecondFactor] = useState(false); const [lockoutEndEpoch, setLockoutEndEpoch] = useState(null); const { display: countdownDisplay, expired: lockoutExpired } = - useLoginCountdown(lockoutEndEpoch); + useLockoutCountdown(lockoutEndEpoch); const { control, handleSubmit, formState } = useForm({ resolver: loginResolver, @@ -143,8 +101,7 @@ export default function SignInForm({ ); break; case 429: { - const epoch = Number(err.headers.get("x-account-lockout")); - setLockoutEndEpoch(isNaN(epoch) ? null : epoch); + setLockoutEndEpoch(lockoutEndFrom(err.headers)); setError( "Too many failed login attempts. Please wait before trying again.", ); diff --git a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx index 1f2f2c25a4b..688819673d8 100644 --- a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx +++ b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx @@ -1,6 +1,11 @@ -import type { FormEvent } from "react"; +import { useState, type FormEvent } from "react"; import { Button, Callout } from "@shellhub/design-system/primitives"; +import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; +import { + lockoutEndFrom, + useLockoutCountdown, +} from "@/hooks/useLockoutCountdown"; import { useOtpInput } from "@/hooks/useOtpInput"; import OtpCells from "@/components/mfa/OtpCells"; import AuthActions, { type AuthLink } from "@/components/auth/AuthActions"; @@ -14,7 +19,9 @@ interface MfaCodeFormProps { /** * The second step of a sign-in: the six cells for the authenticator code, exchanged for a full * session. A wrong code clears the cells and leaves the partial session in place, so the user - * can try again without signing in from scratch. `onVerified` runs once the session is full. + * can try again without signing in from scratch. Too many wrong codes lock the form out, with a + * countdown to when the user may try again, still without signing in from scratch. `onVerified` + * runs once the session is full. */ export default function MfaCodeForm({ onVerified, @@ -23,22 +30,43 @@ export default function MfaCodeForm({ }: MfaCodeFormProps) { const otp = useOtpInput(6); const { loginWithMfa, loading, error } = useAuthStore(); + const [lockoutEndEpoch, setLockoutEndEpoch] = useState(null); + const { display: countdownDisplay, expired: lockoutExpired } = + useLockoutCountdown(lockoutEndEpoch); const handleSubmit = async (e: FormEvent) => { e.preventDefault(); if (!otp.isComplete) return; + setLockoutEndEpoch(null); try { await loginWithMfa(otp.getValue()); onVerified(); - } catch { + } catch (err) { + if (isSdkError(err) && err.status === 429) { + setLockoutEndEpoch(lockoutEndFrom(err.headers)); + } otp.reset(); } }; return (
void handleSubmit(e)} className="space-y-6"> - {error && {error}} + {lockoutExpired && ( + + Your timeout has finished. Please enter a new code. + + )} + {error && !lockoutExpired && ( + + + {error} + {countdownDisplay ? ( + ({countdownDisplay}) + ) : null} + + + )} diff --git a/ui/apps/console/src/hooks/useLockoutCountdown.ts b/ui/apps/console/src/hooks/useLockoutCountdown.ts new file mode 100644 index 00000000000..0eebf85a13b --- /dev/null +++ b/ui/apps/console/src/hooks/useLockoutCountdown.ts @@ -0,0 +1,64 @@ +import { useState, useEffect } from "react"; + +interface CountdownState { + display: string; + expired: boolean; + epoch: number | null; +} + +/** + * Reads the end of an account lockout from a 429 response's `X-Account-Lockout` header, as the Unix + * timestamp in seconds that {@link useLockoutCountdown} takes. A missing, zero or malformed header + * gives `null`, so the form shows the lockout without a countdown rather than one that already ended. + */ +export function lockoutEndFrom(headers: Headers): number | null { + const epoch = Number(headers.get("x-account-lockout")); + + return epoch > 0 ? epoch : null; +} + +/** + * Counts down to the end of an account lockout, given as the Unix timestamp in seconds that the + * `X-Account-Lockout` header carries. `display` is the time left in whole minutes or seconds, empty + * until the first tick. `expired` turns true once the lockout ends, and both reset when a new + * lockout replaces the old one, so a second lockout never reads as already finished. + */ +export function useLockoutCountdown(lockoutEndEpoch: number | null) { + const [state, setState] = useState({ + display: "", + expired: false, + epoch: null, + }); + + useEffect(() => { + if (lockoutEndEpoch === null) return; + + const interval = setInterval(() => { + const diff = lockoutEndEpoch - Date.now() / 1000; + if (diff <= 0) { + clearInterval(interval); + setState({ display: "", expired: true, epoch: lockoutEndEpoch }); + } else if (diff < 60) { + const s = Math.floor(diff); + setState({ + display: `${s} ${s === 1 ? "second" : "seconds"}`, + expired: false, + epoch: lockoutEndEpoch, + }); + } else { + const m = Math.floor(diff / 60); + setState({ + display: `${m} ${m === 1 ? "minute" : "minutes"}`, + expired: false, + epoch: lockoutEndEpoch, + }); + } + }, 1000); + + return () => clearInterval(interval); + }, [lockoutEndEpoch]); + + if (state.epoch !== lockoutEndEpoch) return { display: "", expired: false }; + + return { display: state.display, expired: state.expired }; +} diff --git a/ui/apps/console/src/pages/__tests__/Login.test.tsx b/ui/apps/console/src/pages/__tests__/Login.test.tsx index 41a9f4cee3d..e74efd8e210 100644 --- a/ui/apps/console/src/pages/__tests__/Login.test.tsx +++ b/ui/apps/console/src/pages/__tests__/Login.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, screen, waitFor } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { MemoryRouter } from "react-router-dom"; import { http, HttpResponse } from "msw"; @@ -307,6 +307,21 @@ describe("Login", () => { screen.queryByText(/too many failed login attempts/i), ).not.toBeInTheDocument(); }); + + it("shows the lockout without a countdown when the deadline header is missing", async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + setLoginError(429); + + renderLogin(); + await fillAndSubmit(); + + await screen.findByText(/too many failed login attempts/i); + await act(() => vi.advanceTimersByTimeAsync(3000)); + expect( + screen.queryByText(/your timeout has finished/i), + ).not.toBeInTheDocument(); + expect(screen.queryByText(/seconds|minutes/i)).not.toBeInTheDocument(); + }); }); describe("under a browser-translated DOM", () => { diff --git a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx index e5577f5f593..b5bd73cfaee 100644 --- a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx +++ b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx @@ -1,6 +1,14 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; -import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + act, + render, + screen, + fireEvent, + waitFor, +} from "@testing-library/react"; import { MemoryRouter, Route, Routes } from "react-router-dom"; +import { http, HttpResponse } from "msw"; +import { server } from "@/tests/msw"; import { useAuthStore } from "@/stores/authStore"; import { PENDING_DEVICE_CODE_KEY, @@ -35,16 +43,30 @@ function submitCode() { fireEvent.click(screen.getByRole("button", { name: /verify/i })); } +function respondToCode(status: number, headers: Record = {}) { + server.use( + http.post("*/api/user/mfa/auth", () => + HttpResponse.json({}, { status, headers }), + ), + ); +} + +async function passSeconds(seconds: number) { + for (let i = 0; i < seconds; i++) { + await act(() => vi.advanceTimersByTimeAsync(1000)); + } +} + +function lockOutMfa(secondsLeft: number) { + const epoch = Math.floor(Date.now() / 1000) + secondsLeft; + respondToCode(429, { "x-account-lockout": String(epoch) }); +} + describe("MfaLogin", () => { beforeEach(() => { localStorage.removeItem(PENDING_DEVICE_CODE_KEY); - useAuthStore.setState({ - token: null, - mfaToken: "temp-mfa-token", - loading: false, - error: null, - loginWithMfa: vi.fn(), - }); + useAuthStore.setState(useAuthStore.getInitialState(), true); + useAuthStore.setState({ mfaToken: "temp-mfa-token" }); }); it("renders MFA login form when mfaToken exists", () => { @@ -95,6 +117,73 @@ describe("MfaLogin", () => { }); }); + it("keeps the user on the code step, with the token, after a wrong code", async () => { + respondToCode(401); + + renderMfaLogin(); + fillCode(6, "9"); + submitCode(); + + await screen.findByText("Invalid verification code"); + expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); + expect(screen.queryByText("Login Page")).not.toBeInTheDocument(); + }); + + describe("lockout", () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("shows the lockout and its remaining time instead of a wrong-code message", async () => { + lockOutMfa(30); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + expect( + screen.queryByText("Invalid verification code"), + ).not.toBeInTheDocument(); + await passSeconds(1); + expect(screen.getByText(/\(2[89] seconds\)/)).toBeInTheDocument(); + expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); + }); + + it("tells the user to try again once the lockout ends", async () => { + lockOutMfa(1); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + await passSeconds(3); + expect( + screen.getByText(/your timeout has finished/i), + ).toBeInTheDocument(); + }); + + it("shows the lockout without a countdown when the deadline header is missing", async () => { + respondToCode(429); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + await passSeconds(3); + expect( + screen.queryByText(/your timeout has finished/i), + ).not.toBeInTheDocument(); + expect(screen.queryByText(/seconds|minutes/i)).not.toBeInTheDocument(); + }); + }); + it("has link to recovery page", () => { renderMfaLogin(); diff --git a/ui/apps/console/src/stores/authStore.ts b/ui/apps/console/src/stores/authStore.ts index 4946a4821b9..d6cdeadaade 100644 --- a/ui/apps/console/src/stores/authStore.ts +++ b/ui/apps/console/src/stores/authStore.ts @@ -11,6 +11,7 @@ import { type UserOrigin, } from "../client"; import { queryClient } from "../api/queryClient"; +import { isSdkError } from "../api/errors"; import { tearDownChatwoot } from "../hooks/chatwootRuntime"; import { useVaultStore } from "./vaultStore"; import { useTerminalStore } from "./terminalStore"; @@ -235,7 +236,14 @@ export const useAuthStore = create()( mfaEnabled: true, loading: false, }); - } catch { + } catch (err) { + if (isSdkError(err) && err.status === 429) { + set({ + loading: false, + error: "Too many failed attempts. Please wait before trying again.", + }); + throw err; + } set({ loading: false, error: "Invalid verification code" }); throw new Error("Invalid verification code"); }