cronofy / cronofy-node

Node wrapper for the Cronofy API
https://docs.cronofy.com/developers
MIT License
49 stars 23 forks source link

Function calls modify options argument #67

Closed fastfedora closed 4 years ago

fastfedora commented 4 years ago

All the functions in this library modify the options object passed in as an argument to inject one or more additional properties. If this object is then re-used, the injected value will override any value set on the client itself.

The place where I ran into this bug is in code designed to catch when the token expires and automatically refresh the token, then re-execute the function. I wrap all relevant calls it the library with a function that catches 401 responses, refreshes the token and then re-executes the call:

export async function callWithTokenRefresh(
  userId: string,
  accountId: string,
  client: Cronofy,
  method: Function,
  args: any[],
  retryCount: number = 0,
) {
  try {
    return await method.apply(client, args);
  } catch (e) {
    if (e.statusCode !== 401) {
      throw e;
    }
    else if (retryCount > 1) {
      throw new HttpsError('permission-denied',
                           `Too many retries trying to refresh token for user ${userId}`);
    }

    await refreshAccessToken(userId, accountId, client);

    return await callWithTokenRefresh(userId, accountId, client, method, args, retryCount + 1);
  }
}

What this code does is to attempt to call a function on cronofy-node. If it receives a 401 error, it refreshes and saves a new acccess token, then attempts the call again.

The second call was failing, however, because the wrong access_token was being provided. I finally tracked it down to the options argument in args being modified by cronofy-node to add an access_token key, which was then used instead of client.config.access_token (which had the correct updated access token).

This bug report is just to document the bug. I'll be submitting a PR in a minute that fixes the issue.