From f625b22ec5a563f6822cc8eb231f8871c0c6f648 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Mon, 5 Mar 2018 10:46:51 +0000 Subject: [PATCH] Move the geolocation out of login, to the `editingSession` action --- .../Analytics/AnalyticsController.coffee | 8 ++-- .../Analytics/AnalyticsManager.coffee | 10 +++-- .../AuthenticationController.coffee | 16 +++---- .../Analytics/AnalyticsControllerTests.coffee | 7 ++- .../AuthenticationControllerTests.coffee | 44 ------------------- 5 files changed, 21 insertions(+), 64 deletions(-) diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee index 24173f2aed..88aa757e92 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsController.coffee @@ -1,16 +1,18 @@ AnalyticsManager = require "./AnalyticsManager" Errors = require "../Errors/Errors" AuthenticationController = require("../Authentication/AuthenticationController") +GeoIpLookup = require '../../infrastructure/GeoIpLookup' module.exports = AnalyticsController = updateEditingSession: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) projectId = req.params.projectId - countryCode = req.session.countryCode || null if userId? - AnalyticsManager.updateEditingSession userId, projectId, countryCode, {}, (error) -> - respondWith(error, res, next) + GeoIpLookup.getDetails req.ip, (err, geoDetails) -> + countryCode = geoDetails?.country_code || null + AnalyticsManager.updateEditingSession userId, projectId, countryCode, {}, (error) -> + respondWith(error, res, next) else res.send 204 diff --git a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee index be6b821183..6bf3943c81 100644 --- a/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee +++ b/services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee @@ -46,6 +46,11 @@ module.exports = updateEditingSession: (userId, projectId, countryCode, segmentation = {}, callback = (error) ->) -> if userId+"" == settings.smokeTest?.userId+"" return callback() + query = + userId: userId + projectId: projectId + if countryCode + query.countryCode = countryCode opts = body: segmentation: segmentation @@ -53,10 +58,7 @@ module.exports = method: "PUT" timeout: 1000 url: "/editingSession" - qs: - userId: userId - projectId: projectId - countryCode: countryCode || undefined + qs: query maxAttempts: 20 retryDelay: 5000 if settings.overleaf? diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 98495cfcec..08a19aebad 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -12,7 +12,6 @@ UserHandler = require("../User/UserHandler") UserSessionsManager = require("../User/UserSessionsManager") Analytics = require "../Analytics/AnalyticsManager" passport = require 'passport' -GeoIpLookup = require '../../infrastructure/GeoIpLookup' module.exports = AuthenticationController = @@ -48,17 +47,12 @@ module.exports = AuthenticationController = req.session[key] = value # copy to the old `session.user` location, for backward-comptability req.session.user = req.session.passport.user - # try to geolocate the user to a country-code - GeoIpLookup.getDetails req.ip, (err, geoDetails) -> + req.session.save (err) -> if err? - logger.err {err, ip: req.ip}, "error geolocating session, continuing" - req.session.countryCode = geoDetails?.country_code || null - req.session.save (err) -> - if err? - logger.err {user_id: user._id}, "error saving regenerated session after login" - return callback(err) - UserSessionsManager.trackSession(user, req.sessionID, () ->) - callback(err) + logger.err {user_id: user._id}, "error saving regenerated session after login" + return callback(err) + UserSessionsManager.trackSession(user, req.sessionID, () ->) + callback(err) passportLogin: (req, res, next) -> # This function is middleware which wraps the passport.authenticate middleware, diff --git a/services/web/test/unit/coffee/Analytics/AnalyticsControllerTests.coffee b/services/web/test/unit/coffee/Analytics/AnalyticsControllerTests.coffee index edfc14c8ed..002940d263 100644 --- a/services/web/test/unit/coffee/Analytics/AnalyticsControllerTests.coffee +++ b/services/web/test/unit/coffee/Analytics/AnalyticsControllerTests.coffee @@ -22,6 +22,8 @@ describe 'AnalyticsController', -> "../Authentication/AuthenticationController":@AuthenticationController "logger-sharelatex": log:-> + '../../infrastructure/GeoIpLookup': @GeoIpLookup = + getDetails: sinon.stub() @res = send:-> @@ -31,7 +33,8 @@ describe 'AnalyticsController', -> @req = params: projectId: "a project id" - session: {countryCode: 'US'} + @GeoIpLookup.getDetails = sinon.stub() + .callsArgWith(1, null, {country_code: 'XY'}) it "delegates to the AnalyticsManager", (done) -> @AuthenticationController.getLoggedInUserId.returns("1234") @@ -40,7 +43,7 @@ describe 'AnalyticsController', -> @AnalyticsManager.updateEditingSession.calledWith( "1234", "a project id", - 'US', + 'XY', {} ).should.equal true done() diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index 707b362eb9..467e2165f6 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee @@ -28,8 +28,6 @@ describe "AuthenticationController", -> trackSession: sinon.stub() untrackSession: sinon.stub() revokeAllUserSessions: sinon.stub().callsArgWith(1, null) - "../../infrastructure/GeoIpLookup": @GeoIpLookup = - getDetails: sinon.stub().callsArgWith(1, null, {}) @user = _id: ObjectId() email: @email = "USER@example.com" @@ -174,8 +172,6 @@ describe "AuthenticationController", -> @req.session.save = sinon.stub().callsArgWith(0, null) @req.sessionStore = {generate: sinon.stub()} @UserSessionsManager.trackSession = sinon.stub() - @GeoIpLookup.getDetails = sinon.stub() - .callsArgWith(1, null, @geoDetails = {country_code: 'US'}) @call = (callback) => @AuthenticationController.afterLoginSessionSetup @req, @user, callback @@ -194,13 +190,6 @@ describe "AuthenticationController", -> @UserSessionsManager.trackSession.callCount.should.equal 1 done() - it 'should call GeoIpLookup.getDetails', (done) -> - @call (err) => - @GeoIpLookup.getDetails.callCount.should.equal 1 - @req.session.countryCode.should.exist - @req.session.countryCode.should.equal @geoDetails.country_code - done() - it 'should call req.session.save', (done) -> @call (err) => @req.session.save.callCount.should.equal 1 @@ -222,39 +211,6 @@ describe "AuthenticationController", -> @UserSessionsManager.trackSession.callCount.should.equal 0 done() - describe 'when GeoIpLookup produces an error', -> - beforeEach -> - @GeoIpLookup.getDetails = sinon.stub() - .callsArgWith(1, new Error('woops')) - - it 'should not produce an error', (done) -> - @call (err) => - expect(err).to.not.exist - done() - - it 'should still call req.session.save', (done) -> - @call (err) => - @req.session.save.callCount.should.equal 1 - expect(@req.session.countryCode).to.equal null - done() - - describe 'when GeoIpLookup produces no data', -> - beforeEach -> - @GeoIpLookup.getDetails = sinon.stub() - .callsArgWith(1, null, undefined) - - it 'should not produce an error', (done) -> - @call (err) => - expect(err).to.not.exist - done() - - it 'should still call req.session.save', (done) -> - @call (err) => - @req.session.save.callCount.should.equal 1 - expect(@req.session.countryCode).to.equal null - done() - - describe 'getSessionUser', -> it 'should get the user object from session', ->