From d684d0707e63cf241c2dc01e43fe7fcdba4199e1 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 07:52:10 -0400 Subject: [PATCH 1/6] fix(atproto): end custody on PDS password reset Change made because a custodial user who reset their PDS password was left half-migrated: the PDS had their new password while the API kept the old encrypted credentials and still treated the account as custodial, so every later custodial session attempt got a 401 and the user's events silently stopped publishing. - resetPdsPassword ends custody in the same request as the PDS write (clear stored credentials, mark non-custodial) instead of trusting the client to finish a 3-call flow - completeTakeOwnership becomes an idempotent no-op when the identity is already non-custodial, so the platform's existing flow still succeeds with no client change - completeTakeOwnership invalidates the cached PDS session so it cannot serve a stale agent - unit specs assert the custody write and session invalidation; the devnet e2e verifies custody ends on reset and that a failed reset leaves custody untouched --- .../atproto-identity-recovery.service.spec.ts | 25 +++++++++-- .../atproto-identity-recovery.service.ts | 19 ++++++-- .../atproto-identity.controller.spec.ts | 32 ++++++++------ .../atproto-identity.controller.ts | 14 +++++- test/pds/pds-password-reset.e2e-spec.ts | 43 +++++++++++++++++-- 5 files changed, 109 insertions(+), 24 deletions(-) diff --git a/src/atproto-identity/atproto-identity-recovery.service.spec.ts b/src/atproto-identity/atproto-identity-recovery.service.spec.ts index e110d264..cd4feae0 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.spec.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.spec.ts @@ -6,6 +6,7 @@ import { AtprotoIdentityRecoveryService } from './atproto-identity-recovery.serv import { UserAtprotoIdentityService } from '../user-atproto-identity/user-atproto-identity.service'; import { PdsAccountService } from '../pds/pds-account.service'; import { PdsCredentialService } from '../pds/pds-credential.service'; +import { PdsSessionService } from '../pds/pds-session.service'; import { UserService } from '../user/user.service'; import { UserAtprotoIdentityEntity } from '../user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity'; @@ -14,6 +15,7 @@ describe('AtprotoIdentityRecoveryService', () => { let userAtprotoIdentityService: jest.Mocked; let pdsAccountService: jest.Mocked; let pdsCredentialService: jest.Mocked; + let pdsSessionService: jest.Mocked; let userService: jest.Mocked; const mockUser = { @@ -59,6 +61,10 @@ describe('AtprotoIdentityRecoveryService', () => { encrypt: jest.fn(), }; + const mockPdsSessionService = { + invalidateSession: jest.fn(), + }; + const mockUserService = { findByUlid: jest.fn(), }; @@ -85,6 +91,10 @@ describe('AtprotoIdentityRecoveryService', () => { provide: PdsCredentialService, useValue: mockPdsCredentialService, }, + { + provide: PdsSessionService, + useValue: mockPdsSessionService, + }, { provide: UserService, useValue: mockUserService, @@ -106,6 +116,7 @@ describe('AtprotoIdentityRecoveryService', () => { userAtprotoIdentityService = module.get(UserAtprotoIdentityService); pdsAccountService = module.get(PdsAccountService); pdsCredentialService = module.get(PdsCredentialService); + pdsSessionService = module.get(PdsSessionService); userService = module.get(UserService); }); @@ -535,6 +546,12 @@ describe('AtprotoIdentityRecoveryService', () => { isCustodial: false, }, ); + + // Assert - cached session invalidated so it cannot serve a stale agent + expect(pdsSessionService.invalidateSession).toHaveBeenCalledWith( + 'test-tenant', + mockIdentityEntity.did, + ); }); it('should throw BadRequestException when no identity exists', async () => { @@ -547,7 +564,7 @@ describe('AtprotoIdentityRecoveryService', () => { ).rejects.toThrow(BadRequestException); }); - it('should throw BadRequestException when identity is already non-custodial', async () => { + it('should succeed as a no-op when identity is already non-custodial', async () => { // Arrange const nonCustodialIdentity = { ...mockIdentityEntity, @@ -557,10 +574,12 @@ describe('AtprotoIdentityRecoveryService', () => { nonCustodialIdentity as UserAtprotoIdentityEntity, ); - // Act & Assert + // Act & Assert - idempotent: resolves without touching the identity await expect( service.completeTakeOwnership('test-tenant', mockUser.ulid), - ).rejects.toThrow(BadRequestException); + ).resolves.toBeUndefined(); + expect(userAtprotoIdentityService.update).not.toHaveBeenCalled(); + expect(pdsSessionService.invalidateSession).not.toHaveBeenCalled(); }); }); }); diff --git a/src/atproto-identity/atproto-identity-recovery.service.ts b/src/atproto-identity/atproto-identity-recovery.service.ts index 23d19775..3bf4fd23 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.ts @@ -13,6 +13,7 @@ import { UserAtprotoIdentityService } from '../user-atproto-identity/user-atprot import { UserAtprotoIdentityEntity } from '../user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity'; import { PdsAccountService } from '../pds/pds-account.service'; import { PdsCredentialService } from '../pds/pds-credential.service'; +import { PdsSessionService } from '../pds/pds-session.service'; import { isServiceNotConfiguredError } from '../pds/pds-error-detection'; import { UserService } from '../user/user.service'; import { AllConfigType } from '../config/config.type'; @@ -40,6 +41,7 @@ export class AtprotoIdentityRecoveryService { private readonly userAtprotoIdentityService: UserAtprotoIdentityService, private readonly pdsAccountService: PdsAccountService, private readonly pdsCredentialService: PdsCredentialService, + private readonly pdsSessionService: PdsSessionService, private readonly userService: UserService, private readonly configService: ConfigService, @Inject(REQUEST) private readonly request?: any, @@ -248,11 +250,16 @@ export class AtprotoIdentityRecoveryService { } /** - * Complete take ownership: user confirms they've set password, we clear credentials. + * Complete take ownership: clear credentials, mark non-custodial, and + * invalidate any cached PDS session so it cannot serve a stale agent. + * + * Idempotent: an already-non-custodial identity is a successful no-op, so + * clients that call this after the server has already ended custody (the + * platform's reset flow) see success rather than a 400. * * @param tenantId - The tenant ID * @param userUlid - The user's ULID - * @throws BadRequestException if no custodial identity exists + * @throws BadRequestException if the user has no AT Protocol identity */ async completeTakeOwnership( tenantId: string, @@ -266,9 +273,10 @@ export class AtprotoIdentityRecoveryService { throw new BadRequestException('User has no AT Protocol identity'); } if (!identity.isCustodial) { - throw new BadRequestException( - 'User already owns their AT Protocol identity', + this.logger.log( + `Take ownership already complete for user ${userUlid}: identity is non-custodial`, ); + return; } // Clear credentials and mark as non-custodial @@ -277,6 +285,9 @@ export class AtprotoIdentityRecoveryService { isCustodial: false, }); + // The cached session was minted from the credentials we just cleared + await this.pdsSessionService.invalidateSession(tenantId, identity.did); + this.logger.log( `Completed take ownership for user ${userUlid}: cleared credentials, now non-custodial`, ); diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index 9912429f..2a61b473 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -619,30 +619,30 @@ describe('AtprotoIdentityController', () => { ).rejects.toThrow(BadRequestException); }); - it('should throw BadRequestException when user already owns identity', async () => { - // Arrange + it('should return success when user already owns identity (idempotent no-op)', async () => { + // Arrange - the service resolves without doing anything in this case jest .spyOn(recoveryService, 'completeTakeOwnership') - .mockRejectedValue( - new BadRequestException( - 'User already owns their AT Protocol identity', - ), - ); + .mockResolvedValue(undefined); - // Act & Assert - await expect( - controller.completeTakeOwnership(mockRequest), - ).rejects.toThrow(BadRequestException); + // Act + const result = await controller.completeTakeOwnership(mockRequest); + + // Assert + expect(result).toEqual({ success: true }); }); }); describe('resetPdsPassword', () => { - it('should reset password when user has custodial identity', async () => { + it('should reset password and end custody in the same request', async () => { // Arrange jest .spyOn(identityService, 'findByUserUlid') .mockResolvedValue(mockIdentityEntity as UserAtprotoIdentityEntity); jest.spyOn(pdsAccountService, 'resetPassword').mockResolvedValue(); + jest + .spyOn(recoveryService, 'completeTakeOwnership') + .mockResolvedValue(undefined); // Act const result = await controller.resetPdsPassword(mockRequest, { @@ -660,6 +660,11 @@ describe('AtprotoIdentityController', () => { 'valid-reset-token', 'new-secure-password-123', ); + // Custody ends server-side, not via a later client call + expect(recoveryService.completeTakeOwnership).toHaveBeenCalledWith( + 'test-tenant', + '01234567890123456789012345', + ); expect(result).toEqual({ success: true }); }); @@ -724,6 +729,9 @@ describe('AtprotoIdentityController', () => { password: 'new-secure-password-123', }), ).rejects.toThrow(BadRequestException); + + // A failed reset must leave custody untouched + expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); }); }); diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index 520ba7aa..b5a940bb 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -285,7 +285,9 @@ export class AtprotoIdentityController { /** * Reset PDS password using a token received via email. * - * User must have a custodial identity to use this endpoint. + * User must have a custodial identity to use this endpoint. On success the + * identity stops being custodial: stored credentials are cleared and the + * cached PDS session is invalidated in the same request. * Rate limited to prevent abuse - password reset is a sensitive operation. */ @ApiBearerAuth() @@ -301,7 +303,8 @@ export class AtprotoIdentityController { summary: 'Reset PDS password using token from email', }) @ApiOkResponse({ - description: 'Password reset successful', + description: + 'Password reset successful; the identity is no longer custodial', }) @ApiResponse({ status: HttpStatus.BAD_REQUEST, @@ -349,6 +352,13 @@ export class AtprotoIdentityController { throw error; } + // The user now owns the password they just set, so custody ends here, in + // the same request as the PDS write. Waiting for the client to call + // take-ownership/complete leaves stale stored credentials whenever that + // follow-up never arrives, and stale credentials silently break event + // publishing for the account. + await this.recoveryService.completeTakeOwnership(tenantId, user.ulid); + return { success: true }; } diff --git a/test/pds/pds-password-reset.e2e-spec.ts b/test/pds/pds-password-reset.e2e-spec.ts index b9385bcf..cacac240 100644 --- a/test/pds/pds-password-reset.e2e-spec.ts +++ b/test/pds/pds-password-reset.e2e-spec.ts @@ -146,7 +146,23 @@ describeIfPds('PDS Password Reset (e2e)', () => { expect(userHandle).toBeDefined(); }); - it('should reset PDS password and verify new password works', async () => { + it('should reject an invalid token and leave custody untouched', async () => { + // Valid format but non-existent token — PDS rejects it. This runs + // before the real reset, while the identity is still custodial. + await serverApp + .post('/api/atproto/identity/reset-pds-password') + .set('Authorization', `Bearer ${authToken}`) + .send({ token: 'AAAAA-BBBBB', password: 'ValidPassword123!' }) + .expect(400); + + const identityResponse = await serverApp + .get('/api/atproto/identity') + .set('Authorization', `Bearer ${authToken}`) + .expect(200); + expect(identityResponse.body.isCustodial).toBe(true); + }); + + it('should reset PDS password, verify new password works, and end custody', async () => { const newPassword = 'NewSecurePassword789!'; // Get reset token via email @@ -172,6 +188,26 @@ describeIfPds('PDS Password Reset (e2e)', () => { expect(sessionResponse.body.handle).toBe(userHandle); expect(sessionResponse.body.did).toBeDefined(); expect(sessionResponse.body.accessJwt).toBeDefined(); + + // Custody must end in the same request as the reset: the API no + // longer knows the password, and stale stored credentials would + // silently break event publishing for this account + const identityResponse = await serverApp + .get('/api/atproto/identity') + .set('Authorization', `Bearer ${authToken}`) + .expect(200); + expect(identityResponse.body.isCustodial).toBe(false); + }); + + it('should treat completeTakeOwnership as success after custody already ended', async () => { + // The platform client still calls take-ownership/complete after the + // reset; it must see an idempotent success, not a 400 + const completeResponse = await serverApp + .post('/api/atproto/identity/take-ownership/complete') + .set('Authorization', `Bearer ${authToken}`) + .expect(200); + + expect(completeResponse.body).toEqual({ success: true }); }); }); @@ -193,8 +229,9 @@ describeIfPds('PDS Password Reset (e2e)', () => { .expect(422); }); - it('should reject expired or invalid token from PDS', async () => { - // Valid format but non-existent token — PDS will reject it + it('should reject reset attempts once the user owns the identity', async () => { + // The flow above ended custody, so the non-custodial guard fires + // before any PDS call regardless of token validity await serverApp .post('/api/atproto/identity/reset-pds-password') .set('Authorization', `Bearer ${authToken}`) From d47606b0caf38c7a8bfd41ad4844d82bfe6ed8a6 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 08:49:47 -0400 Subject: [PATCH 2/6] fix(atproto): survive failure between PDS reset and custody flip Change made because the token is consumed the moment the PDS accepts the reset, so a crash or failed database write immediately after left the account stranded with stale custodial credentials, and the error response stopped the client from making the completion call that would have retried the flip. - resetPdsPassword reports success once the PDS write has succeeded; a custody-flip failure is logged loudly instead of masking the reset, which keeps the client's take-ownership/complete call as a live retry - PdsSessionService finishes an unrecorded handoff on use: a definitive 401 for stored custodial credentials can only mean the user reset the password themselves, so the service clears the credentials, marks the identity non-custodial, and invalidates the cached session; strictly 401-only, never on network errors or 5xx, and repair failures never mask the session error - no schema change: the PDS itself is the durable record of the handoff, and the sync scheduler's session attempts are the retry loop This also lazily heals identities that were stranded before the fix, the next time anything attempts a session for them. --- .../atproto-identity.controller.spec.ts | 22 ++++ .../atproto-identity.controller.ts | 15 ++- src/pds/pds-session.service.spec.ts | 105 +++++++++++++++++- src/pds/pds-session.service.ts | 64 ++++++++++- 4 files changed, 199 insertions(+), 7 deletions(-) diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index 2a61b473..f142d46d 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -668,6 +668,28 @@ describe('AtprotoIdentityController', () => { expect(result).toEqual({ success: true }); }); + it('should still return success when ending custody fails after the PDS reset', async () => { + // Arrange + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue(mockIdentityEntity as UserAtprotoIdentityEntity); + jest.spyOn(pdsAccountService, 'resetPassword').mockResolvedValue(); + jest + .spyOn(recoveryService, 'completeTakeOwnership') + .mockRejectedValue(new Error('DB write failed')); + + // Act + const result = await controller.resetPdsPassword(mockRequest, { + token: 'valid-reset-token', + password: 'new-secure-password-123', + }); + + // Assert - the PDS write succeeded and the token is consumed, so the + // response must not read as a failed reset; the client's follow-up + // take-ownership/complete call is the retry for the custody flip + expect(result).toEqual({ success: true }); + }); + it('should throw NotFoundException when user not found', async () => { // Arrange jest.spyOn(userService, 'findById').mockResolvedValueOnce(null); diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index b5a940bb..e770603f 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -357,7 +357,20 @@ export class AtprotoIdentityController { // take-ownership/complete leaves stale stored credentials whenever that // follow-up never arrives, and stale credentials silently break event // publishing for the account. - await this.recoveryService.completeTakeOwnership(tenantId, user.ulid); + try { + await this.recoveryService.completeTakeOwnership(tenantId, user.ulid); + } catch (error) { + // The PDS reset already succeeded and the token is consumed, so this + // must not surface as a failed reset — an error here would also stop + // the client from making its take-ownership/complete call, which is + // the retry for this exact write. If that retry never comes either, + // the stale credentials produce a definitive 401 on the next session + // attempt and PdsSessionService finishes the handoff from there. + this.logger.error( + `PDS password reset succeeded but ending custody failed for user ${user.ulid}; awaiting client retry or session-401 repair`, + { tenantId, error: error.message }, + ); + } return { success: true }; } diff --git a/src/pds/pds-session.service.spec.ts b/src/pds/pds-session.service.spec.ts index c82b4c6b..6cc8f616 100644 --- a/src/pds/pds-session.service.spec.ts +++ b/src/pds/pds-session.service.spec.ts @@ -9,7 +9,7 @@ import { UserAtprotoIdentityEntity } from '../user-atproto-identity/infrastructu import { BlueskyService } from '../bluesky/bluesky.service'; import { ElastiCacheService } from '../elasticache/elasticache.service'; import { Agent } from '@atproto/api'; -import { SessionUnavailableError } from './pds.errors'; +import { PdsApiError, SessionUnavailableError } from './pds.errors'; // Mock the @atproto/api Agent and CredentialSession const mockResumeSession = jest.fn().mockResolvedValue(undefined); @@ -28,6 +28,7 @@ describe('PdsSessionService', () => { let mockUserAtprotoIdentityService: { findByUserUlid: jest.Mock; findByDid: jest.Mock; + update: jest.Mock; }; let mockPdsCredentialService: { decrypt: jest.Mock; @@ -81,6 +82,7 @@ describe('PdsSessionService', () => { mockUserAtprotoIdentityService = { findByUserUlid: jest.fn(), findByDid: jest.fn(), + update: jest.fn(), }; mockPdsCredentialService = { @@ -370,6 +372,107 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); + expect(result).toBeNull(); + // A plain error is not a definitive PDS rejection - no repair + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + }); + + describe('orphaned take-ownership repair', () => { + const arrangeCustodialSessionAttempt = () => { + const custodialIdentity = createMockIdentity(); + mockUserAtprotoIdentityService.findByUserUlid.mockResolvedValue( + custodialIdentity, + ); + mockElastiCacheService.get.mockResolvedValue(null); + mockPdsCredentialService.decrypt.mockReturnValue(decryptedPassword); + return custodialIdentity; + }; + + it('should finish take-ownership when the PDS definitively rejects stored credentials', async () => { + const custodialIdentity = arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + custodialIdentity, + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError( + 'Invalid identifier or password', + 401, + 'AuthenticationRequired', + ), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + // Session still fails this cycle... + expect(result).toBeNull(); + // ...but the unrecorded handoff is completed + expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + { + pdsCredentials: null, + isCustodial: false, + }, + ); + // ...and the cached session is invalidated + expect(mockElastiCacheService.del).toHaveBeenCalled(); + }); + + it('should not end custody on a non-401 PDS error', async () => { + arrangeCustodialSessionAttempt(); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Bad gateway', 502), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it('should not end custody on a PDS error with no status (network failure)', async () => { + arrangeCustodialSessionAttempt(); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('socket hang up'), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it('should skip repair when the identity is already non-custodial at lookup', async () => { + arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + createMockIdentity({ isCustodial: false, pdsCredentials: null }), + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Invalid identifier or password', 401), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it('should still return null when the repair itself fails', async () => { + const custodialIdentity = arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + custodialIdentity, + ); + mockUserAtprotoIdentityService.update.mockRejectedValue( + new Error('DB write failed'), + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Invalid identifier or password', 401), + ); + + // Must not throw: repair failure cannot mask the session failure + const result = await service.getSessionForUser(tenantId, userUlid); + expect(result).toBeNull(); }); }); diff --git a/src/pds/pds-session.service.ts b/src/pds/pds-session.service.ts index 3c10a668..ab1692be 100644 --- a/src/pds/pds-session.service.ts +++ b/src/pds/pds-session.service.ts @@ -16,7 +16,7 @@ import { import { UserAtprotoIdentityService } from '../user-atproto-identity/user-atproto-identity.service'; import { BlueskyService } from '../bluesky/bluesky.service'; import { ElastiCacheService } from '../elasticache/elasticache.service'; -import { SessionUnavailableError } from './pds.errors'; +import { PdsApiError, SessionUnavailableError } from './pds.errors'; /** * Result of a successful session retrieval. @@ -331,10 +331,24 @@ export class PdsSessionService { ); // Create session with PDS - const session = await this.pdsAccountService.createSession( - identity.did, - password, - ); + let session: CreateSessionResponse; + try { + session = await this.pdsAccountService.createSession( + identity.did, + password, + ); + } catch (error) { + // A definitive 401 means the PDS password no longer matches the + // stored credentials. The only flow that changes a custodial + // account's password is the user's own email-token reset (take- + // ownership step 1), so this is an ownership handoff that never got + // recorded — finish it. Strictly 401 only: network errors and 5xx + // must not end custody. + if (error instanceof PdsApiError && error.statusCode === 401) { + await this.completeOrphanedTakeOwnership(tenantId, identity.did); + } + throw error; + } // Cache the session const cacheKey = this.getCacheKey(tenantId, identity.did); @@ -362,6 +376,46 @@ export class PdsSessionService { } } + /** + * Finish a take-ownership handoff that was never recorded: clear the stale + * credentials, mark the identity non-custodial, and drop any cached + * session. Called when stored custodial credentials are definitively + * rejected by the PDS — those credentials can never work again, so leaving + * them keeps the account failing silently on every publish attempt. + * + * Never throws: repair failure must not mask the original session error. + */ + private async completeOrphanedTakeOwnership( + tenantId: string, + did: string, + ): Promise { + try { + const identity = await this.userAtprotoIdentityService.findByDid( + tenantId, + did, + ); + if (!identity || !identity.isCustodial) { + return; + } + + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + pdsCredentials: null, + isCustodial: false, + }); + await this.invalidateSession(tenantId, did); + + this.logger.warn( + `Completed orphaned take-ownership for DID ${did}: stored credentials were stale, account is now non-custodial`, + { tenantId }, + ); + } catch (repairError) { + this.logger.error( + `Failed to repair orphaned custodial identity for DID ${did}`, + { tenantId, error: repairError.message }, + ); + } + } + /** * Create an Agent instance from a session response. * From cc12cbe2181aa55359fdb12323f8d0abf133dade Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 17:01:40 -0400 Subject: [PATCH 3/6] fix(atproto): gate custody repair on recorded handoff provenance Change made because a PDS login 401 alone cannot prove the user reset their password: the PDS deliberately returns the identical 401 for unknown accounts to prevent enumeration, so a wrong PDS URL, an incomplete PDS restore, or a deleted account would have read as an ownership handoff and converted every touched custodial identity to non-custodial. - new nullable takeOwnershipPendingAt column on userAtprotoIdentities (additive migration, no backfill): written immediately before the reset is submitted to the PDS, withdrawn if the PDS rejects it, resolved by the same write that ends custody - the session-401 repair now requires the marker; identities without recorded provenance are never modified regardless of PDS response - tests: 401 with marker repairs (credential clear + flip + cache invalidation resolve the marker); 401 without marker never writes; the controller records the marker before the PDS call and withdraws it on a failed reset --- .../atproto-identity-recovery.service.spec.ts | 4 ++- .../atproto-identity-recovery.service.ts | 4 ++- .../atproto-identity.controller.spec.ts | 14 +++++++- .../atproto-identity.controller.ts | 21 ++++++++++++ ...TakeOwnershipPendingToAtprotoIdentities.ts | 25 ++++++++++++++ src/pds/pds-session.service.spec.ts | 29 +++++++++++++--- src/pds/pds-session.service.ts | 34 ++++++++++++------- .../entities/user-atproto-identity.entity.ts | 13 +++++++ .../user-atproto-identity.service.ts | 1 + 9 files changed, 125 insertions(+), 20 deletions(-) create mode 100644 src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts diff --git a/src/atproto-identity/atproto-identity-recovery.service.spec.ts b/src/atproto-identity/atproto-identity-recovery.service.spec.ts index cd4feae0..cf620695 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.spec.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.spec.ts @@ -537,13 +537,15 @@ describe('AtprotoIdentityRecoveryService', () => { // Act await service.completeTakeOwnership('test-tenant', mockUser.ulid); - // Assert - credentials cleared and marked as non-custodial + // Assert - credentials cleared, marked non-custodial, pending-handoff + // marker resolved in the same write expect(userAtprotoIdentityService.update).toHaveBeenCalledWith( 'test-tenant', mockIdentityEntity.id, { pdsCredentials: null, isCustodial: false, + takeOwnershipPendingAt: null, }, ); diff --git a/src/atproto-identity/atproto-identity-recovery.service.ts b/src/atproto-identity/atproto-identity-recovery.service.ts index 3bf4fd23..11967071 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.ts @@ -279,10 +279,12 @@ export class AtprotoIdentityRecoveryService { return; } - // Clear credentials and mark as non-custodial + // Clear credentials and mark as non-custodial; the pending-handoff + // marker is resolved by the same write await this.userAtprotoIdentityService.update(tenantId, identity.id, { pdsCredentials: null, isCustodial: false, + takeOwnershipPendingAt: null, }); // The cached session was minted from the credentials we just cleared diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index f142d46d..94744623 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -75,6 +75,7 @@ describe('AtprotoIdentityController', () => { beforeEach(async () => { const mockIdentityService = { findByUserUlid: jest.fn(), + update: jest.fn(), }; const mockAtprotoIdentityService = { @@ -660,6 +661,11 @@ describe('AtprotoIdentityController', () => { 'valid-reset-token', 'new-secure-password-123', ); + // The pending handoff is recorded before the PDS write so repair + // paths have provenance for this identity + expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { + takeOwnershipPendingAt: expect.any(Date), + }); // Custody ends server-side, not via a later client call expect(recoveryService.completeTakeOwnership).toHaveBeenCalledWith( 'test-tenant', @@ -752,8 +758,14 @@ describe('AtprotoIdentityController', () => { }), ).rejects.toThrow(BadRequestException); - // A failed reset must leave custody untouched + // A failed reset must leave custody untouched and withdraw the + // pending-handoff marker it recorded before the PDS call expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); + expect(identityService.update).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + { takeOwnershipPendingAt: null }, + ); }); }); diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index e770603f..2a81bde6 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -342,10 +342,31 @@ export class AtprotoIdentityController { ); } + // Record the pending handoff BEFORE the PDS write. A later PDS login 401 + // is only trustworthy proof of a completed reset for identities carrying + // this marker — the PDS returns the same 401 for unknown accounts, so + // without provenance a systemic failure (wrong PDS URL, incomplete + // restore) would read as mass ownership handoffs. + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipPendingAt: new Date(), + }); + // Call PDS to reset password try { await this.pdsAccountService.resetPassword(dto.token, dto.password); } catch (error) { + // The reset did not happen — withdraw the pending marker (best effort; + // a lingering marker only matters if this identity later draws a 401) + try { + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipPendingAt: null, + }); + } catch (clearError) { + this.logger.warn( + `Failed to clear pending take-ownership marker for user ${user.ulid} after failed reset`, + { tenantId, error: clearError.message }, + ); + } if (error instanceof PdsApiError) { throw new BadRequestException(error.message); } diff --git a/src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts b/src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts new file mode 100644 index 00000000..c06d4048 --- /dev/null +++ b/src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts @@ -0,0 +1,25 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddTakeOwnershipPendingToAtprotoIdentities1786741129455 + implements MigrationInterface +{ + name = 'AddTakeOwnershipPendingToAtprotoIdentities1786741129455'; + + public async up(queryRunner: QueryRunner): Promise { + const schema = queryRunner.connection.options.name || 'public'; + + await queryRunner.query(` + ALTER TABLE "${schema}"."userAtprotoIdentities" + ADD COLUMN IF NOT EXISTS "takeOwnershipPendingAt" TIMESTAMP + `); + } + + public async down(queryRunner: QueryRunner): Promise { + const schema = queryRunner.connection.options.name || 'public'; + + await queryRunner.query(` + ALTER TABLE "${schema}"."userAtprotoIdentities" + DROP COLUMN IF EXISTS "takeOwnershipPendingAt" + `); + } +} diff --git a/src/pds/pds-session.service.spec.ts b/src/pds/pds-session.service.spec.ts index 6cc8f616..4361603a 100644 --- a/src/pds/pds-session.service.spec.ts +++ b/src/pds/pds-session.service.spec.ts @@ -389,10 +389,10 @@ describe('PdsSessionService', () => { return custodialIdentity; }; - it('should finish take-ownership when the PDS definitively rejects stored credentials', async () => { + it('should finish take-ownership on 401 when a pending handoff was recorded', async () => { const custodialIdentity = arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - custodialIdentity, + createMockIdentity({ takeOwnershipPendingAt: new Date() }), ); mockPdsAccountService.createSession.mockRejectedValue( new PdsApiError( @@ -406,19 +406,38 @@ describe('PdsSessionService', () => { // Session still fails this cycle... expect(result).toBeNull(); - // ...but the unrecorded handoff is completed + // ...but the recorded handoff is completed expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( tenantId, custodialIdentity.id, { pdsCredentials: null, isCustodial: false, + takeOwnershipPendingAt: null, }, ); // ...and the cached session is invalidated expect(mockElastiCacheService.del).toHaveBeenCalled(); }); + it('should not end custody on 401 without a recorded pending handoff', async () => { + // The PDS returns the same 401 for unknown accounts (anti- + // enumeration), so a systemic failure like a wrong PDS URL or an + // incomplete PDS restore must never convert custodial identities + arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + createMockIdentity({ takeOwnershipPendingAt: null }), + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Invalid identifier or password', 401), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + it('should not end custody on a non-401 PDS error', async () => { arrangeCustodialSessionAttempt(); mockPdsAccountService.createSession.mockRejectedValue( @@ -459,9 +478,9 @@ describe('PdsSessionService', () => { }); it('should still return null when the repair itself fails', async () => { - const custodialIdentity = arrangeCustodialSessionAttempt(); + arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - custodialIdentity, + createMockIdentity({ takeOwnershipPendingAt: new Date() }), ); mockUserAtprotoIdentityService.update.mockRejectedValue( new Error('DB write failed'), diff --git a/src/pds/pds-session.service.ts b/src/pds/pds-session.service.ts index ab1692be..b9063d46 100644 --- a/src/pds/pds-session.service.ts +++ b/src/pds/pds-session.service.ts @@ -338,12 +338,14 @@ export class PdsSessionService { password, ); } catch (error) { - // A definitive 401 means the PDS password no longer matches the - // stored credentials. The only flow that changes a custodial - // account's password is the user's own email-token reset (take- - // ownership step 1), so this is an ownership handoff that never got - // recorded — finish it. Strictly 401 only: network errors and 5xx - // must not end custody. + // A 401 here MAY mean the user reset the PDS password themselves + // (take-ownership step 1) and custody was never ended. But a 401 + // alone is ambiguous: the PDS returns the identical 401 for unknown + // accounts to prevent enumeration, so wrong PDS URL, incomplete PDS + // restore, or a deleted account all look the same. Only identities + // that recorded a pending handoff before their reset may have + // custody ended here — completeOrphanedTakeOwnership checks the + // marker. Strictly 401 only: network errors and 5xx never qualify. if (error instanceof PdsApiError && error.statusCode === 401) { await this.completeOrphanedTakeOwnership(tenantId, identity.did); } @@ -377,11 +379,14 @@ export class PdsSessionService { } /** - * Finish a take-ownership handoff that was never recorded: clear the stale - * credentials, mark the identity non-custodial, and drop any cached - * session. Called when stored custodial credentials are definitively - * rejected by the PDS — those credentials can never work again, so leaving - * them keeps the account failing silently on every publish attempt. + * Finish a take-ownership handoff that was recorded but never completed: + * clear the stale credentials, mark the identity non-custodial, and drop + * any cached session. + * + * Guarded by the takeOwnershipPendingAt marker, which resetPdsPassword + * writes before submitting the reset to the PDS. Identities without the + * marker are left untouched no matter what the PDS said — a 401 without + * recorded provenance is not evidence of a handoff. * * Never throws: repair failure must not mask the original session error. */ @@ -394,13 +399,18 @@ export class PdsSessionService { tenantId, did, ); - if (!identity || !identity.isCustodial) { + if ( + !identity || + !identity.isCustodial || + !identity.takeOwnershipPendingAt + ) { return; } await this.userAtprotoIdentityService.update(tenantId, identity.id, { pdsCredentials: null, isCustodial: false, + takeOwnershipPendingAt: null, }); await this.invalidateSession(tenantId, did); diff --git a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts index 420684da..3916e449 100644 --- a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts +++ b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts @@ -91,6 +91,19 @@ export class UserAtprotoIdentityEntity extends EntityRelationalHelper { @Column({ type: 'boolean', default: true }) isCustodial: boolean; + /** + * Set immediately before a take-ownership PDS password reset is submitted; + * cleared when the reset fails or custody is ended. + * + * This is the provenance record for repair paths: a PDS login 401 alone is + * ambiguous (the PDS returns the same 401 for unknown accounts to prevent + * enumeration, so wrong PDS URL / incomplete restore / deleted account all + * look like a bad password). Only identities carrying this marker may have + * custody ended in response to a 401. + */ + @Column({ type: 'timestamp', nullable: true }) + takeOwnershipPendingAt: Date | null; + @CreateDateColumn({ type: 'timestamp' }) createdAt: Date; diff --git a/src/user-atproto-identity/user-atproto-identity.service.ts b/src/user-atproto-identity/user-atproto-identity.service.ts index f052858b..04579b92 100644 --- a/src/user-atproto-identity/user-atproto-identity.service.ts +++ b/src/user-atproto-identity/user-atproto-identity.service.ts @@ -142,6 +142,7 @@ export class UserAtprotoIdentityService { pdsUrl: string; pdsCredentials: string | null; isCustodial: boolean; + takeOwnershipPendingAt: Date | null; }>, ): Promise> { await this.getTenantRepository(tenantId); From b04af73dc3017a15ea236834ee65356bc7c74630 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 17:44:25 -0400 Subject: [PATCH 4/6] fix(atproto): distinguish prepared, ambiguous, and confirmed resets Change made because a single pre-call marker conflated intent with evidence: it was withdrawn on every PDS error (including timeouts and 5xx, which do not prove the reset failed), and a crash before the PDS submission left a marker that a later unrelated 401 could satisfy. Replace the timestamp marker with a three-state column: - 'pending' is written before the PDS call and records intent only. The session-401 repair never acts on it. - 'ambiguous' replaces it when the PDS gives no definitive answer (timeout, connection loss, 5xx) - the reset may have committed, so the repair may finish the handoff. The client gets a 502 and can safely retry. - 'confirmed' is written the moment the PDS acknowledges the reset, before the custody flip, so a durable repair-eligible record exists even if the flip then fails. The marker only ever advances toward stronger evidence; rejections never withdraw it, because a rejected retry may have been refused precisely because an earlier attempt consumed the token. Proof removes it instead: the custody flip, or a successful login with the stored credentials, which shows no reset committed and sweeps a stale 'pending'/'ambiguous' marker on the next session use. --- .../atproto-identity-recovery.service.spec.ts | 2 +- .../atproto-identity-recovery.service.ts | 2 +- .../atproto-identity.controller.spec.ts | 129 ++++++++++++++++- .../atproto-identity.controller.ts | 109 +++++++++++--- ...TakeOwnershipStatusToAtprotoIdentities.ts} | 8 +- src/pds/pds-session.service.spec.ts | 136 +++++++++++++++++- src/pds/pds-session.service.ts | 63 ++++++-- .../entities/user-atproto-identity.entity.ts | 37 +++-- .../user-atproto-identity.service.ts | 7 +- 9 files changed, 434 insertions(+), 59 deletions(-) rename src/database/migrations/{1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts => 1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts} (67%) diff --git a/src/atproto-identity/atproto-identity-recovery.service.spec.ts b/src/atproto-identity/atproto-identity-recovery.service.spec.ts index cf620695..29d52006 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.spec.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.spec.ts @@ -545,7 +545,7 @@ describe('AtprotoIdentityRecoveryService', () => { { pdsCredentials: null, isCustodial: false, - takeOwnershipPendingAt: null, + takeOwnershipStatus: null, }, ); diff --git a/src/atproto-identity/atproto-identity-recovery.service.ts b/src/atproto-identity/atproto-identity-recovery.service.ts index 11967071..8b6a6520 100644 --- a/src/atproto-identity/atproto-identity-recovery.service.ts +++ b/src/atproto-identity/atproto-identity-recovery.service.ts @@ -284,7 +284,7 @@ export class AtprotoIdentityRecoveryService { await this.userAtprotoIdentityService.update(tenantId, identity.id, { pdsCredentials: null, isCustodial: false, - takeOwnershipPendingAt: null, + takeOwnershipStatus: null, }); // The cached session was minted from the credentials we just cleared diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index 94744623..7cda5c2f 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -3,6 +3,7 @@ import { ConflictException, NotFoundException, BadRequestException, + BadGatewayException, } from '@nestjs/common'; import { AtprotoIdentityController } from './atproto-identity.controller'; import { AtprotoIdentityService } from './atproto-identity.service'; @@ -39,6 +40,7 @@ describe('AtprotoIdentityController', () => { pdsUrl: 'https://pds.openmeet.net', pdsCredentials: 'encrypted-credentials-should-not-be-exposed', isCustodial: true, + takeOwnershipStatus: null, createdAt: new Date('2025-01-01T00:00:00Z'), updatedAt: new Date('2025-01-01T00:00:00Z'), }; @@ -662,9 +664,13 @@ describe('AtprotoIdentityController', () => { 'new-secure-password-123', ); // The pending handoff is recorded before the PDS write so repair - // paths have provenance for this identity + // paths have provenance for this identity... expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { - takeOwnershipPendingAt: expect.any(Date), + takeOwnershipStatus: 'pending', + }); + // ...and upgraded to 'confirmed' once the PDS acknowledges the reset + expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { + takeOwnershipStatus: 'confirmed', }); // Custody ends server-side, not via a later client call expect(recoveryService.completeTakeOwnership).toHaveBeenCalledWith( @@ -694,6 +700,13 @@ describe('AtprotoIdentityController', () => { // response must not read as a failed reset; the client's follow-up // take-ownership/complete call is the retry for the custody flip expect(result).toEqual({ success: true }); + // The 'confirmed' marker survives so the session-401 repair can also + // finish the handoff if the client never completes + expect(identityService.update).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + { takeOwnershipStatus: 'confirmed' }, + ); }); it('should throw NotFoundException when user not found', async () => { @@ -739,7 +752,7 @@ describe('AtprotoIdentityController', () => { ).rejects.toThrow(BadRequestException); }); - it('should throw BadRequestException when PDS returns PdsApiError', async () => { + it('should throw BadRequestException when the PDS definitively rejects the reset', async () => { // Arrange jest .spyOn(identityService, 'findByUserUlid') @@ -758,15 +771,119 @@ describe('AtprotoIdentityController', () => { }), ).rejects.toThrow(BadRequestException); - // A failed reset must leave custody untouched and withdraw the - // pending-handoff marker it recorded before the PDS call + // A definitively rejected reset leaves custody untouched. The marker + // stays 'pending' (never withdrawn, never upgraded): 'pending' is + // inert for the 401 repair and a later successful login sweeps it expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); + expect(identityService.update).toHaveBeenCalledTimes(1); + expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { + takeOwnershipStatus: 'pending', + }); + }); + + it("should mark the reset 'ambiguous' and throw BadGatewayException when the PDS gives no definitive answer", async () => { + // Arrange - a 502 from the PDS does not prove the reset failed; it + // may have committed with the response lost + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue(mockIdentityEntity as UserAtprotoIdentityEntity); + jest + .spyOn(pdsAccountService, 'resetPassword') + .mockRejectedValue(new PdsApiError('Bad gateway', 502)); + + // Act & Assert + await expect( + controller.resetPdsPassword(mockRequest, { + token: 'valid-reset-token', + password: 'new-secure-password-123', + }), + ).rejects.toThrow(BadGatewayException); + + expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); + expect(identityService.update).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + { takeOwnershipStatus: 'ambiguous' }, + ); + }); + + it("should mark the reset 'ambiguous' on a network failure with no PDS response", async () => { + // Arrange - no statusCode at all: the request may never have arrived, + // or the response was lost after the PDS committed the reset + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue(mockIdentityEntity as UserAtprotoIdentityEntity); + jest + .spyOn(pdsAccountService, 'resetPassword') + .mockRejectedValue(new PdsApiError('socket hang up')); + + // Act & Assert + await expect( + controller.resetPdsPassword(mockRequest, { + token: 'valid-reset-token', + password: 'new-secure-password-123', + }), + ).rejects.toThrow(BadGatewayException); + expect(identityService.update).toHaveBeenLastCalledWith( 'test-tenant', 1, - { takeOwnershipPendingAt: null }, + { takeOwnershipStatus: 'ambiguous' }, ); }); + + it("should not downgrade an existing 'ambiguous' marker when a retry is definitively rejected", async () => { + // Arrange - an earlier attempt ended ambiguous; this retry's token is + // rejected, which proves nothing about the earlier attempt (it may + // have been rejected precisely because that attempt consumed it) + jest.spyOn(identityService, 'findByUserUlid').mockResolvedValue({ + ...mockIdentityEntity, + takeOwnershipStatus: 'ambiguous', + } as UserAtprotoIdentityEntity); + jest + .spyOn(pdsAccountService, 'resetPassword') + .mockRejectedValue( + new PdsApiError('Token has expired', 400, 'ExpiredToken'), + ); + + // Act & Assert + await expect( + controller.resetPdsPassword(mockRequest, { + token: 'expired-token', + password: 'new-secure-password-123', + }), + ).rejects.toThrow(BadRequestException); + + // No marker writes at all: 'ambiguous' must survive the rejection + expect(identityService.update).not.toHaveBeenCalled(); + }); + + it("should upgrade an existing 'ambiguous' marker to 'confirmed' when a retry succeeds", async () => { + // Arrange - the retry succeeding proves the earlier ambiguous attempt + // never consumed the token; this reset is the one that committed + jest.spyOn(identityService, 'findByUserUlid').mockResolvedValue({ + ...mockIdentityEntity, + takeOwnershipStatus: 'ambiguous', + } as UserAtprotoIdentityEntity); + jest.spyOn(pdsAccountService, 'resetPassword').mockResolvedValue(); + jest + .spyOn(recoveryService, 'completeTakeOwnership') + .mockResolvedValue(undefined); + + // Act + const result = await controller.resetPdsPassword(mockRequest, { + token: 'valid-reset-token', + password: 'new-secure-password-123', + }); + + // Assert + expect(result).toEqual({ success: true }); + expect(identityService.update).toHaveBeenCalledTimes(1); + expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { + takeOwnershipStatus: 'confirmed', + }); + expect(recoveryService.completeTakeOwnership).toHaveBeenCalled(); + }); }); describe('hasActiveSession', () => { diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index 2a81bde6..5f2fe198 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -11,6 +11,7 @@ import { UseGuards, NotFoundException, BadRequestException, + BadGatewayException, } from '@nestjs/common'; import { ApiBearerAuth, @@ -310,6 +311,11 @@ export class AtprotoIdentityController { status: HttpStatus.BAD_REQUEST, description: 'User has no custodial identity or invalid token', }) + @ApiResponse({ + status: HttpStatus.BAD_GATEWAY, + description: + 'The PDS did not confirm the reset (timeout or server error); safe to retry', + }) @ApiResponse({ status: HttpStatus.TOO_MANY_REQUESTS, description: 'Rate limit exceeded - max 3 reset attempts per hour', @@ -343,34 +349,80 @@ export class AtprotoIdentityController { } // Record the pending handoff BEFORE the PDS write. A later PDS login 401 - // is only trustworthy proof of a completed reset for identities carrying - // this marker — the PDS returns the same 401 for unknown accounts, so - // without provenance a systemic failure (wrong PDS URL, incomplete - // restore) would read as mass ownership handoffs. - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipPendingAt: new Date(), - }); + // is only trustworthy proof of a completed reset for identities whose + // marker shows the PDS actually saw a reset ('ambiguous' or 'confirmed') + // — the PDS returns the same 401 for unknown accounts, so without + // provenance a systemic failure (wrong PDS URL, incomplete restore) + // would read as mass ownership handoffs. + // + // The marker only ever advances toward stronger evidence here (pending + // -> ambiguous -> confirmed); rejections never withdraw it, because a + // rejected retry may have been refused precisely because an earlier + // attempt consumed the token when it committed. Only proof removes the + // marker: the custody flip, or a later successful login with the stored + // credentials (which shows no reset committed — PdsSessionService + // clears 'pending'/'ambiguous' then). + const priorStatus = identity.takeOwnershipStatus; + if (priorStatus === 'ambiguous' || priorStatus === 'confirmed') { + this.logger.warn( + `Password reset requested for user ${user.ulid} while a prior reset attempt is still '${priorStatus}'; keeping that marker`, + { tenantId }, + ); + } else { + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipStatus: 'pending', + }); + } // Call PDS to reset password try { await this.pdsAccountService.resetPassword(dto.token, dto.password); } catch (error) { - // The reset did not happen — withdraw the pending marker (best effort; - // a lingering marker only matters if this identity later draws a 401) - try { - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipPendingAt: null, - }); - } catch (clearError) { - this.logger.warn( - `Failed to clear pending take-ownership marker for user ${user.ulid} after failed reset`, - { tenantId, error: clearError.message }, - ); - } - if (error instanceof PdsApiError) { + if (this.isDefinitivePdsRejection(error)) { + // The PDS evaluated this request and refused it, so THIS attempt + // did not change the password. A marker this request wrote stays + // 'pending', which the session-401 repair ignores and a later + // successful login sweeps away. throw new BadRequestException(error.message); } - throw error; + + // No definitive answer from the PDS (timeout, connection loss, 5xx): + // the reset may have committed with the response lost. Upgrade the + // marker so the session-401 repair may finish the handoff if it did. + if (priorStatus !== 'confirmed') { + try { + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipStatus: 'ambiguous', + }); + } catch (markError) { + this.logger.error( + `Failed to mark ambiguous password reset for user ${user.ulid}; marker stays 'pending' and needs manual triage if the reset committed`, + { tenantId, error: markError.message }, + ); + } + } + this.logger.error( + `PDS gave no definitive answer to a password reset for user ${user.ulid}`, + { tenantId, error: error.message }, + ); + throw new BadGatewayException( + 'The PDS did not confirm the password reset. Please try again.', + ); + } + + // The PDS acknowledged the reset — record that durably FIRST. If ending + // custody below fails, the 'confirmed' marker is what lets the client's + // take-ownership/complete retry or the session-401 repair finish the + // handoff later. + try { + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipStatus: 'confirmed', + }); + } catch (markError) { + this.logger.error( + `Failed to record confirmed password reset for user ${user.ulid}`, + { tenantId, error: markError.message }, + ); } // The user now owns the password they just set, so custody ends here, in @@ -396,6 +448,21 @@ export class AtprotoIdentityController { return { success: true }; } + /** + * True when the PDS itself evaluated the request and refused it (any 4xx + * response, e.g. an invalid or expired token) — proof that this attempt + * changed nothing. Network failures, timeouts, and 5xx responses are NOT + * definitive: the reset may have committed with the response lost. + */ + private isDefinitivePdsRejection(error: unknown): error is PdsApiError { + return ( + error instanceof PdsApiError && + typeof error.statusCode === 'number' && + error.statusCode >= 400 && + error.statusCode < 500 + ); + } + /** * Disconnect (logout) the user's AT Protocol session. * diff --git a/src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts b/src/database/migrations/1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts similarity index 67% rename from src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts rename to src/database/migrations/1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts index c06d4048..06ee1b58 100644 --- a/src/database/migrations/1786741129455-AddTakeOwnershipPendingToAtprotoIdentities.ts +++ b/src/database/migrations/1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts @@ -1,16 +1,16 @@ import { MigrationInterface, QueryRunner } from 'typeorm'; -export class AddTakeOwnershipPendingToAtprotoIdentities1786741129455 +export class AddTakeOwnershipStatusToAtprotoIdentities1786741129455 implements MigrationInterface { - name = 'AddTakeOwnershipPendingToAtprotoIdentities1786741129455'; + name = 'AddTakeOwnershipStatusToAtprotoIdentities1786741129455'; public async up(queryRunner: QueryRunner): Promise { const schema = queryRunner.connection.options.name || 'public'; await queryRunner.query(` ALTER TABLE "${schema}"."userAtprotoIdentities" - ADD COLUMN IF NOT EXISTS "takeOwnershipPendingAt" TIMESTAMP + ADD COLUMN IF NOT EXISTS "takeOwnershipStatus" character varying(16) `); } @@ -19,7 +19,7 @@ export class AddTakeOwnershipPendingToAtprotoIdentities1786741129455 await queryRunner.query(` ALTER TABLE "${schema}"."userAtprotoIdentities" - DROP COLUMN IF EXISTS "takeOwnershipPendingAt" + DROP COLUMN IF EXISTS "takeOwnershipStatus" `); } } diff --git a/src/pds/pds-session.service.spec.ts b/src/pds/pds-session.service.spec.ts index 4361603a..9145ac7e 100644 --- a/src/pds/pds-session.service.spec.ts +++ b/src/pds/pds-session.service.spec.ts @@ -69,6 +69,7 @@ describe('PdsSessionService', () => { pdsUrl: testPdsUrl, pdsCredentials: encryptedCredentials, isCustodial: true, + takeOwnershipStatus: null, createdAt: new Date(), updatedAt: new Date(), ...overrides, @@ -389,10 +390,10 @@ describe('PdsSessionService', () => { return custodialIdentity; }; - it('should finish take-ownership on 401 when a pending handoff was recorded', async () => { + it("should finish take-ownership on 401 when a reset's outcome was ambiguous", async () => { const custodialIdentity = arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipPendingAt: new Date() }), + createMockIdentity({ takeOwnershipStatus: 'ambiguous' }), ); mockPdsAccountService.createSession.mockRejectedValue( new PdsApiError( @@ -413,20 +414,61 @@ describe('PdsSessionService', () => { { pdsCredentials: null, isCustodial: false, - takeOwnershipPendingAt: null, + takeOwnershipStatus: null, }, ); // ...and the cached session is invalidated expect(mockElastiCacheService.del).toHaveBeenCalled(); }); - it('should not end custody on 401 without a recorded pending handoff', async () => { + it('should finish take-ownership on 401 when a reset was confirmed but custody never flipped', async () => { + const custodialIdentity = arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + createMockIdentity({ takeOwnershipStatus: 'confirmed' }), + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Invalid identifier or password', 401), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + { + pdsCredentials: null, + isCustodial: false, + takeOwnershipStatus: null, + }, + ); + }); + + it('should not end custody on 401 without a recorded marker', async () => { // The PDS returns the same 401 for unknown accounts (anti- // enumeration), so a systemic failure like a wrong PDS URL or an // incomplete PDS restore must never convert custodial identities arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipPendingAt: null }), + createMockIdentity({ takeOwnershipStatus: null }), + ); + mockPdsAccountService.createSession.mockRejectedValue( + new PdsApiError('Invalid identifier or password', 401), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it("should not end custody on 401 when the marker is only 'pending'", async () => { + // 'pending' records intent before the PDS submission — a crash in + // that window leaves it behind without any reset having happened, + // so it must never satisfy the repair on its own + arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + createMockIdentity({ takeOwnershipStatus: 'pending' }), ); mockPdsAccountService.createSession.mockRejectedValue( new PdsApiError('Invalid identifier or password', 401), @@ -480,7 +522,7 @@ describe('PdsSessionService', () => { it('should still return null when the repair itself fails', async () => { arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipPendingAt: new Date() }), + createMockIdentity({ takeOwnershipStatus: 'ambiguous' }), ); mockUserAtprotoIdentityService.update.mockRejectedValue( new Error('DB write failed'), @@ -495,6 +537,88 @@ describe('PdsSessionService', () => { expect(result).toBeNull(); }); }); + + describe('stale marker sweep on successful login', () => { + const sessionResponse = { + did: testDid, + handle: testHandle, + accessJwt: 'fresh-access-jwt', + refreshJwt: 'fresh-refresh-jwt', + }; + + const arrangeSuccessfulFreshSession = ( + takeOwnershipStatus: 'pending' | 'ambiguous' | 'confirmed' | null, + ) => { + const custodialIdentity = createMockIdentity({ takeOwnershipStatus }); + mockUserAtprotoIdentityService.findByUserUlid.mockResolvedValue( + custodialIdentity, + ); + mockElastiCacheService.get.mockResolvedValue(null); + mockPdsCredentialService.decrypt.mockReturnValue(decryptedPassword); + mockPdsAccountService.createSession.mockResolvedValue(sessionResponse); + return custodialIdentity; + }; + + it("should clear a 'pending' marker when the stored credentials still authenticate", async () => { + // A successful login proves no reset committed, so a marker stranded + // by a crash before the PDS submission is swept instead of lingering + const custodialIdentity = arrangeSuccessfulFreshSession('pending'); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + { takeOwnershipStatus: null }, + ); + }); + + it("should clear an 'ambiguous' marker when the stored credentials still authenticate", async () => { + const custodialIdentity = arrangeSuccessfulFreshSession('ambiguous'); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + { takeOwnershipStatus: null }, + ); + }); + + it("should NOT clear a 'confirmed' marker even when the stored credentials authenticate", async () => { + // 'confirmed' means the PDS acknowledged a reset; old credentials + // still working is an anomaly (e.g. PDS restore), not refutation + arrangeSuccessfulFreshSession('confirmed'); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it('should not touch identities without a marker', async () => { + arrangeSuccessfulFreshSession(null); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + }); + + it('should still return the session when the sweep write fails', async () => { + arrangeSuccessfulFreshSession('pending'); + mockUserAtprotoIdentityService.update.mockRejectedValue( + new Error('DB write failed'), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(result!.source).toBe('fresh'); + }); + }); }); describe('getSessionForDid()', () => { diff --git a/src/pds/pds-session.service.ts b/src/pds/pds-session.service.ts index b9063d46..5f67ce85 100644 --- a/src/pds/pds-session.service.ts +++ b/src/pds/pds-session.service.ts @@ -14,6 +14,7 @@ import { CreateSessionResponse, } from './pds-account.service'; import { UserAtprotoIdentityService } from '../user-atproto-identity/user-atproto-identity.service'; +import { TakeOwnershipStatus } from '../user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity'; import { BlueskyService } from '../bluesky/bluesky.service'; import { ElastiCacheService } from '../elasticache/elasticache.service'; import { PdsApiError, SessionUnavailableError } from './pds.errors'; @@ -207,11 +208,13 @@ export class PdsSessionService { private async getSessionForIdentity( tenantId: string, identity: { + id: number; did: string; handle: string | null; pdsUrl: string; pdsCredentials: string | null; isCustodial: boolean; + takeOwnershipStatus: TakeOwnershipStatus | null; }, ): Promise { // Case 1: OAuth user (non-custodial) - delegate to BlueskyService @@ -230,10 +233,12 @@ export class PdsSessionService { // Case 3: Custodial account with credentials // TypeScript knows pdsCredentials is not null at this point return this.handleCustodialSession(tenantId, { + id: identity.id, did: identity.did, handle: identity.handle, pdsUrl: identity.pdsUrl, pdsCredentials: identity.pdsCredentials, + takeOwnershipStatus: identity.takeOwnershipStatus, }); } @@ -273,10 +278,12 @@ export class PdsSessionService { private async handleCustodialSession( tenantId: string, identity: { + id: number; did: string; handle: string | null; pdsUrl: string; pdsCredentials: string; + takeOwnershipStatus: TakeOwnershipStatus | null; }, ): Promise { const cacheKey = this.getCacheKey(tenantId, identity.did); @@ -318,10 +325,12 @@ export class PdsSessionService { private async createFreshCustodialSession( tenantId: string, identity: { + id: number; did: string; handle: string | null; pdsUrl: string; pdsCredentials: string; + takeOwnershipStatus: TakeOwnershipStatus | null; }, ): Promise { try { @@ -343,15 +352,47 @@ export class PdsSessionService { // alone is ambiguous: the PDS returns the identical 401 for unknown // accounts to prevent enumeration, so wrong PDS URL, incomplete PDS // restore, or a deleted account all look the same. Only identities - // that recorded a pending handoff before their reset may have - // custody ended here — completeOrphanedTakeOwnership checks the - // marker. Strictly 401 only: network errors and 5xx never qualify. + // whose marker shows the PDS actually saw a reset may have custody + // ended here — completeOrphanedTakeOwnership checks the marker. + // Strictly 401 only: network errors and 5xx never qualify. if (error instanceof PdsApiError && error.statusCode === 401) { await this.completeOrphanedTakeOwnership(tenantId, identity.did); } throw error; } + // The stored credentials just authenticated, which is proof that no + // password reset committed. Sweep away a leftover marker from a reset + // attempt that was prepared but never confirmed ('pending') or whose + // outcome was lost ('ambiguous') — without this, a stale marker would + // keep the identity eligible for the 401 repair indefinitely. + // 'confirmed' is NOT swept: the PDS acknowledged that reset, so a + // still-working old password is an anomaly to investigate, not proof. + if ( + identity.takeOwnershipStatus === 'pending' || + identity.takeOwnershipStatus === 'ambiguous' + ) { + try { + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + takeOwnershipStatus: null, + }); + this.logger.log( + `Cleared stale '${identity.takeOwnershipStatus}' take-ownership marker for DID ${identity.did}: stored credentials still authenticate`, + { tenantId }, + ); + } catch (sweepError) { + this.logger.warn( + `Failed to clear stale take-ownership marker for DID ${identity.did}`, + { tenantId, error: sweepError.message }, + ); + } + } else if (identity.takeOwnershipStatus === 'confirmed') { + this.logger.error( + `Stored credentials for DID ${identity.did} still authenticate despite a 'confirmed' password reset — possible PDS restore from backup; leaving the marker`, + { tenantId }, + ); + } + // Cache the session const cacheKey = this.getCacheKey(tenantId, identity.did); await this.elastiCacheService.set( @@ -383,10 +424,13 @@ export class PdsSessionService { * clear the stale credentials, mark the identity non-custodial, and drop * any cached session. * - * Guarded by the takeOwnershipPendingAt marker, which resetPdsPassword - * writes before submitting the reset to the PDS. Identities without the - * marker are left untouched no matter what the PDS said — a 401 without - * recorded provenance is not evidence of a handoff. + * Guarded by the takeOwnershipStatus marker: only 'ambiguous' (the PDS + * gave no definitive answer to a reset) or 'confirmed' (the PDS + * acknowledged a reset but custody was never flipped) qualify. A bare + * 'pending' records intent only — the reset may never have reached the + * PDS, so a 401 against it proves nothing. Identities without a + * qualifying marker are left untouched no matter what the PDS said — a + * 401 without recorded provenance is not evidence of a handoff. * * Never throws: repair failure must not mask the original session error. */ @@ -402,7 +446,8 @@ export class PdsSessionService { if ( !identity || !identity.isCustodial || - !identity.takeOwnershipPendingAt + (identity.takeOwnershipStatus !== 'ambiguous' && + identity.takeOwnershipStatus !== 'confirmed') ) { return; } @@ -410,7 +455,7 @@ export class PdsSessionService { await this.userAtprotoIdentityService.update(tenantId, identity.id, { pdsCredentials: null, isCustodial: false, - takeOwnershipPendingAt: null, + takeOwnershipStatus: null, }); await this.invalidateSession(tenantId, did); diff --git a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts index 3916e449..c84e0336 100644 --- a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts +++ b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts @@ -11,6 +11,12 @@ import { import { EntityRelationalHelper } from '../../../../../utils/relational-entity-helper'; import { UserEntity } from '../../../../../user/infrastructure/persistence/relational/entities/user.entity'; +/** + * States of the take-ownership password-reset marker. See the + * takeOwnershipStatus column doc for the meaning of each state. + */ +export type TakeOwnershipStatus = 'pending' | 'ambiguous' | 'confirmed'; + /** * Entity representing a user's AT Protocol identity. * @@ -92,17 +98,30 @@ export class UserAtprotoIdentityEntity extends EntityRelationalHelper { isCustodial: boolean; /** - * Set immediately before a take-ownership PDS password reset is submitted; - * cleared when the reset fails or custody is ended. + * Provenance record for the take-ownership password reset, written before + * the reset is submitted to the PDS and advanced as evidence accumulates: + * + * - 'pending' — a reset request was prepared; the PDS may never have + * received it. Records intent only, NOT proof of a reset. + * - 'ambiguous' — the PDS gave no definitive answer (timeout, 5xx); the + * reset may have committed with the response lost. + * - 'confirmed' — the PDS acknowledged the reset but ending custody in the + * same request failed; custody must still be ended. + * + * A PDS login 401 alone is ambiguous (the PDS returns the same 401 for + * unknown accounts to prevent enumeration, so wrong PDS URL / incomplete + * restore / deleted account all look like a bad password). Custody may be + * ended in response to a 401 only for 'ambiguous' or 'confirmed' — never + * for 'pending', which a crash before the PDS submission can strand. * - * This is the provenance record for repair paths: a PDS login 401 alone is - * ambiguous (the PDS returns the same 401 for unknown accounts to prevent - * enumeration, so wrong PDS URL / incomplete restore / deleted account all - * look like a bad password). Only identities carrying this marker may have - * custody ended in response to a 401. + * The marker is only ever removed by proof: the custody flip itself, or a + * successful login with the stored credentials (which shows no reset + * committed) clearing 'pending'/'ambiguous'. Rejection responses do not + * clear it — a rejected retry cannot vouch for an earlier attempt whose + * token it may itself have consumed. */ - @Column({ type: 'timestamp', nullable: true }) - takeOwnershipPendingAt: Date | null; + @Column({ type: 'varchar', length: 16, nullable: true }) + takeOwnershipStatus: TakeOwnershipStatus | null; @CreateDateColumn({ type: 'timestamp' }) createdAt: Date; diff --git a/src/user-atproto-identity/user-atproto-identity.service.ts b/src/user-atproto-identity/user-atproto-identity.service.ts index 04579b92..1a1ca071 100644 --- a/src/user-atproto-identity/user-atproto-identity.service.ts +++ b/src/user-atproto-identity/user-atproto-identity.service.ts @@ -2,7 +2,10 @@ import { Injectable, Inject, Scope } from '@nestjs/common'; import { Repository } from 'typeorm'; import { REQUEST } from '@nestjs/core'; import { TenantConnectionService } from '../tenant/tenant.service'; -import { UserAtprotoIdentityEntity } from './infrastructure/persistence/relational/entities/user-atproto-identity.entity'; +import { + TakeOwnershipStatus, + UserAtprotoIdentityEntity, +} from './infrastructure/persistence/relational/entities/user-atproto-identity.entity'; import { NullableType } from '../utils/types/nullable.type'; /** @@ -142,7 +145,7 @@ export class UserAtprotoIdentityService { pdsUrl: string; pdsCredentials: string | null; isCustodial: boolean; - takeOwnershipPendingAt: Date | null; + takeOwnershipStatus: TakeOwnershipStatus | null; }>, ): Promise> { await this.getTenantRepository(tenantId); From 1153fb1bd4b456efa0a6265d80812cb256460cb9 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 17:44:33 -0400 Subject: [PATCH 5/6] test(e2e): page through the dashboard when locating the test event Change made because this test assumed its event lands on page 1 of the dashboard, but every e2e suite logs in as the same admin user and the dashboard sorts by startDate ascending with a default page size of 10. The test's far-future (2099) event sorts last, so once concurrently running suites have created ten earlier events the lookup missed it and the test failed without any product defect. Walk the dashboard pages until the event is found or the listing is exhausted, which is deterministic regardless of what parallel suites have created. --- .../events-attendees.e2e-spec.ts | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/test/event-attendees/events-attendees.e2e-spec.ts b/test/event-attendees/events-attendees.e2e-spec.ts index a7b3b657..2ed7a73c 100644 --- a/test/event-attendees/events-attendees.e2e-spec.ts +++ b/test/event-attendees/events-attendees.e2e-spec.ts @@ -103,18 +103,28 @@ describe('EventAttendeeController (e2e)', () => { // First attend the event to make sure we have an attendance record await attendEvent(token, testEvent.slug); - const getMyEventsResponse = await request(TESTING_APP_URL) - .get('/api/events/dashboard') - .set('Authorization', `Bearer ${token}`) - .set('x-tenant-id', TESTING_TENANT_ID); + // Every e2e suite logs in as the same admin user, so the dashboard also + // contains events created by concurrently running suites — and it sorts + // by startDate ascending, which puts this test's far-future event last. + // Page through instead of assuming the event lands on page 1. + let foundEvent: { slug: string } | undefined; + for (let page = 1; page <= 50 && !foundEvent; page++) { + const getMyEventsResponse = await request(TESTING_APP_URL) + .get('/api/events/dashboard') + .query({ page, limit: 50 }) + .set('Authorization', `Bearer ${token}`) + .set('x-tenant-id', TESTING_TENANT_ID); - expect(getMyEventsResponse.status).toBe(200); - expect(Array.isArray(getMyEventsResponse.body.data)).toBe(true); + expect(getMyEventsResponse.status).toBe(200); + expect(Array.isArray(getMyEventsResponse.body.data)).toBe(true); - // Find the created test event in the dashboard response - const foundEvent = getMyEventsResponse.body.data.find( - (e: { slug: string }) => e.slug === testEvent.slug, - ); + foundEvent = getMyEventsResponse.body.data.find( + (e: { slug: string }) => e.slug === testEvent.slug, + ); + if (getMyEventsResponse.body.data.length === 0) { + break; + } + } expect(foundEvent).toBeDefined(); }); From e4203e95ab570544a059a5b778a59dcc75c16cb4 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Fri, 14 Aug 2026 19:13:18 -0400 Subject: [PATCH 6/6] fix(atproto): make marker transitions compare-and-set, repair on confirmed only Change made because unconditional marker writes let stale readers overwrite stronger states: a session attempt that read 'pending' could authenticate before the reset committed and then clear the marker after the controller wrote 'confirmed', losing the repair provenance exactly when it was needed; concurrent reset requests could similarly downgrade 'confirmed' to 'ambiguous'. And 'ambiguous' explicitly includes "the request never reached the PDS", so it could not safely authorize the destructive 401 repair - a failed connection followed by a systemic 401 (wrong PDS URL, incomplete restore) would still falsely convert that identity. - All marker state changes go through a compare-and-set transition (UPDATE ... WHERE the row still holds one of the expected states and is still custodial). Stale writers lose instead of overwriting: the sweep clears exactly the state it observed, 'ambiguous' can never replace 'confirmed', and markers are never written to identities whose custody already flipped. The generic update() now only accepts clearing the marker as part of a custody flip. - The automatic session-401 repair now requires 'confirmed' - the PDS acknowledged the reset, so the handoff is certain. An 'ambiguous' identity drawing a 401 is logged loudly for reconciliation instead of being converted; the client's 502-retry path remains the promotion route to 'confirmed', and a successful login still sweeps stale 'pending'/'ambiguous' markers. --- .../atproto-identity.controller.spec.ts | 83 ++++++++++++++----- .../atproto-identity.controller.ts | 79 ++++++++++-------- src/pds/pds-session.service.spec.ts | 66 ++++++++++----- src/pds/pds-session.service.ts | 73 ++++++++++------ .../entities/user-atproto-identity.entity.ts | 23 +++-- .../user-atproto-identity.service.ts | 58 ++++++++++++- 6 files changed, 268 insertions(+), 114 deletions(-) diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index 7cda5c2f..8c9ed035 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -78,6 +78,7 @@ describe('AtprotoIdentityController', () => { const mockIdentityService = { findByUserUlid: jest.fn(), update: jest.fn(), + transitionTakeOwnershipStatus: jest.fn().mockResolvedValue(true), }; const mockAtprotoIdentityService = { @@ -665,13 +666,18 @@ describe('AtprotoIdentityController', () => { ); // The pending handoff is recorded before the PDS write so repair // paths have provenance for this identity... - expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { - takeOwnershipStatus: 'pending', - }); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith('test-tenant', 1, [null, 'pending'], 'pending'); // ...and upgraded to 'confirmed' once the PDS acknowledges the reset - expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { - takeOwnershipStatus: 'confirmed', - }); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith( + 'test-tenant', + 1, + [null, 'pending', 'ambiguous', 'confirmed'], + 'confirmed', + ); // Custody ends server-side, not via a later client call expect(recoveryService.completeTakeOwnership).toHaveBeenCalledWith( 'test-tenant', @@ -702,10 +708,13 @@ describe('AtprotoIdentityController', () => { expect(result).toEqual({ success: true }); // The 'confirmed' marker survives so the session-401 repair can also // finish the handoff if the client never completes - expect(identityService.update).toHaveBeenLastCalledWith( + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( 'test-tenant', 1, - { takeOwnershipStatus: 'confirmed' }, + [null, 'pending', 'ambiguous', 'confirmed'], + 'confirmed', ); }); @@ -775,10 +784,12 @@ describe('AtprotoIdentityController', () => { // stays 'pending' (never withdrawn, never upgraded): 'pending' is // inert for the 401 repair and a later successful login sweeps it expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); - expect(identityService.update).toHaveBeenCalledTimes(1); - expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { - takeOwnershipStatus: 'pending', - }); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledTimes(1); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith('test-tenant', 1, [null, 'pending'], 'pending'); }); it("should mark the reset 'ambiguous' and throw BadGatewayException when the PDS gives no definitive answer", async () => { @@ -800,10 +811,13 @@ describe('AtprotoIdentityController', () => { ).rejects.toThrow(BadGatewayException); expect(recoveryService.completeTakeOwnership).not.toHaveBeenCalled(); - expect(identityService.update).toHaveBeenLastCalledWith( + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( 'test-tenant', 1, - { takeOwnershipStatus: 'ambiguous' }, + [null, 'pending'], + 'ambiguous', ); }); @@ -825,21 +839,29 @@ describe('AtprotoIdentityController', () => { }), ).rejects.toThrow(BadGatewayException); - expect(identityService.update).toHaveBeenLastCalledWith( + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( 'test-tenant', 1, - { takeOwnershipStatus: 'ambiguous' }, + [null, 'pending'], + 'ambiguous', ); }); it("should not downgrade an existing 'ambiguous' marker when a retry is definitively rejected", async () => { // Arrange - an earlier attempt ended ambiguous; this retry's token is // rejected, which proves nothing about the earlier attempt (it may - // have been rejected precisely because that attempt consumed it) + // have been rejected precisely because that attempt consumed it). + // The compare-and-set refuses the 'pending' write against the + // stronger stored state jest.spyOn(identityService, 'findByUserUlid').mockResolvedValue({ ...mockIdentityEntity, takeOwnershipStatus: 'ambiguous', } as UserAtprotoIdentityEntity); + jest + .spyOn(identityService, 'transitionTakeOwnershipStatus') + .mockResolvedValue(false); jest .spyOn(pdsAccountService, 'resetPassword') .mockRejectedValue( @@ -854,7 +876,14 @@ describe('AtprotoIdentityController', () => { }), ).rejects.toThrow(BadRequestException); - // No marker writes at all: 'ambiguous' must survive the rejection + // Only the pending transition was attempted, and its expected-state + // list can never overwrite 'ambiguous' or 'confirmed' + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledTimes(1); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith('test-tenant', 1, [null, 'pending'], 'pending'); expect(identityService.update).not.toHaveBeenCalled(); }); @@ -865,6 +894,12 @@ describe('AtprotoIdentityController', () => { ...mockIdentityEntity, takeOwnershipStatus: 'ambiguous', } as UserAtprotoIdentityEntity); + // The 'pending' transition loses against the stored 'ambiguous'; the + // 'confirmed' transition applies from any state + jest + .spyOn(identityService, 'transitionTakeOwnershipStatus') + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true); jest.spyOn(pdsAccountService, 'resetPassword').mockResolvedValue(); jest .spyOn(recoveryService, 'completeTakeOwnership') @@ -878,10 +913,14 @@ describe('AtprotoIdentityController', () => { // Assert expect(result).toEqual({ success: true }); - expect(identityService.update).toHaveBeenCalledTimes(1); - expect(identityService.update).toHaveBeenCalledWith('test-tenant', 1, { - takeOwnershipStatus: 'confirmed', - }); + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + [null, 'pending', 'ambiguous', 'confirmed'], + 'confirmed', + ); expect(recoveryService.completeTakeOwnership).toHaveBeenCalled(); }); }); diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index 5f2fe198..264cd503 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -350,28 +350,31 @@ export class AtprotoIdentityController { // Record the pending handoff BEFORE the PDS write. A later PDS login 401 // is only trustworthy proof of a completed reset for identities whose - // marker shows the PDS actually saw a reset ('ambiguous' or 'confirmed') - // — the PDS returns the same 401 for unknown accounts, so without - // provenance a systemic failure (wrong PDS URL, incomplete restore) - // would read as mass ownership handoffs. + // marker is 'confirmed' — the PDS returns the same 401 for unknown + // accounts, so without provenance a systemic failure (wrong PDS URL, + // incomplete restore) would read as mass ownership handoffs. // - // The marker only ever advances toward stronger evidence here (pending - // -> ambiguous -> confirmed); rejections never withdraw it, because a - // rejected retry may have been refused precisely because an earlier - // attempt consumed the token when it committed. Only proof removes the - // marker: the custody flip, or a later successful login with the stored - // credentials (which shows no reset committed — PdsSessionService - // clears 'pending'/'ambiguous' then). - const priorStatus = identity.takeOwnershipStatus; - if (priorStatus === 'ambiguous' || priorStatus === 'confirmed') { + // All marker writes are compare-and-set transitions that only advance + // toward stronger evidence (pending -> ambiguous -> confirmed); a stale + // writer loses rather than overwriting a stronger state. Rejections + // never withdraw the marker, because a rejected retry may have been + // refused precisely because an earlier attempt consumed the token when + // it committed. Only proof removes the marker: the custody flip, or a + // later successful login with the stored credentials (which shows no + // reset committed — PdsSessionService sweeps 'pending'/'ambiguous' + // then). + const markedPending = + await this.userAtprotoIdentityService.transitionTakeOwnershipStatus( + tenantId, + identity.id, + [null, 'pending'], + 'pending', + ); + if (!markedPending) { this.logger.warn( - `Password reset requested for user ${user.ulid} while a prior reset attempt is still '${priorStatus}'; keeping that marker`, + `Password reset requested for user ${user.ulid} while an earlier attempt's marker is unresolved; keeping the stronger state`, { tenantId }, ); - } else { - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipStatus: 'pending', - }); } // Call PDS to reset password @@ -387,19 +390,21 @@ export class AtprotoIdentityController { } // No definitive answer from the PDS (timeout, connection loss, 5xx): - // the reset may have committed with the response lost. Upgrade the - // marker so the session-401 repair may finish the handoff if it did. - if (priorStatus !== 'confirmed') { - try { - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipStatus: 'ambiguous', - }); - } catch (markError) { - this.logger.error( - `Failed to mark ambiguous password reset for user ${user.ulid}; marker stays 'pending' and needs manual triage if the reset committed`, - { tenantId, error: markError.message }, - ); - } + // the reset may have committed with the response lost. Record the + // ambiguity durably; the transition leaves a stronger 'confirmed' + // from a concurrent attempt untouched. + try { + await this.userAtprotoIdentityService.transitionTakeOwnershipStatus( + tenantId, + identity.id, + [null, 'pending'], + 'ambiguous', + ); + } catch (markError) { + this.logger.error( + `Failed to mark ambiguous password reset for user ${user.ulid}; marker stays 'pending' and needs manual triage if the reset committed`, + { tenantId, error: markError.message }, + ); } this.logger.error( `PDS gave no definitive answer to a password reset for user ${user.ulid}`, @@ -413,11 +418,15 @@ export class AtprotoIdentityController { // The PDS acknowledged the reset — record that durably FIRST. If ending // custody below fails, the 'confirmed' marker is what lets the client's // take-ownership/complete retry or the session-401 repair finish the - // handoff later. + // handoff later. Applied from any prior state; it only skips when a + // concurrent flip already ended custody, which needs no record. try { - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipStatus: 'confirmed', - }); + await this.userAtprotoIdentityService.transitionTakeOwnershipStatus( + tenantId, + identity.id, + [null, 'pending', 'ambiguous', 'confirmed'], + 'confirmed', + ); } catch (markError) { this.logger.error( `Failed to record confirmed password reset for user ${user.ulid}`, diff --git a/src/pds/pds-session.service.spec.ts b/src/pds/pds-session.service.spec.ts index 9145ac7e..c6cf2143 100644 --- a/src/pds/pds-session.service.spec.ts +++ b/src/pds/pds-session.service.spec.ts @@ -29,6 +29,7 @@ describe('PdsSessionService', () => { findByUserUlid: jest.Mock; findByDid: jest.Mock; update: jest.Mock; + transitionTakeOwnershipStatus: jest.Mock; }; let mockPdsCredentialService: { decrypt: jest.Mock; @@ -84,6 +85,7 @@ describe('PdsSessionService', () => { findByUserUlid: jest.fn(), findByDid: jest.fn(), update: jest.fn(), + transitionTakeOwnershipStatus: jest.fn().mockResolvedValue(true), }; mockPdsCredentialService = { @@ -390,10 +392,10 @@ describe('PdsSessionService', () => { return custodialIdentity; }; - it("should finish take-ownership on 401 when a reset's outcome was ambiguous", async () => { + it('should finish take-ownership on 401 when a reset was confirmed but custody never flipped', async () => { const custodialIdentity = arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipStatus: 'ambiguous' }), + createMockIdentity({ takeOwnershipStatus: 'confirmed' }), ); mockPdsAccountService.createSession.mockRejectedValue( new PdsApiError( @@ -421,10 +423,14 @@ describe('PdsSessionService', () => { expect(mockElastiCacheService.del).toHaveBeenCalled(); }); - it('should finish take-ownership on 401 when a reset was confirmed but custody never flipped', async () => { - const custodialIdentity = arrangeCustodialSessionAttempt(); + it("should not auto-repair on 401 when the marker is only 'ambiguous'", async () => { + // Ambiguity includes "the reset request never reached the PDS", so + // this 401 could still be systemic (wrong PDS URL, incomplete + // restore). Destroying credentials needs the certainty of + // 'confirmed'; ambiguous cases are surfaced for reconciliation + arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipStatus: 'confirmed' }), + createMockIdentity({ takeOwnershipStatus: 'ambiguous' }), ); mockPdsAccountService.createSession.mockRejectedValue( new PdsApiError('Invalid identifier or password', 401), @@ -433,15 +439,7 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); expect(result).toBeNull(); - expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( - tenantId, - custodialIdentity.id, - { - pdsCredentials: null, - isCustodial: false, - takeOwnershipStatus: null, - }, - ); + expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); }); it('should not end custody on 401 without a recorded marker', async () => { @@ -522,7 +520,7 @@ describe('PdsSessionService', () => { it('should still return null when the repair itself fails', async () => { arrangeCustodialSessionAttempt(); mockUserAtprotoIdentityService.findByDid.mockResolvedValue( - createMockIdentity({ takeOwnershipStatus: 'ambiguous' }), + createMockIdentity({ takeOwnershipStatus: 'confirmed' }), ); mockUserAtprotoIdentityService.update.mockRejectedValue( new Error('DB write failed'), @@ -567,10 +565,15 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); expect(result).not.toBeNull(); - expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + // Compare-and-set against exactly the state that was read, so the + // sweep loses if the marker advanced while the login was in flight + expect( + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith( tenantId, custodialIdentity.id, - { takeOwnershipStatus: null }, + ['pending'], + null, ); }); @@ -580,10 +583,13 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); expect(result).not.toBeNull(); - expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + expect( + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith( tenantId, custodialIdentity.id, - { takeOwnershipStatus: null }, + ['ambiguous'], + null, ); }); @@ -595,7 +601,9 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); expect(result).not.toBeNull(); - expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + expect( + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus, + ).not.toHaveBeenCalled(); }); it('should not touch identities without a marker', async () => { @@ -604,12 +612,26 @@ describe('PdsSessionService', () => { const result = await service.getSessionForUser(tenantId, userUlid); expect(result).not.toBeNull(); - expect(mockUserAtprotoIdentityService.update).not.toHaveBeenCalled(); + expect( + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus, + ).not.toHaveBeenCalled(); + }); + + it('should still return the session when the sweep loses the compare-and-set race', async () => { + arrangeSuccessfulFreshSession('pending'); + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus.mockResolvedValue( + false, + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(result!.source).toBe('fresh'); }); it('should still return the session when the sweep write fails', async () => { arrangeSuccessfulFreshSession('pending'); - mockUserAtprotoIdentityService.update.mockRejectedValue( + mockUserAtprotoIdentityService.transitionTakeOwnershipStatus.mockRejectedValue( new Error('DB write failed'), ); diff --git a/src/pds/pds-session.service.ts b/src/pds/pds-session.service.ts index 5f67ce85..806facf2 100644 --- a/src/pds/pds-session.service.ts +++ b/src/pds/pds-session.service.ts @@ -362,24 +362,39 @@ export class PdsSessionService { } // The stored credentials just authenticated, which is proof that no - // password reset committed. Sweep away a leftover marker from a reset - // attempt that was prepared but never confirmed ('pending') or whose - // outcome was lost ('ambiguous') — without this, a stale marker would - // keep the identity eligible for the 401 repair indefinitely. - // 'confirmed' is NOT swept: the PDS acknowledged that reset, so a - // still-working old password is an anomaly to investigate, not proof. + // password reset had committed when this identity was read. Sweep + // away a leftover marker from a reset attempt that was prepared but + // never confirmed ('pending') or whose outcome was lost ('ambiguous') + // — without this, a stale marker would linger indefinitely. The + // compare-and-set only clears the exact state that was read: if the + // marker advanced while this login was in flight (e.g. the reset + // confirmed meanwhile), the proof predates the change and the + // stronger marker survives. 'confirmed' is never swept: the PDS + // acknowledged that reset, so a still-working old password is an + // anomaly to investigate, not proof. if ( identity.takeOwnershipStatus === 'pending' || identity.takeOwnershipStatus === 'ambiguous' ) { try { - await this.userAtprotoIdentityService.update(tenantId, identity.id, { - takeOwnershipStatus: null, - }); - this.logger.log( - `Cleared stale '${identity.takeOwnershipStatus}' take-ownership marker for DID ${identity.did}: stored credentials still authenticate`, - { tenantId }, - ); + const swept = + await this.userAtprotoIdentityService.transitionTakeOwnershipStatus( + tenantId, + identity.id, + [identity.takeOwnershipStatus], + null, + ); + if (swept) { + this.logger.log( + `Cleared stale '${identity.takeOwnershipStatus}' take-ownership marker for DID ${identity.did}: stored credentials still authenticate`, + { tenantId }, + ); + } else { + this.logger.warn( + `Take-ownership marker for DID ${identity.did} advanced past '${identity.takeOwnershipStatus}' during login; keeping the stronger state`, + { tenantId }, + ); + } } catch (sweepError) { this.logger.warn( `Failed to clear stale take-ownership marker for DID ${identity.did}`, @@ -424,11 +439,12 @@ export class PdsSessionService { * clear the stale credentials, mark the identity non-custodial, and drop * any cached session. * - * Guarded by the takeOwnershipStatus marker: only 'ambiguous' (the PDS - * gave no definitive answer to a reset) or 'confirmed' (the PDS - * acknowledged a reset but custody was never flipped) qualify. A bare - * 'pending' records intent only — the reset may never have reached the - * PDS, so a 401 against it proves nothing. Identities without a + * Guarded by the takeOwnershipStatus marker: only 'confirmed' — the PDS + * acknowledged a reset but custody was never flipped — authorizes this + * destructive repair. 'pending' records intent only, and even 'ambiguous' + * includes "the request never reached the PDS", so a 401 against either + * could still be systemic (wrong PDS URL, incomplete restore); + * 'ambiguous' is logged for reconciliation instead. Identities without a * qualifying marker are left untouched no matter what the PDS said — a * 401 without recorded provenance is not evidence of a handoff. * @@ -443,12 +459,21 @@ export class PdsSessionService { tenantId, did, ); - if ( - !identity || - !identity.isCustodial || - (identity.takeOwnershipStatus !== 'ambiguous' && - identity.takeOwnershipStatus !== 'confirmed') - ) { + if (!identity || !identity.isCustodial) { + return; + } + if (identity.takeOwnershipStatus === 'ambiguous') { + // Ambiguity includes "the reset request never reached the PDS", so + // this 401 could still be systemic (wrong PDS URL, incomplete + // restore) rather than proof of a committed reset. Destroying + // credentials needs certainty; flag for reconciliation instead. + this.logger.error( + `Custodial login 401 for DID ${did} with an 'ambiguous' take-ownership marker; not auto-repairing — needs reconciliation (the reset may have committed, or the 401 may be systemic)`, + { tenantId }, + ); + return; + } + if (identity.takeOwnershipStatus !== 'confirmed') { return; } diff --git a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts index c84e0336..b6c7ed97 100644 --- a/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts +++ b/src/user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity.ts @@ -104,21 +104,26 @@ export class UserAtprotoIdentityEntity extends EntityRelationalHelper { * - 'pending' — a reset request was prepared; the PDS may never have * received it. Records intent only, NOT proof of a reset. * - 'ambiguous' — the PDS gave no definitive answer (timeout, 5xx); the - * reset may have committed with the response lost. + * reset may have committed with the response lost, or the + * request may never have arrived at all. * - 'confirmed' — the PDS acknowledged the reset but ending custody in the * same request failed; custody must still be ended. * * A PDS login 401 alone is ambiguous (the PDS returns the same 401 for * unknown accounts to prevent enumeration, so wrong PDS URL / incomplete - * restore / deleted account all look like a bad password). Custody may be - * ended in response to a 401 only for 'ambiguous' or 'confirmed' — never - * for 'pending', which a crash before the PDS submission can strand. + * restore / deleted account all look like a bad password). Only + * 'confirmed' authorizes automatically ending custody in response to a + * 401; 'ambiguous' is surfaced for reconciliation instead, since its 401 + * could still be systemic, and 'pending' proves nothing. * - * The marker is only ever removed by proof: the custody flip itself, or a - * successful login with the stored credentials (which shows no reset - * committed) clearing 'pending'/'ambiguous'. Rejection responses do not - * clear it — a rejected retry cannot vouch for an earlier attempt whose - * token it may itself have consumed. + * State changes go through the compare-and-set + * transitionTakeOwnershipStatus so the marker only advances toward + * stronger evidence; stale writers lose instead of overwriting. It is + * only ever removed by proof: the custody flip itself, or a successful + * login with the stored credentials (which shows no reset committed) + * sweeping 'pending'/'ambiguous'. Rejection responses do not clear it — a + * rejected retry cannot vouch for an earlier attempt whose token it may + * itself have consumed. */ @Column({ type: 'varchar', length: 16, nullable: true }) takeOwnershipStatus: TakeOwnershipStatus | null; diff --git a/src/user-atproto-identity/user-atproto-identity.service.ts b/src/user-atproto-identity/user-atproto-identity.service.ts index 1a1ca071..7254564b 100644 --- a/src/user-atproto-identity/user-atproto-identity.service.ts +++ b/src/user-atproto-identity/user-atproto-identity.service.ts @@ -1,5 +1,5 @@ import { Injectable, Inject, Scope } from '@nestjs/common'; -import { Repository } from 'typeorm'; +import { Brackets, Repository } from 'typeorm'; import { REQUEST } from '@nestjs/core'; import { TenantConnectionService } from '../tenant/tenant.service'; import { @@ -145,7 +145,10 @@ export class UserAtprotoIdentityService { pdsUrl: string; pdsCredentials: string | null; isCustodial: boolean; - takeOwnershipStatus: TakeOwnershipStatus | null; + // Only null is accepted here: flips clear the marker together with + // the credentials, but setting a state must go through the + // compare-and-set transitionTakeOwnershipStatus, never a blind write + takeOwnershipStatus: null; }>, ): Promise> { await this.getTenantRepository(tenantId); @@ -158,4 +161,55 @@ export class UserAtprotoIdentityService { Object.assign(existing, data); return this.repository.save(existing); } + + /** + * Compare-and-set transition of the take-ownership marker. + * + * The marker must only ever move toward stronger evidence, and several + * writers race on it: the reset request, the sweep in session creation, + * and the custody flip. An unconditional write lets a stale reader + * overwrite a stronger state — e.g. a session attempt that read 'pending' + * clearing the marker after the reset was confirmed. This transition only + * applies while the row still holds one of the expected states and is + * still custodial, so stale writers lose instead. + * + * @returns true when the transition was applied + */ + async transitionTakeOwnershipStatus( + tenantId: string, + id: number, + expected: (TakeOwnershipStatus | null)[], + to: TakeOwnershipStatus | null, + ): Promise { + await this.getTenantRepository(tenantId); + + const expectedStatuses = expected.filter( + (status): status is TakeOwnershipStatus => status !== null, + ); + const expectNull = expected.includes(null); + + const result = await this.repository + .createQueryBuilder() + .update() + .set({ takeOwnershipStatus: to }) + .where('id = :id', { id }) + .andWhere('"isCustodial" = true') + .andWhere( + new Brackets((qb) => { + if (expectedStatuses.length > 0) { + qb.where('"takeOwnershipStatus" IN (:...expectedStatuses)', { + expectedStatuses, + }); + if (expectNull) { + qb.orWhere('"takeOwnershipStatus" IS NULL'); + } + } else { + qb.where('"takeOwnershipStatus" IS NULL'); + } + }), + ) + .execute(); + + return (result.affected ?? 0) > 0; + } }