mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-13 14:32:03 +00:00
fix: close four small gaps around the managed-profile surface
- bound profile creation at ten an hour per account, the ceiling its guardian sibling has carried since it shipped - distinguish a failed roster read from a slow one, with the way back - cover the three caller states the roster read has to refuse: an unaccepted invitation, an expired grant, and MANAGE held elsewhere - cover what deletion does to a guardian standing inside the record
This commit is contained in:
1 parent
47c4560253
commit
8793bbf229
4 files changed
+213
-5
No files matched your search
@@ -64,15 +64,20 @@ export async function refreshGuardianStepUp(): Promise<void> {
|
||||
);
|
||||
// The guardian-invitation bucket, for the same reason `global-setup.ts`
|
||||
// clears the auth bucket between its own logins: the ceiling is ten
|
||||
// invitations an hour per caller, and this journey spends one every time it
|
||||
// invitations an hour per caller (and ten creations), and this journey spends
|
||||
// one of each every time it
|
||||
// runs. On CI that never matters; locally, the eleventh run inside an hour
|
||||
// is answered by the product's own 429 rather than by what the test is
|
||||
// about, and the failure reads as a broken invitation form. Only this one
|
||||
// fixture account's bucket is touched.
|
||||
await pool.query(
|
||||
`DELETE FROM rate_limits
|
||||
WHERE key = 'managed-profile:guardian-invite:' ||
|
||||
(SELECT id FROM users WHERE username = $1)`,
|
||||
WHERE key IN (
|
||||
'managed-profile:guardian-invite:' ||
|
||||
(SELECT id FROM users WHERE username = $1),
|
||||
'managed-profile:create:' ||
|
||||
(SELECT id FROM users WHERE username = $1)
|
||||
)`,
|
||||
[E2E_GUARDIAN.username],
|
||||
);
|
||||
} finally {
|
||||
|
||||
@@ -6,12 +6,30 @@ import {
|
||||
MFA_STEP_UP_MAX_AGE_SECONDS,
|
||||
requireFreshMfa,
|
||||
} from "@/lib/api-handler";
|
||||
import { apiSuccess, returnAllZodIssues, safeJson } from "@/lib/api-response";
|
||||
import {
|
||||
apiError,
|
||||
apiSuccess,
|
||||
returnAllZodIssues,
|
||||
safeJson,
|
||||
} from "@/lib/api-response";
|
||||
import { auditLog } from "@/lib/auth/audit";
|
||||
import { createManagedProfile } from "@/lib/managed-profiles/create";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { checkRateLimit } from "@/lib/rate-limit";
|
||||
import { isValidTimezone } from "@/lib/tz/format";
|
||||
|
||||
/**
|
||||
* The same ceiling its guardian sibling carries, and for the same reason.
|
||||
*
|
||||
* `POST /api/managed-profiles/{id}/guardians` has been rate-limited at ten an
|
||||
* hour since it shipped; this route creates a real account row on every call
|
||||
* and had none. A person cannot mistype their way into a hundred accounts, but
|
||||
* a script can, and the two endpoints of one family answering differently to
|
||||
* the same abuse is the kind of asymmetry nobody notices until it is used.
|
||||
*/
|
||||
const CREATE_LIMIT = 10;
|
||||
const CREATE_WINDOW_MS = 60 * 60 * 1000;
|
||||
|
||||
const managedProfileSchema = z
|
||||
.object({
|
||||
displayName: z.string().trim().min(1).max(80),
|
||||
@@ -32,6 +50,16 @@ const managedProfileSchema = z
|
||||
*/
|
||||
export const POST = apiHandler(async (request: NextRequest) => {
|
||||
const { user } = await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS);
|
||||
// Before the body is read, exactly as the guardian route does it: the cheap
|
||||
// refusal comes first so a flood costs a bucket lookup rather than a parse.
|
||||
const rateLimit = await checkRateLimit(
|
||||
`managed-profile:create:${user.id}`,
|
||||
CREATE_LIMIT,
|
||||
CREATE_WINDOW_MS,
|
||||
);
|
||||
if (!rateLimit.allowed) {
|
||||
return apiError("Too many profiles created, try again later", 429);
|
||||
}
|
||||
const { data: body, error: jsonError } = await safeJson(request, {
|
||||
maxBytes: 64 * 1024,
|
||||
});
|
||||
|
||||
@@ -11,6 +11,7 @@ import { ManagedProfileGuardians } from "@/components/settings/access/managed-pr
|
||||
import { SettingsCard } from "@/components/settings/settings-card";
|
||||
import { SettingsCardHeader } from "@/components/settings/_card-header";
|
||||
import { ConfirmButton } from "@/components/ui/confirm-button";
|
||||
import { QueryErrorCard } from "@/components/ui/query-error-card";
|
||||
import { useAuth } from "@/hooks/use-auth";
|
||||
import { useTranslations } from "@/lib/i18n/context";
|
||||
import {
|
||||
@@ -171,6 +172,20 @@ function ManagedProfileRow({
|
||||
onRetry={() => remove.mutate(profile.accountId)}
|
||||
/>
|
||||
)}
|
||||
{/* A failed roster read is not an absent roster.
|
||||
Without this branch a 500 renders exactly as a still-loading one —
|
||||
the row with no guardian panel and no delete control — and the person
|
||||
waits for something that is never coming. UI-STANDARDS §6: a read
|
||||
that failed says so and offers the way back. It is deliberately NOT
|
||||
the same as the 404 arm: a 404 means this account is no longer a
|
||||
Guardian of that profile, which is an answer, and the row correctly
|
||||
shows no destructive control for it. */}
|
||||
{guardians.isError && (
|
||||
<QueryErrorCard
|
||||
title={t("recordSharing.managed.rosterLoadError")}
|
||||
onRetry={() => void guardians.refetch()}
|
||||
/>
|
||||
)}
|
||||
{guardians.data && (
|
||||
<ManagedProfileGuardians
|
||||
profileId={profile.accountId}
|
||||
|
||||
@@ -19,7 +19,7 @@ import { NextRequest } from "next/server";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { cookieJar, headerJar, queuedSessionIds } from "./mock-next-headers";
|
||||
import { getPrismaClient, truncateAllTables } from "./setup";
|
||||
import { getPrismaClient, switchSessionTo, truncateAllTables } from "./setup";
|
||||
import { createManagedProfile } from "@/lib/managed-profiles/create";
|
||||
import { acceptGrant, inviteGrant } from "@/lib/sharing/grants";
|
||||
|
||||
@@ -725,3 +725,163 @@ describe("every call the managed-profile card can make", () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("the three caller states the roster read has to refuse", () => {
|
||||
it("refuses a Guardian whose invitation has not been accepted", async () => {
|
||||
// PENDING confers nothing. `activeGuardianWhere` requires `acceptedAt`, so
|
||||
// an invitee who can see the invitation on their own panel still cannot
|
||||
// read the roster of the record it is for — and must not be able to, or
|
||||
// an invitation would disclose the household before it was accepted.
|
||||
const { invitee, profile } = await profileWithPendingSecondGuardian();
|
||||
signIn(invitee);
|
||||
|
||||
const response = await listGuardians(profile.id);
|
||||
expect(response.status).toBe(404);
|
||||
expect(await errorCodeOf(response)).toBe("managed_profile.not_found");
|
||||
});
|
||||
|
||||
it("refuses a Guardian whose grant has expired", async () => {
|
||||
// The other half of the same predicate, and the one a `revokedAt IS NULL`
|
||||
// check alone would miss: nobody ended this grant, the clock did.
|
||||
const { invitee, profile, invitation } =
|
||||
await profileWithPendingSecondGuardian();
|
||||
await acceptGrant({ grantId: invitation.id, granteeId: invitee.id });
|
||||
await getPrismaClient().accountGrant.update({
|
||||
where: { id: invitation.id },
|
||||
data: { expiresAt: new Date(Date.now() - 60_000) },
|
||||
});
|
||||
signIn(invitee);
|
||||
|
||||
const response = await listGuardians(profile.id);
|
||||
expect(response.status).toBe(404);
|
||||
expect(await errorCodeOf(response)).toBe("managed_profile.not_found");
|
||||
});
|
||||
|
||||
it("refuses a holder of MANAGE on an ordinary record, against a real managed profile", async () => {
|
||||
// Distinct from the leg above that points a delegate at THEIR OWN grantor:
|
||||
// this caller holds a live MANAGE grant somewhere, and aims it at somebody
|
||||
// else's managed profile. Holding the level is not holding this record.
|
||||
const { profile } = await profileWithPendingSecondGuardian();
|
||||
const owner = await person("owner");
|
||||
const manager = await person("manager");
|
||||
const invitation = await inviteGrant({
|
||||
grantorId: owner.id,
|
||||
granteeId: manager.id,
|
||||
access: "MANAGE",
|
||||
scope: null,
|
||||
});
|
||||
await acceptGrant({ grantId: invitation.id, granteeId: manager.id });
|
||||
signIn(manager);
|
||||
|
||||
const response = await listGuardians(profile.id);
|
||||
expect(response.status).toBe(404);
|
||||
expect(await errorCodeOf(response)).toBe("managed_profile.not_found");
|
||||
});
|
||||
});
|
||||
|
||||
describe("what deletion does to a Guardian who is standing inside the record", () => {
|
||||
it("puts their session back in their own account rather than nowhere", async () => {
|
||||
// The composition worth an integration leg: `Session.actingAsUserId` is a
|
||||
// foreign key with ON DELETE SET NULL, and the epoch is bumped by a
|
||||
// database trigger on that column. Deleting the profile therefore has to
|
||||
// leave the other Guardian's browser in a state it can prove — back in
|
||||
// their own record, on a NEW epoch — rather than pointed at a row that is
|
||||
// gone. A session left holding a dangling selector is the shape that
|
||||
// strands somebody behind the fence with no way out.
|
||||
const { creator, invitee, profile, invitation } =
|
||||
await profileWithPendingSecondGuardian();
|
||||
await acceptGrant({ grantId: invitation.id, granteeId: invitee.id });
|
||||
await switchSessionTo(invitee.sessionId, profile.id);
|
||||
|
||||
const before = await getPrismaClient().session.findUniqueOrThrow({
|
||||
where: { id: invitee.sessionId },
|
||||
});
|
||||
// Non-zero proof: they really were inside the record before the delete.
|
||||
expect(before.actingAsUserId).toBe(profile.id);
|
||||
|
||||
signIn(creator);
|
||||
expect((await deleteProfile(profile.id)).status).toBe(200);
|
||||
|
||||
const after = await getPrismaClient().session.findUniqueOrThrow({
|
||||
where: { id: invitee.sessionId },
|
||||
});
|
||||
// The session survives — deleting a record must not sign anybody out —
|
||||
// and it is back in its own account.
|
||||
expect(after.actingAsUserId).toBeNull();
|
||||
// And the context moved, so a request formed inside the deleted record is
|
||||
// refused as stale rather than served against nothing.
|
||||
expect(after.recordEpoch).toBeGreaterThan(before.recordEpoch);
|
||||
});
|
||||
});
|
||||
|
||||
describe("how many profiles one account may create in an hour", () => {
|
||||
it("refuses the eleventh, with the same ceiling the guardian route carries", async () => {
|
||||
// Ten an hour. The guardian invitation has been bounded since it shipped;
|
||||
// this route creates a real account row per call and was not, which is the
|
||||
// kind of asymmetry between two endpoints of one family that nobody
|
||||
// notices until it is used.
|
||||
const creator = await person("creator");
|
||||
signIn(creator);
|
||||
|
||||
for (let i = 0; i < 10; i += 1) {
|
||||
const allowed = await createProfile({
|
||||
displayName: `Managed record ${i}`,
|
||||
locale: "en",
|
||||
timezone: "UTC",
|
||||
});
|
||||
expect(allowed.status, `creation ${i}`).toBe(201);
|
||||
}
|
||||
|
||||
const refused = await createProfile({
|
||||
displayName: "One too many",
|
||||
locale: "en",
|
||||
timezone: "UTC",
|
||||
});
|
||||
expect(refused.status).toBe(429);
|
||||
// No error code, like every other rate-limit refusal in this family — the
|
||||
// client branches on status, which is recorded in the findings.
|
||||
expect(await errorCodeOf(refused)).toBeUndefined();
|
||||
// And nothing was created by the refused call.
|
||||
expect(
|
||||
await getPrismaClient().user.count({
|
||||
where: { managedProfileAt: { not: null } },
|
||||
}),
|
||||
).toBe(10);
|
||||
});
|
||||
|
||||
it("bounds the caller and not the instance", async () => {
|
||||
// A per-user bucket, so one busy account cannot stop another from creating
|
||||
// a record. `checkRateLimit` is keyed by caller id; this proves the key
|
||||
// really carries it rather than being a single global bucket.
|
||||
const first = await person("creator");
|
||||
signIn(first);
|
||||
for (let i = 0; i < 10; i += 1) {
|
||||
await createProfile({
|
||||
displayName: `Managed record ${i}`,
|
||||
locale: "en",
|
||||
timezone: "UTC",
|
||||
});
|
||||
}
|
||||
expect(
|
||||
(
|
||||
await createProfile({
|
||||
displayName: "One too many",
|
||||
locale: "en",
|
||||
timezone: "UTC",
|
||||
})
|
||||
).status,
|
||||
).toBe(429);
|
||||
|
||||
const second = await person("creator");
|
||||
signIn(second);
|
||||
expect(
|
||||
(
|
||||
await createProfile({
|
||||
displayName: "Somebody else's first",
|
||||
locale: "en",
|
||||
timezone: "UTC",
|
||||
})
|
||||
).status,
|
||||
).toBe(201);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user