feat(python): add OAuth2 scopes parameter support to CredentialConfiguration #612
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 WalkthroughAdds optional scopes support to credentials and OAuth2 token requests across async and sync clients. Introduces a scopes property in CredentialConfiguration, includes scope in POST params when provided, and adjusts sync token URL derivation via issuer parsing. Updates and adds unit tests covering scopes as list or string and a sync test signature change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Cred as CredentialConfiguration
participant OAuth as OAuth2 Client
participant REST as Token Endpoint
App->>Cred: Initialize (client_id, client_secret, issuer, audience, scopes?)
App->>OAuth: get_authentication_header()
note over OAuth: Build token request\n- issuer via parser (sync)\n- include scope if provided\n- list -> space-delimited
OAuth->>REST: POST /oauth/token\n{grant_type, client_id, client_secret, audience, [scope]}
REST-->>OAuth: {access_token, expires_in}
OAuth-->>App: Authorization: Bearer <token>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/clients/python/template/src/oauth2.py.mustache (1)
72-78: Normalize scopes and avoid sending empty/invalid scope paramGuard against empty strings and support common iterables (tuple/set). Keeps payload clean across providers.
- # Add scope parameter if scopes are configured - if configuration.scopes is not None: - if isinstance(configuration.scopes, list): - post_params["scope"] = " ".join(configuration.scopes) - else: - post_params["scope"] = configuration.scopes + # Add scope parameter if scopes are configured (normalize and skip empty) + scopes = configuration.scopes + if scopes: + if isinstance(scopes, str): + scope_str = scopes.strip() + elif isinstance(scopes, (list, tuple, set)): + scope_str = " ".join(s for s in scopes if isinstance(s, str) and s.strip()) + else: + scope_str = str(scopes).strip() + if scope_str: + post_params["scope"] = scope_strconfig/clients/python/template/src/sync/oauth2.py.mustache (1)
72-78: Mirror async normalization for scopesApply the same normalization/empty-skip logic here to keep clients consistent.
- # Add scope parameter if scopes are configured - if configuration.scopes is not None: - if isinstance(configuration.scopes, list): - post_params["scope"] = " ".join(configuration.scopes) - else: - post_params["scope"] = configuration.scopes + # Add scope parameter if scopes are configured (normalize and skip empty) + scopes = configuration.scopes + if scopes: + if isinstance(scopes, str): + scope_str = scopes.strip() + elif isinstance(scopes, (list, tuple, set)): + scope_str = " ".join(s for s in scopes if isinstance(s, str) and s.strip()) + else: + scope_str = str(scopes).strip() + if scope_str: + post_params["scope"] = scope_strconfig/clients/python/template/src/credentials.py.mustache (1)
112-125: Optional: normalize on set to reduce caller burdenTrimming strings and filtering empty items helps avoid sending empty scope later.
@scopes.setter def scopes(self, value): - """ - Update the scopes - """ - self._scopes = value + """ + Update the scopes (normalize; keep None for empty) + """ + if value is None: + self._scopes = None + elif isinstance(value, str): + self._scopes = value.strip() or None + elif isinstance(value, (list, tuple, set)): + cleaned = [s for s in value if isinstance(s, str) and s.strip()] + self._scopes = cleaned or None + else: + self._scopes = str(value).strip() or None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/clients/python/template/src/credentials.py.mustache(3 hunks)config/clients/python/template/src/oauth2.py.mustache(1 hunks)config/clients/python/template/src/sync/oauth2.py.mustache(2 hunks)config/clients/python/template/test/credentials_test.py.mustache(1 hunks)config/clients/python/template/test/oauth2_test.py.mustache(1 hunks)config/clients/python/template/test/sync/oauth2_test.py.mustache(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/clients/python/template/src/oauth2.py.mustacheconfig/clients/python/template/src/sync/oauth2.py.mustacheconfig/clients/python/template/test/credentials_test.py.mustacheconfig/clients/python/template/src/credentials.py.mustacheconfig/clients/python/template/test/oauth2_test.py.mustacheconfig/clients/python/template/test/sync/oauth2_test.py.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/clients/python/template/src/oauth2.py.mustacheconfig/clients/python/template/src/sync/oauth2.py.mustacheconfig/clients/python/template/test/credentials_test.py.mustacheconfig/clients/python/template/src/credentials.py.mustacheconfig/clients/python/template/test/oauth2_test.py.mustacheconfig/clients/python/template/test/sync/oauth2_test.py.mustache
🔇 Additional comments (10)
config/clients/python/template/src/sync/oauth2.py.mustache (1)
63-63: Good: derive token URL via issuer parserSwitching to
_parse_issuercentralizes issuer handling and aligns with async client behavior.config/clients/python/template/test/oauth2_test.py.mustache (2)
488-544: LGTM: covers scopes as listSolid assertions on Authorization header and payload shape (including scope).
545-601: LGTM: covers scopes as stringEnsures space-delimited string is forwarded verbatim.
config/clients/python/template/src/credentials.py.mustache (2)
22-22: Docstring addition looks goodParameter description is clear and matches behavior.
32-40: Constructor wiring is correct
scopesplumbed into_scopeswithout side effects.config/clients/python/template/test/credentials_test.py.mustache (2)
134-151: LGTM: list scopes path validatedConfirms method and stored scopes.
152-169: LGTM: string scopes path validatedCovers alternate input type.
config/clients/python/template/test/sync/oauth2_test.py.mustache (3)
215-215: Good change: sync retry test converted to sync defMatches synchronous client API.
266-321: LGTM: sync client with list scopesParallels async coverage and asserts scope in post params.
323-379: LGTM: sync client with string scopesCovers string-input path thoroughly.
…uration This is a backport of openfga/python-sdk#213
rhamzeh
force-pushed
the
feat/python-scopes
branch
from
54aeebb to
f69d325
Compare
…uration
This is a backport of openfga/python-sdk#213
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Bug Fixes