api keys: revoked ones stop filling the screen, and can be forgotten
Five revoked keys, no live ones, and no way to remove any of them — which is what the screen looks like after a few rounds of testing. `listApiKeys` returns every row and nothing filtered them, so a list whose job is "which of my keys still work" answered with a wall of dead ones. Nobody decided that. Revoke was implemented and the list was never taught the difference. Dead keys — revoked or past expiry — are now hidden behind a "Show 5 revoked or expired keys" toggle rather than merely dimmed, because on any account in use for a while they outnumber the live ones. The empty state distinguishes "No keys yet" from "No keys in use": the first is false on an account whose keys are all revoked, and it is exactly the sentence that sends somebody hunting for a bug in the list. DELETE /:id/record removes the row for good, and only after it is revoked. The `revokedAt is not null` clause is in the statement rather than checked before it, so there is no ordering in which a live key is deleted by a request that raced a revoke. Nothing is weakened by allowing it: the key cannot be honoured again either way, since authentication matches a hash and a deleted row has none. What goes is the history — `lastUsedAt`, the only evidence of whether a leaked key was used before it was killed — which is why it stays a second, deliberate step rather than what revoke does. Also corrected the copy, which claimed keys are per-app. They are not scoped to anything: one key works everywhere, and per-device is a revocation convenience, not a requirement. The owner had to ask. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -2,7 +2,7 @@ import type { FormEvent } from 'react';
|
||||
import { useState } from 'react';
|
||||
import { useQuery, useQueryClient } from '@tanstack/react-query';
|
||||
import { toast } from 'sonner';
|
||||
import { Copy, Ban, Plus, KeyRound, Loader2 } from 'lucide-react';
|
||||
import { Copy, Ban, Plus, KeyRound, Loader2, Trash2 } from 'lucide-react';
|
||||
import { useClient } from 'hooks/useClient';
|
||||
import { Input } from '@/components/ui/input';
|
||||
import { Label } from '@/components/ui/label';
|
||||
@@ -53,6 +53,7 @@ export const ApiKeys = () => {
|
||||
const queryClient = useQueryClient();
|
||||
const [name, setName] = useState('');
|
||||
const [isCreating, setIsCreating] = useState(false);
|
||||
const [showRevoked, setShowRevoked] = useState(false);
|
||||
const [minted, setMinted] = useState<Minted | null>(null);
|
||||
|
||||
const { data, isLoading } = useQuery<{ keys: ApiKey[] }>({
|
||||
@@ -79,6 +80,15 @@ export const ApiKeys = () => {
|
||||
}
|
||||
};
|
||||
|
||||
const forget = async (id: number) => {
|
||||
try {
|
||||
await client.delete(`/api-keys/${id}/record`);
|
||||
await refresh();
|
||||
} catch {
|
||||
toast.error('Could not remove it');
|
||||
}
|
||||
};
|
||||
|
||||
const revoke = async (id: number) => {
|
||||
try {
|
||||
await client.delete(`/api-keys/${id}`);
|
||||
@@ -89,7 +99,19 @@ export const ApiKeys = () => {
|
||||
}
|
||||
};
|
||||
|
||||
const keys = data?.keys ?? [];
|
||||
const allKeys = data?.keys ?? [];
|
||||
|
||||
// Dead means revoked OR past its expiry — the same test the row rendering uses, lifted here so the
|
||||
// count in the toggle and the rows it reveals can never disagree.
|
||||
const isDead = (entry: ApiKey) =>
|
||||
!!entry.revokedAt || (!!entry.expiresAt && new Date(entry.expiresAt).getTime() <= Date.now());
|
||||
|
||||
// Dead keys are HIDDEN by default rather than merely dimmed. Revoking is the normal end of a key's
|
||||
// life, so on any account that has been used for a while the dead ones outnumber the live ones and the
|
||||
// screen stops answering the question it exists for — which of my keys still work. The rows are kept
|
||||
// and reachable; they are just not the first thing shown.
|
||||
const deadKeys = allKeys.filter(isDead);
|
||||
const keys = showRevoked ? allKeys : allKeys.filter((entry) => !isDead(entry));
|
||||
|
||||
if (isLoading) {
|
||||
return (
|
||||
@@ -102,8 +124,8 @@ export const ApiKeys = () => {
|
||||
return (
|
||||
<div className="grid gap-5">
|
||||
<p className="text-sm text-duck-dark/70 dark:text-foreground/70">
|
||||
An API key signs in as you, without a password and without expiring. Give each app or device its own, so you can
|
||||
cut one off without touching the rest. Send it as{' '}
|
||||
An API key signs in as you, without a password and without expiring. One key works everywhere — a key is not
|
||||
tied to an app. Giving each device its own is only so you can cut one off without touching the rest. Send it as{' '}
|
||||
<code className="font-mono text-xs">Authorization: Bearer …</code>.
|
||||
</p>
|
||||
|
||||
@@ -150,7 +172,9 @@ export const ApiKeys = () => {
|
||||
|
||||
{keys.length === 0 ? (
|
||||
<div className="rounded-lg border border-dashed border-duck-dark/15 dark:border-foreground/15 p-6 text-center text-sm text-duck-dark/40 dark:text-foreground/40">
|
||||
No keys yet.
|
||||
{/* "No keys yet" would be false on an account whose keys are all revoked, and it is the sentence
|
||||
that would send somebody looking for a bug in the list. */}
|
||||
{allKeys.length === 0 ? 'No keys yet.' : 'No keys in use.'}
|
||||
</div>
|
||||
) : (
|
||||
<div className="grid gap-2">
|
||||
@@ -181,11 +205,23 @@ export const ApiKeys = () => {
|
||||
{lastUsed ? `last used ${lastUsed}` : 'never used'}
|
||||
</div>
|
||||
</div>
|
||||
{!dead && (
|
||||
{dead ? (
|
||||
// Only offered once the key is already dead — the server refuses it otherwise, and the
|
||||
// button matching that rule is what keeps the two from drifting.
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="sm"
|
||||
onClick={() => revoke(entry.id)}
|
||||
onClick={() => void forget(entry.id)}
|
||||
title="Remove this record"
|
||||
className="hover:text-red-500"
|
||||
>
|
||||
<Trash2 className="h-3.5 w-3.5" />
|
||||
</Button>
|
||||
) : (
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="sm"
|
||||
onClick={() => void revoke(entry.id)}
|
||||
title="Revoke"
|
||||
className="hover:text-red-500"
|
||||
>
|
||||
@@ -197,6 +233,18 @@ export const ApiKeys = () => {
|
||||
})}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{deadKeys.length > 0 && (
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => setShowRevoked((v) => !v)}
|
||||
className="mt-3 cursor-pointer text-xs text-duck-dark/40 underline underline-offset-2 transition-colors hover:text-duck-dark/70 dark:text-foreground/40 dark:hover:text-foreground/70"
|
||||
>
|
||||
{showRevoked
|
||||
? 'Hide revoked and expired'
|
||||
: `Show ${deadKeys.length} revoked or expired key${deadKeys.length === 1 ? '' : 's'}`}
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
@@ -30,7 +30,7 @@ const sections: SettingsSection[] = [
|
||||
key: 'api-keys',
|
||||
icon: KeyRound,
|
||||
title: 'API keys',
|
||||
description: 'Per-app keys that sign in as you, revocable one at a time',
|
||||
description: 'Keys that sign in as you, revocable one at a time',
|
||||
content: <ApiKeys />,
|
||||
},
|
||||
{
|
||||
|
||||
@@ -3,6 +3,7 @@ export {
|
||||
createApiKey,
|
||||
listApiKeys,
|
||||
revokeApiKey,
|
||||
deleteApiKey,
|
||||
touchApiKey,
|
||||
type ApiKeyIdentity,
|
||||
} from './queries';
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { eq, and, isNull, sql } from 'drizzle-orm';
|
||||
import { eq, and, isNull, isNotNull, sql } from 'drizzle-orm';
|
||||
import { db } from '../db';
|
||||
import { apiKeys } from './schema';
|
||||
import { users } from '../auth/schema';
|
||||
@@ -99,6 +99,32 @@ export async function revokeApiKey(id: number, userId: number): Promise<boolean>
|
||||
return rows.length > 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete a revoked key's row for good.
|
||||
*
|
||||
* Separate from revoking, and only reachable AFTER it — the `revokedAt is not null` clause is in the
|
||||
* statement rather than checked before it, so there is no window where a live key is deleted by a request
|
||||
* that raced a revoke.
|
||||
*
|
||||
* ── Why this is a second step and not what revoke does ──
|
||||
*
|
||||
* The row IS the record that the key existed. `lastUsedAt` is the only evidence of whether a leaked key
|
||||
* was used before it was killed, and on what date; deleting on revoke would throw that away at exactly
|
||||
* the moment somebody most wants it. So revoke keeps everything and this exists for afterwards, when the
|
||||
* question has been answered and the row is only clutter.
|
||||
*
|
||||
* Deleting is safe in a way it would not be for a password: the key can never be honoured again either
|
||||
* way, because `authenticate` matches on a hash that no longer has a row. Losing the row loses the
|
||||
* history, not the protection.
|
||||
*/
|
||||
export async function deleteApiKey(id: number, userId: number): Promise<boolean> {
|
||||
const rows = await db
|
||||
.delete(apiKeys)
|
||||
.where(and(eq(apiKeys.id, id), eq(apiKeys.userId, userId), isNotNull(apiKeys.revokedAt)))
|
||||
.returning({ id: apiKeys.id });
|
||||
return rows.length > 0;
|
||||
}
|
||||
|
||||
/** How stale last_used_at is allowed to get. A write per request is not worth this column's value. */
|
||||
const TOUCH_DEBOUNCE_MS = 60_000;
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { createRouter } from '../../create-router';
|
||||
import { createApiKey, listApiKeys, revokeApiKey } from 'officerdb';
|
||||
import { createApiKey, listApiKeys, revokeApiKey, deleteApiKey } from 'officerdb';
|
||||
import { mintApiKey } from '../../auth-token';
|
||||
import * as errors from '../../custom-errors';
|
||||
|
||||
@@ -60,3 +60,26 @@ apiKeysRouter.delete('/:id', async (ctx) => {
|
||||
if (!(await revokeApiKey(id, user.id))) throw errors.NOT_FOUND('no such api key');
|
||||
return ctx.json({ ok: true });
|
||||
});
|
||||
|
||||
// ── Forgetting a key, which is not the same as revoking it ──
|
||||
//
|
||||
// DELETE /:id revokes and keeps the row, because the row is the record that the key existed and
|
||||
// `lastUsedAt` is the only evidence of whether a leaked one was used before it was killed. This removes
|
||||
// that record, and it is deliberately a SECOND request after the first: `deleteApiKey` refuses a key that
|
||||
// is not already revoked, in the statement rather than before it, so there is no ordering in which a live
|
||||
// key is deleted by something that raced a revoke.
|
||||
//
|
||||
// Nothing is weakened by allowing it. The key cannot be honoured again either way — authentication
|
||||
// matches a hash, and a deleted row has none. What goes is the history, not the protection.
|
||||
apiKeysRouter.delete('/:id/record', async (ctx) => {
|
||||
const user = ctx.get('user');
|
||||
const id = Number(ctx.req.param('id'));
|
||||
if (!Number.isInteger(id)) throw errors.BAD_REQUEST('invalid id');
|
||||
|
||||
if (!(await deleteApiKey(id, user.id))) {
|
||||
// One message for both "not yours / never existed" and "still live", on purpose: the first must not
|
||||
// be distinguishable from the second by someone probing ids that are not theirs.
|
||||
throw errors.NOT_FOUND('no such revoked api key');
|
||||
}
|
||||
return ctx.json({ ok: true });
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user