search

badgateway / oauth2-client

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

add optional idToken to OAuth2Token #118

Closed redguardtoo closed 1 year ago

redguardtoo commented 1 year ago

Hi, thanks for this nice project. There is one missing feature. Our oauth server also returns id_token which we use to get our user id.. So I add support for it.

evert commented 1 year ago

Hi!

I appreciate the PR but frankly I'm not entirely sure what to with this. The main issue is that id_token is an OpenID connect feature, and so far OpenID Connect has been out of scope for this project.

I've considered creating a second library called oidc-client that uses oauth2-client as a dependency and further implements OpenID Connect features, but this feels out of scope for now at least.

redguardtoo commented 1 year ago

I've considered creating a second library called oidc-client that uses oauth2-client as a dependency and further implements OpenID Connect features, but this feels out of scope for now at least.

Hi, @evert , thanks for the feedback. I'm sorry if I don't give enough context.

Right now I'm working on a heavy traffic web app for a non-profit organization.

Version 1 of our code follows standard workflow, we get access token, then call /userinfo end point to get customer id (cid).

The problem is the oauth service is a commercial 3rd party service. Our app's huge traffic costs my organization too much money on the service.

So version 2 of code removes the call of /userinfo because cid is also embedded in id_token

In our existing code we use the open source lib js-client-oauth2 which also returns "data" where id_token is embedded (see https://github.com/mulesoft-labs/js-client-oauth2/blob/master/src/client-oauth2.js#L302).

Now my task is to replace js-client-oauth2 because it's kind of abandoned and has some vulnerability.

So in version 3, I chose your excellent lib and has been working hard on it for more than a week. `My deadline is this Friday, and "id_token" is the last missing piece. So I will be very grateful if you can support id_token.

evert commented 1 year ago

Hi @redguardtoo ,

Your motivation makes sense, but 'I have a feature to deliver' is not a direct response to my main concern. However, I've given this some more thought and also realized that it's not super easy to extend the Client class with this behavior.

So I made a new PR #119, which moves the parser function inside the client class. Now you can subclass the client and add idToken yourself.

I hope this is a good enough compromise.

redguardtoo commented 1 year ago

Hi @redguardtoo ,

Your motivation makes sense, but 'I have a feature to deliver' is not a direct response to my main concern. However, I've given this some more thought and also realized that it's not super easy to extend the Client class with this behavior.

So I made a new PR #119, which moves the parser function inside the client class. Now you can subclass the client and add idToken yourself.

I hope this is a good enough compromise.

Ok, the new PR looks good. Thank you very much. You can close my pr.