From a49cec68437e47935eaf92d4b353bc86e56950f5 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Mon, 9 Dec 2024 17:43:10 +0100 Subject: [PATCH] [real-time] ensure tc is enabled when reviewer role is used (#22245) * Support for adding reviewer role * show reviewer in track changes user list * added "review" in assertClientCanViewProject * test if reviewer can read project * added collaboratorsGetter tests * eit toggle-track-changes when track changes changes * [real-time] ensure tc is enabled when reviewer role is used * use assertClientCanReviewProjectAndDoc, refactor _assertClientCanApplyUpdate GitOrigin-RevId: 158bd1ff0d4b4977da950134f8ad8b3740855290 --- .../real-time/app/js/AuthorizationManager.js | 17 +++ .../real-time/app/js/WebsocketController.js | 39 ++++--- .../test/unit/js/AuthorizationManagerTests.js | 101 +++++++++++++++++- .../test/unit/js/WebsocketControllerTests.js | 44 +++++++- 4 files changed, 179 insertions(+), 22 deletions(-) diff --git a/services/real-time/app/js/AuthorizationManager.js b/services/real-time/app/js/AuthorizationManager.js index 261e2e91ee..b8633ad4d0 100644 --- a/services/real-time/app/js/AuthorizationManager.js +++ b/services/real-time/app/js/AuthorizationManager.js @@ -18,6 +18,14 @@ module.exports = AuthorizationManager = { ) }, + assertClientCanReviewProject(client, callback) { + AuthorizationManager._assertClientHasPrivilegeLevel( + client, + ['readAndWrite', 'owner', 'review'], + callback + ) + }, + _assertClientHasPrivilegeLevel(client, allowedLevels, callback) { if (allowedLevels.includes(client.ol_context.privilege_level)) { callback(null) @@ -44,6 +52,15 @@ module.exports = AuthorizationManager = { }) }, + assertClientCanReviewProjectAndDoc(client, docId, callback) { + AuthorizationManager.assertClientCanReviewProject(client, function (error) { + if (error) { + return callback(error) + } + AuthorizationManager._assertClientCanAccessDoc(client, docId, callback) + }) + }, + _assertClientCanAccessDoc(client, docId, callback) { if (client.ol_context[`doc:${docId}`] === 'allowed') { callback(null) diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index 4888f56097..dec567709a 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -622,26 +622,25 @@ module.exports = WebsocketController = { }, _assertClientCanApplyUpdate(client, docId, update, callback) { - AuthorizationManager.assertClientCanEditProjectAndDoc( - client, - docId, - function (error) { - if ( - error && - error.message === 'not authorized' && - WebsocketController._isCommentUpdate(update) - ) { - // This might be a comment op, which we only need read-only priveleges for - AuthorizationManager.assertClientCanViewProjectAndDoc( - client, - docId, - callback - ) - return - } - callback(error) - } - ) + if (WebsocketController._isCommentUpdate(update)) { + return AuthorizationManager.assertClientCanViewProjectAndDoc( + client, + docId, + callback + ) + } else if (update.meta?.tc) { + return AuthorizationManager.assertClientCanReviewProjectAndDoc( + client, + docId, + callback + ) + } else { + return AuthorizationManager.assertClientCanEditProjectAndDoc( + client, + docId, + callback + ) + } }, _isCommentUpdate(update) { diff --git a/services/real-time/test/unit/js/AuthorizationManagerTests.js b/services/real-time/test/unit/js/AuthorizationManagerTests.js index 57882eaacc..a23d814806 100644 --- a/services/real-time/test/unit/js/AuthorizationManagerTests.js +++ b/services/real-time/test/unit/js/AuthorizationManagerTests.js @@ -228,7 +228,7 @@ describe('AuthorizationManager', function () { }) }) - return describe('assertClientCanEditProjectAndDoc', function () { + describe('assertClientCanEditProjectAndDoc', function () { beforeEach(function () { this.doc_id = '12345' this.callback = sinon.stub() @@ -326,4 +326,103 @@ describe('AuthorizationManager', function () { }) }) }) + + return describe('assertClientCanReviewProjectAndDoc', function () { + beforeEach(function () { + this.doc_id = '12345' + this.callback = sinon.stub() + return (this.client.ol_context = {}) + }) + + describe('when not authorised at the project level', function () { + beforeEach(function () { + return (this.client.ol_context.privilege_level = 'readOnly') + }) + + it('should not allow access', function () { + return this.AuthorizationManager.assertClientCanReviewProjectAndDoc( + this.client, + this.doc_id, + err => err.message.should.equal('not authorized') + ) + }) + + return describe('even when authorised at the doc level', function () { + beforeEach(function (done) { + return this.AuthorizationManager.addAccessToDoc( + this.client, + this.doc_id, + done + ) + }) + + return it('should not allow access', function () { + return this.AuthorizationManager.assertClientCanReviewProjectAndDoc( + this.client, + this.doc_id, + err => err.message.should.equal('not authorized') + ) + }) + }) + }) + + return describe('when authorised at the project level', function () { + beforeEach(function () { + return (this.client.ol_context.privilege_level = 'review') + }) + + describe('and not authorised at the document level', function () { + return it('should not allow access', function () { + return this.AuthorizationManager.assertClientCanReviewProjectAndDoc( + this.client, + this.doc_id, + err => err.message.should.equal('not authorized') + ) + }) + }) + + describe('and authorised at the document level', function () { + beforeEach(function (done) { + return this.AuthorizationManager.addAccessToDoc( + this.client, + this.doc_id, + done + ) + }) + + return it('should allow access', function () { + this.AuthorizationManager.assertClientCanReviewProjectAndDoc( + this.client, + this.doc_id, + this.callback + ) + return this.callback.calledWith(null).should.equal(true) + }) + }) + + return describe('when document authorisation is added and then removed', function () { + beforeEach(function (done) { + return this.AuthorizationManager.addAccessToDoc( + this.client, + this.doc_id, + () => { + return this.AuthorizationManager.removeAccessToDoc( + this.client, + this.doc_id, + done + ) + } + ) + }) + + return it('should deny access', function () { + return this.AuthorizationManager.assertClientCanReviewProjectAndDoc( + this.client, + this.doc_id, + err => err.message.should.equal('not authorized') + ) + }) + }) + }) + }) }) diff --git a/services/real-time/test/unit/js/WebsocketControllerTests.js b/services/real-time/test/unit/js/WebsocketControllerTests.js index 532a93d5be..1f3ca67724 100644 --- a/services/real-time/test/unit/js/WebsocketControllerTests.js +++ b/services/real-time/test/unit/js/WebsocketControllerTests.js @@ -1578,6 +1578,8 @@ describe('WebsocketController', function () { } // comments may still be in an edit op this.comment_update = { op: [{ c: 'bar', p: 132 }] } this.AuthorizationManager.assertClientCanEditProjectAndDoc = sinon.stub() + this.AuthorizationManager.assertClientCanReviewProjectAndDoc = + sinon.stub() return (this.AuthorizationManager.assertClientCanViewProjectAndDoc = sinon.stub()) }) @@ -1633,7 +1635,7 @@ describe('WebsocketController', function () { }) }) - return describe('with a totally unauthorized client', function () { + describe('with a totally unauthorized client', function () { return it('should return an error', function (done) { this.AuthorizationManager.assertClientCanEditProjectAndDoc.yields( new Error('not authorized') @@ -1652,5 +1654,45 @@ describe('WebsocketController', function () { ) }) }) + + describe('with a review client', function () { + it('op with tc should succeed', function (done) { + this.AuthorizationManager.assertClientCanEditProjectAndDoc.yields( + new Error('not authorized') + ) + this.AuthorizationManager.assertClientCanViewProjectAndDoc.yields(null) + this.AuthorizationManager.assertClientCanReviewProjectAndDoc.yields( + null + ) + return this.WebsocketController._assertClientCanApplyUpdate( + this.client, + this.doc_id, + { op: [{ p: 10, i: 'a' }], meta: { tc: '123456' } }, + error => { + expect(error).to.be.null + return done() + } + ) + }) + + return it('op without tc should fail', function (done) { + this.AuthorizationManager.assertClientCanEditProjectAndDoc.yields( + new Error('not authorized') + ) + this.AuthorizationManager.assertClientCanViewProjectAndDoc.yields(null) + this.AuthorizationManager.assertClientCanReviewProjectAndDoc.yields( + null + ) + return this.WebsocketController._assertClientCanApplyUpdate( + this.client, + this.doc_id, + { op: [{ p: 10, i: 'a' }] }, + error => { + expect(error.message).to.equal('not authorized') + return done() + } + ) + }) + }) }) })