admin.auth().createCustomToken() is not correctly returning a rejected promise

[cee-chen]

Environment

• Operating System version: OSX 10.13.4
• Firebase SDK version: firebase-admin@5.11.0, firebase-functions@1.0.3, firebase-tools@3.18.5
• Library version: Node v6.11.5 on Cloud Functions, v8.10.0 locally (happens on both)
• Firebase Product: auth

Describe the problem

createCustomToken() correctly returns chained .then() resolved promises but not .catch() rejected errors. Nothing inside a .catch() block that immediately follows createCustomToken() fires at all - it's as if the promise just returns immediately without stopping (which no other Firebase promise that I've used so far has done).

This is frustrating for my use case because I'm trying to throw my own custom error/exception and not Firebase's.

Relevant Code:

const functions = require('firebase-functions');
const admin = require('firebase-admin');

admin.initializeApp(); // Must be initialized with credentials with a private_key in order to use createCustomToken

exports.notWorking = functions.https.onRequest((req, res) => {
  Promise.resolve() // This can be anything, for example admin.auth().getUserByEmail()
    .then(() => {
      return admin.auth().createCustomToken() // This is purposely left blank to simulate failure
        .then((customToken) => {
          res.status(201).json({
            name: 'Success',
            token: customToken,
          })
        })
        .catch((error) => {
          console.log('Never fires');
          throw new CustomException('My custom error message');
        })
    })
    .catch((error) => {
      console.error(`createCustomToken is immediately returning Firebase's error and not mine`);
      res.status(500).json(error);
    })
});

function CustomException(message) {
  this.name = 'CustomMessage';
  this.message = message;
}

Steps to reproduce:

1. Run the above code in Cloud Functions. Visit the endpoint directly. You should see the following JSON response:

{"code":"auth/argument-error","message":"First argument to createCustomToken() must be a non-empty string uid."}

• Note also that Never fires never outputs into the server-side logs.

---

2. Replace .createCustomToken() in the code above with another admin function, such as .createUser() (params deliberately left blank to invoke an error), or even a generic Promise.reject(). You should now see the following JSON response:

{"name":"CustomMessage","message":"Custom error message"}

• Note that Never fires is now being logged server-side, which means that the first/nested .catch() is now actually being respected.

---

I'm not an expert on promises or anything, so I'm not totally sure what the scenario for this is called (swallowed error?) - but it definitely doesn't seem like expected behavior for a promise to just ignore a .catch() block. I'd love for this to get fixed so I can handle .createCustomToken() with my own error messaging instead of having to use Firebase's.

[bojeil-google]

This is debatable. For example our JS client SDKs with promise returning APIs also throw synchronous errors when provided with invalid arguments. We aim to fail fast when you provide invalid types for arguments. I know there are other APIs that intentionally do this. Here is an example with the importUsers API:
https://github.com/firebase/firebase-admin-node/blob/master/src/auth/auth-api-request.ts#L587

[cee-chen]

Gotcha, thanks for the context! Maybe I need to read up a bit more on promises and throw vs Promise.reject(), but shouldn't throwing still be caught by a subsequent createCustomToken().catch() block? Or am I missing something?

My concern is that it makes the promise behavior feel a little unpredictable, but if it's determined it's 100% intentional, would it be possible to document the behavior?

[hiranya911]

To add to what @bojeil-google mentioned, how certain types of errors are handled in JS is largely a matter of preference and style. The style the Admin SDK strives to adhere to is:

• throw errors and fail fast in case of invalid arguments, and
• reject a promise for other runtime errors (e.g. back-end server errors).

In that sense the createCustomToken() is implemented correctly. But I think the SDK is not consistent across all its APIs when it comes to the above conventions. As you have noticed the createUser() method rejects a promise for all types of errors. We should probably seek out such instances, and fix them to be consistent with the rest of the SDK.

[cee-chen]

Thanks, it's good to know that pattern - would be fantastic to have in the docs somewhere perhaps. Agreed that consistency is usually best above all, and appreciate that potentially being addressed down the road.

Apologies again if this stems from me not fully understanding how promises work, and thank you in advance for taking any time out of your day to clarify. My confusion is that:

createCustomToken()
  .catch(() => { console.log('Never fires') })

is not working, whereas

createCustomToken('userId', {})
  .then(() => { throw new Error('This gets caught') }
  .catch((error) => console.log('Fires', error))

does.

Either way, it sounds like the behavior is potentially solved by never chaining .catch()s onto Firebase promises and instead relying on returning the promise and letting a parent catch handle all errors from there. That makes my step-by-step logging a little less granular than I'd like, but I'll find a way to structure my code around it. Thanks!

[hiranya911]

@constancecchen I would also like to draw your attention to #285. Today, createCustomToken() can only fail due to input errors (since the method is entirely a local operation), which are signaled by throwing exceptions. But this will change with #285. After that createCustomToken() may call remote back-end services, and related errors will be signaled by rejecting promises. If you must catch all types of errors, here's what I'd recommend:

try {
  admin.auth().createCustomToken(uid, claims)
    .then((token) => {})
    .catch((err) => {
      // handle runtime error
    });
} catch (err) {
  // Handle input/logic error.
  // You typically don't have to do this. These errors indicate developer errors or bugs in the
  // developer code, which are usually caught and fixed during testing. 
}

[cee-chen]

Thanks, good to know. Excited for that PR, by the way! :) Thanks for implementing it!

I'll modify my project to accommodate existing behavior since it's intentional. Also since it's intentional, consider this issue closed as wontfix, I suppose. I'll dig a bit more into promises some other time and try to get to the bottom of why a thrown error would skip being caught.