feat: Allow the api token issuer to be a fully qualified url with a path component #151
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 Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements configurable OAuth2 token endpoint handling by normalizing the Changes
Sequence DiagramsequenceDiagram
participant Config as OAuth2 Configuration
participant Client as OAuth2Client
participant Normalizer as ApiTokenIssuerNormalizer
participant Validator as Credentials.EnsureValid()
Config->>Normalizer: ApiTokenIssuer (raw input)
Note over Normalizer: Normalize: add https:// if no scheme
Normalizer-->>Client: normalized issuer URL
Client->>Client: Parse URL (extract host/port/path)
alt Path is empty or "/"
Client->>Client: Append "/oauth/token"
end
Client->>Client: Store as _apiTokenIssuer
Config->>Validator: Validate ApiTokenIssuer
Validator->>Normalizer: Normalize for validation
Normalizer-->>Validator: normalized URL
Validator->>Validator: Validate with IsWellFormedUriString
alt Invalid
Validator-->>Config: FgaValidationError
else Valid
Validator-->>Config: ✓
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR allows the API token issuer to accept a fully qualified URL with a path component. Previously, the SDK only supported domain-based issuer values and automatically appended /oauth/token. Now consumers can provide custom paths in the issuer URL while maintaining backward compatibility with domain-only configurations.
- Normalizes API token issuer URLs by adding https:// scheme when missing and preserving custom paths
- Updates OAuth2Client to handle full URLs instead of constructing them from domain fragments
- Adds comprehensive test coverage for various issuer URL formats
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/OpenFga.Sdk/Configuration/Credentials.cs |
Updates interface properties and validation logic to use new URL normalizer |
src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs |
New utility class that normalizes issuer URLs by adding scheme when missing |
src/OpenFga.Sdk/ApiClient/OAuth2Client.cs |
Modifies token exchange logic to use normalized URLs with path handling |
src/OpenFga.Sdk.Test/Configuration/ApiTokenIssuerNormalizerTests.cs |
Test coverage for URL normalization functionality |
src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs |
Test coverage for OAuth2Client path handling and alias fix for Configuration |
src/OpenFga.Sdk.Test/Api/OpenFgaApiTests.cs |
Updates Configuration class usage to use alias for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/OpenFga.Sdk/ApiClient/OAuth2Client.cs (1)
101-112: Consider simplifying the AbsolutePath check.The check
string.IsNullOrWhiteSpace(normalizedApiTokenIssuerUri.AbsolutePath)on line 109 may be redundant. For absolute URIs, theAbsolutePathproperty always returns at least"/"and never null or whitespace according to .NET documentation. You could simplify this to just checknormalizedApiTokenIssuerUri.AbsolutePath.Equals("/").Apply this diff to simplify the condition:
- var normalizedApiTokenIssuerUriWithPath = string.IsNullOrWhiteSpace(normalizedApiTokenIssuerUri.AbsolutePath) || normalizedApiTokenIssuerUri.AbsolutePath.Equals("/") + var normalizedApiTokenIssuerUriWithPath = normalizedApiTokenIssuerUri.AbsolutePath.Equals("/") ? new Uri(normalizedApiTokenIssuerUri, DEFAULT_API_TOKEN_ISSUER_PATH) : normalizedApiTokenIssuerUri;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/OpenFga.Sdk.Test/Api/OpenFgaApiTests.cs(27 hunks)src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs(8 hunks)src/OpenFga.Sdk.Test/Configuration/ApiTokenIssuerNormalizerTests.cs(1 hunks)src/OpenFga.Sdk/ApiClient/OAuth2Client.cs(3 hunks)src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs(1 hunks)src/OpenFga.Sdk/Configuration/Credentials.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/OpenFga.Sdk/ApiClient/OAuth2Client.cs (2)
src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs (2)
ApiTokenIssuerNormalizer(5-29)Normalize(14-28)src/OpenFga.Sdk/ApiClient/RequestBuilder.cs (1)
Uri(87-98)
src/OpenFga.Sdk.Test/Configuration/ApiTokenIssuerNormalizerTests.cs (2)
src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs (1)
Theory(310-357)src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs (2)
ApiTokenIssuerNormalizer(5-29)Normalize(14-28)
src/OpenFga.Sdk.Test/Api/OpenFgaApiTests.cs (1)
src/OpenFga.Sdk/Configuration/Configuration.cs (2)
Configuration(11-216)Configuration(18-24)
src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs (3)
src/OpenFga.Sdk/Configuration/Configuration.cs (2)
Configuration(11-216)Configuration(18-24)src/OpenFga.Sdk.Test/Configuration/ApiTokenIssuerNormalizerTests.cs (1)
Theory(7-26)src/OpenFga.Sdk/ApiClient/OAuth2Client.cs (5)
Task(133-161)Task(163-197)Task(204-213)OAuth2Client(20-216)OAuth2Client(87-126)
src/OpenFga.Sdk/Configuration/Credentials.cs (2)
src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs (1)
Credentials(32-46)src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs (2)
ApiTokenIssuerNormalizer(5-29)Normalize(14-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test net8.0 on ubuntu-latest
- GitHub Check: Test netcoreapp3.1 on windows-latest
- GitHub Check: Test net8.0 on windows-latest
- GitHub Check: Test net48 on windows-latest
- GitHub Check: Test net8.0 on macos-latest
- GitHub Check: Test net9.0 on ubuntu-latest
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
src/OpenFga.Sdk.Test/Api/OpenFgaApiTests.cs (1)
19-19: LGTM! Type alias improves code readability.The introduction of the
SdkConfigurationalias and its consistent use throughout the test file improves readability without changing functionality.src/OpenFga.Sdk/Configuration/ApiTokenIssuerNormalizer.cs (1)
14-28: LGTM! The normalization logic is correct.The implementation correctly handles null/whitespace input and prepends
https://when no scheme is present.src/OpenFga.Sdk/ApiClient/OAuth2Client.cs (1)
134-140: LGTM! Token endpoint construction is correct.Using the pre-constructed
_apiTokenIssuerasBasePathwith an emptyPathTemplatecorrectly implements the configurable token endpoint feature.src/OpenFga.Sdk.Test/ApiClient/OAuth2ClientTests.cs (2)
15-15: LGTM! Type alias improves consistency.The
SdkConfigurationalias aligns with the same change inOpenFgaApiTests.csand improves code readability.
308-357: LGTM! Comprehensive test coverage for token issuer path handling.The test thoroughly validates the ApiTokenIssuer normalization behavior across various scenarios:
- URLs with/without schemes
- URLs with/without ports
- URLs with/without custom paths
- URLs with trailing slashes
The test correctly captures the actual request URI and asserts it against expected values.
src/OpenFga.Sdk.Test/Configuration/ApiTokenIssuerNormalizerTests.cs (1)
7-26: Good test coverage for normalization behavior.The test cases comprehensively cover the expected normalization scenarios including edge cases like null/empty input, domains without schemes, domains with ports and paths.
src/OpenFga.Sdk/Configuration/Credentials.cs (4)
53-67: LGTM! Excellent documentation improvements.The enhanced documentation clearly explains the normalization behavior with helpful examples. This aligns well with the PR objectives and will help users understand how to configure the ApiTokenIssuer.
104-106: Verify constructor exception behavior is acceptable.The public parameterless constructor now calls
EnsureValid(), which can throwFgaRequiredParamErrororFgaValidationErrorduring object construction. Ensure this aligns with your error handling strategy, as some developers prefer constructors not to throw (preferring lazy validation instead).If this is intentional and documented, this is acceptable. Otherwise, consider moving validation to when the credentials are actually used.
147-154: LGTM! Validation logic correctly normalizes before checking.The validation properly normalizes the ApiTokenIssuer using
ApiTokenIssuerNormalizer.Normalizebefore validating it withIsWellFormedUriString. This ensures consistent validation behavior.
163-167: LGTM! URI validation logic is correct.The updated
IsWellFormedUriStringmethod correctly handles nullable input and validates that the URI has a valid HTTP/HTTPS scheme.
OsmanMElsayed
force-pushed
the
feat/allow-api-token-issuer-with-path
branch
from
e965563 to
6cf3ecc
Compare
OsmanMElsayed
force-pushed
the
feat/allow-api-token-issuer-with-path
branch
from
2ab2829 to
0f6f469
Compare
rhamzeh
left a comment
rhamzeh
left a comment
There was a problem hiding this comment.
Actually before merging, would you mind updating the README and adding a brief note to the unreleased section in the Changelog?
We haven't updated the READMEs of the rest of the SDKs but we should.
We can be as brief as possible, so something as simple as in the ClientCredentials example, adding a comment:
// API Token Issuer can contain:
// - a scheme, defaults to https
// - a path, defaults to /oauth/token
// - a port
ApiTokenIssuer = Environment.GetEnvironmentVariable("FGA_API_TOKEN_ISSUER"),
rhamzeh
dismissed
their stale review
Dismissing approval just till Changelog and README are updated
Done ✔️ . @rhamzeh |
…ath component (#151) * feat: Allow the api token issuer to be a fully qualified url with a path component * chore: Address PR comment
2 participants
Description
What problem is being solved?
openfga/sdk-generator#238
How is it being solved?
Allow the consume to provide a URL with a path component when setting the
ApiTokenIssuervalue, otherwise default to/oauth/token.The behavior of the
ApiTokenIssuermatches the expectations defined in the following table:What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
Bug Fixes
Tests