From c6d08647c73c777e1f442224d3c4291f1842ea11 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 4 Jun 2020 15:52:13 +0100 Subject: [PATCH 1/2] [misc] socket.io: use a secondary publicId for public facing usages --- .../coffee/DocumentUpdaterController.coffee | 2 +- services/real-time/app/coffee/Router.coffee | 4 ++- .../app/coffee/WebsocketController.coffee | 12 +++---- .../app/coffee/WebsocketLoadBalancer.coffee | 2 +- services/real-time/package.json | 1 + .../acceptance/coffee/ApplyUpdateTests.coffee | 6 ++-- .../coffee/ClientTrackingTests.coffee | 6 ++-- .../acceptance/coffee/JoinProjectTests.coffee | 2 +- .../coffee/LeaveProjectTests.coffee | 4 +-- .../coffee/ReceiveUpdateTests.coffee | 4 +-- .../coffee/helpers/RealTimeClient.coffee | 2 ++ .../DocumentUpdaterControllerTests.coffee | 2 +- .../coffee/WebsocketControllerTests.coffee | 33 ++++++++++--------- .../unit/coffee/helpers/MockClient.coffee | 1 + 14 files changed, 44 insertions(+), 37 deletions(-) diff --git a/services/real-time/app/coffee/DocumentUpdaterController.coffee b/services/real-time/app/coffee/DocumentUpdaterController.coffee index 2611d484ad..24b6a7c525 100644 --- a/services/real-time/app/coffee/DocumentUpdaterController.coffee +++ b/services/real-time/app/coffee/DocumentUpdaterController.coffee @@ -69,7 +69,7 @@ module.exports = DocumentUpdaterController = # send messages only to unique clients (due to duplicate entries in io.sockets.clients) for client in clientList when not seen[client.id] seen[client.id] = true - if client.id == update.meta.source + if client.publicId == update.meta.source logger.log doc_id: doc_id, version: update.v, source: update.meta?.source, "distributing update to sender" client.emit "otUpdateApplied", v: update.v, doc: update.doc else if !update.dup # Duplicate ops should just be sent back to sending client for acknowledgement diff --git a/services/real-time/app/coffee/Router.coffee b/services/real-time/app/coffee/Router.coffee index fcdb0c93dc..0266c71eec 100644 --- a/services/real-time/app/coffee/Router.coffee +++ b/services/real-time/app/coffee/Router.coffee @@ -6,6 +6,7 @@ HttpController = require "./HttpController" HttpApiController = require "./HttpApiController" Utils = require "./Utils" bodyParser = require "body-parser" +base64id = require("base64id") basicAuth = require('basic-auth-connect') httpAuth = basicAuth (user, pass)-> @@ -67,7 +68,8 @@ module.exports = Router = return # send positive confirmation that the client has a valid connection - client.emit("connectionAccepted") + client.publicId = base64id.generateId() + client.emit("connectionAccepted", null, client.publicId) metrics.inc('socket-io.connection') metrics.gauge('socket-io.clients', io.sockets.clients()?.length) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 17d27b15b6..4cc3a26daa 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -45,7 +45,7 @@ module.exports = WebsocketController = callback null, project, privilegeLevel, WebsocketController.PROTOCOL_VERSION # No need to block for setting the user as connected in the cursor tracking - ConnectedUsersManager.updateUserPosition project_id, client.id, user, null, () -> + ConnectedUsersManager.updateUserPosition project_id, client.publicId, user, null, () -> # We want to flush a project if there are no more (local) connected clients # but we need to wait for the triggering client to disconnect. How long we wait @@ -58,10 +58,10 @@ module.exports = WebsocketController = metrics.inc "editor.leave-project" logger.log {project_id, user_id, client_id: client.id}, "client leaving project" - WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id + WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.publicId # We can do this in the background - ConnectedUsersManager.markUserAsDisconnected project_id, client.id, (err) -> + ConnectedUsersManager.markUserAsDisconnected project_id, client.publicId, (err) -> if err? logger.error {err, project_id, user_id, client_id: client.id}, "error marking client as disconnected" @@ -143,7 +143,7 @@ module.exports = WebsocketController = if error? logger.warn {err: error, client_id: client.id, project_id, user_id}, "silently ignoring unauthorized updateClientPosition. Client likely hasn't called joinProject yet." return callback() - cursorData.id = client.id + cursorData.id = client.publicId cursorData.user_id = user_id if user_id? cursorData.email = email if email? # Don't store anonymous users in redis to avoid influx @@ -159,7 +159,7 @@ module.exports = WebsocketController = last_name else "" - ConnectedUsersManager.updateUserPosition(project_id, client.id, { + ConnectedUsersManager.updateUserPosition(project_id, client.publicId, { first_name: first_name, last_name: last_name, email: email, @@ -205,7 +205,7 @@ module.exports = WebsocketController = , 100 return callback(error) update.meta ||= {} - update.meta.source = client.id + update.meta.source = client.publicId update.meta.user_id = user_id metrics.inc "editor.doc-update", 0.3 diff --git a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee index 12b7ef812b..9508ac8714 100644 --- a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee +++ b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee @@ -72,7 +72,7 @@ module.exports = WebsocketLoadBalancer = clientList = io.sockets.clients(message.room_id) logger.log {channel:channel, message: message.message, room_id: message.room_id, message_id: message._id, socketIoClients: (client.id for client in clientList)}, "refreshing client list" for client in clientList - ConnectedUsersManager.refreshClient(message.room_id, client.id) + ConnectedUsersManager.refreshClient(message.room_id, client.publicId) else if message.room_id? if message._id? && Settings.checkEventOrder status = EventLogger.checkEventOrder("editor-events", message._id, message) diff --git a/services/real-time/package.json b/services/real-time/package.json index 1e20ac28fc..356a78e712 100644 --- a/services/real-time/package.json +++ b/services/real-time/package.json @@ -22,6 +22,7 @@ }, "dependencies": { "async": "^0.9.0", + "base64id": "0.1.0", "basic-auth-connect": "^1.0.0", "body-parser": "^1.12.0", "connect-redis": "^2.1.0", diff --git a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee index 7820a090d8..e7babeb81c 100644 --- a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee +++ b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee @@ -55,7 +55,7 @@ describe "applyOtUpdate", -> update = JSON.parse(update) update.op.should.deep.equal @update.op update.meta.should.deep.equal { - source: @client.socket.sessionid + source: @client.publicId user_id: @user_id } done() @@ -208,7 +208,7 @@ describe "applyOtUpdate", -> update = JSON.parse(update) update.op.should.deep.equal @comment_update.op update.meta.should.deep.equal { - source: @client.socket.sessionid + source: @client.publicId user_id: @user_id } done() @@ -219,4 +219,4 @@ describe "applyOtUpdate", -> (cb) => rclient.del "pending-updates-list", cb (cb) => rclient.del "DocsWithPendingUpdates", "#{@project_id}:#{@doc_id}", cb (cb) => rclient.del redisSettings.documentupdater.key_schema.pendingUpdates({@doc_id}), cb - ], done \ No newline at end of file + ], done diff --git a/services/real-time/test/acceptance/coffee/ClientTrackingTests.coffee b/services/real-time/test/acceptance/coffee/ClientTrackingTests.coffee index d13663ed70..e76eca7d8e 100644 --- a/services/real-time/test/acceptance/coffee/ClientTrackingTests.coffee +++ b/services/real-time/test/acceptance/coffee/ClientTrackingTests.coffee @@ -63,7 +63,7 @@ describe "clientTracking", -> row: @row column: @column doc_id: @doc_id - id: @clientA.socket.sessionid + id: @clientA.publicId user_id: @user_id name: "Joe Bloggs" } @@ -72,7 +72,7 @@ describe "clientTracking", -> it "should record the update in getConnectedUsers", (done) -> @clientB.emit "clientTracking.getConnectedUsers", (error, users) => for user in users - if user.client_id == @clientA.socket.sessionid + if user.client_id == @clientA.publicId expect(user.cursorData).to.deep.equal({ row: @row column: @column @@ -139,7 +139,7 @@ describe "clientTracking", -> row: @row column: @column doc_id: @doc_id - id: @anonymous.socket.sessionid + id: @anonymous.publicId user_id: "anonymous-user" name: "" } diff --git a/services/real-time/test/acceptance/coffee/JoinProjectTests.coffee b/services/real-time/test/acceptance/coffee/JoinProjectTests.coffee index b3707e0981..11082cdb6c 100644 --- a/services/real-time/test/acceptance/coffee/JoinProjectTests.coffee +++ b/services/real-time/test/acceptance/coffee/JoinProjectTests.coffee @@ -55,7 +55,7 @@ describe "joinProject", -> @client.emit "clientTracking.getConnectedUsers", (error, users) => connected = false for user in users - if user.client_id == @client.socket.sessionid and user.user_id == @user_id + if user.client_id == @client.publicId and user.user_id == @user_id connected = true break expect(connected).to.equal true diff --git a/services/real-time/test/acceptance/coffee/LeaveProjectTests.coffee b/services/real-time/test/acceptance/coffee/LeaveProjectTests.coffee index 5925472245..91ec1a1159 100644 --- a/services/real-time/test/acceptance/coffee/LeaveProjectTests.coffee +++ b/services/real-time/test/acceptance/coffee/LeaveProjectTests.coffee @@ -64,12 +64,12 @@ describe "leaveProject", -> ], done it "should emit a disconnect message to the room", -> - @clientBDisconnectMessages.should.deep.equal [@clientA.socket.sessionid] + @clientBDisconnectMessages.should.deep.equal [@clientA.publicId] it "should no longer list the client in connected users", (done) -> @clientB.emit "clientTracking.getConnectedUsers", (error, users) => for user in users - if user.client_id == @clientA.socket.sessionid + if user.client_id == @clientA.publicId throw "Expected clientA to not be listed in connected users" return done() diff --git a/services/real-time/test/acceptance/coffee/ReceiveUpdateTests.coffee b/services/real-time/test/acceptance/coffee/ReceiveUpdateTests.coffee index ec2c26ca4e..fb504dd9aa 100644 --- a/services/real-time/test/acceptance/coffee/ReceiveUpdateTests.coffee +++ b/services/real-time/test/acceptance/coffee/ReceiveUpdateTests.coffee @@ -65,7 +65,7 @@ describe "receiveUpdate", -> doc_id: @doc_id op: meta: - source: @clientA.socket.sessionid + source: @clientA.publicId v: @version doc: @doc_id op: [{i: "foo", p: 50}] @@ -97,4 +97,4 @@ describe "receiveUpdate", -> it "should disconnect the clients", -> @clientA.socket.connected.should.equal false - @clientB.socket.connected.should.equal false \ No newline at end of file + @clientB.socket.connected.should.equal false diff --git a/services/real-time/test/acceptance/coffee/helpers/RealTimeClient.coffee b/services/real-time/test/acceptance/coffee/helpers/RealTimeClient.coffee index 21da045e83..e3254a79a5 100644 --- a/services/real-time/test/acceptance/coffee/helpers/RealTimeClient.coffee +++ b/services/real-time/test/acceptance/coffee/helpers/RealTimeClient.coffee @@ -37,6 +37,8 @@ module.exports = Client = connect: (cookie) -> client = io.connect("http://localhost:3026", 'force new connection': true) + client.on 'connectionAccepted', (_, publicId) -> + client.publicId = publicId return client getConnectedClients: (callback = (error, clients) ->) -> diff --git a/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee b/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee index b5574c8d16..b2e52c7d56 100644 --- a/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee +++ b/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee @@ -93,7 +93,7 @@ describe "DocumentUpdaterController", -> @otherClients = [new MockClient(), new MockClient()] @update = op: [ t: "foo", p: 12 ] - meta: source: @sourceClient.id + meta: source: @sourceClient.publicId v: @version = 42 doc: @doc_id @io.sockets = diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index 498b425281..6416ded845 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -21,6 +21,7 @@ describe 'WebsocketController', -> @callback = sinon.stub() @client = id: @client_id = "mock-client-id-123" + publicId: "other-id-#{Math.random()}" params: {} set: sinon.stub() get: (param, cb) -> cb null, @params[param] @@ -106,7 +107,7 @@ describe 'WebsocketController', -> it "should mark the user as connected in ConnectedUsersManager", -> @ConnectedUsersManager.updateUserPosition - .calledWith(@project_id, @client.id, @user, null) + .calledWith(@project_id, @client.publicId, @user, null) .should.equal true it "should increment the join-project metric", -> @@ -185,12 +186,12 @@ describe 'WebsocketController', -> it "should end clientTracking.clientDisconnected to the project room", -> @WebsocketLoadBalancer.emitToRoom - .calledWith(@project_id, "clientTracking.clientDisconnected", @client.id) + .calledWith(@project_id, "clientTracking.clientDisconnected", @client.publicId) .should.equal true it "should mark the user as disconnected", -> @ConnectedUsersManager.markUserAsDisconnected - .calledWith(@project_id, @client.id) + .calledWith(@project_id, @client.publicId) .should.equal true it "should flush the project in the document updater", -> @@ -223,12 +224,12 @@ describe 'WebsocketController', -> it "should not end clientTracking.clientDisconnected to the project room", -> @WebsocketLoadBalancer.emitToRoom - .calledWith(@project_id, "clientTracking.clientDisconnected", @client.id) + .calledWith(@project_id, "clientTracking.clientDisconnected", @client.publicId) .should.equal false it "should not mark the user as disconnected", -> @ConnectedUsersManager.markUserAsDisconnected - .calledWith(@project_id, @client.id) + .calledWith(@project_id, @client.publicId) .should.equal false it "should not flush the project in the document updater", -> @@ -247,12 +248,12 @@ describe 'WebsocketController', -> it "should not end clientTracking.clientDisconnected to the project room", -> @WebsocketLoadBalancer.emitToRoom - .calledWith(@project_id, "clientTracking.clientDisconnected", @client.id) + .calledWith(@project_id, "clientTracking.clientDisconnected", @client.publicId) .should.equal false it "should not mark the user as disconnected", -> @ConnectedUsersManager.markUserAsDisconnected - .calledWith(@project_id, @client.id) + .calledWith(@project_id, @client.publicId) .should.equal false it "should not flush the project in the document updater", -> @@ -488,7 +489,7 @@ describe 'WebsocketController', -> @populatedCursorData = doc_id: @doc_id, - id: @client.id + id: @client.publicId name: "#{@first_name} #{@last_name}" row: @row column: @column @@ -499,7 +500,7 @@ describe 'WebsocketController', -> @WebsocketLoadBalancer.emitToRoom.calledWith(@project_id, "clientTracking.clientUpdated", @populatedCursorData).should.equal true it "should send the cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.id, { + @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.publicId, { _id: @user_id, email: @email, first_name: @first_name, @@ -528,7 +529,7 @@ describe 'WebsocketController', -> @populatedCursorData = doc_id: @doc_id, - id: @client.id + id: @client.publicId name: "#{@first_name}" row: @row column: @column @@ -539,7 +540,7 @@ describe 'WebsocketController', -> @WebsocketLoadBalancer.emitToRoom.calledWith(@project_id, "clientTracking.clientUpdated", @populatedCursorData).should.equal true it "should send the cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.id, { + @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.publicId, { _id: @user_id, email: @email, first_name: @first_name, @@ -568,7 +569,7 @@ describe 'WebsocketController', -> @populatedCursorData = doc_id: @doc_id, - id: @client.id + id: @client.publicId name: "#{@last_name}" row: @row column: @column @@ -579,7 +580,7 @@ describe 'WebsocketController', -> @WebsocketLoadBalancer.emitToRoom.calledWith(@project_id, "clientTracking.clientUpdated", @populatedCursorData).should.equal true it "should send the cursor data to the connected user manager", (done)-> - @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.id, { + @ConnectedUsersManager.updateUserPosition.calledWith(@project_id, @client.publicId, { _id: @user_id, email: @email, first_name: undefined, @@ -609,7 +610,7 @@ describe 'WebsocketController', -> @WebsocketLoadBalancer.emitToRoom .calledWith(@project_id, "clientTracking.clientUpdated", { doc_id: @doc_id, - id: @client.id, + id: @client.publicId, user_id: @user_id, name: "", row: @row, @@ -631,7 +632,7 @@ describe 'WebsocketController', -> @WebsocketLoadBalancer.emitToRoom .calledWith(@project_id, "clientTracking.clientUpdated", { doc_id: @doc_id, - id: @client.id + id: @client.publicId name: "" row: @row column: @column @@ -655,7 +656,7 @@ describe 'WebsocketController', -> @WebsocketController.applyOtUpdate @client, @doc_id, @update, @callback it "should set the source of the update to the client id", -> - @update.meta.source.should.equal @client.id + @update.meta.source.should.equal @client.publicId it "should set the user_id of the update to the user id", -> @update.meta.user_id.should.equal @user_id diff --git a/services/real-time/test/unit/coffee/helpers/MockClient.coffee b/services/real-time/test/unit/coffee/helpers/MockClient.coffee index 82c3c02b19..4dcddeb01d 100644 --- a/services/real-time/test/unit/coffee/helpers/MockClient.coffee +++ b/services/real-time/test/unit/coffee/helpers/MockClient.coffee @@ -9,6 +9,7 @@ module.exports = class MockClient @emit = sinon.stub() @disconnect = sinon.stub() @id = idCounter++ + @publicId = idCounter++ set : (key, value, callback) -> @attributes[key] = value callback() if callback? From 32af7001fc6b9355d37914df52a27388cf615c42 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 8 Jun 2020 11:29:40 +0100 Subject: [PATCH 2/2] [misc] Router: prefix the publicId with 'P.' for easy differentiation --- services/real-time/app/coffee/Router.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/real-time/app/coffee/Router.coffee b/services/real-time/app/coffee/Router.coffee index 0266c71eec..91fa27930b 100644 --- a/services/real-time/app/coffee/Router.coffee +++ b/services/real-time/app/coffee/Router.coffee @@ -68,7 +68,7 @@ module.exports = Router = return # send positive confirmation that the client has a valid connection - client.publicId = base64id.generateId() + client.publicId = 'P.' + base64id.generateId() client.emit("connectionAccepted", null, client.publicId) metrics.inc('socket-io.connection')