Skip to content

Commit d064d6d

Browse files
LZoogLZoog
authored
Merge pull request #20283 from mozilla/FXA-13288
feat(glean): Capture 'scopes' in oauth access token creation Glean event
2 parents d955f6e + 6a9c88e commit d064d6d

7 files changed

Lines changed: 155 additions & 46 deletions

File tree

‎packages/fxa-auth-server/lib/metrics/glean/index.spec.ts‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
65
import { AppError } from '@fxa/accounts/errors';
76
import { AuthRequest } from '../../types';
87

@@ -691,6 +690,40 @@ describe('Glean server side events', () => {
691690
});
692691

693692
describe('oauth', () => {
693+
describe('tokenCreated', () => {
694+
it('normalizes space-separated scopes to sorted comma-separated', async () => {
695+
const glean = gleanMetrics(config);
696+
await glean.oauth.tokenCreated(request, {
697+
scopes: 'https://identity.mozilla.com/apps/smartwindow profile:uid',
698+
});
699+
expect(gleanMocks['recordAccessTokenCreated']).toHaveBeenCalledTimes(1);
700+
const metrics = gleanMocks['recordAccessTokenCreated'].mock.calls[0][0];
701+
expect(metrics['scopes']).toBe(
702+
'https://identity.mozilla.com/apps/smartwindow,profile:uid'
703+
);
704+
});
705+
706+
it('handles undefined scopes', async () => {
707+
const glean = gleanMetrics(config);
708+
await glean.oauth.tokenCreated(request, {
709+
scopes: undefined,
710+
});
711+
expect(gleanMocks['recordAccessTokenCreated']).toHaveBeenCalledTimes(1);
712+
const metrics = gleanMocks['recordAccessTokenCreated'].mock.calls[0][0];
713+
expect(metrics['scopes']).toBe('');
714+
});
715+
716+
it('handles scopes passed as an array', async () => {
717+
const glean = gleanMetrics(config);
718+
await glean.oauth.tokenCreated(request, {
719+
scopes: ['profile', 'openid'],
720+
});
721+
expect(gleanMocks['recordAccessTokenCreated']).toHaveBeenCalledTimes(1);
722+
const metrics = gleanMocks['recordAccessTokenCreated'].mock.calls[0][0];
723+
expect(metrics['scopes']).toBe('openid,profile');
724+
});
725+
});
726+
694727
describe('tokenChecked', () => {
695728
it('sends an empty ip address', async () => {
696729
const glean = gleanMetrics(config);
@@ -706,8 +739,7 @@ describe('Glean server side events', () => {
706739
scopes: undefined,
707740
});
708741
expect(gleanMocks['recordAccessTokenChecked']).toHaveBeenCalledTimes(1);
709-
const metrics =
710-
gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
742+
const metrics = gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
711743
expect(metrics['scopes']).toBe('');
712744
});
713745

@@ -717,8 +749,7 @@ describe('Glean server side events', () => {
717749
scopes: '',
718750
});
719751
expect(gleanMocks['recordAccessTokenChecked']).toHaveBeenCalledTimes(1);
720-
const metrics =
721-
gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
752+
const metrics = gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
722753
expect(metrics['scopes']).toBe('');
723754
});
724755

@@ -728,8 +759,7 @@ describe('Glean server side events', () => {
728759
scopes: ['profile', 'openid'],
729760
});
730761
expect(gleanMocks['recordAccessTokenChecked']).toHaveBeenCalledTimes(1);
731-
const metrics =
732-
gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
762+
const metrics = gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
733763
expect(metrics['scopes']).toBe('openid,profile');
734764
});
735765

@@ -739,8 +769,7 @@ describe('Glean server side events', () => {
739769
scopes: 'profile,openid',
740770
});
741771
expect(gleanMocks['recordAccessTokenChecked']).toHaveBeenCalledTimes(1);
742-
const metrics =
743-
gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
772+
const metrics = gleanMocks['recordAccessTokenChecked'].mock.calls[0][0];
744773
expect(metrics['scopes']).toBe('openid,profile');
745774
});
746775
});
@@ -893,11 +922,12 @@ describe('Glean server side events', () => {
893922
request: req as any,
894923
error,
895924
});
896-
expect(mockGleanObj.registration.error as jest.Mock).toHaveBeenCalledTimes(1);
897-
expect(mockGleanObj.registration.error as jest.Mock).toHaveBeenCalledWith(
898-
req,
899-
{ reason: 'REQUEST_BLOCKED' }
900-
);
925+
expect(
926+
mockGleanObj.registration.error as jest.Mock
927+
).toHaveBeenCalledTimes(1);
928+
expect(
929+
mockGleanObj.registration.error as jest.Mock
930+
).toHaveBeenCalledWith(req, { reason: 'REQUEST_BLOCKED' });
901931
});
902932
});
903933

‎packages/fxa-auth-server/lib/metrics/glean/index.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,20 @@ export function gleanMetrics(config: ConfigType) {
273273

274274
oauth: {
275275
tokenCreated: createEventFn('access_token_created', {
276-
additionalMetrics: extraKeyReasonCb,
276+
additionalMetrics: (metrics) => ({
277+
reason: metrics.reason ?? '',
278+
scopes: metrics.scopes
279+
? // Array: in at least verify.js, getScopeValues() returns string[]
280+
Array.isArray(metrics.scopes)
281+
? metrics.scopes.sort().join(',')
282+
: // String: in at least token.js, ScopeSet.toString() returns space-separated scopes
283+
metrics.scopes
284+
.split(/[,\s]+/)
285+
.filter(Boolean)
286+
.sort()
287+
.join(',')
288+
: '',
289+
}),
277290
}),
278291
tokenChecked: createEventFn('access_token_checked', {
279292
skipClientIp: true,

‎packages/fxa-auth-server/lib/metrics/glean/server_events.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,7 @@ class EventsServerEventLogger {
409409
* @param {string} utm_source - The source from where the user started. For example, if the user clicked on a link on the Mozilla accounts web site, this value could be 'fx-website'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
410410
* @param {string} utm_term - This metric is similar to the `utm.source`; it is used in the Firefox browser. For example, if the user started from about:welcome, then the value could be 'aboutwelcome-default-screen'. The value has a max length of 128 characters with the alphanumeric characters, _ (underscore), forward slash (/), . (period), % (percentage sign), and - (hyphen) in the allowed set of characters..
411411
* @param {string} reason - additional context-dependent info, e.g. the cause of an error.
412+
* @param {string} scopes - The OAuth scopes granted for the access token, in a comma-separated list.
412413
*/
413414
recordAccessTokenCreated({
414415
user_agent,
@@ -428,6 +429,7 @@ class EventsServerEventLogger {
428429
utm_source,
429430
utm_term,
430431
reason,
432+
scopes,
431433
}: {
432434
user_agent: string;
433435
ip_address: string;
@@ -446,12 +448,14 @@ class EventsServerEventLogger {
446448
utm_source: string;
447449
utm_term: string;
448450
reason: string;
451+
scopes: string;
449452
}) {
450453
const event = {
451454
category: 'access_token',
452455
name: 'created',
453456
extra: {
454457
reason: String(reason),
458+
scopes: String(scopes),
455459
},
456460
};
457461
this.#record({

‎packages/fxa-auth-server/lib/routes/oauth/token.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,7 @@ module.exports = ({ log, oauthDB, db, mailer, devices, statsd, glean }) => {
533533
reason: params.reason
534534
? `${params.grant_type}:${params.reason}`
535535
: params.grant_type,
536+
scopes: grant.scope?.toString() || '',
536537
});
537538

538539
if (tokens.keys_jwe) {

‎packages/fxa-auth-server/lib/routes/oauth/token.spec.ts‎

Lines changed: 83 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import sinon from 'sinon';
66
const Joi = require('joi');
77
const { Container } = require('typedi');
8+
const ScopeSet = require('fxa-shared').oauth.scopes;
89

910
const {
1011
OAUTH_SCOPE_OLD_SYNC,
@@ -24,14 +25,12 @@ const UID = 'eaf0';
2425
const CLIENT_SECRET =
2526
'b93ef8a8f3e553a430d7e5b904c6132b2722633af9f03128029201d24a97f2a8';
2627
const CLIENT_ID = '98e6508e88680e1b';
27-
const CODE =
28-
'df6dcfe7bf6b54a65db5742cbcdce5c0a84a5da81a0bb6bdf5fc793eef041fc6';
28+
const CODE = 'df6dcfe7bf6b54a65db5742cbcdce5c0a84a5da81a0bb6bdf5fc793eef041fc6';
2929
const REFRESH_TOKEN = CODE;
3030
const PKCE_CODE_VERIFIER = 'au3dqDz2dOB0_vSikXCUf4S8Gc-37dL-F7sGxtxpR3R';
3131
const CODE_WITH_KEYS = 'afafaf';
3232
const CODE_WITHOUT_KEYS = 'f0f0f0';
33-
const GRANT_TOKEN_EXCHANGE =
34-
'urn:ietf:params:oauth:grant-type:token-exchange';
33+
const GRANT_TOKEN_EXCHANGE = 'urn:ietf:params:oauth:grant-type:token-exchange';
3534
const SUBJECT_TOKEN_TYPE_REFRESH =
3635
'urn:ietf:params:oauth:token-type:refresh_token';
3736
const FIREFOX_IOS_CLIENT_ID = '1b1a3e44c54fbb58';
@@ -58,10 +57,13 @@ const tokenRoutesDepMocks = {
5857
is: Joi.string().required(),
5958
then: Joi.forbidden(),
6059
}),
61-
clientSecret: Joi.string().hex().required().when('$headers.authorization', {
62-
is: Joi.string().required(),
63-
then: Joi.forbidden(),
64-
}),
60+
clientSecret: Joi.string()
61+
.hex()
62+
.required()
63+
.when('$headers.authorization', {
64+
is: Joi.string().required(),
65+
then: Joi.forbidden(),
66+
}),
6567
},
6668
},
6769
'../../oauth/grant': {
@@ -338,7 +340,51 @@ describe('/token POST', () => {
338340
sinon.assert.calledOnceWithExactly(
339341
mockGlean.oauth.tokenCreated,
340342
request,
341-
{ uid: UID, oauthClientId: CLIENT_ID, reason: 'authorization_code' }
343+
{
344+
uid: UID,
345+
oauthClientId: CLIENT_ID,
346+
reason: 'authorization_code',
347+
scopes: '',
348+
}
349+
);
350+
});
351+
352+
it('logs space-separated scopes from ScopeSet for the token created event', async () => {
353+
const SMARTWINDOW_SCOPES =
354+
'https://identity.mozilla.com/apps/smartwindow profile:uid';
355+
resetAndMockDeps();
356+
jest.doMock('../../oauth/grant', () => ({
357+
generateTokens: tokenRoutesDepMocks['../../oauth/grant'].generateTokens,
358+
validateRequestedGrant: () => ({
359+
offline: true,
360+
userId: buf(UID),
361+
clientId: buf(CLIENT_ID),
362+
scope: ScopeSet.fromString(SMARTWINDOW_SCOPES),
363+
}),
364+
}));
365+
const mockGleanLocal = { oauth: { tokenCreated: sinon.stub() } };
366+
const routes = require('./token')({
367+
...tokenRoutesArgMocks,
368+
glean: mockGleanLocal,
369+
});
370+
const request = {
371+
app: {},
372+
payload: {
373+
client_id: CLIENT_ID,
374+
grant_type: 'fxa-credentials',
375+
},
376+
emitMetricsEvent: () => {},
377+
};
378+
await routes[0].config.handler(request);
379+
sinon.assert.calledOnceWithExactly(
380+
mockGleanLocal.oauth.tokenCreated,
381+
request,
382+
{
383+
uid: UID,
384+
oauthClientId: CLIENT_ID,
385+
reason: 'fxa-credentials',
386+
scopes: SMARTWINDOW_SCOPES,
387+
}
342388
);
343389
});
344390
});
@@ -540,7 +586,8 @@ describe('token exchange grant_type', () => {
540586
canGrant: true,
541587
publicClient: true,
542588
}),
543-
clientAuthValidators: tokenRoutesDepMocks['../../oauth/client'].clientAuthValidators,
589+
clientAuthValidators:
590+
tokenRoutesDepMocks['../../oauth/client'].clientAuthValidators,
544591
}));
545592
jest.doMock('../../oauth/grant', () => ({
546593
generateTokens: (grant: any) => {
@@ -677,8 +724,9 @@ describe('/oauth/token POST', () => {
677724
.rejects(new Error('should not be called'));
678725
jest.resetModules();
679726
jest.doMock('../../oauth/assertion', () => async () => true);
680-
jest.doMock('../../oauth/client', () =>
681-
tokenRoutesDepMocks['../../oauth/client']
727+
jest.doMock(
728+
'../../oauth/client',
729+
() => tokenRoutesDepMocks['../../oauth/client']
682730
);
683731
jest.doMock('../../oauth/grant', () => ({
684732
generateTokens: (grant: any) => ({
@@ -690,8 +738,9 @@ describe('/oauth/token POST', () => {
690738
}),
691739
validateRequestedGrant: () => ({ offline: true, scope: 'testo' }),
692740
}));
693-
jest.doMock('../../oauth/util', () =>
694-
tokenRoutesDepMocks['../../oauth/util']
741+
jest.doMock(
742+
'../../oauth/util',
743+
() => tokenRoutesDepMocks['../../oauth/util']
695744
);
696745
jest.doMock('../utils/oauth', () => ({
697746
newTokenNotification: newTokenNotificationStub,
@@ -758,8 +807,9 @@ describe('/oauth/token POST', () => {
758807
const newTokenNotificationStub = sinon.stub().resolves();
759808
jest.resetModules();
760809
jest.doMock('../../oauth/assertion', () => async () => true);
761-
jest.doMock('../../oauth/client', () =>
762-
tokenRoutesDepMocks['../../oauth/client']
810+
jest.doMock(
811+
'../../oauth/client',
812+
() => tokenRoutesDepMocks['../../oauth/client']
763813
);
764814
jest.doMock('../../oauth/grant', () => ({
765815
generateTokens: (grant: any) => ({
@@ -771,8 +821,9 @@ describe('/oauth/token POST', () => {
771821
}),
772822
validateRequestedGrant: () => ({ offline: true, scope: 'testo' }),
773823
}));
774-
jest.doMock('../../oauth/util', () =>
775-
tokenRoutesDepMocks['../../oauth/util']
824+
jest.doMock(
825+
'../../oauth/util',
826+
() => tokenRoutesDepMocks['../../oauth/util']
776827
);
777828
jest.doMock('../utils/oauth', () => ({
778829
newTokenNotification: newTokenNotificationStub,
@@ -838,20 +889,21 @@ describe('/oauth/token POST', () => {
838889
.rejects(new Error('should not be called'));
839890
jest.resetModules();
840891
jest.doMock('../../oauth/assertion', () => async () => true);
841-
jest.doMock('../../oauth/client', () =>
842-
tokenRoutesDepMocks['../../oauth/client']
892+
jest.doMock(
893+
'../../oauth/client',
894+
() => tokenRoutesDepMocks['../../oauth/client']
843895
);
844896
jest.doMock('../../oauth/grant', () => ({
845-
generateTokens:
846-
tokenRoutesDepMocks['../../oauth/grant'].generateTokens,
897+
generateTokens: tokenRoutesDepMocks['../../oauth/grant'].generateTokens,
847898
validateRequestedGrant: () => ({
848899
offline: true,
849900
scope: OAUTH_SCOPE_SESSION_TOKEN,
850901
clientId: buf(CLIENT_ID),
851902
}),
852903
}));
853-
jest.doMock('../../oauth/util', () =>
854-
tokenRoutesDepMocks['../../oauth/util']
904+
jest.doMock(
905+
'../../oauth/util',
906+
() => tokenRoutesDepMocks['../../oauth/util']
855907
);
856908
jest.doMock('../utils/oauth', () => ({
857909
newTokenNotification: newTokenNotificationStub,
@@ -892,20 +944,21 @@ describe('/oauth/token POST', () => {
892944
const newTokenNotificationStub = sinon.stub().resolves();
893945
jest.resetModules();
894946
jest.doMock('../../oauth/assertion', () => async () => true);
895-
jest.doMock('../../oauth/client', () =>
896-
tokenRoutesDepMocks['../../oauth/client']
947+
jest.doMock(
948+
'../../oauth/client',
949+
() => tokenRoutesDepMocks['../../oauth/client']
897950
);
898951
jest.doMock('../../oauth/grant', () => ({
899-
generateTokens:
900-
tokenRoutesDepMocks['../../oauth/grant'].generateTokens,
952+
generateTokens: tokenRoutesDepMocks['../../oauth/grant'].generateTokens,
901953
validateRequestedGrant: () => ({
902954
offline: true,
903955
scope: 'testo',
904956
clientId: buf(CLIENT_ID),
905957
}),
906958
}));
907-
jest.doMock('../../oauth/util', () =>
908-
tokenRoutesDepMocks['../../oauth/util']
959+
jest.doMock(
960+
'../../oauth/util',
961+
() => tokenRoutesDepMocks['../../oauth/util']
909962
);
910963
jest.doMock('../utils/oauth', () => ({
911964
newTokenNotification: newTokenNotificationStub,

‎packages/fxa-auth-server/test/oauth/routes/token.js‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,12 @@ describe('/token POST', function () {
379379
sinon.assert.calledOnceWithExactly(
380380
mockGlean.oauth.tokenCreated,
381381
request,
382-
{ uid: UID, oauthClientId: CLIENT_ID, reason: 'authorization_code' }
382+
{
383+
uid: UID,
384+
oauthClientId: CLIENT_ID,
385+
reason: 'authorization_code',
386+
scopes: '',
387+
}
383388
);
384389
});
385390
});

0 commit comments

Comments
 (0)