fix(auth): revoke a user's OIDC/BFF sessions when their account is deleted - #6324
Draft
hero101 wants to merge 5 commits into
Draft
fix(auth): revoke a user's OIDC/BFF sessions when their account is deleted#6324hero101 wants to merge 5 commits into
hero101 wants to merge 5 commits into
Conversation
added 3 commits
July 31, 2026 16:14
Full SpecKit flow for #6315 — spec, clarify (3 passes), plan, tasks, analyze (3 passes to zero findings). Key decisions recorded: - markTerminated, never destroy: the tombstone is what produces the 401 that flips the client to signed-out, and it is what discards the cached PII from the session payload. destroy reproduces the reported bug. - Audit trail rides the existing OIDC audit stream, not platform_audit_entry. Checked for an enum collision with 027-platform-role-redesign: none, but 027 already owns the DB-side audit row for user deletion. Net effect is zero DDL and zero migrations. - New OidcCoreModule breaks the UserModule -> OidcModule cycle without a forwardRef, and collapses two Redis clients into one. - Self-healing index write so sessions alive at deploy time are revocable. Refs #6315
Fixes the anchor defect in #6315: deleting a user left their BFF/OIDC session untouched in Redis, so the browser kept authenticating against an account that no longer existed for up to the 30-day absolute ceiling. That is an access-control failure (ISO 27001 A.5.18, SOC 2 CC6.2), and an incomplete erasure too, since the session payload caches display name and email. The root cause was a missing primitive rather than a missing call: sessions live at alkemio:sid:<sid> with no way to get from a subject to that subject's sessions, so they could not be enumerated, so they could not be revoked. WS1 — the primitive - alkemio:sub:<sub> Redis set; TTL rolled FORWARD to the latest member's absolute ceiling and never shortened (a SADD without an EXPIRE leaks the key forever; a shortened TTL would evict a live member and make revocation silently miss a session) - OidcSessionRevocationService.revokeAllForSub(sub, reason, opts?) tombstones every session with markTerminated, NEVER destroy. This is the whole fix: a tombstone yields 401 and flips the client to signed-out, while destroy yields a silent anonymous fall-through with HTTP 200 — the reported bug in a new costume. The tombstone also nulls request_context_cache, so it closes the GDPR Art. 17 half of the defect in the same move. - clears the refresh lock, prunes the index, then revokes the refresh grant at Hydra (RFC 7009, 3s timeout, no retry, no circuit breaker — rationale inline per constitution principle 8). Local teardown completes BEFORE the remote call, so a Hydra outage cannot leave a session alive. - exceptSid, per-session outcomes and partial-failure tolerance so client-web#10070 and server#6073 consume it unchanged - self-healing index write from CookieSessionStrategy, fire-and-forget, so sessions minted before this ships become revocable on their next request instead of never WS2 — the cascade - deleteUser now calls revokeAllForSub AND the pre-existing-but-never-called kratosService.invalidateAllIdentitySessions, both unconditionally (NOT gated on deleteData.deleteIdentity — a surviving identity with live sessions is exactly the orphan state), after commit, each individually trapped so neither can fail the delete mutation WS3 — graceful degradation - the seven me sub-resolvers return 0/[]/empty-page/{} on an empty actorID instead of throwing. Two of the seven are not in the original triage list: notifications throws ForbiddenException rather than ValidationException, and the real conversations thrower is in me.conversations.resolver.fields.ts — fixing only the container would have left the actual client query erroring. New OidcCoreModule breaks the UserModule -> OidcModule cycle without a forwardRef and collapses two Redis clients into one. No DDL, no migration, no GraphQL schema delta. The revocation audit trail rides the existing OIDC audit stream rather than platform_audit_entry, deliberately: 027-platform-role-redesign already owns the DB-side audit row for user deletion. Tests are the audit evidence, not decoration. revocation-ends-access.spec.ts wires the real service to the real strategy and asserts the next request is REJECTED — proof that access ended, not that a method was called. The leak-proof test caught two real defects in the first draft (an upstream error message embedding the token, and error.stack logged raw); both fixed by scrubbing known secret values rather than pattern-guessing. Refs #6315
|
Svetoslav Petkov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📊 PR Metrics SummaryTitle: fix(auth): revoke a user's OIDC/BFF sessions when their account is deleted
Flags
Thresholds{
"critical_loc": 200,
"simple_loc": 100,
"file_count": 10
} |
|
Schema Diff Summary: No blocking changes
Baseline branch: develop |
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds subject-scoped Redis session indexing, OIDC session revocation with tombstones and audit events, post-transaction user-deletion integration, and empty-value handling for orphaned ChangesOIDC session revocation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an auth lifecycle gap where deleting a user left their OIDC/BFF Redis-backed sessions valid, enabling continued authentication (and leaving cached PII in session payloads) until absolute TTL expiry. It introduces a per-subject session index + revocation primitive, wires it into deleteUser, and hardens GraphQL me sub-resolvers to degrade cleanly when no actor can be resolved.
Changes:
- Add a Redis
alkemio:sub:<sub>index and anOidcSessionRevocationServicethat tombstones (not destroys) all sessions for a subject and attempts RFC 7009 refresh-token revocation. - Wire revocation into the post-commit user-deletion cascade (plus calling the previously-unused Kratos session invalidation).
- Make
mesub-resolvers return empty values (and warn) whenactorIDis empty; add tests for the new behavior.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/api/me/me.resolver.fields.ts | Degrade multiple me.* sub-resolvers to empty values when no actorID. |
| src/services/api/me/me.resolver.fields.spec.ts | Adds tests for degradation behavior (note: hook cleanup issue raised in comments). |
| src/services/api/me/me.conversations.resolver.fields.ts | Degrade me.conversations.conversations to [] when no actorID. |
| src/services/api/me/me.conversations.resolver.fields.spec.ts | Adds tests for conversations degradation (note: spy restore issue raised in comments). |
| src/domain/community/user/user.service.ts | Calls OIDC session revocation + Kratos session invalidation post-commit during deleteUser. |
| src/domain/community/user/user.service.delete.spec.ts | New unit coverage for the deletion cascade ordering, unconditional behavior, and failure tolerance. |
| src/domain/community/user/user.module.ts | Imports OidcCoreModule to access revocation service without creating a module cycle. |
| src/core/auth/oidc/strategies/cookie-session.strategy.ts | Adds optional Redis client injection and a fire-and-forget self-healing index write. |
| src/core/auth/oidc/strategies/cookie-session.strategy.index.spec.ts | Tests self-healing indexing behavior and non-awaiting semantics. |
| src/core/auth/oidc/session-index.redis.ts | Implements alkemio:sub:<sub> set primitives with TTL roll-forward rules. |
| src/core/auth/oidc/session-index.redis.spec.ts | Tests index TTL invariants, keyspace safety, and edge cases. |
| src/core/auth/oidc/revocation/subject-contract.spec.ts | Pins cross-service invariant session.sub === user.authenticationID. |
| src/core/auth/oidc/revocation/session-revocation.types.ts | Adds reason/outcome/report types for revocation. |
| src/core/auth/oidc/revocation/revocation-ends-access.spec.ts | Integration-style test proving revocation causes the next request to be rejected. |
| src/core/auth/oidc/revocation/oidc-session-revocation.service.ts | New revocation service: tombstone local sessions, prune index, clear refresh lock, RFC 7009 revoke. |
| src/core/auth/oidc/oidc.tokens.ts | Adds OIDC_REDIS_CLIENT DI token for a shared Redis client. |
| src/core/auth/oidc/oidc.module.ts | Hoists foundational providers into OidcCoreModule and re-exports it. |
| src/core/auth/oidc/oidc.controller.ts | Maintains the per-subject index on callback/logout/teardown paths. |
| src/core/auth/oidc/oidc.controller.spec.ts | Tests controller-driven index write/prune behavior and best-effort failure semantics. |
| src/core/auth/oidc/oidc-core.module.ts | New dependency-light module providing OIDC Redis client, session store handle, and revocation service. |
| src/core/auth/oidc/audit.ts | Extends audit vocabulary with session revocation event types and reason field. |
| specs/107-oidc-session-revocation/tasks.md | Adds SDD tasks artifact for the feature. |
| specs/107-oidc-session-revocation/quickstart.md | Adds quickstart/manual verification guide. |
| specs/107-oidc-session-revocation/plan.md | Adds implementation plan artifact. |
| specs/107-oidc-session-revocation/data-model.md | Documents Redis keyspace + type contracts (no DDL/migrations). |
| specs/107-oidc-session-revocation/contracts/session-revocation-service.md | Documents internal revocation service contract and consumer matrix. |
| specs/107-oidc-session-revocation/contracts/redis-keyspace.md | Documents Redis key families and invariants. |
| specs/107-oidc-session-revocation/contracts/graphql-me-degradation.md | Documents me degradation contract. |
| specs/107-oidc-session-revocation/contracts/audit-events.md | Documents the audit event semantics for session revocation. |
| specs/107-oidc-session-revocation/checklists/requirements.md | Adds spec quality checklist artifact. |
| CLAUDE.md | Updates repo guidance with the new feature’s summary + plan pointer. |
| .specify/feature.json | Points Spec Kit feature directory to specs/107-oidc-session-revocation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/core/auth/oidc/oidc.controller.ts (2)
556-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the destroy-then-deindex block into one private helper.
The same seven-line promise wrapper around
req.session.destroynow appears three times, each followed bydeindexSession. A single helper keeps the "capturesubbefore destroy" invariant in one place, so a future edit cannot restore the ordering bug in only two of the three call sites.♻️ Proposed helper
+ /** + * server#6315 / FR-003 — destroy the session, then prune the index with the + * `sub` captured beforehand. Reading `sub` after destroy would prune with + * `undefined` and silently do nothing. + */ + private async destroyAndDeindex( + req: Request, + sub: string | undefined | null + ): Promise<void> { + const sid = req.sessionID; + await new Promise<void>(resolve => { + try { + req.session.destroy(() => resolve()); + } catch { + resolve(); + } + }); + await this.deindexSession(sub, sid); + }Also applies to: 631-638, 690-697
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/oidc.controller.ts` around lines 556 - 563, Extract the repeated session-destruction and deindexing sequence into one private helper, preserving the invariant that the subject is captured before req.session.destroy runs. Update all three call sites, including the blocks around the referenced flows, to invoke this helper and remove their duplicated Promise wrapper and direct deindexSession calls.
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth new index-maintenance loggers use the NestJS
Loggerinstead of Winston. The coding guidelines require Winston logging, andOidcSessionRevocationServicein this same PR injectsWINSTON_MODULE_NEST_PROVIDER. The shared root cause is that the two index-maintenance classes instantiate@nestjs/commonLoggerdirectly, so this feature writes its warnings to two different logging backends.
src/core/auth/oidc/oidc.controller.ts#L100-L101: replace thenew Logger(...)field with an injectedLoggerServicefromWINSTON_MODULE_NEST_PROVIDER, and pass aLogContextvalue as the context argument inindexSessionanddeindexSession.src/core/auth/oidc/strategies/cookie-session.strategy.ts#L45-L45: replace thenew Logger(...)field with the same injected WinstonLoggerService, and update thereindexSessionwarn call to the(message, context)signature.Both spec files assert on
Logger.prototype.warn, so update those assertions to the injected logger mock.As per coding guidelines: "Use the Winston logger with signatures
verbose/warning(message, context)anderror(message, stacktrace, context)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/oidc.controller.ts` around lines 100 - 101, Replace the direct NestJS Logger instances in src/core/auth/oidc/oidc.controller.ts lines 100-101 and src/core/auth/oidc/strategies/cookie-session.strategy.ts line 45 with injected LoggerService instances from WINSTON_MODULE_NEST_PROVIDER; pass a LogContext value to the indexSession, deindexSession, and reindexSession warning calls using the (message, context) signature. Update both spec files to assert against the injected logger mock instead of Logger.prototype.warn.Source: Coding guidelines
src/core/auth/oidc/revocation/subject-contract.spec.ts (1)
110-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test asserts only the assignment it just made.
user.authenticationID = nullfollowed byexpect(user.authenticationID).toBeNull()passes for any entity shape, including one where the column later becomes non-nullable at the database level. It does not pin the stated intent.A type-level assertion pins the nullability the comment describes:
♻️ Proposed type-level pin
it('user.authenticationID is nullable, and that is a legitimate state', () => { - const user = new User(); - user.authenticationID = null; - - expect(user.authenticationID).toBeNull(); + // Fails to compile if the property stops accepting null. + const authenticationID: User['authenticationID'] = null; + + expect(authenticationID).toBeNull(); });As per path instructions: "Unit tests use the
*.spec.tssuffix and should be added when they provide meaningful signal; trivial pass-through coverage is not required."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/revocation/subject-contract.spec.ts` around lines 110 - 120, Replace the assignment-and-read assertion in the authenticationID test with a type-level assertion that verifies User.authenticationID accepts string or null, preserving the intended nullable contract without relying on runtime assignment. Keep the test focused on the User entity’s declared property type and retain the existing spec naming.Source: Coding guidelines
src/domain/community/user/user.service.ts (1)
589-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated error-logging blocks.
The two
catchblocks at lines 596-606 and 615-626 log the same shape (userID,authenticationID,error.message,error.stack,LogContext.AUTH) and differ only in the message text. Extract a small helper to remove the duplication.♻️ Proposed refactor to remove duplication
+ private logRevocationFailure( + message: string, + userID: string, + authenticationID: string, + error: any + ): void { + this.logger.error?.( + { message, userID, authenticationID, error: error?.message }, + error?.stack, + LogContext.AUTH + ); + } + if (user.authenticationID) { try { await this.oidcSessionRevocationService.revokeAllForSub( user.authenticationID, 'account_deleted' ); } catch (error: any) { - this.logger.error?.( - { - message: - 'Failed to revoke OIDC sessions during user deletion; the deletion still stands', - userID: id, - authenticationID: user.authenticationID, - error: error?.message, - }, - error?.stack, - LogContext.AUTH - ); + this.logRevocationFailure( + 'Failed to revoke OIDC sessions during user deletion; the deletion still stands', + id, + user.authenticationID, + error + ); } try { await this.kratosService.invalidateAllIdentitySessions( user.authenticationID ); } catch (error: any) { - this.logger.error?.( - { - message: - 'Failed to invalidate Kratos identity sessions during user deletion; the deletion still stands', - userID: id, - authenticationID: user.authenticationID, - error: error?.message, - }, - error?.stack, - LogContext.AUTH - ); + this.logRevocationFailure( + 'Failed to invalidate Kratos identity sessions during user deletion; the deletion still stands', + id, + user.authenticationID, + error + ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/community/user/user.service.ts` around lines 589 - 628, Extract the duplicated catch-block logging in the user deletion flow into a small helper method, preserving the existing logger payload fields, stack argument, and LogContext.AUTH value. Have the helper accept the operation-specific message, then call it from both revokeAllForSub and invalidateAllIdentitySessions error handlers without changing deletion behavior.src/services/api/me/me.resolver.fields.ts (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "degrade on missing actor" guard into a shared helper. Seven resolver fields across two files repeat the same
if (!actorContext.actorID) { this.logger.warn(...); return <default>; }pattern, differing only in the log message and default value. The shared root cause is the lack of a common helper for this cross-cutting concern.
src/services/api/me/me.resolver.fields.ts#L38-L46: replace with a call to a shared helper, e.g.this.degradeOrElse('me.notifications', () => this.inAppNotificationService.getPaginatedNotifications(...)), passing the empty-page default.src/services/api/me/me.resolver.fields.ts#L63-L67: use the same helper with0as the default.src/services/api/me/me.resolver.fields.ts#L112-L116: use the same helper with0as the default.src/services/api/me/me.resolver.fields.ts#L137-L141: use the same helper with[]as the default.src/services/api/me/me.resolver.fields.ts#L163-L167: use the same helper with[]as the default.src/services/api/me/me.resolver.fields.ts#L225-L229: use the same helper with{}as the default.src/services/api/me/me.conversations.resolver.fields.ts#L24-L28: use the same helper (or a shared base class/mixin, since this is a different resolver class) with[]as the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/api/me/me.resolver.fields.ts` around lines 38 - 46, Extract the repeated missing-actor guard into a shared degradeOrElse helper that accepts the field name, fallback value, and resolver operation, while preserving the warning behavior. Replace the guards in src/services/api/me/me.resolver.fields.ts at lines 38-46, 63-67, 112-116, 137-141, 163-167, and 225-229 with helper calls using defaults PaginatedInAppNotifications empty page, 0, 0, [], [], and {} respectively; update src/services/api/me/me.conversations.resolver.fields.ts lines 24-28 to use the same shared helper or a base-class/mixin equivalent with [] as its default.src/core/auth/oidc/audit.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
@corepath alias for the revocation type import. Replace the relative path with@core/auth/oidc/revocation/session-revocation.types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/audit.ts` at line 3, Update the SessionRevocationReason import in audit.ts to use the `@core/auth/oidc/revocation/session-revocation.types` alias instead of the relative revocation path.Source: Coding guidelines
src/core/auth/oidc/oidc-core.module.ts (1)
39-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach an error listener to the shared Redis client.
The
OIDC_REDIS_CLIENTfactory creates the client with no.on('error', ...)listener.ioredisonly emitserrorevents when a listener is registered; without one, connection errors on this now-shared client (session store and per-subject index) are silently dropped rather than logged.Add a listener that logs the error with the Winston logger at warn or error level, so a Redis outage on this shared connection is visible in operations instead of failing silently.
🔧 Proposed fix
{ provide: OIDC_REDIS_CLIENT, inject: [ConfigService], useFactory: (configService: ConfigService<AlkemioConfig, true>) => { const { host, port } = configService.get('storage.redis', { infer: true, }); - return new Redis({ host, port: Number(port) }); + const client = new Redis({ host, port: Number(port) }); + client.on('error', err => { + logger.warn('OIDC Redis client error', { error: err.message }); + }); + return client; }, },Please confirm with the latest
ioredisdocumentation whether an unhandlederrorevent on a client instance can, under any configuration used here, still surface as an unhandled rejection/exception rather than being silently dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/oidc-core.module.ts` around lines 39 - 48, In the OIDC_REDIS_CLIENT factory function, attach an error event listener to the Redis client instance that is created by new Redis({ host, port: Number(port) }) before returning it. The listener should log errors using the Winston logger at warn or error level to ensure connection errors on this shared Redis client are visible in operations rather than silently dropped. Inject the Logger service into the factory to access the logging capability.specs/107-oidc-session-revocation/contracts/audit-events.md (1)
76-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the overloaded
error_codefield.
error_codeholds the outcome name (for example"revoked") on success and the redacted failure cause on failure. A field namederror_codethat is always populated, even on success, can mislead a log consumer who filters on "non-nullerror_code" to find real errors. Consider renaming this field (for exampleoutcome_code) or documenting explicitly that it is not error-specific.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/107-oidc-session-revocation/contracts/audit-events.md` around lines 76 - 88, The `error_code` field in the audit events table holds outcome names like "revoked" on success and error details on failure, which is misleading for consumers filtering on non-null values to find real errors. Either rename the `error_code` field to a neutral name like `outcome_code` throughout the table and related documentation to clarify it always contains a meaningful status value, or add explicit documentation in the field description stating that the field is populated on both success and failure outcomes and that it contains outcome names (like "revoked") on success and redacted error causes on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/107-oidc-session-revocation/quickstart.md`:
- Around line 16-18: Replace the contributor-specific absolute path in the
quickstart installation steps with a repository-relative instruction, such as
changing to the repository root, while preserving the subsequent pnpm install
command.
In `@specs/107-oidc-session-revocation/spec.md`:
- Around line 21-33: Close the pre-deployment gap described around
revokeAllForSub by ensuring account deletion invalidates sessions that have not
yet been indexed, using a subject-level revocation marker checked during session
validation or another safe bounded mechanism. Add coverage for deletion
occurring before the session’s first authenticated request, and update the
affected specification text and User Story 1/SC-001 traceability so the behavior
is consistent.
In `@src/core/auth/oidc/revocation/oidc-session-revocation.service.ts`:
- Around line 140-144: The revokedCount and failedCount filters overlap when an
entry has outcome === 'revoked' but tokenRevocation === 'failed', causing both
counters to count the same session. Refactor the failedCount filter to exclude
entries already counted in revokedCount by checking only for outcome ===
'failed' and adding a separate counter for remote revocation failures where
outcome === 'revoked' && tokenRevocation === 'failed'. Update the
SessionRevocationReport interface to include the new remote-failure counter,
update the complete flag derivation to account for all failure paths, and update
the test assertions at lines 479, 544, and 797 to match the partitioned counter
logic so the audit record reflects distinct session outcomes rather than
overlapping counts.
- Around line 134-138: Replace the sequential await pattern in the loop that
processes sids with a bounded concurrency approach. Instead of awaiting each
this.revokeOne call individually within the loop iteration, collect the promises
and execute them with a concurrency limit (such as using Promise.allSettled with
chunking or a concurrency-control library). Preserve the order of entries if
required, or ensure the final entries array is properly populated from the
concurrent batch results. Verify that the test assertions in
oidc-session-revocation.service.spec.ts at Lines 295-300 and Line 325 still
pass, since they currently assume strict command ordering that may no longer
hold under bounded concurrency.
In `@src/core/auth/oidc/revocation/revocation-ends-access.spec.ts`:
- Around line 90-110: Update the seedPayload fixture so its alkemio_actor_id is
a real actor identifier such as actor-1, ensuring CookieSessionStrategy.validate
produces an authenticated context. Replace the resolves.not.toBeNull()
assertions in the affected baseline and revocation tests with assertions
matching the expected actorID and isAnonymous: false, including the cases around
the identified assertion groups.
In `@src/core/auth/oidc/session-index.redis.ts`:
- Around line 59-73: The TTL update in the session-index operation must be
atomic and never shorten an existing expiry. Replace the separate redis.sadd,
redis.ttl, and redis.expire calls with an atomic Lua script that performs SADD,
sets the candidate TTL when the key has no expiry, and extends an existing TTL
only when the candidate is greater; then update the affected tests to cover
initialization, extension, and non-shortening behavior.
In `@src/core/auth/oidc/strategies/cookie-session.strategy.ts`:
- Around line 131-146: Update the self-healing flow around reindexSession and
addSessionToSubIndex so index maintenance uses a single Redis pipeline rather
than separate SADD, TTL, and EXPIRE round trips. Reuse the session-index Redis
implementation, including EXPIRE GT semantics, to eliminate the TTL read while
preserving non-blocking error handling and the existing tombstone/TTL branch
behavior.
---
Nitpick comments:
In `@specs/107-oidc-session-revocation/contracts/audit-events.md`:
- Around line 76-88: The `error_code` field in the audit events table holds
outcome names like "revoked" on success and error details on failure, which is
misleading for consumers filtering on non-null values to find real errors.
Either rename the `error_code` field to a neutral name like `outcome_code`
throughout the table and related documentation to clarify it always contains a
meaningful status value, or add explicit documentation in the field description
stating that the field is populated on both success and failure outcomes and
that it contains outcome names (like "revoked") on success and redacted error
causes on failure.
In `@src/core/auth/oidc/audit.ts`:
- Line 3: Update the SessionRevocationReason import in audit.ts to use the
`@core/auth/oidc/revocation/session-revocation.types` alias instead of the
relative revocation path.
In `@src/core/auth/oidc/oidc-core.module.ts`:
- Around line 39-48: In the OIDC_REDIS_CLIENT factory function, attach an error
event listener to the Redis client instance that is created by new Redis({ host,
port: Number(port) }) before returning it. The listener should log errors using
the Winston logger at warn or error level to ensure connection errors on this
shared Redis client are visible in operations rather than silently dropped.
Inject the Logger service into the factory to access the logging capability.
In `@src/core/auth/oidc/oidc.controller.ts`:
- Around line 556-563: Extract the repeated session-destruction and deindexing
sequence into one private helper, preserving the invariant that the subject is
captured before req.session.destroy runs. Update all three call sites, including
the blocks around the referenced flows, to invoke this helper and remove their
duplicated Promise wrapper and direct deindexSession calls.
- Around line 100-101: Replace the direct NestJS Logger instances in
src/core/auth/oidc/oidc.controller.ts lines 100-101 and
src/core/auth/oidc/strategies/cookie-session.strategy.ts line 45 with injected
LoggerService instances from WINSTON_MODULE_NEST_PROVIDER; pass a LogContext
value to the indexSession, deindexSession, and reindexSession warning calls
using the (message, context) signature. Update both spec files to assert against
the injected logger mock instead of Logger.prototype.warn.
In `@src/core/auth/oidc/revocation/subject-contract.spec.ts`:
- Around line 110-120: Replace the assignment-and-read assertion in the
authenticationID test with a type-level assertion that verifies
User.authenticationID accepts string or null, preserving the intended nullable
contract without relying on runtime assignment. Keep the test focused on the
User entity’s declared property type and retain the existing spec naming.
In `@src/domain/community/user/user.service.ts`:
- Around line 589-628: Extract the duplicated catch-block logging in the user
deletion flow into a small helper method, preserving the existing logger payload
fields, stack argument, and LogContext.AUTH value. Have the helper accept the
operation-specific message, then call it from both revokeAllForSub and
invalidateAllIdentitySessions error handlers without changing deletion behavior.
In `@src/services/api/me/me.resolver.fields.ts`:
- Around line 38-46: Extract the repeated missing-actor guard into a shared
degradeOrElse helper that accepts the field name, fallback value, and resolver
operation, while preserving the warning behavior. Replace the guards in
src/services/api/me/me.resolver.fields.ts at lines 38-46, 63-67, 112-116,
137-141, 163-167, and 225-229 with helper calls using defaults
PaginatedInAppNotifications empty page, 0, 0, [], [], and {} respectively;
update src/services/api/me/me.conversations.resolver.fields.ts lines 24-28 to
use the same shared helper or a base-class/mixin equivalent with [] as its
default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0451d0ad-2bbe-4439-9341-67f1c4c47458
📒 Files selected for processing (35)
.specify/feature.jsonCLAUDE.mdspecs/107-oidc-session-revocation/checklists/requirements.mdspecs/107-oidc-session-revocation/contracts/audit-events.mdspecs/107-oidc-session-revocation/contracts/graphql-me-degradation.mdspecs/107-oidc-session-revocation/contracts/redis-keyspace.mdspecs/107-oidc-session-revocation/contracts/session-revocation-service.mdspecs/107-oidc-session-revocation/data-model.mdspecs/107-oidc-session-revocation/plan.mdspecs/107-oidc-session-revocation/quickstart.mdspecs/107-oidc-session-revocation/research.mdspecs/107-oidc-session-revocation/spec.mdspecs/107-oidc-session-revocation/tasks.mdsrc/core/auth/oidc/audit.tssrc/core/auth/oidc/oidc-core.module.tssrc/core/auth/oidc/oidc.controller.spec.tssrc/core/auth/oidc/oidc.controller.tssrc/core/auth/oidc/oidc.module.tssrc/core/auth/oidc/oidc.tokens.tssrc/core/auth/oidc/revocation/oidc-session-revocation.service.spec.tssrc/core/auth/oidc/revocation/oidc-session-revocation.service.tssrc/core/auth/oidc/revocation/revocation-ends-access.spec.tssrc/core/auth/oidc/revocation/session-revocation.types.tssrc/core/auth/oidc/revocation/subject-contract.spec.tssrc/core/auth/oidc/session-index.redis.spec.tssrc/core/auth/oidc/session-index.redis.tssrc/core/auth/oidc/strategies/cookie-session.strategy.index.spec.tssrc/core/auth/oidc/strategies/cookie-session.strategy.tssrc/domain/community/user/user.module.tssrc/domain/community/user/user.service.delete.spec.tssrc/domain/community/user/user.service.tssrc/services/api/me/me.conversations.resolver.fields.spec.tssrc/services/api/me/me.conversations.resolver.fields.tssrc/services/api/me/me.resolver.fields.spec.tssrc/services/api/me/me.resolver.fields.ts
Addresses two rounds of review on #6324 (CodeRabbit, Copilot, and a local xhigh pass) — 24 findings, none declined. The headline defect: the per-session tombstone is not a trustworthy record that a revocation happened, because this feature does not own the key it lives in. express-session owns alkemio:sid:<sid>, so a request already in flight when revocation runs holds the pre-revocation payload and persists it afterwards (an explicit save() on /refresh, or the lazy idle renewal firing at response end), erasing the tombstone. By then the sid has also been SREM'd from the index, so no retry can find it — the session is alive and permanently unrevocable, which is the bug this PR exists to fix, reachable by a race whose window is a whole request. Independently, sessions minted before the index shipped are not in it at all, so revoking a user who has not made a request since deploy enumerates nothing. Both are fixed by an account-level marker, alkemio:subrevoked:<sub>, holding the epoch-second at which the subject was revoked and checked on every authenticated request. It needs neither an intact payload nor index membership. A timestamp rather than a flag is what keeps it from being a permanent ban: a session whose created_at is later was minted after the revocation and passes, so a re-login needs no cleanup. Not written for exceptSid revocations — it rejects by subject and cannot spare the surviving session. Reads fail OPEN: a hard Redis outage already 503s at the session read, so a failure here means Redis is up and one command blipped, and failing closed would sign out the platform to defend a window the tombstone already covers. Correctness - both index writes are single atomic EVAL scripts. Client-side SADD/TTL/EXPIRE could die between the first and the last (key immortal) or interleave with a concurrent login so the shorter TTL lands last and evicts a live member. EXPIRE ... GT alone does not fix it: Redis reads a key with no expiry as infinite, so GT never sets the FIRST expiry - the login callback de-indexes the sid regenerate() destroyed. Nothing else did — logout and revocation are the only de-index paths and neither runs on a re-login — so every re-login left a phantom member, and a later revocation emitted one session.revoked record per phantom, outcome=success - session teardown runs with bounded concurrency (5). The 3s RFC 7009 bound governs one call; awaiting strictly in turn multiplied it by the session count and could stall deleteUser for tens of seconds against a black-holed Hydra Audit integrity - revokedCount / failedCount / tokenRevocationFailedCount now partition the entries. A session tombstoned locally whose upstream grant survived was counted in two buckets and read, in the summary, as a session still alive - a missing revocation_endpoint reports skipped, not failed. initDiscovery is fire-and-forget with indefinite retry, so an unresolved endpoint is routine in dev, CI and restart waves; reporting it as failure audited fully successful teardowns as outcome=failure - post-tombstone cleanup moved past the success boundary, so a Redis blip during the SREM no longer reports a provably-401ing session as failed - no blind DEL of the refresh-lock key. The mutex in production is the in-process refreshInFlight Map, so the key is never written; and releaseRefreshLock is an owner-checked compare-and-delete specifically so a lock cannot be stolen, which an unconditional DEL would defeat Leak and noise - scrubbing floors at 4 characters, not 8: `secrets` carries the cached display name, and short real names were sailing through the gate that was their only defence — no backstop pattern matches a personal name - deleteUser passes error.stack through redactStack. A stack begins with the error's own message, so logging it raw defeats a redacted message — the practice this same module documents as a leak vector - me degradation logs drop WARN -> verbose and route through Winston + LogContext.AUTH. Anonymous and guest contexts both carry an empty actorID and the SPA queries me on nearly every page load, so this was up to six WARN lines per logged-out visit, and none of it was filterable in Elastic - the dead me.conversations guard is gone; both branches returned the same empty envelope, so it only ever added a log line Tests (+26) - the race, end to end: revoke, write the live payload back over the tombstone, assert the session still 401s - an unindexed session revoked with entries: [] and still rejected - a session created after the revocation still authenticates - marker semantics incl. later-timestamp-wins; re-login de-indexing; marker skipped for exceptSid; marker failure tolerated; counter partitioning - the end-to-end fixture carried alkemio_actor_id: null, so validate() returned an ANONYMOUS context and every resolves.not.toBeNull() passed for a session that never authenticated — including the baseline test that existed to stop the suite passing for the wrong reason. Now asserts the identity - Logger.prototype spies removed. With isolate: false and clearMocks (not restoreMocks), an unrestored prototype spy leaks a no-op logger into every later file in the worker - the strategy index spec derived its clock from a hardcoded epoch while the guard compares against Date.now(), so four tests would have started failing on 2027-02-14 with an unrelated-looking 401 Refs #6315 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (6)
specs/107-oidc-session-revocation/contracts/graphql-me-degradation.md:133
- This acceptance bullet says seven warn lines are emitted, but the implementation/tests emit
verboselogs for these degradation paths. Keeping this aligned avoids confusion when manually verifying behavior.
- seven warn lines were emitted.
specs/107-oidc-session-revocation/quickstart.md:164
- Quickstart says the post-fix behavior produces “Seven warn lines”, but the implementation emits
verboselogs for degradedmefields. The quickstart should match the real log level to avoid false-negative manual verification.
`[]`; `notifications.total` `0`. Seven warn lines in the server log.
specs/107-oidc-session-revocation/contracts/session-revocation-service.md:80
- Session revocation docs say the revocation flow deletes
alkemio:sid:<sid>:refresh-lock, but the implementation explicitly does not touch refresh-lock keys (see the FR-011 note inOidcSessionRevocationService#revokeOne). This contract should match the shipped behavior so operators/reviewers don’t verify the wrong thing.
3. `DEL alkemio:sid:<sid>:refresh-lock` (FR-011).
specs/107-oidc-session-revocation/data-model.md:46
- This data model section states the
alkemio:sid:<sid>:refresh-lockkey is deleted during revocation, but the revocation service deliberately does not delete refresh-lock keys (it only tombstones + prunes the subject index + does RFC7009). Keeping this accurate matters for operational/debug expectations.
`refresh-lock.ts:16`. Deleted as part of revocation (FR-011) so nothing keyed to
a dead session lingers.
specs/107-oidc-session-revocation/quickstart.md:118
- Quickstart verification currently claims the refresh mutex key is removed by revocation, but the implementation explicitly does not delete
:refresh-lockkeys. This step can mislead manual verification and incident response.
This issue also appears on line 164 of the same file.
# c) The refresh mutex is gone
redis-cli EXISTS alkemio:sid:<sid>:refresh-lock
# → 0
specs/107-oidc-session-revocation/contracts/graphql-me-degradation.md:88
- This section says the
medegradation emits warn-level logs, but the implementation/tests uselogger.verbose(...)(to avoid log noise for expected anonymous traffic). The contract should match the actual log level.
This issue also appears on line 133 of the same file.
Every degraded field emits exactly one **warn**-level line before returning:
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/107-oidc-session-revocation/contracts/redis-keyspace.md (1)
82-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate invariant I2 to reference the actual enforcement.
The
Enforced bycell namesthe max(...) above. The TTL-roll script at lines 45-54 contains nomax(...); it uses thettl < 0 or candidate > ttlbranch. Align the wording so the invariant stays traceable to the script.📝 Proposed wording
-| I2 | TTL only ever grows | the `max(...)` above + spec | +| I2 | TTL only ever grows | the `ttl < 0 or candidate > ttl` branch in the `EVAL` above + spec |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/107-oidc-session-revocation/contracts/redis-keyspace.md` at line 82, Update invariant I2 in the Redis keyspace specification to reference the TTL-roll script’s actual enforcement condition, specifically the `ttl < 0 or candidate > ttl` branch, instead of referring to the nonexistent `max(...)` expression. Preserve the invariant’s meaning that TTL only increases.
🧹 Nitpick comments (6)
src/core/auth/oidc/revocation/revocation-ends-access.spec.ts (1)
496-511: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider pinning the marker boundary.
The fresh session uses
created_at = now + 5, so the test does not distinguish a>comparison from a>=comparison in the marker check. A session minted in the same second as the revocation is the realistic edge case. Add one case wherecreated_atequals the marker timestamp, and assert the expected decision.This is optional; the current test already covers the "not a permanent ban" property.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/revocation/revocation-ends-access.spec.ts` around lines 496 - 511, Extend the revocation boundary tests around revokeAllForSub and the fresh-session validation to cover a session whose created_at exactly equals the revocation marker timestamp, asserting the expected authentication decision; keep the existing future-timestamp case that verifies revocation is not permanent.src/domain/community/user/user.service.ts (3)
593-635: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider emitting a domain event instead of calling the OIDC service directly.
UserServicenow callsOidcSessionRevocationServicedirectly after the commit. The constructor comment on Lines 89-90 records the cost: the direct dependency would be a module cycle, soOidcCoreModuleexists to break it. AUserDeleteddomain event with an OIDC-side listener would remove that coupling, remove the need for the cycle workaround, and keep the best-effort semantics, because a listener failure cannot fail the mutation.Two properties would need care in the event route: the event must publish after the commit, and the listener must receive
authenticationID. If the team prefers the direct call for delivery guarantees, keep it and record the decision in the spec so the divergence from the event standard is traceable.As per coding guidelines: "Emit domain events instead of performing direct repository writes from API or orchestration code" and "For new GraphQL surface area, align with
docs/Pagination.md, enforce DTO validation, and emit domain events instead of direct repository writes".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/community/user/user.service.ts` around lines 593 - 635, Replace the direct OIDC revocation call in the user-deletion flow with a UserDeleted domain event. Publish it only after the deletion transaction commits and include authenticationID in the event payload. Add an OIDC-side listener that invokes OidcSessionRevocationService.revokeAllForSub with best-effort error handling, then remove the direct dependency and related cycle workaround from UserService.Source: Coding guidelines
593-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
user.authenticationIDguards.Lines 593 and 641 test the same condition and the blocks are adjacent. One guard would hold the session revocation, the Kratos session invalidation, the metadata clear, and the conditional identity deletion, in that order. This keeps the documented ordering and removes one branch.
Also applies to: 641-641
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/community/user/user.service.ts` at line 593, The user deletion flow in user.service.ts contains two separate if guards checking the same user.authenticationID condition, creating redundant branching. Merge the two user.authenticationID guards into a single conditional block, combining the operations from both blocks while preserving their documented order: session revocation, followed by Kratos session invalidation, followed by metadata clear, followed by conditional identity deletion. This eliminates the duplicate branch while maintaining the correct operation sequence.
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the redaction helpers from
oidc-session-revocation.service.ts.Both helpers are consumed by the revocation service and
user.service.ts. Move them to a separate redaction module and update both imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/community/user/user.service.ts` around lines 18 - 22, Extract the redactError and redactStack helpers from OidcSessionRevocationService into a dedicated redaction module, then update imports in both OidcSessionRevocationService and the user service to use that module while preserving their existing behavior and exports.src/core/auth/oidc/session-index.redis.ts (1)
86-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFloor the candidate TTL before it reaches
EXPIRE.
candidateTtlSisabsoluteExpiresAtEpochSeconds - nowS. If a caller ever passes a fractionalabsolute_expires_at, the value stays fractional andEXPIRErejects it withvalue is not an integer or out of range. The wholeEVALthen fails, so theSADDis rolled back and the session is never indexed. Today's producers pass integers, so this is defensive only, but the failure mode is a silent loss of index coverage.Add the floor in the script so it holds for every caller.
♻️ Proposed fix
local added = redis.call('SADD', KEYS[1], ARGV[1]) -local candidate = tonumber(ARGV[2]) +local candidate = math.floor(tonumber(ARGV[2])) if candidate < 1 then candidate = 1 end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/session-index.redis.ts` around lines 86 - 97, Floor the TTL inside ADD_AND_ROLL_TTL_LUA before passing it to EXPIRE, ensuring fractional absoluteExpiresAtEpochSeconds values become valid integer TTLs for every caller. Keep the existing candidateTtlS calculation and EVAL invocation behavior unchanged.src/core/auth/oidc/strategies/cookie-session.strategy.index.spec.ts (1)
262-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unindexed-session test distinct, and pin the equality boundary.
Two points on this
describeblock:
This test is byte-equivalent in behaviour to
rejects a session minted BEFORE the subject was revokedat lines 230-241. Both build a default payload and setsubRevokedAt = created_at + 1. Nothing here models index membership, so the test cannot demonstrate the property its comment claims. Either assert something the other test does not — for example that no index read (smembers) happens before the rejection — or fold it into the earlier test.The block does not cover
revokedAt === payload.created_at. The implementation rejects on equality (revokedAt >= payload.created_atincookie-session.strategy.tsline 130), so a same-second re-login is refused. That operator is the whole control; pin it with a test so a later change to>is caught.Based on learnings: use a risk-based testing approach; add tests when they provide real signal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth/oidc/strategies/cookie-session.strategy.index.spec.ts` around lines 262 - 275, The unindexed-session test duplicates an existing revocation test and does not verify index-independent rejection. Update the relevant describe block to either remove it or assert the distinct behavior that no smembers/index read occurs before rejection; additionally add a test covering revokedAt equal to payload.created_at, verifying cookie-session validation rejects the session at the >= boundary.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/community/user/user.service.ts`:
- Around line 606-611: Update the catch blocks for clearIdentityActorMetadata
and deleteIdentityById to use redactError(error) for logged errors and
redactStack(error) for logged stacks, matching invalidateAllIdentitySessions.
Ensure no raw error?.message or error?.stack values remain in this deletion
path.
In `@src/services/api/me/me.resolver.fields.spec.ts`:
- Around line 283-286: Update the test case around resolver.conversations to
capture its returned value and assert that it is the expected empty conversation
envelope, while retaining the existing logger.verbose non-invocation assertion.
Ensure the test verifies both the result and the absence of logging.
---
Outside diff comments:
In `@specs/107-oidc-session-revocation/contracts/redis-keyspace.md`:
- Line 82: Update invariant I2 in the Redis keyspace specification to reference
the TTL-roll script’s actual enforcement condition, specifically the `ttl < 0 or
candidate > ttl` branch, instead of referring to the nonexistent `max(...)`
expression. Preserve the invariant’s meaning that TTL only increases.
---
Nitpick comments:
In `@src/core/auth/oidc/revocation/revocation-ends-access.spec.ts`:
- Around line 496-511: Extend the revocation boundary tests around
revokeAllForSub and the fresh-session validation to cover a session whose
created_at exactly equals the revocation marker timestamp, asserting the
expected authentication decision; keep the existing future-timestamp case that
verifies revocation is not permanent.
In `@src/core/auth/oidc/session-index.redis.ts`:
- Around line 86-97: Floor the TTL inside ADD_AND_ROLL_TTL_LUA before passing it
to EXPIRE, ensuring fractional absoluteExpiresAtEpochSeconds values become valid
integer TTLs for every caller. Keep the existing candidateTtlS calculation and
EVAL invocation behavior unchanged.
In `@src/core/auth/oidc/strategies/cookie-session.strategy.index.spec.ts`:
- Around line 262-275: The unindexed-session test duplicates an existing
revocation test and does not verify index-independent rejection. Update the
relevant describe block to either remove it or assert the distinct behavior that
no smembers/index read occurs before rejection; additionally add a test covering
revokedAt equal to payload.created_at, verifying cookie-session validation
rejects the session at the >= boundary.
In `@src/domain/community/user/user.service.ts`:
- Around line 593-635: Replace the direct OIDC revocation call in the
user-deletion flow with a UserDeleted domain event. Publish it only after the
deletion transaction commits and include authenticationID in the event payload.
Add an OIDC-side listener that invokes
OidcSessionRevocationService.revokeAllForSub with best-effort error handling,
then remove the direct dependency and related cycle workaround from UserService.
- Line 593: The user deletion flow in user.service.ts contains two separate if
guards checking the same user.authenticationID condition, creating redundant
branching. Merge the two user.authenticationID guards into a single conditional
block, combining the operations from both blocks while preserving their
documented order: session revocation, followed by Kratos session invalidation,
followed by metadata clear, followed by conditional identity deletion. This
eliminates the duplicate branch while maintaining the correct operation
sequence.
- Around line 18-22: Extract the redactError and redactStack helpers from
OidcSessionRevocationService into a dedicated redaction module, then update
imports in both OidcSessionRevocationService and the user service to use that
module while preserving their existing behavior and exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c38340ce-2be6-4962-a476-20b0bb8a9fe3
📒 Files selected for processing (23)
specs/107-oidc-session-revocation/contracts/redis-keyspace.mdspecs/107-oidc-session-revocation/quickstart.mdspecs/107-oidc-session-revocation/spec.mdsrc/core/auth/oidc/audit.tssrc/core/auth/oidc/oidc.controller.spec.tssrc/core/auth/oidc/oidc.controller.tssrc/core/auth/oidc/revocation/oidc-session-revocation.service.spec.tssrc/core/auth/oidc/revocation/oidc-session-revocation.service.tssrc/core/auth/oidc/revocation/revocation-ends-access.spec.tssrc/core/auth/oidc/revocation/session-revocation.types.tssrc/core/auth/oidc/revocation/subject-contract.spec.tssrc/core/auth/oidc/session-index.redis.spec.tssrc/core/auth/oidc/session-index.redis.tssrc/core/auth/oidc/strategies/cookie-session.strategy.index.spec.tssrc/core/auth/oidc/strategies/cookie-session.strategy.spec.tssrc/core/auth/oidc/strategies/cookie-session.strategy.tssrc/domain/community/user/user.service.tssrc/services/api/me/me.conversations.resolver.fields.spec.tssrc/services/api/me/me.conversations.resolver.fields.tssrc/services/api/me/me.resolver.fields.spec.tssrc/services/api/me/me.resolver.fields.tstest/integration/oidc/bearer-test-harness.tstest/integration/oidc/oidc-test-harness.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/services/api/me/me.conversations.resolver.fields.ts
- src/services/api/me/me.conversations.resolver.fields.spec.ts
- src/core/auth/oidc/session-index.redis.spec.ts
- src/core/auth/oidc/revocation/subject-contract.spec.ts
Round 2 of review on #6324 (CodeRabbit, 2 findings, both agreed). - clearIdentityActorMetadata and deleteIdentityById still logged error.message and error.stack raw. They hit the same Kratos client as invalidateAllIdentitySessions, so they can surface the same material, and a stack repeats the message — which is what defeats a redacted message. Both now use redactError/redactStack, so the deletion path has one policy rather than a redacted first call and two unredacted ones. - the me.conversations degradation test asserted only that nothing was logged, so a wrong non-empty result would have passed. It now asserts the empty envelope as well. Refs #6315 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (2)
specs/107-oidc-session-revocation/contracts/session-revocation-service.md:81
- The contract claims per-session teardown includes deleting
alkemio:sid:<sid>:refresh-lock, but the implementation inOidcSessionRevocationService#revokeOneexplicitly does NOT delete the refresh lock (seesrc/core/auth/oidc/revocation/oidc-session-revocation.service.ts:353+). This mismatch makes the contract misleading for future consumers/maintainers.
1. `get(sid)` → capture `client_id` and `refresh_token` **before** any mutation
(the tombstone blanks them).
2. `markTerminated(sid, reason, { sub, client_id })`.
3. `DEL alkemio:sid:<sid>:refresh-lock` (FR-011).
4. `SREM alkemio:sub:<sub> sid`.
specs/107-oidc-session-revocation/contracts/graphql-me-degradation.md:88
- This contract says degraded
mefields log at warn level, but the implementation/tests uselogger.verbose(e.g.src/services/api/me/me.resolver.fields.ts:41+,me.resolver.fields.spec.ts:98+). The contract should match the actual log level (and the rationale inme.conversations.resolver.fields.spec.tsfor avoiding WARN noise).
Every degraded field emits exactly one **warn**-level line before returning:
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #6315
Deleting a user left their BFF/OIDC session untouched in Redis. The browser kept
authenticating against an account that no longer existed for up to the 30-day
absolute ceiling — an access-control failure (ISO 27001 A.5.18, SOC 2 CC6.2),
and an incomplete erasure too, since the session payload caches display name and
email (GDPR Art. 17).
The root cause is a missing primitive, not a missing call. Sessions live at
alkemio:sid:<sid>and there is no way to get from a subject to that subject'ssessions, so they cannot be enumerated, so they cannot be revoked. That same gap
blocks client-web#10070 and server#6073.
What this changes
WS1 — per-subject session index +
OidcSessionRevocationServicealkemio:sub:<sub>Redis set. TTL rolled forward to the latestmember's absolute ceiling and never shortened — a
SADDwithout anEXPIREleaks the key forever, and a shortened TTL would evict a live member and
make revocation silently miss a session.
revokeAllForSub(sub, reason, opts?)tombstones every session withmarkTerminated, neverdestroy. This is the whole fix. A tombstoneyields 401 and deterministically flips
client-webto signed-out;destroyyields a silent anonymous fall-through with HTTP 200 — the reported bug in a
new costume. The tombstone also nulls
request_context_cache, so one verbcloses both the access defect and the PII-residue defect.
Hydra (RFC 7009 — did not exist anywhere in this repo before). 3 s
timeout, no retry, no circuit breaker, each with a written rationale inline
because constitution principle 8 requires all three.
leave a session alive.
exceptSid, per-session outcomes and partial-failure tolerance soserver#6073 and client-web#10070 consume it unchanged.
CookieSessionStrategy, fire-and-forget.Without it every session alive on deploy day is permanently unrevocable —
precisely the population carrying this bug. Unawaited, but not unobserved: the
.catch()logs.WS2 — the deletion cascade
deleteUsernow callsrevokeAllForSubandkratosService.invalidateAllIdentitySessions(which already existed and wassimply never called from here). Both unconditional — deliberately not
gated on
deleteData.deleteIdentity, since a surviving Kratos identity withlive sessions is exactly the orphan state. Both after commit, outside the
transaction, each individually trapped so neither can fail the delete
mutation. Deletion has broken against Kratos four times before (#5350, #5678,
#4762, #2137); a security improvement must not become an availability
regression.
WS3 — graceful degradation in
meThe seven
mesub-resolvers return0/[]/ an empty page /{}on anempty
actorIDinstead of throwing. Two of the seven were not in the originaltriage list, and missing either would have shipped a fix that does not work:
notificationsthrowsForbiddenException, notValidationException, so anexception-type sweep skips it — and it is the paginated non-nullable field.
conversationsthrower is inme.conversations.resolver.fields.ts.Relaxing only the container in
me.resolver.fields.tsleavesme { conversations { conversations } }erroring exactly as before — which iswhat
UserPendingMembershipsactually selects.Incidentally this also fixes a live defect wider than the orphan case:
createAnonymous()setsactorID = '', so these fields error for everyanonymous visitor today.
Structural note
New
OidcCoreModuleholdsOidcService, the Redis client, the session-storehandle and the revocation service.
UserModulecannot importOidcModule—that pulls in Authentication / ActorContext / Authorization /
PlatformAuthorizationPolicy, several of which sit above it, so it would be a
cycle. No
forwardRef: constitution principle 2 says circular dependenciesrequire redesign, and
forwardRefmakes a cycle legal rather than absent. Thehoist also collapses two Redis connections into one.
Footprint
No DDL. No migration. No GraphQL schema delta. Verified, not assumed —
schema:print/sort/diffproduced only unrelated description-text drift onCalloutContributorsMapView(a different feature's un-regenerated docstrings),which was reverted rather than folded in.
Audit-enum collision check (design-input trap 6)
Checked against
027-platform-role-redesign, whose slice A is code-complete andin review, so the risk was live rather than hypothetical:
account_deleted?PlatformAuditCategoryplatform_role_assignment,platform_user_record,platform_configuration,platform_resourcePlatformAuditOutcomeidentity_deleted,account_resetPlatformAuditInitiatorRoleNo literal collision — but the check surfaced something more useful. 027
already owns the database-side audit row for user deletion (its T062 writes
platform_user_record/identity_deleted), and its residual-risk registeralready carries an open high finding (
spec-server-27) that this row iswritten on only one branch. A second writer would make that finding unfixable.
So the revocation trail rides the existing OIDC audit stream instead
(
session.revocation.initiated→session.revokedper session →session.revocation.completed). It fits the data better anyway — these areper-session events carrying
sub/client_id/rp_id, none of whichplatform_audit_entryhas a column for. Net effect: zero DDL, and this mergeswith 027 in either order without touching
src/migrations/.Tests are the audit evidence
Per the compliance framing, "we call the method" is not proof that access ended.
revocation-ends-access.spec.tswires the real revocation service tothe real
CookieSessionStrategyover a shared in-memory Redis and assertsthe next request is rejected with
CookieSessionInvalidError.rejectsisload-bearing:
not.toBeNull()would pass for a merely-destroyed session.Also asserts the cached display name and email are gone, and that a different
subject's session still authenticates.
subject-contract.spec.tspinssession.sub === user.authenticationID(trap 7). If that cross-service convention ever changes,
revokeAllForSubwould return cheerful empty successful reports while revoking nothing — a
control failing open and silent. The test's comment says so explicitly, so a
future failure reads as "the subject source changed" rather than "the fixture
drifted".
upstream error message embedding the refresh token verbatim (
"revocation failed for token <value>"— no pattern reliably catches every shape), anderror.stackbeing logged raw, which defeats a redacted message entirely.Fixed by scrubbing the known secret values the session holds rather than
pattern-guessing, with the pattern rules kept as a backstop.
Gates:
pnpm lint,pnpm build,pnpm test:ci:no:coverage(7514 passed / 673files / 0 failures), schema contract — all green in one uninterrupted run.
SDD artifacts
specs/107-oidc-session-revocation/—spec.md,research.md(R0–R13),plan.md,data-model.md,quickstart.md,contracts/×4,tasks.md(39/39). Constitution check 10/10, no violations, no complexity-tracking
entries.
All ambiguities were resolved by decision and recorded in
spec.md'sClarifications section with rationale.
Every
file:linein the triage document was re-verified against this branch.Its behavioural claims all hold; two anchors had drifted, and its
meguardlist was two short (see WS3 above) —
research.mdR0/R9 record both.Findings recorded, not acted on
prompt: 'login'(alkemio#1989 lead) — confirmed as a code fact. It ishardcoded unconditionally on every authorization request at
oidc.controller.ts:228(the triage doc said:169; it drifted). Per OIDC Core§3.1.2.1 this forces re-authentication and is exactly what stops Hydra setting
skip=true, defeating SSO. The mechanism is real and present; whether it causes#1989 needs a live check. No change made — out of scope.
authenticationIDbackfill — assessed, recommended, not implemented.Migration
1764590889000-userIdentityCleanup.ts:26addsauthenticationID uuidnullable with no backfill, and cannot backfill in pure SQL because the
Kratos UUID is not derivable from the dropped email-based
accountUpn. There-link is lazy and login-only (
user.identity.service.ts, email → identity).WS1–WS3 do not require it, but there is an honest residual gap worth
stating plainly: for a user whose
authenticationIDisNULL,deleteUserskips revocation entirely (correctly — there is no subject to revoke by), so
their session is still not revoked on deletion. The exposure is bounded —
such a session resolves to no actor, so it carries no authorization, and after
WS3 it degrades to a clean empty state — but #6315's literal title stays true
for that subpopulation until the backfill lands. Recommendation: prioritise
the backfill as a follow-up; it needs a Kratos email→identity lookup inside a
migration, a pattern this repo does not have, so it deserves its own design.
Follow-ups to file
Enumerated for a human to action — no issues were created.
admin email-change flow (
user.email.change.service.ts:519, from Allow changing a user's login email while preserving their Alkemio profile #6064)revokes only the Kratos session, so the BFF session survives an admin email
change with full API access. Same defect class as this one, already
deployed, no issue filed.
revokeAllForSub(sub, 'email_changed')is readyand needs one call site.
authenticationIDbackfill migration — see above.of "What this unlocks" into must-have scope. Two open bugs (BUG: Deleting a user does NOT revoke their session (their session stays active BUT cannot login) #6315,
client-web#10070) plus one silent production defect currently sit in a
deliberately-deferred bucket that nobody owns.
Alkemio now has several RPs; without it each new one recreates this bug.
prompt: 'login'lead (alkemio#1989) — evidenceabove.
tokens and any assistant delegation tokens (alkemio#1955). OIDC: session management UI — per-session revoke + sign out of all devices #6073 names a
"webhook → Hydra / Synapse fan-out" that does not exist yet.
Downstream consumers of the WS1 primitive
exceptSid,which ships and is tested. No change to this service required.
exceptSid,per-session outcomes and partial-failure tolerance; all three ship. A
list-sessions read method is still to be added, and contract C10 already
guarantees it cannot expose token material.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation