Skip to content

Commit e25556b

Browse files
facundo-herodevsfacundo-herodevs
authored
fix: use NES user id for cli amplitude events (#526)
1 parent 2121def commit e25556b

2 files changed

Lines changed: 84 additions & 23 deletions

File tree

‎src/service/analytics.svc.ts‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,12 @@ export async function refreshIdentityFromStoredToken(): Promise<boolean> {
9595
return false;
9696
}
9797

98-
const entries = toIdentityEntries(claims);
99-
const signature = buildIdentitySignature(entries);
98+
const signature = buildIdentitySignature(claims);
10099
if (signature === lastIdentitySignature) {
101100
return false;
102101
}
103102
applyIdentityClaims(claims, signature);
104-
emitIdentify(entries, claims.user_id);
103+
emitIdentify(claims);
105104
return true;
106105
} catch (error) {
107106
logAnalyticsError('refreshIdentityFromStoredToken', error);
@@ -151,10 +150,15 @@ function buildIdentifyEventOptions(userId?: string) {
151150
}
152151

153152
function normalizeClaim(value: unknown): string {
154-
if (typeof value !== 'string') {
155-
return '';
153+
if (typeof value === 'string') {
154+
return value.trim();
156155
}
157-
return value.trim();
156+
157+
if (typeof value === 'number' && Number.isFinite(value)) {
158+
return String(value);
159+
}
160+
161+
return '';
158162
}
159163

160164
function extractIdentityClaims(accessToken: string | undefined): IdentityClaims | undefined {
@@ -163,8 +167,10 @@ function extractIdentityClaims(accessToken: string | undefined): IdentityClaims
163167
return;
164168
}
165169

170+
const nesClaims = payload.nes as { identity?: unknown } | undefined;
171+
166172
const identity: IdentityClaims = {
167-
user_id: normalizeClaim(payload.sub) || undefined,
173+
user_id: normalizeClaim(nesClaims?.identity) || undefined,
168174
email: normalizeClaim(payload.email) || undefined,
169175
organization_name: normalizeClaim(payload.company) || undefined,
170176
role: normalizeClaim(payload.role) || undefined,
@@ -203,8 +209,10 @@ function toIdentityEntries(identity: IdentityClaims): Array<[keyof IdentityClaim
203209
return entries;
204210
}
205211

206-
function buildIdentitySignature(entries: Array<[keyof IdentityClaims, string]>): string {
207-
return entries.map(([field, value]) => `${field}:${value}`).join('|');
212+
function buildIdentitySignature(identity: IdentityClaims): string {
213+
return toIdentityEntries(identity)
214+
.map(([field, value]) => `${field}:${value}`)
215+
.join('|');
208216
}
209217

210218
function applyIdentityClaims(claims: IdentityClaims, signature: string): void {
@@ -213,13 +221,16 @@ function applyIdentityClaims(claims: IdentityClaims, signature: string): void {
213221
analyticsContext = { ...analyticsContext, ...claims };
214222
}
215223

216-
function emitIdentify(entries: Array<[keyof IdentityClaims, string]>, userId?: string): void {
224+
function emitIdentify(claims: IdentityClaims): void {
217225
const amplitudeIdentify = new Identify();
218-
for (const [field, value] of entries) {
219-
amplitudeIdentify.set(field, value);
226+
for (const field of IDENTITY_FIELDS) {
227+
const value = claims[field];
228+
if (value) {
229+
amplitudeIdentify.set(field, value);
230+
}
220231
}
221232

222-
const eventOptions = buildIdentifyEventOptions(userId);
233+
const eventOptions = buildIdentifyEventOptions(claims.user_id);
223234
void toSafeAnalyticsResult(identify(amplitudeIdentify, eventOptions), 'identify').promise;
224235
void toSafeAnalyticsResult(_track('Identify Call', { source: SOURCE }, eventOptions), 'track:Identify Call').promise;
225236
}

‎test/service/analytics.svc.test.ts‎

Lines changed: 60 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ describe('analytics.svc', () => {
125125
mockAuthTokenService.getStoredTokens.resolves({
126126
accessToken: createAccessToken({
127127
sub: 'user-1',
128+
nes: { identity: 'nes-user-1' },
128129
email: 'dev@herodevs.com',
129130
company: 'HeroDevs',
130131
role: 'Software Engineer',
@@ -138,7 +139,7 @@ describe('analytics.svc', () => {
138139
expect(mockAmplitude.track.getCall(0).args[0]).toBe('Identify Call');
139140

140141
const metadata = mockAmplitude.identify.getCall(0).args[1];
141-
expect(metadata.user_id).toBe('user-1');
142+
expect(metadata.user_id).toBe('nes-user-1');
142143
expect(typeof metadata.platform).toBe('string');
143144
expect(typeof metadata.os_name).toBe('string');
144145
expect(typeof metadata.os_version).toBe('string');
@@ -148,6 +149,7 @@ describe('analytics.svc', () => {
148149
it('should use env CI token identity when stored token is unavailable', async () => {
149150
mockConfig.ciTokenFromEnv = createAccessToken({
150151
sub: 'env-user-1',
152+
nes: { identity: 'nes-env-user-1' },
151153
email: 'env@herodevs.com',
152154
company: 'HeroDevs',
153155
role: 'Platform Engineer',
@@ -159,8 +161,8 @@ describe('analytics.svc', () => {
159161
expect(mockAmplitude.identify.calledOnce).toBe(true);
160162
expect(mockAmplitude.track.calledOnce).toBe(true);
161163
expect(mockAmplitude.track.getCall(0).args[0]).toBe('Identify Call');
162-
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('env-user-1');
163-
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBe('env-user-1');
164+
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('nes-env-user-1');
165+
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBe('nes-env-user-1');
164166
});
165167

166168
it('should handle case when npm_package_version is undefined', async () => {
@@ -302,6 +304,7 @@ describe('analytics.svc', () => {
302304
mockAuthTokenService.getStoredTokens.resolves({
303305
accessToken: createAccessToken({
304306
sub: 'user-123',
307+
nes: { identity: 'nes-user-123' },
305308
email: 'dev@herodevs.com',
306309
company: 'HeroDevs',
307310
role: 'Software Engineer',
@@ -313,7 +316,7 @@ describe('analytics.svc', () => {
313316

314317
expect(mockAmplitude.track.callCount).toBe(2);
315318
const trackCall = mockAmplitude.track.getCall(1);
316-
expect(trackCall.args[2].user_id).toBe('user-123');
319+
expect(trackCall.args[2].user_id).toBe('nes-user-123');
317320
});
318321

319322
it('should not throw when amplitude track throws synchronously', async () => {
@@ -351,6 +354,7 @@ describe('analytics.svc', () => {
351354
mockAuthTokenService.getStoredTokens.resolves({
352355
accessToken: createAccessToken({
353356
sub: 'user-1',
357+
nes: { identity: 'nes-user-1' },
354358
email: 'dev@herodevs.com',
355359
company: 'HeroDevs',
356360
role: 'Software Engineer',
@@ -365,12 +369,30 @@ describe('analytics.svc', () => {
365369
expect(identifyBuilder.set.calledWith('email', 'dev@herodevs.com')).toBe(true);
366370
expect(identifyBuilder.set.calledWith('organization_name', 'HeroDevs')).toBe(true);
367371
expect(identifyBuilder.set.calledWith('role', 'Software Engineer')).toBe(true);
368-
expect(identifyBuilder.set.calledWith('user_id', 'user-1')).toBe(true);
372+
expect(identifyBuilder.set.calledWith('user_id', 'nes-user-1')).toBe(true);
369373

370374
const identifyEventCall = mockAmplitude.track.getCall(0);
371375
expect(identifyEventCall.args[0]).toBe('Identify Call');
372376
expect(identifyEventCall.args[1]).toEqual({ source: 'cli' });
373-
expect(identifyEventCall.args[2].user_id).toBe('user-1');
377+
expect(identifyEventCall.args[2].user_id).toBe('nes-user-1');
378+
});
379+
380+
it('should prefer nes.identity over sub when both are present', async () => {
381+
const mod = await setupModule();
382+
mockAuthTokenService.getStoredTokens.resolves({
383+
accessToken: createAccessToken({
384+
sub: 'keycloak-user-1',
385+
nes: { identity: 'nes-user-1' },
386+
email: 'dev@herodevs.com',
387+
}),
388+
});
389+
390+
await mod.refreshIdentityFromStoredToken();
391+
392+
expect(mockAmplitude.identify.calledOnce).toBe(true);
393+
expect(mockAmplitude.track.calledOnce).toBe(true);
394+
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('nes-user-1');
395+
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBe('nes-user-1');
374396
});
375397

376398
it('should ignore non-canonical claim aliases for identity mapping', async () => {
@@ -401,9 +423,34 @@ describe('analytics.svc', () => {
401423
expect(mockAmplitude.track.called).toBe(false);
402424
});
403425

426+
it('should not fall back to sub when nes.identity is missing', async () => {
427+
const mod = await setupModule();
428+
mockAuthTokenService.getStoredTokens.resolves({
429+
accessToken: createAccessToken({
430+
sub: 'keycloak-user-1',
431+
email: 'dev@herodevs.com',
432+
company: 'HeroDevs',
433+
role: 'Software Engineer',
434+
}),
435+
});
436+
437+
await mod.refreshIdentityFromStoredToken();
438+
439+
expect(mockAmplitude.identify.calledOnce).toBe(true);
440+
expect(mockAmplitude.track.calledOnce).toBe(true);
441+
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBeUndefined();
442+
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBeUndefined();
443+
const identifyBuilder = mockAmplitude.Identify.getCall(0).returnValue as { set: sinon.SinonStub };
444+
expect(identifyBuilder.set.calledWith('email', 'dev@herodevs.com')).toBe(true);
445+
expect(identifyBuilder.set.calledWith('organization_name', 'HeroDevs')).toBe(true);
446+
expect(identifyBuilder.set.calledWith('role', 'Software Engineer')).toBe(true);
447+
expect(mockAmplitude.track.getCall(0).args[1]).toEqual({ source: 'cli' });
448+
});
449+
404450
it('should fall back to env CI token when stored token payload is malformed', async () => {
405451
mockConfig.ciTokenFromEnv = createAccessToken({
406452
sub: 'env-fallback-user',
453+
nes: { identity: 'nes-env-fallback-user' },
407454
email: 'env-fallback@herodevs.com',
408455
company: 'HeroDevs',
409456
role: 'Engineer',
@@ -417,18 +464,20 @@ describe('analytics.svc', () => {
417464

418465
expect(mockAmplitude.identify.calledOnce).toBe(true);
419466
expect(mockAmplitude.track.calledOnce).toBe(true);
420-
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('env-fallback-user');
467+
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('nes-env-fallback-user');
421468
});
422469

423470
it('should prefer stored token identity over env CI token identity', async () => {
424471
mockConfig.ciTokenFromEnv = createAccessToken({
425472
sub: 'env-user',
473+
nes: { identity: 'nes-env-user' },
426474
email: 'env@herodevs.com',
427475
});
428476
const mod = await setupModule();
429477
mockAuthTokenService.getStoredTokens.resolves({
430478
accessToken: createAccessToken({
431479
sub: 'stored-user',
480+
nes: { identity: 'nes-stored-user' },
432481
email: 'stored@herodevs.com',
433482
company: 'HeroDevs',
434483
}),
@@ -438,15 +487,16 @@ describe('analytics.svc', () => {
438487

439488
expect(mockAmplitude.identify.calledOnce).toBe(true);
440489
expect(mockAmplitude.track.calledOnce).toBe(true);
441-
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('stored-user');
442-
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBe('stored-user');
490+
expect(mockAmplitude.identify.getCall(0).args[1].user_id).toBe('nes-stored-user');
491+
expect(mockAmplitude.track.getCall(0).args[2].user_id).toBe('nes-stored-user');
443492
});
444493

445494
it('should clear cached identity when no claims are available', async () => {
446495
const mod = await setupModule();
447496
mockAuthTokenService.getStoredTokens.resolves({
448497
accessToken: createAccessToken({
449498
sub: 'user-1',
499+
nes: { identity: 'nes-user-1' },
450500
email: 'dev@herodevs.com',
451501
company: 'HeroDevs',
452502
role: 'Software Engineer',
@@ -455,7 +505,7 @@ describe('analytics.svc', () => {
455505

456506
await mod.refreshIdentityFromStoredToken();
457507
mod.track('authenticated-event');
458-
expect(mockAmplitude.track.getCall(1).args[2].user_id).toBe('user-1');
508+
expect(mockAmplitude.track.getCall(1).args[2].user_id).toBe('nes-user-1');
459509

460510
mockAuthTokenService.getStoredTokens.resolves(undefined);
461511
await mod.refreshIdentityFromStoredToken();

0 commit comments

Comments
 (0)