diff --git a/src/atproto-identity/atproto-identity.controller.spec.ts b/src/atproto-identity/atproto-identity.controller.spec.ts index 8c9ed035..79bfe9c3 100644 --- a/src/atproto-identity/atproto-identity.controller.spec.ts +++ b/src/atproto-identity/atproto-identity.controller.spec.ts @@ -31,6 +31,8 @@ describe('AtprotoIdentityController', () => { let pdsAccountService: PdsAccountService; let pdsSessionService: PdsSessionService; let blueskyService: BlueskyService; + let authBlueskyService: AuthBlueskyService; + let configService: ConfigService; const mockIdentityEntity: Partial = { id: 1, @@ -179,6 +181,8 @@ describe('AtprotoIdentityController', () => { pdsAccountService = module.get(PdsAccountService); pdsSessionService = module.get(PdsSessionService); blueskyService = module.get(BlueskyService); + authBlueskyService = module.get(AuthBlueskyService); + configService = module.get(ConfigService); }); it('should be defined', () => { @@ -1268,4 +1272,86 @@ describe('AtprotoIdentityController', () => { ).rejects.toThrow(BadRequestException); }); }); + + describe('getIdentity - identity:handle is only required for our PDS', () => { + // identity:handle authorizes a handle rename. updateHandle rejects any + // identity that is not on our PDS, so an external identity is never + // required to hold it and must not be asked to reconnect for it. + const ourPdsNonCustodial: Partial = { + ...mockIdentityEntity, + pdsCredentials: null, + isCustodial: false, + }; + + beforeEach(() => { + // Both cases hold a live OAuth session; only the host PDS differs. + jest.spyOn(blueskyService, 'tryResumeSession').mockResolvedValue({ + sessionManager: {}, + } as never); + jest.spyOn(configService, 'get').mockImplementation((key: string) => { + if (key === 'pds.url') return 'https://pds.openmeet.net'; + if (key === 'pds.serviceHandleDomains') return '.opnmt.me'; + if (key === 'ATPROTO_OAUTH_SCOPES') + return 'atproto account:email identity:handle'; + return null; + }); + }); + + it('should report identity:handle missing for an our-PDS identity', async () => { + // Arrange + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue(ourPdsNonCustodial as UserAtprotoIdentityEntity); + jest + .spyOn(authBlueskyService, 'getScopeMismatch') + .mockResolvedValue(['identity:handle']); + + // Act + const result = await controller.getIdentity(mockRequest); + + // Assert + expect(result?.isOurPds).toBe(true); + expect(result?.scopeMismatch).toBe(true); + expect(result?.missingScopes).toEqual(['identity:handle']); + }); + + it('should not report identity:handle missing for an external-PDS identity', async () => { + // Arrange + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue( + mockNonCustodialIdentity as UserAtprotoIdentityEntity, + ); + jest + .spyOn(authBlueskyService, 'getScopeMismatch') + .mockResolvedValue(['identity:handle']); + + // Act + const result = await controller.getIdentity(mockRequest); + + // Assert + expect(result?.isOurPds).toBe(false); + expect(result?.scopeMismatch).toBe(false); + expect(result?.missingScopes).toEqual([]); + }); + + it('should still report other missing scopes for an external-PDS identity', async () => { + // Arrange: stored flag holds identity:handle alongside a real gap + jest + .spyOn(identityService, 'findByUserUlid') + .mockResolvedValue( + mockNonCustodialIdentity as UserAtprotoIdentityEntity, + ); + jest + .spyOn(authBlueskyService, 'getScopeMismatch') + .mockResolvedValue(['identity:handle', 'account:email']); + + // Act + const result = await controller.getIdentity(mockRequest); + + // Assert + expect(result?.scopeMismatch).toBe(true); + expect(result?.missingScopes).toEqual(['account:email']); + }); + }); }); diff --git a/src/atproto-identity/atproto-identity.controller.ts b/src/atproto-identity/atproto-identity.controller.ts index 264cd503..0f2a8a42 100644 --- a/src/atproto-identity/atproto-identity.controller.ts +++ b/src/atproto-identity/atproto-identity.controller.ts @@ -43,6 +43,8 @@ import { NullableType } from '../utils/types/nullable.type'; import { AllConfigType } from '../config/config.type'; import { UserAtprotoIdentityEntity } from '../user-atproto-identity/infrastructure/persistence/relational/entities/user-atproto-identity.entity'; import { AuthBlueskyService } from '../auth-bluesky/auth-bluesky.service'; +import { getRequiredScopesForIdentity } from '../utils/check-scope-mismatch'; +import { getAtprotoConfiguredScopes } from '../utils/bluesky'; @ApiTags('AT Protocol Identity') @Controller({ @@ -543,6 +545,7 @@ export class AtprotoIdentityController { tenantId: string, ): Promise { const ourPdsUrl = this.configService.get('pds.url', { infer: true }); + const isOurPds = identity.pdsUrl === ourPdsUrl; const serviceHandleDomains = this.configService.get('pds.serviceHandleDomains', { infer: true }) || ''; const validHandleDomains = serviceHandleDomains @@ -599,8 +602,16 @@ export class AtprotoIdentityController { identity.did, ); if (mismatchData && mismatchData.length > 0) { - scopeMismatch = true; - missingScopes = mismatchData; + // The stored list was measured against the global scope list, so + // narrow it to what this identity is actually required to hold. + const requiredScopes = getRequiredScopesForIdentity( + getAtprotoConfiguredScopes(this.configService), + { isOurPds }, + ).split(/\s+/); + missingScopes = mismatchData.filter((scope) => + requiredScopes.includes(scope), + ); + scopeMismatch = missingScopes.length > 0; } } catch (error) { this.logger.warn('Failed to check scope mismatch', { @@ -615,7 +626,7 @@ export class AtprotoIdentityController { handle: identity.handle, pdsUrl: identity.pdsUrl, isCustodial: identity.isCustodial, - isOurPds: identity.pdsUrl === ourPdsUrl, + isOurPds, hasActiveSession, scopeMismatch, missingScopes, diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 912d318e..89d4688a 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -981,6 +981,91 @@ describe('AuthService', () => { expect(result?.atprotoIdentity?.scopeMismatch).toBe(false); expect(result?.atprotoIdentity?.missingScopes).toEqual([]); }); + + describe('identity:handle is only required for our PDS', () => { + // identity:handle authorizes a handle rename. updateHandle rejects any + // identity that is not on our PDS, so an external identity is never + // required to hold it and must not be asked to reconnect for it. + const ourPdsIdentity = { + ...baseIdentity, + did: 'did:plc:ourpds456', + handle: 'test-user.opnmt.me', + pdsUrl: 'https://pds.openmeet.net', + }; + + const sessionGranting = (scope: string) => ({ + sessionManager: { + getTokenInfo: jest.fn().mockResolvedValue({ scope }), + }, + }); + + beforeEach(() => { + mockConfigService.get.mockImplementation((key: string) => { + if (key === 'pds.url') return 'https://pds.openmeet.net'; + if (key === 'pds.serviceHandleDomains') return '.opnmt.me'; + if (key === 'ATPROTO_OAUTH_SCOPES') + return 'atproto identity:handle repo:community.lexicon.calendar.fake'; + return null; + }); + }); + + it('should report identity:handle missing for an our-PDS identity', async () => { + // Arrange: took ownership, session predates the identity:handle scope + mockUserAtprotoIdentityService.findByUserUlid.mockResolvedValue( + ourPdsIdentity, + ); + mockBlueskyService.tryResumeSession.mockResolvedValue( + sessionGranting('atproto repo:community.lexicon.calendar.fake'), + ); + + // Act + const result = await authService.me(jwtPayload); + + // Assert + expect(result?.atprotoIdentity?.isOurPds).toBe(true); + expect(result?.atprotoIdentity?.scopeMismatch).toBe(true); + expect(result?.atprotoIdentity?.missingScopes).toEqual([ + 'identity:handle', + ]); + }); + + it('should not report identity:handle missing for an external-PDS identity', async () => { + // Arrange: same stale session shape, but hosted on bsky.social + mockUserAtprotoIdentityService.findByUserUlid.mockResolvedValue( + baseIdentity, + ); + mockBlueskyService.tryResumeSession.mockResolvedValue( + sessionGranting('atproto repo:community.lexicon.calendar.fake'), + ); + + // Act + const result = await authService.me(jwtPayload); + + // Assert + expect(result?.atprotoIdentity?.isOurPds).toBe(false); + expect(result?.atprotoIdentity?.scopeMismatch).toBe(false); + expect(result?.atprotoIdentity?.missingScopes).toEqual([]); + }); + + it('should still report other missing scopes for an external-PDS identity', async () => { + // Arrange: external identity missing identity:handle AND a real scope + mockUserAtprotoIdentityService.findByUserUlid.mockResolvedValue( + baseIdentity, + ); + mockBlueskyService.tryResumeSession.mockResolvedValue( + sessionGranting('atproto'), + ); + + // Act + const result = await authService.me(jwtPayload); + + // Assert + expect(result?.atprotoIdentity?.scopeMismatch).toBe(true); + expect(result?.atprotoIdentity?.missingScopes).toEqual([ + 'repo:community.lexicon.calendar.fake', + ]); + }); + }); }); describe('validateSocialLogin - PDS Account Auto-Creation', () => { diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index e7811c06..025960e8 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -61,7 +61,10 @@ import { MeResponse } from './dto/me-response.dto'; import { AtprotoIdentityDto } from '../atproto-identity/dto/atproto-identity.dto'; import { ElastiCacheService } from '../elasticache/elasticache.service'; import { getTenantConfig } from '../utils/tenant-config'; -import { checkScopeMismatch } from '../utils/check-scope-mismatch'; +import { + checkScopeMismatch, + getRequiredScopesForIdentity, +} from '../utils/check-scope-mismatch'; import { getAtprotoConfiguredScopes } from '../utils/bluesky'; import { OAuthSession } from '@atproto/oauth-client'; import { CreateLoginLinkResponseDto } from './dto/create-login-link-response.dto'; @@ -670,6 +673,7 @@ export class AuthService { if (identity) { // Map to DTO, explicitly excluding pdsCredentials const ourPdsUrl = this.configService.get('pds.url', { infer: true }); + const isOurPds = identity.pdsUrl === ourPdsUrl; const serviceHandleDomains = this.configService.get('pds.serviceHandleDomains', { infer: true, @@ -706,7 +710,9 @@ export class AuthService { this.configService, ); scopeMissingScopes = checkScopeMismatch( - configuredScopes, + getRequiredScopesForIdentity(configuredScopes, { + isOurPds, + }), tokenInfo.scope, ); scopeHasMismatch = scopeMissingScopes.length > 0; @@ -726,7 +732,7 @@ export class AuthService { handle: identity.handle, pdsUrl: identity.pdsUrl, isCustodial: identity.isCustodial, - isOurPds: identity.pdsUrl === ourPdsUrl, + isOurPds, hasActiveSession, scopeMismatch: scopeHasMismatch, missingScopes: scopeMissingScopes, diff --git a/src/utils/bluesky.spec.ts b/src/utils/bluesky.spec.ts index d934ac01..a366e56f 100644 --- a/src/utils/bluesky.spec.ts +++ b/src/utils/bluesky.spec.ts @@ -1,4 +1,5 @@ -import { createPlcFallbackFetch } from './bluesky'; +import { ConfigService } from '@nestjs/config'; +import { createPlcFallbackFetch, getAtprotoConfiguredScopes } from './bluesky'; describe('createPlcFallbackFetch', () => { let mockGlobalFetch: jest.Mock; @@ -183,3 +184,33 @@ describe('createPlcFallbackFetch', () => { }); }); }); + +describe('getAtprotoConfiguredScopes', () => { + const configWith = (scopes?: string) => + ({ get: () => scopes }) as unknown as ConfigService; + + it('should use the configured scopes when ATPROTO_OAUTH_SCOPES is set', () => { + const result = getAtprotoConfiguredScopes(configWith('atproto repo:a.b.c')); + expect(result).toBe('atproto repo:a.b.c'); + }); + + it('should fall back to the defaults when ATPROTO_OAUTH_SCOPES is unset', () => { + const scopes = getAtprotoConfiguredScopes(configWith(undefined)).split(' '); + expect(scopes).toEqual( + expect.arrayContaining([ + 'atproto', + 'account:email', + 'repo:community.lexicon.calendar.event', + 'repo:community.lexicon.calendar.rsvp', + ]), + ); + }); + + // Without this scope the PDS rejects com.atproto.identity.updateHandle with a + // 403 for anyone whose session is OAuth rather than custodial - so taking + // ownership of your own account costs you the ability to rename it. + it('should request identity:handle so users can change their own handle', () => { + const scopes = getAtprotoConfiguredScopes(configWith(undefined)).split(' '); + expect(scopes).toContain('identity:handle'); + }); +}); diff --git a/src/utils/bluesky.ts b/src/utils/bluesky.ts index bd04d7b9..e1ba548e 100644 --- a/src/utils/bluesky.ts +++ b/src/utils/bluesky.ts @@ -16,7 +16,7 @@ import { createRequestLock } from '../auth-bluesky/stores/redlock'; * Default OAuth scopes if ATPROTO_OAUTH_SCOPES env var is not set. */ const DEFAULT_ATPROTO_OAUTH_SCOPES = - 'atproto account:email rpc:app.bsky.actor.getProfile?aud=did:web:api.bsky.app#bsky_appview repo:community.lexicon.calendar.event repo:community.lexicon.calendar.rsvp'; + 'atproto account:email identity:handle rpc:app.bsky.actor.getProfile?aud=did:web:api.bsky.app#bsky_appview repo:community.lexicon.calendar.event repo:community.lexicon.calendar.rsvp'; /** * Get the configured ATProto OAuth scopes from env var or default. diff --git a/src/utils/check-scope-mismatch.spec.ts b/src/utils/check-scope-mismatch.spec.ts index dbfdd9ec..764904c1 100644 --- a/src/utils/check-scope-mismatch.spec.ts +++ b/src/utils/check-scope-mismatch.spec.ts @@ -1,4 +1,7 @@ -import { checkScopeMismatch } from './check-scope-mismatch'; +import { + checkScopeMismatch, + getRequiredScopesForIdentity, +} from './check-scope-mismatch'; describe('checkScopeMismatch', () => { it('should return empty array when scopes are the same', () => { @@ -72,3 +75,69 @@ describe('checkScopeMismatch', () => { ).toEqual([]); }); }); + +describe('getRequiredScopesForIdentity', () => { + const configured = 'atproto account:email identity:handle repo:x.y.event'; + + it('should require identity:handle for an identity on our PDS', () => { + expect( + getRequiredScopesForIdentity(configured, { isOurPds: true }), + ).toEqual('atproto account:email identity:handle repo:x.y.event'); + }); + + it('should not require identity:handle for an external PDS identity', () => { + expect( + getRequiredScopesForIdentity(configured, { isOurPds: false }), + ).toEqual('atproto account:email repo:x.y.event'); + }); + + it('should keep every other scope for an external PDS identity', () => { + const required = getRequiredScopesForIdentity(configured, { + isOurPds: false, + }).split(' '); + expect(required).toEqual( + expect.arrayContaining(['atproto', 'account:email', 'repo:x.y.event']), + ); + expect(required).toHaveLength(3); + }); + + it('should preserve the configured order', () => { + expect( + getRequiredScopesForIdentity('identity:handle atproto', { + isOurPds: true, + }), + ).toEqual('identity:handle atproto'); + }); + + it('should tolerate extra whitespace in the configured list', () => { + expect( + getRequiredScopesForIdentity(' atproto identity:handle ', { + isOurPds: false, + }), + ).toEqual('atproto'); + }); + + it('should return an empty string when nothing is configured', () => { + expect(getRequiredScopesForIdentity('', { isOurPds: true })).toEqual(''); + }); + + it('should feed checkScopeMismatch so an external identity has no gap', () => { + // The session was granted under the scope list that predates + // identity:handle. Our PDS still needs it; an external PDS does not. + const granted = 'atproto account:email repo:x.y.event'; + + expect( + checkScopeMismatch( + getRequiredScopesForIdentity(configured, { isOurPds: true }), + granted, + ), + ).toEqual(['identity:handle']); + + expect( + checkScopeMismatch( + getRequiredScopesForIdentity(configured, { isOurPds: false }), + granted, + ), + ).toEqual([]); + }); +}); diff --git a/src/utils/check-scope-mismatch.ts b/src/utils/check-scope-mismatch.ts index 5c8c3ae7..54a72ba5 100644 --- a/src/utils/check-scope-mismatch.ts +++ b/src/utils/check-scope-mismatch.ts @@ -18,3 +18,35 @@ export function checkScopeMismatch( return [...configured].filter((scope) => !granted.has(scope)); } + +/** + * The scopes a session for this identity is expected to carry. + * + * Most configured scopes apply to every identity. `identity:handle` does not. + * It authorizes com.atproto.identity.updateHandle, and updateHandle rejects + * any identity that is not on our PDS. Measuring an external identity against + * it reports a gap that can never close, and asks that user to grant a handle + * rename permission we never use for them. + * + * The scope stays in the configured list and in the OAuth client metadata. + * Users who take ownership of an account on our PDS sign in over OAuth, and + * they do need it. + * + * Build the yardstick with this before comparing against granted scopes, + * rather than filtering the comparison afterwards. A scope that turns on some + * other condition adds a field to `identity` and a case here. + * + * @param configuredScopes - Space-delimited scopes the app is configured with + * @param identity.isOurPds - Whether the identity is hosted on OpenMeet's PDS + * @returns Space-delimited scopes required for this identity + */ +export function getRequiredScopesForIdentity( + configuredScopes: string, + identity: { isOurPds: boolean }, +): string { + return configuredScopes + .split(/\s+/) + .filter((scope) => scope.length > 0) + .filter((scope) => (scope === 'identity:handle' ? identity.isOurPds : true)) + .join(' '); +}