From e5bd7ce55153d1c56d6c186b4e5341a8c648a518 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 13 Jun 2018 17:22:34 +0100 Subject: [PATCH 01/21] Add message explaining Collaborator subscription to v1 users --- services/web/app/views/subscriptions/new.pug | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/services/web/app/views/subscriptions/new.pug b/services/web/app/views/subscriptions/new.pug index 4eae442f1e..c8228f4802 100644 --- a/services/web/app/views/subscriptions/new.pug +++ b/services/web/app/views/subscriptions/new.pug @@ -10,6 +10,7 @@ block scripts block content .content.content-alt + .container(ng-controller="NewSubscriptionController" ng-cloak) .row.card-group .col-md-5.col-md-push-4 @@ -35,6 +36,15 @@ block content .row(ng-if="plansVariant == 'more-details' && planCode == 'student-annual' || plansVariant == 'more-details' && planCode == 'student-monthly'") .col-xs-12 p.student-disclaimer #{translate('student_disclaimer')} + + if plan_code == 'collaborator_free_trial_7_days' && !!settings.overleaf + .row + .col-xs-12 + p.small.row-spaced-small + | The Collaborator plan is a new Overleaf v2 plan which includes track-changes, references search, and Dropbox & GitHub integration. While Overleaf v2 is in beta, you can also still subscribe to a + | + a(href=settings.overleaf.host + "/plans") legacy plan in Overleaf v1 + | . hr.thin .row .col-md-12.text-center From eda031023cc6893d40b1c6796ab0bc4bdd2e18e7 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 14 Jun 2018 10:15:20 +0100 Subject: [PATCH 02/21] Update subscription thank you page to remove ShareLaTeX references --- .../views/subscriptions/successful_subscription.pug | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/services/web/app/views/subscriptions/successful_subscription.pug b/services/web/app/views/subscriptions/successful_subscription.pug index 184c89c0d2..6fb4fc7958 100644 --- a/services/web/app/views/subscriptions/successful_subscription.pug +++ b/services/web/app/views/subscriptions/successful_subscription.pug @@ -18,17 +18,11 @@ block content p.letter-from-founders p #{translate("thanks_for_subscribing_you_help_sl", {planName:subscription.name})} p #{translate("need_anything_contact_us_at")} - a(href='mailto:support@sharelatex.com') support@sharelatex.com - | . #{translate("goes_straight_to_our_inboxes")}. + a(href='mailto:support@sharelatex.com') #{settings.adminEmail} + | . p #{translate("regards")}, br - | Henry and James - .portraits - span.img-circle - img(src=buildImgPath("about/henry_oswald.jpg")) - |   - span.img-circle - img(src=buildImgPath("about/james_allen.jpg")) + | The #{settings.appName} Team p a.btn.btn-primary(href="/project") < #{translate("back_to_your_projects")} From 32e14527c6fed9b6be642af83f8769f18a481583 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 14 Jun 2018 10:31:06 +0100 Subject: [PATCH 03/21] Make links readable in v2 alerts --- services/web/public/stylesheets/components/alerts.less | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/web/public/stylesheets/components/alerts.less b/services/web/public/stylesheets/components/alerts.less index 6ed93d29d4..ecdcba07a8 100755 --- a/services/web/public/stylesheets/components/alerts.less +++ b/services/web/public/stylesheets/components/alerts.less @@ -65,3 +65,10 @@ .alert-danger { .alert-variant(@alert-danger-bg; @alert-danger-border; @alert-danger-text); } + +.alert when (@is-overleaf = true) { + a { + color: white; + text-decoration: underline; + } +} \ No newline at end of file From 3de1721aa25fa6b23676f9d0496c8146329cedc1 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Fri, 15 Jun 2018 16:11:45 -0500 Subject: [PATCH 04/21] Send event to Google Analytics when variant selected --- services/web/public/coffee/main/plans.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/public/coffee/main/plans.coffee b/services/web/public/coffee/main/plans.coffee index 2915425dc0..1637d79374 100644 --- a/services/web/public/coffee/main/plans.coffee +++ b/services/web/public/coffee/main/plans.coffee @@ -153,6 +153,7 @@ define [ if $scope.shouldABTestPlans sixpack.participate 'plans-details', ['default', 'more-details'], (chosenVariation, rawResponse)-> $scope.plansVariant = chosenVariation + event_tracking.send 'subscription-funnel', 'plans-page-loaded', chosenVariation $scope.showPlans = true From fe225fbbd3eab7506bcf276fa978d28a8860ac2b Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Fri, 15 Jun 2018 16:29:42 -0500 Subject: [PATCH 05/21] Include event label when features table viewed --- .../web/app/views/subscriptions/_plans_page_details_more.pug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/views/subscriptions/_plans_page_details_more.pug b/services/web/app/views/subscriptions/_plans_page_details_more.pug index 17c683201e..d4867c2811 100644 --- a/services/web/app/views/subscriptions/_plans_page_details_more.pug +++ b/services/web/app/views/subscriptions/_plans_page_details_more.pug @@ -87,7 +87,7 @@ div +plan_switch('table') .col-md-3.text-right +currency_dropdown - .row(event-tracking="features-table-viewed" event-tracking-ga="subscription-funnel" event-tracking-trigger="scroll" event-tracking-send-once="true") + .row(event-tracking="features-table-viewed" event-tracking-ga="subscription-funnel" event-tracking-trigger="scroll" event-tracking-send-once="true" event-tracking-label=`exp-{{plansVariant}}`) .col-sm-12(ng-if="ui.view != 'student'") +table_premium .col-sm-12(ng-if="ui.view == 'student'") From f9c074a31efd04196ccc7b57a9c0604368d7244a Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Fri, 15 Jun 2018 17:29:08 +0100 Subject: [PATCH 06/21] Simplify check for when we are editing lines with metadata commands --- .../aceEditor/metadata/MetadataManager.coffee | 38 +++---------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee index c9308626bc..1ff12efc38 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee @@ -18,41 +18,15 @@ define [ return if change.action not in ['remove', 'insert'] return - cursorPosition = @editor.getCursorPosition() + end = change.end - range = new Range(end.row, 0, end.row, end.column) - lineUpToCursor = @editor.getSession().getTextRange range - if lineUpToCursor.trim() == '%' or lineUpToCursor.slice(0, 1) == '\\' - range = new Range(end.row, 0, end.row, end.column + 80) - lineUpToCursor = @editor.getSession().getTextRange range - commandFragment = getLastCommandFragment lineUpToCursor + lineText = @editor.getSession().getLine end.row - linesContainPackage = _.any( - change.lines, - (line) -> line.match(/^\\usepackage(?:\[.{0,80}?])?{(.{0,80}?)}/) - ) - linesContainReqPackage = _.any( - change.lines, - (line) -> line.match(/^\\RequirePackage(?:\[.{0,80}?])?{(.{0,80}?)}/) - ) - linesContainLabel = _.any( - change.lines, - (line) -> line.match(/\\label{(.{0,80}?)}/) - ) - linesContainMeta = - linesContainPackage or - linesContainLabel or - linesContainReqPackage + # Defensive check against extremely long lines + return if lineText.length > 10000 - lastCommandFragmentIsLabel = commandFragment?.slice(0, 7) == '\\label{' - lastCommandFragmentIsPackage = commandFragment?.slice(0, 11) == '\\usepackage' - lastCommandFragmentIsReqPack = commandFragment?.slice(0, 15) == '\\RequirePackage' - lastCommandFragmentIsMeta = - lastCommandFragmentIsPackage or - lastCommandFragmentIsLabel or - lastCommandFragmentIsReqPack - - if linesContainMeta or lastCommandFragmentIsMeta + # Check if edited line contains metadata commands + if /\\(usepackage|RequirePackage|label)(\[.*])?({.*})?/.test(lineText) @scheduleLoadCurrentDocMetaFromServer() @editor.on "changeSession", (e) => From 81c102b5010944ca11f4ff2ae00f11afe1d71d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez=20Capel?= Date: Mon, 18 Jun 2018 14:18:23 +0100 Subject: [PATCH 07/21] Add method to import invites --- .../Subscription/TeamInvitesHandler.coffee | 15 ++++++++++++++- .../Subscription/TeamInvitesHandlerTests.coffee | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee b/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee index e09e99c94a..c3b3b11089 100644 --- a/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee +++ b/services/web/app/coffee/Features/Subscription/TeamInvitesHandler.coffee @@ -56,6 +56,20 @@ module.exports = TeamInvitesHandler = return callback(error) if error? createInvite(subscription, email, inviterName, callback) + importInvite: (subscription, inviterName, email, token, sentAt, callback) -> + checkIfInviteIsPossible subscription, email, (error, possible, reason) -> + return callback(error) if error? + return callback(reason) unless possible + + subscription.teamInvites.push({ + email: email + inviterName: inviterName + token: token + sentAt: sentAt + }) + + subscription.save callback + acceptInvite: (token, userId, callback) -> logger.log {userId}, "Accepting invite" TeamInvitesHandler.getInvite token, (err, invite, subscription) -> @@ -93,7 +107,6 @@ createInvite = (subscription, email, inviterName, callback) -> return callback(error) if error? return callback(reason) unless possible - invite = subscription.teamInvites.find (invite) -> invite.email == email if !invite? diff --git a/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee b/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee index 527ba8a143..d23d1fc3a6 100644 --- a/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee +++ b/services/web/test/unit/coffee/Subscription/TeamInvitesHandlerTests.coffee @@ -175,6 +175,22 @@ describe "TeamInvitesHandler", -> ).should.equal true done() + describe "importInvite", -> + beforeEach -> + @sentAt = new Date() + + it "can imports an invite from v1", -> + @TeamInvitesHandler.importInvite @subscription, "A-Team", "hannibal@a-team.org", + "secret", @sentAt, (error) => + expect(error).not.to.exist + + @subscription.save.calledOnce.should.eq true + + invite = @subscription.teamInvites.find (i) -> i.email == "hannibal@a-team.org" + expect(invite.token).to.eq("secret") + expect(invite.sentAt).to.eq(@sentAt) + + describe "acceptInvite", -> beforeEach -> @user = { From 3d272ca297075cf2473697bf862a25e9573bd163 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Mon, 18 Jun 2018 18:37:58 +0200 Subject: [PATCH 08/21] replace OldAssetProxy --- .../infrastructure/OldAssetProxy.coffee | 16 ----- .../coffee/infrastructure/ProxyManager.coffee | 43 +++++++++++++ .../app/coffee/infrastructure/Server.coffee | 4 +- .../infrastructure/ProxyManagerTests.coffee | 64 +++++++++++++++++++ 4 files changed, 109 insertions(+), 18 deletions(-) delete mode 100644 services/web/app/coffee/infrastructure/OldAssetProxy.coffee create mode 100644 services/web/app/coffee/infrastructure/ProxyManager.coffee create mode 100644 services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee diff --git a/services/web/app/coffee/infrastructure/OldAssetProxy.coffee b/services/web/app/coffee/infrastructure/OldAssetProxy.coffee deleted file mode 100644 index 7912f0ed14..0000000000 --- a/services/web/app/coffee/infrastructure/OldAssetProxy.coffee +++ /dev/null @@ -1,16 +0,0 @@ -settings = require("settings-sharelatex") -logger = require("logger-sharelatex") -request = require("request") - -module.exports = (req, res, next)-> - requestedUrl = req.url - - redirectUrl = settings.proxyUrls[requestedUrl] - if redirectUrl? - logger.log redirectUrl:redirectUrl, reqUrl:req.url, "proxying url" - upstream = request(redirectUrl) - upstream.on "error", (error) -> - logger.error err: error, "error in OldAssetProxy" - upstream.pipe(res) - else - next() diff --git a/services/web/app/coffee/infrastructure/ProxyManager.coffee b/services/web/app/coffee/infrastructure/ProxyManager.coffee new file mode 100644 index 0000000000..45ca6b7a48 --- /dev/null +++ b/services/web/app/coffee/infrastructure/ProxyManager.coffee @@ -0,0 +1,43 @@ +settings = require("settings-sharelatex") +logger = require("logger-sharelatex") +httpProxy = require 'express-http-proxy' + +module.exports = + # add proxy for all paths listed in `settings.proxyUrls`and log errors + apply: (app) -> + for requestUrl, target of settings.proxyUrls + targetUrl = @makeTargetUrl(target) + if targetUrl? + app.use requestUrl, httpProxy(targetUrl) + else + logger.error "Cannot make proxy target from #{target}" + + # takes a 'target' and return an URL to proxy to. + # 'target' can be: + # - a String, representing the URL + # - an Object with: + # - a path attribute (String) + # - a baseURL attribute (String) + # - a baseURL attribute (Object) with: + # - a setting attribute pointing to a value in the settings + makeTargetUrl: (target) -> + return target if typeof target is 'string' + return target.path unless target.baseUrl? + + if typeof target.baseUrl is 'string' + baseUrl = target.baseUrl + else if target.baseUrl.setting? + baseUrl = digSettingValue target.baseUrl.setting + + return null unless baseUrl? + "#{baseUrl}#{target.path}" + +# given a setting path (e.g. 'apis.v1.url') recursively find the corresponding +# settings value +digSettingValue = (attributesPath, dig = null) -> + dig ||= settings + [nextAttribute, leftAttributes...] = attributesPath.split('.') + dig = dig[nextAttribute] + return null unless dig? + return dig if leftAttributes.length == 0 + digSettingValue(leftAttributes.join('.'), dig) diff --git a/services/web/app/coffee/infrastructure/Server.coffee b/services/web/app/coffee/infrastructure/Server.coffee index 39fc3c131b..522599c659 100644 --- a/services/web/app/coffee/infrastructure/Server.coffee +++ b/services/web/app/coffee/infrastructure/Server.coffee @@ -32,7 +32,7 @@ Mongoose = require("./Mongoose") oneDayInMilliseconds = 86400000 ReferalConnect = require('../Features/Referal/ReferalConnect') RedirectManager = require("./RedirectManager") -OldAssetProxy = require("./OldAssetProxy") +ProxyManager = require("./ProxyManager") translations = require("translations-sharelatex").setup(Settings.i18n) Modules = require "./Modules" @@ -74,7 +74,7 @@ app.use methodOverride() app.use metrics.http.monitor(logger) app.use RedirectManager -app.use OldAssetProxy +ProxyManager.apply(app) webRouter.use cookieParser(Settings.security.sessionSecret) diff --git a/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee b/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee new file mode 100644 index 0000000000..3c3c4c7ccd --- /dev/null +++ b/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee @@ -0,0 +1,64 @@ +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = '../../../../app/js/infrastructure/ProxyManager' +SandboxedModule = require('sandboxed-module') +MockRequest = require "../helpers/MockRequest" +MockResponse = require "../helpers/MockResponse" + +describe "ProxyManager", -> + before -> + @settings = proxyUrls: {} + @httpProxy = sinon.stub() + @proxyManager = SandboxedModule.require modulePath, requires: + 'settings-sharelatex': @settings + 'logger-sharelatex': + error: -> + log: -> console.log(arguments) + 'httpProxy': @httpProxy + + describe 'apply', -> + before -> + @sandbox = sinon.sandbox.create() + @app = use: sinon.stub() + @sandbox.stub(@proxyManager, 'makeTargetUrl') + + after -> + @sandbox.restore() + + beforeEach -> + @app.use.reset() + @requestUrl = '/foo/bar' + @settings.proxyUrls[@requestUrl] = 'http://whatever' + + it 'sets routes', -> + @proxyManager.makeTargetUrl.returns('http://whatever') + @proxyManager.apply(@app) + @app.use.calledWith(@requestUrl).should.equal true + + it 'logs errors', -> + @proxyManager.makeTargetUrl.returns(null) + @proxyManager.apply(@app) + @app.use.called.should.equal false + + describe 'makeTargetUrl', -> + it 'returns Strings', -> + target = 'http://whatever' + @proxyManager.makeTargetUrl(target).should.equal target + + it 'makes from object', -> + target = path: 'baz' + @proxyManager.makeTargetUrl(target).should.equal 'baz' + + it 'makes with baseUrl', -> + target = path: 'baz', baseUrl: 'foo.bar/' + @proxyManager.makeTargetUrl(target).should.equal 'foo.bar/baz' + + it 'makes with settingsUrl', -> + @settings.apis = v1: url: 'foo.bar/' + target = path: 'baz', baseUrl: { setting: 'apis.v1.url' } + @proxyManager.makeTargetUrl(target).should.equal 'foo.bar/baz' + + target = path: 'baz', baseUrl: { setting: 'incorrect.setting' } + expect(@proxyManager.makeTargetUrl(target)).to.equal null From 0c86a7dc9b89773858b823c3196ae7beb9809fa2 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 08:43:27 +0100 Subject: [PATCH 09/21] Revert "Simplify package metadata check" --- .../aceEditor/metadata/MetadataManager.coffee | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee index 1ff12efc38..c9308626bc 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/metadata/MetadataManager.coffee @@ -18,15 +18,41 @@ define [ return if change.action not in ['remove', 'insert'] return - + cursorPosition = @editor.getCursorPosition() end = change.end - lineText = @editor.getSession().getLine end.row + range = new Range(end.row, 0, end.row, end.column) + lineUpToCursor = @editor.getSession().getTextRange range + if lineUpToCursor.trim() == '%' or lineUpToCursor.slice(0, 1) == '\\' + range = new Range(end.row, 0, end.row, end.column + 80) + lineUpToCursor = @editor.getSession().getTextRange range + commandFragment = getLastCommandFragment lineUpToCursor - # Defensive check against extremely long lines - return if lineText.length > 10000 + linesContainPackage = _.any( + change.lines, + (line) -> line.match(/^\\usepackage(?:\[.{0,80}?])?{(.{0,80}?)}/) + ) + linesContainReqPackage = _.any( + change.lines, + (line) -> line.match(/^\\RequirePackage(?:\[.{0,80}?])?{(.{0,80}?)}/) + ) + linesContainLabel = _.any( + change.lines, + (line) -> line.match(/\\label{(.{0,80}?)}/) + ) + linesContainMeta = + linesContainPackage or + linesContainLabel or + linesContainReqPackage - # Check if edited line contains metadata commands - if /\\(usepackage|RequirePackage|label)(\[.*])?({.*})?/.test(lineText) + lastCommandFragmentIsLabel = commandFragment?.slice(0, 7) == '\\label{' + lastCommandFragmentIsPackage = commandFragment?.slice(0, 11) == '\\usepackage' + lastCommandFragmentIsReqPack = commandFragment?.slice(0, 15) == '\\RequirePackage' + lastCommandFragmentIsMeta = + lastCommandFragmentIsPackage or + lastCommandFragmentIsLabel or + lastCommandFragmentIsReqPack + + if linesContainMeta or lastCommandFragmentIsMeta @scheduleLoadCurrentDocMetaFromServer() @editor.on "changeSession", (e) => From 024663144317e7351a0f7f793cbe5b2045e4aa5a Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 13 Jun 2018 15:30:46 +0100 Subject: [PATCH 10/21] Add public API endpoints to reach CLSIs - `/api/clsi/compile/:submission_id` - `/api/clsi/compile/:submission_id/build/:build_id/output/:file` Also per review: - DRY up ClsiManager.sendRequestOnce and ClsiManager.sendExternalRequest - Include submission_id in a log message - Don't include timeout in limits when getting file --- .../Features/Compile/ClsiManager.coffee | 51 +++++++---- .../Features/Compile/CompileController.coffee | 33 +++++++ services/web/app/coffee/router.coffee | 16 ++++ .../acceptance/coffee/ApiClsiTests.coffee | 88 ++++++++++++++++++ .../coffee/helpers/MockClsiApi.coffee | 39 ++++++++ .../coffee/Compile/ClsiManagerTests.coffee | 87 ++++++++++++++++-- .../Compile/CompileControllerTests.coffee | 91 +++++++++++++++++++ 7 files changed, 378 insertions(+), 27 deletions(-) create mode 100644 services/web/test/acceptance/coffee/ApiClsiTests.coffee create mode 100644 services/web/test/acceptance/coffee/helpers/MockClsiApi.coffee diff --git a/services/web/app/coffee/Features/Compile/ClsiManager.coffee b/services/web/app/coffee/Features/Compile/ClsiManager.coffee index c27bc2fe90..e5158cdb9b 100755 --- a/services/web/app/coffee/Features/Compile/ClsiManager.coffee +++ b/services/web/app/coffee/Features/Compile/ClsiManager.coffee @@ -14,6 +14,7 @@ async = require("async") ClsiFormatChecker = require("./ClsiFormatChecker") DocumentUpdaterHandler = require "../DocumentUpdater/DocumentUpdaterHandler" Metrics = require('metrics-sharelatex') +Errors = require ('../Errors/Errors') module.exports = ClsiManager = @@ -35,24 +36,16 @@ module.exports = ClsiManager = else return callback(error) logger.log project_id: project_id, "sending compile to CLSI" - ClsiFormatChecker.checkRecoursesForProblems req.compile?.resources, (err, validationProblems)-> - if err? - logger.err err, project_id, "could not check resources for potential problems before sending to clsi" - return callback(err) - if validationProblems? - logger.log project_id:project_id, validationProblems:validationProblems, "problems with users latex before compile was attempted" - return callback(null, "validation-problems", null, null, validationProblems) - ClsiManager._postToClsi project_id, user_id, req, options.compileGroup, (error, response) -> - if error? - logger.err err:error, project_id:project_id, "error sending request to clsi" - return callback(error) - logger.log project_id: project_id, outputFilesLength: response?.outputFiles?.length, status: response?.status, compile_status: response?.compile?.status, "received compile response from CLSI" - ClsiCookieManager._getServerId project_id, (err, clsiServerId)-> - if err? - logger.err err:err, project_id:project_id, "error getting server id" - return callback(err) - outputFiles = ClsiManager._parseOutputFiles(project_id, response?.compile?.outputFiles) - callback(null, response?.compile?.status, outputFiles, clsiServerId) + ClsiManager._sendBuiltRequest project_id, user_id, req, options, (error, status, result...) -> + return callback(error) if error? + callback(error, status, result...) + + # for public API requests where there is no project id + sendExternalRequest: (submission_id, clsi_request, options = {}, callback = (error, status, outputFiles, clsiServerId, validationProblems) ->) -> + logger.log submission_id: submission_id, "sending external compile to CLSI", clsi_request + ClsiManager._sendBuiltRequest submission_id, null, clsi_request, options, (error, status, result...) -> + return callback(error) if error? + callback(error, status, result...) stopCompile: (project_id, user_id, options, callback = (error) ->) -> compilerUrl = @_getCompilerUrl(options?.compileGroup, project_id, user_id, "compile/stop") @@ -74,6 +67,26 @@ module.exports = ClsiManager = return callback(error) if error? callback() + _sendBuiltRequest: (project_id, user_id, req, options = {}, callback = (error, status, outputFiles, clsiServerId, validationProblems) ->) -> + ClsiFormatChecker.checkRecoursesForProblems req.compile?.resources, (err, validationProblems)-> + if err? + logger.err err, project_id, "could not check resources for potential problems before sending to clsi" + return callback(err) + if validationProblems? + logger.log project_id:project_id, validationProblems:validationProblems, "problems with users latex before compile was attempted" + return callback(null, "validation-problems", null, null, validationProblems) + ClsiManager._postToClsi project_id, user_id, req, options.compileGroup, (error, response) -> + if error? + logger.err err:error, project_id:project_id, "error sending request to clsi" + return callback(error) + logger.log project_id: project_id, outputFilesLength: response?.outputFiles?.length, status: response?.status, compile_status: response?.compile?.status, "received compile response from CLSI" + ClsiCookieManager._getServerId project_id, (err, clsiServerId)-> + if err? + logger.err err:err, project_id:project_id, "error getting server id" + return callback(err) + outputFiles = ClsiManager._parseOutputFiles(project_id, response?.compile?.outputFiles) + callback(null, response?.compile?.status, outputFiles, clsiServerId) + _makeRequest: (project_id, opts, callback)-> ClsiCookieManager.getCookieJar project_id, (err, jar)-> if err? @@ -87,7 +100,7 @@ module.exports = ClsiManager = ClsiCookieManager.setServerId project_id, response, (err)-> if err? logger.warn err:err, project_id:project_id, "error setting server id" - + return callback err, response, body _getCompilerUrl: (compileGroup, project_id, user_id, action) -> diff --git a/services/web/app/coffee/Features/Compile/CompileController.coffee b/services/web/app/coffee/Features/Compile/CompileController.coffee index decfac515b..ee3e20fdc9 100755 --- a/services/web/app/coffee/Features/Compile/CompileController.coffee +++ b/services/web/app/coffee/Features/Compile/CompileController.coffee @@ -55,6 +55,33 @@ module.exports = CompileController = return next(error) if error? res.status(200).send() + # Used for submissions through the public API + compileSubmission: (req, res, next = (error) ->) -> + res.setTimeout(5 * 60 * 1000) + submission_id = req.params.submission_id + options = {} + if req.body?.rootResourcePath? + options.rootResourcePath = req.body.rootResourcePath + if req.body?.compiler + options.compiler = req.body.compiler + if req.body?.draft + options.draft = req.body.draft + if req.body?.check in ['validate', 'error', 'silent'] + options.check = req.body.check + options.compileGroup = req.body?.compileGroup || Settings.defaultFeatures.compileGroup + options.timeout = req.body?.timeout || Settings.defaultFeatures.compileTimeout + logger.log {options:options, submission_id:submission_id}, "got compileSubmission request" + ClsiManager.sendExternalRequest submission_id, req.body, options, (error, status, outputFiles, clsiServerId, validationProblems) -> + return next(error) if error? + logger.log {submission_id:submission_id, files:outputFiles}, "compileSubmission output files" + res.contentType("application/json") + res.status(200).send JSON.stringify { + status: status + outputFiles: outputFiles + clsiServerId: clsiServerId + validationProblems: validationProblems + } + _compileAsUser: (req, callback) -> # callback with user_id if per-user, undefined otherwise if not Settings.disablePerUserCompiles @@ -139,6 +166,12 @@ module.exports = CompileController = url = CompileController._getFileUrl project_id, user_id, req.params.build_id, req.params.file CompileController.proxyToClsi(project_id, url, req, res, next) + getFileFromClsiWithoutUser: (req, res, next = (error) ->) -> + submission_id = req.params.submission_id + url = CompileController._getFileUrl submission_id, null, req.params.build_id, req.params.file + limits = { compileGroup: req.body?.compileGroup || Settings.defaultFeatures.compileGroup } + CompileController.proxyToClsiWithLimits(submission_id, url, limits, req, res, next) + # compute a GET file url for a given project, user (optional), build (optional) and file _getFileUrl: (project_id, user_id, build_id, file) -> if user_id? and build_id? diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 339b4abe1c..0abc170589 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -295,6 +295,22 @@ module.exports = class Router webRouter.post "/confirm-password", AuthenticationController.requireLogin(), SudoModeController.submitPassword + # New "api" endpoints. Started as a way for v1 to call over to v2 (for + # long-term features, as opposed to the nominally temporary ones in the + # overleaf-integration module), but may expand beyond that role. + publicApiRouter.post '/api/clsi/compile/:submission_id', AuthenticationController.httpAuth, CompileController.compileSubmission + publicApiRouter.get /^\/api\/clsi\/compile\/([^\/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/, + ((req, res, next) -> + params = + "submission_id": req.params[0] + "build_id": req.params[1] + "file": req.params[2] + req.params = params + next() + ), + AuthenticationController.httpAuth, + CompileController.getFileFromClsiWithoutUser + #Admin Stuff webRouter.get '/admin', AuthorizationMiddlewear.ensureUserIsSiteAdmin, AdminController.index webRouter.get '/admin/user', AuthorizationMiddlewear.ensureUserIsSiteAdmin, (req, res)-> res.redirect("/admin/register") #this gets removed by admin-panel addon diff --git a/services/web/test/acceptance/coffee/ApiClsiTests.coffee b/services/web/test/acceptance/coffee/ApiClsiTests.coffee new file mode 100644 index 0000000000..d19e11f933 --- /dev/null +++ b/services/web/test/acceptance/coffee/ApiClsiTests.coffee @@ -0,0 +1,88 @@ +expect = require("chai").expect +request = require './helpers/request' +Settings = require "settings-sharelatex" + +auth = new Buffer('sharelatex:password').toString("base64") +authed_request = request.defaults + headers: + Authorization: "Basic #{auth}" + + +describe 'ApiClsiTests', -> + describe 'compile', -> + before (done) -> + @compileSpec = + compile: + options: + compiler: 'pdflatex' + timeout: 60 + rootResourcePath: 'main.tex' + resources: [ + path: 'main/tex' + content: "\\documentclass{article}\n\\begin{document}\nHello World\n\\end{document}" + , + path: 'image.png' + url: 'www.example.com/image.png' + modified: 123456789 + ] + done() + + describe 'valid request', -> + it 'returns success and a list of output files', (done) -> + authed_request.post { + uri: '/api/clsi/compile/abcd' + json: @compileSpec + }, (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 200 + expect(response.body).to.deep.equal { + status: 'success' + outputFiles: [ + path: 'project.pdf' + url: '/project/abcd/build/1234/output/project.pdf' + type: 'pdf' + build: 1234 + , + path: 'project.log' + url: '/project/abcd/build/1234/output/project.log' + type: 'log' + build: 1234 + ] + } + done() + + describe 'unauthorized', -> + it 'returns 401', (done) -> + request.post { + uri: '/api/clsi/compile/abcd' + json: @compileSpec + }, (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 401 + expect(response.body).to.equal 'Unauthorized' + done() + + describe 'get output', -> + describe 'valid file', -> + it 'returns the file', (done) -> + authed_request.get '/api/clsi/compile/abcd/build/1234/output/project.pdf', (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 200 + expect(response.body).to.equal 'mock-pdf' + done() + + describe 'invalid file', -> + it 'returns 404', (done) -> + authed_request.get '/api/clsi/compile/abcd/build/1234/output/project.aux', (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 404 + expect(response.body).to.not.equal 'mock-pdf' + done() + + describe 'unauthorized', -> + it 'returns 401', (done) -> + request.get '/api/clsi/compile/abcd/build/1234/output/project.pdf', (error, response, body) -> + throw error if error? + expect(response.statusCode).to.equal 401 + expect(response.body).to.not.equal 'mock-pdf' + done() diff --git a/services/web/test/acceptance/coffee/helpers/MockClsiApi.coffee b/services/web/test/acceptance/coffee/helpers/MockClsiApi.coffee new file mode 100644 index 0000000000..2f9180960d --- /dev/null +++ b/services/web/test/acceptance/coffee/helpers/MockClsiApi.coffee @@ -0,0 +1,39 @@ +express = require("express") +app = express() + +module.exports = MockClsiApi = + run: () -> + app.post "/project/:project_id/compile", (req, res, next) => + res.status(200).send { + compile: + status: 'success' + error: null + outputFiles: [ + url: "/project/#{req.params.project_id}/build/1234/output/project.pdf" + path: 'project.pdf' + type: 'pdf' + build: 1234 + , + url: "/project/#{req.params.project_id}/build/1234/output/project.log" + path: 'project.log' + type: 'log' + build: 1234 + ] + } + + app.get "/project/:project_id/build/:build_id/output/*", (req, res, next) -> + filename = req.params[0] + if filename == 'project.pdf' + res.status(200).send 'mock-pdf' + else if filename == 'project.log' + res.status(200).send 'mock-log' + else + res.sendStatus(404) + + app.listen 3013, (error) -> + throw error if error? + .on "error", (error) -> + console.error "error starting MockClsiApi:", error.message + process.exit(1) + +MockClsiApi.run() diff --git a/services/web/test/unit/coffee/Compile/ClsiManagerTests.coffee b/services/web/test/unit/coffee/Compile/ClsiManagerTests.coffee index bd02af2f28..c8c39d9952 100644 --- a/services/web/test/unit/coffee/Compile/ClsiManagerTests.coffee +++ b/services/web/test/unit/coffee/Compile/ClsiManagerTests.coffee @@ -8,7 +8,7 @@ SandboxedModule = require('sandboxed-module') describe "ClsiManager", -> beforeEach -> @jar = {cookie:"stuff"} - @ClsiCookieManager = + @ClsiCookieManager = getCookieJar: sinon.stub().callsArgWith(1, null, @jar) setServerId: sinon.stub().callsArgWith(2) _getServerId:sinon.stub() @@ -99,8 +99,8 @@ describe "ClsiManager", -> status: @status = "failure" }) @ClsiManager.sendRequest @project_id, @user_id, {}, @callback - - it "should call the callback with a failure statue", -> + + it "should call the callback with a failure status", -> @callback.calledWith(null, @status).should.equal true describe "with a sync conflict", -> @@ -137,11 +137,82 @@ describe "ClsiManager", -> it "should call the callback with an error", -> @callback.calledWithExactly(new Error("failed")).should.equal true + describe "sendExternalRequest", -> + beforeEach -> + @submission_id = "submission-id" + @clsi_request = "mock-request" + @ClsiCookieManager._getServerId.callsArgWith(1, null, "clsi3") + + describe "with a successful compile", -> + beforeEach -> + @ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + compile: + status: @status = "success" + outputFiles: [{ + url: "#{@settings.apis.clsi.url}/project/#{@submission_id}/build/1234/output/output.pdf" + path: "output.pdf" + type: "pdf" + build: 1234 + },{ + url: "#{@settings.apis.clsi.url}/project/#{@submission_id}/build/1234/output/output.log" + path: "output.log" + type: "log" + build: 1234 + }] + }) + @ClsiManager.sendExternalRequest @submission_id, @clsi_request, {compileGroup:"standard"}, @callback + + it "should send the request to the CLSI", -> + @ClsiManager._postToClsi + .calledWith(@submission_id, null, @clsi_request, "standard") + .should.equal true + + it "should call the callback with the status and output files", -> + outputFiles = [{ + url: "/project/#{@submission_id}/build/1234/output/output.pdf" + path: "output.pdf" + type: "pdf" + build: 1234 + },{ + url: "/project/#{@submission_id}/build/1234/output/output.log" + path: "output.log" + type: "log" + build: 1234 + }] + @callback.calledWith(null, @status, outputFiles).should.equal true + + describe "with a failed compile", -> + beforeEach -> + @ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + compile: + status: @status = "failure" + }) + @ClsiManager.sendExternalRequest @submission_id, @clsi_request, {}, @callback + + it "should call the callback with a failure status", -> + @callback.calledWith(null, @status).should.equal true + + describe "when the resources fail the precompile check", -> + beforeEach -> + @ClsiFormatChecker.checkRecoursesForProblems = sinon.stub().callsArgWith(1, new Error("failed")) + @ClsiManager._postToClsi = sinon.stub().callsArgWith(4, null, { + compile: + status: @status = "failure" + }) + @ClsiManager.sendExternalRequest @submission_id, @clsi_request, {}, @callback + + it "should call the callback only once", -> + @callback.calledOnce.should.equal true + + it "should call the callback with an error", -> + @callback.calledWithExactly(new Error("failed")).should.equal true + + describe "deleteAuxFiles", -> beforeEach -> @ClsiManager._makeRequest = sinon.stub().callsArg(2) @DocumentUpdaterHandler.clearProjectState = sinon.stub().callsArg(1) - + describe "with the standard compileGroup", -> beforeEach -> @ClsiManager.deleteAuxFiles @project_id, @user_id, {compileGroup: "standard"}, @callback @@ -158,7 +229,7 @@ describe "ClsiManager", -> it "should call the callback", -> @callback.called.should.equal true - + describe "_buildRequest", -> beforeEach -> @@ -441,7 +512,7 @@ describe "ClsiManager", -> it "should call the callback", -> @callback.called.should.equal true - + describe "with param file", -> beforeEach -> @ClsiManager.wordCount @project_id, @user_id, "main.tex", {}, @callback @@ -450,7 +521,7 @@ describe "ClsiManager", -> @ClsiManager._makeRequest .calledWith(@project_id, { method: "GET", url: "http://clsi.example.com/project/#{@project_id}/user/#{@user_id}/wordcount", qs:{file:"main.tex",image:undefined}}) .should.equal true - + describe "with image", -> beforeEach -> @req.compile.options.imageName = @image = "example.com/mock/image" @@ -468,7 +539,7 @@ describe "ClsiManager", -> beforeEach -> @response = {there:"something"} @request.callsArgWith(1, null, @response) - @opts = + @opts = method: "SOMETHIGN" url: "http://a place on the web" diff --git a/services/web/test/unit/coffee/Compile/CompileControllerTests.coffee b/services/web/test/unit/coffee/Compile/CompileControllerTests.coffee index 8a09ac866e..ced3260412 100644 --- a/services/web/test/unit/coffee/Compile/CompileControllerTests.coffee +++ b/services/web/test/unit/coffee/Compile/CompileControllerTests.coffee @@ -29,6 +29,9 @@ describe "CompileController", -> url: "clsi.example.com" clsi_priority: url: "clsi-priority.example.com" + defaultFeatures: + compileGroup: 'standard' + compileTimeout: 60 @jar = {cookie:"stuff"} @ClsiCookieManager = getCookieJar:sinon.stub().callsArgWith(1, null, @jar) @@ -109,6 +112,62 @@ describe "CompileController", -> .calledWith(@project_id, @user_id, { isAutoCompile: false, draft: true }) .should.equal true + describe "compileSubmission", -> + beforeEach -> + @submission_id = 'sub-1234' + @req.params = + submission_id: @submission_id + @req.body = {} + @ClsiManager.sendExternalRequest = sinon.stub() + .callsArgWith(3, null, @status = "success", @outputFiles = ["mock-output-files"], \ + @clsiServerId = "mock-server-id", @validationProblems = null) + + it "should set the content-type of the response to application/json", -> + @CompileController.compileSubmission @req, @res, @next + @res.contentType + .calledWith("application/json") + .should.equal true + + it "should send a successful response reporting the status and files", -> + @CompileController.compileSubmission @req, @res, @next + @res.statusCode.should.equal 200 + @res.body.should.equal JSON.stringify({ + status: @status + outputFiles: @outputFiles + clsiServerId: 'mock-server-id' + validationProblems: null + }) + + describe "with compileGroup and timeout", -> + beforeEach -> + @req.body = + compileGroup: 'special' + timeout: 600 + @CompileController.compileSubmission @req, @res, @next + + it "should use the supplied values", -> + @ClsiManager.sendExternalRequest + .calledWith(@submission_id, { compileGroup: 'special', timeout: 600 }, \ + { compileGroup: 'special', timeout: 600 }) + .should.equal true + + describe "with other supported options but not compileGroup and timeout", -> + beforeEach -> + @req.body = + rootResourcePath: 'main.tex' + compiler: 'lualatex' + draft: true + check: 'validate' + @CompileController.compileSubmission @req, @res, @next + + it "should use the other options but default values for compileGroup and timeout", -> + @ClsiManager.sendExternalRequest + .calledWith(@submission_id, \ + {rootResourcePath: 'main.tex', compiler: 'lualatex', draft: true, check: 'validate'}, \ + {rootResourcePath: 'main.tex', compiler: 'lualatex', draft: true, check: 'validate', \ + compileGroup: 'standard', timeout: 60}) + .should.equal true + describe "downloadPdf", -> beforeEach -> @req.params = @@ -167,6 +226,38 @@ describe "CompileController", -> done() @CompileController.downloadPdf @req, @res + describe "getFileFromClsiWithoutUser", -> + beforeEach -> + @submission_id = 'sub-1234' + @build_id = 123456 + @file = 'project.pdf' + @req.params = + submission_id: @submission_id + build_id: @build_id + file: @file + @req.body = {} + @expected_url = "/project/#{@submission_id}/build/#{@build_id}/output/#{@file}" + @CompileController.proxyToClsiWithLimits = sinon.stub() + + describe "without limits specified", -> + beforeEach -> + @CompileController.getFileFromClsiWithoutUser @req, @res, @next + + it "should proxy to CLSI with correct URL and default limits", -> + @CompileController.proxyToClsiWithLimits + .calledWith(@submission_id, @expected_url, {compileGroup: 'standard'}) + .should.equal true + + describe "with limits specified", -> + beforeEach -> + @req.body = {compileTimeout: 600, compileGroup: 'special'} + @CompileController.getFileFromClsiWithoutUser @req, @res, @next + + it "should proxy to CLSI with correct URL and specified limits", -> + @CompileController.proxyToClsiWithLimits + .calledWith(@submission_id, @expected_url, {compileGroup: 'special'}) + .should.equal true + describe "proxyToClsi", -> beforeEach -> @request.returns(@proxy = { From 99728d8f18022f9a9d14abea61656093ccd3d50b Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 13:55:01 +0100 Subject: [PATCH 11/21] Bail out with exit code on acceptance test failure --- services/web/Makefile | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/services/web/Makefile b/services/web/Makefile index 474155a478..26534b8612 100644 --- a/services/web/Makefile +++ b/services/web/Makefile @@ -196,8 +196,11 @@ test_frontend: test_clean # stop service test_acceptance: test_acceptance_app test_acceptance_modules -test_acceptance_app: test_acceptance_app_start_service test_acceptance_app_run - $(MAKE) test_acceptance_app_stop_service +test_acceptance_app: + @set -e; \ + $(MAKE) test_acceptance_app_start_service; \ + $(MAKE) test_acceptance_app_run; \ + $(MAKE) test_acceptance_app_stop_service; test_acceptance_app_start_service: test_clean # stop service and clear dbs $(MAKE) compile @@ -208,10 +211,12 @@ test_acceptance_app_stop_service: test_acceptance_app_run: @docker-compose ${DOCKER_COMPOSE_FLAGS} exec -T test_acceptance npm -q run test:acceptance -- ${MOCHA_ARGS}; \ - if [ $$? -eq 137 ]; then \ - echo "\nOh dear, it looks like the web process crashed! To see why, run:\n\n\tdocker-compose logs test_acceptance\n"; \ - exit 1; \ - fi + result=$$?; \ + if [ $$result -eq 137 ]; then \ + docker-compose logs --tail=50 test_acceptance; \ + echo "\nOh dear, it looks like the web process crashed! Some logs are above, but to see them all, run:\n\n\tdocker-compose logs test_acceptance\n"; \ + fi; \ + exit $$result test_acceptance_modules: @set -e; \ From 8492373a14d8a1aa015989697bf4e9df03cb9c94 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 19 Jun 2018 16:24:27 +0100 Subject: [PATCH 12/21] Add V1ConnectionError --- services/web/app/coffee/Features/Errors/Errors.coffee | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/web/app/coffee/Features/Errors/Errors.coffee b/services/web/app/coffee/Features/Errors/Errors.coffee index af1e437d4b..25b3f3e06d 100644 --- a/services/web/app/coffee/Features/Errors/Errors.coffee +++ b/services/web/app/coffee/Features/Errors/Errors.coffee @@ -61,6 +61,13 @@ ProjectHistoryDisabledError = (message) -> return error ProjectHistoryDisabledError.prototype.__proto___ = Error.prototype +V1ConnectionError = (message) -> + error = new Error(message) + error.name = "V1ConnectionError" + error.__proto__ = V1ConnectionError.prototype + return error +V1ConnectionError.prototype.__proto___ = Error.prototype + module.exports = Errors = NotFoundError: NotFoundError ServiceNotConfiguredError: ServiceNotConfiguredError @@ -71,3 +78,4 @@ module.exports = Errors = UnsupportedExportRecordsError: UnsupportedExportRecordsError V1HistoryNotSyncedError: V1HistoryNotSyncedError ProjectHistoryDisabledError: ProjectHistoryDisabledError + V1ConnectionError: V1ConnectionError From 82a8e370710724b5239af4dc62b4ee513d7cd917 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 19 Jun 2018 16:25:31 +0100 Subject: [PATCH 13/21] Handle V1 connection refused when getting v1 subscription --- .../app/coffee/Features/Project/ProjectController.coffee | 6 +++++- .../Features/Subscription/V1SubscriptionManager.coffee | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectController.coffee b/services/web/app/coffee/Features/Project/ProjectController.coffee index 8ea2dc3189..05ebb9f108 100644 --- a/services/web/app/coffee/Features/Project/ProjectController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectController.coffee @@ -27,6 +27,7 @@ CollaboratorsHandler = require '../Collaborators/CollaboratorsHandler' Modules = require '../../infrastructure/Modules' ProjectEntityHandler = require './ProjectEntityHandler' crypto = require 'crypto' +{ V1ConnectionError } = require '../Errors/Errors' module.exports = ProjectController = @@ -183,7 +184,10 @@ module.exports = ProjectController = return cb(null, projects: [], tags: [], noConnection: true) return cb(error, projects[0]) # hooks.fire returns an array of results, only need first hasSubscription: (cb)-> - LimitationsManager.userHasSubscriptionOrIsGroupMember currentUser, cb + LimitationsManager.userHasSubscriptionOrIsGroupMember currentUser, (error, hasSub) -> + if error? and error instanceof V1ConnectionError + return cb(null, true) + return cb(error, hasSub) user: (cb) -> User.findById user_id, "featureSwitches overleaf awareOfV2 features", cb }, (err, results)-> diff --git a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee index dcf23a3453..857881df4d 100644 --- a/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee +++ b/services/web/app/coffee/Features/Subscription/V1SubscriptionManager.coffee @@ -2,6 +2,7 @@ UserGetter = require "../User/UserGetter" request = require "request" settings = require "settings-sharelatex" logger = require "logger-sharelatex" +{ V1ConnectionError } = require "../Errors/Errors" module.exports = V1SubscriptionManager = # Returned planCode = 'v1_pro' | 'v1_pro_plus' | 'v1_student' | 'v1_free' | null @@ -58,7 +59,10 @@ module.exports = V1SubscriptionManager = json: true, timeout: 5 * 1000 }, (error, response, body) -> - return callback(error) if error? + if error? + # Specially handle no connection err, so warning can be shown + error = new V1ConnectionError('No V1 connection') if error.code == 'ECONNREFUSED' + return callback(error) if 200 <= response.statusCode < 300 return callback null, body else From 8de9e9fae43f234d45e51e9f2720e835de028630 Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Tue, 19 Jun 2018 16:25:56 +0100 Subject: [PATCH 14/21] Use error type instead of error message to check v1 connection error --- .../web/app/coffee/Features/Project/ProjectController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Project/ProjectController.coffee b/services/web/app/coffee/Features/Project/ProjectController.coffee index 05ebb9f108..1b7051e39a 100644 --- a/services/web/app/coffee/Features/Project/ProjectController.coffee +++ b/services/web/app/coffee/Features/Project/ProjectController.coffee @@ -180,7 +180,7 @@ module.exports = ProjectController = ProjectGetter.findAllUsersProjects user_id, 'name lastUpdated publicAccesLevel archived owner_ref tokens', cb v1Projects: (cb) -> Modules.hooks.fire "findAllV1Projects", user_id, (error, projects = []) -> - if error? and error.message == 'No V1 connection' + if error? and error instanceof V1ConnectionError return cb(null, projects: [], tags: [], noConnection: true) return cb(error, projects[0]) # hooks.fire returns an array of results, only need first hasSubscription: (cb)-> From bbed5fca9ab6f140b339562df56132426e226ad3 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Wed, 20 Jun 2018 10:58:19 +0200 Subject: [PATCH 15/21] simplify proxy --- .../coffee/infrastructure/ProxyManager.coffee | 20 +------------------ .../infrastructure/ProxyManagerTests.coffee | 14 +++++-------- 2 files changed, 6 insertions(+), 28 deletions(-) diff --git a/services/web/app/coffee/infrastructure/ProxyManager.coffee b/services/web/app/coffee/infrastructure/ProxyManager.coffee index 45ca6b7a48..d93c696ab1 100644 --- a/services/web/app/coffee/infrastructure/ProxyManager.coffee +++ b/services/web/app/coffee/infrastructure/ProxyManager.coffee @@ -18,26 +18,8 @@ module.exports = # - an Object with: # - a path attribute (String) # - a baseURL attribute (String) - # - a baseURL attribute (Object) with: - # - a setting attribute pointing to a value in the settings makeTargetUrl: (target) -> return target if typeof target is 'string' return target.path unless target.baseUrl? + "#{target.baseUrl}#{target.path or ''}" - if typeof target.baseUrl is 'string' - baseUrl = target.baseUrl - else if target.baseUrl.setting? - baseUrl = digSettingValue target.baseUrl.setting - - return null unless baseUrl? - "#{baseUrl}#{target.path}" - -# given a setting path (e.g. 'apis.v1.url') recursively find the corresponding -# settings value -digSettingValue = (attributesPath, dig = null) -> - dig ||= settings - [nextAttribute, leftAttributes...] = attributesPath.split('.') - dig = dig[nextAttribute] - return null unless dig? - return dig if leftAttributes.length == 0 - digSettingValue(leftAttributes.join('.'), dig) diff --git a/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee b/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee index 3c3c4c7ccd..5782a4e2fc 100644 --- a/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee +++ b/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee @@ -47,18 +47,14 @@ describe "ProxyManager", -> target = 'http://whatever' @proxyManager.makeTargetUrl(target).should.equal target - it 'makes from object', -> + it 'makes with path', -> target = path: 'baz' @proxyManager.makeTargetUrl(target).should.equal 'baz' it 'makes with baseUrl', -> + target = baseUrl: 'foo.bar' + @proxyManager.makeTargetUrl(target).should.equal 'foo.bar' + + it 'makes with baseUrl and path', -> target = path: 'baz', baseUrl: 'foo.bar/' @proxyManager.makeTargetUrl(target).should.equal 'foo.bar/baz' - - it 'makes with settingsUrl', -> - @settings.apis = v1: url: 'foo.bar/' - target = path: 'baz', baseUrl: { setting: 'apis.v1.url' } - @proxyManager.makeTargetUrl(target).should.equal 'foo.bar/baz' - - target = path: 'baz', baseUrl: { setting: 'incorrect.setting' } - expect(@proxyManager.makeTargetUrl(target)).to.equal null From 6c1994e25b815a1970e6c64d292eb8289703d14d Mon Sep 17 00:00:00 2001 From: Alasdair Smith Date: Wed, 20 Jun 2018 11:19:23 +0100 Subject: [PATCH 16/21] Stub errors in tests to cache --- .../web/test/unit/coffee/Project/ProjectControllerTests.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee b/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee index 1b66ba9bf9..8de7c6aed1 100644 --- a/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectControllerTests.coffee @@ -5,6 +5,7 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/Project/ProjectController" expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" describe "ProjectController", -> @@ -100,6 +101,7 @@ describe "ProjectController", -> "../Collaborators/CollaboratorsHandler": @CollaboratorsHandler "../../infrastructure/Modules": @Modules "./ProjectEntityHandler": @ProjectEntityHandler + "../Errors/Errors": Errors @projectName = "£12321jkj9ujkljds" @req = From 74ca0c4220ed241f16a711bcba12188185bda8b0 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 13:52:56 +0100 Subject: [PATCH 17/21] Refactor email template system --- .../Email/Bodies/SingleCTAEmailBody.coffee | 16 +- .../Email/Bodies/ol-SingleCTAEmailBody.coffee | 16 +- .../coffee/Features/Email/EmailBuilder.coffee | 236 +++++++----------- 3 files changed, 110 insertions(+), 158 deletions(-) diff --git a/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee b/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee index cf4adf8cd7..bffa485f07 100644 --- a/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee +++ b/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee @@ -4,13 +4,17 @@ settings = require "settings-sharelatex" module.exports = _.template """
-

- <%= title %> -

+ <% if (title) { %> +

+ <%= title %> +

+ <% } %>
 
-

- <%= greeting %> -

+ <% if (greeting) { %> +

+ <%= greeting %> +

+ <% } %>

<%= message %>

diff --git a/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee b/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee index 45bc00383c..ee9907c019 100644 --- a/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee +++ b/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee @@ -4,13 +4,17 @@ settings = require "settings-sharelatex" module.exports = _.template """
-

- <%= title %> -

+ <% if (title) { %> +

+ <%= title %> +

+ <% } %>
 
-

- <%= greeting %> -

+ <% if (greeting) { %> +

+ <%= greeting %> +

+ <% } %>

<%= message %>

diff --git a/services/web/app/coffee/Features/Email/EmailBuilder.coffee b/services/web/app/coffee/Features/Email/EmailBuilder.coffee index 8952e7b864..a33f02e695 100644 --- a/services/web/app/coffee/Features/Email/EmailBuilder.coffee +++ b/services/web/app/coffee/Features/Email/EmailBuilder.coffee @@ -1,5 +1,6 @@ _ = require('underscore') settings = require("settings-sharelatex") +marked = require('marked') PersonalEmailLayout = require("./Layouts/PersonalEmailLayout") NotificationEmailLayout = require("./Layouts/NotificationEmailLayout") @@ -7,169 +8,113 @@ BaseWithHeaderEmailLayout = require("./Layouts/" + settings.brandPrefix + "BaseW SingleCTAEmailBody = require("./Bodies/" + settings.brandPrefix + "SingleCTAEmailBody") +CTAEmailTemplate = (content) -> + content.greeting ?= () -> 'Hi,' + content.secondaryMessage ?= () -> "" + return { + subject: (opts) -> content.subject(opts), + layout: BaseWithHeaderEmailLayout, + plainTextTemplate: (opts) -> """ +#{content.greeting(opts)} + +#{content.message(opts).trim()} + +#{content.ctaText(opts)}: #{content.ctaURL(opts)} + +#{content.secondaryMessage?(opts).trim() or ""} + +Regards, +The #{settings.appName} Team - #{settings.siteUrl} + """ + compiledTemplate: (opts) -> + SingleCTAEmailBody({ + title: content.title?(opts) + greeting: content.greeting(opts) + message: marked(content.message(opts).trim()) + secondaryMessage: marked(content.secondaryMessage(opts).trim()) + ctaText: content.ctaText(opts) + ctaURL: content.ctaURL(opts) + gmailGoToAction: content.gmailGoToAction?(opts) + }) + } + templates = {} -templates.registered = - subject: _.template "Activate your #{settings.appName} Account" - layout: PersonalEmailLayout - type: "notification" - plainTextTemplate: _.template """ -Congratulations, you've just had an account created for you on #{settings.appName} with the email address "<%= to %>". +templates.registered = CTAEmailTemplate({ + subject: () -> "Activate your #{settings.appName} Account" + message: (opts) -> """ +Congratulations, you've just had an account created for you on #{settings.appName} with the email address '#{opts.to}'. -Click here to set your password and log in: <%= setNewPasswordUrl %> - -If you have any questions or problems, please contact #{settings.adminEmail} +Click here to set your password and log in: """ - compiledTemplate: _.template """ -

Congratulations, you've just had an account created for you on #{settings.appName} with the email address "<%= to %>".

+ secondaryMessage: () -> "If you have any questions or problems, please contact #{settings.adminEmail}" + ctaText: () -> "Set password" + ctaURL: (opts) -> opts.setNewPasswordUrl +}) -

Click here to set your password and log in.

- -

If you have any questions or problems, please contact #{settings.adminEmail}.

+templates.canceledSubscription = CTAEmailTemplate({ + subject: () -> "#{settings.appName} thoughts" + message: () -> """ +I'm sorry to see you cancelled your #{settings.appName} premium account. Would you mind giving us some feedback on what the site is lacking at the moment via this quick survey? """ + secondaryMessage: () -> "Thank you in advance!" + ctaText: () -> "Leave Feedback" + ctaURL: (opts) -> "https://sharelatex.typeform.com/to/f5lBiZ" +}) - -templates.canceledSubscription = - subject: _.template "ShareLaTeX thoughts" - layout: PersonalEmailLayout - type:"lifecycle" - plainTextTemplate: _.template """ -Hi <%= first_name %>, - -I'm sorry to see you cancelled your ShareLaTeX premium account. Would you mind giving me some advice on what the site is lacking at the moment via this survey?: - -https://sharelatex.typeform.com/to/f5lBiZ - -Thank you in advance. - -Henry - -ShareLaTeX Co-founder -""" - compiledTemplate: _.template ''' -

Hi <%= first_name %>,

- -

I'm sorry to see you cancelled your ShareLaTeX premium account. Would you mind giving me some advice on what the site is lacking at the moment via this survey?

- -

Thank you in advance.

- -

-Henry
-ShareLaTeX Co-founder -

-''' - - -templates.passwordResetRequested = - subject: _.template "Password Reset - #{settings.appName}" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Password Reset - -We got a request to reset your #{settings.appName} password. - -Click this link to reset your password: <%= setNewPasswordUrl %> - +templates.passwordResetRequested = CTAEmailTemplate({ + subject: () -> "Password Reset - #{settings.appName}" + title: () -> "Password Reset" + message: () -> "We got a request to reset your #{settings.appName} password." + secondaryMessage: () -> """ If you ignore this message, your password won't be changed. If you didn't request a password reset, let us know. - -Thank you - -#{settings.appName} - <%= siteUrl %> """ - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "Password Reset" - greeting: "Hi," - message: "We got a request to reset your #{settings.appName} password." - secondaryMessage: "If you ignore this message, your password won't be changed.
If you didn't request a password reset, let us know." - ctaText: "Reset password" - ctaURL: opts.setNewPasswordUrl - gmailGoToAction: null - }) + ctaText: () -> "Reset password" + ctaURL: (opts) -> opts.setNewPasswordUrl +}) +templates.confirmEmail = CTAEmailTemplate({ + subject: () -> "Confirm Email - #{settings.appName}" + title: () -> "Confirm Email" + message: () -> "Please confirm your email on #{settings.appName}." + ctaText: () -> "Confirm Email" + ctaURL: (opts) -> opts.confirmEmailUrl +}) -templates.projectInvite = - subject: _.template "<%= project.name %> - shared by <%= owner.email %>" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Hi, <%= owner.email %> wants to share '<%= project.name %>' with you. +templates.projectInvite = CTAEmailTemplate({ + subject: (opts) -> "#{opts.project.name} - shared by #{opts.owner.email}" + title: (opts) -> "#{ opts.project.name } - shared by #{ opts.owner.email }" + message: (opts) -> "#{ opts.owner.email } wants to share '#{ opts.project.name }' with you." + ctaText: () -> "View project" + ctaURL: (opts) -> opts.inviteUrl + gmailGoToAction: (opts) -> + target: opts.inviteUrl + name: "View project" + description: "Join #{ opts.project.name } at #{ settings.appName }" +}) -Follow this link to view the project: <%= inviteUrl %> - -Thank you - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "#{ opts.project.name } – shared by #{ opts.owner.email }" - greeting: "Hi," - message: "#{ opts.owner.email } wants to share “#{ opts.project.name }” with you." - secondaryMessage: null - ctaText: "View project" - ctaURL: opts.inviteUrl - gmailGoToAction: - target: opts.inviteUrl - name: "View project" - description: "Join #{ opts.project.name } at ShareLaTeX" - }) - - -templates.verifyEmailToJoinTeam = - subject: _.template "<%= inviterName %> has invited you to join a team on #{settings.appName}" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ - -Please click the button below to join the team and enjoy the benefits of an upgraded <%= appName %> account. - -<%= acceptInviteUrl %> - -Thank You - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "#{opts.inviterName} has invited you to join a team on #{settings.appName}" - greeting: "Hi," - message: "Please click the button below to join the team and enjoy the benefits of an upgraded #{ opts.appName } account." - secondaryMessage: null - ctaText: "Verify now" - ctaURL: opts.acceptInviteUrl - gmailGoToAction: null - }) - -templates.testEmail = - subject: _.template "A Test Email from ShareLaTeX" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Hi, - -This is a test email sent from ShareLaTeX. - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "A Test Email from ShareLaTeX" - greeting: "Hi," - message: "This is a test email sent from ShareLaTeX" - secondaryMessage: null - ctaText: "Open ShareLaTeX" - ctaURL: "/" - gmailGoToAction: null - }) +templates.verifyEmailToJoinTeam = CTAEmailTemplate({ + subject: (opts) -> "#{ opts.inviterName } has invited you to join a team on #{settings.appName}" + title: (opts) -> "#{opts.inviterName} has invited you to join a team on #{settings.appName}" + message: (opts) -> "Please click the button below to join the team and enjoy the benefits of an upgraded #{ settings.appName } account." + ctaText: (opts) -> "Join now" + ctaURL: (opts) -> opts.acceptInviteUrl +}) +templates.testEmail = CTAEmailTemplate({ + subject: () -> "A Test Email from #{settings.appName}" + title: () -> "A Test Email from #{settings.appName}" + greeting: () -> "Hi," + message: () -> "This is a test Email from #{settings.appName}" + ctaText: () -> "Open #{settings.appName}" + ctaURL: () -> settings.siteUrl +}) module.exports = templates: templates - + CTAEmailTemplate: CTAEmailTemplate buildEmail: (templateName, opts)-> template = templates[templateName] opts.siteUrl = settings.siteUrl @@ -180,5 +125,4 @@ module.exports = subject : template.subject(opts) html: template.layout(opts) text: template?.plainTextTemplate?(opts) - type:template.type } From 0dcbc5facb934907ab212eebaa556e4e191091b1 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 13:55:34 +0100 Subject: [PATCH 18/21] Send out confirmation emails on register and record confirmedAt date --- .../PasswordReset/PasswordResetHandler.coffee | 4 +- .../Security/OneTimeTokenHandler.coffee | 18 +-- .../User/UserEmailsConfirmationHandler.coffee | 42 ++++++ .../Features/User/UserEmailsController.coffee | 43 ++++-- .../User/UserRegistrationHandler.coffee | 2 +- .../coffee/Features/User/UserUpdater.coffee | 25 +++- services/web/app/coffee/models/User.coffee | 3 +- services/web/app/coffee/router.coffee | 4 + services/web/app/views/user/confirm_email.pug | 28 ++++ .../public/coffee/directives/asyncForm.coffee | 16 ++- .../acceptance/coffee/UserEmailsTests.coffee | 132 ++++++++++++++++++ .../PasswordResetHandlerTests.coffee | 10 +- .../Security/OneTimeTokenHandlerTests.coffee | 8 +- .../UserEmailsConfirmationHandlerTests.coffee | 125 +++++++++++++++++ .../User/UserEmailsControllerTests.coffee | 92 +++++++----- .../User/UserRegistrationHandlerTests.coffee | 4 +- .../unit/coffee/User/UserUpdaterTests.coffee | 39 +++++- 17 files changed, 516 insertions(+), 79 deletions(-) create mode 100644 services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee create mode 100644 services/web/app/views/user/confirm_email.pug create mode 100644 services/web/test/acceptance/coffee/UserEmailsTests.coffee create mode 100644 services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee index 3947b63004..d383e8b669 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee @@ -14,7 +14,7 @@ module.exports = if !user? or user.holdingAccount logger.err email:email, "user could not be found for password reset" return callback(null, false) - OneTimeTokenHandler.getNewToken user._id, (err, token)-> + OneTimeTokenHandler.getNewToken 'password', user._id, (err, token)-> if err then return callback(err) emailOptions = to : email @@ -24,7 +24,7 @@ module.exports = callback null, true setNewUserPassword: (token, password, callback = (error, found, user_id) ->)-> - OneTimeTokenHandler.getValueFromTokenAndExpire token, (err, user_id)-> + OneTimeTokenHandler.getValueFromTokenAndExpire 'password', token, (err, user_id)-> if err then return callback(err) if !user_id? return callback null, false, null diff --git a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee index 3824703efb..c989ef9505 100644 --- a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee +++ b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee @@ -6,29 +6,29 @@ logger = require("logger-sharelatex") ONE_HOUR_IN_S = 60 * 60 -buildKey = (token)-> return "password_token:#{token}" +buildKey = (use, token)-> return "#{use}_token:#{token}" module.exports = - getNewToken: (value, options = {}, callback)-> + getNewToken: (use, value, options = {}, callback)-> # options is optional if typeof options == "function" callback = options options = {} expiresIn = options.expiresIn or ONE_HOUR_IN_S - logger.log value:value, "generating token for password reset" token = crypto.randomBytes(32).toString("hex") + logger.log {value, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" multi = rclient.multi() - multi.set buildKey(token), value - multi.expire buildKey(token), expiresIn + multi.set buildKey(use, token), value + multi.expire buildKey(use, token), expiresIn multi.exec (err)-> callback(err, token) - getValueFromTokenAndExpire: (token, callback)-> - logger.log token:token, "getting user id from password token" + getValueFromTokenAndExpire: (use, token, callback)-> + logger.log token_start: token.slice(0,8), "getting value from #{use} token" multi = rclient.multi() - multi.get buildKey(token) - multi.del buildKey(token) + multi.get buildKey(use, token) + multi.del buildKey(use, token) multi.exec (err, results)-> callback err, results?[0] diff --git a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee new file mode 100644 index 0000000000..1fbad4a294 --- /dev/null +++ b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee @@ -0,0 +1,42 @@ +EmailHelper = require "../Helpers/EmailHelper" +EmailHandler = require "../Email/EmailHandler" +OneTimeTokenHandler = require "../Security/OneTimeTokenHandler" +settings = require 'settings-sharelatex' +Errors = require "../Errors/Errors" +logger = require "logger-sharelatex" +UserUpdater = require "./UserUpdater" + +ONE_YEAR_IN_S = 365 * 24 * 60 * 60 + +module.exports = UserEmailsConfirmationHandler = + serializeData: (user_id, email) -> + JSON.stringify({user_id, email}) + + deserializeData: (data) -> + JSON.parse(data) + + sendConfirmationEmail: (user_id, email, emailTemplate, callback = (error) ->) -> + if arguments.length == 3 + callback = emailTemplate + emailTemplate = 'confirmEmail' + email = EmailHelper.parseEmail(email) + return callback(new Error('invalid email')) if !email? + value = UserEmailsConfirmationHandler.serializeData(user_id, email) + OneTimeTokenHandler.getNewToken 'email_confirmation', value, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> + return callback(err) if err? + emailOptions = + to: email + confirmEmailUrl: "#{settings.siteUrl}/user/emails/confirm?token=#{token}" + EmailHandler.sendEmail emailTemplate, emailOptions, callback + + confirmEmailFromToken: (token, callback = (error) ->) -> + logger.log {token_start: token.slice(0,8)}, 'confirming email from token' + OneTimeTokenHandler.getValueFromTokenAndExpire 'email_confirmation', token, (error, data) -> + return callback(error) if error? + if !data? + return callback(new Errors.NotFoundError('no token found')) + {user_id, email} = UserEmailsConfirmationHandler.deserializeData(data) + logger.log {data, user_id, email, token_start: token.slice(0,8)}, 'found data for email confirmation' + if !user_id? or email != EmailHelper.parseEmail(email) + return callback(new Errors.NotFoundError('invalid data')) + UserUpdater.confirmEmail user_id, email, callback diff --git a/services/web/app/coffee/Features/User/UserEmailsController.coffee b/services/web/app/coffee/Features/User/UserEmailsController.coffee index 07433d2dde..f877cbe345 100644 --- a/services/web/app/coffee/Features/User/UserEmailsController.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsController.coffee @@ -2,42 +2,67 @@ AuthenticationController = require('../Authentication/AuthenticationController') UserGetter = require("./UserGetter") UserUpdater = require("./UserUpdater") EmailHelper = require("../Helpers/EmailHelper") +UserEmailsConfirmationHandler = require "./UserEmailsConfirmationHandler" logger = require("logger-sharelatex") +Errors = require "../Errors/Errors" module.exports = UserEmailsController = - list: (req, res) -> + list: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) UserGetter.getUserFullEmails userId, (error, fullEmails) -> - return res.sendStatus 500 if error? + return next(error) if error? res.json fullEmails - add: (req, res) -> + add: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.addEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? - res.sendStatus 200 + return next(error) if error? + UserEmailsConfirmationHandler.sendConfirmationEmail userId, email, (err) -> + return next(error) if error? + res.sendStatus 204 - remove: (req, res) -> + remove: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.removeEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? + return next(error) if error? res.sendStatus 200 - setDefault: (req, res) -> + setDefault: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.setDefaultEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? + return next(error) if error? res.sendStatus 200 + + showConfirm: (req, res, next) -> + res.render 'user/confirm_email', { + token: req.query.token, + title: 'confirm_email' + } + + confirm: (req, res, next) -> + token = req.body.token + if !token? + return res.sendStatus 422 + UserEmailsConfirmationHandler.confirmEmailFromToken token, (error) -> + if error? + if error instanceof Errors.NotFoundError + res.status(404).json({ + message: 'Sorry, your confirmation token is invalid or has expired. Please request a new email confirmation link.' + }) + else + next(error) + else + res.sendStatus 200 \ No newline at end of file diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 85cdabdbdf..df7fe93218 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -74,7 +74,7 @@ module.exports = UserRegistrationHandler = logger.log {email}, "user already exists, resending welcome email" ONE_WEEK = 7 * 24 * 60 * 60 # seconds - OneTimeTokenHandler.getNewToken user._id, { expiresIn: ONE_WEEK }, (err, token)-> + OneTimeTokenHandler.getNewToken 'password', user._id, { expiresIn: ONE_WEEK }, (err, token)-> return callback(err) if err? setNewPasswordUrl = "#{settings.siteUrl}/user/activate?token=#{token}&user_id=#{user._id}" diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 22b31239bd..4de994b5c2 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -5,6 +5,8 @@ db = mongojs.db async = require("async") ObjectId = mongojs.ObjectId UserGetter = require("./UserGetter") +EmailHelper = require "../Helpers/EmailHelper" +Errors = require "../Errors/Errors" module.exports = UserUpdater = updateUser: (query, update, callback = (error) ->) -> @@ -53,7 +55,6 @@ module.exports = UserUpdater = return callback(error) callback() - # remove one of the user's email addresses. The email cannot be the user's # default email address removeEmailAddress: (userId, email, callback) -> @@ -63,7 +64,7 @@ module.exports = UserUpdater = if error? logger.err error:error, 'problem removing users email' return callback(error) - if res.nMatched == 0 + if res.n == 0 return callback(new Error('Cannot remove default email')) callback() @@ -77,10 +78,28 @@ module.exports = UserUpdater = if error? logger.err error:error, 'problem setting default emails' return callback(error) - if res.nMatched == 0 + if res.n == 0 # TODO: Check n or nMatched? return callback(new Error('Default email does not belong to user')) callback() + confirmEmail: (userId, email, callback) -> + email = EmailHelper.parseEmail(email) + return callback(new Error('invalid email')) if !email? + logger.log {userId, email}, 'confirming user email' + query = + _id: userId + 'emails.email': email + update = + $set: + 'emails.$.confirmedAt': new Date() + @updateUser query, update, (error, res) -> + return callback(error) if error? + logger.log {res, userId, email}, "tried to confirm email" + if res.n == 0 + return callback(new Errors.NotFoundError('user id and email do no match')) + callback() + + [ 'updateUser' 'changeEmailAddress' diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index 71982ba40b..730b3668c6 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -10,7 +10,8 @@ UserSchema = new Schema email : {type : String, default : ''} emails: [{ email: { type : String, default : '' }, - createdAt: { type : Date, default: () -> new Date() } + createdAt: { type : Date, default: () -> new Date() }, + confirmedAt: { type: Date } }], first_name : {type : String, default : ''} last_name : {type : String, default : ''} diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 339b4abe1c..6014fea661 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -120,6 +120,10 @@ module.exports = class Router webRouter.post '/user/emails/default', AuthenticationController.requireLogin(), UserEmailsController.setDefault + webRouter.get '/user/emails/confirm', + UserEmailsController.showConfirm + webRouter.post '/user/emails/confirm', + UserEmailsController.confirm webRouter.get '/user/sessions', AuthenticationController.requireLogin(), diff --git a/services/web/app/views/user/confirm_email.pug b/services/web/app/views/user/confirm_email.pug new file mode 100644 index 0000000000..388a0a0252 --- /dev/null +++ b/services/web/app/views/user/confirm_email.pug @@ -0,0 +1,28 @@ +extends ../layout + +block content + .content.content-alt + .container + .row + .col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4 + .card + .page-header + h1 #{translate("confirm_email")} + form( + async-form="confirm-email", + name="confirmEmailForm" + action="/user/emails/confirm", + method="POST", + id="confirmEmailForm", + auto-submit="true", + ng-cloak + ) + input(type="hidden", name="_csrf", value=csrfToken) + input(type="hidden", name="token", value=token) + form-messages(for="confirmEmailForm") + .alert.alert-success(ng-show="confirmEmailForm.response.success") + | Thank you, your email is now confirmed + p.text-center(ng-show="!confirmEmailForm.response.success && !confirmEmailForm.response.error") + i.fa.fa-fw.fa-spin.fa-spinner + | + | Confirming your email... diff --git a/services/web/public/coffee/directives/asyncForm.coffee b/services/web/public/coffee/directives/asyncForm.coffee index 15e3c9b3fe..acafec563d 100644 --- a/services/web/public/coffee/directives/asyncForm.coffee +++ b/services/web/public/coffee/directives/asyncForm.coffee @@ -15,11 +15,6 @@ define [ scope[attrs.name].response = response = {} scope[attrs.name].inflight = false - element.on "submit", (e) -> - e.preventDefault() - validateCaptchaIfEnabled (response) -> - submitRequest response - validateCaptchaIfEnabled = (callback = (response) ->) -> if attrs.captcha? validateCaptcha callback @@ -84,6 +79,17 @@ define [ text: data.message?.text or data.message or "Something went wrong talking to the server :(. Please try again." type: 'error' ga('send', 'event', formName, 'failure', data.message) + + submit = () -> + validateCaptchaIfEnabled (response) -> + submitRequest response + + element.on "submit", (e) -> + e.preventDefault() + submit() + + if attrs.autoSubmit + submit() } App.directive "formMessages", () -> diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee new file mode 100644 index 0000000000..8cdbea75fa --- /dev/null +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -0,0 +1,132 @@ +expect = require("chai").expect +async = require("async") +User = require "./helpers/User" +request = require "./helpers/request" +settings = require "settings-sharelatex" +rclient = require("redis-sharelatex").createClient(settings.redis.web) + +describe "UserEmails", -> + beforeEach (done) -> + @timeout(20000) + @user = new User() + @user.login done + + describe 'confirming an email', -> + it 'should confirm the email', (done) -> + token = null + async.series [ + (cb) => + @user.request { + method: 'POST', + url: '/user/emails', + json: + email: 'newly-added-email@example.com' + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 204 + cb() + (cb) => + @user.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.not.exist + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # There should only be one confirmation token at the moment + expect(keys.length).to.equal 1 + token = keys[0].split(':')[1] + cb() + (cb) => + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 200 + cb() + (cb) => + @user.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.exist + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # Token should be deleted after use + expect(keys.length).to.equal 0 + cb() + ], done + + it 'should not allow confirmation of the email if the user has changed', (done) -> + token1 = null + token2 = null + @user2 = new User() + @email = 'duplicate-email@example.com' + async.series [ + (cb) => @user2.login cb + (cb) => + # Create email for first user + @user.request { + method: 'POST', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # There should only be one confirmation token at the moment, + # for the first user + expect(keys.length).to.equal 1 + token1 = keys[0].split(':')[1] + cb() + (cb) => + # Delete the email from the first user + @user.request { + method: 'DELETE', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + # Create email for second user + @user2.request { + method: 'POST', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + # Original confirmation token should no longer work + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token1 + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 404 + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # The first token has been used, so this should be token2 now + expect(keys.length).to.equal 1 + token2 = keys[0].split(':')[1] + cb() + (cb) => + # Second user should be able to confirm the email + @user2.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token2 + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 200 + cb() + (cb) => + @user2.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.exist + cb() + ], done diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee index 261f5582dd..7ee82fbd7e 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee @@ -41,7 +41,7 @@ describe "PasswordResetHandler", -> it "should check the user exists", (done)-> @UserGetter.getUserByMainEmail.callsArgWith(1) - @OneTimeTokenHandler.getNewToken.callsArgWith(1) + @OneTimeTokenHandler.getNewToken.yields() @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> exists.should.equal false done() @@ -50,7 +50,7 @@ describe "PasswordResetHandler", -> it "should send the email with the token", (done)-> @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.callsArgWith(1, null, @token) + @OneTimeTokenHandler.getNewToken.yields(null, @token) @EmailHandler.sendEmail.callsArgWith(2) @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> @EmailHandler.sendEmail.called.should.equal true @@ -63,7 +63,7 @@ describe "PasswordResetHandler", -> it "should return exists = false for a holdingAccount", (done) -> @user.holdingAccount = true @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.callsArgWith(1) + @OneTimeTokenHandler.getNewToken.yields() @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> exists.should.equal false done() @@ -71,14 +71,14 @@ describe "PasswordResetHandler", -> describe "setNewUserPassword", -> it "should return false if no user id can be found", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.callsArgWith(1) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields() @PasswordResetHandler.setNewUserPassword @token, @password, (err, found) => found.should.equal false @AuthenticationManager.setUserPassword.called.should.equal false done() it "should set the user password", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.callsArgWith(1, null, @user_id) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, @user_id) @AuthenticationManager.setUserPassword.callsArgWith(2) @PasswordResetHandler.setNewUserPassword @token, @password, (err, found, user_id) => found.should.equal true diff --git a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee index 1940f34d07..21e8d9f288 100644 --- a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee +++ b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee @@ -37,21 +37,21 @@ describe "OneTimeTokenHandler", -> it "should set a new token into redis with a ttl", (done)-> @redisMulti.exec.callsArgWith(0) - @OneTimeTokenHandler.getNewToken @value, (err, token) => + @OneTimeTokenHandler.getNewToken 'password', @value, (err, token) => @redisMulti.set.calledWith("password_token:#{@stubbedToken.toString("hex")}", @value).should.equal true @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", 60 * 60).should.equal true done() it "should return if there was an error", (done)-> @redisMulti.exec.callsArgWith(0, "error") - @OneTimeTokenHandler.getNewToken @value, (err, token)=> + @OneTimeTokenHandler.getNewToken 'password', @value, (err, token)=> err.should.exist done() it "should allow the expiry time to be overridden", (done) -> @redisMulti.exec.callsArgWith(0) @ttl = 42 - @OneTimeTokenHandler.getNewToken @value, {expiresIn: @ttl}, (err, token) => + @OneTimeTokenHandler.getNewToken 'password', @value, {expiresIn: @ttl}, (err, token) => @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", @ttl).should.equal true done() @@ -59,7 +59,7 @@ describe "OneTimeTokenHandler", -> it "should get and delete the token", (done)-> @redisMulti.exec.callsArgWith(0, null, [@value]) - @OneTimeTokenHandler.getValueFromTokenAndExpire @stubbedToken, (err, value)=> + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', @stubbedToken, (err, value)=> value.should.equal @value @redisMulti.get.calledWith("password_token:#{@stubbedToken}").should.equal true @redisMulti.del.calledWith("password_token:#{@stubbedToken}").should.equal true diff --git a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee new file mode 100644 index 0000000000..22cd70c4bc --- /dev/null +++ b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee @@ -0,0 +1,125 @@ +should = require('chai').should() +SandboxedModule = require('sandboxed-module') +assert = require('assert') +path = require('path') +sinon = require('sinon') +modulePath = path.join __dirname, "../../../../app/js/Features/User/UserEmailsConfirmationHandler" +expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" +EmailHelper = require "../../../../app/js/Features/Helpers/EmailHelper" + +describe "UserEmailsConfirmationHandler", -> + beforeEach -> + @UserEmailsConfirmationHandler = SandboxedModule.require modulePath, requires: + "settings-sharelatex": @settings = + siteUrl: "emails.example.com" + "logger-sharelatex": @logger = { log: sinon.stub() } + "../Security/OneTimeTokenHandler": @OneTimeTokenHandler = {} + "../Errors/Errors": Errors + "./UserUpdater": @UserUpdater = {} + "../Email/EmailHandler": @EmailHandler = {} + "../Helpers/EmailHelper": EmailHelper + @user_id = "mock-user-id" + @email = "mock@example.com" + @callback = sinon.stub() + + describe "sendConfirmationEmail", -> + beforeEach -> + @OneTimeTokenHandler.getNewToken = sinon.stub().yields(null, @token = "new-token") + @EmailHandler.sendEmail = sinon.stub().yields() + + describe 'successfully', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, @email, @callback + + it "should generate a token for the user which references their id and email", -> + @OneTimeTokenHandler.getNewToken + .calledWith( + 'email_confirmation', + JSON.stringify({@user_id, @email}), + { expiresIn: 365 * 24 * 60 * 60 } + ) + .should.equal true + + it 'should send an email to the user', -> + @EmailHandler.sendEmail + .calledWith('confirmEmail', { + to: @email, + confirmEmailUrl: 'emails.example.com/user/emails/confirm?token=new-token' + }) + .should.equal true + + it 'should call the callback', -> + @callback.called.should.equal true + + describe 'with invalid email', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, '!"£$%^&*()', @callback + + it 'should return an error', -> + @callback.calledWith(sinon.match.instanceOf(Error)).should.equal true + + describe 'a custom template', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, @email, 'myCustomTemplate', @callback + + it 'should send an email with the given template', -> + @EmailHandler.sendEmail + .calledWith('myCustomTemplate') + .should.equal true + + describe "confirmEmailFromToken", -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@user_id, @email}) + ) + @UserUpdater.confirmEmail = sinon.stub().yields() + + describe "successfully", -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call getValueFromTokenAndExpire", -> + @OneTimeTokenHandler.getValueFromTokenAndExpire + .calledWith('email_confirmation', @token) + .should.equal true + + it "should confirm the email of the user_id", -> + @UserUpdater.confirmEmail + .calledWith(@user_id, @email) + .should.equal true + + it "should call the callback", -> + @callback.called.should.equal true + + describe 'with an expired token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields(null, null) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + + describe 'with no user_id in the token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@email}) + ) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + + describe 'with no email in the token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@user_id}) + ) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + diff --git a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee index 5b4a32fb38..d0e8076276 100644 --- a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee +++ b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee @@ -8,6 +8,7 @@ modulePath = "../../../../app/js/Features/User/UserEmailsController.js" SandboxedModule = require('sandboxed-module') MockRequest = require "../helpers/MockRequest" MockResponse = require "../helpers/MockResponse" +Errors = require("../../../../app/js/Features/Errors/Errors") describe "UserEmailsController", -> beforeEach -> @@ -30,6 +31,8 @@ describe "UserEmailsController", -> "./UserGetter": @UserGetter "./UserUpdater": @UserUpdater "../Helpers/EmailHelper": @EmailHelper + "./UserEmailsConfirmationHandler": @UserEmailsConfirmationHandler = {} + "../Errors/Errors": Errors "logger-sharelatex": log: -> console.log(arguments) err: -> @@ -47,30 +50,29 @@ describe "UserEmailsController", -> assertCalledWith @UserGetter.getUserFullEmails, @user._id done() - it 'handles error', (done) -> - @UserGetter.getUserFullEmails.callsArgWith 1, new Error('Oups') - - @UserEmailsController.list @req, - sendStatus: (code) => - code.should.equal 500 - done() - describe 'Add', -> beforeEach -> @newEmail = 'new_email@baz.com' @req.body.email = @newEmail @EmailHelper.parseEmail.returns @newEmail + @UserUpdater.addEmailAddress.callsArgWith 2, null + @UserEmailsConfirmationHandler.sendConfirmationEmail = sinon.stub().yields() it 'adds new email', (done) -> - @UserUpdater.addEmailAddress.callsArgWith 2, null - @UserEmailsController.add @req, sendStatus: (code) => - code.should.equal 200 + code.should.equal 204 assertCalledWith @EmailHelper.parseEmail, @newEmail assertCalledWith @UserUpdater.addEmailAddress, @user._id, @newEmail done() + it 'sends an email confirmation', (done) -> + @UserEmailsController.add @req, + sendStatus: (code) => + code.should.equal 204 + assertCalledWith @UserEmailsConfirmationHandler.sendConfirmationEmail, @user._id, @newEmail + done() + it 'handles email parse error', (done) -> @EmailHelper.parseEmail.returns null @@ -80,14 +82,6 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.addEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.addEmailAddress.callsArgWith 2, new Error('Oups') - - @UserEmailsController.add @req, - sendStatus: (code) => - code.should.equal 500 - done() - describe 'remove', -> beforeEach -> @email = 'email_to_remove@bar.com' @@ -113,15 +107,6 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.removeEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.removeEmailAddress.callsArgWith 2, new Error('Oups') - - @UserEmailsController.remove @req, - sendStatus: (code) => - code.should.equal 500 - done() - - describe 'setDefault', -> beforeEach -> @email = "email_to_set_default@bar.com" @@ -147,11 +132,50 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.setDefaultEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.setDefaultEmailAddress.callsArgWith 2, new Error('Oups') + describe 'confirm', -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken = sinon.stub().yields() + @res = + sendStatus: sinon.stub() + json: sinon.stub() + @res.status = sinon.stub().returns(@res) + @next = sinon.stub() + @token = 'mock-token' + @req.body = token: @token + + describe 'successfully', -> + beforeEach -> + @UserEmailsController.confirm @req, @res, @next + + it 'should confirm the email from the token', -> + @UserEmailsConfirmationHandler.confirmEmailFromToken + .calledWith(@token) + .should.equal true + + it 'should return a 200 status', -> + @res.sendStatus.calledWith(200).should.equal true + + describe 'without a token', -> + beforeEach -> + @req.body.token = null + @UserEmailsController.confirm @req, @res, @next + + it 'should return a 422 status', -> + @res.sendStatus.calledWith(422).should.equal true + + describe 'when confirming fails', -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken = sinon.stub().yields( + new Errors.NotFoundError('not found') + ) + @UserEmailsController.confirm @req, @res, @next + + it 'should return a 404 error code with a message', -> + @res.status.calledWith(404).should.equal true + @res.json.calledWith({ + message: 'Sorry, your confirmation token is invalid or has expired. Please request a new email confirmation link.' + }).should.equal true + + - @UserEmailsController.setDefault @req, - sendStatus: (code) => - code.should.equal 500 - done() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index 3a8801bc08..e9a6c568dd 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee @@ -152,7 +152,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @email = "email@example.com" @crypto.randomBytes = sinon.stub().returns({toString: () => @password = "mock-password"}) - @OneTimeTokenHandler.getNewToken.callsArgWith(2, null, @token = "mock-token") + @OneTimeTokenHandler.getNewToken.yields(null, @token = "mock-token") @handler.registerNewUser = sinon.stub() @callback = sinon.stub() @@ -171,7 +171,7 @@ describe "UserRegistrationHandler", -> it "should generate a new password reset token", -> @OneTimeTokenHandler.getNewToken - .calledWith(@user_id, expiresIn: 7 * 24 * 60 * 60) + .calledWith('password', @user_id, expiresIn: 7 * 24 * 60 * 60) .should.equal true it "should send a registered email", -> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 7b3be3df2b..6b9a1ecc85 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -5,11 +5,12 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/User/UserUpdater" expect = require("chai").expect +tk = require('timekeeper') describe "UserUpdater", -> beforeEach -> - + tk.freeze(Date.now()) @settings = {} @mongojs = db:{} @@ -32,6 +33,9 @@ describe "UserUpdater", -> email:"hello@world.com" @newEmail = "bob@bob.com" + afterEach -> + tk.reset() + describe 'changeEmailAddress', -> beforeEach -> @UserGetter.getUserEmail.callsArgWith(1, null, @stubbedUser.email) @@ -103,7 +107,7 @@ describe "UserUpdater", -> done() it 'handle missed update', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> should.exist(err) @@ -111,7 +115,7 @@ describe "UserUpdater", -> describe 'setDefaultEmailAddress', -> it 'set default', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> should.not.exist(err) @@ -129,10 +133,37 @@ describe "UserUpdater", -> done() it 'handle missed update', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> should.exist(err) done() + describe 'confirmEmail', -> + it 'should update the email record', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @UserUpdater.updateUser.calledWith( + { _id: @stubbedUser._id, 'emails.email': @newEmail }, + $set: { 'emails.$.confirmedAt': new Date() } + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + it 'handle missed update', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + From 4608a59e3db340deb489e2872da5e70bc2e56c62 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 20 Jun 2018 17:27:22 +0100 Subject: [PATCH 19/21] Store OneTimeTokens in mongo rather than redis --- .../Security/OneTimeTokenHandler.coffee | 54 +++++--- .../User/UserEmailsConfirmationHandler.coffee | 12 +- .../app/coffee/infrastructure/mongojs.coffee | 2 +- .../acceptance/coffee/UserEmailsTests.coffee | 92 ++++++++++--- .../Security/OneTimeTokenHandlerTests.coffee | 121 +++++++++++------- .../UserEmailsConfirmationHandlerTests.coffee | 8 +- 6 files changed, 198 insertions(+), 91 deletions(-) diff --git a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee index c989ef9505..69c9f5b0e9 100644 --- a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee +++ b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee @@ -1,34 +1,50 @@ Settings = require('settings-sharelatex') -RedisWrapper = require("../../infrastructure/RedisWrapper") -rclient = RedisWrapper.client("one_time_token") crypto = require("crypto") logger = require("logger-sharelatex") +{db} = require "../../infrastructure/mongojs" +Errors = require "../Errors/Errors" ONE_HOUR_IN_S = 60 * 60 -buildKey = (use, token)-> return "#{use}_token:#{token}" - module.exports = - - getNewToken: (use, value, options = {}, callback)-> + getNewToken: (use, data, options = {}, callback = (error, data) ->)-> # options is optional if typeof options == "function" callback = options options = {} expiresIn = options.expiresIn or ONE_HOUR_IN_S + createdAt = new Date() + expiresAt = new Date(createdAt.getTime() + expiresIn * 1000) token = crypto.randomBytes(32).toString("hex") - logger.log {value, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" - multi = rclient.multi() - multi.set buildKey(use, token), value - multi.expire buildKey(use, token), expiresIn - multi.exec (err)-> - callback(err, token) + logger.log {data, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" + db.tokens.insert { + use: use + token: token, + data: data, + createdAt: createdAt, + expiresAt: expiresAt + }, (error) -> + return callback(error) if error? + callback null, token - getValueFromTokenAndExpire: (use, token, callback)-> - logger.log token_start: token.slice(0,8), "getting value from #{use} token" - multi = rclient.multi() - multi.get buildKey(use, token) - multi.del buildKey(use, token) - multi.exec (err, results)-> - callback err, results?[0] + getValueFromTokenAndExpire: (use, token, callback = (error, data) ->)-> + logger.log token_start: token.slice(0,8), "getting data from #{use} token" + now = new Date() + db.tokens.findAndModify { + query: { + use: use, + token: token, + expiresAt: { $gt: now }, + usedAt: { $exists: false } + }, + update: { + $set: { + usedAt: now + } + } + }, (error, token) -> + return callback(error) if error? + if !token? + return callback(new Errors.NotFoundError('no token found')) + return callback null, token.data diff --git a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee index 1fbad4a294..dd87570450 100644 --- a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee @@ -9,20 +9,14 @@ UserUpdater = require "./UserUpdater" ONE_YEAR_IN_S = 365 * 24 * 60 * 60 module.exports = UserEmailsConfirmationHandler = - serializeData: (user_id, email) -> - JSON.stringify({user_id, email}) - - deserializeData: (data) -> - JSON.parse(data) - sendConfirmationEmail: (user_id, email, emailTemplate, callback = (error) ->) -> if arguments.length == 3 callback = emailTemplate emailTemplate = 'confirmEmail' email = EmailHelper.parseEmail(email) return callback(new Error('invalid email')) if !email? - value = UserEmailsConfirmationHandler.serializeData(user_id, email) - OneTimeTokenHandler.getNewToken 'email_confirmation', value, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> + data = {user_id, email} + OneTimeTokenHandler.getNewToken 'email_confirmation', data, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> return callback(err) if err? emailOptions = to: email @@ -35,7 +29,7 @@ module.exports = UserEmailsConfirmationHandler = return callback(error) if error? if !data? return callback(new Errors.NotFoundError('no token found')) - {user_id, email} = UserEmailsConfirmationHandler.deserializeData(data) + {user_id, email} = data logger.log {data, user_id, email, token_start: token.slice(0,8)}, 'found data for email confirmation' if !user_id? or email != EmailHelper.parseEmail(email) return callback(new Errors.NotFoundError('invalid data')) diff --git a/services/web/app/coffee/infrastructure/mongojs.coffee b/services/web/app/coffee/infrastructure/mongojs.coffee index f1ed213435..8492b9023c 100644 --- a/services/web/app/coffee/infrastructure/mongojs.coffee +++ b/services/web/app/coffee/infrastructure/mongojs.coffee @@ -1,6 +1,6 @@ Settings = require "settings-sharelatex" mongojs = require "mongojs" -db = mongojs(Settings.mongo.url, ["projects", "users", "userstubs"]) +db = mongojs(Settings.mongo.url, ["projects", "users", "userstubs", "tokens"]) module.exports = db: db ObjectId: mongojs.ObjectId diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee index 8cdbea75fa..248c0331ae 100644 --- a/services/web/test/acceptance/coffee/UserEmailsTests.coffee +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -3,7 +3,7 @@ async = require("async") User = require "./helpers/User" request = require "./helpers/request" settings = require "settings-sharelatex" -rclient = require("redis-sharelatex").createClient(settings.redis.web) +{db, ObjectId} = require("../../../app/js/infrastructure/mongojs") describe "UserEmails", -> beforeEach (done) -> @@ -32,10 +32,15 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.not.exist cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # There should only be one confirmation token at the moment - expect(keys.length).to.equal 1 - token = keys[0].split(':')[1] + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal 'newly-added-email@example.com' + expect(tokens[0].data.user_id).to.equal @user._id + token = tokens[0].token cb() (cb) => @user.request { @@ -54,9 +59,12 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.exist cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # Token should be deleted after use - expect(keys.length).to.equal 0 + expect(tokens.length).to.equal 0 cb() ], done @@ -75,11 +83,15 @@ describe "UserEmails", -> json: {@email} }, cb (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> - # There should only be one confirmation token at the moment, - # for the first user - expect(keys.length).to.equal 1 - token1 = keys[0].split(':')[1] + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => + # There should only be one confirmation token at the moment + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user._id + token1 = tokens[0].token cb() (cb) => # Delete the email from the first user @@ -107,14 +119,19 @@ describe "UserEmails", -> expect(response.statusCode).to.equal 404 cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # The first token has been used, so this should be token2 now - expect(keys.length).to.equal 1 - token2 = keys[0].split(':')[1] + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user2._id + token2 = tokens[0].token cb() (cb) => # Second user should be able to confirm the email - @user2.request { + @user2.request { method: 'POST', url: '/user/emails/confirm', json: @@ -130,3 +147,48 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.exist cb() ], done + + describe "with an expired token", -> + it 'should not confirm the email', (done) -> + token = null + async.series [ + (cb) => + @user.request { + method: 'POST', + url: '/user/emails', + json: + email: @email = 'expired-token-email@example.com' + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 204 + cb() + (cb) => + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => + # There should only be one confirmation token at the moment + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user._id + token = tokens[0].token + cb() + (cb) => + db.tokens.update { + token: token + }, { + $set: { + expiresAt: new Date(Date.now() - 1000000) + } + }, cb + (cb) => + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 404 + cb() + ], done diff --git a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee index 21e8d9f288..c0e6d9cc13 100644 --- a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee +++ b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee @@ -5,64 +5,99 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/Security/OneTimeTokenHandler" expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" +tk = require("timekeeper") describe "OneTimeTokenHandler", -> - beforeEach -> - @value = "user id here" - @stubbedToken = require("crypto").randomBytes(32) - - @settings = - redis: - web:{} - @redisMulti = - set:sinon.stub() - get:sinon.stub() - del:sinon.stub() - expire:sinon.stub() - exec:sinon.stub() - self = @ + tk.freeze Date.now() # freeze the time for these tests + @stubbedToken = "mock-token" + @callback = sinon.stub() @OneTimeTokenHandler = SandboxedModule.require modulePath, requires: - "../../infrastructure/RedisWrapper" : - client: => - auth:-> - multi: -> return self.redisMulti - "settings-sharelatex":@settings "logger-sharelatex": log:-> "crypto": randomBytes: () => @stubbedToken + "../Errors/Errors": Errors + "../../infrastructure/mongojs": db: @db = tokens: {} + afterEach -> + tk.reset() describe "getNewToken", -> + beforeEach -> + @db.tokens.insert = sinon.stub().yields() - it "should set a new token into redis with a ttl", (done)-> - @redisMulti.exec.callsArgWith(0) - @OneTimeTokenHandler.getNewToken 'password', @value, (err, token) => - @redisMulti.set.calledWith("password_token:#{@stubbedToken.toString("hex")}", @value).should.equal true - @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", 60 * 60).should.equal true - done() + describe 'normally', -> + beforeEach -> + @OneTimeTokenHandler.getNewToken 'password', 'mock-data-to-store', @callback - it "should return if there was an error", (done)-> - @redisMulti.exec.callsArgWith(0, "error") - @OneTimeTokenHandler.getNewToken 'password', @value, (err, token)=> - err.should.exist - done() + it "should insert a generated token with a 1 hour expiry", -> + @db.tokens.insert + .calledWith({ + use: 'password' + token: @stubbedToken, + createdAt: new Date(), + expiresAt: new Date(Date.now() + 60 * 60 * 1000) + data: 'mock-data-to-store' + }) + .should.equal true - it "should allow the expiry time to be overridden", (done) -> - @redisMulti.exec.callsArgWith(0) - @ttl = 42 - @OneTimeTokenHandler.getNewToken 'password', @value, {expiresIn: @ttl}, (err, token) => - @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", @ttl).should.equal true - done() + it 'should call the callback with the token', -> + @callback.calledWith(null, @stubbedToken).should.equal true + + describe 'with an optional expiresIn parameter', -> + beforeEach -> + @OneTimeTokenHandler.getNewToken 'password', 'mock-data-to-store', { expiresIn: 42 }, @callback + + it "should insert a generated token with a custom expiry", -> + @db.tokens.insert + .calledWith({ + use: 'password' + token: @stubbedToken, + createdAt: new Date(), + expiresAt: new Date(Date.now() + 42 * 1000) + data: 'mock-data-to-store' + }) + .should.equal true + + it 'should call the callback with the token', -> + @callback.calledWith(null, @stubbedToken).should.equal true describe "getValueFromTokenAndExpire", -> + describe 'successfully', -> + beforeEach -> + @db.tokens.findAndModify = sinon.stub().yields(null, { data: 'mock-data' }) + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', 'mock-token', @callback + + it 'should expire the token', -> + @db.tokens.findAndModify + .calledWith({ + query: { + use: 'password' + token: 'mock-token', + expiresAt: { $gt: new Date() }, + usedAt: { $exists: false } + }, + update: { + $set: { usedAt: new Date() } + } + }) + .should.equal true + + it 'should return the data', -> + @callback.calledWith(null, 'mock-data').should.equal true + + describe 'when a valid token is not found', -> + beforeEach -> + @db.tokens.findAndModify = sinon.stub().yields(null, null) + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', 'mock-token', @callback + + it 'should return a NotFoundError', -> + @callback + .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) + .should.equal true + + - it "should get and delete the token", (done)-> - @redisMulti.exec.callsArgWith(0, null, [@value]) - @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', @stubbedToken, (err, value)=> - value.should.equal @value - @redisMulti.get.calledWith("password_token:#{@stubbedToken}").should.equal true - @redisMulti.del.calledWith("password_token:#{@stubbedToken}").should.equal true - done() diff --git a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee index 22cd70c4bc..95f72e0907 100644 --- a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee @@ -36,7 +36,7 @@ describe "UserEmailsConfirmationHandler", -> @OneTimeTokenHandler.getNewToken .calledWith( 'email_confirmation', - JSON.stringify({@user_id, @email}), + {@user_id, @email}, { expiresIn: 365 * 24 * 60 * 60 } ) .should.equal true @@ -72,7 +72,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@user_id, @email}) + {@user_id, @email} ) @UserUpdater.confirmEmail = sinon.stub().yields() @@ -105,7 +105,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@email}) + {@email} ) @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback @@ -116,7 +116,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@user_id}) + {@user_id} ) @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback From de45c08585bc93d124518336a4b6763e82c81c8c Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 21 Jun 2018 11:00:25 +0100 Subject: [PATCH 20/21] Restrict token look ups by id to not conflict with other tests --- services/web/test/acceptance/coffee/UserEmailsTests.coffee | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee index 248c0331ae..01f1dde531 100644 --- a/services/web/test/acceptance/coffee/UserEmailsTests.coffee +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -34,6 +34,7 @@ describe "UserEmails", -> (cb) => db.tokens.find { use: 'email_confirmation', + 'data.user_id': @user._id, usedAt: { $exists: false } }, (error, tokens) => # There should only be one confirmation token at the moment @@ -61,6 +62,7 @@ describe "UserEmails", -> (cb) => db.tokens.find { use: 'email_confirmation', + 'data.user_id': @user._id, usedAt: { $exists: false } }, (error, tokens) => # Token should be deleted after use @@ -85,6 +87,7 @@ describe "UserEmails", -> (cb) => db.tokens.find { use: 'email_confirmation', + 'data.user_id': @user._id, usedAt: { $exists: false } }, (error, tokens) => # There should only be one confirmation token at the moment @@ -121,6 +124,7 @@ describe "UserEmails", -> (cb) => db.tokens.find { use: 'email_confirmation', + 'data.user_id': @user2._id, usedAt: { $exists: false } }, (error, tokens) => # The first token has been used, so this should be token2 now @@ -165,6 +169,7 @@ describe "UserEmails", -> (cb) => db.tokens.find { use: 'email_confirmation', + 'data.user_id': @user._id, usedAt: { $exists: false } }, (error, tokens) => # There should only be one confirmation token at the moment From caee25d85d3b6aa778f3bb04015a3a5efc9b7107 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 21 Jun 2018 15:52:20 +0200 Subject: [PATCH 21/21] Revert "Replace OldAssetsProxy" --- .../infrastructure/OldAssetProxy.coffee | 16 +++++ .../coffee/infrastructure/ProxyManager.coffee | 25 -------- .../app/coffee/infrastructure/Server.coffee | 4 +- .../infrastructure/ProxyManagerTests.coffee | 60 ------------------- 4 files changed, 18 insertions(+), 87 deletions(-) create mode 100644 services/web/app/coffee/infrastructure/OldAssetProxy.coffee delete mode 100644 services/web/app/coffee/infrastructure/ProxyManager.coffee delete mode 100644 services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee diff --git a/services/web/app/coffee/infrastructure/OldAssetProxy.coffee b/services/web/app/coffee/infrastructure/OldAssetProxy.coffee new file mode 100644 index 0000000000..7912f0ed14 --- /dev/null +++ b/services/web/app/coffee/infrastructure/OldAssetProxy.coffee @@ -0,0 +1,16 @@ +settings = require("settings-sharelatex") +logger = require("logger-sharelatex") +request = require("request") + +module.exports = (req, res, next)-> + requestedUrl = req.url + + redirectUrl = settings.proxyUrls[requestedUrl] + if redirectUrl? + logger.log redirectUrl:redirectUrl, reqUrl:req.url, "proxying url" + upstream = request(redirectUrl) + upstream.on "error", (error) -> + logger.error err: error, "error in OldAssetProxy" + upstream.pipe(res) + else + next() diff --git a/services/web/app/coffee/infrastructure/ProxyManager.coffee b/services/web/app/coffee/infrastructure/ProxyManager.coffee deleted file mode 100644 index d93c696ab1..0000000000 --- a/services/web/app/coffee/infrastructure/ProxyManager.coffee +++ /dev/null @@ -1,25 +0,0 @@ -settings = require("settings-sharelatex") -logger = require("logger-sharelatex") -httpProxy = require 'express-http-proxy' - -module.exports = - # add proxy for all paths listed in `settings.proxyUrls`and log errors - apply: (app) -> - for requestUrl, target of settings.proxyUrls - targetUrl = @makeTargetUrl(target) - if targetUrl? - app.use requestUrl, httpProxy(targetUrl) - else - logger.error "Cannot make proxy target from #{target}" - - # takes a 'target' and return an URL to proxy to. - # 'target' can be: - # - a String, representing the URL - # - an Object with: - # - a path attribute (String) - # - a baseURL attribute (String) - makeTargetUrl: (target) -> - return target if typeof target is 'string' - return target.path unless target.baseUrl? - "#{target.baseUrl}#{target.path or ''}" - diff --git a/services/web/app/coffee/infrastructure/Server.coffee b/services/web/app/coffee/infrastructure/Server.coffee index 522599c659..39fc3c131b 100644 --- a/services/web/app/coffee/infrastructure/Server.coffee +++ b/services/web/app/coffee/infrastructure/Server.coffee @@ -32,7 +32,7 @@ Mongoose = require("./Mongoose") oneDayInMilliseconds = 86400000 ReferalConnect = require('../Features/Referal/ReferalConnect') RedirectManager = require("./RedirectManager") -ProxyManager = require("./ProxyManager") +OldAssetProxy = require("./OldAssetProxy") translations = require("translations-sharelatex").setup(Settings.i18n) Modules = require "./Modules" @@ -74,7 +74,7 @@ app.use methodOverride() app.use metrics.http.monitor(logger) app.use RedirectManager -ProxyManager.apply(app) +app.use OldAssetProxy webRouter.use cookieParser(Settings.security.sessionSecret) diff --git a/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee b/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee deleted file mode 100644 index 5782a4e2fc..0000000000 --- a/services/web/test/unit/coffee/infrastructure/ProxyManagerTests.coffee +++ /dev/null @@ -1,60 +0,0 @@ -sinon = require('sinon') -chai = require('chai') -should = chai.should() -expect = chai.expect -modulePath = '../../../../app/js/infrastructure/ProxyManager' -SandboxedModule = require('sandboxed-module') -MockRequest = require "../helpers/MockRequest" -MockResponse = require "../helpers/MockResponse" - -describe "ProxyManager", -> - before -> - @settings = proxyUrls: {} - @httpProxy = sinon.stub() - @proxyManager = SandboxedModule.require modulePath, requires: - 'settings-sharelatex': @settings - 'logger-sharelatex': - error: -> - log: -> console.log(arguments) - 'httpProxy': @httpProxy - - describe 'apply', -> - before -> - @sandbox = sinon.sandbox.create() - @app = use: sinon.stub() - @sandbox.stub(@proxyManager, 'makeTargetUrl') - - after -> - @sandbox.restore() - - beforeEach -> - @app.use.reset() - @requestUrl = '/foo/bar' - @settings.proxyUrls[@requestUrl] = 'http://whatever' - - it 'sets routes', -> - @proxyManager.makeTargetUrl.returns('http://whatever') - @proxyManager.apply(@app) - @app.use.calledWith(@requestUrl).should.equal true - - it 'logs errors', -> - @proxyManager.makeTargetUrl.returns(null) - @proxyManager.apply(@app) - @app.use.called.should.equal false - - describe 'makeTargetUrl', -> - it 'returns Strings', -> - target = 'http://whatever' - @proxyManager.makeTargetUrl(target).should.equal target - - it 'makes with path', -> - target = path: 'baz' - @proxyManager.makeTargetUrl(target).should.equal 'baz' - - it 'makes with baseUrl', -> - target = baseUrl: 'foo.bar' - @proxyManager.makeTargetUrl(target).should.equal 'foo.bar' - - it 'makes with baseUrl and path', -> - target = path: 'baz', baseUrl: 'foo.bar/' - @proxyManager.makeTargetUrl(target).should.equal 'foo.bar/baz'