From d8717a06a2ab76a25195b5a865f172648998e952 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 19 Oct 2017 14:42:17 +0100 Subject: [PATCH] Fix track-changes with token-access --- .../Collaborators/CollaboratorsHandler.coffee | 7 ++++ .../Editor/EditorHttpController.coffee | 26 +++++++------ .../Project/ProjectEditorHandler.coffee | 23 ++++++++--- .../app/views/project/editor/review-panel.pug | 13 +++++++ .../controllers/ReviewPanelController.coffee | 16 ++++++-- .../stylesheets/app/editor/review-panel.less | 5 +++ .../CollaboratorsHandlerTests.coffee | 26 +++++++++++++ .../Editor/EditorHttpControllerTests.coffee | 7 ++++ .../Project/ProjectEditorHandlerTests.coffee | 39 +++++++++++++------ 9 files changed, 131 insertions(+), 31 deletions(-) diff --git a/services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee b/services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee index 2749fab545..f676d156b9 100644 --- a/services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee +++ b/services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee @@ -102,6 +102,13 @@ module.exports = CollaboratorsHandler = CollaboratorsHandler._loadMembers members, (error, users) -> callback error, users + getTokenMembersWithPrivilegeLevels: (project_id, callback = (error, members) ->) -> + CollaboratorsHandler.getMemberIdsWithPrivilegeLevels project_id, (error, members = []) -> + return callback(error) if error? + members = members.filter((m) -> m.source == Sources.TOKEN) + CollaboratorsHandler._loadMembers members, (error, users) -> + callback error, users + getMemberIdPrivilegeLevel: (user_id, project_id, callback = (error, privilegeLevel) ->) -> # In future if the schema changes and getting all member ids is more expensive (multiple documents) # then optimise this. diff --git a/services/web/app/coffee/Features/Editor/EditorHttpController.coffee b/services/web/app/coffee/Features/Editor/EditorHttpController.coffee index eb22334684..fc1e2dd3c0 100644 --- a/services/web/app/coffee/Features/Editor/EditorHttpController.coffee +++ b/services/web/app/coffee/Features/Editor/EditorHttpController.coffee @@ -41,21 +41,23 @@ module.exports = EditorHttpController = return callback(new Error("not found")) if !project? CollaboratorsHandler.getInvitedMembersWithPrivilegeLevels project, (error, members) -> return callback(error) if error? - UserGetter.getUser user_id, { isAdmin: true }, (error, user) -> + CollaboratorsHandler.getTokenMembersWithPrivilegeLevels project, (error, tokenMembers) -> return callback(error) if error? - token = TokenAccessHandler.getRequestToken(req, project_id) - AuthorizationManager.getPrivilegeLevelForProject user_id, project_id, token, (error, privilegeLevel) -> + UserGetter.getUser user_id, { isAdmin: true }, (error, user) -> return callback(error) if error? - if !privilegeLevel? or privilegeLevel == PrivilegeLevels.NONE - logger.log {project_id, user_id, privilegeLevel}, "not an acceptable privilege level, returning null" - return callback null, null, false - CollaboratorsInviteHandler.getAllInvites project_id, (error, invites) -> + token = TokenAccessHandler.getRequestToken(req, project_id) + AuthorizationManager.getPrivilegeLevelForProject user_id, project_id, token, (error, privilegeLevel) -> return callback(error) if error? - logger.log {project_id, user_id, memberCount: members.length, inviteCount: invites.length, privilegeLevel}, "returning project model view" - callback(null, - ProjectEditorHandler.buildProjectModelView(project, members, invites), - privilegeLevel - ) + if !privilegeLevel? or privilegeLevel == PrivilegeLevels.NONE + logger.log {project_id, user_id, privilegeLevel}, "not an acceptable privilege level, returning null" + return callback null, null, false + CollaboratorsInviteHandler.getAllInvites project_id, (error, invites) -> + return callback(error) if error? + logger.log {project_id, user_id, memberCount: members.length, inviteCount: invites.length, privilegeLevel}, "returning project model view" + callback(null, + ProjectEditorHandler.buildProjectModelView(project, members, invites, tokenMembers), + privilegeLevel + ) restoreDoc: (req, res, next) -> project_id = req.params.Project_id diff --git a/services/web/app/coffee/Features/Project/ProjectEditorHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEditorHandler.coffee index 65cf2f5398..38fdd8a294 100644 --- a/services/web/app/coffee/Features/Project/ProjectEditorHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEditorHandler.coffee @@ -3,7 +3,7 @@ _ = require("underscore") module.exports = ProjectEditorHandler = trackChangesAvailable: false - buildProjectModelView: (project, members, invites) -> + buildProjectModelView: (project, members, invites, tokenMembers=[]) -> result = _id : project._id name : project.name @@ -17,15 +17,20 @@ module.exports = ProjectEditorHandler = deletedByExternalDataSource : project.deletedByExternalDataSource || false deletedDocs: project.deletedDocs members: [] + tokenMembers: [] invites: invites tokens: project.tokens if !result.invites? result.invites = [] - - {owner, ownerFeatures, members} = @buildOwnerAndMembersViews(members) + + {owner, ownerFeatures, members, tokenMembers} = @buildOwnerAndMembersViews( + members, + tokenMembers + ) result.owner = owner result.members = members + result.tokenMembers = tokenMembers result.features = _.defaults(ownerFeatures or {}, { collaborators: -1 # Infinite @@ -41,17 +46,25 @@ module.exports = ProjectEditorHandler = return result - buildOwnerAndMembersViews: (members) -> + buildOwnerAndMembersViews: (members, tokenMembers) -> owner = null ownerFeatures = null filteredMembers = [] + filteredTokenMembers = [] for member in members if member.privilegeLevel == "owner" ownerFeatures = member.user.features owner = @buildUserModelView member.user, "owner" else filteredMembers.push @buildUserModelView member.user, member.privilegeLevel - {owner: owner, ownerFeatures: ownerFeatures, members: filteredMembers} + for tokenMember in tokenMembers + filteredTokenMembers.push @buildUserModelView tokenMember.user, tokenMember.privilegeLevel + return { + owner: owner, + ownerFeatures: ownerFeatures, + members: filteredMembers, + tokenMembers: filteredTokenMembers + } buildUserModelView: (user, privileges) -> _id : user._id diff --git a/services/web/app/views/project/editor/review-panel.pug b/services/web/app/views/project/editor/review-panel.pug index 6db177eba8..0736f26451 100644 --- a/services/web/app/views/project/editor/review-panel.pug +++ b/services/web/app/views/project/editor/review-panel.pug @@ -80,6 +80,19 @@ on-toggle="toggleTrackChangesForUser(isOn, member.id);" disabled="reviewPanel.trackChangesOnForEveryone || !project.features.trackChanges || !permissions.write" ) + li.rp-tc-state-separator + li.rp-tc-state-item( + ng-repeat="member in reviewPanel.formattedProjectTokenMembers" + ) + span.rp-tc-state-item-name( + ng-class="{ 'rp-tc-state-item-name-disabled' : reviewPanel.trackChangesOnForEveryone}" + style="color: hsl({{ member.hue }}, 70%, 40%);" + ) {{ member.name }} + review-panel-toggle( + ng-model="reviewPanel.trackChangesState[member.id].value" + on-toggle="toggleTrackChangesForUser(isOn, member.id);" + disabled="reviewPanel.trackChangesOnForEveryone || !project.features.trackChanges || !permissions.write" + ) .rp-entry-list( review-panel-sorted diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 998c1ceefb..dd80809dca 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -32,6 +32,7 @@ define [ resolvedThreadIds: {} rendererData: {} formattedProjectMembers: {} + formattedProjectTokenMembers: {} fullTCStateCollapsed: true loadingThreads: false # All selected changes. If a aggregated change (insertion + deletion) is selection, the two ids @@ -77,6 +78,13 @@ define [ if member.privileges == "readAndWrite" $scope.reviewPanel.formattedProjectMembers[member._id] = formatUser(member) + $scope.$watch "project.tokenMembers", (tokenMembers) -> + $scope.reviewPanel.formattedProjectTokenMembers = {} + if $scope.project?.tokenMembers? + for member in tokenMembers + if member.privileges == "readAndWrite" + $scope.reviewPanel.formattedProjectTokenMembers[member._id] = formatUser(member) + $scope.commentState = adding: false content: "" @@ -612,9 +620,10 @@ define [ _setEveryoneTCState = (newValue, isLocal = false) -> $scope.reviewPanel.trackChangesOnForEveryone = newValue - for member in $scope.project.members + project = $scope.project + for member in project.members.concat(project.tokenMembers) _setUserTCState(member._id, newValue, isLocal) - _setUserTCState($scope.project.owner._id, newValue, isLocal) + _setUserTCState(project.owner._id, newValue, isLocal) applyClientTrackChangesStateToServer = () -> if $scope.reviewPanel.trackChangesOnForEveryone @@ -630,8 +639,9 @@ define [ if typeof state is "boolean" _setEveryoneTCState state else + project = $scope.project $scope.reviewPanel.trackChangesOnForEveryone = false - for member in $scope.project.members + for member in project.members.concat(project.tokenMembers) _setUserTCState(member._id, state[member._id] ? false) _setUserTCState($scope.project.owner._id, state[$scope.project.owner._id] ? false) diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 13b8b7e03f..f525f36f4e 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -188,6 +188,11 @@ padding-bottom: 5px; } } + .rp-tc-state-separator { + border-bottom: 1px solid @rp-border-grey; + // margin-top: 4px; + // margin-bottom: 4px; + } .rp-tc-state-item-everyone { border-bottom: 1px solid @rp-border-grey; color: @red; diff --git a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee index 5c68fd9ef6..845449b3b1 100644 --- a/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee @@ -141,6 +141,32 @@ describe "CollaboratorsHandler", -> ]) .should.equal true + describe "getTokenMembersWithPrivilegeLevels", -> + beforeEach -> + @CollaboratorHandler.getMemberIdsWithPrivilegeLevels = sinon.stub() + @CollaboratorHandler.getMemberIdsWithPrivilegeLevels.withArgs(@project_id).yields(null, [ + { id: "read-only-ref-1", privilegeLevel: "readOnly", source: 'token' } + { id: "read-only-ref-2", privilegeLevel: "readOnly", source: 'invite' } + { id: "read-write-ref-1", privilegeLevel: "readAndWrite", source: 'token' } + { id: "read-write-ref-2", privilegeLevel: "readAndWrite", source: 'invite' } + { id: "doesnt-exist", privilegeLevel: "readAndWrite", source: 'invite' } + ]) + @UserGetter.getUserOrUserStubById = sinon.stub() + @UserGetter.getUserOrUserStubById.withArgs("read-only-ref-1").yields(null, { _id: "read-only-ref-1" }) + @UserGetter.getUserOrUserStubById.withArgs("read-only-ref-2").yields(null, { _id: "read-only-ref-2" }) + @UserGetter.getUserOrUserStubById.withArgs("read-write-ref-1").yields(null, { _id: "read-write-ref-1" }) + @UserGetter.getUserOrUserStubById.withArgs("read-write-ref-2").yields(null, { _id: "read-write-ref-2" }) + @UserGetter.getUserOrUserStubById.withArgs("doesnt-exist").yields(null, null) + @CollaboratorHandler.getTokenMembersWithPrivilegeLevels @project_id, @callback + + it "should return an array of token members with their privilege levels", -> + @callback + .calledWith(null, [ + { user: { _id: "read-only-ref-1" }, privilegeLevel: "readOnly" } + { user: { _id: "read-write-ref-1" }, privilegeLevel: "readAndWrite"} + ]) + .should.equal true + describe "getMemberIdPrivilegeLevel", -> beforeEach -> @CollaboratorHandler.getMemberIdsWithPrivilegeLevels = sinon.stub() diff --git a/services/web/test/UnitTests/coffee/Editor/EditorHttpControllerTests.coffee b/services/web/test/UnitTests/coffee/Editor/EditorHttpControllerTests.coffee index 2ee9a10a93..258b6aafbe 100644 --- a/services/web/test/UnitTests/coffee/Editor/EditorHttpControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Editor/EditorHttpControllerTests.coffee @@ -101,6 +101,7 @@ describe "EditorHttpController", -> _id: @user_id = "user-id" projects: {} @members = ["members", "mock"] + @tokenMembers = ['one', 'two'] @projectModelView = _id: @project_id owner:{_id:"something"} @@ -112,6 +113,7 @@ describe "EditorHttpController", -> @ProjectEditorHandler.buildProjectModelView = sinon.stub().returns(@projectModelView) @ProjectGetter.getProjectWithoutDocLines = sinon.stub().callsArgWith(1, null, @project) @CollaboratorsHandler.getInvitedMembersWithPrivilegeLevels = sinon.stub().callsArgWith(1, null, @members) + @CollaboratorsHandler.getTokenMembersWithPrivilegeLevels = sinon.stub().callsArgWith(1, null, @tokenMembers) @CollaboratorsInviteHandler.getAllInvites = sinon.stub().callsArgWith(1, null, @invites) @UserGetter.getUser = sinon.stub().callsArgWith(2, null, @user) @@ -131,6 +133,11 @@ describe "EditorHttpController", -> .calledWith(@project) .should.equal true + it "should get the list of users who access the project via token links", -> + @CollaboratorsHandler.getTokenMembersWithPrivilegeLevels + .calledWith(@project) + .should.equal true + it "should look up the user", -> @UserGetter.getUser .calledWith(@user_id, { isAdmin: true }) diff --git a/services/web/test/UnitTests/coffee/Project/ProjectEditorHandlerTests.coffee b/services/web/test/UnitTests/coffee/Project/ProjectEditorHandlerTests.coffee index dc68e5dda6..b840387f61 100644 --- a/services/web/test/UnitTests/coffee/Project/ProjectEditorHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/Project/ProjectEditorHandlerTests.coffee @@ -68,6 +68,15 @@ describe "ProjectEditorHandler", -> }, privilegeLevel: "readAndWrite" }] + @tokenMembers = [{ + user: { + _id: "token-read-only-id" + first_name : "TRead" + last_name : "TOnly" + email : "token-read-only@sharelatex.com" + }, + privilegeLevel: "readOnly" + }] @invites = [ {_id: "invite_one", email: "user-one@example.com", privileges: "readOnly", projectId: @project._id} {_id: "invite_two", email: "user-two@example.com", privileges: "readOnly", projectId: @project._id} @@ -77,7 +86,7 @@ describe "ProjectEditorHandler", -> describe "buildProjectModelView", -> describe "with owner and members included", -> beforeEach -> - @result = @handler.buildProjectModelView @project, @members, @invites + @result = @handler.buildProjectModelView @project, @members, @invites, @tokenMembers it "should include the id", -> should.exist @result._id @@ -127,6 +136,10 @@ describe "ProjectEditorHandler", -> findMember("read-write-id").last_name.should.equal "Write" findMember("read-write-id").email.should.equal "read-write@sharelatex.com" + it 'should include a list of tokenMembers', -> + @result.tokenMembers.length.should.equal 1 + @result.tokenMembers[0]._id.should.equal @tokenMembers[0].user._id + it "should include folders in the project", -> @result.rootFolder[0]._id.should.equal "root-folder-id" @result.rootFolder[0].name.should.equal "" @@ -157,16 +170,16 @@ describe "ProjectEditorHandler", -> it "should set the deletedByExternalDataSource flag to false when it is not there", -> delete @project.deletedByExternalDataSource - result = @handler.buildProjectModelView @project, @members + result = @handler.buildProjectModelView @project, @members, [], [] result.deletedByExternalDataSource.should.equal false it "should set the deletedByExternalDataSource flag to false when it is false", -> - result = @handler.buildProjectModelView @project, @members + result = @handler.buildProjectModelView @project, @members, [], [] result.deletedByExternalDataSource.should.equal false it "should set the deletedByExternalDataSource flag to true when it is true", -> @project.deletedByExternalDataSource = true - result = @handler.buildProjectModelView @project, @members + result = @handler.buildProjectModelView @project, @members, [], [] result.deletedByExternalDataSource.should.equal true describe "features", -> @@ -176,7 +189,7 @@ describe "ProjectEditorHandler", -> collaborators: 3 compileGroup:"priority" compileTimeout: 96 - @result = @handler.buildProjectModelView @project, @members + @result = @handler.buildProjectModelView @project, @members, [], [] it "should copy the owner features to the project", -> @result.features.versioning.should.equal @owner.features.versioning @@ -191,10 +204,10 @@ describe "ProjectEditorHandler", -> collaborators: 3 compileGroup:"priority" compileTimeout: 22 - @result = @handler.buildOwnerAndMembersViews @members + @result = @handler.buildOwnerAndMembersViews @members, @tokenMembers - it 'should produce an object with owner, ownerFeatures and members keys', -> - expect(@result).to.have.all.keys ['owner', 'ownerFeatures', 'members'] + it 'should produce an object with the right keys', -> + expect(@result).to.have.all.keys ['owner', 'ownerFeatures', 'members', 'tokenMembers'] it 'should separate the owner from the members', -> @result.members.length.should.equal(@members.length-1) @@ -202,6 +215,10 @@ describe "ProjectEditorHandler", -> expect(@result.owner.email).to.equal @owner.email expect(@result.members.filter((m) => m._id == @owner._id).length).to.equal 0 + it 'should include a list of tokenMembers', -> + @result.tokenMembers.length.should.equal 1 + @result.tokenMembers[0]._id.should.equal @tokenMembers[0].user._id + it 'should extract the ownerFeatures from the owner object', -> expect(@result.ownerFeatures).to.deep.equal @owner.features @@ -209,10 +226,10 @@ describe "ProjectEditorHandler", -> beforeEach -> # remove the owner from members list @membersWithoutOwner = @members.filter((m) => m.user._id != @owner._id) - @result = @handler.buildOwnerAndMembersViews @membersWithoutOwner + @result = @handler.buildOwnerAndMembersViews @membersWithoutOwner, @tokenMembers - it 'should produce an object with owner, ownerFeatures and members keys', -> - expect(@result).to.have.all.keys ['owner', 'ownerFeatures', 'members'] + it 'should produce an object with the right keys', -> + expect(@result).to.have.all.keys ['owner', 'ownerFeatures', 'members', 'tokenMembers'] it 'should not separate out an owner', -> @result.members.length.should.equal @membersWithoutOwner.length