Avoiding Global State - Real Example with Passport.js

28 Jul 2020 - JavaScript, TypeScript, Jest

Here is a quick example about issues created by global states in software.

Context

We all agree global states may be harmful if not carefully used. Unfortunately you may already use them without knowing. Recently I encountered a similar situation with Passport.js (it is an awesome authentication middleware for Node.js).

While unit testing the authentication layer of an API I was really surprised with the output. Here is a simplified test file to show you the issue because it took me a bunch of hours to understand the root cause of the issue in our code base.

For both tests, we create an Express application, attach it an authentication layer bound to a password, and then we send an HTTP request & look at the received cookies to see if everything is ok:

14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
// [...] skip modules

// tested Express app
const createApp = (appName, masterPassword) => {
  const app = express();
  app.use(passport.initialize());
  app.use(cookieSession({ secret: "secret" }));
  app.get("/login", passport.authenticate("basic"), (req, res) => {
    res.send();
  });

  passport.use(
    new BasicStrategy((username, password, done) => {
      if (password !== masterPassword) {
        return done(new Error("Unknown user"));
      } else {
        return done(null, username);
      }
    })
  );

  passport.serializeUser((user, callback) => {
    callback(null, `${appName}-${user}`);
  });

  return app;
};

const launchTest = async (appName, username, masterPassword) => {
  const agent = await request.agent(createApp(appName, masterPassword));

  const authHeader = `Basic ${Buffer.from(
    `${username}:${masterPassword}`
  ).toString("base64")}`;

  await agent.get("/login").set("Authorization", authHeader).expect(200);

  const sessionCookieValue = Buffer.from(
    agent.jar.getCookie("express:sess", CookieAccessInfo.All).value,
    "base64"
  ).toString("utf8");

  expect(JSON.parse(sessionCookieValue)).toStrictEqual({
    passport: { user: `${appName}-${username}` },
  });
};

test("Global state issue - 1", async () => {
  await launchTest("app1", "gabin", "password1");
});

test("Global state issue - 2", async () => {
  await launchTest("app2", "arthur", "password2");
});

Given the previous snippet, we expect the serialized users to be app1-gabin & app2-arthur but here is the output:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
 FAIL  test/global-states.test.ts
  ✓ Global state issue - 1 (16 ms)
  ✕ Global state issue - 2 (6 ms)

  ● Global state issue - 2

    expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Object {
        "passport": Object {
    -     "user": "app2-arthur",
    +     "user": "app1-arthur",
        },
      }

      54 |   ).toString("utf8");
      55 |
    > 56 |   expect(JSON.parse(sessionCookieValue)).toStrictEqual({
         |                                          ^
      57 |     passport: { user: `${appName}-${username}` },
      58 |   });
      59 | };

      at test/global-states.test.ts:56:42
      at fulfilled (test/global-states.test.ts:5:58)

It is surprising, isn’t it? I encourage you to try to find the issue by yourself.

Investigations

If you didn’t find it, here is a hint: look at the title of this blog post 😄

In the real code base it was the connection to the database (and not a string) that was shared between tests, and as stated in node-postgres all statements within a transaction must use the same client, otherwise 💥.

Now, as you can imagine, the passport variable (coming from import * as passport from "passport") has global states. If we look in its source code we can find the following about the serializeUser function:

1
2
3
4
5
6
Authenticator.prototype.serializeUser = function(fn, req, done) {
  if (typeof fn === 'function') {
    return this._serializers.push(fn);
  }

  // ...

Also, here is the exported value:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
var Passport = require('./authenticator')
  , SessionStrategy = require('./strategies/session');


/**
 * Export default singleton.
 *
 * @api public
 */
exports = module.exports = new Passport();

We can see that serializeUser is a stateful function in the Authenticator class (as it modify this._serializers array) and import * as passport from "passport" corresponds to a singleton of that class.

Quick & Dirty fix 😎

To validate our issue and “fix it” we can reset the global state manually before each test. It works, but it is a very bad idea for a lot of reasons: we don’t have to know about internals of that library, it may contain other global states, etc.

The associated patch for our 💩fix:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
diff --git a/js/packages/server/test/global-states.test.ts b/js/packages/server/test/global-states.test.ts
index 3884240..4713ba2 100644
--- a/js/packages/server/test/global-states.test.ts
+++ b/js/packages/server/test/global-states.test.ts
@@ -6,6 +6,10 @@ import { CookieAccessInfo } from "cookiejar";
 const express = require("express");
 const cookieSession = require("cookie-session");

+beforeEach(() => {
+  passport._serializers = [];
+});
+
 // tested Express app
 const createApp = (appName, masterPassword): Application => {
   const app = express();

As you can see you cannot keep that in your code. Other engineers will have a hard time to understand it and it may not survive on the long term.

Long Term Solution

Luckily as seen in above snippet, the exported value by Passport.js is an instance of the Authenticator class. Which means we can create an instance of it for each Express application:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
diff --git a/js/packages/server/test/global-states.test.ts b/js/packages/server/test/global-states.test.ts
index 3884240..6ba63e8 100644
--- a/js/packages/server/test/global-states.test.ts
+++ b/js/packages/server/test/global-states.test.ts
@@ -8,18 +8,20 @@ const cookieSession = require("cookie-session");

 // tested Express app
 const createApp = (appName: string, masterPassword: string): Application => {
+  const authenticator = new passport.Authenticator();
+
   const app = express();
-  app.use(passport.initialize());
+  app.use(authenticator.initialize());
   app.use(cookieSession({ secret: "secret" }));
   app.get(
     "/login",
-    passport.authenticate("basic"),
+    authenticator.authenticate("basic"),
     (req: Request, res: Response) => {
       res.send();
     }
   );

-  passport.use(
+  authenticator.use(
     new BasicStrategy((username, password, done) => {
       if (password !== masterPassword) {
         return done(new Error("Unknown user"));
@@ -29,7 +31,7 @@ const createApp = (appName: string, masterPassword: string): Application => {
     })
   );

-  passport.serializeUser((user, callback) => {
+  authenticator.serializeUser((user, callback) => {
     callback(null, `${appName}-${user}`);
   });

Now the tests are successful and with a good comment for the future readers you have a robust design.

Conclusion

Be careful with global states as they might be hidden in surprising places. Always write tests in your code: they will allow you to reduce the coupling, protect you during library upgrades and code changes, etc.

In that case, Passport.js exports by default a singleton instance to simplify the usage of the library. While not perfect for all use cases, it is still a great open-source library!