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

Add client credentials flow example to readme #640

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Conversation

sezanzeb
Copy link
Contributor

@sezanzeb sezanzeb commented Nov 30, 2023

As for the headline: names like "client credentials flow" "client credentials grant" and "client credentials grant flow" are flying around. I just went with the latter, idk.

@sezanzeb sezanzeb marked this pull request as ready for review November 30, 2023 09:07
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
});

const tokenSet = await client.grant({
audience: 'insert-your-audience',

This comment was marked as outdated.

This comment was marked as outdated.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Aranđel Šarenac <[email protected]>
README.md Outdated Show resolved Hide resolved
README.md Outdated
});

const tokenSet = await client.grant({
audience: 'insert-your-audience',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
audience: 'insert-your-audience',
resource: 'urn:example:third-party-api',

Copy link
Contributor Author

@sezanzeb sezanzeb Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into the auth0 web ui, the thing that jumps into view multiple times is the audience, and the quickstart shows it with an audience.

I tried to find something about this resource string in auth0 and by searching the internet, but couldn't. I also tried to look into the code, but the type of the grant function is just

export interface GrantBody {
  grant_type: string;

  [key: string]: unknown;
}

In there readme it mentions

client.authorizationUrl({
  scope: 'openid email profile',
  resource: 'https://my.api.example.com/resource/32178',

Is the resource in that case "32178"?

So I have no idea what the resource string is, and I definitely would want to see where to put the audience in the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auth0 mentions an urn in the context of SAML: https://auth0.com/blog/url-uri-urn-differences/

the wikipedia entry says SAML is important for SSO: https://en.wikipedia.org/wiki/Security_Assertion_Markup_Language

Is this really relevant for the client credentials flow?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@sezanzeb sezanzeb Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that (for example) auth0 doesn't implement this yet (https://community.auth0.com/t/support-rfc-8707/66169). I really think it would be beneficial to document both options. OAuth was very complicated (at least for me) to figure out, because of the plethora of variants, and I'm pretty sure I won't be the last person to search the docs and/or readme for "audience". So I think it's better to give more hints.

I made a commit that at least mentions both properties in docs/README.md

One could still also mention them in the readme like this, but as long as it is at least mentioned somewhere, I don't mind commiting your proposal without audience.

Suggested change
audience: 'insert-your-audience',
audience: 'example', // optional
resource: 'urn:example:third-party-api', // optional, for RFC 8707 compatibility

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sezanzeb This repo's history will show you that it's aim is to support the OIDC Conforming behaviour as RP.

That means couple of things:

  • SHOULD promote conforming use cases, like suggesting RFC 8707 way of asking for a resource when those OPs implement RFC 8707.
  • Should NOT promote any edge cases for different OP implementations around.
    Auth0 is OIDC provider vendor, but it's not conforming to 8707 spec as described.
    For example, I may not be using Auth0 as my OP, I may or may not be using grant function, and I don't want to be mislead into having to think about audience parameter at all in those cases.
    So that's a rationale why audience should not land in docs in this library. More like in your server implementation repo is where the place for those docs is.
  • Should NOT prevent you from passing extra parameters when those are required. It's also stated in types with this type descriptor: [key: string]: unknown;, that's why typescript is not complaining when you add audience. Also, you can look at source code and see that all arguments you define are in fact being passed to provider.

Copy link
Contributor Author

@sezanzeb sezanzeb Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should NOT promote any edge cases for different OP implementations around.

This is for giving hints for less knowledgeable people like me. I'm not a specialist for oauth, and my intention for using a library is, that it will help me set things up correctly without having to have a solid understanding of the thing at hand.

Actually, I don't remember where I even got the idea that I can insert audience there. Might just have been a random comment somewhere that I found by pure luck, or I probably used my previous code as a reference, which was making the request via axios and correctly used audience already.

AuthorizationParameters has audience?: string; in its interface by the way.

I made another commit, this time removing audience from README.md, and saying that audience is non-standard in docs/README.md

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@panva panva merged commit 76db75c into panva:main Dec 8, 2023
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants