diff --git a/services/track-changes/app/coffee/UpdatesManager.coffee b/services/track-changes/app/coffee/UpdatesManager.coffee index 1014001835..7261b3f341 100644 --- a/services/track-changes/app/coffee/UpdatesManager.coffee +++ b/services/track-changes/app/coffee/UpdatesManager.coffee @@ -79,7 +79,8 @@ module.exports = UpdatesManager = fillUserInfo: (updates, callback = (error, updates) ->) -> users = {} for update in updates - users[update.meta.user_id] = true + if UpdatesManager._validUserId(update.meta.user_id) + users[update.meta.user_id] = true jobs = [] for user_id, _ of users @@ -95,5 +96,12 @@ module.exports = UpdatesManager = for update in updates user_id = update.meta.user_id delete update.meta.user_id - update.meta.user = users[user_id] + if UpdatesManager._validUserId(user_id) + update.meta.user = users[user_id] callback null, updates + + _validUserId: (user_id) -> + if !user_id? + return false + else + return !!user_id.match(/^[a-f0-9]{24}$/) diff --git a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee index 6720548b26..85dc9a1bd6 100644 --- a/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee +++ b/services/track-changes/test/acceptance/coffee/GettingUpdatesTests.coffee @@ -33,7 +33,7 @@ describe "Getting updates", -> v: 3 }, { op: [{ i: "two ", p: 4 }] - meta: { ts: @to - 2 * @minutes, user_id: @user_id } + meta: { ts: @to - 2 * @minutes } v: 4 }, { op: [{ i: "three ", p: 8 }] @@ -71,6 +71,5 @@ describe "Getting updates", -> meta: start_ts: @to - 2 * @minutes end_ts: @to - 2 * @minutes - user: @user v: 4 }] diff --git a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee index fd2056e38d..67fc7b0d14 100644 --- a/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee +++ b/services/track-changes/test/unit/coffee/UpdatesManager/UpdatesManagerTests.coffee @@ -243,64 +243,92 @@ describe "UpdatesManager", -> @callback.calledWith(null, @updatesWithUserInfo).should.equal true describe "fillUserInfo", -> - beforeEach (done) -> - @user_id_1 = "user-id-1" - @user_id_2 = "user-id-2" - @updates = [{ - meta: - user_id: @user_id_1 - op: "mock-op-1" - }, { - meta: - user_id: @user_id_1 - op: "mock-op-2" - }, { - meta: - user_id: @user_id_2 - op: "mock-op-3" - }] - @user_info = - "user-id-1": { - email: "user1@sharelatex.com" - } - "user-id-2": { - email: "user2@sharelatex.com" - } - @WebApiManager.getUserInfo = (user_id, callback = (error, userInfo) ->) => - callback null, @user_info[user_id] - sinon.spy @WebApiManager, "getUserInfo" + describe "with valid users", -> + beforeEach (done) -> + {ObjectId} = require "mongojs" + @user_id_1 = ObjectId().toString() + @user_id_2 = ObjectId().toString() + @updates = [{ + meta: + user_id: @user_id_1 + op: "mock-op-1" + }, { + meta: + user_id: @user_id_1 + op: "mock-op-2" + }, { + meta: + user_id: @user_id_2 + op: "mock-op-3" + }] + @user_info = {} + @user_info[@user_id_1] = email: "user1@sharelatex.com" + @user_info[@user_id_2] = email: "user2@sharelatex.com" + + @WebApiManager.getUserInfo = (user_id, callback = (error, userInfo) ->) => + callback null, @user_info[user_id] + sinon.spy @WebApiManager, "getUserInfo" - @UpdatesManager.fillUserInfo @updates, (error, @results) => - done() - - it "should only call getUserInfo once for each user_id", -> - @WebApiManager.getUserInfo.calledTwice.should.equal true - @WebApiManager.getUserInfo - .calledWith(@user_id_1) - .should.equal true - @WebApiManager.getUserInfo - .calledWith(@user_id_2) - .should.equal true - - it "should return the updates with the user info filled", -> - expect(@results).to.deep.equal [{ - meta: - user: - email: "user1@sharelatex.com" - op: "mock-op-1" - }, { - meta: - user: - email: "user1@sharelatex.com" - op: "mock-op-2" - }, { - meta: - user: - email: "user2@sharelatex.com" - op: "mock-op-3" - }] + @UpdatesManager.fillUserInfo @updates, (error, @results) => + done() + it "should only call getUserInfo once for each user_id", -> + @WebApiManager.getUserInfo.calledTwice.should.equal true + @WebApiManager.getUserInfo + .calledWith(@user_id_1) + .should.equal true + @WebApiManager.getUserInfo + .calledWith(@user_id_2) + .should.equal true + it "should return the updates with the user info filled", -> + expect(@results).to.deep.equal [{ + meta: + user: + email: "user1@sharelatex.com" + op: "mock-op-1" + }, { + meta: + user: + email: "user1@sharelatex.com" + op: "mock-op-2" + }, { + meta: + user: + email: "user2@sharelatex.com" + op: "mock-op-3" + }] + + + describe "with invalid user ids", -> + beforeEach (done) -> + @updates = [{ + meta: + user_id: null + op: "mock-op-1" + }, { + meta: + user_id: "anonymous-user" + op: "mock-op-2" + }] + @WebApiManager.getUserInfo = (user_id, callback = (error, userInfo) ->) => + callback null, @user_info[user_id] + sinon.spy @WebApiManager, "getUserInfo" + + @UpdatesManager.fillUserInfo @updates, (error, @results) => + done() + + it "should not call getUserInfo", -> + @WebApiManager.getUserInfo.called.should.equal false + + it "should return the updates without the user info filled", -> + expect(@results).to.deep.equal [{ + meta: {} + op: "mock-op-1" + }, { + meta: {} + op: "mock-op-2" + }]