From b3e66e82574339342b72804d952f5ff1ec78cba3 Mon Sep 17 00:00:00 2001 From: cstns Date: Thu, 22 Oct 2026 11:40:51 +0300 Subject: [PATCH] Don't fail concurrent comms credential requests for the same tab session createClient deleted the existing client then inserted a new one, so two requests for the same username could both pass the delete and the second insert hit the primary key. Upsert instead, so the last request wins as it already does when they arrive one after the other. Closes #9019 --- forge/db/controllers/BrokerClient.js | 3 ++- .../forge/db/controllers/BrokerClient_spec.js | 19 +++++++++-------- test/unit/forge/routes/api/team_spec.js | 21 +++++++++++++++++++ 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/forge/db/controllers/BrokerClient.js b/forge/db/controllers/BrokerClient.js index fcd2677e7d..0a8419f279 100644 --- a/forge/db/controllers/BrokerClient.js +++ b/forge/db/controllers/BrokerClient.js @@ -8,7 +8,8 @@ async function createClient (app, { username, ownerId, ownerType, prefix, where, await existingClient.destroy() } const password = generateToken(32, prefix) - await app.db.models.BrokerClient.create({ username, password, ownerId, ownerType }) + // Upsert, as a concurrent request for the same username can recreate it after the destroy above + await app.db.models.BrokerClient.upsert({ username, password, ownerId, ownerType }) const url = usePublicUrl ? (app.config.broker.public_url || app.config.broker.url || null) : (app.config.broker.url || null) diff --git a/test/unit/forge/db/controllers/BrokerClient_spec.js b/test/unit/forge/db/controllers/BrokerClient_spec.js index 2615357a27..c88e75eb3c 100644 --- a/test/unit/forge/db/controllers/BrokerClient_spec.js +++ b/test/unit/forge/db/controllers/BrokerClient_spec.js @@ -20,6 +20,7 @@ describe('BrokerClient', function () { models: { BrokerClient: { create: sandbox.stub(), + upsert: sandbox.stub(), destroy: sandbox.stub(), findOne: sandbox.stub(), findOrCreate: sandbox.stub(), @@ -134,7 +135,7 @@ describe('BrokerClient', function () { it('should create broker client for team frontend', async function () { const fakeClient = fakeBrokerClient() app.db.models.BrokerClient.findOne.resolves(null) - app.db.models.BrokerClient.create.resolves(fakeClient) + app.db.models.BrokerClient.upsert.resolves(fakeClient) const user = { id: 7, hashid: 'userHash' } const team = { id: 11, hashid: 'teamHash' } const sessionId = 'tab-1234567890' @@ -144,7 +145,7 @@ describe('BrokerClient', function () { should(result).have.property('password') result.password.should.match(/^ffbtf_/) should(result).have.property('url', 'http://public.url') - app.db.models.BrokerClient.create.calledWith({ + app.db.models.BrokerClient.upsert.calledWith({ username: expectedUsername, password: result.password, ownerId: '' + user.id, @@ -154,7 +155,7 @@ describe('BrokerClient', function () { it('should destroy existing client for same session before creating a new one', async function () { const existingClient = fakeBrokerClient() app.db.models.BrokerClient.findOne.resolves(existingClient) - app.db.models.BrokerClient.create.resolves(fakeBrokerClient()) + app.db.models.BrokerClient.upsert.resolves(fakeBrokerClient()) const user = { id: 7, hashid: 'userHash' } const team = { id: 11, hashid: 'teamHash' } const result = await BrokerClient.createClientForTeamFrontend(app, user, team, 'tab-1234567890') @@ -163,7 +164,7 @@ describe('BrokerClient', function () { }) it('should look up existing client scoped to the full username (including sessionId) so different tabs do not revoke each other', async function () { app.db.models.BrokerClient.findOne.resolves(null) - app.db.models.BrokerClient.create.resolves(fakeBrokerClient()) + app.db.models.BrokerClient.upsert.resolves(fakeBrokerClient()) const user = { id: 7, hashid: 'userHash' } const team = { id: 11, hashid: 'teamHash' } await BrokerClient.createClientForTeamFrontend(app, user, team, 'tab-A') @@ -173,7 +174,7 @@ describe('BrokerClient', function () { }) it('should include a last will that clears the tab presence entry', async function () { app.db.models.BrokerClient.findOne.resolves(null) - app.db.models.BrokerClient.create.resolves(fakeBrokerClient()) + app.db.models.BrokerClient.upsert.resolves(fakeBrokerClient()) const user = { id: 7, hashid: 'userHash' } const team = { id: 11, hashid: 'teamHash' } const sessionId = 'tab-1234567890' @@ -194,7 +195,7 @@ describe('BrokerClient', function () { it('should create broker client for expert client', async function () { const fakeClient = fakeBrokerClient() app.db.models.BrokerClient.findOne.resolves(null) // no existing client - app.db.models.BrokerClient.create.resolves(fakeClient) + app.db.models.BrokerClient.upsert.resolves(fakeClient) const user = { id: 123, hashid: 'abc123' } const sessionId = 'sess123' const result = await BrokerClient.createClientForExpertClient(app, user, sessionId) @@ -202,7 +203,7 @@ describe('BrokerClient', function () { should(result).have.property('username', expectedUsername) should(result).have.property('password') result.password.should.match(/^ffbec_/) // check it has the right prefix - app.db.models.BrokerClient.create.calledWith({ + app.db.models.BrokerClient.upsert.calledWith({ username: expectedUsername, password: result.password, ownerId: '' + user.id, @@ -212,7 +213,7 @@ describe('BrokerClient', function () { it('should destroy existing client before creating new one', async function () { const fakeClient = fakeBrokerClient() app.db.models.BrokerClient.findOne.resolves(fakeClient) // existing client found - app.db.models.BrokerClient.create.resolves(fakeClient) + app.db.models.BrokerClient.upsert.resolves(fakeClient) const user = { id: 123, hashid: 'abc123' } const sessionId = 'sess123' const result = await BrokerClient.createClientForExpertClient(app, user, sessionId) @@ -221,7 +222,7 @@ describe('BrokerClient', function () { should(result).have.property('password') result.password.should.match(/^ffbec_/) // check it has the right prefix fakeClient.destroy.called.should.be.true() // should have destroyed existing client - app.db.models.BrokerClient.create.calledWith({ + app.db.models.BrokerClient.upsert.calledWith({ username: expectedUsername, password: result.password, ownerId: '' + user.id, diff --git a/test/unit/forge/routes/api/team_spec.js b/test/unit/forge/routes/api/team_spec.js index d495ebf3f9..4aebef6f2d 100644 --- a/test/unit/forge/routes/api/team_spec.js +++ b/test/unit/forge/routes/api/team_spec.js @@ -2030,6 +2030,27 @@ describe('Team API', function () { result.will.should.have.property('payload') should.doesNotThrow(() => JSON.parse(result.will.payload)) }) + it('issues credentials to concurrent requests for the same session', async function () { + // Tabs that share sessionStorage (a restored or duplicated tab) ask for + // the same username at the same moment + const responses = await Promise.all([1, 2, 3, 4].map(() => app.inject({ + method: 'POST', + url: `/api/v1/teams/${TestObjects.ATeam.hashid}/comms-credentials`, + payload: { sessionId: 'tab-concurrent' }, + cookies: { sid: TestObjects.tokens.bob } + }))) + responses.map(r => r.statusCode).should.eql([200, 200, 200, 200]) + const clients = await app.db.models.BrokerClient.findAll({ + where: { username: `fe-team:${TestObjects.bob.hashid}:${TestObjects.ATeam.hashid}:tab-concurrent` } + }) + clients.should.have.length(1) + // The last credentials written are the ones that still authenticate + const results = await Promise.all(responses.map(r => { + const { username, password } = r.json() + return app.db.controllers.BrokerClient.authenticateCredentials(username, password) + })) + results.filter(Boolean).should.have.length(1) + }) it('rejects a non-member with 404', async function () { const response = await app.inject({ method: 'POST',