Skip to content

chore(fxa-shared): Dedupe ConnectedServicesFactory builder #19593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

chore(fxa-shared): Dedupe ConnectedServicesFactory builder #19593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39f5629
chore(fxa-shared): Dedupe ConnectedServicesFactory build
nshirley Oct 23, 2025
f7278fb
update attached-clients route tests
nshirley Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 11 additions & 21 deletions packages/fxa-auth-server/test/local/routes/attached-clients.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,15 @@ describe('/account/attached_clients', () => {
request.auth.credentials.id = SESSIONS[0].id;
const result = await route(request);

assert.equal(result.length, 6);
console.debug('Result:', result);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug


// Even though there are 7 potential clients (devices + oauth clients),
// the service deduplicates them, so we only see 5 clients in the result.
// Specifically:
// - DEVICES[2] (device with both sessionTokenId and refreshTokenId)
// - OAUTH_CLIENTS[3] (OAuth Mega-Device linked to DEVICES[2].refreshTokenId)
// These two records get merged into a single AttachedClient instead of appearing twice.
assert.equal(result.length, 5);

assert.equal(db.touchSessionToken.callCount, 1);
const args = db.touchSessionToken.args[0];
Expand Down Expand Up @@ -258,26 +266,8 @@ describe('/account/attached_clients', () => {
userAgent: '',
os: null,
});
// The cloud OAuth service using only access tokens.
assert.deepEqual(result[3], {
clientId: OAUTH_CLIENTS[0].client_id,
deviceId: null,
sessionTokenId: null,
refreshTokenId: null,
isCurrentSession: false,
deviceType: null,
name: 'Legacy OAuth Service',
createdTime: now - 1600,
createdTimeFormatted: 'a few seconds ago',
lastAccessTime: now - 200,
lastAccessTimeFormatted: 'a few seconds ago',
scope: ['a', 'b'],
location: {},
userAgent: '',
os: null,
});
// The cloud OAuth service using a refresh token.
assert.deepEqual(result[4], {
assert.deepEqual(result[3], {
clientId: OAUTH_CLIENTS[1].client_id,
deviceId: null,
sessionTokenId: null,
Expand All @@ -295,7 +285,7 @@ describe('/account/attached_clients', () => {
os: null,
});
// The web-only login session.
assert.deepEqual(result[5], {
assert.deepEqual(result[4], {
clientId: null,
deviceId: null,
sessionTokenId: SESSIONS[0].id,
Expand Down
116 changes: 91 additions & 25 deletions packages/fxa-shared/connected-services/factories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export interface IConnectedServicesFactoryBindings extends IClientFormatter {
export class ConnectedServicesFactory {
protected clientsBySessionTokenId = new Map<string, AttachedClient>();
protected clientsByRefreshTokenId = new Map<string, AttachedClient>();
protected clientsByDeviceId = new Map<string, AttachedClient>();
protected attachedClients: AttachedClient[] = [];

constructor(protected readonly bindings: IConnectedServicesFactoryBindings) {}
Expand All @@ -143,6 +144,7 @@ export class ConnectedServicesFactory {
this.attachedClients = [];
this.clientsBySessionTokenId = new Map<string, AttachedClient>();
this.clientsByRefreshTokenId = new Map<string, AttachedClient>();
this.clientsByDeviceId = new Map<string, AttachedClient>();
}

/**
Expand Down Expand Up @@ -178,6 +180,11 @@ export class ConnectedServicesFactory {

protected async mergeSessions(sessionTokenId: string) {
for (const session of await this.bindings.sessions()) {
if (!session.id) {
// on the off chance a session without an ID is returned, skip it.
continue;
}

let client = this.clientsBySessionTokenId.get(session.id);
if (!client) {
client = {
Expand All @@ -186,6 +193,15 @@ export class ConnectedServicesFactory {
createdTime: session.createdAt,
};
this.attachedClients.push(client);

this.clientsBySessionTokenId.set(session.id, client);
} else {
if (!client.sessionTokenId) {
client.sessionTokenId = session.id;
}
if (!this.clientsBySessionTokenId.has(session.id)) {
this.clientsBySessionTokenId.set(session.id, client);
}
}

client.createdTime = Math.min(
Expand All @@ -205,15 +221,25 @@ export class ConnectedServicesFactory {

// Location, OS and UA are currently only available on sessionTokens, so we can
// copy across without worrying about merging with data from the device record.
client.location = session.location ? { ...session.location } : null;
client.os = session.uaOS || null;
if (!session.uaBrowser) {
// Only update if the session has the data (to avoid overwriting with empty values from duplicate rows)
if (session.location) {
client.location = { ...session.location };
}
if (session.uaOS) {
client.os = session.uaOS;
}

// Only set userAgent if session has browser info
if (session.uaBrowser) {
if (!session.uaBrowserVersion) {
client.userAgent = session.uaBrowser;
} else {
const { uaBrowser: browser, uaBrowserVersion: version } = session;
client.userAgent = `${browser} ${version.split('.')[0]}`;
}
} else if (!client.userAgent) {
// Only set empty if client doesn't already have a userAgent
client.userAgent = '';
} else if (!session.uaBrowserVersion) {
client.userAgent = session.uaBrowser;
} else {
const { uaBrowser: browser, uaBrowserVersion: version } = session;
client.userAgent = `${browser} ${version.split('.')[0]}`;
}

if (!client.name) {
Expand All @@ -224,6 +250,9 @@ export class ConnectedServicesFactory {

protected async mergeOauthClients() {
for (const oauthClient of await this.bindings.oauthClients()) {
if (!oauthClient.refresh_token_id) {
continue;
}
let client = this.clientsByRefreshTokenId.get(
oauthClient.refresh_token_id
);
Expand All @@ -237,7 +266,9 @@ export class ConnectedServicesFactory {
lastAccessTime: oauthClient.last_access_time,
};
this.attachedClients.push(client);
this.clientsByRefreshTokenId.set(oauthClient.refresh_token_id, client);
}

client.clientId = oauthClient.client_id;
client.scope = oauthClient.scope;
client.createdTime = Math.min(
Expand All @@ -263,25 +294,60 @@ export class ConnectedServicesFactory {

protected async mergeDevices() {
for (const device of await this.bindings.deviceList()) {
const client: AttachedClient = {
...this.getDefaultClientFields(),
sessionTokenId: device.sessionTokenId || null,
// The refreshTokenId might be a dangling pointer, don't set it
// until we know whether the corresponding token exists in the OAuth db.
refreshTokenId: null,
deviceId: device.id,
deviceType: device.type,
name: device.name,
createdTime: device.createdAt,
lastAccessTime: device.lastAccessTime,
};
this.attachedClients.push(client);
if (device.sessionTokenId) {
this.clientsBySessionTokenId.set(device.sessionTokenId, client);
if (!device.id) {
// on the off chance a device without an ID is returned, skip it.
continue;
}
if (device.refreshTokenId) {
this.clientsByRefreshTokenId.set(device.refreshTokenId, client);

// Since the device record is returned via the accountDevices_17 stored procedure,
// the device.id is a Buffer object. We need to convert it to a hex string for the Map key.
const deviceIdHex = hex(device.id);
const client = this.clientsByDeviceId.get(deviceIdHex);

if (!client) {
const client: AttachedClient = {
...this.getDefaultClientFields(),
sessionTokenId: device.sessionTokenId || null,
// The refreshTokenId might be a dangling pointer, don't set it
// until we know whether the corresponding token exists in the OAuth db.
refreshTokenId: null,
deviceId: device.id,
deviceType: device.type,
name: device.name,
createdTime: device.createdAt,
lastAccessTime: device.lastAccessTime,
};
this.attachedClients.push(client);

this.clientsByDeviceId.set(deviceIdHex, client);

if (device.sessionTokenId) {
this.clientsBySessionTokenId.set(device.sessionTokenId, client);
}
if (device.refreshTokenId) {
this.clientsByRefreshTokenId.set(device.refreshTokenId, client);
}
} else {
// otherwise, we have record of the client for this device and we
// can update the client with the new information.
if (device.sessionTokenId) {
client.sessionTokenId = device.sessionTokenId;
this.clientsBySessionTokenId.set(device.sessionTokenId, client);
}
if (device.refreshTokenId) {
client.refreshTokenId = device.refreshTokenId;
this.clientsByRefreshTokenId.set(device.refreshTokenId, client);
}
client.createdTime = Math.min(
client.createdTime || Number.POSITIVE_INFINITY,
device.createdAt || Number.POSITIVE_INFINITY
);
client.lastAccessTime = Math.max(
client.lastAccessTime || 0,
device.lastAccessTime || 0
);
}
``;
}
}

Expand Down
Loading