-
Notifications
You must be signed in to change notification settings - Fork 22
feat: support custom OIDC token endpoint paths for Zitadel, Entra ID, and other providers #271
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request adds blank lines around the testConfig declaration in the test file, making a purely cosmetic formatting adjustment with no functional impact. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~1 minute Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for reviewing! |
|
Thanks for the PR @wizzfi1 Some issues:
|
2e0c5ca to
466fb81
Compare
466fb81 to
e98670f
Compare
e98670f to
02c8184
Compare
Thanks for the feedback! I've addressed all the issues: ✅ Fixed PR title/content mismatch - focused solely on OIDC token path functionality ✅ Added comprehensive tests - 7 test cases covering all OIDC scenarios from #238 ✅ Updated README and CHANGELOG - added documentation for the new feature The OIDC token path handling is now consistent with other SDKs and supports custom paths for providers like Zitadel and Entra ID." |
… and other providers - Normalize OIDC token URL construction to handle custom paths - Respect existing paths when provided (e.g., /oauth/v2/token) - Only append /oauth/token when no path is specified - Add proper URL parsing and validation for both domain-only and full URL issuers - Fix telemetry configuration issues - Fix client assertion audience for full URL issuers - Add comprehensive tests covering Zitadel, Entra ID, and custom path scenarios - Update README with OIDC configuration examples - Update CHANGELOG with new feature Fixes openfga#141 References openfga#238
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those things @wizzfi1! 🙏
If you don't mind, I left some additional feedback if you are willing to address. Also, if using AI to assist, please take a pass through as sometimes it will add superfluous and process-related comments, and many of them aren't valuable to keep in the codebase.
Thanks!
| ## [Unreleased](https://github.com/openfga/js-sdk/compare/v0.9.0...HEAD) | ||
|
|
||
| ### Added | ||
| - Support for custom OIDC token endpoint paths (#141, #238) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to reference openfga/sdk-generator#238?
CHANGELOG.md
Outdated
| - Compatibility with Zitadel, Entra ID, and other OIDC providers using non-standard token paths | ||
|
|
||
| ### Fixed | ||
| - OIDC token URL construction to handle custom paths consistently with other SDKs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "fixed" line entry this different than #141 referenced above? I think just having an "added" entry as you do above should suffice.
credentials/credentials.ts
Outdated
| userAgent: this.baseOptions?.headers["User-Agent"], | ||
| fgaMethod: "TokenExchange", | ||
| url, | ||
| url: tokenUrl, // FIXED: Use tokenUrl instead of undefined 'url' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't seem necessary, would you mind removing it?
credentials/credentials.ts
Outdated
| //Fixed audience: Handle both full URLs and domain-only issuers | ||
| const audienceIssuer = config.apiTokenIssuer.startsWith("http://") || config.apiTokenIssuer.startsWith("https://") | ||
| ? config.apiTokenIssuer | ||
| : `https://${config.apiTokenIssuer}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we have the same logic to check the scheme and prepend https:// if needed a few places here? Let's move that logic to its own internal function to not repeat if possible.
README.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| ### OIDC Token Endpoint Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a new section is needed for this info; it can be included in the "Client Credentials" subsection above. I think it can be a more concise non-vendor specific mention about the behavior and how a full path can be set and it will be respected.
README.md
Outdated
| This ensures compatibility with OIDC providers that use non-standard token endpoint paths. | ||
|
|
||
| ``` | ||
| ## 3. Create the OIDC Test File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to include this info or example tests in the README.
Updated the changelog to reflect new features and changes.
Removed OIDC Token Endpoint Configuration section and related details from README.
Refactored API token issuer normalization into a reusable function for improved code clarity and maintainability.
|
Thank you very much for your review! @jimmyjames I have implemented the changes. Please go through as I await your kind feedback. Thank you! |
|
|
||
|
|
||
| ## [Unreleased](https://github.com/openfga/js-sdk/compare/v0.9.0...HEAD) | ||
| ## [Unreleased](https://github.com/openfga/sdk-generator#238) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be changed, it would now link out to an issue in the generator repo. My comment below about referencing #238 from the generator repo was intended for that line, not here.
| }); | ||
| ``` | ||
|
|
||
| #### Default Behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The information here is very specific to the token endpoint handling for the client credentials grant. Having a separate section would be a bit confusing it should be info within the client credentials section.
| - `auth.example.com/oauth/v2` → `https://auth.example.com/oauth/v2` | ||
| - `https://auth.example.com/oauth/v2/token` → `https://auth.example.com/oauth/v2/token` | ||
|
|
||
| #### Provider Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to have specific vendor examples, general behavior documentation will be good. Also a full test file is not necessary just the token domain/path handling will suffice.
Summary
This PR fixes an inconsistency in how the JS SDK constructs the OIDC token endpoint URL.
/oauth/v2/token).Motivation
Addresses #141
(Inconsistency in OIDC token path construction between SDKs and incompatibility with custom token paths)
Notes
npm test✅).Summary by CodeRabbit
Note: No end-user visible changes in this release.