docusign / docusign-esign-node-client

The Official DocuSign Node.js Client Library used to interact with the eSign REST API. Send, sign, and approve documents using this client.
http://docusign.github.io/docusign-esign-node-client
MIT License
144 stars 99 forks source link

Generate Access Token SDK Method is broken since most recent RC Version #351

Open Ivanrenes opened 1 month ago

Ivanrenes commented 1 month ago

When trying to generate token with generateAccessToken using the client it just don't work, throws an empty error (see screenshot). With previous version it works as usual, anything changed for that?

image

Update: Just found the issue in your code, please merge this PR ASAP

sonawane-sanket commented 1 month ago

@Ivanrenes

Thank you for raising this issue and for submitting the PR with a solution. We've resolved this in release 7.0.1.

We genuinely appreciate your active participation and timely feedback. Please update to the latest version and let us know if you encounter any further problems.

Ivanrenes commented 1 month ago

@sonawane-sanket Please check https://github.com/docusign/docusign-esign-node-client/pull/352#pullrequestreview-2077517229 comment, generateAccessToken keeps broken

sonawane-sanket commented 1 month ago

@Ivanrenes I reviewed your comment. We'll validate it internally and get back to you at the earliest. Thank you for bringing this to our attention.

sonawane-sanket commented 1 month ago

@Ivanrenes We need some more information on how you're using our SDK. Are you loading it in a browser? It would be very helpful if you could share a sample code snippet so we can better understand the context and reproduce the issue on our end.

nikodunk commented 1 month ago

@sonawane-sanket Yep, broken now for us too on a node server integration. Following your docs you recommend doing:

const auth = await authenticate()
const dsApiClient = new docusign.ApiClient()
dsApiClient.setBasePath(auth.basePath)

is where it throws or RC2 with TypeError: Cannot read properties of undefined (reading 'basePath')

was auth.basePath removed?

sonawane-sanket commented 1 month ago

@sonawane-sanket Yep, broken now for us too on a node server integration. Following your docs you recommend doing:

const auth = await authenticate()
const dsApiClient = new docusign.ApiClient()
dsApiClient.setBasePath(auth.basePath)

is where it throws or RC2 with TypeError: Cannot read properties of undefined (reading 'basePath')

was auth.basePath removed?

Hey @nikodunk We recently released a new version, 7.0.1.

Could you please confirm if you are still experiencing the issue? And If the issue is not related to generating a token, can you please create a new ticket so we can close this one?

We would be happy to assist you further.

Ivanrenes commented 1 month ago

@sonawane-sanket will create a POC today.

nikodunk commented 3 weeks ago

is where it throws or RC2 with TypeError: Cannot read properties of undefined (reading 'basePath') was auth.basePath removed?

Hey @nikodunk We recently released a new version, 7.0.1.

Could you please confirm if you are still experiencing the issue? And If the issue is not related to generating a token, can you please create a new ticket so we can close this one?

Validated that this now works for us again in 7.0.2. Both the security alert, and the RC regression are now fixed for us. Thank you!

Ivanrenes commented 3 weeks ago

@nikodunk also this main issue topic here ?

nikodunk commented 3 weeks ago

I think so. The reproduction steps I described in my original comment above no longer bugs out on 7.0.2 (the path issues). I'm not sure I'd close this yet until we get a second positive though. Can you give 7.0.2 a spin and see if you can reproduce?

Ivanrenes commented 3 weeks ago

@nikodunk I found the issue niko,

If you use addDefaultHeader

const apiClient = new ApiClient();
apiClient.addDefaultHeader('Authorization', `Bearer ${tokensParsed.accessToken}`);

You are writing defaultHeaders in a global scope inside the library

  /**
   * Adds request headers to the API client. Useful for Authentication.
   */
  exports.prototype.addDefaultHeader = function addDefaultHeader(
    header,
    value
  ) {
    defaultHeaders[header] = value;
  };

So every time need to create a new client instace You must call addDefaultHeader

const apiClient = new ApiClient();
apiClient.setOAuthBasePath(this.docuSignKeys.oAuthApi);
apiClient.addDefaultHeader(
  'Authorization',
  `Basic ${Buffer.from(clientComb).toString('base64')}`
);

Because the way that you override headers in generateAccessToken

image

SOLUTION: So you MUST internally RESET global default headers every time a new api client instance is created to avoid this issue.

nikodunk commented 3 weeks ago

Are you saying if you do .addDefaultHeader, then it fixes the issue for you too?

PS I didn't come up with this btw, it's just what is (or was at some point) the recommended integration from the docusign docs.

zz-kivo commented 4 days ago

@sonawane-sanket This looks like a bug and a security issue to me. The docs show to set default headers for a given client but they are being applied at a global level and not set on the client instance. Here's an example of the issue:

import { ApiClient } from "docusign-esign";

const getClient = (token: string) => {
  const client = new ApiClient({
    basePath: "base_path",
    oAuthBasePath: "oauth-path",
  });
  client.addDefaultHeader("Authorization", `Bearer ${token}`);

  return client;
};

// request comes in for first user
const client1 = getClient("request1");
// request comes in for second user
const client2 = getClient("request2");

try {
  // first user makes a call
  await client1.generateAccessToken("clientId", "clientSecret", "code");
} catch (e: any) {
  // call was made with authorization token for second user
  console.log("ERROR", e.request.headers.authorization);
}
Ivanrenes commented 4 days ago

@zz-kivo I had the same issue, I created a PR fixing that but they just ignore it