fix(provenance): address #143 re-review — shared resolver + decoupled badge
Architecture & design: - Arch A: introduce resolveProvenance() as the single source of truth for deriving a write's actor/aiChatId from the SIGNED identity, and wire it into BOTH transport seams — the REST jwt.strategy and the collab authentication.extension. Previously the collab seam derived actor from the token claim alone and ignored user.isAgent, so a flagged service account's page-content edits over the websocket persisted as lastUpdatedSource='user', drifting from REST. The seams now share one resolver and can't diverge. - Arch B: drop AiAgentBadge's page-history coupling. The generic ui/ badge no longer imports historyAtoms; it exposes an onActivate callback fired after the deep-link, and the history row passes onActivate to close its own modal. Suggestions/warnings: - S1: soften the jwt.strategy provenance comment (applies to every REST write). - S2/suggestion-3: drop the redundant comment-list-item null-aiChatId test (covered by ai-agent-badge.test.tsx). - S3: de-duplicate jwt.strategy.spec test #3 (the no-claim→'user' half duplicated test #2); keep only the signed actor='agent' claim assertion. - W2: add keyboard-activation tests for the badge (Enter/Space, unrelated key). - W3: flip the design doc status to "реализовано (#143)". Tests: - new auth-provenance.decorator.spec.ts unit-tests resolveProvenance + agentSourceFields. - new collab-seam test: is_agent user with no claim → actor='agent' (Arch A regression guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -208,4 +208,19 @@ describe('AuthenticationExtension.onAuthenticate', () => {
|
||||
expect(ctx.actor).toBe('user');
|
||||
expect(ctx.aiChatId).toBeNull();
|
||||
});
|
||||
|
||||
it('is_agent user with NO claim → actor=agent (collab seam consults the signed identity)', async () => {
|
||||
// Arch A regression guard: a flagged service account editing page CONTENT
|
||||
// over the collab websocket carries a plain COLLAB token (no actor claim).
|
||||
// Before the shared resolveProvenance() wiring this seam derived actor from
|
||||
// the claim alone, so such edits persisted as lastUpdatedSource='user' —
|
||||
// drifting from the REST seam. The seam must now stamp 'agent' from the
|
||||
// is_agent flag, matching jwt.strategy.
|
||||
userRepo.findById.mockResolvedValue(buildUser({ isAgent: true }));
|
||||
const ctx = await ext.onAuthenticate(buildData() as any);
|
||||
|
||||
expect(ctx.actor).toBe('agent');
|
||||
// No internal ai_chats row for an MCP/service-account collab edit → null.
|
||||
expect(ctx.aiChatId).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -15,6 +15,7 @@ import { SpaceRole } from '../../common/helpers/types/permission';
|
||||
import { isUserDisabled } from '../../common/helpers';
|
||||
import { getPageId } from '../collaboration.util';
|
||||
import { JwtCollabPayload, JwtType } from '../../core/auth/dto/jwt-payload';
|
||||
import { resolveProvenance } from '../../common/decorators/auth-provenance.decorator';
|
||||
|
||||
@Injectable()
|
||||
export class AuthenticationExtension implements Extension {
|
||||
@@ -103,13 +104,17 @@ export class AuthenticationExtension implements Extension {
|
||||
|
||||
this.logger.debug(`Authenticated user ${user.id} on page ${pageId}`);
|
||||
|
||||
// Carry the signed agent-edit provenance claim into the hocuspocus
|
||||
// connection context (§6.6 / §15 C2). The human collab path omits these
|
||||
// claims, so it resolves to actor='user' / aiChatId=null.
|
||||
// Carry the agent-edit provenance into the hocuspocus connection context
|
||||
// (§6.6 / §15 C2), derived via the SAME resolver as the REST seam so the two
|
||||
// can't drift. An is_agent service account (e.g. the MCP bot) is attributed
|
||||
// 'agent' here too, so its page-content edits over collab persist as
|
||||
// lastUpdatedSource='agent' (#143 review Arch A) — not just its REST writes.
|
||||
// The human collab path carries no claim and is not flagged → actor='user'.
|
||||
const provenance = resolveProvenance(user, jwtPayload);
|
||||
return {
|
||||
user,
|
||||
actor: jwtPayload.actor ?? 'user',
|
||||
aiChatId: jwtPayload.aiChatId ?? null,
|
||||
actor: provenance.actor,
|
||||
aiChatId: provenance.aiChatId,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
import {
|
||||
resolveProvenance,
|
||||
agentSourceFields,
|
||||
} from './auth-provenance.decorator';
|
||||
|
||||
/**
|
||||
* Unit tests for the shared provenance helpers (#143 review, Arch A & follow-up
|
||||
* 5). resolveProvenance is the single source of truth wired into BOTH transport
|
||||
* seams (REST jwt.strategy + collab authentication.extension) — testing it here
|
||||
* pins the derivation matrix so the seams can't silently drift. agentSourceFields
|
||||
* is the one-place write-stamp idiom reused at every insert/update site.
|
||||
*/
|
||||
describe('resolveProvenance', () => {
|
||||
it("flags an is_agent user as 'agent' even with no claim (the closed collab gap)", () => {
|
||||
expect(resolveProvenance({ isAgent: true }, undefined)).toEqual({
|
||||
actor: 'agent',
|
||||
aiChatId: null,
|
||||
});
|
||||
});
|
||||
|
||||
it("an is_agent user keeps the claim's aiChatId when present", () => {
|
||||
expect(
|
||||
resolveProvenance({ isAgent: true }, { aiChatId: 'chat-1' }),
|
||||
).toEqual({ actor: 'agent', aiChatId: 'chat-1' });
|
||||
});
|
||||
|
||||
it("honors a signed actor='agent' claim on a non-agent user (internal AI-chat token)", () => {
|
||||
expect(
|
||||
resolveProvenance(
|
||||
{ isAgent: false },
|
||||
{ actor: 'agent', aiChatId: 'chat-2' },
|
||||
),
|
||||
).toEqual({ actor: 'agent', aiChatId: 'chat-2' });
|
||||
});
|
||||
|
||||
it("a plain user with no claim resolves to 'user' with null chat", () => {
|
||||
expect(resolveProvenance({ isAgent: false }, undefined)).toEqual({
|
||||
actor: 'user',
|
||||
aiChatId: null,
|
||||
});
|
||||
});
|
||||
|
||||
it('tolerates a null/undefined user (defaults to the claim, else user)', () => {
|
||||
expect(resolveProvenance(null, null)).toEqual({
|
||||
actor: 'user',
|
||||
aiChatId: null,
|
||||
});
|
||||
expect(resolveProvenance(undefined, { actor: 'agent' })).toEqual({
|
||||
actor: 'agent',
|
||||
aiChatId: null,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('agentSourceFields', () => {
|
||||
it('stamps the configured source + chat columns for an agent write', () => {
|
||||
expect(
|
||||
agentSourceFields(
|
||||
{ actor: 'agent', aiChatId: 'chat-1' },
|
||||
'createdSource',
|
||||
'aiChatId',
|
||||
),
|
||||
).toEqual({ createdSource: 'agent', aiChatId: 'chat-1' });
|
||||
});
|
||||
|
||||
it('uses the per-table column names passed in (page update variant)', () => {
|
||||
expect(
|
||||
agentSourceFields(
|
||||
{ actor: 'agent', aiChatId: null },
|
||||
'lastUpdatedSource',
|
||||
'lastUpdatedAiChatId',
|
||||
),
|
||||
).toEqual({ lastUpdatedSource: 'agent', lastUpdatedAiChatId: null });
|
||||
});
|
||||
|
||||
it('returns {} for a user write so the column keeps its default', () => {
|
||||
expect(
|
||||
agentSourceFields(
|
||||
{ actor: 'user', aiChatId: null },
|
||||
'createdSource',
|
||||
'aiChatId',
|
||||
),
|
||||
).toEqual({});
|
||||
});
|
||||
|
||||
it('returns {} when provenance is undefined', () => {
|
||||
expect(
|
||||
agentSourceFields(undefined, 'createdSource', 'aiChatId'),
|
||||
).toEqual({});
|
||||
});
|
||||
});
|
||||
@@ -13,6 +13,30 @@ export interface AuthProvenanceData {
|
||||
aiChatId: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Single source of truth for deriving a write's provenance from the SIGNED
|
||||
* server-side identity (#143 review, Arch A). Used by BOTH transport seams — the
|
||||
* REST access-token strategy and the collab websocket auth — so they can't drift:
|
||||
*
|
||||
* - A `user.isAgent` service account (e.g. the MCP bot) stamps 'agent' on every
|
||||
* write. It has no internal ai_chats row, so aiChatId comes from the claim
|
||||
* (usually null).
|
||||
* - Otherwise honor the actor claim minted into the internal AI agent's token
|
||||
* (actor='agent' + aiChatId); a normal user token carries no claim → 'user'.
|
||||
*
|
||||
* Provenance is NEVER read from a client body field, so a normal user cannot fake
|
||||
* an 'agent' marker.
|
||||
*/
|
||||
export function resolveProvenance(
|
||||
user: { isAgent?: boolean | null } | null | undefined,
|
||||
claim: { actor?: ProvenanceSource; aiChatId?: string | null } | null | undefined,
|
||||
): AuthProvenanceData {
|
||||
const actor: ProvenanceSource = user?.isAgent
|
||||
? 'agent'
|
||||
: (claim?.actor ?? 'user');
|
||||
return { actor, aiChatId: claim?.aiChatId ?? null };
|
||||
}
|
||||
|
||||
/**
|
||||
* Agent-edit write-stamp fields for a repository insert/update (#143 review).
|
||||
* Spread into the row being written: for an agent it stamps the `*Source`
|
||||
|
||||
@@ -77,31 +77,20 @@ describe('JwtStrategy — provenance derivation', () => {
|
||||
expect(req.raw.aiChatId).toBeNull();
|
||||
});
|
||||
|
||||
it("does NOT let an 'actor' claim escalate a non-agent user beyond the existing claim semantics", async () => {
|
||||
// A non-agent user. The only way the token carries actor='agent' is the
|
||||
// internal AI-chat's server-minted token (the claim cannot be set by a
|
||||
// client on a plain login). We assert the derivation falls back to the
|
||||
// claim ONLY when is_agent is false — i.e. an is_agent=false user is never
|
||||
// forced to 'agent' by anything other than that signed claim, and a plain
|
||||
// user (no claim) stays 'user'.
|
||||
it("honors a SIGNED actor='agent' claim on a non-agent user's token (the internal AI-chat path)", async () => {
|
||||
// A non-agent user (the plain no-claim → 'user' case is covered above). A
|
||||
// token that DOES carry actor='agent' resolves to 'agent' — BY DESIGN: that
|
||||
// claim can only exist on a SERVER-MINTED provenance token (the internal AI
|
||||
// chat), never on a plain login token, because the token is signed with the
|
||||
// app secret. The guarantee is that a client cannot FORGE this signed claim,
|
||||
// not that the strategy ignores it. (A plain user still cannot obtain
|
||||
// 'agent' — they have no way to get such a token.)
|
||||
const { strategy } = makeStrategy({
|
||||
id: 'user-1',
|
||||
isAgent: false,
|
||||
deactivatedAt: null,
|
||||
deletedAt: null,
|
||||
});
|
||||
const req = makeReq();
|
||||
|
||||
// No actor claim (the plain-user login case): stays 'user'.
|
||||
await strategy.validate(req, accessPayload() as any);
|
||||
expect(req.raw.actor).toBe('user');
|
||||
|
||||
// A token that DOES carry actor='agent' resolves to 'agent' — BY DESIGN:
|
||||
// that claim can only exist on a SERVER-MINTED provenance token (the internal
|
||||
// AI chat), never on a plain login token, because the token is signed with
|
||||
// the app secret. The security guarantee is that a client cannot forge this
|
||||
// signed claim, NOT that the strategy ignores it. (A plain user therefore
|
||||
// still cannot obtain 'agent' — they have no way to get such a token.)
|
||||
const req2 = makeReq();
|
||||
await strategy.validate(req2, accessPayload({ actor: 'agent', aiChatId: 'chat-1' }) as any);
|
||||
expect(req2.raw.actor).toBe('agent');
|
||||
|
||||
@@ -10,6 +10,7 @@ import { SessionActivityService } from '../../session/session-activity.service';
|
||||
import { FastifyRequest } from 'fastify';
|
||||
import { extractBearerTokenFromHeader, isUserDisabled } from '../../../common/helpers';
|
||||
import { ModuleRef } from '@nestjs/core';
|
||||
import { resolveProvenance } from '../../../common/decorators/auth-provenance.decorator';
|
||||
|
||||
@Injectable()
|
||||
export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
|
||||
@@ -72,18 +73,14 @@ export class JwtStrategy extends PassportStrategy(Strategy, 'jwt') {
|
||||
}
|
||||
|
||||
// Propagate the agent-edit provenance onto the request so REST
|
||||
// services/controllers can set the 'agent' marker off it. Provenance is
|
||||
// derived from the SIGNED server-side identity, never from a client body
|
||||
// field, so a normal user cannot fake an 'agent' badge:
|
||||
// - An account flagged is_agent (an MCP service account) stamps EVERY write
|
||||
// as 'agent'. It has no internal ai_chats row, so aiChatId stays null.
|
||||
// - Otherwise fall back to the actor claim minted into the internal AI
|
||||
// agent's token (actor='agent' + aiChatId); a normal user token carries
|
||||
// no claim and resolves to 'user' (unchanged behaviour).
|
||||
req.raw.actor = user.isAgent
|
||||
? 'agent'
|
||||
: ((payload as JwtPayload).actor ?? 'user');
|
||||
req.raw.aiChatId = (payload as JwtPayload).aiChatId ?? null;
|
||||
// services/controllers can set the 'agent' marker off it. Derived from the
|
||||
// SIGNED server-side identity via the shared resolver (also used by the
|
||||
// collab seam, so the two never drift), never from a client body field — so
|
||||
// an is_agent service account stamps every REST write made with an access
|
||||
// token, and a normal user cannot fake an 'agent' badge.
|
||||
const provenance = resolveProvenance(user, payload as JwtPayload);
|
||||
req.raw.actor = provenance.actor;
|
||||
req.raw.aiChatId = provenance.aiChatId;
|
||||
|
||||
return { user, workspace };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user