From 72cc285184738b4933db8ae1a36951e68f078e04 Mon Sep 17 00:00:00 2001 From: Michael Barrett Date: Fri, 8 Sep 2023 13:55:02 +0100 Subject: [PATCH] Refactor validating specified newsletters in custom sign-up form (#18032) refs https://github.com/TryGhost/Product/issues/3837 Moved the logic for validating specified newsletters to controller so that the request can be failed --- .../lib/controllers/RouterController.js | 35 +++- ghost/members-api/lib/members-api.js | 3 +- .../lib/repositories/MemberRepository.js | 20 +-- .../test/unit/lib/controllers/router.test.js | 151 ++++++++++++++++++ .../test/unit/lib/repositories/member.test.js | 146 ----------------- 5 files changed, 186 insertions(+), 169 deletions(-) diff --git a/ghost/members-api/lib/controllers/RouterController.js b/ghost/members-api/lib/controllers/RouterController.js index e27818e51c..a06457f30d 100644 --- a/ghost/members-api/lib/controllers/RouterController.js +++ b/ghost/members-api/lib/controllers/RouterController.js @@ -16,7 +16,8 @@ const messages = { memberNotFound: 'No member exists with this e-mail address.', memberNotFoundSignUp: 'No member exists with this e-mail address. Please sign up first.', invalidType: 'Invalid checkout type.', - notConfigured: 'This site is not accepting payments at the moment.' + notConfigured: 'This site is not accepting payments at the moment.', + invalidNewsletterId: 'Cannot subscribe to invalid newsletter {ids}' }; module.exports = class RouterController { @@ -35,6 +36,7 @@ module.exports = class RouterController { * @param {any} deps.tokenService * @param {any} deps.sendEmailWithMagicLink * @param {{isSet(name: string): boolean}} deps.labsService + * @param {any} deps.newslettersService */ constructor({ offersAPI, @@ -48,7 +50,8 @@ module.exports = class RouterController { tokenService, memberAttributionService, sendEmailWithMagicLink, - labsService + labsService, + newslettersService }) { this._offersAPI = offersAPI; this._paymentsService = paymentsService; @@ -62,6 +65,7 @@ module.exports = class RouterController { this._sendEmailWithMagicLink = sendEmailWithMagicLink; this._memberAttributionService = memberAttributionService; this.labsService = labsService; + this._newslettersService = newslettersService; } async ensureStripe(_req, res, next) { @@ -476,12 +480,37 @@ module.exports = class RouterController { }); } + // Validate requested newsletters + let {newsletters: requestedNewsletters} = req.body; + + if (requestedNewsletters && requestedNewsletters.length > 0) { + const newsletterIds = requestedNewsletters.map(newsletter => newsletter.id); + const newsletters = await this._newslettersService.browse({ + filter: `id:[${newsletterIds}]`, + columns: ['id','status'] + }); + + if (newsletters.length !== newsletterIds.length) { + const validNewsletterIds = newsletters.map(newsletter => newsletter.id); + const invalidNewsletterIds = newsletterIds.filter(id => !validNewsletterIds.includes(id)); + + throw new errors.BadRequestError({ + message: tpl(messages.invalidNewsletterId, {ids: invalidNewsletterIds}) + }); + } + + requestedNewsletters = newsletters + .filter(newsletter => newsletter.status === 'active') + .map(newsletter => ({id: newsletter.id})); + } + // Someone tries to signup with a user that already exists // -> doesn't really matter: we'll send a login link - const tokenData = _.pick(req.body, ['labels', 'name', 'newsletters']); + const tokenData = _.pick(req.body, ['labels', 'name']); if (req.ip) { tokenData.reqIp = req.ip; } + tokenData.newsletters = requestedNewsletters; // Save attribution data in the tokenData tokenData.attribution = await this._memberAttributionService.getAttribution(req.body.urlHistory); diff --git a/ghost/members-api/lib/members-api.js b/ghost/members-api/lib/members-api.js index 00715695db..d187eda7b7 100644 --- a/ghost/members-api/lib/members-api.js +++ b/ghost/members-api/lib/members-api.js @@ -188,7 +188,8 @@ module.exports = function MembersAPI({ tokenService, sendEmailWithMagicLink, memberAttributionService, - labsService + labsService, + newslettersService }); const wellKnownController = new WellKnownController({ diff --git a/ghost/members-api/lib/repositories/MemberRepository.js b/ghost/members-api/lib/repositories/MemberRepository.js index 0065f36658..aa8d90797e 100644 --- a/ghost/members-api/lib/repositories/MemberRepository.js +++ b/ghost/members-api/lib/repositories/MemberRepository.js @@ -18,8 +18,7 @@ const messages = { productNotFound: 'Could not find Product {id}', bulkActionRequiresFilter: 'Cannot perform {action} without a filter or all=true', tierArchived: 'Cannot use archived Tiers', - invalidEmail: 'Invalid Email', - invalidNewsletterId: 'Cannot subscribe to invalid newsletter {ids}' + invalidEmail: 'Invalid Email' }; /** @@ -282,23 +281,6 @@ module.exports = class MemberRepository { memberStatusData.status = 'comped'; } - //checks for custom signUp forms - if (memberData.newsletters && memberData.newsletters.length > 0) { - const newsletterIds = memberData.newsletters.map(newsletter => newsletter.id); - const newsletters = await this._newslettersService.browse({ - filter: `id:[${newsletterIds}]`, - columns: ['id','status'] - }); - if (newsletters.length !== newsletterIds.length) { - const validNewsletterIds = newsletters.map(newsletter => newsletter.id); - const invalidIds = newsletterIds.filter(id => !validNewsletterIds.includes(id)); - throw new errors.BadRequestError({message: tpl(messages.invalidNewsletterId, {ids: invalidIds})}); - } - memberData.newsletters = newsletters - .filter(newsletter => newsletter.status === 'active') - .map(newsletter => ({id: newsletter.id})); - } - // Subscribe members to default newsletters if (memberData.subscribed !== false && !memberData.newsletters) { const browseOptions = _.pick(options, 'transacting'); diff --git a/ghost/members-api/test/unit/lib/controllers/router.test.js b/ghost/members-api/test/unit/lib/controllers/router.test.js index 21523e8b59..fa28b43794 100644 --- a/ghost/members-api/test/unit/lib/controllers/router.test.js +++ b/ghost/members-api/test/unit/lib/controllers/router.test.js @@ -90,4 +90,155 @@ describe('RouterController', function () { sinon.restore(); }); }); + + describe('sendMagicLink', function () { + describe('newsletters', function () { + let req, res, sendEmailWithMagicLinkStub; + + const createRouterController = (deps = {}) => { + return new RouterController({ + allowSelfSignup: sinon.stub().returns(true), + memberAttributionService: { + getAttribution: sinon.stub().resolves({}) + }, + sendEmailWithMagicLink: sendEmailWithMagicLinkStub, + ...deps + }); + }; + + beforeEach(function () { + req = { + body: { + email: 'jamie@example.com', + emailType: 'signup' + }, + get: sinon.stub() + }; + res = { + writeHead: sinon.stub(), + + end: sinon.stub() + }; + sendEmailWithMagicLinkStub = sinon.stub().resolves(); + }); + + it('adds specified newsletters to the tokenData', async function () { + const newsletters = [ + { + id: 'abc123', + status: 'active' + }, + { + id: 'def456', + status: 'active' + }, + { + id: 'ghi789', + status: 'active' + } + ]; + + req.body.newsletters = newsletters.map(newsletter => ({id: newsletter.id})); + + const newsletterIds = newsletters.map(newsletter => newsletter.id); + const newslettersServiceStub = { + browse: sinon.stub() + }; + + newslettersServiceStub.browse + .withArgs({ + filter: `id:[${newsletterIds}]`, + columns: ['id', 'status'] + }) + .resolves(newsletters); + + const controller = createRouterController({ + newslettersService: newslettersServiceStub + }); + + await controller.sendMagicLink(req, res); + + res.writeHead.calledOnceWith(201).should.be.true(); + res.end.calledOnceWith('Created.').should.be.true(); + + sendEmailWithMagicLinkStub.calledOnce.should.be.true(); + sendEmailWithMagicLinkStub.args[0][0].tokenData.newsletters.should.eql([ + {id: newsletters[0].id}, + {id: newsletters[1].id}, + {id: newsletters[2].id} + ]); + }); + + it('validates specified newsletters', async function () { + const INVALID_NEWSLETTER_ID = 'abc123'; + + req.body.newsletters = [ + {id: INVALID_NEWSLETTER_ID} + ]; + + const newslettersServiceStub = { + browse: sinon.stub() + }; + + newslettersServiceStub.browse + .withArgs({ + filter: `id:[${INVALID_NEWSLETTER_ID}]`, + columns: ['id', 'status'] + }) + .resolves([]); + + const controller = createRouterController({ + newslettersService: newslettersServiceStub + }); + + await controller.sendMagicLink(req, res).should.be.rejectedWith(`Cannot subscribe to invalid newsletter ${INVALID_NEWSLETTER_ID}`); + }); + + it('does not add specified newsletters to the tokenData if they are archived', async function () { + const newsletters = [ + { + id: 'abc123', + status: 'active' + }, + { + id: 'def456', + status: 'archived' + }, + { + id: 'ghi789', + status: 'active' + } + ]; + + req.body.newsletters = newsletters.map(newsletter => ({id: newsletter.id})); + + const newsletterIds = newsletters.map(newsletter => newsletter.id); + const newslettersServiceStub = { + browse: sinon.stub() + }; + + newslettersServiceStub.browse + .withArgs({ + filter: `id:[${newsletterIds}]`, + columns: ['id', 'status'] + }) + .resolves(newsletters); + + const controller = createRouterController({ + newslettersService: newslettersServiceStub + }); + + await controller.sendMagicLink(req, res); + + res.writeHead.calledOnceWith(201).should.be.true(); + res.end.calledOnceWith('Created.').should.be.true(); + + sendEmailWithMagicLinkStub.calledOnce.should.be.true(); + sendEmailWithMagicLinkStub.args[0][0].tokenData.newsletters.should.eql([ + {id: newsletters[0].id}, + {id: newsletters[2].id} + ]); + }); + }); + }); }); diff --git a/ghost/members-api/test/unit/lib/repositories/member.test.js b/ghost/members-api/test/unit/lib/repositories/member.test.js index 37d4e0a4ca..0baf70977d 100644 --- a/ghost/members-api/test/unit/lib/repositories/member.test.js +++ b/ghost/members-api/test/unit/lib/repositories/member.test.js @@ -310,150 +310,4 @@ describe('MemberRepository', function () { })).should.be.true(); }); }); - - describe('create', function () { - let memberStub, memberModelStub, newslettersServiceStub; - - beforeEach(function () { - memberStub = { - get: sinon.stub(), - related: sinon.stub() - }; - - memberStub.related - .withArgs('products').returns({ - models: [] - }) - .withArgs('newsletters').returns({ - models: [] - }); - - memberModelStub = { - add: sinon.stub().resolves(memberStub) - }; - - newslettersServiceStub = { - browse: sinon.stub() - }; - }); - - it('subscribes a member to the specified newsletters', async function () { - const newsletters = [{ - id: 'abc123', - status: 'active' - }, - { - id: 'def456', - status: 'active' - }, - { - id: 'ghi789', - status: 'active' - }]; - - const newsletterIds = newsletters.map(newsletter => newsletter.id); - newslettersServiceStub.browse - .withArgs({ - filter: `id:[${newsletterIds}]`, - columns: ['id', 'status'] - }) - .resolves(newsletters); - - const repo = new MemberRepository({ - Member: memberModelStub, - MemberStatusEvent: { - add: sinon.stub().resolves() - }, - MemberSubscribeEvent: { - add: sinon.stub().resolves() - }, - newslettersService: newslettersServiceStub - }); - - await repo.create({ - email: 'jamie@example.com', - email_disabled: false, - newsletters: [ - {id: newsletters[0].id}, - {id: newsletters[1].id}, - {id: newsletters[2].id} - ] - }); - - newslettersServiceStub.browse.calledOnce.should.be.true(); - memberModelStub.add.calledOnce.should.be.true(); - memberModelStub.add.args[0][0].newsletters.should.eql([ - {id: newsletters[0].id}, - {id: newsletters[1].id}, - {id: newsletters[2].id} - ]); - }); - - it('does not allow a member to be subscribed to an invalid newsletter', async function () { - const INVALID_NEWSLETTER_ID = 'abc123'; - - newslettersServiceStub.browse - .withArgs({ - filter: `id:[${INVALID_NEWSLETTER_ID}]`, - columns: ['id', 'status'] - }) - .resolves([]); - - const repo = new MemberRepository({ - Member: memberModelStub, - MemberStatusEvent: { - add: sinon.stub().resolves() - }, - MemberSubscribeEvent: { - add: sinon.stub().resolves() - }, - newslettersService: newslettersServiceStub - }); - - await repo.create({ - email: 'jamie@example.com', - email_disabled: false, - newsletters: [ - {id: INVALID_NEWSLETTER_ID} - ] - }).should.be.rejectedWith(`Cannot subscribe to invalid newsletter ${INVALID_NEWSLETTER_ID}`); - }); - - it('does not subscribe a member to an archived newsletter', async function () { - const newsletter = { - id: 'abc123', - status: 'archived' - }; - - newslettersServiceStub.browse - .withArgs({ - filter: `id:[${newsletter.id}]`, - columns: ['id', 'status'] - }) - .resolves([newsletter]); - - const repo = new MemberRepository({ - Member: memberModelStub, - MemberStatusEvent: { - add: sinon.stub().resolves() - }, - MemberSubscribeEvent: { - add: sinon.stub().resolves() - }, - newslettersService: newslettersServiceStub - }); - - await repo.create({ - email: 'jamie@example.com', - email_disabled: false, - newsletters: [ - {id: newsletter.id} - ] - }); - - newslettersServiceStub.browse.calledOnce.should.be.true(); - memberModelStub.add.calledOnce.should.be.true(); - memberModelStub.add.args[0][0].newsletters.should.eql([]); - }); - }); });