From 998eb62e22d6e3186c517aa6685980b7018eed5f Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Fri, 8 May 2020 13:03:44 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Added=20success=20indicator=20for?= =?UTF-8?q?=20members=20magic=20links?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add a query param that indicates whether signin/up succeeded or failed - Add unit tests for all 3 possible cases for the createSessionFromMagicLink middleware - Added an acceptance test to show the behaviour works in principle --- core/server/services/members/middleware.js | 10 ++- test/frontend-acceptance/members_spec.js | 22 +++++ test/unit/services/members/middleware_spec.js | 81 +++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 test/unit/services/members/middleware_spec.js diff --git a/core/server/services/members/middleware.js b/core/server/services/members/middleware.js index 717774c0a5..e4e7c6ba69 100644 --- a/core/server/services/members/middleware.js +++ b/core/server/services/members/middleware.js @@ -108,16 +108,18 @@ const createSessionFromMagicLink = async function (req, res, next) { // We need to include the subdirectory, // members is already removed from the path by express because it's a mount path - const redirectPath = `${urlUtils.getSubdir()}${req.path}?${searchParams.toString()}`; + let redirectPath = `${urlUtils.getSubdir()}${req.path}`; try { await membersService.ssr.exchangeTokenForSession(req, res); - // Do a standard 302 redirect - return res.redirect(redirectPath); + // Do a standard 302 redirect, with success=true + searchParams.set('success', true); } catch (err) { logging.warn(err.message); - return res.redirect(redirectPath); + searchParams.set('success', false); + } finally { + res.redirect(`${redirectPath}?${searchParams.toString()}`); } }; diff --git a/test/frontend-acceptance/members_spec.js b/test/frontend-acceptance/members_spec.js index 8720e125ac..87444eba33 100644 --- a/test/frontend-acceptance/members_spec.js +++ b/test/frontend-acceptance/members_spec.js @@ -89,6 +89,18 @@ describe('Basic Members Routes', function () { return request.put('/members/api/subscriptions/123') .expect(400); }); + + it('should serve theme 404 on members endpoint', function () { + return request.get('/members/') + .expect(404) + .expect('Content-Type', 'text/html; charset=utf-8'); + }); + + it('should redirect invalid token on members endpoint', function () { + return request.get('/members/?token=abc&action=signup') + .expect(302) + .expect('Location', '/?action=signup&success=false'); + }); }); }); @@ -161,6 +173,16 @@ describe('Basic Members Routes', function () { return request.put('/members/api/subscriptions/123') .expect(404); }); + + it('should serve 404 on members endpoint', function () { + return request.get('/members/') + .expect(404); + }); + + it('should not redirect members endpoint with token', function () { + return request.get('/members/?token=abc&action=signup') + .expect(404); + }); }); }); }); diff --git a/test/unit/services/members/middleware_spec.js b/test/unit/services/members/middleware_spec.js new file mode 100644 index 0000000000..c7d0dce80e --- /dev/null +++ b/test/unit/services/members/middleware_spec.js @@ -0,0 +1,81 @@ +const should = require('should'); +const sinon = require('sinon'); + +const urlUtils = require('../../../../core/server/lib/url-utils'); +const membersService = require('../../../../core/server/services/members'); +const membersMiddleware = require('../../../../core/server/services/members/middleware'); + +describe('Members Service Middleware', function () { + describe('createSessionFromMagicLink', function () { + let req; + let res; + let next; + + beforeEach(function () { + req = {}; + res = {}; + next = sinon.stub(); + + res.redirect = sinon.stub().returns(''); + + // Stub the members Service, handle this in separate tests + membersService.ssr.exchangeTokenForSession = sinon.stub(); + + sinon.stub(urlUtils, 'getSubdir').returns('/blah'); + }); + + afterEach(function () { + sinon.restore(); + }); + + it('calls next if url does not include a token', async function () { + // This recreates what express does, note: members is mounted so path will be / + req.url = '/members'; + req.path = '/'; + req.query = {}; + + // Call the middleware + await membersMiddleware.createSessionFromMagicLink(req, res, next); + + // Check behaviour + next.calledOnce.should.be.true(); + next.firstCall.args.should.be.an.Array().with.lengthOf(0); + }); + + it('redirects correctly on success', async function () { + // This recreates what express does, note: members is mounted so path will be / + req.url = '/members?token=test&action=signup'; + req.path = '/'; + req.query = {token: 'test', action: 'signup'}; + + // Fake token handling success + membersService.ssr.exchangeTokenForSession.resolves(); + + // Call the middleware + await membersMiddleware.createSessionFromMagicLink(req, res, next); + + // Check behaviour + next.calledOnce.should.be.false(); + res.redirect.calledOnce.should.be.true(); + res.redirect.firstCall.args[0].should.eql('/blah/?action=signup&success=true'); + }); + + it('redirects correctly on failure', async function () { + // This recreates what express does, note: members is mounted so path will be / + req.url = '/members?token=test&action=signup'; + req.path = '/'; + req.query = {token: 'test', action: 'signup'}; + + // Fake token handling failure + membersService.ssr.exchangeTokenForSession.rejects(); + + // Call the middleware + await membersMiddleware.createSessionFromMagicLink(req, res, next); + + // Check behaviour + next.calledOnce.should.be.false(); + res.redirect.calledOnce.should.be.true(); + res.redirect.firstCall.args[0].should.eql('/blah/?action=signup&success=false'); + }); + }); +});