badgateway / oauth2-client

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

OAuth2Client `server` value gets truncated to just the base URL #142

Closed ryancwalsh closed 3 months ago

ryancwalsh commented 3 months ago

I thought this would work (notice the value of "server"):

const client = new OAuth2Client({
  clientId: 'id',
  clientSecret: 'secret',
  server: 'https://api.schwabapi.com/v1/oauth',
});

export async function getAuthorizeUri() {
  return client.authorizationCode.getAuthorizeUri({
    redirectUri: schwabRedirectUri,
    scope: ['readonly'], 
  });
}

But then getAuthorizeUri() returns https://api.schwabapi.com/authorize?client_id=id&response_type=code&redirect_uri=________&scope=readonly, where the URL is missing /v1/oauth.

It should instead return https://api.schwabapi.com/v1/oauth/authorize?client_id=id&response_type=code&redirect_uri=________&scope=readonly.

A workaround would be for me to prepend /v1/oauth to the values for authorizationEndpoint and tokenEndpoint, but it would be convenient to just leave them appended to "server".

Am I misunderstanding something?

Thanks!

evert commented 3 months ago

To use https://api.schwabapi.com/v1/oauth/ as a base url and an authorize endpoint directly below it, use:

{
  server: 'https://api.schwabapi.com/v1/oauth/',  // note the final slash
  authorizationEndpoint: 'authorize' // no slash at the start
}

By default authorizationEndpoint is /authorize, and the standard rules for expanding relative urls are applied, like:

new URL(authorizationEndpoint, server);
ryancwalsh commented 3 months ago

Ah, yes!

Makes sense!

I'm curious:

https://github.com/badgateway/oauth2-client?tab=readme-ov-file#usage has defaults with preceding slashes:

  tokenEndpoint: '/token',
  authorizationEndpoint: '/authorize',

Any thoughts on making those defaults not have preceding slashes and instructing in the docs that the "server" should end in a slash?

The Oauth2 flows I've seen are always at a path like /v1/oauth/..., and I wonder if that's the case for most other people too, in which case it seems like default endpoints would be more convenient without the preceding slash.

I'm no expert, so it's just food for thought.

In any case, your suggestion works for me, so I'm all set.

Thanks!

evert commented 3 months ago

It's a fair point. Something to consider for a future BC break version. Also perhaps something that can be clarified in the docs.

I'm hoping more people will simply use the discovery document. Way better to just let the server tell the client where everything can be found!