diff --git a/src/platform/platform-role/platform.role.resolver.mutations.spec.ts b/src/platform/platform-role/platform.role.resolver.mutations.spec.ts index a169efc461..6a741f92d7 100644 --- a/src/platform/platform-role/platform.role.resolver.mutations.spec.ts +++ b/src/platform/platform-role/platform.role.resolver.mutations.spec.ts @@ -1,4 +1,5 @@ import { RoleChangeType } from '@alkemio/notifications-lib'; +import { LogContext } from '@common/enums'; import { AuthorizationPrivilege } from '@common/enums/authorization.privilege'; import { LicensingCredentialBasedCredentialType } from '@common/enums/licensing.credential.based.credential.type'; import { RoleName } from '@common/enums/role.name'; @@ -11,11 +12,13 @@ import { LicenseService } from '@domain/common/license/license.service'; import { UserLookupService } from '@domain/community/user-lookup/user.lookup.service'; import { AccountService } from '@domain/space/account/account.service'; import { AccountLicenseService } from '@domain/space/account/account.service.license'; +import { LoggerService } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { PlatformService } from '@platform/platform/platform.service'; import { NotificationPlatformAdapter } from '@services/adapters/notification-adapter/notification.platform.adapter'; import { MockWinstonProvider } from '@test/mocks/winston.provider.mock'; import { defaultMockerFactory } from '@test/utils/default.mocker.factory'; +import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston'; import { type Mock } from 'vitest'; import { PlatformRoleResolverMutations } from './platform.role.resolver.mutations'; @@ -31,6 +34,7 @@ describe('PlatformRoleResolverMutations', () => { let licenseService: LicenseService; let notificationPlatformAdapter: NotificationPlatformAdapter; let roleSetAuthorizationService: RoleSetAuthorizationService; + let logger: LoggerService; const mockActorContext = { actorID: 'actor-1', @@ -67,6 +71,7 @@ describe('PlatformRoleResolverMutations', () => { licenseService = module.get(LicenseService); notificationPlatformAdapter = module.get(NotificationPlatformAdapter); roleSetAuthorizationService = module.get(RoleSetAuthorizationService); + logger = module.get(WINSTON_MODULE_NEST_PROVIDER); }); describe('assignPlatformRoleToUser', () => { @@ -182,6 +187,38 @@ describe('PlatformRoleResolverMutations', () => { }) ); }); + + // The notification is dispatched fire-and-forget, so a rejection that + // escapes would become an unhandled rejection and — under Node's default + // `--unhandled-rejections=throw` — take the whole process down rather than + // fail the request. Notifying is best-effort; it must stay contained. + it('should contain a notification dispatch failure and log it', async () => { + const roleData = { + actorID: 'user-target', + role: RoleName.GLOBAL_ADMIN, + }; + ( + notificationPlatformAdapter.platformGlobalRoleChanged as Mock + ).mockRejectedValue( + new TypeError("Cannot read properties of null (reading 'displayName')") + ); + + await expect( + resolver.assignPlatformRoleToUser(mockActorContext, roleData as any) + ).resolves.toBe(mockUser); + + await vi.waitFor(() => + expect(logger.error).toHaveBeenCalledWith( + { + message: 'Notification dispatch failed', + event: 'platformGlobalRoleChanged', + error: expect.stringContaining('TypeError'), + }, + expect.any(String), + LogContext.NOTIFICATIONS + ) + ); + }); }); describe('removePlatformRoleFromUser', () => { @@ -270,5 +307,33 @@ describe('PlatformRoleResolverMutations', () => { }) ); }); + + it('should contain a notification dispatch failure and log it', async () => { + const roleData = { + actorID: 'user-target', + role: RoleName.GLOBAL_ADMIN, + }; + ( + notificationPlatformAdapter.platformGlobalRoleChanged as Mock + ).mockRejectedValue( + new TypeError("Cannot read properties of null (reading 'displayName')") + ); + + await expect( + resolver.removePlatformRoleFromUser(mockActorContext, roleData as any) + ).resolves.toBe(mockUser); + + await vi.waitFor(() => + expect(logger.error).toHaveBeenCalledWith( + { + message: 'Notification dispatch failed', + event: 'platformGlobalRoleChanged', + error: expect.stringContaining('TypeError'), + }, + expect.any(String), + LogContext.NOTIFICATIONS + ) + ); + }); }); }); diff --git a/src/platform/platform-role/platform.role.resolver.mutations.ts b/src/platform/platform-role/platform.role.resolver.mutations.ts index 802dbb7430..15efe3cea6 100644 --- a/src/platform/platform-role/platform.role.resolver.mutations.ts +++ b/src/platform/platform-role/platform.role.resolver.mutations.ts @@ -1,4 +1,5 @@ import { RoleChangeType } from '@alkemio/notifications-lib'; +import { LogContext } from '@common/enums'; import { AuthorizationPrivilege } from '@common/enums/authorization.privilege'; import { LicensingCredentialBasedCredentialType } from '@common/enums/licensing.credential.based.credential.type'; import { RoleName } from '@common/enums/role.name'; @@ -14,12 +15,14 @@ import { UserLookupService } from '@domain/community/user-lookup/user.lookup.ser import { AccountService } from '@domain/space/account/account.service'; import { AccountLicenseService } from '@domain/space/account/account.service.license'; import { AccountLookupService } from '@domain/space/account.lookup/account.lookup.service'; +import { Inject, LoggerService } from '@nestjs/common'; import { Args, Mutation, Resolver } from '@nestjs/graphql'; import { PlatformService } from '@platform/platform/platform.service'; import { NotificationInputPlatformGlobalRoleChange } from '@services/adapters/notification-adapter/dto/platform/notification.dto.input.platform.global.role.change'; import { NotificationPlatformAdapter } from '@services/adapters/notification-adapter/notification.platform.adapter'; import { InstrumentResolver } from '@src/apm/decorators'; import { CurrentActor } from '@src/common/decorators'; +import { WINSTON_MODULE_NEST_PROVIDER } from 'nest-winston'; import { AssignPlatformRoleInput } from './dto/platform.role.dto.assign'; import { RemovePlatformRoleInput } from './dto/platform.role.dto.remove'; @@ -37,7 +40,8 @@ export class PlatformRoleResolverMutations { private roleSetService: RoleSetService, private userLookupService: UserLookupService, private roleSetAuthorizationService: RoleSetAuthorizationService, - private platformService: PlatformService + private platformService: PlatformService, + @Inject(WINSTON_MODULE_NEST_PROVIDER) private readonly logger: LoggerService ) {} @Mutation(() => IUser, { @@ -183,20 +187,52 @@ export class PlatformRoleResolverMutations { await this.licenseService.saveAll(licenses); } - private async notifyPlatformGlobalRoleChange( + private notifyPlatformGlobalRoleChange( triggeredBy: string, user: IUser, type: RoleChangeType, role: string - ) { + ): void { const notificationInput: NotificationInputPlatformGlobalRoleChange = { triggeredBy, userID: user.id, type: type, role: role, }; - await this.notificationPlatformAdapter.platformGlobalRoleChanged( - notificationInput + this.dispatchNotification( + this.notificationPlatformAdapter.platformGlobalRoleChanged( + notificationInput + ), + 'platformGlobalRoleChanged' ); } + + /** + * Wraps a fire-and-forget notification dispatch with a `.catch` so an + * unhandled rejection from a downstream notification adapter (e.g. a user + * row with a null profile → TypeError while building the payload) does not + * crash the Node process. + * + * The notification is still side-effectful: failures are logged at ERROR + * with structured details so monitoring can pick them up. Notifications + * are intentionally not awaited at the resolver level; we don't want a + * downstream notification problem to fail the user-facing mutation. + */ + private dispatchNotification( + promise: Promise, + eventLabel: string + ): void { + void promise.catch(error => { + const stack = error instanceof Error ? (error.stack ?? '') : ''; + this.logger.error?.( + { + message: 'Notification dispatch failed', + event: eventLabel, + error: String(error), + }, + stack, + LogContext.NOTIFICATIONS + ); + }); + } } diff --git a/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts b/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts index 87f848ecfb..e50190cba4 100644 --- a/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts +++ b/src/services/adapters/notification-external-adapter/notification.external.adapter.spec.ts @@ -330,6 +330,154 @@ describe('NotificationExternalAdapter', () => { }); }); + // A user row with a null profile used to make these payload builders throw a + // TypeError, which — dispatched fire-and-forget — reached Node's default + // `--unhandled-rejections=throw` and killed the server outright. + describe('display name resolution with an incomplete user row', () => { + const buildRemovedPayloadFor = (subject: any, recipient: any) => + adapter.buildPlatformUserRemovedNotificationPayload( + NotificationEvent.PLATFORM_ADMIN_USER_PROFILE_REMOVED, + 'admin-1', + [recipient], + subject + ); + + beforeEach(() => { + vi.mocked(configService.get).mockReturnValue('https://platform.test'); + vi.mocked(urlGeneratorService.createUrlForUserNameID).mockReturnValue( + '/user/1' + ); + }); + + it('should build every payload slot without throwing when profiles are null', async () => { + // Covers all three patched dereferences at once: the subject user, the + // triggering user (fetched), and each recipient. + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: 'Admin', + lastName: 'User', + email: 'admin@test.com', + nameID: 'admin-user', + profile: null, + } as any); + + const result = await buildRemovedPayloadFor( + { + id: 'removed-1', + firstName: 'Removed', + lastName: 'User', + email: 'removed@test.com', + nameID: 'removed-user', + profile: null, + }, + { + id: 'recipient-1', + firstName: null, + lastName: null, + email: 'recipient@test.com', + nameID: 'recipient', + profile: null, + } + ); + + expect(result.user.displayName).toBe('Removed User'); + expect(result.triggeredBy.profile.displayName).toBe('Admin User'); + expect(result.recipients[0].profile.displayName).toBe( + 'recipient@test.com' + ); + }); + + it('should prefer the profile display name over the fallbacks', async () => { + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: 'Admin', + lastName: 'User', + email: 'admin@test.com', + nameID: 'admin-user', + profile: { displayName: 'Preferred Name' }, + } as any); + + const result = await buildRemovedPayloadFor( + { + email: 'removed@test.com', + firstName: 'Removed', + lastName: 'User', + profile: { displayName: 'Removed Display' }, + }, + { + id: 'recipient-1', + email: 'recipient@test.com', + nameID: 'recipient', + profile: { displayName: 'Recipient Display' }, + } + ); + + expect(result.user.displayName).toBe('Removed Display'); + expect(result.triggeredBy.profile.displayName).toBe('Preferred Name'); + expect(result.recipients[0].profile.displayName).toBe( + 'Recipient Display' + ); + }); + + it('should fall through an empty display name and blank names to the email', async () => { + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'admin-1', + firstName: ' ', + lastName: '', + email: 'admin@test.com', + nameID: 'admin-user', + profile: { displayName: '' }, + } as any); + + const result = await buildRemovedPayloadFor( + { + email: 'removed@test.com', + firstName: undefined, + lastName: undefined, + profile: undefined, + }, + { + id: 'recipient-1', + firstName: '', + lastName: '', + email: 'recipient@test.com', + nameID: 'recipient', + profile: { displayName: '' }, + } + ); + + expect(result.user.displayName).toBe('removed@test.com'); + expect(result.triggeredBy.profile.displayName).toBe('admin@test.com'); + expect(result.recipients[0].profile.displayName).toBe( + 'recipient@test.com' + ); + }); + + it('should build the global role change payload for a user with no profile', async () => { + // The exact live path: revoking a platform role from an incomplete row. + vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ + id: 'user-1', + firstName: 'Test', + lastName: 'User', + email: 'test@test.com', + nameID: 'test-user', + profile: null, + } as any); + + const result = + await adapter.buildPlatformGlobalRoleChangedNotificationPayload( + NotificationEvent.PLATFORM_ADMIN_GLOBAL_ROLE_CHANGED, + 'admin-1', + [], + 'user-1', + 'REMOVED' as any, + 'GLOBAL_ADMIN' + ); + + expect(result.triggeredBy.profile.displayName).toBe('Test User'); + }); + }); + describe('buildSpaceCommunityApplicationCreatedNotificationPayload', () => { it('should build application payload with applicant', async () => { vi.mocked(userLookupService.getUserByIdOrFail).mockResolvedValue({ diff --git a/src/services/adapters/notification-external-adapter/notification.external.adapter.ts b/src/services/adapters/notification-external-adapter/notification.external.adapter.ts index 14d883bdb5..68ea88cb83 100644 --- a/src/services/adapters/notification-external-adapter/notification.external.adapter.ts +++ b/src/services/adapters/notification-external-adapter/notification.external.adapter.ts @@ -883,7 +883,7 @@ export class NotificationExternalAdapter { ); const result: NotificationEventPayloadPlatformUserRemoved = { user: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), email: user.email, }, ...basePayload, @@ -1237,7 +1237,7 @@ export class NotificationExternalAdapter { lastName: user.lastName, email: user.email, profile: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), url: userURL, }, type: ActorType.USER, @@ -1252,13 +1252,38 @@ export class NotificationExternalAdapter { lastName: user.lastName, email: user.email, profile: { - displayName: user.profile.displayName, + displayName: this.resolveUserDisplayName(user), url: this.urlGeneratorService.createUrlForUserNameID(user.nameID), }, type: ActorType.USER, }; } + /** + * A notification payload must never be the thing that takes a request — or + * the process — down. + * + * Every caller of the three payload builders that read `profile.displayName` + * loads the user with `relations: { profile: true }`, so a null profile means + * the ROW is incomplete, not that the relation was forgotten. Dereferencing + * it unguarded turned one such row into a `TypeError`, and because + * `notifyPlatformGlobalRoleChange` invokes its builder without `await` and + * without a catch, that rejection reached Node's default + * `--unhandled-rejections=throw` and killed the server outright — observed + * twice during live verification of workspace#027-platform-role-redesign, + * once via role revocation and once via `createDiscussion`. + * + * Falls back to the user's name, then their email, so the notification still + * carries a usable human identifier instead of failing to send. + */ + private resolveUserDisplayName(user: IUser): string { + return ( + user.profile?.displayName || + `${user.firstName ?? ''} ${user.lastName ?? ''}`.trim() || + user.email + ); + } + private getPlatformURL(): string { return this.configService.get('hosting.endpoint_cluster', { infer: true }); }