Track the isRestrictedUser flag on clients

Then, don't send new chat messages and new comments to those restricted clients.
We do this because we don't want to leak private information (email addresses
and names) to "restricted" users, those who have read-only access via a
shared token.
This commit is contained in:
Shane Kilkelly 2019-10-04 10:30:24 +01:00
parent b6a7a0ab4c
commit 6765d03339
6 changed files with 165 additions and 89 deletions

View file

@ -4,7 +4,7 @@ logger = require "logger-sharelatex"
{ CodedError } = require "./Errors"
module.exports = WebApiManager =
joinProject: (project_id, user, callback = (error, project, privilegeLevel) ->) ->
joinProject: (project_id, user, callback = (error, project, privilegeLevel, isRestrictedUser) ->) ->
user_id = user._id
logger.log {project_id, user_id}, "sending join project request to web"
url = "#{settings.apis.web.url}/project/#{project_id}/join"
@ -24,7 +24,7 @@ module.exports = WebApiManager =
}, (error, response, data) ->
return callback(error) if error?
if 200 <= response.statusCode < 300
callback null, data?.project, data?.privilegeLevel
callback null, data?.project, data?.privilegeLevel, data?.isRestrictedUser
else if response.statusCode == 429
logger.log(project_id, user_id, "rate-limit hit when joining project")
callback(new CodedError("rate-limit hit when joining project", "TooManyRequests"))

View file

@ -13,12 +13,12 @@ module.exports = WebsocketController =
# it will force a full refresh of the page. Useful for non-backwards
# compatible protocol changes. Use only in extreme need.
PROTOCOL_VERSION: 2
joinProject: (client, user, project_id, callback = (error, project, privilegeLevel, protocolVersion) ->) ->
user_id = user?._id
logger.log {user_id, project_id, client_id: client.id}, "user joining project"
metrics.inc "editor.join-project"
WebApiManager.joinProject project_id, user, (error, project, privilegeLevel) ->
WebApiManager.joinProject project_id, user, (error, project, privilegeLevel, isRestrictedUser) ->
return callback(error) if error?
if !privilegeLevel or privilegeLevel == ""
@ -26,7 +26,6 @@ module.exports = WebsocketController =
logger.warn {err, project_id, user_id, client_id: client.id}, "user is not authorized to join project"
return callback(err)
client.set("privilege_level", privilegeLevel)
client.set("user_id", user_id)
client.set("project_id", project_id)
@ -37,18 +36,19 @@ module.exports = WebsocketController =
client.set("connected_time", new Date())
client.set("signup_date", user?.signUpDate)
client.set("login_count", user?.loginCount)
client.set("is_restricted_user", !!(isRestrictedUser))
RoomManager.joinProject client, project_id, (err) ->
logger.log {user_id, project_id, client_id: client.id}, "user joined project"
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, () ->
# 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
# is determined by FLUSH_IF_EMPTY_DELAY.
FLUSH_IF_EMPTY_DELAY: 500 #ms
FLUSH_IF_EMPTY_DELAY: 500 #ms
leaveProject: (io, client, callback = (error) ->) ->
metrics.inc "editor.leave-project"
Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) ->
@ -56,7 +56,7 @@ module.exports = WebsocketController =
logger.log {project_id, user_id, client_id: client.id}, "client leaving project"
WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id
# bail out if the client had not managed to authenticate or join
# the project. Prevents downstream errors in docupdater from
# flushProjectToMongoAndDelete with null project_id.
@ -66,12 +66,12 @@ module.exports = WebsocketController =
if not project_id?
logger.log {user_id: user_id, client_id: client.id}, "client leaving, not in project"
return callback()
# We can do this in the background
ConnectedUsersManager.markUserAsDisconnected project_id, client.id, (err) ->
if err?
logger.error {err, project_id, user_id, client_id: client.id}, "error marking client as disconnected"
RoomManager.leaveProjectAndDocs(client)
setTimeout () ->
remainingClients = io.sockets.clients(project_id)
@ -82,14 +82,14 @@ module.exports = WebsocketController =
logger.error {err, project_id, user_id, client_id: client.id}, "error flushing to doc updater after leaving project"
callback()
, WebsocketController.FLUSH_IF_EMPTY_DELAY
joinDoc: (client, doc_id, fromVersion = -1, options, callback = (error, doclines, version, ops, ranges) ->) ->
metrics.inc "editor.join-doc"
Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) ->
return callback(error) if error?
return callback(new Error("no project_id found on client")) if !project_id?
logger.log {user_id, project_id, doc_id, fromVersion, client_id: client.id}, "client joining doc"
AuthorizationManager.assertClientCanViewProject client, (error) ->
return callback(error) if error?
# ensure the per-doc applied-ops channel is subscribed before sending the
@ -124,7 +124,7 @@ module.exports = WebsocketController =
AuthorizationManager.addAccessToDoc client, doc_id
logger.log {user_id, project_id, doc_id, fromVersion, client_id: client.id}, "client joined doc"
callback null, escapedLines, version, ops, ranges
leaveDoc: (client, doc_id, callback = (error) ->) ->
metrics.inc "editor.leave-doc"
Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) ->
@ -227,9 +227,9 @@ module.exports = WebsocketController =
return callback(error)
else
return callback(null)
_isCommentUpdate: (update) ->
for op in update.op
if !op.c?
return false
return true
return true

View file

@ -7,6 +7,8 @@ HealthCheckManager = require "./HealthCheckManager"
RoomManager = require "./RoomManager"
ChannelManager = require "./ChannelManager"
ConnectedUsersManager = require "./ConnectedUsersManager"
Utils = require './Utils'
Async = require 'async'
module.exports = WebsocketLoadBalancer =
rclientPubList: RedisClientManager.createClientList(Settings.redis.pubsub)
@ -58,7 +60,7 @@ module.exports = WebsocketLoadBalancer =
else if message.message is 'clientTracking.refresh' && message.room_id?
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
for client in clientList
ConnectedUsersManager.refreshClient(message.room_id, client.id)
else if message.room_id?
if message._id? && Settings.checkEventOrder
@ -69,12 +71,27 @@ module.exports = WebsocketLoadBalancer =
clientList = io.sockets.clients(message.room_id)
# avoid unnecessary work if no clients are connected
return if clientList.length is 0
logger.log {channel:channel, message: message.message, room_id: message.room_id, message_id: message._id, socketIoClients: (client.id for client in clientList)}, "distributing event to clients"
logger.log {
channel: channel,
message: message.message,
room_id: message.room_id,
message_id: message._id,
socketIoClients: (client.id for client in clientList)
}, "distributing event to clients"
seen = {}
for client in clientList when not seen[client.id]
seen[client.id] = true
client.emit(message.message, message.payload...)
# Send the messages to clients async, don't wait for them all to finish
Async.eachSeries clientList
, (client, cb) ->
Utils.getClientAttributes client, ['is_restricted_user'], (err, {is_restricted_user}) ->
return cb(err) if err?
if !seen[client.id]
seen[client.id] = true
if !(is_restricted_user && message.message in ['new-chat-message', 'new-comment'])
client.emit(message.message, message.payload...)
cb()
, (err) ->
if err?
logger.err {err, message}, "Error sending message to clients"
else if message.health_check?
logger.debug {message}, "got health check message in editor events channel"
HealthCheckManager.check channel, message.key

View file

@ -20,17 +20,18 @@ describe 'WebApiManager', ->
user: "username"
pass: "password"
"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() }
describe "joinProject", ->
describe "successfully", ->
beforeEach ->
@response = {
project: { name: "Test project" }
privilegeLevel: "owner"
privilegeLevel: "owner",
isRestrictedUser: true
}
@request.post = sinon.stub().callsArgWith(1, null, {statusCode: 200}, @response)
@WebApiManager.joinProject @project_id, @user, @callback
it "should send a request to web to join the project", ->
@request.post
.calledWith({
@ -46,22 +47,22 @@ describe 'WebApiManager', ->
headers: {}
})
.should.equal true
it "should return the project and privilegeLevel", ->
it "should return the project, privilegeLevel, and restricted flag", ->
@callback
.calledWith(null, @response.project, @response.privilegeLevel)
.calledWith(null, @response.project, @response.privilegeLevel, @response.isRestrictedUser)
.should.equal true
describe "with an error from web", ->
beforeEach ->
@request.post = sinon.stub().callsArgWith(1, null, {statusCode: 500}, null)
@WebApiManager.joinProject @project_id, @user_id, @callback
it "should call the callback with an error", ->
@callback
.calledWith(new Error("non-success code from web: 500"))
.should.equal true
describe "when the project is over its rate limit", ->
beforeEach ->
@request.post = sinon.stub().callsArgWith(1, null, {statusCode: 429}, null)

View file

@ -37,10 +37,10 @@ describe 'WebsocketController', ->
inc: sinon.stub()
set: sinon.stub()
"./RoomManager": @RoomManager = {}
afterEach ->
tk.reset()
describe "joinProject", ->
describe "when authorised", ->
beforeEach ->
@ -53,61 +53,65 @@ describe 'WebsocketController', ->
}
@privilegeLevel = "owner"
@ConnectedUsersManager.updateUserPosition = sinon.stub().callsArg(4)
@WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, @project, @privilegeLevel)
@isRestrictedUser = true
@WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, @project, @privilegeLevel, @isRestrictedUser)
@RoomManager.joinProject = sinon.stub().callsArg(2)
@WebsocketController.joinProject @client, @user, @project_id, @callback
it "should load the project from web", ->
@WebApiManager.joinProject
.calledWith(@project_id, @user)
.should.equal true
it "should join the project room", ->
@RoomManager.joinProject.calledWith(@client, @project_id).should.equal true
it "should set the privilege level on the client", ->
@client.set.calledWith("privilege_level", @privilegeLevel).should.equal true
it "should set the user's id on the client", ->
@client.set.calledWith("user_id", @user._id).should.equal true
it "should set the user's email on the client", ->
@client.set.calledWith("email", @user.email).should.equal true
it "should set the user's first_name on the client", ->
@client.set.calledWith("first_name", @user.first_name).should.equal true
it "should set the user's last_name on the client", ->
@client.set.calledWith("last_name", @user.last_name).should.equal true
it "should set the user's sign up date on the client", ->
@client.set.calledWith("signup_date", @user.signUpDate).should.equal true
it "should set the user's login_count on the client", ->
@client.set.calledWith("login_count", @user.loginCount).should.equal true
it "should set the connected time on the client", ->
@client.set.calledWith("connected_time", new Date()).should.equal true
it "should set the project_id on the client", ->
@client.set.calledWith("project_id", @project_id).should.equal true
it "should set the project owner id on the client", ->
@client.set.calledWith("owner_id", @owner_id).should.equal true
it "should set the is_restricted_user flag on the client", ->
@client.set.calledWith("is_restricted_user", @isRestrictedUser).should.equal true
it "should call the callback with the project, privilegeLevel and protocolVersion", ->
@callback
.calledWith(null, @project, @privilegeLevel, @WebsocketController.PROTOCOL_VERSION)
.should.equal true
it "should mark the user as connected in ConnectedUsersManager", ->
@ConnectedUsersManager.updateUserPosition
.calledWith(@project_id, @client.id, @user, null)
.should.equal true
it "should increment the join-project metric", ->
@metrics.inc.calledWith("editor.join-project").should.equal true
describe "when not authorized", ->
beforeEach ->
@WebApiManager.joinProject = sinon.stub().callsArgWith(2, null, null, null)
@ -138,30 +142,30 @@ describe 'WebsocketController', ->
@client.params.user_id = @user_id
@WebsocketController.FLUSH_IF_EMPTY_DELAY = 0
tk.reset() # Allow setTimeout to work.
describe "when the project is empty", ->
beforeEach (done) ->
@clientsInRoom = []
@WebsocketController.leaveProject @io, @client, done
it "should end clientTracking.clientDisconnected to the project room", ->
@WebsocketLoadBalancer.emitToRoom
.calledWith(@project_id, "clientTracking.clientDisconnected", @client.id)
.should.equal true
it "should mark the user as disconnected", ->
@ConnectedUsersManager.markUserAsDisconnected
.calledWith(@project_id, @client.id)
.should.equal true
it "should flush the project in the document updater", ->
@DocumentUpdaterManager.flushProjectToMongoAndDelete
.calledWith(@project_id)
.should.equal true
it "should increment the leave-project metric", ->
@metrics.inc.calledWith("editor.leave-project").should.equal true
it "should track the disconnection in RoomManager", ->
@RoomManager.leaveProjectAndDocs
.calledWith(@client)
@ -171,7 +175,7 @@ describe 'WebsocketController', ->
beforeEach ->
@clientsInRoom = ["mock-remaining-client"]
@WebsocketController.leaveProject @io, @client
it "should not flush the project in the document updater", ->
@DocumentUpdaterManager.flushProjectToMongoAndDelete
.called.should.equal false
@ -232,7 +236,7 @@ describe 'WebsocketController', ->
@ops = ["mock", "ops"]
@ranges = { "mock": "ranges" }
@options = {}
@client.params.project_id = @project_id
@AuthorizationManager.addAccessToDoc = sinon.stub()
@AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null)
@ -275,7 +279,7 @@ describe 'WebsocketController', ->
beforeEach ->
@fromVersion = 40
@WebsocketController.joinDoc @client, @doc_id, @fromVersion, @options, @callback
it "should get the document from the DocumentUpdaterManager with fromVersion", ->
@DocumentUpdaterManager.getDocument
.calledWith(@project_id, @doc_id, @fromVersion)
@ -285,7 +289,7 @@ describe 'WebsocketController', ->
beforeEach ->
@doc_lines.push ["räksmörgås"]
@WebsocketController.joinDoc @client, @doc_id, -1, @options, @callback
it "should call the callback with the escaped lines", ->
escaped_lines = @callback.args[0][1]
escaped_word = escaped_lines.pop()
@ -327,44 +331,44 @@ describe 'WebsocketController', ->
beforeEach ->
@AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, @err = new Error("not authorized"))
@WebsocketController.joinDoc @client, @doc_id, -1, @options, @callback
it "should call the callback with an error", ->
@callback.calledWith(@err).should.equal true
it "should not call the DocumentUpdaterManager", ->
@DocumentUpdaterManager.getDocument.called.should.equal false
describe "leaveDoc", ->
beforeEach ->
@doc_id = "doc-id-123"
@doc_id = "doc-id-123"
@client.params.project_id = @project_id
@RoomManager.leaveDoc = sinon.stub()
@WebsocketController.leaveDoc @client, @doc_id, @callback
it "should remove the client from the doc_id room", ->
@RoomManager.leaveDoc
.calledWith(@client, @doc_id).should.equal true
it "should call the callback", ->
@callback.called.should.equal true
it "should increment the leave-doc metric", ->
@metrics.inc.calledWith("editor.leave-doc").should.equal true
describe "getConnectedUsers", ->
beforeEach ->
@client.params.project_id = @project_id
@users = ["mock", "users"]
@WebsocketLoadBalancer.emitToRoom = sinon.stub()
@ConnectedUsersManager.getConnectedUsers = sinon.stub().callsArgWith(1, null, @users)
describe "when authorized", ->
beforeEach (done) ->
@AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null)
@WebsocketController.getConnectedUsers @client, (args...) =>
@callback(args...)
done()
it "should check that the client is authorized to view the project", ->
@AuthorizationManager.assertClientCanViewProject
.calledWith(@client)
@ -379,26 +383,26 @@ describe 'WebsocketController', ->
@ConnectedUsersManager.getConnectedUsers
.calledWith(@project_id)
.should.equal true
it "should return the users", ->
@callback.calledWith(null, @users).should.equal true
it "should increment the get-connected-users metric", ->
@metrics.inc.calledWith("editor.get-connected-users").should.equal true
describe "when not authorized", ->
beforeEach ->
@AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, @err = new Error("not authorized"))
@WebsocketController.getConnectedUsers @client, @callback
it "should not get the connected users for the project", ->
@ConnectedUsersManager.getConnectedUsers
.called
.should.equal false
it "should return an error", ->
@callback.calledWith(@err).should.equal true
describe "updateClientPosition", ->
beforeEach ->
@WebsocketLoadBalancer.emitToRoom = sinon.stub()
@ -422,7 +426,7 @@ describe 'WebsocketController', ->
@client.get = (param, callback) => callback null, @clientParams[param]
@WebsocketController.updateClientPosition @client, @update
@populatedCursorData =
@populatedCursorData =
doc_id: @doc_id,
id: @client.id
name: "#{@first_name} #{@last_name}"
@ -462,7 +466,7 @@ describe 'WebsocketController', ->
@client.get = (param, callback) => callback null, @clientParams[param]
@WebsocketController.updateClientPosition @client, @update
@populatedCursorData =
@populatedCursorData =
doc_id: @doc_id,
id: @client.id
name: "#{@first_name}"
@ -502,7 +506,7 @@ describe 'WebsocketController', ->
@client.get = (param, callback) => callback null, @clientParams[param]
@WebsocketController.updateClientPosition @client, @update
@populatedCursorData =
@populatedCursorData =
doc_id: @doc_id,
id: @client.id
name: "#{@last_name}"
@ -603,7 +607,7 @@ describe 'WebsocketController', ->
it "should call the callback", ->
@callback.called.should.equal true
it "should increment the doc updates", ->
@metrics.inc.calledWith("editor.doc-update").should.equal true
@ -621,7 +625,7 @@ describe 'WebsocketController', ->
it "should call the callback with the error", ->
@callback.calledWith(@error).should.equal true
describe "when not authorized", ->
beforeEach ->
@client.disconnect = sinon.stub()
@ -645,7 +649,7 @@ describe 'WebsocketController', ->
@comment_update = { op: [{c: "bar", p: 132}] }
@AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub()
@AuthorizationManager.assertClientCanViewProjectAndDoc = sinon.stub()
describe "with a read-write client", ->
it "should return successfully", (done) ->
@AuthorizationManager.assertClientCanEditProjectAndDoc.yields(null)
@ -660,7 +664,7 @@ describe 'WebsocketController', ->
@WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @edit_update, (error) ->
expect(error.message).to.equal "not authorized"
done()
describe "with a read-only client and a comment op", ->
it "should return successfully", (done) ->
@AuthorizationManager.assertClientCanEditProjectAndDoc.yields(new Error("not authorized"))
@ -668,7 +672,7 @@ describe 'WebsocketController', ->
@WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @comment_update, (error) ->
expect(error).to.be.null
done()
describe "with a totally unauthorized client", ->
it "should return an error", (done) ->
@AuthorizationManager.assertClientCanEditProjectAndDoc.yields(new Error("not authorized"))

View file

@ -8,7 +8,7 @@ describe "WebsocketLoadBalancer", ->
@rclient = {}
@RoomEvents = {on: sinon.stub()}
@WebsocketLoadBalancer = SandboxedModule.require modulePath, requires:
"./RedisClientManager":
"./RedisClientManager":
createClientList: () => []
"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() }
"./SafeJsonParse": @SafeJsonParse =
@ -18,6 +18,12 @@ describe "WebsocketLoadBalancer", ->
"./RoomManager" : @RoomManager = {eventSource: sinon.stub().returns @RoomEvents}
"./ChannelManager": @ChannelManager = {publish: sinon.stub()}
"./ConnectedUsersManager": @ConnectedUsersManager = {refreshClient: sinon.stub()}
"./Utils": @Utils = {
getClientAttributes: sinon.spy(
(client, _attrs, callback) ->
callback(null, {is_restricted_user: !!client.__isRestricted})
)
}
@io = {}
@WebsocketLoadBalancer.rclientPubList = [{publish: sinon.stub()}]
@WebsocketLoadBalancer.rclientSubList = [{
@ -51,7 +57,7 @@ describe "WebsocketLoadBalancer", ->
@WebsocketLoadBalancer.emitToRoom
.calledWith("all", @message, @payload...)
.should.equal true
describe "listenForEditorEvents", ->
beforeEach ->
@WebsocketLoadBalancer._processEditorEvent = sinon.stub()
@ -70,9 +76,10 @@ describe "WebsocketLoadBalancer", ->
describe "_processEditorEvent", ->
describe "with bad JSON", ->
beforeEach ->
@isRestrictedUser = false
@SafeJsonParse.parse = sinon.stub().callsArgWith 1, new Error("oops")
@WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", "blah")
it "should log an error", ->
@logger.error.called.should.equal true
@ -98,6 +105,54 @@ describe "WebsocketLoadBalancer", ->
@emit2.calledWith(@message, @payload...).should.equal true
@emit3.called.should.equal false # duplicate client should be ignored
describe "with a designated room, and restricted clients, not restricted message", ->
beforeEach ->
@io.sockets =
clients: sinon.stub().returns([
{id: 'client-id-1', emit: @emit1 = sinon.stub()}
{id: 'client-id-2', emit: @emit2 = sinon.stub()}
{id: 'client-id-1', emit: @emit3 = sinon.stub()} # duplicate client
{id: 'client-id-4', emit: @emit4 = sinon.stub(), __isRestricted: true}
])
data = JSON.stringify
room_id: @room_id
message: @message
payload: @payload
@WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", data)
it "should send the message to all (unique) clients in the room", ->
@io.sockets.clients
.calledWith(@room_id)
.should.equal true
@emit1.calledWith(@message, @payload...).should.equal true
@emit2.calledWith(@message, @payload...).should.equal true
@emit3.called.should.equal false # duplicate client should be ignored
@emit4.called.should.equal true # restricted client, but should be called
describe "with a designated room, and restricted clients, restricted message", ->
beforeEach ->
@io.sockets =
clients: sinon.stub().returns([
{id: 'client-id-1', emit: @emit1 = sinon.stub()}
{id: 'client-id-2', emit: @emit2 = sinon.stub()}
{id: 'client-id-1', emit: @emit3 = sinon.stub()} # duplicate client
{id: 'client-id-4', emit: @emit4 = sinon.stub(), __isRestricted: true}
])
data = JSON.stringify
room_id: @room_id
message: @restrictedMessage = 'new-comment'
payload: @payload
@WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", data)
it "should send the message to all (unique) clients in the room, who are not restricted", ->
@io.sockets.clients
.calledWith(@room_id)
.should.equal true
@emit1.calledWith(@restrictedMessage, @payload...).should.equal true
@emit2.calledWith(@restrictedMessage, @payload...).should.equal true
@emit3.called.should.equal false # duplicate client should be ignored
@emit4.called.should.equal false # restricted client, should not be called
describe "when emitting to all", ->
beforeEach ->
@io.sockets =
@ -110,4 +165,3 @@ describe "WebsocketLoadBalancer", ->
it "should send the message to all clients", ->
@emit.calledWith(@message, @payload...).should.equal true