badgateway / oauth2-client

OAuth2 client for Node and browsers
https://www.npmjs.com/package/@badgateway/oauth2-client
MIT License
285 stars 34 forks source link

Fetch is not defined #63

Closed gsaravanakumar932 closed 2 years ago

gsaravanakumar932 commented 2 years ago

I checked the library, it is very useful and readme is understandable to anyone. Applause for that. I tried to run your source code, but unfortunately i got the error with client_credentials flow,

But then I checked your source code, there is no declaration for fetch anywhere in the files.

~\node_modules\@badgateway\oauth2-client\dist\client.js:157
        const resp = await fetch(uri, {
                     ^

ReferenceError: fetch is not defined;

A simple test file to reproduce the bug:

const { OAuth2Client} = require('@badgateway/oauth2-client');

const client = new OAuth2Client({ scope: [ 'email', 'offline_access', 'openid' ],

grantType: 'client_credentials', clientId: 'a*****', clientSecret: '**3', discoveryEndpoint: 'https://login.microsoftonline.com/*8/v2.0/.well-known/openid-configuration', tokenEndpoint: 'https://login.microsoftonline.com/*****************8/oauth2/v2.0/token', authorizationEndpoint: 'https://login.microsoftonline.com/*****************8/oauth2/v2.0/authorize' });

const express = require('express');

async function main() { const token = await client.clientCredentials(); console.log('called ::: ', token); }

const app = express();

app.listen(3001, () => { console.log( Msal Node Auth Code Sample app listening on port ${3001}! ); main(); });

Please help and provide a solution to fix it.

evert commented 2 years ago

Hi @gsaravanakumar932 ,

If fetch is not defined it means you're on an older version of Node. fetch() was added to node in version 18. But, there is a workaround, check out the documentation here:

https://github.com/badgateway/oauth2-client#support-for-older-node-versions

gsaravanakumar932 commented 2 years ago

if (!global.fetch) { const nodeFetch = require('node-fetch'); global.fetch = nodeFetch; global.Headers = nodeFetch.Headers; global.Request = nodeFetch.Request; global.Response = nodeFetch.Response; }

@evert Yes. I added the above code and that error gone. But getting the below error,

oauth2Code: 'invalid_request' message: 'OAuth2 error invalid_request. AADSTS90014: The required field 'scope' is missing from the credential. Ensure that you have all the necessary parameters for the login request.

But if you see my previous comment, i have already added scope.

evert commented 2 years ago

hi @gsaravanakumar932 ,

the scope parameter is passed with the clientCredentials function, not the constructor.

const token = await client.clientCredentials({
  scope: [
    'email',
    'offline_access',
    'openid'
  ],
});
gsaravanakumar932 commented 2 years ago

Working now. Thanks

mattbishop commented 2 years ago

@evert we are trying to use this in AWS lambda, which is pinned to Node 14. Would you consider peer-including node-fetch and adding these global updates to your codebase so we have a simpler path to using auth2-client?

evert commented 2 years ago

@mattbishop what issue are you having with the polyfill method? You could use this file as-is:

https://github.com/badgateway/oauth2-client/blob/main/test/polyfills.js

The trade-off I have to make is that either make more people have more dependencies (including browser users), or require Node 14 and 16 users to do a bit of extra work. This group will shrink over time at least =) peer dependencies are not optional

If the issue is simple that you don't like the noise of these extra lines of code, it shouldn't be too hard to make a small npm package that requires and re-exports everything from @badgateway/oauth2-client, depends on node-fetch and runs the polyfill by default.

mattbishop commented 2 years ago

Let me share the whole story. I recommended this lib to a team I'm working with that is building an integration that runs in AWS Lambda. They installed, ran into this error and reported back "it didn't work." We investigated and found this issue and the instructions to polyfill. They still didn't want to go forward and instead wanted to find a lib that "just worked." Not all developers are the fix-it-yourself variety, and perhaps that's not the audience you hope to reach.

Providing a workaround is OK, but for adoption, oauth2-client should be as close to drop-in usable as possible. The day when we can dispense with fetch polyfills is still years away, and many folks who need this lib won't be on 18 for a very long time.

evert commented 2 years ago

It's a trade-off I guess! One option could be to release a separate version for older node versions (say we use v2 for that, and v3 for Node 18+).

It does have the nice benefit of being super small. Requiring node-fetch for everyone it's no longer a 0 dependency package either.; which was one of the goals.

So idk! Maybe i'll change my mind :S

mattbishop commented 2 years ago

I like this; v2 could be old node with polyfill and animal sniffing for features, and v3 could be Node 18 and ESM?

evert commented 2 years ago

Node 18 and ESM

Need to look into ESM again! Last time the situation was a bit weird with typescript targetting it, but i believe it's better now?

mattbishop commented 2 years ago

Docs: https://www.typescriptlang.org/docs/handbook/esm-node.html