Skip to content

Commit

Permalink
🐛 Fixed sending multiple support email verification emails (#15044)
Browse files Browse the repository at this point in the history 
refs https://github.com/TryGhost/Team/issues/1686

- When the settings are updated with the `members_support_address` key present, it would always send a verification email
- Root cause is that the service failed to check if the email was changed or not. Due to a bug it always thought the email was changed, triggering the verification flow.
- The admin app will always send all the settings keys when changing some other value. This causes a lot of email verification emails.
- Added tests and email count checks in tests
  • Loading branch information
@SimonBackx
SimonBackx committed Jul 19, 2022
1 parent d1f2040 commit 2e1a756
Show file tree
Hide file tree
Showing 3 changed files with 325 additions and 2 deletions.
2 changes: 1 addition & 1 deletion core/server/services/settings/settings-bread-service.js
View file
Expand Up @@ -297,7 +297,7 @@ class SettingsBREADService {
if (EMAIL_KEYS.includes(setting.key)) {
const email = setting.value;
const key = setting.key;
const hasChanged = getSetting(setting) !== email;
const hasChanged = getSetting(setting).value !== email;

if (await this.requiresEmailVerification({email, hasChanged})) {
emailsToVerify.push({email, key});
Expand Down
289 changes: 289 additions & 0 deletions test/e2e-api/admin/__snapshots__/settings.test.js.snap
View file
Expand Up @@ -916,6 +916,295 @@ Object {
}
`;
exports[`Settings API Edit does not trigger email verification flow if members_support_address remains the same 1: [body] 1`] = `
Object {
"meta": Object {},
"settings": Array [
Object {
"key": "title",
"value": "[]",
},
Object {
"key": "description",
"value": "Thoughts, stories and ideas",
},
Object {
"key": "logo",
"value": "",
},
Object {
"key": "cover_image",
"value": "https://static.ghost.org/v4.0.0/images/publication-cover.jpg",
},
Object {
"key": "icon",
"value": "http://127.0.0.1:2369/content/images/size/w256h256/2019/07/icon.png",
},
Object {
"key": "accent_color",
"value": "#FF1A75",
},
Object {
"key": "locale",
"value": "ua",
},
Object {
"key": "timezone",
"value": "Pacific/Auckland",
},
Object {
"key": "codeinjection_head",
"value": null,
},
Object {
"key": "codeinjection_foot",
"value": "",
},
Object {
"key": "facebook",
"value": "ghost",
},
Object {
"key": "twitter",
"value": "@ghost",
},
Object {
"key": "navigation",
"value": "[{\\"label\\":\\"label1\\"}]",
},
Object {
"key": "secondary_navigation",
"value": "[{\\"label\\":\\"Data & privacy\\",\\"url\\":\\"/privacy/\\"},{\\"label\\":\\"Contact\\",\\"url\\":\\"/contact/\\"},{\\"label\\":\\"Contribute →\\",\\"url\\":\\"/contribute/\\"}]",
},
Object {
"key": "meta_title",
"value": "SEO title",
},
Object {
"key": "meta_description",
"value": "SEO description",
},
Object {
"key": "og_image",
"value": "http://127.0.0.1:2369/content/images/2019/07/facebook.png",
},
Object {
"key": "og_title",
"value": "facebook title",
},
Object {
"key": "og_description",
"value": "facebook description",
},
Object {
"key": "twitter_image",
"value": "http://127.0.0.1:2369/content/images/2019/07/twitter.png",
},
Object {
"key": "twitter_title",
"value": "twitter title",
},
Object {
"key": "twitter_description",
"value": "twitter description",
},
Object {
"key": "active_theme",
"value": "casper",
},
Object {
"key": "is_private",
"value": false,
},
Object {
"key": "password",
"value": "",
},
Object {
"key": "public_hash",
"value": StringMatching /\\[a-z0-9\\]\\{30\\}/,
},
Object {
"key": "default_content_visibility",
"value": "public",
},
Object {
"key": "default_content_visibility_tiers",
"value": "[]",
},
Object {
"key": "members_signup_access",
"value": "all",
},
Object {
"key": "members_support_address",
"value": "support@example.com",
},
Object {
"key": "stripe_secret_key",
"value": null,
},
Object {
"key": "stripe_publishable_key",
"value": null,
},
Object {
"key": "stripe_plans",
"value": "[]",
},
Object {
"key": "stripe_connect_publishable_key",
"value": "pk_test_for_stripe",
},
Object {
"key": "stripe_connect_secret_key",
"value": "••••••••",
},
Object {
"key": "stripe_connect_livemode",
"value": null,
},
Object {
"key": "stripe_connect_display_name",
"value": null,
},
Object {
"key": "stripe_connect_account_id",
"value": null,
},
Object {
"key": "members_monthly_price_id",
"value": null,
},
Object {
"key": "members_yearly_price_id",
"value": null,
},
Object {
"key": "portal_name",
"value": true,
},
Object {
"key": "portal_button",
"value": true,
},
Object {
"key": "portal_plans",
"value": "[\\"free\\"]",
},
Object {
"key": "portal_products",
"value": "[]",
},
Object {
"key": "portal_button_style",
"value": "icon-and-text",
},
Object {
"key": "portal_button_icon",
"value": null,
},
Object {
"key": "portal_button_signup_text",
"value": "Subscribe",
},
Object {
"key": "mailgun_domain",
"value": null,
},
Object {
"key": "mailgun_api_key",
"value": null,
},
Object {
"key": "mailgun_base_url",
"value": null,
},
Object {
"key": "email_track_opens",
"value": true,
},
Object {
"key": "email_verification_required",
"value": false,
},
Object {
"key": "amp",
"value": false,
},
Object {
"key": "amp_gtag_id",
"value": null,
},
Object {
"key": "firstpromoter",
"value": false,
},
Object {
"key": "firstpromoter_id",
"value": null,
},
Object {
"key": "labs",
"value": "{\\"members\\":true}",
},
Object {
"key": "slack_url",
"value": "",
},
Object {
"key": "slack_username",
"value": "New Slack Username",
},
Object {
"key": "unsplash",
"value": false,
},
Object {
"key": "shared_views",
"value": "[]",
},
Object {
"key": "editor_default_email_recipients",
"value": "visibility",
},
Object {
"key": "editor_default_email_recipients_filter",
"value": "all",
},
Object {
"key": "members_enabled",
"value": true,
},
Object {
"key": "members_invite_only",
"value": false,
},
Object {
"key": "paid_members_enabled",
"value": true,
},
Object {
"key": "firstpromoter_account",
"value": null,
},
],
}
`;
exports[`Settings API Edit does not trigger email verification flow if members_support_address remains the same 2: [headers] 1`] = `
Object {
"access-control-allow-origin": "http://127.0.0.1:2369",
"cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
"content-length": "3376",
"content-type": "application/json; charset=utf-8",
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
"vary": "Origin, Accept-Encoding",
"x-cache-invalidate": "/*",
"x-powered-by": "Express",
}
`;
exports[`Settings API Edit editing members_support_address triggers email verification flow 1: [body] 1`] = `
Object {
"meta": Object {
Expand Down
36 changes: 35 additions & 1 deletion test/e2e-api/admin/settings.test.js
View file
Expand Up @@ -167,6 +167,8 @@ describe('Settings API', function () {
.matchHeaderSnapshot({
etag: anyEtag
});

mockManager.assert.sentEmailCount(0);
});

it('removes image size prefixes when setting the icon', async function () {
Expand Down Expand Up @@ -197,6 +199,8 @@ describe('Settings API', function () {
// Check if not changed (also check internal ones)
const afterValue = settingsCache.get('icon');
assert.equal(afterValue, 'http://127.0.0.1:2369/content/images/2019/07/icon.png');

mockManager.assert.sentEmailCount(0);
});

it('cannot edit uneditable settings', async function () {
Expand All @@ -215,6 +219,7 @@ describe('Settings API', function () {
const emailVerificationRequired = body.settings.find(setting => setting.key === 'email_verification_required');
assert.strictEqual(emailVerificationRequired.value, false);
});
mockManager.assert.sentEmailCount(0);
});

it('editing members_support_address triggers email verification flow', async function () {
Expand All @@ -237,12 +242,40 @@ describe('Settings API', function () {
sent_email_verification: ['members_support_address']
});
});


mockManager.assert.sentEmailCount(1);
mockManager.assert.sentEmail({
subject: 'Verify email address',
to: 'support@example.com'
});
});

it('does not trigger email verification flow if members_support_address remains the same', async function () {
await models.Settings.edit({
key: 'members_support_address',
value: 'support@example.com'
});

await agent.put('settings/')
.body({
settings: [{key: 'members_support_address', value: 'support@example.com'}]
})
.expectStatus(200)
.matchBodySnapshot({
settings: matchSettingsArray(CURRENT_SETTINGS_COUNT)
})
.matchHeaderSnapshot({
etag: anyEtag
})
.expect(({body}) => {
const membersSupportAddress = body.settings.find(setting => setting.key === 'members_support_address');
assert.strictEqual(membersSupportAddress.value, 'support@example.com');

assert.deepEqual(body.meta, {});
});

mockManager.assert.sentEmailCount(0);
});
});

describe('verify key update', function () {
Expand All @@ -263,6 +296,7 @@ describe('Settings API', function () {
const membersSupportAddress = body.settings.find(setting => setting.key === 'members_support_address');
assert.strictEqual(membersSupportAddress.value, 'support@example.com');
});
mockManager.assert.sentEmailCount(0);
});

it('cannot update invalid keys via token', async function () {
Expand Down

0 comments on commit 2e1a756

Please sign in to comment.