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

Couple of things not working as expected #111

Closed brianjenkins94 closed 1 year ago

brianjenkins94 commented 1 year ago

I'm using authorization_code grant, and got it working like so:

const client = new OAuth2Client({
    "authorizationEndpoint": "/auth",
    "tokenEndpoint": new URL("./token", AUTH_URL).href,
    "server": "https://developer.foo.com/oauth",
    "clientId": INTEGRATION_KEY,
    "clientSecret": SECRET_KEY
});

From the README:

// OAuth2 client secret. Only required for 'client_credentials', 'password'
// flows. You should not specify this for authorization_code.

I'm not sure why this suggests not using the clientSecret. I needed it for it to work.


From the types:

/**
 * The hostname of the OAuth2 server.
 * If provided, we'll attempt to discover all the other related endpoints.
 *
 * If this is not desired, just specify the other endpoints manually.
 *
 * This url will also be used as the base URL for all other urls. This lets
 * you specify all the other urls as relative.
 */
server?: string;

I couldn't get relative URLs to work, and looking here, I think these need to be ./ instead of /.

This would also explain why I needed to specify tokenEndpoint at all, since the default should have worked.


From the README:

/**
 * You are responsible for implementing this function.
 * it's purpose is to supply the 'initial' oauth2 token.
 */
getNewToken: async () => {

  // Example
  return client.clientCredentials();

  // Another example
-  return client.authorizationCode({
    code: '..',
    redirectUri: '..',
  });

This needed to be client.authorizationCode.getToken().

evert commented 1 year ago

// OAuth2 client secret. Only required for 'client_credentials', 'password' // flows. You should not specify this for authorization_code.

I'm not sure why this suggests not using the clientSecret. I needed it for it to work.

Yes, this is wrong. Thanks!

I couldn't get relative URLs to work, and looking here, I think these need to be ./ instead of /.

This would also explain why I needed to specify tokenEndpoint at all, since the default should have worked.

Relative urls follow the same rules browsers do. So if https://foo/bar/ is your baseUrl, and you are navigating to /zim, the resulting URL is https://foo/zim, not https://foo/bar/zim. We don't blindly concatenate. Using ./zim would work, but so would zim without a slash. This is the same as (for example) filesystem paths.

This needed to be client.authorizationCode.getToken().

Thanks! Will also fix this

brianjenkins94 commented 1 year ago

I'm saying the ultimate URL I needed was https://developer.foo.com/oauth/token, but based on this:

case 'authorizationEndpoint':
  return resolve('/authorize', this.settings.server);

where resolve is defined as:

function resolve(uri: string, base?: string): string {
  return new URL(uri, base).toString();
}

if I set my server URL to https://developer.foo.com/oauth/, and based off of the docs saying:

/**
 * This url will also be used as the base URL for all other urls. This lets
 * you specify all the other urls as relative.
 */

I would have expected this:

const client = new OAuth2Client({
    "tokenEndpoint": "/token",
    "server": "https://developer.foo.com/oauth",

to yield https://developer.foo.com/oauth/token.

But I think I also see what you're saying, where:

new URL("./token", "https://developer.foo.com/oauth").toString() // => 'https://developer.foo.com/token'

which is not what I would have expected but, yeah, makes sense.


Then again, with a trailing slash:

new URL("./token", "https://developer.foo.com/oauth/").toString() // => 'https://developer.foo.com/oauth/token'

it yields what I was expecting but the way resolve is written wouldn't resolve in that way.

evert commented 1 year ago

Yes, the confusion makes sense but this is indeed by design and also how (for example) the HTML <base> tag behaves. Web standards have a pretty strong definition of how relative URIs should behave.

brianjenkins94 commented 1 year ago

jk, this worked with the trailing slash:

const client = new OAuth2Client({
    "authorizationEndpoint": "./auth",
    "tokenEndpoint": "./token",
    "server": "https://developer.foo.com/oauth/",
evert commented 1 year ago

Yes! I would write this as:

const client = new OAuth2Client({
    "authorizationEndpoint": "auth",
    "tokenEndpoint": "token",
    "server": "https://developer.foo.com/oauth/",

But it's identical to prefixing with ./

brianjenkins94 commented 1 year ago

Cheers, thanks for helping me work through it!