From 0b307bc22f19b8eca614f0dd8c760c32b5011133 Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Tue, 23 Feb 2016 21:05:27 -0500 Subject: [PATCH] Improves AdapterLoader, enforces configuraiton on Adapters --- spec/AdapterLoader.spec.js | 27 ++++++++-- spec/MockAdapter.js | 6 ++- spec/MockEmailAdapterWithOptions.js | 3 +- spec/OneSignalPushAdapter.spec.js | 33 ++++++++----- spec/ParseUser.spec.js | 15 ++++-- src/Adapters/AdapterLoader.js | 57 ++++++++++++---------- src/Adapters/Email/MailAdapter.js | 6 +++ src/Adapters/Email/SimpleMailgunAdapter.js | 15 ++++-- src/Adapters/Files/S3Adapter.js | 31 +++++++++--- src/Adapters/Logger/FileLoggerAdapter.js | 7 ++- src/Adapters/Push/OneSignalPushAdapter.js | 4 ++ src/Adapters/loadAdapter.js | 25 ---------- src/Config.js | 2 +- src/Controllers/AdaptableController.js | 1 - src/Controllers/MailController.js | 29 +++++++++++ src/Routers/UsersRouter.js | 10 +--- src/index.js | 14 ++---- 17 files changed, 176 insertions(+), 109 deletions(-) create mode 100644 src/Adapters/Email/MailAdapter.js delete mode 100644 src/Adapters/loadAdapter.js create mode 100644 src/Controllers/MailController.js diff --git a/spec/AdapterLoader.spec.js b/spec/AdapterLoader.spec.js index 80f30d6f..f32867e0 100644 --- a/spec/AdapterLoader.spec.js +++ b/spec/AdapterLoader.spec.js @@ -2,15 +2,17 @@ var loadAdapter = require("../src/Adapters/AdapterLoader").loadAdapter; var FilesAdapter = require("../src/Adapters/Files/FilesAdapter").default; -describe("AdaptableController", ()=>{ +describe("AdapterLoader", ()=>{ it("should instantiate an adapter from string in object", (done) => { var adapterPath = require('path').resolve("./spec/MockAdapter"); var adapter = loadAdapter({ adapter: adapterPath, - key: "value", - foo: "bar" + options: { + key: "value", + foo: "bar" + } }); expect(adapter instanceof Object).toBe(true); @@ -24,7 +26,6 @@ describe("AdaptableController", ()=>{ var adapter = loadAdapter(adapterPath); expect(adapter instanceof Object).toBe(true); - expect(adapter.options).toBe(adapterPath); done(); }); @@ -65,4 +66,22 @@ describe("AdaptableController", ()=>{ expect(adapter).toBe(originalAdapter); done(); }); + + it("should fail loading an improperly configured adapter", (done) => { + var Adapter = function(options) { + if (!options.foo) { + throw "foo is required for that adapter"; + } + } + var adapterOptions = { + param: "key", + doSomething: function() {} + }; + + expect(() => { + var adapter = loadAdapter(adapterOptions, Adapter); + expect(adapter).toEqual(adapterOptions); + }).not.toThrow("foo is required for that adapter"); + done(); + }); }); diff --git a/spec/MockAdapter.js b/spec/MockAdapter.js index 60d8ef86..c3f55784 100644 --- a/spec/MockAdapter.js +++ b/spec/MockAdapter.js @@ -1,3 +1,5 @@ module.exports = function(options) { - this.options = options; -} + return { + options: options + }; +}; diff --git a/spec/MockEmailAdapterWithOptions.js b/spec/MockEmailAdapterWithOptions.js index fe402e06..d5b6141a 100644 --- a/spec/MockEmailAdapterWithOptions.js +++ b/spec/MockEmailAdapterWithOptions.js @@ -3,6 +3,7 @@ module.exports = options => { throw "Options were not provided" } return { - sendVerificationEmail: () => Promise.resolve() + sendVerificationEmail: () => Promise.resolve(), + sendMail: () => Promise.resolve() } } diff --git a/spec/OneSignalPushAdapter.spec.js b/spec/OneSignalPushAdapter.spec.js index 2c165c45..f3ae2cdb 100644 --- a/spec/OneSignalPushAdapter.spec.js +++ b/spec/OneSignalPushAdapter.spec.js @@ -1,13 +1,15 @@ var OneSignalPushAdapter = require('../src/Adapters/Push/OneSignalPushAdapter'); var classifyInstallations = require('../src/Adapters/Push/PushAdapterUtils').classifyInstallations; + +// Make mock config +var pushConfig = { + oneSignalAppId:"APP ID", + oneSignalApiKey:"API KEY" +}; + describe('OneSignalPushAdapter', () => { it('can be initialized', (done) => { - // Make mock config - var pushConfig = { - oneSignalAppId:"APP ID", - oneSignalApiKey:"API KEY" - }; var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); @@ -17,9 +19,17 @@ describe('OneSignalPushAdapter', () => { expect(senderMap.android instanceof Function).toBe(true); done(); }); + + it('cannt be initialized if options are missing', (done) => { + + expect(() => { + new OneSignalPushAdapter(); + }).toThrow("Trying to initialiazed OneSignalPushAdapter without oneSignalAppId or oneSignalApiKey"); + done(); + }); it('can get valid push types', (done) => { - var oneSignalPushAdapter = new OneSignalPushAdapter(); + var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); expect(oneSignalPushAdapter.getValidPushTypes()).toEqual(['ios', 'android']); done(); @@ -56,7 +66,7 @@ describe('OneSignalPushAdapter', () => { it('can send push notifications', (done) => { - var oneSignalPushAdapter = new OneSignalPushAdapter(); + var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); // Mock android ios senders var androidSender = jasmine.createSpy('send') @@ -108,7 +118,7 @@ describe('OneSignalPushAdapter', () => { }); it("can send iOS notifications", (done) => { - var oneSignalPushAdapter = new OneSignalPushAdapter(); + var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); var sendToOneSignal = jasmine.createSpy('sendToOneSignal'); oneSignalPushAdapter.sendToOneSignal = sendToOneSignal; @@ -135,7 +145,7 @@ describe('OneSignalPushAdapter', () => { }); it("can send Android notifications", (done) => { - var oneSignalPushAdapter = new OneSignalPushAdapter(); + var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); var sendToOneSignal = jasmine.createSpy('sendToOneSignal'); oneSignalPushAdapter.sendToOneSignal = sendToOneSignal; @@ -157,10 +167,7 @@ describe('OneSignalPushAdapter', () => { }); it("can post the correct data", (done) => { - var pushConfig = { - oneSignalAppId:"APP ID", - oneSignalApiKey:"API KEY" - }; + var oneSignalPushAdapter = new OneSignalPushAdapter(pushConfig); var write = jasmine.createSpy('write'); diff --git a/spec/ParseUser.spec.js b/spec/ParseUser.spec.js index 23d41fdd..64477074 100644 --- a/spec/ParseUser.spec.js +++ b/spec/ParseUser.spec.js @@ -51,7 +51,8 @@ describe('Parse.User testing', () => { it('sends verification email if email verification is enabled', done => { var emailAdapter = { - sendVerificationEmail: () => Promise.resolve() + sendVerificationEmail: () => Promise.resolve(), + sendMail: () => Promise.resolve() } setServerConfiguration({ serverURL: 'http://localhost:8378/1', @@ -89,7 +90,8 @@ describe('Parse.User testing', () => { it('does not send verification email if email verification is disabled', done => { var emailAdapter = { - sendVerificationEmail: () => Promise.resolve() + sendVerificationEmail: () => Promise.resolve(), + sendMail: () => Promise.resolve() } setServerConfiguration({ serverURL: 'http://localhost:8378/1', @@ -131,7 +133,8 @@ describe('Parse.User testing', () => { expect(options.appName).toEqual('emailing app'); expect(options.user.get('email')).toEqual('user@parse.com'); done(); - } + }, + sendMail: () => {} } setServerConfiguration({ serverURL: 'http://localhost:8378/1', @@ -175,7 +178,8 @@ describe('Parse.User testing', () => { done(); }); }); - } + }, + sendMail: () => {} } setServerConfiguration({ serverURL: 'http://localhost:8378/1', @@ -232,7 +236,8 @@ describe('Parse.User testing', () => { done(); }); }); - } + }, + sendMail: () => {} } setServerConfiguration({ serverURL: 'http://localhost:8378/1', diff --git a/src/Adapters/AdapterLoader.js b/src/Adapters/AdapterLoader.js index 1557324b..5b46f22d 100644 --- a/src/Adapters/AdapterLoader.js +++ b/src/Adapters/AdapterLoader.js @@ -1,36 +1,43 @@ -export function loadAdapter(options, defaultAdapter) { - let adapter; +export function loadAdapter(adapter, defaultAdapter, options) { - // We have options and options have adapter key - if (options) { - // Pass an adapter as a module name, a function or an instance - if (typeof options == "string" || typeof options == "function" || options.constructor != Object) { - adapter = options; + if (!adapter) + { + if (!defaultAdapter) { + return options; } - if (options.adapter) { - adapter = options.adapter; + // Load from the default adapter when no adapter is set + return loadAdapter(defaultAdapter, undefined, options); + } else if (typeof adapter === "function") { + try { + return adapter(options); + } catch(e) { + var Adapter = adapter; + return new Adapter(options); } - } - - if (!adapter) { - adapter = defaultAdapter; - } - - // This is a string, require the module - if (typeof adapter === "string") { + } else if (typeof adapter === "string") { adapter = require(adapter); // If it's define as a module, get the default if (adapter.default) { adapter = adapter.default; } + + return loadAdapter(adapter, undefined, options); + } else if (adapter.module) { + return loadAdapter(adapter.module, undefined, adapter.options); + } else if (adapter.class) { + return loadAdapter(adapter.class, undefined, adapter.options); + } else if (adapter.adapter) { + return loadAdapter(adapter.adapter, undefined, adapter.options); + } else { + // Try to load the defaultAdapter with the options + // The default adapter should throw if the options are + // incompatible + try { + return loadAdapter(defaultAdapter, undefined, adapter); + } catch (e) {}; } - // From there it's either a function or an object - // if it's an function, instanciate and pass the options - if (typeof adapter === "function") { - var Adapter = adapter; - adapter = new Adapter(options); - } - return adapter; + // return the adapter as is as it's unusable otherwise + return adapter; } -module.exports = { loadAdapter } +export default loadAdapter; diff --git a/src/Adapters/Email/MailAdapter.js b/src/Adapters/Email/MailAdapter.js new file mode 100644 index 00000000..ceccf931 --- /dev/null +++ b/src/Adapters/Email/MailAdapter.js @@ -0,0 +1,6 @@ +export class MailAdapter { + sendVerificationEmail(options) {} + sendMail(options) {} +} + +export default MailAdapter; diff --git a/src/Adapters/Email/SimpleMailgunAdapter.js b/src/Adapters/Email/SimpleMailgunAdapter.js index 2d51173d..f2460182 100644 --- a/src/Adapters/Email/SimpleMailgunAdapter.js +++ b/src/Adapters/Email/SimpleMailgunAdapter.js @@ -6,7 +6,7 @@ let SimpleMailgunAdapter = mailgunOptions => { } let mailgun = Mailgun(mailgunOptions); - let sendMail = (to, subject, text) => { + let sendMail = ({to, subject, text}) => { let data = { from: mailgunOptions.fromAddress, to: to, @@ -24,16 +24,21 @@ let SimpleMailgunAdapter = mailgunOptions => { }); } - return { + return Object.freeze({ sendVerificationEmail: ({ link, user, appName, }) => { let verifyMessage = "Hi,\n\n" + "You are being asked to confirm the e-mail address " + user.email + " with " + appName + "\n\n" + "" + "Click here to confirm it:\n" + link; - return sendMail(user.email, 'Please verify your e-mail for ' + appName, verifyMessage); - } - } + return sendMail({ + to:user.email, + subject: 'Please verify your e-mail for ' + appName, + text: verifyMessage + }); + }, + sendMail: sendMail + }); } module.exports = SimpleMailgunAdapter diff --git a/src/Adapters/Files/S3Adapter.js b/src/Adapters/Files/S3Adapter.js index 0732fbfe..d63880f4 100644 --- a/src/Adapters/Files/S3Adapter.js +++ b/src/Adapters/Files/S3Adapter.js @@ -4,23 +4,38 @@ import * as AWS from 'aws-sdk'; import { FilesAdapter } from './FilesAdapter'; +import requiredParameter from '../../requiredParameter'; const DEFAULT_S3_REGION = "us-east-1"; +function parseS3AdapterOptions(...options) { + if (options.length === 1 && typeof options[0] == "object") { + return options; + } + + const additionalOptions = options[3] || {}; + + return { + accessKey: options[0], + secretKey: options[1], + bucket: options[2], + region: additionalOptions.region + } +} + export class S3Adapter extends FilesAdapter { // Creates an S3 session. // Providing AWS access and secret keys is mandatory // Region and bucket will use sane defaults if omitted constructor( - accessKey, - secretKey, - bucket, - { region = DEFAULT_S3_REGION, - bucketPrefix = '', - directAccess = false } = {} - ) { + accessKey = requiredParameter('S3Adapter requires an accessKey'), + secretKey = requiredParameter('S3Adapter requires a secretKey'), + bucket, + { region = DEFAULT_S3_REGION, + bucketPrefix = '', + directAccess = false } = {}) { super(); - + this._region = region; this._bucket = bucket; this._bucketPrefix = bucketPrefix; diff --git a/src/Adapters/Logger/FileLoggerAdapter.js b/src/Adapters/Logger/FileLoggerAdapter.js index 9e308242..5c8bd495 100644 --- a/src/Adapters/Logger/FileLoggerAdapter.js +++ b/src/Adapters/Logger/FileLoggerAdapter.js @@ -99,9 +99,12 @@ let _verifyTransports = ({infoLogger, errorLogger, logsFolder}) => { } export class FileLoggerAdapter extends LoggerAdapter { - constructor(options = {}) { + constructor(options) { super(); - + if (options && !options.logsFolder) { + throw "FileLoggerAdapter requires logsFolder"; + } + options = options || {}; this._logsFolder = options.logsFolder || LOGS_FOLDER; // check logs folder exists diff --git a/src/Adapters/Push/OneSignalPushAdapter.js b/src/Adapters/Push/OneSignalPushAdapter.js index fe2fcc0b..ae5e8283 100644 --- a/src/Adapters/Push/OneSignalPushAdapter.js +++ b/src/Adapters/Push/OneSignalPushAdapter.js @@ -18,6 +18,10 @@ export class OneSignalPushAdapter extends PushAdapter { this.validPushTypes = ['ios', 'android']; this.senderMap = {}; this.OneSignalConfig = {}; + const { oneSignalAppId, oneSignalApiKey } = pushConfig; + if (!oneSignalAppId || !oneSignalApiKey) { + throw "Trying to initialiazed OneSignalPushAdapter without oneSignalAppId or oneSignalApiKey"; + } this.OneSignalConfig['appId'] = pushConfig['oneSignalAppId']; this.OneSignalConfig['apiKey'] = pushConfig['oneSignalApiKey']; diff --git a/src/Adapters/loadAdapter.js b/src/Adapters/loadAdapter.js deleted file mode 100644 index 2ab7b350..00000000 --- a/src/Adapters/loadAdapter.js +++ /dev/null @@ -1,25 +0,0 @@ -export default options => { - if (!options) { - return undefined; - } - - if (typeof options === 'string') { - //Configuring via module name with no options - return require(options)(); - } - - if (!options.module && !options.class) { - //Configuring via object - return options; - } - - if (options.module) { - //Configuring via module name + options - return require(options.module)(options.options) - } - - if (options.class) { - //Configuring via class + options - return options.class(options.options); - } -} diff --git a/src/Config.js b/src/Config.js index 2391a831..1203b0a3 100644 --- a/src/Config.js +++ b/src/Config.js @@ -24,8 +24,8 @@ export class Config { this.allowClientClassCreation = cacheInfo.allowClientClassCreation; this.database = DatabaseAdapter.getDatabaseConnection(applicationId, cacheInfo.collectionPrefix); + this.mailController = cacheInfo.mailController; this.verifyUserEmails = cacheInfo.verifyUserEmails; - this.emailAdapter = cacheInfo.emailAdapter; this.appName = cacheInfo.appName; this.hooksController = cacheInfo.hooksController; diff --git a/src/Controllers/AdaptableController.js b/src/Controllers/AdaptableController.js index ef45b022..83f3f0a0 100644 --- a/src/Controllers/AdaptableController.js +++ b/src/Controllers/AdaptableController.js @@ -31,7 +31,6 @@ export class AdaptableController { } validateAdapter(adapter) { - if (!adapter) { throw new Error(this.constructor.name+" requires an adapter"); } diff --git a/src/Controllers/MailController.js b/src/Controllers/MailController.js new file mode 100644 index 00000000..ee467fe6 --- /dev/null +++ b/src/Controllers/MailController.js @@ -0,0 +1,29 @@ +import AdaptableController from './AdaptableController'; +import { MailAdapter } from '../Adapters/Email/MailAdapter'; +import { randomString } from '../cryptoUtils'; +import { inflate } from '../triggers'; + +export class MailController extends AdaptableController { + setEmailVerificationStatus(user, status) { + if (status == false) { + user._email_verify_token = randomString(25); + } + user.emailVerified = status; + } + sendVerificationEmail(user, config) { + const token = encodeURIComponent(user._email_verify_token); + const username = encodeURIComponent(user.username); + let link = `${config.mount}/verify_email?token=${token}&username=${username}`; + this.adapter.sendVerificationEmail({ + appName: config.appName, + link: link, + user: inflate('_User', user), + }); + } + sendMail(options) { + this.adapter.sendMail(options); + } + expectedAdapterType() { + return MailAdapter; + } +} diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 79dee41c..1e329734 100644 --- a/src/Routers/UsersRouter.js +++ b/src/Routers/UsersRouter.js @@ -28,8 +28,7 @@ export class UsersRouter extends ClassesRouter { req.params.className = '_User'; if (req.config.verifyUserEmails) { - req.body._email_verify_token = cryptoUtils.randomString(25); - req.body.emailVerified = false; + req.config.mailController.setEmailVerificationStatus(req.body, false); } let p = super.handleCreate(req); @@ -37,12 +36,7 @@ export class UsersRouter extends ClassesRouter { if (req.config.verifyUserEmails) { // Send email as fire-and-forget once the user makes it into the DB. p.then(() => { - let link = req.config.mount + "/verify_email?token=" + encodeURIComponent(req.body._email_verify_token) + "&username=" + encodeURIComponent(req.body.username); - req.config.emailAdapter.sendVerificationEmail({ - appName: req.config.appName, - link: link, - user: triggers.inflate('_User', req.body), - }); + req.config.mailController.sendVerificationEmail(req.body, req.config); }); } return p; diff --git a/src/index.js b/src/index.js index 247a9274..74a63bf9 100644 --- a/src/index.js +++ b/src/index.js @@ -16,11 +16,11 @@ import ParsePushAdapter from './Adapters/Push/ParsePushAdapter'; //import passwordReset from './passwordReset'; import PromiseRouter from './PromiseRouter'; import verifyEmail from './verifyEmail'; -import loadAdapter from './Adapters/loadAdapter'; import { AnalyticsRouter } from './Routers/AnalyticsRouter'; import { ClassesRouter } from './Routers/ClassesRouter'; import { FileLoggerAdapter } from './Adapters/Logger/FileLoggerAdapter'; import { FilesController } from './Controllers/FilesController'; +import { MailController } from './Controllers/MailController'; import { FilesRouter } from './Routers/FilesRouter'; import { FunctionsRouter } from './Routers/FunctionsRouter'; import { GridStoreAdapter } from './Adapters/Files/GridStoreAdapter'; @@ -30,7 +30,7 @@ import { HooksRouter } from './Routers/HooksRouter'; import { HooksController } from './Controllers/HooksController'; import { InstallationsRouter } from './Routers/InstallationsRouter'; -import { AdapterLoader } from './Adapters/AdapterLoader'; +import { loadAdapter } from './Adapters/AdapterLoader'; import { LoggerController } from './Controllers/LoggerController'; import { PushController } from './Controllers/PushController'; import { PushRouter } from './Routers/PushRouter'; @@ -79,9 +79,6 @@ let validateEmailConfiguration = (verifyUserEmails, appName, emailAdapter) => { if (!emailAdapter) { throw 'User email verification was enabled, but no email adapter was provided'; } - if (typeof emailAdapter.sendVerificationEmail !== 'function') { - throw 'Invalid email adapter: no sendVerificationEmail() function was provided'; - } } } @@ -164,11 +161,10 @@ function ParseServer({ appName: appName, }); - if (verifyUserEmails && process.env.PARSE_EXPERIMENTAL_EMAIL_VERIFICATION_ENABLED || process.env.TESTING == 1) { - emailAdapter = loadAdapter(emailAdapter); - validateEmailConfiguration(verifyUserEmails, appName, emailAdapter); + if (verifyUserEmails && (process.env.PARSE_EXPERIMENTAL_EMAIL_VERIFICATION_ENABLED || process.env.TESTING == 1)) { + let mailController = new MailController(loadAdapter(emailAdapter)); + cache.apps[appId].mailController = mailController; cache.apps[appId].verifyUserEmails = verifyUserEmails; - cache.apps[appId].emailAdapter = emailAdapter; } // To maintain compatibility. TODO: Remove in some version that breaks backwards compatability