Inject secrets into the services (badges#3652)

This is a reworking of badges#3410 based on some feedback @calebcartwright left on that PR.

The goals of injecting the secrets are threefold:

1. Simplify testing
2. Be consistent with all of the other config (which is injected)
3. Encapsulate the sensitive auth-related code in one place so it can be studied and tested thoroughly

- Rather than add more code to BaseService to handle authorization logic, it delegates that to an AuthHelper class.
- When the server starts, it fetches the credentials from `config` and injects them into `BaseService.register()` which passes them to `invoke()`.
- In `invoke()` the service's auth configuration is checked (`static get auth()`, much like `static get route()`).
- If the auth config is present, an AuthHelper instance is created and attached to the new instance.
- Then within the service, the password, basic auth config, or bearer authentication can be accessed via e.g. `this.authHelper.basicAuth` and passed to `this._requestJson()` and friends.
- Everything is being done very explicitly, so it should be very clear where and how the configured secrets are being used.
- Testing different configurations of services can now be done by injecting the config into `invoke()` in `.spec` files instead of mocking global state in the service tests as was done before. See the new Jira spec files for a good example of this.

Ref badges#3393

.eslintrc.yml

@@ -30,6 +30,7 @@ overrides:
                 'category',
                 'isDeprecated',
                 'route',
+                'auth',
                 'examples',
                 '_cacheLength',
                 'defaultBadgeData',

core/base-service/auth-helper.js

@@ -0,0 +1,43 @@
+'use strict'
+
+class AuthHelper {
+  constructor({ userKey, passKey, isRequired = false }, privateConfig) {
+    if (!userKey && !passKey) {
+      throw Error('Expected userKey or passKey to be set')
+    }
+
+    this._userKey = userKey
+    this._passKey = passKey
+    this.user = userKey ? privateConfig[userKey] : undefined
+    this.pass = passKey ? privateConfig[passKey] : undefined
+    this.isRequired = isRequired
+  }
+
+  get isConfigured() {
+    return (
+      (this._userKey ? Boolean(this.user) : true) &&
+      (this._passKey ? Boolean(this.pass) : true)
+    )
+  }
+
+  get isValid() {
+    if (this.isRequired) {
+      return this.isConfigured
+    } else {
+      const configIsEmpty = !this.user && !this.pass
+      return this.isConfigured || configIsEmpty
+    }
+  }
+
+  get basicAuth() {
+    const { user, pass } = this
+    return this.isConfigured ? { user, pass } : undefined
+  }
+
+  get bearerAuthHeader() {
+    const { pass } = this
+    return this.isConfigured ? { Authorization: `Bearer ${pass}` } : undefined
+  }
+}
+
+module.exports = { AuthHelper }

core/base-service/auth-helper.spec.js

@@ -0,0 +1,96 @@
+'use strict'
+
+const { expect } = require('chai')
+const { test, given, forCases } = require('sazerac')
+const { AuthHelper } = require('./auth-helper')
+
+describe('AuthHelper', function() {
+  it('throws without userKey or passKey', function() {
+    expect(() => new AuthHelper({}, {})).to.throw(
+      Error,
+      'Expected userKey or passKey to be set'
+    )
+  })
+
+  describe('isValid', function() {
+    function validate(config, privateConfig) {
+      return new AuthHelper(config, privateConfig).isValid
+    }
+    test(validate, () => {
+      forCases([
+        // Fully configured user + pass.
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass', isRequired: true },
+          { myci_user: 'admin', myci_pass: 'abc123' }
+        ),
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass' },
+          { myci_user: 'admin', myci_pass: 'abc123' }
+        ),
+        // Fully configured user or pass.
+        given(
+          { userKey: 'myci_user', isRequired: true },
+          { myci_user: 'admin' }
+        ),
+        given(
+          { passKey: 'myci_pass', isRequired: true },
+          { myci_pass: 'abc123' }
+        ),
+        given({ userKey: 'myci_user' }, { myci_user: 'admin' }),
+        given({ passKey: 'myci_pass' }, { myci_pass: 'abc123' }),
+        // Empty config.
+        given({ userKey: 'myci_user', passKey: 'myci_pass' }, {}),
+        given({ userKey: 'myci_user' }, {}),
+        given({ passKey: 'myci_pass' }, {}),
+      ]).expect(true)
+
+      forCases([
+        // Partly configured.
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass', isRequired: true },
+          { myci_user: 'admin' }
+        ),
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass' },
+          { myci_user: 'admin' }
+        ),
+        // Missing required config.
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass', isRequired: true },
+          {}
+        ),
+        given({ userKey: 'myci_user', isRequired: true }, {}),
+        given({ passKey: 'myci_pass', isRequired: true }, {}),
+      ]).expect(false)
+    })
+  })
+
+  describe('basicAuth', function() {
+    function validate(config, privateConfig) {
+      return new AuthHelper(config, privateConfig).basicAuth
+    }
+    test(validate, () => {
+      forCases([
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass', isRequired: true },
+          { myci_user: 'admin', myci_pass: 'abc123' }
+        ),
+        given(
+          { userKey: 'myci_user', passKey: 'myci_pass' },
+          { myci_user: 'admin', myci_pass: 'abc123' }
+        ),
+      ]).expect({ user: 'admin', pass: 'abc123' })
+      given({ userKey: 'myci_user' }, { myci_user: 'admin' }).expect({
+        user: 'admin',
+        pass: undefined,
+      })
+      given({ passKey: 'myci_pass' }, { myci_pass: 'abc123' }).expect({
+        user: undefined,
+        pass: 'abc123',
+      })
+      given({ userKey: 'myci_user', passKey: 'myci_pass' }, {}).expect(
+        undefined
+      )
+    })
+  })
+})

core/base-service/base.js

@@ -4,13 +4,15 @@ const decamelize = require('decamelize')
 // See available emoji at http://emoji.muan.co/
 const emojic = require('emojic')
 const Joi = require('@hapi/joi')
+const { AuthHelper } = require('./auth-helper')
 const { assertValidCategory } = require('./categories')
 const checkErrorResponse = require('./check-error-response')
 const coalesceBadge = require('./coalesce-badge')
 const {
   NotFound,
   InvalidResponse,
   Inaccessible,
+  ImproperlyConfigured,
   InvalidParameter,
   Deprecated,
 } = require('./errors')
@@ -132,6 +134,33 @@ module.exports = class BaseService {
     throw new Error(`Route not defined for ${this.name}`)
   }
 
+  /**
+   * Configuration for the authentication helper that prepares credentials
+   * for upstream requests.
+   *
+   * @abstract
+   * @return {object} auth
+   * @return {string} auth.userKey
+   *    (Optional) The key from `privateConfig` to use as the username.
+   * @return {string} auth.passKey
+   *    (Optional) The key from `privateConfig` to use as the password.
+   *    If auth is configured, either `userKey` or `passKey` is required.
+   * @return {string} auth.isRequired
+   *    (Optional) If `true`, the service will return `NotFound` unless the
+   *    configured credentials are present.
+   *
+   * See also the config schema in `./server.js` and `doc/server-secrets.md`.
+   *
+   * To use the configured auth in the handler or fetch method, pass the
+   * credentials to the request. For example:
+   * `{ options: { auth: this.authHelper.basicAuth } }`
+   * `{ options: { headers: this.authHelper.bearerAuthHeader } }`
+   * `{ options: { qs: { token: this.authHelper.pass } } }`
+   */
+  static get auth() {
+    return undefined
+  }
+
   /**
    * Example URLs for this service. These should use the format
    * specified in `route`, and can be used to demonstrate how to use badges for
@@ -238,8 +267,9 @@ module.exports = class BaseService {
     return result
   }
 
-  constructor({ sendAndCacheRequest }, { handleInternalErrors }) {
+  constructor({ sendAndCacheRequest, authHelper }, { handleInternalErrors }) {
     this._requestFetcher = sendAndCacheRequest
+    this.authHelper = authHelper
     this._handleInternalErrors = handleInternalErrors
   }
 
@@ -303,6 +333,7 @@ module.exports = class BaseService {
         color: 'red',
       }
     } else if (
+      error instanceof ImproperlyConfigured ||
       error instanceof InvalidResponse ||
       error instanceof Inaccessible ||
       error instanceof Deprecated
@@ -353,12 +384,26 @@ module.exports = class BaseService {
     trace.logTrace('inbound', emojic.ticket, 'Named params', namedParams)
     trace.logTrace('inbound', emojic.crayon, 'Query params', queryParams)
 
-    const serviceInstance = new this(context, config)
+    // Like the service instance, the auth helper could be reused for each request.
+    // However, moving its instantiation to `register()` makes `invoke()` harder
+    // to test.
+    const authHelper = this.auth
+      ? new AuthHelper(this.auth, config.private)
+      : undefined
+
+    const serviceInstance = new this({ ...context, authHelper }, config)
 
     let serviceError
+    if (authHelper && !authHelper.isValid) {
+      const prettyMessage = authHelper.isRequired
+        ? 'credentials have not been configured'
+        : 'credentials are misconfigured'
+      serviceError = new ImproperlyConfigured({ prettyMessage })
+    }
+
     const { queryParamSchema } = this.route
     let transformedQueryParams
-    if (queryParamSchema) {
+    if (!serviceError && queryParamSchema) {
       try {
         transformedQueryParams = validate(
           {

core/base-service/base.spec.js

@@ -64,7 +64,7 @@ class DummyService extends BaseService {
 }
 
 describe('BaseService', function() {
-  const defaultConfig = { handleInternalErrors: false }
+  const defaultConfig = { handleInternalErrors: false, private: {} }
 
   it('Invokes the handler as expected', async function() {
     expect(
@@ -482,4 +482,43 @@ describe('BaseService', function() {
       }
     })
   })
+
+  describe('auth', function() {
+    class AuthService extends DummyService {
+      static get auth() {
+        return {
+          passKey: 'myci_pass',
+          isRequired: true,
+        }
+      }
+
+      async handle() {
+        return {
+          message: `The CI password is ${this.authHelper.pass}`,
+        }
+      }
+    }
+
+    it('when auth is configured properly, invoke() sets authHelper', async function() {
+      expect(
+        await AuthService.invoke(
+          {},
+          { defaultConfig, private: { myci_pass: 'abc123' } },
+          { namedParamA: 'bar.bar.bar' }
+        )
+      ).to.deep.equal({ message: 'The CI password is abc123' })
+    })
+
+    it('when auth is not configured properly, invoke() returns inacessible', async function() {
+      expect(
+        await AuthService.invoke({}, defaultConfig, {
+          namedParamA: 'bar.bar.bar',
+        })
+      ).to.deep.equal({
+        color: 'lightgray',
+        isError: true,
+        message: 'credentials have not been configured',
+      })
+    })
+  })
 })

core/base-service/errors.js

@@ -72,6 +72,23 @@ class Inaccessible extends ShieldsRuntimeError {
   }
 }
 
+class ImproperlyConfigured extends ShieldsRuntimeError {
+  get name() {
+    return 'ImproperlyConfigured'
+  }
+  get defaultPrettyMessage() {
+    return 'improperly configured'
+  }
+
+  constructor(props = {}) {
+    const message = props.underlyingError
+      ? `ImproperlyConfigured: ${props.underlyingError.message}`
+      : 'ImproperlyConfigured'
+    super(props, message)
+    this.response = props.response
+  }
+}
+
 class InvalidParameter extends ShieldsRuntimeError {
   get name() {
     return 'InvalidParameter'
@@ -106,6 +123,7 @@ class Deprecated extends ShieldsRuntimeError {
 module.exports = {
   ShieldsRuntimeError,
   NotFound,
+  ImproperlyConfigured,
   InvalidResponse,
   Inaccessible,
   InvalidParameter,

core/server/server.js

@@ -248,6 +248,7 @@ module.exports = class Server {
           profiling: config.public.profiling,
           fetchLimitBytes: bytes(config.public.fetchLimit),
           rasterUrl: config.public.rasterUrl,
+          private: config.private,
         }
       )
     )

dangerfile.js

@@ -112,10 +112,13 @@ if (allFiles.length > 100) {
 
     // eslint-disable-next-line promise/prefer-await-to-then
     danger.git.diffForFile(file).then(({ diff }) => {
-      if (diff.includes('serverSecrets') && !secretsDocs.modified) {
+      if (
+        (diff.includes('authHelper') || diff.includes('serverSecrets')) &&
+        !secretsDocs.modified
+      ) {
         warn(
           [
-            `:books: Remember to ensure any changes to \`serverSecrets\` `,
+            `:books: Remember to ensure any changes to \`config.private\` `,
             `in \`${file}\` are reflected in the [server secrets documentation]`,
             '(https://github.com/badges/shields/blob/master/doc/server-secrets.md)',
           ].join('')

server.js

@@ -1,16 +1,17 @@
 'use strict'
 /* eslint-disable import/order */
 
+const fs = require('fs')
+const path = require('path')
+
 require('dotenv').config()
 
 // Set up Sentry reporting as early in the process as possible.
+const config = require('config').util.toObject()
 const Raven = require('raven')
-const serverSecrets = require('./lib/server-secrets')
-
-Raven.config(process.env.SENTRY_DSN || serverSecrets.sentry_dsn).install()
+Raven.config(process.env.SENTRY_DSN || config.private.sentry_dsn).install()
 Raven.disableConsoleAlerts()
 
-const config = require('config').util.toObject()
 if (+process.argv[2]) {
   config.public.bind.port = +process.argv[2]
 }
@@ -21,6 +22,14 @@ if (process.argv[3]) {
 console.log('Configuration:')
 console.dir(config.public, { depth: null })
 
+const legacySecretsPath = path.join(__dirname, 'private', 'secret.json')
+if (fs.existsSync(legacySecretsPath)) {
+  console.error(
+    `Legacy secrets file found at ${legacySecretsPath}. It should be deleted and secrets replaced with environment variables or config/local.yml`
+  )
+  process.exit(1)
+}
+
 const Server = require('./core/server/server')
 const server = (module.exports = new Server(config))