Merge pull request #18844 from mozilla/FXA-11607

task(auth-server): Replace accountStatusCheck customs rule with new rate limit library call

Author: dschom

.circleci/config.yml

@@ -177,6 +177,7 @@ executors:
       CUSTOMS_SERVER_URL: none
       HUSKY_SKIP_INSTALL: 1
       AUTH_CLOUDTASKS_USE_LOCAL_EMULATOR: true
+      RATE_LIMIT__RULES: ""
 
   # Contains a pre-installed fxa stack and browsers for doing ui test
   # automation. Perfect for running smoke tests against remote targets.

libs/accounts/rate-limit/src/lib/rate-limit.spec.ts

@@ -33,6 +33,17 @@ describe('rate-limit', () => {
     expect(rateLimit).toBeDefined();
   });
 
+  it('can determine if action is supported', () => {
+    rateLimit = new RateLimit(
+      parseConfigRules(['test:ip:1:1s:1s']),
+      redis,
+      statsd
+    );
+
+    expect(rateLimit.supportsAction('test')).toBeTruthy();
+    expect(rateLimit.supportsAction('foo')).toBeFalsy();
+  });
+
   it('throws if no action can be found', async () => {
     rateLimit = new RateLimit({}, redis, statsd);
     expect(rateLimit.check('foo', {})).rejects.toThrow(

libs/accounts/rate-limit/src/lib/rate-limit.ts

@@ -52,6 +52,15 @@ export class RateLimit {
     }
   }
 
+  /**
+   * Checks to see if there are rules for a given action.
+   * @param action - Name of action
+   * @returns - True if action has rules, otherwise false.
+   */
+  supportsAction(action:string) {
+    return this.rules[action] != null;
+  }
+
   /**
    * Checks to see if a rate limit has been exceeded.
    * @param action The action being conducted. Important, if the action is not defined, a runtime error will occur!
@@ -73,7 +82,7 @@ export class RateLimit {
       throw new ActionNotFound(action);
     }
 
-    const openBlocks = [];
+    const openBlocks = new Array<BlockStatus>();
 
     // Important! Set timestamp of check upfront.
     // This reduces small variance because of wait on IO operations.
@@ -159,9 +168,7 @@ export class RateLimit {
       //    - Open question, do we also want to no blocks with shorter bans? Or just the block with largest
       //      ban (ie the biggest retryAfter value).
 
-      return {
-        ...block,
-      };
+      return block;
     }
 
     // Made it through the gauntlet of rules. No blocks found!

packages/fxa-auth-server/bin/key_server.js

@@ -51,6 +51,7 @@ const {
   RecoveryPhoneService,
   TwilioFactory,
 } = require('@fxa/accounts/recovery-phone');
+const { parseConfigRules, RateLimit } = require('@fxa/accounts/rate-limit');
 const { AccountManager } = require('@fxa/shared/account/account');
 const { setupAccountDatabase } = require('@fxa/shared/db/mysql/account');
 const { EmailCloudTaskManager } = require('../lib/email-cloud-tasks');
@@ -220,6 +221,16 @@ async function run(config) {
     Container.get(AppleIAP);
   }
 
+  // Create rate limiter (aka customs v2)
+  const rules = parseConfigRules(config.rateLimit.rules);
+  const rateLimitRedis = new Redis({
+    ...config.redis,
+    ...config.redis.customs,
+  });
+  const rateLimit = new RateLimit(rules, rateLimitRedis, statsd);
+  Container.set(RateLimit, rateLimit);
+
+  // Create Recovery Phone Service
   const twilio = TwilioFactory.useFactory(config.twilio);
   const recoveryPhoneRedis = new Redis({
     ...config.redis,
@@ -308,7 +319,7 @@ async function run(config) {
     config.domain
   );
   const Password = require('../lib/crypto/password')(log, config);
-  const customs = new Customs(config.customsUrl, log, error, statsd);
+  const customs = new Customs(config.customsUrl, log, error, statsd, rateLimit);
   const zendeskClient = require('../lib/zendesk-client').createZendeskClient(
     config
   );

packages/fxa-auth-server/config/index.ts

@@ -2160,6 +2160,18 @@ const convictConf = convict({
       },
     },
   },
+  rateLimit: {
+    rules: {
+      default: [
+        'unblockEmail          : email  : 10   : 24 hours    : 24 hours    ',
+        'accountStatusCheck    : ip     : 20   : 15 minutes  : 15 minutes  ',
+        'accountStatusCheck    : email  : 20   : 15 minutes  : 15 minutes  ',
+      ],
+      doc: 'Rules for rate limiting user actions. These are essentially customs v2 rules.',
+      env: 'RATE_LIMIT__RULES',
+      format: Array,
+    },
+  },
   recoveryPhone: {
     enabled: {
       default: false,

packages/fxa-auth-server/lib/customs.js

@@ -17,10 +17,12 @@ const localizeTimestamp =
 const serviceName = 'customs';
 
 class CustomsClient {
-  constructor(url, log, error, statsd) {
+  constructor(url, log, error, statsd, rateLimit) {
     this.log = log;
     this.error = error;
     this.statsd = statsd;
+    this.rateLimit = rateLimit;
+
     const customsHttpAgentConfig = config.get('customsHttpAgent');
 
     if (url !== 'none') {
@@ -87,6 +89,14 @@ class CustomsClient {
   }
 
   async check(request, email, action) {
+    const checked = await this.checkV2(request, 'check', action, {
+      ip: request?.app?.clientAddress,
+      email,
+    });
+    if (checked) {
+      return;
+    }
+
     const result = await this.makeRequest('/check', {
       ...this.sanitizePayload({
         ip: request.app.clientAddress,
@@ -108,6 +118,14 @@ class CustomsClient {
   }
 
   async checkAuthenticated(request, uid, action) {
+    const checked = await this.checkV2(request, 'checkAuthenticated', action, {
+      ip: request?.app?.clientAddress,
+      uid,
+    });
+    if (checked) {
+      return;
+    }
+
     const result = await this.makeRequest('/checkAuthenticated', {
       ...this.sanitizePayload({
         action,
@@ -121,6 +139,13 @@ class CustomsClient {
   }
 
   async checkIpOnly(request, action) {
+    const checked = await this.checkV2(request, 'checkIpOnly', action, {
+      ip: request?.app?.clientAddress,
+    });
+    if (checked) {
+      return;
+    }
+
     const result = await this.makeRequest('/checkIpOnly', {
       ...this.sanitizePayload({
         action,
@@ -257,6 +282,76 @@ class CustomsClient {
       });
     }
   }
+
+  // #region Customs V2
+
+  /**
+   * Version 2 Customs Approach
+   * =======================================================================================
+   * This uses a library provided by libs and works directly with Redis to make rate limiting
+   * decisions. The previous customs check to see if there is 'new' configuration for the
+   * customs action being checked. If there is, we will call into this code instead of calling
+   * the legacy customs service.
+   */
+  async checkV2(request, type, action, opts) {
+    // Short circuit if rate limit wasn't provided.
+    if (this.rateLimit == null) {
+      return false;
+    }
+
+    // Fallback to the legacy customs service approach, if v2 action isn't configured
+    const actionConfigured = this.rateLimit.supportsAction(action);
+    if (!actionConfigured) {
+      this.statsd?.increment(`${serviceName}.check.v1`, [`action:${action}`]);
+      return false;
+    }
+
+    // Otherwise, call the new nx lib instead of the legacy service
+    this.statsd?.increment(`${serviceName}.check.v2`, [`action:${action}`]);
+    const result = await this.rateLimit.check(action, opts);
+
+    // If statsd was provided, record metrics
+    this.statsd?.increment(`${serviceName}.request.v2.${type}`, {
+      action,
+      block: result != null,
+      blockReason: result?.reason || '',
+    });
+
+
+    // If no result, we exit. Check essentially passes.
+    if (result == null) {
+      return true;
+    }
+
+    // We use the rate limiter to allow X number unblock attempts per day. Once
+    // unblock attempts have been exhausted, the user cannot request an unblock
+    // code and must wait until the unblockEmail ban duration has expired. Similar
+    // logic existed in the old customs server, but these sorts of decisions are
+    // actually domain of the service using customs and not customs itself, so
+    // this is the revised approach.
+    let canUnblock = false;
+    const { email } = opts;
+    if (email) {
+      const unblockResult = await this.rateLimit.check('unblockEmail', {
+        email,
+      });
+      canUnblock = unblockResult == null;
+    }
+
+    request.emitMetricsEvent('customs.blocked');
+    const retryAfterLocalized = localizeTimestamp.format(
+      Date.now() + result.retryAfter * 1000,
+      request.headers['accept-language']
+    );
+
+    throw this.error.tooManyRequests(
+      result.retryAfter,
+      retryAfterLocalized,
+      canUnblock
+    );
+
+  }
+  // #endregion
 }
 
 module.exports = CustomsClient;

packages/fxa-auth-server/lib/error.js

@@ -432,7 +432,7 @@ AppError.tooManyRequests = function (
     extraData.verificationReason = 'login';
   }
 
-  return new AppError(
+  const error = new AppError(
     {
       code: 429,
       error: 'Too Many Requests',
@@ -444,6 +444,8 @@ AppError.tooManyRequests = function (
       'retry-after': retryAfter,
     }
   );
+
+  return error;
 };
 
 AppError.requestBlocked = function (canUnblock) {

packages/fxa-auth-server/test/local/customs.js

@@ -4,12 +4,11 @@
 
 'use strict';
 
-const ROOT_DIR = '../..';
 
 const sinon = require('sinon');
 const assert = { ...sinon.assert, ...require('chai').assert };
 const mocks = require('../mocks');
-const error = require(`${ROOT_DIR}/lib/error.js`);
+const error = require(`../../lib/error.js`);
 const nock = require('nock');
 
 const CUSTOMS_URL_REAL = 'http://localhost:7000';
@@ -18,7 +17,7 @@ const CUSTOMS_URL_MISSING = 'http://localhost:7001';
 const customsServer = nock(CUSTOMS_URL_REAL).defaultReplyHeaders({
   'Content-Type': 'application/json',
 });
-const Customs = require(`${ROOT_DIR}/lib/customs.js`);
+const Customs = require(`../../lib/customs.js`);
 
 describe('Customs', () => {
   let customsNoUrl;
@@ -720,6 +719,52 @@ describe('Customs', () => {
     });
   });
 
+  describe('customs v2', () => {
+    const mockRateLimit = {
+      supportsAction: sinon.spy(() => true),
+      check: sinon.spy(),
+    };
+    const customs = new Customs(
+      CUSTOMS_URL_REAL,
+      log,
+      error,
+      statsd,
+      mockRateLimit
+    );
+
+    it('can allow checkAccountStatus with rate-limit lib', async () => {
+      mockRateLimit.check = sandbox.spy(async () => {
+        return await Promise.resolve(null);
+      });
+      // Should no throw
+      await customs.check(request, email, 'accountStatusCheck');
+    });
+
+    it('can block checkAccountStatus with rate-limit lib', async () => {
+      mockRateLimit.check = sandbox.spy(async () => {
+        return await Promise.resolve({
+          retryAfter: 1000,
+          reason: 'too-many-attempts',
+        });
+      });
+
+      let customsError = undefined;
+      try {
+        await customs.check(request, email, 'accountStatusCheck');
+      } catch (err) {
+        customsError = err;
+      }
+
+      assert.isDefined(error);
+      assert.equal(customsError.errno, 114);
+      assert.equal(customsError.output.payload.error, 'Too Many Requests');
+      assert.equal(
+        customsError.output.payload.message,
+        'Client has sent too many requests'
+      );
+    });
+  });
+
   describe('statsd metrics', () => {
     const tags = {
       block: true,
@@ -748,7 +793,7 @@ describe('Customs', () => {
         await customsWithUrl.check(request, email, action);
         assert.fail('should have failed');
       } catch (err) {
-          assert.isTrue(
+        assert.isTrue(
           statsd.increment.calledWithExactly('customs.request.check', {
             action,
             ...validTags,

packages/fxa-auth-server/test/local/routes/account.js

@@ -1544,13 +1544,15 @@ describe('/account/status', () => {
     );
     const mockMailer = mocks.mockMailer();
     const mockPush = mocks.mockPush();
+    const mockCustoms = mocks.mockCustoms();
     const verificationReminders = mocks.mockVerificationReminders();
     const subscriptionAccountReminders = mocks.mockVerificationReminders();
     const accountRoutes = makeRoutes({
       config,
       db: mockDB,
       log: mockLog,
       mailer: mockMailer,
+      customs: mockCustoms,
       Password: function () {
         return {
           unwrap: function () {

packages/fxa-auth-server/test/mocks.js

@@ -353,6 +353,7 @@ function mockCustoms(errors) {
   return mockObject(CUSTOMS_METHOD_NAMES)({
     checkAuthenticated: optionallyThrow(errors, 'checkAuthenticated'),
     checkIpOnly: optionallyThrow(errors, 'checkIpOnly'),
+    checkV2: sinon.spy(() => false),
   });
 }