chore(tests): more tests and WebExecuter changes (#425)

Author: pajgo

rfcs/facebook_oauth_tests_throttling_fix_upd.md

@@ -0,0 +1,104 @@
+# Facebook OAuth Tests and Service Logic
+First version of this file was targeted only on Tests update but after some research,
+decided to change some logic in `OAuth` handler too.
+ 
+General changes were intended to add additional tests for Facebook OAuth 
+and provide a better WebExecuter error handling when Facebook throttling occurs.
+
+After some checks on OAuth endpoint logic, I found that error handling must be updated.
+Current error handling was passing a lot of extra data in response, that's shouldn't be shown to the client.
+
+## Tests changes
+### OAuth Error tests
+Tests use stub as `http.auth.test` method with custom error and check that `service`
+produces the correct response.
+
+These tests aim that OAuth Errors coming from `@hapi/bell` exist in service response:
+1. The Test checking that service responds with correct error in case of OAuth error.
+2. The Test checking that additional data do not exist in response. 
+
+Also added one integrity test checking that `WebExecuter` will throw a different error when
+`waitFor...` timeout error is thrown. This error appears in the situation when `Page` wait's for some content, but this content or response not received because of some unpredicted errors.
+
+### WebExecuter Timeout Error
+Added Additional error processing logic to the `helpers/oauth/facebook/web-executer.js`, this will
+provide more informative output from tests. Now if `page.waitFor...` Timeout happens, the error message will
+contain last Service response or page contents.
+
+## Service logic changes
+### @hapi/bell Boom error handling
+In `production` ENV the Service still renders full error data coming from `@hapi/bell`. Stubbed OAuth call with simulated errors
+returns such response:
+
+```javascript
+const $ms_users_inj_post_messsge = { payload:
+   { data:
+      { data: { i_am_very_long_body: true, },
+        isBoom: true,
+        isServer: false,
+        output:
+         { statusCode: 403,
+           payload:
+            { statusCode: 403, error: 'Forbidden', message: 'X-Throttled' },
+           headers: {} },
+        name: 'Error',
+        message: 'X-Throttled',
+        stack:
+         'Error: X-Throttled\n    at Context.forbidden (/src/test/suites/oauth/facebook.js:97:34)\n    at process._tickCallback (internal/process/next_tick.js:68:7)' },
+     isBoom: true,
+     isServer: true,
+     output:
+      { statusCode: 500,
+        payload:
+         { statusCode: 500,
+           error: 'Internal Server Error',
+           message: 'An internal server error occurred' },
+        headers: {} },
+     name: 'Error',
+     message: 'BadError' },
+  error: true,
+  type: 'ms-users:attached',
+  title: 'Failed to attach account',
+  meta: {} }
+```
+
+Even if `HTTP.IncomingMessage` deleted from error, too much data passed in response.
+Rendering of the Error is performed by `serialize-error` in Hook instead `Hapi` server and this shows all error contents.
+
+This great for debug purposes but not for passing to the client.
+I decided to add a new error `OAuthError` with custom `toJSON` serializer, which removes all unnecessary data
+and returns Simplified Object. This error is only for the `auth/OAuth` scope.
+
+After this change Errors render in this way:
+
+```javascript
+const p = { 
+  payload: { 
+    message: 'BadError',
+    name: 'OAuthError',
+    inner_error: {
+      message: 'BadError',
+      name: 'Error',
+      stack:
+       'Error: BadError\n    at Object.internal (/src/test/suites/oauth/facebook.js:106:28)\n    at Object.invoke (/src/node_modules/sinon/lib/sinon/behavior.js:151:35)\n    at module.exports.internals.Auth.functionStub (/src/node_modules/sinon/lib/sinon/stub.js:130:47)\n    at Function.invoke (/src/node_modules/sinon/lib/sinon/spy.js:297:51)\n    at module.exports.internals.Auth.functionStub (/src/node_modules/sinon/lib/sinon/spy.js:90:30)\n    at Users.test (/src/src/auth/oauth/index.js:149:45)\n    at Users.tryCatcher (/src/node_modules/bluebird/js/release/util.js:16:23)\n    at Promise._settlePromiseFromHandler (/src/node_modules/bluebird/js/release/promise.js:517:31)\n    at Promise._settlePromise (/src/node_modules/bluebird/js/release/promise.js:574:18)\n    at Promise._settlePromiseCtx (/src/node_modules/bluebird/js/release/promise.js:611:10)\n    at _drainQueueStep (/src/node_modules/bluebird/js/release/async.js:142:12)\n    at _drainQueue (/src/node_modules/bluebird/js/release/async.js:131:9)\n    at Async._drainQueues (/src/node_modules/bluebird/js/release/async.js:147:5)\n    at Immediate.Async.drainQueues [as _onImmediate] (/src/node_modules/bluebird/js/release/async.js:17:14)\n    at runCallback (timers.js:705:18)\n    at tryOnImmediate (timers.js:676:5)\n    at processImmediate (timers.js:658:5)',
+      data:
+       { message: 'X-Throttled',
+         name: 'Error',
+         stack:
+          'Error: X-Throttled\n    at Context.forbidden (/src/test/suites/oauth/facebook.js:97:34)\n    at process._tickCallback (internal/process/next_tick.js:68:7)',
+         data: { i_am_very_long_body: true } } } },
+   error: true,
+   type: 'ms-users:attached',
+   title: 'Failed to attach account',
+   meta: {} 
+}
+
+```
+
+## TODO
+- [x] WebExecuter Timeout handling
+- [x] Test checking that service return error
+- [x] `@hapi/boom` error serialization on `OAuth preResponse` hook
+- [x] Clean up code
+- [x] Cleanup docs
+ 

src/auth/oauth/index.js

@@ -9,7 +9,7 @@ const extractJWT = require('./utils/extractJWT');
 const { getInternalData } = require('../../utils/userData');
 
 const { verifyToken, loginAttempt } = require('../../utils/amqp');
-const { Redirect } = require('./utils/errors');
+const { Redirect, OAuthError } = require('./utils/errors');
 const { USERS_ID_FIELD, ErrorTotpRequired } = require('../../constants');
 
 // helpers
@@ -18,28 +18,6 @@ const is404 = ({ statusCode }) => statusCode === 404;
 const isError = ({ statusCode }) => statusCode >= 400;
 const isHTMLRedirect = ({ statusCode, source }) => statusCode === 200 && source;
 
-/**
- * Cleanup boom error and return matching error;
- * @param {Boom} error
- * @returns {error}
- */
-function checkBoomError(error) {
-  const { data: errData } = error;
-  const { message } = error;
-
-  // can contain another Boom error
-  if (errData !== null && typeof errData === 'object') {
-    // delete reference to http.IncommingMessage
-    delete errData.data.res;
-  }
-
-  if (message.startsWith('App rejected')) {
-    return Errors.AuthenticationRequiredError(`OAuth ${error.message}`, error);
-  }
-
-  return error;
-}
-
 /**
  * Authentication handler
  *
@@ -171,9 +149,15 @@ module.exports = async function authHandler({ action, transportRequest }) {
     const { credentials } = await http.auth.test(strategy, transportRequest);
     response = [null, credentials];
   } catch (err) {
-    // No need to go further if Oauth Error happened
     if (Boom.isBoom(err)) {
-      throw checkBoomError(err);
+      const { message } = err;
+
+      // Error thrown when user declines OAuth
+      if (message.startsWith('App rejected')) {
+        throw Errors.AuthenticationRequiredError(`OAuth ${err.message}`, err);
+      }
+
+      throw OAuthError(err.message, err);
     }
     // continue if redirect
     response = [err];

src/auth/oauth/utils/errors.js

@@ -1,5 +1,69 @@
 const Errors = require('common-errors');
+const Boom = require('@hapi/boom');
 
 exports.Redirect = Errors.helpers.generateClass('Redirect', {
   args: ['redirectUri'],
 });
+
+const OAuthError = exports.OAuthError = Errors.helpers.generateClass('OAuthError', {
+  args: ['message', 'inner_error'],
+});
+
+/**
+ * Removes `@hapi/boom` attributes
+ * @param error
+ * @returns {*|{stack: *, data: *, name: *, message: *}}
+ */
+function stripBoomAttrs(error) {
+  const { message, name, stack, data: errorData } = error;
+  let data;
+
+  /* assuming that 'data' not passed or it's not and object when Boom.error created. */
+  if (errorData !== null && typeof errorData === 'object') {
+    /* Remove http.IncomingMessage - @hapi/wreck adds it when error is coming from response. */
+    if (errorData.isResponseError) {
+      const { res, ...otherData } = errorData;
+      data = otherData;
+    } else {
+      ({ data } = error);
+    }
+  } else {
+    data = errorData;
+  }
+
+  /* Recursive check */
+  if (Boom.isBoom(error.data)) {
+    data = stripBoomAttrs(error.data);
+  }
+
+  return {
+    message,
+    name,
+    stack,
+    data,
+  };
+}
+
+/**
+ * OAuthError.toJSON custom serialization
+ * Returns simplified object
+ * @returns {*}
+ */
+OAuthError.prototype.toJSON = function toJSON() {
+  const { inner_error: innerError, message, name, stack } = this;
+  let innerErrorJsonObj;
+
+  /* Boom error contains additional data */
+  if (Boom.isBoom(innerError)) {
+    innerErrorJsonObj = stripBoomAttrs(innerError);
+  } else {
+    innerErrorJsonObj = innerError;
+  }
+
+  return {
+    message,
+    name,
+    stack,
+    inner_error: innerErrorJsonObj,
+  };
+};

test/helpers/oauth/facebook/graph-api.js

@@ -1,5 +1,5 @@
 const request = require('request-promise');
-
+const assert = require('assert');
 /**
  * Class wraps Facebook Graph API requests
  */
@@ -12,6 +12,15 @@ class GraphAPI {
     json: true,
   });
 
+  /**
+   * Just to be sure that correct user passed
+   * @param user
+   */
+  static checkUser(user) {
+    assert(user, 'No user provided');
+    assert(user.id, 'User must have `id`');
+  }
+
   /**
    * Creates test user with passed `props`
    * @param props
@@ -25,43 +34,50 @@ class GraphAPI {
         installed: false,
         ...props,
       },
-    }).promise();
+    });
   }
 
   /**
-   * Deletes test user by id
-   * @param userId
-   * @returns {*}
+   * Deletes test user
+   * @param facebook user
+   * @returns {Promise<*>}
    */
-  static deleteTestUser(userId) {
+  static deleteTestUser(user) {
+    this.checkUser(user);
+
     return this.graphApi({
-      uri: `${userId}`,
+      uri: `${user.id}`,
       method: 'DELETE',
-    }).promise();
+    });
   }
 
   /**
    * Removes all Application permissions.
    * This only the way to De Authorize Application from user.
-   * @param userId
-   * @returns {Promise<void>}
+   * @param facebook user
+   * @returns {Promise<*>}
    */
-  static async deAuthApplication(userId) {
+  static deAuthApplication(user) {
+    this.checkUser(user);
+
     return this.graphApi({
-      uri: `/${userId}/permissions`,
+      uri: `/${user.id}/permissions`,
       method: 'DELETE',
     });
   }
 
   /**
    * Delete any Application permission from user.
-   * @param userId
+   * @param facebook user
    * @param permission
-   * @returns {Promise<void>}
+   * @returns {Promise<*>}
    */
-  static async deletePermission(userId, permission) {
+  static deletePermission(user, permission) {
+    this.checkUser(user);
+    assert(permission, 'No `permission` provided');
+
     return this.graphApi({
-      uri: `/${userId}/permissions/${permission}`,
+      uri: `/${user.id}/permissions/${permission}`,
       method: 'DELETE',
     });
   }