mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix(auth): close the MFA policy lockout and add self-service enrollment (#531)
Fixes #529 (opened investigating #515). Setting MFA policy to "required"/"admins only" saved regardless of whether the mfa enterprise feature was licensed, and there was no enrollment UI at all, so any instance that flipped the toggle locked every unenrolled user out with no way back in. The login page and Settings save also both collapsed the resulting error into a generic message, hiding the real reason. - Reject saving mfaPolicy to admins_only/required server-side unless mfa is licensed - Surface the specific server error on login and on a failed settings save instead of a generic fallback - Add a self-service two-factor authentication enrollment flow (QR code, manual entry, recovery codes, verify, disable) so a licensed admin can actually satisfy the policy before it's enforced - Fix a pending-enrollment dead end, silent error swallowing in verify/disable, and a silent clipboard-copy failure on the recovery codes screen - Add the integration test that actually proves the fix: a real login attempt returns 403 MFA_ENROLLMENT_REQUIRED
This commit is contained in:
@@ -6,7 +6,7 @@ vi.resetModules();
|
||||
const { mockEnterpriseFeatures } = await import("../../helpers/enterprise-mock.js");
|
||||
mockEnterpriseFeatures(["mfa"]);
|
||||
|
||||
const { buildTestApp, loginAsAdmin } = await import("../test-server.js");
|
||||
const { buildTestApp, loginAsAdmin, loginAsUser } = await import("../test-server.js");
|
||||
const { db, schema } = await import("../../../apps/api/src/db/index.js");
|
||||
|
||||
import type { TestApp } from "../test-server.js";
|
||||
@@ -112,6 +112,36 @@ describe("POST /api/auth/mfa/enroll", () => {
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.code).toBe("MFA_ALREADY_ENABLED");
|
||||
});
|
||||
|
||||
it("restarting enrollment after canceling (no verify in between) issues a fresh, working secret instead of 409ing", async () => {
|
||||
// First attempt: user clicks Enable, sees the QR, then cancels/abandons
|
||||
// without verifying. This leaves a pending, unverified secret.
|
||||
await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/mfa/enroll",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
|
||||
// Second attempt: user clicks Enable again later. Must not be a dead
|
||||
// end requiring an admin reset -- it should just issue a new secret.
|
||||
const secondEnrollRes = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/mfa/enroll",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
expect(secondEnrollRes.statusCode).toBe(200);
|
||||
const { uri: secondUri } = JSON.parse(secondEnrollRes.body);
|
||||
|
||||
// The new secret is genuinely live: verifying with it actually works.
|
||||
const code = generateTotpCode(secondUri);
|
||||
const verifyRes = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/mfa/verify",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
payload: { code },
|
||||
});
|
||||
expect(verifyRes.statusCode).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
describe("POST /api/auth/mfa/verify", () => {
|
||||
@@ -244,6 +274,47 @@ describe("POST /api/auth/users/:id/mfa/reset", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /api/auth/session totpEnabled", () => {
|
||||
afterEach(async () => {
|
||||
await clearMfaState("admin");
|
||||
});
|
||||
|
||||
it("is false when the user has not enrolled", async () => {
|
||||
const res = await testApp.app.inject({
|
||||
method: "GET",
|
||||
url: "/api/auth/session",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
expect(res.statusCode).toBe(200);
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.user.totpEnabled).toBe(false);
|
||||
});
|
||||
|
||||
it("is true once the user has completed enrollment", async () => {
|
||||
const enrollRes = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/mfa/enroll",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
const { uri } = JSON.parse(enrollRes.body);
|
||||
const code = generateTotpCode(uri);
|
||||
await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/mfa/verify",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
payload: { code },
|
||||
});
|
||||
|
||||
const res = await testApp.app.inject({
|
||||
method: "GET",
|
||||
url: "/api/auth/session",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.user.totpEnabled).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("MFA login flow", () => {
|
||||
let totpUri: string;
|
||||
|
||||
@@ -307,3 +378,115 @@ describe("MFA login flow", () => {
|
||||
expect(body.expiresAt).toBeDefined();
|
||||
});
|
||||
});
|
||||
|
||||
async function setMfaPolicy(value: "optional" | "admins_only" | "required"): Promise<void> {
|
||||
await db
|
||||
.insert(schema.settings)
|
||||
.values({ key: "mfaPolicy", value })
|
||||
.onConflictDoUpdate({ target: schema.settings.key, set: { value } });
|
||||
}
|
||||
|
||||
// This is the actual fix for #515/#529: exercises the real /api/auth/login
|
||||
// route end to end, not a mocked response. Everything else in this repo that
|
||||
// covers this bug (the license gate on saving the setting, the frontend's
|
||||
// handling of a stubbed 403) would still pass if this exact backend branch
|
||||
// were reverted or its response code were renamed.
|
||||
describe("POST /api/auth/login with mfaPolicy enforcement", () => {
|
||||
afterEach(async () => {
|
||||
await setMfaPolicy("optional");
|
||||
await clearMfaState("admin");
|
||||
});
|
||||
|
||||
it("blocks an unenrolled admin with 403 MFA_ENROLLMENT_REQUIRED when policy is required", async () => {
|
||||
await setMfaPolicy("required");
|
||||
|
||||
const res = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/login",
|
||||
payload: { username: "admin", password: "Adminpass1" },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(403);
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.code).toBe("MFA_ENROLLMENT_REQUIRED");
|
||||
expect(body.token).toBeUndefined();
|
||||
});
|
||||
|
||||
it("blocks an unenrolled admin under an admins_only policy", async () => {
|
||||
await setMfaPolicy("admins_only");
|
||||
|
||||
const res = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/login",
|
||||
payload: { username: "admin", password: "Adminpass1" },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(403);
|
||||
expect(JSON.parse(res.body).code).toBe("MFA_ENROLLMENT_REQUIRED");
|
||||
});
|
||||
|
||||
it("does not block a non-admin user under an admins_only policy", async () => {
|
||||
// Ensure the shared test user exists while policy is still permissive.
|
||||
await loginAsUser(testApp.app);
|
||||
await setMfaPolicy("admins_only");
|
||||
|
||||
const res = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/login",
|
||||
payload: { username: "plainuser", password: "Userpass1" },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(JSON.parse(res.body).token).toBeDefined();
|
||||
});
|
||||
|
||||
it("does not block anyone when policy is optional", async () => {
|
||||
await setMfaPolicy("optional");
|
||||
|
||||
const res = await testApp.app.inject({
|
||||
method: "POST",
|
||||
url: "/api/auth/login",
|
||||
payload: { username: "admin", password: "Adminpass1" },
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
expect(JSON.parse(res.body).token).toBeDefined();
|
||||
});
|
||||
});
|
||||
|
||||
// The mirror image of tests/integration/platform/mfa-policy-license-gate.test.ts
|
||||
// (which proves an UNlicensed instance can't save this policy). This file is
|
||||
// already licensed (mockEnterpriseFeatures(["mfa"]) above), so it proves the
|
||||
// gate doesn't also accidentally block a legitimately licensed instance.
|
||||
describe("PUT /api/v1/settings mfaPolicy (licensed)", () => {
|
||||
afterEach(async () => {
|
||||
await setMfaPolicy("optional");
|
||||
});
|
||||
|
||||
it("allows saving required when mfa is licensed", async () => {
|
||||
const res = await testApp.app.inject({
|
||||
method: "PUT",
|
||||
url: "/api/v1/settings",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
payload: { mfaPolicy: "required" },
|
||||
});
|
||||
expect(res.statusCode).toBe(200);
|
||||
|
||||
const check = await testApp.app.inject({
|
||||
method: "GET",
|
||||
url: "/api/v1/settings/mfaPolicy",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
expect(JSON.parse(check.body).value).toBe("required");
|
||||
});
|
||||
|
||||
it("allows saving admins_only when mfa is licensed", async () => {
|
||||
const res = await testApp.app.inject({
|
||||
method: "PUT",
|
||||
url: "/api/v1/settings",
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
payload: { mfaPolicy: "admins_only" },
|
||||
});
|
||||
expect(res.statusCode).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user