Merge pull request #11 from sharelatex/ja-allow-readonly-comments

Allow readonly users to submit comments

services/real-time/app/coffee/WebsocketController.coffee

@@ -164,16 +164,10 @@ module.exports = WebsocketController =
 					
 
 	applyOtUpdate: (client, doc_id, update, callback = (error) ->) ->
-		cbc_0 = 0 # Callback counter
-		cbc_1 = 0
 		Utils.getClientAttributes client, ["user_id", "project_id"], (error, {user_id, project_id}) ->
-			cbc_0++
 			return callback(error) if error?
 			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.assertClientCanEditProjectAndDoc client, doc_id, (error) ->
-				cbc_1++
+			WebsocketController._assertClientCanApplyUpdate client, doc_id, update, (error) ->
 				if error?
 					logger.error {err: error, doc_id, client_id: client.id, version: update.v}, "client is not authorized to make update"
 					setTimeout () ->
@@ -188,11 +182,27 @@ module.exports = WebsocketController =
 				metrics.set "editor.active-projects", project_id, 0.3
 				metrics.set "editor.active-users", user_id, 0.3
 
-				logger.log {user_id, doc_id, project_id, client_id: client.id, version: update.v, cbc_0, cbc_1}, "sending update to doc updater"
+				logger.log {user_id, doc_id, project_id, client_id: client.id, version: update.v}, "sending update to doc updater"
 
 				DocumentUpdaterManager.queueChange project_id, doc_id, update, (error) ->
 					if error?
 						logger.error {err: error, project_id, doc_id, client_id: client.id, version: update.v}, "document was not available for update"
 						client.disconnect()
 					callback(error)
-					#logger.log {user_id, project_id, doc_id, client_id: client.id}, "applied update"
+
+	_assertClientCanApplyUpdate: (client, doc_id, update, callback) ->
+		AuthorizationManager.assertClientCanEditProjectAndDoc client, doc_id, (error) ->
+			if error?
+				if error.message == "not authorized" and WebsocketController._isCommentUpdate(update)
+					# This might be a comment op, which we only need read-only priveleges for
+					AuthorizationManager.assertClientCanViewProjectAndDoc client, doc_id, callback
+				else
+					return callback(error)
+			else
+				return callback(null)
+	
+	_isCommentUpdate: (update) ->
+		for op in update.op
+			if !op.c?
+				return false
+		return true

services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee

@@ -69,7 +69,7 @@ describe "applyOtUpdate", ->
 				(cb) => rclient.del "PendingUpdates:#{@doc_id}", cb
 			], done
 		
-	describe "when not authorized", ->
+	describe "when authorized to read-only with an edit update", ->
 		before (done) ->
 			async.series [
 				(cb) =>
@@ -109,4 +109,61 @@ describe "applyOtUpdate", ->
 		it "should not put the update in redis", (done) ->
 			rclient.llen "PendingUpdates:#{@doc_id}", (error, len) =>
 				len.should.equal 0
-				done()
+				done()
+				
+	describe "when authorized to read-only with a comment update", ->
+		before (done) ->
+			@comment_update = {
+				op: [{c: "foo", p: 42}]
+			}
+			async.series [
+				(cb) =>
+					FixturesManager.setUpProject {
+						privilegeLevel: "readOnly"
+					}, (e, {@project_id, @user_id}) =>
+						cb(e)
+					
+				(cb) =>
+					FixturesManager.setUpDoc @project_id, {@lines, @version, @ops}, (e, {@doc_id}) =>
+						cb(e)
+
+				(cb) =>
+					@client = RealTimeClient.connect()
+					@client.on "connectionAccepted", cb
+						
+				(cb) =>
+					@client.emit "joinProject", project_id: @project_id, cb
+				
+				(cb) =>
+					@client.emit "joinDoc", @doc_id, cb
+					
+				(cb) =>
+					@client.emit "applyOtUpdate", @doc_id, @comment_update, cb
+			], done
+		
+		it "should push the doc into the pending updates list", (done) ->
+			rclient.lrange "pending-updates-list", 0, -1, (error, [doc_id]) =>
+				doc_id.should.equal "#{@project_id}:#{@doc_id}"
+				done()
+		
+		it "should add the doc to the pending updates set in redis", (done) ->
+			rclient.sismember "DocsWithPendingUpdates", "#{@project_id}:#{@doc_id}", (error, isMember) =>
+				isMember.should.equal 1
+				done()
+
+		it "should push the update into redis", (done) ->
+			rclient.lrange "PendingUpdates:#{@doc_id}", 0, -1, (error, [update]) =>
+				update = JSON.parse(update)
+				update.op.should.deep.equal @comment_update.op
+				update.meta.should.deep.equal {
+					source: @client.socket.sessionid
+					user_id: @user_id
+				}
+				done()
+		
+		after (done) ->
+			async.series [
+				(cb) => rclient.del "pending-updates-list", cb
+				(cb) => rclient.del "DocsWithPendingUpdates", "#{@project_id}:#{@doc_id}", cb
+				(cb) => rclient.del "PendingUpdates:#{@doc_id}", cb
+			], done

services/real-time/test/unit/coffee/WebsocketControllerTests.coffee

@@ -1,6 +1,7 @@
 chai = require('chai')
 should = chai.should()
 sinon = require("sinon")
+expect = chai.expect
 modulePath = "../../../app/js/WebsocketController.js"
 SandboxedModule = require('sandboxed-module')
 tk = require "timekeeper"
@@ -417,7 +418,7 @@ describe 'WebsocketController', ->
 			@update = {op: {p: 12, t: "foo"}}
 			@client.params.user_id = @user_id
 			@client.params.project_id = @project_id
-			@AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub().callsArg(2)
+			@WebsocketController._assertClientCanApplyUpdate = sinon.stub().yields()
 			@DocumentUpdaterManager.queueChange = sinon.stub().callsArg(3)
 
 		describe "succesfully", ->
@@ -465,7 +466,7 @@ describe 'WebsocketController', ->
 		describe "when not authorized", ->
 			beforeEach ->
 				@client.disconnect = sinon.stub()
-				@AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub().callsArgWith(2, @error = new Error("not authorized"))
+				@WebsocketController._assertClientCanApplyUpdate = sinon.stub().yields(@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.
@@ -478,3 +479,41 @@ describe 'WebsocketController', ->
 
 			it "should call the callback with the error", ->
 				@callback.calledWith(@error).should.equal true
+
+	describe "_assertClientCanApplyUpdate", ->
+		beforeEach ->
+			@edit_update = { op: [{i: "foo", p: 42}, {c: "bar", p: 132}] } # comments may still be in an edit op
+			@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)
+				@WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @edit_update, (error) ->
+					expect(error).to.be.null
+					done()
+
+		describe "with a read-only client and an edit op", ->
+			it "should return an error", (done) ->
+				@AuthorizationManager.assertClientCanEditProjectAndDoc.yields(new Error("not authorized"))
+				@AuthorizationManager.assertClientCanViewProjectAndDoc.yields(null)
+				@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"))
+				@AuthorizationManager.assertClientCanViewProjectAndDoc.yields(null)
+				@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"))
+				@AuthorizationManager.assertClientCanViewProjectAndDoc.yields(new Error("not authorized"))
+				@WebsocketController._assertClientCanApplyUpdate @client, @doc_id, @comment_update, (error) ->
+					expect(error.message).to.equal "not authorized"
+					done()