RFC for injecting server secrets + refactoring auth code [azuredevops bintray bitbucket]

core/base-service/base.js

@@ -207,9 +207,33 @@ module.exports = class BaseService {
     return result
   }
 
-  constructor({ sendAndCacheRequest }, { handleInternalErrors }) {
+  constructor(
+    { sendAndCacheRequest },
+    { handleInternalErrors, private: privateConfig }
+  ) {
     this._requestFetcher = sendAndCacheRequest
     this._handleInternalErrors = handleInternalErrors
+    this._privateConfig = privateConfig
+  }
+
+  /**
+   * Get the specified secret key from configuration. If the key doesn't
+   * exist, throw an Inaccessible exception.
+   */
+  getSecret(key) {
+    const result = this._privateConfig[key]
+    if (!result) {
+      throw Inaccessible({ prettyMessage: 'auth not configured' })
+    }
+    return result
+  }
+
+  /**
+   * Get the specified secret key from configuration. If the key doesn't
+   * exist, return `undefined`.
+   */
+  tryGetSecret(key) {
+    return this._privateConfig[key]
   }
 
   async _request({ url, options = {}, errorMessages = {} }) {

core/base-service/legacy-request-handler.js

@@ -63,6 +63,27 @@ function flattenQueryParams(queryParams) {
   return Array.from(union).sort()
 }
 
+// Wrapper around `cachingRequest` that returns a promise rather than needing
+// to pass a callback.
+function promisify(cachingRequest) {
+  return (uri, options) =>
+    new Promise((resolve, reject) => {
+      cachingRequest(uri, options, (err, res, buffer) => {
+        if (err) {
+          if (err instanceof ShieldsRuntimeError) {
+            reject(err)
+          } else {
+            // Wrap the error in an Inaccessible so it can be identified
+            // by the BaseService handler.
+            reject(new Inaccessible({ underlyingError: err }))
+          }
+        } else {
+          resolve({ res, buffer })
+        }
+      })
+    })
+}
+
 // handlerOptions can contain:
 // - handler: The service's request handler function
 // - queryParams: An array of the field names of any custom query parameters
@@ -231,24 +252,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
       })
     }
 
-    // Wrapper around `cachingRequest` that returns a promise rather than
-    // needing to pass a callback.
-    cachingRequest.asPromise = (uri, options) =>
-      new Promise((resolve, reject) => {
-        cachingRequest(uri, options, (err, res, buffer) => {
-          if (err) {
-            if (err instanceof ShieldsRuntimeError) {
-              reject(err)
-            } else {
-              // Wrap the error in an Inaccessible so it can be identified
-              // by the BaseService handler.
-              reject(new Inaccessible({ underlyingError: err }))
-            }
-          } else {
-            resolve({ res, buffer })
-          }
-        })
-      })
+    cachingRequest.asPromise = promisify(cachingRequest)
 
     vendorDomain.run(() => {
       const result = handlerOptions.handler(
@@ -304,6 +308,7 @@ function clearRequestCache() {
 
 module.exports = {
   handleRequest,
+  promisify,
   clearRequestCache,
   // Expose for testing.
   _requestCache: requestCache,

core/server/server.js

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

lib/server-secrets.js

@@ -1,15 +0,0 @@
-'use strict'
-
-const fs = require('fs')
-const path = require('path')
-const config = require('config').util.toObject()
-
-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)
-}
-
-module.exports = config.private

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))
 

services/auth.js

@@ -0,0 +1,35 @@
+'use strict'
+
+const { BaseService } = require('.')
+
+function requiredAuth(serviceInstance, userKey, passKey) {
+  if (!(serviceInstance instanceof BaseService)) {
+    throw Error('serviceInstance should be an instance of BaseService')
+  }
+
+  return {
+    user: userKey ? serviceInstance.getSecret(userKey) : '',
+    pass: passKey ? serviceInstance.getSecret(passKey) : '',
+  }
+}
+
+function optionalAuth(serviceInstance, userKey, passKey) {
+  if (!(serviceInstance instanceof BaseService)) {
+    throw Error('serviceInstance should be an instance of BaseService')
+  }
+
+  let user = '',
+    pass = ''
+  if (userKey && !(user = serviceInstance.tryGetSecret(userKey))) {
+    return undefined
+  }
+  if (passKey && !(pass = serviceInstance.tryGetSecret(passKey))) {
+    return undefined
+  }
+  return { user, pass }
+}
+
+module.exports = {
+  requiredAuth,
+  optionalAuth,
+}

services/azure-devops/azure-devops-base.js

@@ -2,6 +2,7 @@
 
 const Joi = require('joi')
 const { BaseJsonService, NotFound } = require('..')
+const { auth } = require('./azure-devops-helpers')
 
 const latestBuildSchema = Joi.object({
   count: Joi.number().required(),
@@ -29,7 +30,6 @@ module.exports = class AzureDevOpsBase extends BaseJsonService {
     project,
     definitionId,
     branch,
-    headers,
     errorMessages
   ) {
     // Microsoft documentation: https://docs.microsoft.com/en-us/rest/api/azure/devops/build/builds/list?view=azure-devops-rest-5.0
@@ -40,12 +40,9 @@ module.exports = class AzureDevOpsBase extends BaseJsonService {
         $top: 1,
         statusFilter: 'completed',
         'api-version': '5.0-preview.4',
+        branchName: branch ? `refs/heads/${branch}` : undefined,
       },
-      headers,
-    }
-
-    if (branch) {
-      options.qs.branchName = `refs/heads/${branch}`
+      auth: auth(this),
     }
 
     const json = await this.fetch({

services/azure-devops/azure-devops-coverage.service.js

@@ -5,7 +5,7 @@ const {
   coveragePercentage: coveragePercentageColor,
 } = require('../color-formatters')
 const AzureDevOpsBase = require('./azure-devops-base')
-const { keywords, getHeaders } = require('./azure-devops-helpers')
+const { keywords, auth } = require('./azure-devops-helpers')
 
 const documentation = `
 <p>
@@ -100,7 +100,6 @@ module.exports = class AzureDevOpsCoverage extends AzureDevOpsBase {
   }
 
   async handle({ organization, project, definitionId, branch }) {
-    const headers = getHeaders()
     const errorMessages = {
       404: 'build pipeline or coverage not found',
     }
@@ -109,7 +108,6 @@ module.exports = class AzureDevOpsCoverage extends AzureDevOpsBase {
       project,
       definitionId,
       branch,
-      headers,
       errorMessages
     )
     // Microsoft documentation: https://docs.microsoft.com/en-us/rest/api/azure/devops/test/code%20coverage/get%20build%20code%20coverage?view=azure-devops-rest-5.0
@@ -119,7 +117,7 @@ module.exports = class AzureDevOpsCoverage extends AzureDevOpsBase {
         buildId,
         'api-version': '5.0-preview.1',
       },
-      headers,
+      auth: auth(this),
     }
     const json = await this.fetch({
       url,

services/azure-devops/azure-devops-helpers.js

@@ -1,8 +1,8 @@
 'use strict'
 
 const Joi = require('joi')
-const serverSecrets = require('../../lib/server-secrets')
 const { isBuildStatus } = require('../build-status')
+const { optionalAuth } = require('../auth')
 
 const keywords = ['vso', 'vsts', 'azure-devops']
 
@@ -23,15 +23,8 @@ async function fetch(serviceInstance, { url, qs = {}, errorMessages }) {
   return { status }
 }
 
-function getHeaders() {
-  const headers = {}
-  if (serverSecrets.azure_devops_token) {
-    const pat = serverSecrets.azure_devops_token
-    const auth = Buffer.from(`:${pat}`).toString('base64')
-    headers.Authorization = `basic ${auth}`
-  }
-
-  return headers
+function auth(serviceInstance) {
+  return optionalAuth(serviceInstance, undefined, 'azure_devops_token')
 }
 
-module.exports = { keywords, fetch, getHeaders }
+module.exports = { keywords, fetch, auth }

services/azure-devops/azure-devops-tests.service.js

@@ -6,7 +6,7 @@ const {
   renderTestResultBadge,
 } = require('../test-results')
 const AzureDevOpsBase = require('./azure-devops-base')
-const { getHeaders } = require('./azure-devops-helpers')
+const { auth } = require('./azure-devops-helpers')
 
 const commonAttrs = {
   keywords: ['vso', 'vsts', 'azure-devops'],
@@ -192,7 +192,6 @@ module.exports = class AzureDevOpsTests extends AzureDevOpsBase {
       skipped_label: skippedLabel,
     }
   ) {
-    const headers = getHeaders()
     const errorMessages = {
       404: 'build pipeline or test result summary not found',
     }
@@ -201,23 +200,17 @@ module.exports = class AzureDevOpsTests extends AzureDevOpsBase {
       project,
       definitionId,
       branch,
-      headers,
       errorMessages
     )
 
     // https://dev.azure.com/azuredevops-powershell/azuredevops-powershell/_apis/test/ResultSummaryByBuild?buildId=20
-    const url = `https://dev.azure.com/${organization}/${project}/_apis/test/ResultSummaryByBuild`
-    const options = {
-      qs: {
-        buildId,
-      },
-      headers,
-    }
-
     const json = await this.fetch({
-      url,
-      options,
       schema: buildTestResultSummarySchema,
+      url: `https://dev.azure.com/${organization}/${project}/_apis/test/ResultSummaryByBuild`,
+      options: {
+        qs: { buildId },
+        auth: auth(this),
+      },
       errorMessages,
     })
 

services/bintray/bintray.service.js

@@ -1,8 +1,8 @@
 'use strict'
 
 const Joi = require('joi')
+const { optionalAuth } = require('../auth')
 const { renderVersionBadge } = require('../version')
-const serverSecrets = require('../../lib/server-secrets')
 const { BaseJsonService } = require('..')
 
 const schema = Joi.object()
@@ -42,19 +42,13 @@ module.exports = class Bintray extends BaseJsonService {
   }
 
   async fetch({ subject, repo, packageName }) {
-    const options = {}
-    if (serverSecrets.bintray_user) {
-      options.auth = {
-        user: serverSecrets.bintray_user,
-        pass: serverSecrets.bintray_apikey,
-      }
-    }
-
     // https://bintray.com/docs/api/#_get_version
     return this._requestJson({
       schema,
       url: `https://bintray.com/api/v1/packages/${subject}/${repo}/${packageName}/versions/_latest`,
-      options,
+      options: {
+        auth: optionalAuth(this, 'bintray_user', 'bintray_apikey'),
+      },
     })
   }