feat: add OAuth2 scopes parameter support to CredentialConfiguration #213
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 OAuth2 scopes to CredentialConfiguration and includes the scope parameter in OAuth2 client_credentials token requests (async and sync). Updates tests to cover scopes provided as list or string. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant OAuth2Client
participant AuthServer as OAuth2 Token Endpoint
App->>OAuth2Client: get_authentication() with client_credentials
OAuth2Client->>OAuth2Client: Build form: client_id, client_secret, audience, grant_type
alt scopes provided
OAuth2Client->>OAuth2Client: If list -> join with spaces; else use string
OAuth2Client->>AuthServer: POST /oauth/token (form incl. scope)
else no scopes
OAuth2Client->>AuthServer: POST /oauth/token (form without scope)
end
AuthServer-->>OAuth2Client: 200 OK { access_token, expires_in }
OAuth2Client-->>App: Authorization: Bearer <token>
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@evansims Can you please point me to the relevant documentation section for this so I can make changes there? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
openfga_sdk/oauth2.py (1)
82-88: Normalize and only include non-empty scopes; support tuples as wellCurrent logic sends scope="" for empty list/string and doesn’t trim/normalize whitespace. Some providers reject empty scope. Normalize and include only when non-empty; allow tuples too.
- # 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 is not None: + if isinstance(scopes, (list, tuple)): + scope_str = " ".join(s for s in scopes if s) + else: + scope_str = " ".join(str(scopes).split()) + if scope_str: + post_params["scope"] = scope_stropenfga_sdk/sync/oauth2.py (2)
82-88: Normalize and only include non-empty scopes; support tuples as wellSame concern as async path: avoid sending scope="" and normalize whitespace; allow tuples.
- # 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 is not None: + if isinstance(scopes, (list, tuple)): + scope_str = " ".join(s for s in scopes if s) + else: + scope_str = " ".join(str(scopes).split()) + if scope_str: + post_params["scope"] = scope_str
73-74: Prefer using _parse_issuer to construct token_url (handles scheme and custom paths)Hardcoding https://{issuer}/oauth/token can break when api_issuer already includes a scheme or a full oauth endpoint. Using _parse_issuer (as in the async client) avoids duplication and handles inputs robustly.
- token_url = f"https://{configuration.api_issuer}/oauth/token" + token_url = self._credentials._parse_issuer(configuration.api_issuer)test/credentials_test.py (1)
110-145: Add edge-case tests for empty scopesConsider adding:
- scopes=[]
- scopes="" (and possibly " " with extra spaces)
Depending on desired behavior, assert either normalization to None (preferred) or that scope isn’t included in the token request.
openfga_sdk/credentials.py (1)
122-135: Normalize scopes in the setter; reject invalid types earlyNormalizing here simplifies callers and avoids sending empty/invalid values to OAuth servers. Recommend trimming, collapsing whitespace, dropping empty items, and accepting list/tuple; raise on invalid types.
- @scopes.setter - def scopes(self, value): - """ - Update the scopes - """ - self._scopes = value + @scopes.setter + def scopes(self, value): + """ + Update the scopes. Accepts a string or a list/tuple of strings. + Normalizes whitespace and drops empty items; sets to None if empty. + """ + if value is None: + self._scopes = None + return + if isinstance(value, (list, tuple)): + normalized = [str(s).strip() for s in value if s and str(s).strip()] + self._scopes = normalized if normalized else None + return + if isinstance(value, str): + s = " ".join(value.split()).strip() + self._scopes = s if s else None + return + raise ApiValueError("scopes must be a string or a list/tuple of strings")test/sync/oauth2_test.py (1)
164-219: Close REST client at the end of the testThe previous test closes rest_client; this one doesn’t. Add rest_client.close() to avoid leaking resources.
mock_request.assert_called_once_with( @@ }, ) + rest_client.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
openfga_sdk/credentials.py(3 hunks)openfga_sdk/oauth2.py(1 hunks)openfga_sdk/sync/oauth2.py(1 hunks)test/credentials_test.py(1 hunks)test/sync/oauth2_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/credentials_test.py (1)
openfga_sdk/credentials.py (16)
method(153-157)method(160-164)configuration(167-171)configuration(174-178)CredentialConfiguration(25-134)client_id(53-57)client_id(60-64)client_secret(67-71)client_secret(74-78)api_issuer(95-99)api_issuer(102-106)api_audience(81-85)api_audience(88-92)scopes(123-127)scopes(130-134)validate_credentials_config(207-238)
test/sync/oauth2_test.py (3)
test/oauth2_test.py (1)
mock_response(27-30)openfga_sdk/credentials.py (16)
Credentials(137-238)method(153-157)method(160-164)configuration(167-171)configuration(174-178)CredentialConfiguration(25-134)client_id(53-57)client_id(60-64)client_secret(67-71)client_secret(74-78)api_issuer(95-99)api_issuer(102-106)api_audience(81-85)api_audience(88-92)scopes(123-127)scopes(130-134)openfga_sdk/sync/oauth2.py (2)
OAuth2Client(45-161)get_authentication_header(153-161)
openfga_sdk/sync/oauth2.py (1)
openfga_sdk/credentials.py (4)
configuration(167-171)configuration(174-178)scopes(123-127)scopes(130-134)
openfga_sdk/oauth2.py (1)
openfga_sdk/credentials.py (4)
configuration(167-171)configuration(174-178)scopes(123-127)scopes(130-134)
🔇 Additional comments (3)
test/credentials_test.py (1)
110-145: LGTM: verifies scopes accepted as list and stringTests correctly assert preservation of the provided scopes value/type under client_credentials.
openfga_sdk/credentials.py (1)
33-51: CredentialConfiguration: scopes parameter addition — LGTMDocstring, init signature, and internal storage look good and backward compatible.
test/sync/oauth2_test.py (1)
107-163: LGTM: verifies scope serialization (list -> space-delimited)Solid assertions on request payload and headers; mirrors the base client_credentials test well.
| # 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 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add async tests for scopes handling
Sync path has good coverage; async path does not appear to be tested for scope serialization. Please add a mirror test in test/oauth2_test.py that asserts scope is form-encoded as a space-delimited string for list and string inputs.
🏁 Script executed:
#!/bin/bash
# Look for async OAuth2 tests that exercise scope serialization
rg -n -C2 -P 'from\s+openfga_sdk\.oauth2\s+import\s+OAuth2Client' test || true
# Check whether scope assertions exist in async tests
rg -n -C3 -P '\bscope\b' test/oauth2_test.py || trueLength of output: 415
Add synchronous and asynchronous tests for OAuth2Client scope serialization
A search of test/oauth2_test.py found no existing assertions for the “scope” parameter. Please extend that file with:
- Sync tests
• Verify that passingconfiguration.scopesas a list yields a space-delimited"scope"form field
• Verify that passing it as a single string is encoded unchanged - Async tests (
pytest.mark.asyncio)
• Mirror the two sync tests against the async token-request method
This fixes #207
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
Documentation
Tests