diff --git a/src/atproto-identity/atproto-identity-recovery.service.spec.ts b/src/atproto-identity/atproto-identity-recovery.service.spec.ts index e110d264..29d52006 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); }); @@ -526,15 +537,23 @@ 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, + takeOwnershipStatus: null, }, ); + + // 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 +566,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 +576,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..8b6a6520 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,17 +273,23 @@ 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 + // 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, + takeOwnershipStatus: null, }); + // 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..8c9ed035 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'), }; @@ -75,6 +77,8 @@ describe('AtprotoIdentityController', () => { beforeEach(async () => { const mockIdentityService = { findByUserUlid: jest.fn(), + update: jest.fn(), + transitionTakeOwnershipStatus: jest.fn().mockResolvedValue(true), }; const mockAtprotoIdentityService = { @@ -619,30 +623,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,9 +664,60 @@ 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.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith('test-tenant', 1, [null, 'pending'], 'pending'); + // ...and upgraded to 'confirmed' once the PDS acknowledges the reset + 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', + '01234567890123456789012345', + ); 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 }); + // The 'confirmed' marker survives so the session-401 repair can also + // finish the handoff if the client never completes + expect( + identityService.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + [null, 'pending', 'ambiguous', 'confirmed'], + 'confirmed', + ); + }); + it('should throw NotFoundException when user not found', async () => { // Arrange jest.spyOn(userService, 'findById').mockResolvedValueOnce(null); @@ -706,7 +761,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') @@ -724,6 +779,149 @@ describe('AtprotoIdentityController', () => { password: 'new-secure-password-123', }), ).rejects.toThrow(BadRequestException); + + // 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.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 () => { + // 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.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + [null, 'pending'], + '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.transitionTakeOwnershipStatus, + ).toHaveBeenLastCalledWith( + 'test-tenant', + 1, + [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). + // 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( + 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); + + // 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(); + }); + + 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); + // 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') + .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.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 520ba7aa..264cd503 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, @@ -285,7 +286,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,12 +304,18 @@ 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, 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', @@ -339,19 +348,130 @@ 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 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. + // + // 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 an earlier attempt's marker is unresolved; keeping the stronger state`, + { tenantId }, + ); + } + // Call PDS to reset password try { await this.pdsAccountService.resetPassword(dto.token, dto.password); } catch (error) { - 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. 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}`, + { 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. Applied from any prior state; it only skips when a + // concurrent flip already ended custody, which needs no record. + try { + 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}`, + { tenantId, error: markError.message }, + ); + } + + // 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. + 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 }; } + /** + * 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-AddTakeOwnershipStatusToAtprotoIdentities.ts b/src/database/migrations/1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts new file mode 100644 index 00000000..06ee1b58 --- /dev/null +++ b/src/database/migrations/1786741129455-AddTakeOwnershipStatusToAtprotoIdentities.ts @@ -0,0 +1,25 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class AddTakeOwnershipStatusToAtprotoIdentities1786741129455 + implements MigrationInterface +{ + 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 "takeOwnershipStatus" character varying(16) + `); + } + + public async down(queryRunner: QueryRunner): Promise { + const schema = queryRunner.connection.options.name || 'public'; + + await queryRunner.query(` + ALTER TABLE "${schema}"."userAtprotoIdentities" + DROP COLUMN IF EXISTS "takeOwnershipStatus" + `); + } +} diff --git a/src/pds/pds-session.service.spec.ts b/src/pds/pds-session.service.spec.ts index c82b4c6b..c6cf2143 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,8 @@ describe('PdsSessionService', () => { let mockUserAtprotoIdentityService: { findByUserUlid: jest.Mock; findByDid: jest.Mock; + update: jest.Mock; + transitionTakeOwnershipStatus: jest.Mock; }; let mockPdsCredentialService: { decrypt: jest.Mock; @@ -68,6 +70,7 @@ describe('PdsSessionService', () => { pdsUrl: testPdsUrl, pdsCredentials: encryptedCredentials, isCustodial: true, + takeOwnershipStatus: null, createdAt: new Date(), updatedAt: new Date(), ...overrides, @@ -81,6 +84,8 @@ describe('PdsSessionService', () => { mockUserAtprotoIdentityService = { findByUserUlid: jest.fn(), findByDid: jest.fn(), + update: jest.fn(), + transitionTakeOwnershipStatus: jest.fn().mockResolvedValue(true), }; mockPdsCredentialService = { @@ -371,6 +376,269 @@ 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 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, + 'AuthenticationRequired', + ), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + // Session still fails this cycle... + expect(result).toBeNull(); + // ...but the recorded handoff is completed + expect(mockUserAtprotoIdentityService.update).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + { + pdsCredentials: null, + isCustodial: false, + takeOwnershipStatus: null, + }, + ); + // ...and the cached session is invalidated + expect(mockElastiCacheService.del).toHaveBeenCalled(); + }); + + 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: 'ambiguous' }), + ); + 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 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({ 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), + ); + + 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( + 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 () => { + arrangeCustodialSessionAttempt(); + mockUserAtprotoIdentityService.findByDid.mockResolvedValue( + createMockIdentity({ takeOwnershipStatus: 'confirmed' }), + ); + 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(); + }); + }); + + 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(); + // 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, + ['pending'], + 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.transitionTakeOwnershipStatus, + ).toHaveBeenCalledWith( + tenantId, + custodialIdentity.id, + ['ambiguous'], + 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.transitionTakeOwnershipStatus, + ).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.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.transitionTakeOwnershipStatus.mockRejectedValue( + new Error('DB write failed'), + ); + + const result = await service.getSessionForUser(tenantId, userUlid); + + expect(result).not.toBeNull(); + expect(result!.source).toBe('fresh'); }); }); }); diff --git a/src/pds/pds-session.service.ts b/src/pds/pds-session.service.ts index 3c10a668..806facf2 100644 --- a/src/pds/pds-session.service.ts +++ b/src/pds/pds-session.service.ts @@ -14,9 +14,10 @@ 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 { SessionUnavailableError } from './pds.errors'; +import { PdsApiError, SessionUnavailableError } from './pds.errors'; /** * Result of a successful session retrieval. @@ -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 { @@ -331,10 +340,73 @@ 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 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 + // 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 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 { + 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}`, + { 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); @@ -362,6 +434,68 @@ export class PdsSessionService { } } + /** + * 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 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. + * + * 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; + } + 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; + } + + await this.userAtprotoIdentityService.update(tenantId, identity.id, { + pdsCredentials: null, + isCustodial: false, + takeOwnershipStatus: null, + }); + 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. * 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..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 @@ -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. * @@ -91,6 +97,37 @@ export class UserAtprotoIdentityEntity extends EntityRelationalHelper { @Column({ type: 'boolean', default: true }) isCustodial: boolean; + /** + * 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, 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). 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. + * + * 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; + @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..7254564b 100644 --- a/src/user-atproto-identity/user-atproto-identity.service.ts +++ b/src/user-atproto-identity/user-atproto-identity.service.ts @@ -1,8 +1,11 @@ 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 { 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,6 +145,10 @@ export class UserAtprotoIdentityService { pdsUrl: string; pdsCredentials: string | null; isCustodial: boolean; + // 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); @@ -154,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; + } } 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(); }); 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}`)