Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Couple of things not working as expected #111

Closed
brianjenkins94 opened this issue Jul 21, 2023 · 6 comments
Closed

Couple of things not working as expected #111

brianjenkins94 opened this issue Jul 21, 2023 · 6 comments

Comments

@brianjenkins94
Copy link

brianjenkins94 commented Jul 21, 2023

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
Copy link
Collaborator

evert commented Jul 21, 2023

// 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

@evert evert mentioned this issue Jul 21, 2023
@brianjenkins94
Copy link
Author

brianjenkins94 commented Jul 21, 2023

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
Copy link
Collaborator

evert commented Jul 21, 2023

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
Copy link
Author

brianjenkins94 commented Jul 21, 2023

jk, this worked with the trailing slash:

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

@evert
Copy link
Collaborator

evert commented Jul 21, 2023

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
Copy link
Author

Cheers, thanks for helping me work through it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants