Skip to content

Commit

Permalink
chore(security events): add the session.destroy security event
Browse files Browse the repository at this point in the history
Because:
 - we want to know when a user signs out of a session

This commit:
 - adds the 'session.destroy' security event
 - updates the inactive account deletion script to query for the event
  • Loading branch information
chenba authored and dschom committed Feb 14, 2025
1 parent 39047a2 commit a88bb11
Show file tree
Hide file tree
Showing 9 changed files with 47 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
SET NAMES utf8mb4 COLLATE utf8mb4_bin;

CALL assertPatchLevel('161');

INSERT INTO securityEventNames(name) VALUES ('session.destroy');

UPDATE dbMetadata SET value = '162' WHERE name = 'schema-patch-level';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- DELETE FROM securityEventNames WHERE name='session.destroy';

-- UPDATE dbMetadata SET value = '161' WHERE name = 'schema-patch-level';
2 changes: 1 addition & 1 deletion packages/db-migrations/databases/fxa/target-patch.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"level": 161
"level": 162
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ export const securityEventsQuery = (uid, activeByDateTimestamp) =>
EVENT_NAMES['account.login'],
EVENT_NAMES['account.password_reset_success'],
EVENT_NAMES['account.password_changed'],
EVENT_NAMES['session.destroy'],
])
.limit(1)
.first();
6 changes: 3 additions & 3 deletions packages/fxa-auth-server/lib/routes/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,11 +584,11 @@ export class AccountHandler {
});
}

await this.db.securityEvent({
ipAddr: request.app.clientAddress,
await this.accountEventsManager.recordSecurityEvent(this.db, {
name: 'account.create',
tokenId: sessionToken.id,
uid: account.uid,
ipAddr: request.app.clientAddress,
tokenId: sessionToken.id,
});

return this.accountCreateResponse({
Expand Down
9 changes: 9 additions & 0 deletions packages/fxa-auth-server/lib/routes/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

'use strict';

const { Container } = require('typedi');
const error = require('../error');
const isA = require('joi');
const requestHelper = require('../routes/utils/request_helper');
Expand All @@ -15,6 +16,7 @@ const NodeRendererBindings =
const SESSION_DOCS = require('../../docs/swagger/session-api').default;
const DESCRIPTION = require('../../docs/swagger/shared/descriptions').default;
const HEX_STRING = validators.HEX_STRING;
const { AccountEventsManager } = require('../account-events');

module.exports = function (
log,
Expand All @@ -41,6 +43,7 @@ module.exports = function (
);

const otpOptions = config.otp;
const accountEventsManager = Container.get(AccountEventsManager);

const routes = [
{
Expand Down Expand Up @@ -89,6 +92,12 @@ module.exports = function (
}

await db.deleteSessionToken(sessionToken);
await accountEventsManager.recordSecurityEvent(db, {
name: 'session.destroy',
uid,
ipAddr: request.app.clientAddress,
tokenId: sessionToken.id,
});

return {};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const securityEventUidsQuery = (activeByDateTimestamp) =>
EVENT_NAMES['account.login'],
EVENT_NAMES['account.password_reset_success'],
EVENT_NAMES['account.password_changed'],
EVENT_NAMES['session.destroy'],
])
.as('securityEventUids');

Expand Down
22 changes: 21 additions & 1 deletion packages/fxa-auth-server/test/local/routes/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const sinon = require('sinon');
const otplib = require('otplib');
const assert = require('../../assert');
const gleanMock = mocks.mockGlean();
const { Container } = require('typedi');
const { AccountEventsManager } = require('../../../lib/account-events');

const ROOT_DIR = '../../..';

Expand Down Expand Up @@ -65,6 +67,11 @@ function makeRoutes(options = {}) {
const glean = options.glean || gleanMock;
const statsd = options.statsd || mocks.mockStatsd();

Container.set(
AccountEventsManager,
options.accountEventsManager || { recordSecurityEvent: sinon.stub() }
);

const Password =
options.Password || require('../../../lib/crypto/password')(log, config);
const customs = options.customs || {
Expand Down Expand Up @@ -715,12 +722,19 @@ describe('/session/destroy', () => {
let request;
let log;
let db;
let securityEventStub;

beforeEach(() => {
db = mocks.mockDB();
log = mocks.mockLog();
const config = {};
const routes = makeRoutes({ log, config, db });
securityEventStub = sinon.stub();
const routes = makeRoutes({
log,
config,
db,
accountEventsManager: { recordSecurityEvent: securityEventStub },
});
route = getRoute(routes, '/session/destroy');
request = mocks.mockRequest({
credentials: {
Expand All @@ -734,6 +748,12 @@ describe('/session/destroy', () => {
it('responds correctly when session is destroyed', () => {
return runTest(route, request).then((res) => {
assert.equal(Object.keys(res).length, 0);
sinon.assert.calledOnceWithExactly(securityEventStub, db, {
name: 'session.destroy',
uid: 'foo',
ipAddr: '63.245.221.32',
tokenId: undefined,
});
});
});

Expand Down
1 change: 1 addition & 0 deletions packages/fxa-shared/db/models/auth/security-event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const EVENT_NAMES = {
'account.primary_secondary_swapped': 23,
'account.password_reset_otp_sent': 24,
'account.password_reset_otp_verified': 25,
'session.destroy': 26,
} as const;

export type SecurityEventNames = keyof typeof EVENT_NAMES;
Expand Down

0 comments on commit a88bb11

Please sign in to comment.