From 459904c0ef6a97acd5696a54ff843d223034adb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Wed, 12 Aug 2020 16:17:50 +0200 Subject: [PATCH] Merge pull request #2975 from overleaf/cmg-sk-restricted-users-names Hide data from restricted users in history, setting history label usernames on server side GitOrigin-RevId: 820d92064d2c0bc99ba756cca4be1adab0da5753 --- .../Features/Editor/EditorHttpController.js | 2 + .../src/Features/History/HistoryController.js | 85 ++++++++++++++++++- .../project/editor/history/entriesListV2.pug | 9 +- .../history/components/historyLabelsList.js | 17 ++-- .../src/Editor/EditorHttpControllerTests.js | 6 +- .../src/History/HistoryControllerTests.js | 1 + 6 files changed, 102 insertions(+), 18 deletions(-) diff --git a/services/web/app/src/Features/Editor/EditorHttpController.js b/services/web/app/src/Features/Editor/EditorHttpController.js index 8d3573cc8c..afc203b249 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.js +++ b/services/web/app/src/Features/Editor/EditorHttpController.js @@ -68,8 +68,10 @@ async function joinProject(req, res, next) { } // Hide access tokens if this is not the project owner TokenAccessHandler.protectTokens(project, privilegeLevel) + // Hide sensitive data if the user is restricted if (isRestrictedUser) { project.owner = { _id: project.owner._id } + project.members = [] } // Only show the 'renamed or deleted' message once if (project.deletedByExternalDataSource) { diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index e601cf3654..52723f7ddf 100644 --- a/services/web/app/src/Features/History/HistoryController.js +++ b/services/web/app/src/Features/History/HistoryController.js @@ -21,6 +21,7 @@ const logger = require('logger-sharelatex') const request = require('request') const settings = require('settings-sharelatex') const AuthenticationController = require('../Authentication/AuthenticationController') +const UserGetter = require('../User/UserGetter') const Errors = require('../Errors/Errors') const HistoryManager = require('./HistoryManager') const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler') @@ -201,7 +202,12 @@ module.exports = HistoryController = { if (error != null) { return next(error) } - return res.json(labels) + HistoryController._enrichLabels(labels, (err, labels) => { + if (err) { + return next(err) + } + return res.json(labels) + }) } ) }, @@ -222,11 +228,86 @@ module.exports = HistoryController = { if (error != null) { return next(error) } - return res.json(label) + HistoryController._enrichLabel(label, (err, label) => { + if (err) { + return next(err) + } + return res.json(label) + }) } ) }, + _enrichLabel(label, callback) { + if (!label.user_id) { + return callback(null, label) + } + UserGetter.getUser( + label.user_id, + { first_name: 1, last_name: 1, email: 1 }, + (err, user) => { + if (err) { + return callback(err) + } + const newLabel = Object.assign({}, label) + newLabel.user_display_name = HistoryController._displayNameForUser(user) + callback(null, newLabel) + } + ) + }, + + _enrichLabels(labels, callback) { + if (!labels || !labels.length) { + return callback(null, []) + } + const uniqueUsers = new Set(labels.map(label => label.user_id)) + + // For backwards compatibility expect missing user_id fields + uniqueUsers.delete(undefined) + + if (!uniqueUsers.size) { + return callback(null, labels) + } + + UserGetter.getUsers( + Array.from(uniqueUsers), + { first_name: 1, last_name: 1, email: 1 }, + function(err, rawUsers) { + if (err) { + return callback(err) + } + const users = new Map(rawUsers.map(user => [String(user._id), user])) + + labels.forEach(label => { + const user = users.get(label.user_id) + if (!user) return + label.user_display_name = HistoryController._displayNameForUser(user) + }) + callback(null, labels) + } + ) + }, + + _displayNameForUser(user) { + if (user == null) { + return 'Anonymous' + } + if (user.name != null) { + return user.name + } + let name = [user.first_name, user.last_name] + .filter(n => n != null) + .join(' ') + .trim() + if (name === '') { + name = user.email.split('@')[0] + } + if (name == null || name === '') { + return '?' + } + return name + }, + deleteLabel(req, res, next) { const project_id = req.params.Project_id const { label_id } = req.params diff --git a/services/web/app/views/project/editor/history/entriesListV2.pug b/services/web/app/views/project/editor/history/entriesListV2.pug index 4e05ed4b12..300bfde368 100644 --- a/services/web/app/views/project/editor/history/entriesListV2.pug +++ b/services/web/app/views/project/editor/history/entriesListV2.pug @@ -127,10 +127,11 @@ script(type="text/ng-template", id="historyEntryTpl") history-draggable-boundary-on-drag-stop="$ctrl.onDraggingStop(isValidDrop, boundary)" ) - history-label( + history-label( ng-repeat="label in $ctrl.entry.labels | orderBy : '-created_at'" + ng-init="user = $ctrl.buildUserView(label)" label-text="label.comment" - label-owner-name="$ctrl.displayNameById(label.user_id)" + label-owner-name="$ctrl.displayNameById(label.user_id) || 'Anonymous'" label-creation-date-time="label.created_at" is-owned-by-current-user="label.user_id === $ctrl.currentUser.id" on-label-delete="$ctrl.onLabelDelete({ label: label })" @@ -223,13 +224,13 @@ script(type="text/ng-template", id="historyLabelsListTpl") .history-entry-label-metadata .history-entry-label-metadata-user( ng-if="!label.isPseudoCurrentStateLabel" - ng-init="user = $ctrl.getUserById(label.user_id)" + ng-init="user = $ctrl.buildUserView(label)" ) | Saved by span.name( ng-if="user && user._id !== $ctrl.currentUser.id" ng-style="$ctrl.getUserCSSStyle(user, versionWithLabel);" - ) {{ ::$ctrl.displayName(user) }} + ) {{ ::user.displayName }} span.name( ng-if="user && user._id == $ctrl.currentUser.id" ng-style="$ctrl.getUserCSSStyle(user, versionWithLabel);" diff --git a/services/web/frontend/js/ide/history/components/historyLabelsList.js b/services/web/frontend/js/ide/history/components/historyLabelsList.js index 9bd1f4c0c4..44b803e8e7 100644 --- a/services/web/frontend/js/ide/history/components/historyLabelsList.js +++ b/services/web/frontend/js/ide/history/components/historyLabelsList.js @@ -129,16 +129,13 @@ const historyLabelsListController = function($scope, $element, $attrs) { }) } } - // This method (and maybe the one below) will be removed soon. User details data will be - // injected into the history API responses, so we won't need to fetch user data from other - // local data structures. - ctrl.getUserById = id => - _.find(ctrl.users, function(user) { - const curUserId = - (user != null ? user._id : undefined) || - (user != null ? user.id : undefined) - return curUserId === id - }) + ctrl.buildUserView = label => { + const user = { + _id: label.user_id, + displayName: label.user_display_name + } + return user + } ctrl.displayName = displayNameForUser ctrl.getUserCSSStyle = function(user, versionWithLabel) { const curUserId = diff --git a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js index 71e83f7636..a7412660dc 100644 --- a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js +++ b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js @@ -23,11 +23,13 @@ describe('EditorHttpController', function() { _id: 'owner', email: 'owner@example.com', other_property: true - } + }, + members: [{ one: 1 }, { two: 2 }] } this.reducedProjectView = { _id: this.projectView._id, - owner: { _id: this.projectView.owner._id } + owner: { _id: this.projectView.owner._id }, + members: [] } this.doc = { _id: new ObjectId(), name: 'excellent-original-idea.tex' } this.file = { _id: new ObjectId() } diff --git a/services/web/test/unit/src/History/HistoryControllerTests.js b/services/web/test/unit/src/History/HistoryControllerTests.js index 76702b0df5..9af49a471c 100644 --- a/services/web/test/unit/src/History/HistoryControllerTests.js +++ b/services/web/test/unit/src/History/HistoryControllerTests.js @@ -43,6 +43,7 @@ describe('HistoryController', function() { './HistoryManager': (this.HistoryManager = {}), '../Project/ProjectDetailsHandler': (this.ProjectDetailsHandler = {}), '../Project/ProjectEntityUpdateHandler': (this.ProjectEntityUpdateHandler = {}), + '../User/UserGetter': (this.UserGetter = {}), './RestoreManager': (this.RestoreManager = {}) } })