🐛 Fixed Offer Redemptions being over counted (#13988)

refs https://github.com/TryGhost/Team/issues/1257

Offer Redemptions were being overcounted due to the way we were updating
Stripe configuration for the Members service. We would create a new
instance of the members-api, which would have event handlers for
creating Offer Redemptions - by creating a new instance each time Stripe
config changed, we would overcount them.

Here we've pulled out Stripe related logic into the Stripe service, and
updated it internally - rather than creating a new instance. This means
that we've been able to remove all of the logic for re-instantiating the
members-api.

- Bumped members-api & stripe-service
- Removed reinstantiation of members-api
- Used stripe service to execute migrations
- Updated Stripe Service to handle webhooks & migrations
- Used webhook controller from stripe service
- Used disconnect method from stripe service
- Removed unused stripe dependency
- Removed Stripe webhook config from members-api
This commit is contained in:
Fabien 'egg' O'Carroll 2022-01-18 17:56:47 +02:00 committed by GitHub
parent eb68e8d339
commit a565da06b2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 199 additions and 164 deletions

View File

@ -6,6 +6,7 @@ const tpl = require('@tryghost/tpl');
const {BadRequestError} = require('@tryghost/errors');
const settingsService = require('../../services/settings');
const membersService = require('../../services/members');
const stripeService = require('../../services/stripe');
const settingsBREADService = settingsService.getSettingsBREADServiceInstance();
@ -132,7 +133,7 @@ module.exports = {
});
}
await membersService.api.disconnectStripe();
await stripeService.disconnect();
return models.Settings.edit([{
key: 'stripe_connect_publishable_key',

View File

@ -173,21 +173,6 @@ function createApiInstance(config) {
stripe: config.getStripePaymentConfig()
},
models: {
/**
* Settings do not have their own models, so we wrap the webhook in a "fake" model
*/
StripeWebhook: {
async upsert(data, options) {
const settings = [{
key: 'members_stripe_webhook_id',
value: data.webhook_id
}, {
key: 'members_stripe_webhook_secret',
value: data.secret
}];
await models.Settings.edit(settings, options);
}
},
StripeCustomer: models.MemberStripeCustomer,
StripeCustomerSubscription: models.StripeCustomerSubscription,
Member: models.Member,

View File

@ -137,8 +137,6 @@ class MembersConfigProvider {
getStripeUrlConfig() {
const siteUrl = this._urlUtils.getSiteUrl();
const webhookHandlerUrl = new URL('members/webhooks/stripe/', siteUrl);
const checkoutSuccessUrl = new URL(siteUrl);
checkoutSuccessUrl.searchParams.set('stripe', 'success');
const checkoutCancelUrl = new URL(siteUrl);
@ -153,8 +151,7 @@ class MembersConfigProvider {
checkoutSuccess: checkoutSuccessUrl.href,
checkoutCancel: checkoutCancelUrl.href,
billingSuccess: billingSuccessUrl.href,
billingCancel: billingCancelUrl.href,
webhookHandler: webhookHandlerUrl.href
billingCancel: billingCancelUrl.href
};
}
@ -175,11 +172,6 @@ class MembersConfigProvider {
checkoutCancelUrl: urls.checkoutCancel,
billingSuccessUrl: urls.billingSuccess,
billingCancelUrl: urls.billingCancel,
webhookHandlerUrl: urls.webhookHandler,
webhook: {
id: this._settingsCache.get('members_stripe_webhook_id'),
secret: this._settingsCache.get('members_stripe_webhook_secret')
},
product: {
name: this._settingsCache.get('stripe_product_name')
},

View File

@ -237,6 +237,5 @@ module.exports = {
getOfferData,
updateMemberData,
getMemberSiteData,
deleteSession,
stripeWebhooks: (req, res, next) => membersService.api.middleware.handleStripeWebhook(req, res, next)
deleteSession
};

View File

@ -5,7 +5,6 @@ const db = require('../../data/db');
const MembersConfigProvider = require('./config');
const MembersCSVImporter = require('@tryghost/members-importer');
const MembersStats = require('./stats/members-stats');
const createMembersApiInstance = require('./api');
const createMembersSettingsInstance = require('./settings');
const logging = require('@tryghost/logging');
const urlUtils = require('../../../shared/url-utils');
@ -16,7 +15,6 @@ const models = require('../../models');
const _ = require('lodash');
const {GhostMailer} = require('../mail');
const jobsService = require('../jobs');
const stripeService = require('../stripe');
const messages = {
noLiveKeysInDevelopment: 'Cannot use live stripe keys in development. Please restart in production mode.',
@ -26,9 +24,6 @@ const messages = {
emailVerificationEmailMessage: `Email verification needed for site: {siteUrl}, just imported: {importedNumber} members.`
};
// Bind to settings.edited to update systems based on settings changes, similar to the bridge and models/base/listeners
const events = require('../../lib/common/events');
const ghostMailer = new GhostMailer();
const membersConfig = new MembersConfigProvider({
@ -40,16 +35,6 @@ const membersConfig = new MembersConfigProvider({
let membersApi;
let membersSettings;
function reconfigureMembersAPI() {
const reconfiguredMembersAPI = createMembersApiInstance(membersConfig);
reconfiguredMembersAPI.bus.on('ready', function () {
membersApi = reconfiguredMembersAPI;
});
reconfiguredMembersAPI.bus.on('error', function (err) {
logging.error(err);
});
}
/**
* @description Calculates threshold based on following formula
* Threshold = max{[current number of members], [volume threshold]}
@ -57,7 +42,7 @@ function reconfigureMembersAPI() {
* @returns {Promise<number>}
*/
const fetchImportThreshold = async () => {
const membersTotal = await membersService.stats.getTotalMembers();
const membersTotal = await module.exports.stats.getTotalMembers();
const configThreshold = _.get(config.get('hostSettings'), 'emailVerification.importThreshold');
const volumeThreshold = (configThreshold === undefined) ? Infinity : configThreshold;
const threshold = Math.max(membersTotal, volumeThreshold);
@ -68,7 +53,7 @@ const fetchImportThreshold = async () => {
const membersImporter = new MembersCSVImporter({
storagePath: config.getContentPath('data'),
getTimezone: () => settingsCache.get('timezone'),
getMembersApi: () => membersService.api,
getMembersApi: () => module.exports.api,
sendEmail: ghostMailer.send.bind(ghostMailer),
isSet: labsService.isSet.bind(labsService),
addJob: jobsService.addJob.bind(jobsService),
@ -125,18 +110,14 @@ const processImport = async (options) => {
return result;
};
events.on('services.stripe.reconfigured', reconfigureMembersAPI);
const membersService = {
module.exports = {
async init() {
const stripeService = require('../stripe');
const createMembersApiInstance = require('./api');
const env = config.get('env');
// @TODO Move to stripe service
if (env !== 'production') {
if (!process.env.WEBHOOK_SECRET && stripeService.api.configured) {
process.env.WEBHOOK_SECRET = 'DEFAULT_WEBHOOK_SECRET';
logging.warn(tpl(messages.remoteWebhooksInDevelopment));
}
if (stripeService.api.configured && stripeService.api.mode === 'live') {
throw new errors.IncorrectUsageError({
message: tpl(messages.noLiveKeysInDevelopment)
@ -166,6 +147,8 @@ const membersService = {
logging.error(err);
}
})();
await stripeService.migrations.execute();
},
contentGating: require('./content-gating'),
@ -187,7 +170,7 @@ const membersService = {
cookieKeys: [settingsCache.get('theme_session_secret')],
cookieName: 'ghost-members-ssr',
cookieCacheName: 'ghost-members-ssr-cache',
getMembersApi: () => membersService.api
getMembersApi: () => module.exports.api
}),
stripeConnect: require('./stripe-connect'),
@ -199,7 +182,6 @@ const membersService = {
settingsCache: settingsCache,
isSQLite: config.get('database:client') === 'sqlite3'
})
};
module.exports = membersService;
};
module.exports.middleware = require('./middleware');

View File

@ -1,8 +1,6 @@
const DynamicRedirectManager = require('@tryghost/express-dynamic-redirects');
const OffersModule = require('@tryghost/members-offers');
const stripeService = require('../stripe');
const config = require('../../../shared/config');
const urlUtils = require('../../../shared/url-utils');
const models = require('../../models');
@ -19,8 +17,7 @@ module.exports = {
const offersModule = OffersModule.create({
OfferModel: models.Offer,
OfferRedemptionModel: models.OfferRedemption,
redirectManager: redirectManager,
stripeAPIService: stripeService.api
redirectManager: redirectManager
});
this.api = offersModule.api;

View File

@ -1,7 +1,13 @@
const ghostVersion = require('@tryghost/version');
const logging = require('@tryghost/logging');
const tpl = require('@tryghost/tpl');
const messages = {
remoteWebhooksInDevelopment: 'Cannot use remote webhooks in development. See https://ghost.org/docs/webhooks/#stripe-webhooks for developing with Stripe.'
};
// @TODO Refactor to a class w/ constructor
module.exports = {
getConfig(settings, config) {
getConfig(settings, config, urlUtils) {
/**
* @param {'direct' | 'connect'} type - The "type" of keys to fetch from settings
* @returns {{publicKey: string, secretKey: string} | null}
@ -42,16 +48,25 @@ module.exports = {
if (!keys) {
return null;
}
const env = config.get('env');
let webhookSecret = process.env.WEBHOOK_SECRET;
if (env !== 'production') {
if (!webhookSecret) {
webhookSecret = 'DEFAULT_WEBHOOK_SECRET';
logging.warn(tpl(messages.remoteWebhooksInDevelopment));
}
}
const webhookHandlerUrl = new URL('members/webhooks/stripe/', urlUtils.getSiteUrl());
return {
secretKey: keys.secretKey,
publicKey: keys.publicKey,
appInfo: {
name: 'Ghost',
partner_id: 'pp_partner_DKmRVtTs4j9pwZ',
version: ghostVersion.original,
url: 'https://ghost.org/'
},
enablePromoCodes: config.get('enableStripePromoCodes')
enablePromoCodes: config.get('enableStripePromoCodes'),
webhookSecret: webhookSecret,
webhookHandlerUrl: webhookHandlerUrl.href
};
}
};

View File

@ -1,45 +1,53 @@
const _ = require('lodash');
const StripeAPIService = require('@tryghost/members-stripe-service');
const StripeService = require('@tryghost/members-stripe-service');
const membersService = require('../members');
const config = require('../../../shared/config');
const settings = require('../../../shared/settings-cache');
const urlUtils = require('../../../shared/url-utils');
const events = require('../../lib/common/events');
const models = require('../../models');
const {getConfig} = require('./config');
const api = new StripeAPIService({
config: {}
});
const stripeKeySettings = [
'stripe_publishable_key',
'stripe_secret_key',
'stripe_connect_publishable_key',
'stripe_connect_secret_key'
];
function configureApi() {
const cfg = getConfig(settings, config);
const cfg = getConfig(settings, config, urlUtils);
if (cfg) {
api.configure(cfg);
module.exports.configure(cfg);
return true;
}
return false;
}
const debouncedConfigureApi = _.debounce(() => {
configureApi();
events.emit('services.stripe.reconfigured');
}, 600);
module.exports = {
async init() {
configureApi();
events.on('settings.edited', function (model) {
if (!stripeKeySettings.includes(model.get('key'))) {
return;
}
debouncedConfigureApi();
});
},
module.exports = new StripeService({
membersService,
models: _.pick(models, ['Product', 'StripePrice', 'StripeCustomerSubscription', 'StripeProduct', 'MemberStripeCustomer', 'Offer', 'Settings']),
StripeWebhook: {
async get() {
return {
webhook_id: settings.get('members_stripe_webhook_id'),
secret: settings.get('members_stripe_webhook_secret')
};
},
async save(data) {
await models.Settings.edit([{
key: 'members_stripe_webhook_id',
value: data.webhook_id
}, {
key: 'members_stripe_webhook_secret',
value: data.secret
}]);
}
}
});
api
module.exports.init = async function init() {
configureApi();
events.on('settings.edited', function (model) {
if (['stripe_publishable_key', 'stripe_secret_key', 'stripe_connect_publishable_key', 'stripe_connect_secret_key'].includes(model.get('key'))) {
debouncedConfigureApi();
}
});
};

View File

@ -6,6 +6,7 @@ const express = require('../../../shared/express');
const urlUtils = require('../../../shared/url-utils');
const sentry = require('../../../shared/sentry');
const membersService = require('../../services/members');
const stripeService = require('../../services/stripe');
const middleware = membersService.middleware;
const shared = require('../shared');
const labs = require('../../../shared/labs');
@ -28,7 +29,7 @@ module.exports = function setupMembersApp() {
// Routing
// Webhooks
membersApp.post('/webhooks/stripe', middleware.stripeWebhooks);
membersApp.post('/webhooks/stripe', bodyParser.raw({type: 'application/json'}), stripeService.webhookController.handle.bind(stripeService.webhookController));
// Initializes members specific routes as well as assigns members specific data to the req/res objects
// We don't want to add global bodyParser middleware as that interfers with stripe webhook requests on - `/webhooks`.

View File

@ -80,11 +80,12 @@
"@tryghost/limit-service": "1.0.8",
"@tryghost/logging": "2.0.1",
"@tryghost/magic-link": "1.0.15",
"@tryghost/members-api": "3.1.0",
"@tryghost/members-api": "4.0.1",
"@tryghost/members-csv": "1.2.2",
"@tryghost/members-importer": "0.4.0",
"@tryghost/members-offers": "0.10.4",
"@tryghost/members-ssr": "1.0.17",
"@tryghost/members-stripe-service": "0.6.1",
"@tryghost/metrics": "1.0.2",
"@tryghost/minifier": "0.1.9",
"@tryghost/mw-error-handler": "0.1.1",

View File

@ -14,7 +14,7 @@ describe('Stripe Service', function () {
this.clock.restore();
});
it('Emits a "services.stripe.reconfigured" event when it is reconfigured', async function () {
it('Does not emit a "services.stripe.reconfigured" event when it is reconfigured', async function () {
const eventsStub = new events.EventEmitter();
const configureApiStub = sinon.spy();
@ -33,6 +33,6 @@ describe('Stripe Service', function () {
this.clock.tick(600);
sinon.assert.callOrder(configureApiStub, emitReconfiguredEventSpy);
sinon.assert.notCalled(emitReconfiguredEventSpy);
});
});

View File

@ -72,7 +72,7 @@ describe('Members - config', function () {
configUtils.restore();
});
it('Includes the subdirectory in the webhookHandlerUrl', function () {
it('Does not export webhookHandlerUrl', function () {
configUtils.set({
stripeDirect: false,
url: 'http://site.com/subdir'
@ -90,6 +90,6 @@ describe('Members - config', function () {
const paymentConfig = membersConfig.getStripePaymentConfig();
should.equal(paymentConfig.webhookHandlerUrl, 'http://site.com/subdir/members/webhooks/stripe/');
should.not.exist(paymentConfig.webhookHandlerUrl);
});
});

View File

@ -1,85 +1,139 @@
const should = require('should');
const sinon = require('sinon');
const UrlUtils = require('@tryghost/url-utils');
const configUtils = require('../../../../utils/configUtils');
const {getConfig} = require('../../../../../core/server/services/stripe/config');
/**
* @param {object} options
* @param {boolean} options.setDirect - Whether the "direct" keys should be set
* @param {boolean} options.setConnect - Whether the connect_integration keys should be set
*/
function createSettingsMock({setDirect, setConnect}) {
const getStub = sinon.stub();
getStub.withArgs('members_from_address').returns('noreply');
getStub.withArgs('members_signup_access').returns('all');
getStub.withArgs('stripe_secret_key').returns(setDirect ? 'direct_secret' : null);
getStub.withArgs('stripe_publishable_key').returns(setDirect ? 'direct_publishable' : null);
getStub.withArgs('stripe_product_name').returns('Test');
getStub.withArgs('stripe_plans').returns([{
name: 'Monthly',
currency: 'usd',
interval: 'month',
amount: 1000
}, {
name: 'Yearly',
currency: 'usd',
interval: 'year',
amount: 10000
}]);
getStub.withArgs('stripe_connect_secret_key').returns(setConnect ? 'connect_secret' : null);
getStub.withArgs('stripe_connect_publishable_key').returns(setConnect ? 'connect_publishable' : null);
getStub.withArgs('stripe_connect_livemode').returns(true);
getStub.withArgs('stripe_connect_display_name').returns('Test');
getStub.withArgs('stripe_connect_account_id').returns('ac_XXXXXXXXXXXXX');
return {
get: getStub
};
}
function createUrlUtilsMock() {
return new UrlUtils({
getSubdir: configUtils.config.getSubdir,
getSiteUrl: configUtils.config.getSiteUrl,
getAdminUrl: configUtils.config.getAdminUrl,
apiVersions: {
all: ['v3'],
v3: {
admin: 'v3/admin',
content: 'v3/content'
}
},
defaultApiVersion: 'v3',
slugs: ['ghost', 'rss', 'amp'],
redirectCacheMaxAge: 31536000,
baseApiPath: '/ghost/api'
});
}
describe('Stripe - config', function () {
beforeEach(function () {
configUtils.set({
url: 'http://domain.tld/subdir',
admin: {url: 'http://sub.domain.tld'}
});
});
afterEach(function () {
configUtils.restore();
});
it('Uses direct keys when stripeDirect is true, regardles of which keys exist', function () {
const fakeSettings = {
get: sinon.stub()
};
const fakeConfig = {
get: sinon.stub()
};
const fakeSettings = createSettingsMock({setDirect: true, setConnect: true});
configUtils.set({
stripeDirect: true
});
const fakeUrlUtils = createUrlUtilsMock();
fakeSettings.get.withArgs('stripe_connect_secret_key').returns('connect_secret');
fakeSettings.get.withArgs('stripe_connect_publishable_key').returns('connect_publishable');
fakeSettings.get.withArgs('stripe_secret_key').returns('direct_secret');
fakeSettings.get.withArgs('stripe_publishable_key').returns('direct_publishable');
fakeConfig.get.withArgs('stripeDirect').returns(true);
const config = getConfig(fakeSettings, fakeConfig);
const config = getConfig(fakeSettings, configUtils.config, fakeUrlUtils);
should.equal(config.publicKey, 'direct_publishable');
should.equal(config.secretKey, 'direct_secret');
});
it('Does not use connect keys if stripeDirect is true, and the direct keys do not exist', function () {
const fakeSettings = {
get: sinon.stub()
};
const fakeConfig = {
get: sinon.stub()
};
const fakeSettings = createSettingsMock({setDirect: false, setConnect: true});
configUtils.set({
stripeDirect: true
});
const fakeUrlUtils = createUrlUtilsMock();
fakeSettings.get.withArgs('stripe_connect_secret_key').returns('connect_secret');
fakeSettings.get.withArgs('stripe_connect_publishable_key').returns('connect_publishable');
fakeSettings.get.withArgs('stripe_secret_key').returns(null);
fakeSettings.get.withArgs('stripe_publishable_key').returns(null);
fakeConfig.get.withArgs('stripeDirect').returns(true);
const config = getConfig(fakeSettings, fakeConfig);
const config = getConfig(fakeSettings, configUtils.config, fakeUrlUtils);
should.equal(config, null);
});
it('Uses connect keys when stripeDirect is false, and the connect keys exist', function () {
const fakeSettings = {
get: sinon.stub()
};
const fakeConfig = {
get: sinon.stub()
};
const fakeSettings = createSettingsMock({setDirect: true, setConnect: true});
configUtils.set({
stripeDirect: false
});
const fakeUrlUtils = createUrlUtilsMock();
fakeSettings.get.withArgs('stripe_connect_secret_key').returns('connect_secret');
fakeSettings.get.withArgs('stripe_connect_publishable_key').returns('connect_publishable');
fakeSettings.get.withArgs('stripe_secret_key').returns('direct_secret');
fakeSettings.get.withArgs('stripe_publishable_key').returns('direct_publishable');
fakeConfig.get.withArgs('stripeDirect').returns(false);
const config = getConfig(fakeSettings, fakeConfig);
const config = getConfig(fakeSettings, configUtils.config, fakeUrlUtils);
should.equal(config.publicKey, 'connect_publishable');
should.equal(config.secretKey, 'connect_secret');
});
it('Uses direct keys when stripeDirect is false, but the connect keys do not exist', function () {
const fakeSettings = {
get: sinon.stub()
};
const fakeConfig = {
get: sinon.stub()
};
const fakeSettings = createSettingsMock({setDirect: true, setConnect: false});
configUtils.set({
stripeDirect: false
});
const fakeUrlUtils = createUrlUtilsMock();
fakeSettings.get.withArgs('stripe_connect_secret_key').returns(null);
fakeSettings.get.withArgs('stripe_connect_publishable_key').returns(null);
fakeSettings.get.withArgs('stripe_secret_key').returns('direct_secret');
fakeSettings.get.withArgs('stripe_publishable_key').returns('direct_publishable');
fakeConfig.get.withArgs('stripeDirect').returns(false);
const config = getConfig(fakeSettings, fakeConfig);
const config = getConfig(fakeSettings, configUtils.config, fakeUrlUtils);
should.equal(config.publicKey, 'direct_publishable');
should.equal(config.secretKey, 'direct_secret');
});
it('Includes the subdirectory in the webhookHandlerUrl', function () {
configUtils.set({
stripeDirect: false,
url: 'http://site.com/subdir'
});
const fakeSettings = createSettingsMock({setDirect: true, setConnect: false});
const fakeUrlUtils = createUrlUtilsMock();
const config = getConfig(fakeSettings, configUtils.config, fakeUrlUtils);
should.equal(config.webhookHandlerUrl, 'http://site.com/subdir/members/webhooks/stripe/');
});
});

View File

@ -1600,10 +1600,10 @@
"@tryghost/domain-events" "^0.1.4"
"@tryghost/member-events" "^0.3.2"
"@tryghost/members-api@3.1.0":
version "3.1.0"
resolved "https://registry.yarnpkg.com/@tryghost/members-api/-/members-api-3.1.0.tgz#e87a0452dfe0242b16560a103e7a03230a108a2e"
integrity sha512-clUdblXWghXCYyYWzoKP55oj4dhkf+GuNuH2WQQ8bwVcUWnyIOWwFJOqkz7KGGToq7hyn0E80MUdwrKU1zYVAA==
"@tryghost/members-api@4.0.1":
version "4.0.1"
resolved "https://registry.yarnpkg.com/@tryghost/members-api/-/members-api-4.0.1.tgz#d272537874b4372df8dd1fc126bc7d0a768c3949"
integrity sha512-/wpozXUw2xJLIbG/GcZF62jkGY3oGjxuy1ORGmkH4Vuy/1+NV68Fxz99VnqfKPCNU/q63g7BMTG9iR63IM4sUQ==
dependencies:
"@tryghost/debug" "^0.1.2"
"@tryghost/domain-events" "^0.1.4"
@ -1614,7 +1614,7 @@
"@tryghost/member-events" "^0.3.2"
"@tryghost/members-analytics-ingress" "^0.1.6"
"@tryghost/members-payments" "^0.1.6"
"@tryghost/members-stripe-service" "^0.5.2"
"@tryghost/members-stripe-service" "^0.6.1"
"@tryghost/tpl" "^0.1.2"
"@types/jsonwebtoken" "^8.5.1"
bluebird "^3.5.4"
@ -1677,13 +1677,13 @@
jsonwebtoken "^8.5.1"
lodash "^4.17.11"
"@tryghost/members-stripe-service@^0.5.2":
version "0.5.2"
resolved "https://registry.yarnpkg.com/@tryghost/members-stripe-service/-/members-stripe-service-0.5.2.tgz#fd1e9ebc6cd561f928bd97d48b3f27c747e36412"
integrity sha512-zyfYTFfYrZ3dfx84twPwNqiyOJR2sC29YAKXCFW/TVvclYlnp3Z5sfewpmKprt1SDH8e45JSir3zC/2gjE/txQ==
"@tryghost/members-stripe-service@0.6.1", "@tryghost/members-stripe-service@^0.6.1":
version "0.6.1"
resolved "https://registry.yarnpkg.com/@tryghost/members-stripe-service/-/members-stripe-service-0.6.1.tgz#21785b52ae396386526855d2cae5bd121a13d36b"
integrity sha512-SboHA+ZmY/lvsdcvUB2WRzHCWr0wg81/8nPoaiO+S52Uiib3kHxAabfMXaTAkhdD1anLEJIbXTdv4O6KORK5LA==
dependencies:
"@tryghost/debug" "^0.1.4"
"@tryghost/errors" "^0.2.13"
"@tryghost/errors" "1.2.0"
leaky-bucket "^2.2.0"
stripe "^8.174.0"