From ef85bce3b8bbe63efacabbcd36ec5d1944b77b3a Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 2 Sep 2016 16:35:00 +0100 Subject: [PATCH] track permissions when clients join and leave docs --- .../app/coffee/AuthorizationManager.coffee | 26 +++- .../app/coffee/WebsocketController.coffee | 12 +- .../coffee/AuthorizationManagerTests.coffee | 130 +++++++++++++++++- .../coffee/WebsocketControllerTests.coffee | 14 +- 4 files changed, 169 insertions(+), 13 deletions(-) diff --git a/services/real-time/app/coffee/AuthorizationManager.coffee b/services/real-time/app/coffee/AuthorizationManager.coffee index 273e9697cf..b4cf854238 100644 --- a/services/real-time/app/coffee/AuthorizationManager.coffee +++ b/services/real-time/app/coffee/AuthorizationManager.coffee @@ -12,4 +12,28 @@ module.exports = AuthorizationManager = if allowed callback null else - callback new Error("not authorized") \ No newline at end of file + callback new Error("not authorized") + + assertClientCanViewProjectAndDoc: (client, doc_id, callback = (error) ->) -> + AuthorizationManager.assertClientCanViewProject client, (error) -> + return callback(error) if error? + AuthorizationManager._assertClientCanAccessDoc client, doc_id, callback + + assertClientCanEditProjectAndDoc: (client, doc_id, callback = (error) ->) -> + AuthorizationManager.assertClientCanEditProject client, (error) -> + return callback(error) if error? + AuthorizationManager._assertClientCanAccessDoc client, doc_id, callback + + _assertClientCanAccessDoc: (client, doc_id, callback = (error) ->) -> + client.get "doc:#{doc_id}", (error, status) -> + return callback(error) if error? + if status? and status is "allowed" + callback null + else + callback new Error("not authorized") + + addAccessToDoc: (client, doc_id, callback = (error) ->) -> + client.set("doc:#{doc_id}", "allowed", callback) + + removeAccessToDoc: (client, doc_id, callback = (error) ->) -> + client.del("doc:#{doc_id}", callback) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 4e537a664e..0f04981d05 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -91,6 +91,7 @@ module.exports = WebsocketController = logger.err {err, project_id, doc_id, fromVersion, line, client_id: client.id}, "error encoding line uri component" return callback(err) escapedLines.push line + AuthorizationManager.addAccessToDoc client, doc_id client.join(doc_id) callback null, escapedLines, version, ops logger.log {user_id, project_id, doc_id, fromVersion, client_id: client.id}, "client joined doc" @@ -99,8 +100,9 @@ module.exports = WebsocketController = metrics.inc "editor.leave-doc" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> logger.log {user_id, project_id, doc_id, client_id: client.id}, "client leaving doc" - client.leave doc_id - callback() + client.leave doc_id + AuthorizationManager.removeAccessToDoc client, doc_id # may not be needed, could block updates? + callback() updateClientPosition: (client, cursorData, callback = (error) ->) -> metrics.inc "editor.update-client-position", 0.1 @@ -110,7 +112,7 @@ module.exports = WebsocketController = return callback(error) if error? logger.log {user_id, project_id, client_id: client.id, cursorData: cursorData}, "updating client position" - AuthorizationManager.assertClientCanViewProject client, (error) -> + AuthorizationManager.assertClientCanViewProjectAndDoc client, cursorData.doc_id, (error) -> if error? logger.warn {client_id: client.id, project_id, user_id}, "silently ignoring unauthorized updateClientPosition. Client likely hasn't called joinProject yet." return callback() @@ -133,7 +135,7 @@ module.exports = WebsocketController = cursorData.name = "Anonymous" callback() WebsocketLoadBalancer.emitToRoom(project_id, "clientTracking.clientUpdated", cursorData) - + getConnectedUsers: (client, callback = (error, users) ->) -> metrics.inc "editor.get-connected-users" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> @@ -157,7 +159,7 @@ module.exports = WebsocketController = return callback(new Error("no project_id found on client")) if !project_id? # Omit this logging for now since it's likely too noisey #logger.log {user_id, project_id, doc_id, client_id: client.id, update: update}, "applying update" - AuthorizationManager.assertClientCanEditProject client, (error) -> + AuthorizationManager.assertClientCanEditProjectAndDoc client, doc_id, (error) -> cbc_1++ if error? logger.error {err: error, doc_id, client_id: client.id, version: update.v}, "client is not authorized to make update" diff --git a/services/real-time/test/unit/coffee/AuthorizationManagerTests.coffee b/services/real-time/test/unit/coffee/AuthorizationManagerTests.coffee index a4741ab4d8..9856684247 100644 --- a/services/real-time/test/unit/coffee/AuthorizationManagerTests.coffee +++ b/services/real-time/test/unit/coffee/AuthorizationManagerTests.coffee @@ -10,7 +10,15 @@ describe 'AuthorizationManager', -> beforeEach -> @client = params: {} - get: (param, cb) -> cb null, @params[param] + get: (param, cb) -> + cb null, @params[param] + set: (param, value, cb) -> + @params[param] = value + cb() + del: (param, cb) -> + delete @params[param] + cb() + @AuthorizationManager = SandboxedModule.require modulePath, requires: {} describe "assertClientCanViewProject", -> @@ -61,4 +69,122 @@ describe 'AuthorizationManager', -> @client.params.privilege_level = "unknown" @AuthorizationManager.assertClientCanEditProject @client, (error) -> error.message.should.equal "not authorized" - done() \ No newline at end of file + done() + + # check doc access for project + + describe "assertClientCanViewProjectAndDoc", -> + beforeEach () -> + @doc_id = "12345" + @callback = sinon.stub() + @client.params = {} + + describe "when not authorised at the project level", -> + beforeEach () -> + @client.params.privilege_level = "unknown" + + it "should not allow access", () -> + @AuthorizationManager.assertClientCanViewProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "even when authorised at the doc level", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, done + + it "should not allow access", () -> + @AuthorizationManager.assertClientCanViewProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "when authorised at the project level", -> + beforeEach () -> + @client.params.privilege_level = "readOnly" + + describe "and not authorised at the document level", -> + it "should not allow access", () -> + @AuthorizationManager.assertClientCanViewProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "and authorised at the document level", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, done + + it "should allow access", () -> + @AuthorizationManager.assertClientCanViewProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(null) + .should.equal true + + describe "when document authorisation is added and then removed", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, () => + @AuthorizationManager.removeAccessToDoc @client, @doc_id, done + + it "should deny access", () -> + @AuthorizationManager.assertClientCanViewProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "assertClientCanEditProjectAndDoc", -> + beforeEach () -> + @doc_id = "12345" + @callback = sinon.stub() + @client.params = {} + + describe "when not authorised at the project level", -> + beforeEach () -> + @client.params.privilege_level = "readOnly" + + it "should not allow access", () -> + @AuthorizationManager.assertClientCanEditProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "even when authorised at the doc level", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, done + + it "should not allow access", () -> + @AuthorizationManager.assertClientCanEditProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "when authorised at the project level", -> + beforeEach () -> + @client.params.privilege_level = "readAndWrite" + + describe "and not authorised at the document level", -> + it "should not allow access", () -> + @AuthorizationManager.assertClientCanEditProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true + + describe "and authorised at the document level", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, done + + it "should allow access", () -> + @AuthorizationManager.assertClientCanEditProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(null) + .should.equal true + + describe "when document authorisation is added and then removed", -> + beforeEach (done) -> + @AuthorizationManager.addAccessToDoc @client, @doc_id, () => + @AuthorizationManager.removeAccessToDoc @client, @doc_id, done + + it "should deny access", () -> + @AuthorizationManager.assertClientCanEditProjectAndDoc @client, @doc_id, @callback + @callback + .calledWith(new Error("not authorised")) + .should.equal true diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index d648b39c8c..48eccd4bee 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -172,7 +172,7 @@ describe 'WebsocketController', -> @ops = ["mock", "ops"] @client.params.project_id = @project_id - + @AuthorizationManager.addAccessToDoc = sinon.stub() @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null) @DocumentUpdaterManager.getDocument = sinon.stub().callsArgWith(3, null, @doc_lines, @version, @ops) @@ -190,6 +190,11 @@ describe 'WebsocketController', -> @DocumentUpdaterManager.getDocument .calledWith(@project_id, @doc_id, @fromVersion) .should.equal true + + it "should add permissions for the client to access the doc", -> + @AuthorizationManager.addAccessToDoc + .calledWith(@client, @doc_id) + .should.equal true it "should join the client to room for the doc_id", -> @client.join @@ -287,7 +292,7 @@ describe 'WebsocketController', -> beforeEach -> @WebsocketLoadBalancer.emitToRoom = sinon.stub() @ConnectedUsersManager.updateUserPosition = sinon.stub().callsArgWith(4) - @AuthorizationManager.assertClientCanViewProject = sinon.stub().callsArgWith(1, null) + @AuthorizationManager.assertClientCanViewProjectAndDoc = sinon.stub().callsArgWith(2, null) @update = { doc_id: @doc_id = "doc-id-123" row: @row = 42 @@ -362,7 +367,7 @@ describe 'WebsocketController', -> @update = {op: {p: 12, t: "foo"}} @client.params.user_id = @user_id @client.params.project_id = @project_id - @AuthorizationManager.assertClientCanEditProject = sinon.stub().callsArg(1) + @AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub().callsArg(2) @DocumentUpdaterManager.queueChange = sinon.stub().callsArg(3) describe "succesfully", -> @@ -410,7 +415,7 @@ describe 'WebsocketController', -> describe "when not authorized", -> beforeEach -> @client.disconnect = sinon.stub() - @AuthorizationManager.assertClientCanEditProject = sinon.stub().callsArgWith(1, @error = new Error("not authorized")) + @AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub().callsArgWith(2, @error = new Error("not authorized")) @WebsocketController.applyOtUpdate @client, @doc_id, @update, @callback # This happens in a setTimeout to allow the client a chance to receive the error first. @@ -423,4 +428,3 @@ describe 'WebsocketController', -> it "should call the callback with the error", -> @callback.calledWith(@error).should.equal true - \ No newline at end of file