refined OAuth by leilanf#159
Merged
Merged
Conversation
Co-Authored-By: Jakub Filak <jakub@thefilaks.net> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds --token-url, --client-id, --client-secret to `sapcli config set-connection`, so users can store OAuth credentials on a connection without hand-editing ~/.sapcli/config.yml. The underlying CONNECTION_FIELDS list and the connection storage already handle these fields (introduced earlier with the OAuth seam); this change just makes them reachable from the CLI in the same way as every other connection field. Deliberately not exposed as global runtime flags. OAuth is a rare auth method in the ABAP world, and sapcli's config-file model (named connections / users / contexts, current-context, set-* commands) mirrors kubectl, where OIDC credentials likewise live only in kubeconfig — not as global flags on every command. doc/configuration.md gets: - a worked `sapcli config set-connection` example with OAuth flags in the Managing connections section, - the OAuth section reworded from "not exposed as command-line flags" to "not exposed as global flags" with an example showing how to set the values via the subcommand, - the explanatory paragraph clarifying that token_url / client_id / client_secret describe the OAuth application (per tenant, shared by team) and therefore live under connections:, not users:. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds OAuth 2.0 password-grant auth to sapcli: token acquisition, refresh, and caching; Bearer auth injection into HTTP sessions; CLI/config support for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Entry
participant Cache as Token Cache
participant OAuth as OAuth Server
participant ADT as SAP ADT
User->>CLI: Run command referencing connection
CLI->>Cache: get_cached_token(token_url, client_id)
alt cached valid access token
Cache-->>CLI: access token
else missing/expired
alt cached refresh token present
CLI->>OAuth: POST <token_url>/oauth/token (grant_type=refresh_token)
OAuth-->>CLI: new token response
CLI->>Cache: save token response
else no refresh token
CLI->>User: Prompt for password (if password_required)
User-->>CLI: returns password
CLI->>OAuth: POST <token_url>/oauth/token (grant_type=password, user, password)
OAuth-->>CLI: token response / error
CLI->>Cache: save token response (on success)
end
end
CLI->>ADT: Send request with Authorization: Bearer <token>
ADT-->>CLI: Response
sequenceDiagram
participant Config
participant CLI as Connection Init
participant Initializer as OAuthHTTPSessionInitializer
participant Session as HTTP Session
participant Cache as Token Cache
Config->>CLI: Provide connection (may include token_url, client_id, client_secret)
alt token_url configured
CLI->>Initializer: build OAuthHTTPSessionInitializer(...)
CLI->>Session: pass initializer into ADT Connection
Session->>Initializer: initialize_session(session)
Initializer->>Cache: get_token(...)
alt token available
Cache-->>Initializer: token
else fetch/refresh flows...
end
Initializer->>Session: attach BearerAuth
else token_url not configured
CLI->>Session: no initializer (BasicAuth behavior)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sap/config.py (1)
22-27:⚠️ Potential issue | 🟠 Major
client_secretis now persisted but not covered by sensitive-data safety checks.Line 26 adds
client_secretto connection data, but_has_passwords()still only checkspassword. As a result, world-readable file warnings and merge-source credential warnings won’t trigger for OAuth secrets.🔧 Proposed fix
-def _has_passwords(config_data: dict) -> bool: - """Check if the config contains any password fields.""" +def _has_sensitive_credentials(config_data: dict) -> bool: + """Check if the config contains password/client_secret fields.""" users = config_data.get('users', {}) if isinstance(users, dict): for user_def in users.values(): if isinstance(user_def, dict) and user_def.get('password'): return True + connections = config_data.get('connections', {}) + if isinstance(connections, dict): + for conn_def in connections.values(): + if isinstance(conn_def, dict) and conn_def.get('client_secret'): + return True + contexts = config_data.get('contexts', {}) if isinstance(contexts, dict): for context_def in contexts.values(): - if isinstance(context_def, dict) and context_def.get('password'): + if ( + isinstance(context_def, dict) + and (context_def.get('password') or context_def.get('client_secret')) + ): return True return False- if _has_passwords(data): + if _has_sensitive_credentials(data): _check_file_permissions(path)- if _has_passwords(data): + if _has_sensitive_credentials(data): warnings.warn( - f'Source configuration from {url} contains passwords. ' + f'Source configuration from {url} contains credentials. ' 'Shared configuration files should not contain credentials.', UserWarning, stacklevel=2 )- if _has_passwords(data): + if _has_sensitive_credentials(data): warnings.warn( - f'Source configuration file {path} contains passwords. ' + f'Source configuration file {path} contains credentials. ' 'Shared configuration files should not contain credentials.', UserWarning, stacklevel=2 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/config.py` around lines 22 - 27, CONNECTION_FIELDS was extended with 'client_secret' but the sensitive-data detection still only inspects 'password' via _has_passwords(), so add 'client_secret' (and any other OAuth secret keys you want to treat as sensitive) to the check in _has_passwords() or refactor _has_passwords() to check CONNECTION_FIELDS for known secret names; update the function that emits world-readable file or merge-source credential warnings (the code that calls _has_passwords()) to include 'client_secret' so OAuth secrets trigger the same safety warnings as passwords.
🧹 Nitpick comments (6)
test/unit/test_sap_cli.py (1)
761-775: Assert OAuth initializer arguments, not only its type.
isinstanceis helpful, but it won’t catch wrongtoken_url/client_id/client_secretwiring. Consider asserting constructor arguments and that the returned initializer is passed through tosap.adt.Connection.✅ Suggested test tightening
def test_oauth_initializer_when_token_url_present(self): - from sap.http.oauth import OAuthHTTPSessionInitializer args = self._make_args( token_url='https://auth.example.com', client_id='cid', client_secret='csec', ) - with patch('sap.adt.Connection') as mock_connection: + with patch('sap.http.oauth.OAuthHTTPSessionInitializer') as mock_initializer: + with patch('sap.adt.Connection') as mock_connection: + sap.cli.adt_connection_from_args(args) + + mock_initializer.assert_called_once_with( + 'https://auth.example.com', 'cid', 'csec', 'USR', 'pwd' + ) + + _, kwargs = mock_connection.call_args + self.assertIs(kwargs.get('session_initializer'), mock_initializer.return_value) - sap.cli.adt_connection_from_args(args) - - _, kwargs = mock_connection.call_args - initializer = kwargs.get('session_initializer') - self.assertIsInstance(initializer, OAuthHTTPSessionInitializer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_cli.py` around lines 761 - 775, Enhance the test test_oauth_initializer_when_token_url_present to not only assert the type but also verify the OAuthHTTPSessionInitializer was constructed with the correct token_url, client_id, and client_secret and that the exact initializer instance is passed to sap.adt.Connection: after calling sap.cli.adt_connection_from_args(args) inspect mock_connection.call_args to get the session_initializer, check it is the same object returned/created (or inspect its attributes/token_url/client_id/client_secret) and assert those values match the args provided (token_url='https://auth.example.com', client_id='cid', client_secret='csec') so wiring is validated.sap/http/oauth.py (4)
61-69: Missing return type annotation.
get_cached_refresh_tokenis missing the return type annotation for consistency withget_cached_token.💡 Suggested fix
-def get_cached_refresh_token(token_url: str, client_id: str): +def get_cached_refresh_token(token_url: str, client_id: str) -> Optional[str]: """Return the cached refresh token, or None."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/oauth.py` around lines 61 - 69, get_cached_refresh_token is missing a return type annotation; update its signature to include the proper return type (e.g., Optional[str]) to match the style used by get_cached_token. Modify the def get_cached_refresh_token(token_url: str, client_id: str) to include the return type annotation and import typing.Optional if needed, ensuring the function still returns None or token.refresh_token as before.
137-150: Potential None propagation to password grant.If no cached token exists and no refresh token is available,
fetch_token_with_credentialswill be called withuser=None, password=None, which will fail at the OAuth server. Consider documenting this contract or adding an explicit check.💡 Suggested improvement
def get_token(token_url, client_id, client_secret, user=None, password=None): - """Return a valid Bearer token - from cache, refresh, or credentials grant.""" + """Return a valid Bearer token - from cache, refresh, or credentials grant. + + If no cached token or refresh token is available, user and password must + be provided for the password grant flow. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/oauth.py` around lines 137 - 150, get_token may call fetch_token_with_credentials with user=None or password=None which will cause a failing OAuth request; update get_token (or its callers) to validate credentials before calling fetch_token_with_credentials: if user or password is missing, either raise a clear ValueError (e.g., "user and password required for password grant") or return a documented error path, and include this behavior in the function docstring so callers know the contract; reference the get_token and fetch_token_with_credentials functions when adding the validation and error message.
109-131: Missing type annotations.Function parameters and return type are not annotated, unlike
get_cached_tokenand similar functions.💡 Suggested type hints
-def fetch_token_with_credentials(token_url, client_id, client_secret, user, password): +def fetch_token_with_credentials( + token_url: str, client_id: str, client_secret: str, user: str, password: str +) -> str: """Obtain a Bearer token via OAuth 2.0 password grant using provided credentials."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/oauth.py` around lines 109 - 131, Add missing type annotations to fetch_token_with_credentials: annotate token_url, client_id, client_secret, user, and password as str and the return type as str (i.e., def fetch_token_with_credentials(token_url: str, client_id: str, client_secret: str, user: str, password: str) -> str:). Ensure any helper calls like save_token_response(token_url, client_id, token_data) still accept the same typed arguments; add imports from typing only if you choose non-primitive types later.
89-103: Handle JSON decode errors in both OAuth token functions.Both
refresh_access_token(line 100) andfetch_token_with_credentials(line 128) callresponse.json()without error handling. If an OAuth server returns a success status with malformed JSON (possible with misconfigured proxies), aJSONDecodeErrorwill propagate uncaught. Additionally, both functions accesstoken_data['access_token']without guarding againstKeyErrorif the key is missing.For
refresh_access_token, defensive handling is consistent with the function's pattern of returningNoneon failure:🛡️ Suggested defensive handling
if not response.ok: return None - token_data = response.json() + try: + token_data = response.json() + access_token = token_data['access_token'] + except (ValueError, KeyError): + return None save_token_response(token_url, client_id, token_data) - return token_data['access_token'] + return access_tokenFor
fetch_token_with_credentials, follow the pattern of raisingOAuthTokenErrorper the coding guidelines:if not response.ok: raise OAuthTokenError( f'OAuth login failed ({response.status_code}): {response.text}' ) - token_data = response.json() + try: + token_data = response.json() + access_token = token_data['access_token'] + except (ValueError, KeyError) as e: + raise OAuthTokenError(f'Invalid OAuth response: {response.text}') from e save_token_response(token_url, client_id, token_data) - return token_data['access_token'] + return access_tokenThis pattern is already used elsewhere in the codebase (e.g.,
sap/rest/gcts/repo_task.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/oauth.py` around lines 89 - 103, Both OAuth token functions must defensively handle malformed JSON and missing keys: in refresh_access_token wrap the response.json() call in a try/except catching JSONDecodeError and KeyError and return None on any such failure (keeping its current failure contract), and in fetch_token_with_credentials wrap response.json() similarly but raise OAuthTokenError with a clear message and include the response status/text or underlying exception; ensure you still call save_token_response only when token_data parsing succeeds and access_token key exists. Use the function names refresh_access_token and fetch_token_with_credentials and the OAuthTokenError class to locate and implement these changes.sap/http/token_cache.py (1)
198-211: Consider adding brief comments explaining OSError suppression.The silent
passin both_harden_dirand_harden_fileis intentional best-effort hardening, but per coding guidelines, a brief comment would clarify why exceptions are swallowed (e.g., "best-effort hardening; non-fatal if chmod fails").💡 Suggested clarification
def _harden_dir(path: Path) -> None: if os.name == "posix": try: path.chmod(stat.S_IRWXU) # 0o700 except OSError: - pass + pass # Best-effort; non-fatal if chmod fails (e.g., network FS) def _harden_file(path: Path) -> None: if os.name == "posix": try: path.chmod(stat.S_IRUSR | stat.S_IWUSR) # 0o600 except OSError: - pass + pass # Best-effort; non-fatal if chmod fails (e.g., network FS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/token_cache.py` around lines 198 - 211, Add brief inline comments inside _harden_dir and _harden_file explaining why OSError is intentionally suppressed (e.g., "best-effort hardening; non-fatal if chmod fails" or similar), so readers know the silent pass is deliberate; update the comments adjacent to the try/except blocks in those functions (_harden_dir and _harden_file) and keep behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/configuration.md`:
- Line 318: Replace the phrase "user name" with the single-word "username"
throughout the configuration text where it refers to the config key and
descriptions (e.g., the sentence that mentions "alongside `ashost`/`port`, while
your user name still belongs under `users:`" and the other instances), ensuring
inline code and prose use "username" consistently and update any nearby
occurrences that describe the same concept for uniform wording.
---
Outside diff comments:
In `@sap/config.py`:
- Around line 22-27: CONNECTION_FIELDS was extended with 'client_secret' but the
sensitive-data detection still only inspects 'password' via _has_passwords(), so
add 'client_secret' (and any other OAuth secret keys you want to treat as
sensitive) to the check in _has_passwords() or refactor _has_passwords() to
check CONNECTION_FIELDS for known secret names; update the function that emits
world-readable file or merge-source credential warnings (the code that calls
_has_passwords()) to include 'client_secret' so OAuth secrets trigger the same
safety warnings as passwords.
---
Nitpick comments:
In `@sap/http/oauth.py`:
- Around line 61-69: get_cached_refresh_token is missing a return type
annotation; update its signature to include the proper return type (e.g.,
Optional[str]) to match the style used by get_cached_token. Modify the def
get_cached_refresh_token(token_url: str, client_id: str) to include the return
type annotation and import typing.Optional if needed, ensuring the function
still returns None or token.refresh_token as before.
- Around line 137-150: get_token may call fetch_token_with_credentials with
user=None or password=None which will cause a failing OAuth request; update
get_token (or its callers) to validate credentials before calling
fetch_token_with_credentials: if user or password is missing, either raise a
clear ValueError (e.g., "user and password required for password grant") or
return a documented error path, and include this behavior in the function
docstring so callers know the contract; reference the get_token and
fetch_token_with_credentials functions when adding the validation and error
message.
- Around line 109-131: Add missing type annotations to
fetch_token_with_credentials: annotate token_url, client_id, client_secret,
user, and password as str and the return type as str (i.e., def
fetch_token_with_credentials(token_url: str, client_id: str, client_secret: str,
user: str, password: str) -> str:). Ensure any helper calls like
save_token_response(token_url, client_id, token_data) still accept the same
typed arguments; add imports from typing only if you choose non-primitive types
later.
- Around line 89-103: Both OAuth token functions must defensively handle
malformed JSON and missing keys: in refresh_access_token wrap the
response.json() call in a try/except catching JSONDecodeError and KeyError and
return None on any such failure (keeping its current failure contract), and in
fetch_token_with_credentials wrap response.json() similarly but raise
OAuthTokenError with a clear message and include the response status/text or
underlying exception; ensure you still call save_token_response only when
token_data parsing succeeds and access_token key exists. Use the function names
refresh_access_token and fetch_token_with_credentials and the OAuthTokenError
class to locate and implement these changes.
In `@sap/http/token_cache.py`:
- Around line 198-211: Add brief inline comments inside _harden_dir and
_harden_file explaining why OSError is intentionally suppressed (e.g.,
"best-effort hardening; non-fatal if chmod fails" or similar), so readers know
the silent pass is deliberate; update the comments adjacent to the try/except
blocks in those functions (_harden_dir and _harden_file) and keep behavior
unchanged.
In `@test/unit/test_sap_cli.py`:
- Around line 761-775: Enhance the test
test_oauth_initializer_when_token_url_present to not only assert the type but
also verify the OAuthHTTPSessionInitializer was constructed with the correct
token_url, client_id, and client_secret and that the exact initializer instance
is passed to sap.adt.Connection: after calling
sap.cli.adt_connection_from_args(args) inspect mock_connection.call_args to get
the session_initializer, check it is the same object returned/created (or
inspect its attributes/token_url/client_id/client_secret) and assert those
values match the args provided (token_url='https://auth.example.com',
client_id='cid', client_secret='csec') so wiring is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b07c59c3-b6f3-49e8-8260-64babffd8e74
📒 Files selected for processing (12)
doc/configuration.mdsap/cli/__init__.pysap/cli/_entry.pysap/cli/config.pysap/config.pysap/http/oauth.pysap/http/token_cache.pytest/unit/mock.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli_config.pytest/unit/test_sap_http_oauth.pytest/unit/test_sap_http_token_cache.py
|
|
||
| These fields describe the OAuth **application** registration on the target | ||
| system, not the individual user — that is why they sit under `connections:` | ||
| alongside `ashost`/`port`, while your user name still belongs under `users:`. |
There was a problem hiding this comment.
Use username consistently for wording polish.
These lines currently use “user name”; “username” is the more consistent form in technical docs.
Also applies to: 325-325, 336-336
🧰 Tools
🪛 LanguageTool
[style] ~318-~318: It’s more common nowadays to write this noun as one word.
Context: ...alongsideashost/port, while your user name still belongs under users:`. A typical...
(RECOMMENDED_COMPOUNDS)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/configuration.md` at line 318, Replace the phrase "user name" with the
single-word "username" throughout the configuration text where it refers to the
config key and descriptions (e.g., the sentence that mentions "alongside
`ashost`/`port`, while your user name still belongs under `users:`" and the
other instances), ensuring inline code and prose use "username" consistently and
update any nearby occurrences that describe the same concept for uniform
wording.
I was not so sure ~/.sapcli/tokens.json is the right place, I wanted to put it into the right directory (e.g. ~/.local/state/ etc.). The token store was used to be able to add more secure store in the future.
jfilak
force-pushed
the
oauth_by_leilanf
branch
from
ab28f17 to
3ab0cbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/unit/test_sap_http_oauth.py (1)
158-163: ⚡ Quick winStrengthen the default-auth assertion.
build_unauthorized_error()returningUnauthorizedErroris not BasicAuth-specific, so this test would still pass if the default initializer selection regressed. Assert the concrete initializer type, or another BasicAuth-specific behavior, instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_http_oauth.py` around lines 158 - 163, The test currently only checks that client._session_initializer.build_unauthorized_error(...) returns UnauthorizedError which is not BasicAuth-specific; change the assertion to verify the concrete initializer type used by HTTPClient by asserting isinstance(client._session_initializer, BasicAuthSessionInitializer) (or, if the concrete class is named differently in your codebase, use that class name) to ensure the default initializer is BasicAuth; alternatively assert a BasicAuth-specific behavior such as calling client._session_initializer.build_auth_header(...) and verifying the header starts with "Basic ". Ensure you reference the HTTPClient and _session_initializer symbols in the updated assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sap/http/oauth.py`:
- Around line 45-46: The cache key currently built by _cache_key(token_url,
client_id) must include the username to avoid reusing a password-grant token
across different users; change _cache_key to accept a username and include it in
the returned string, then update password_required() to accept a username
parameter and use the new _cache_key(username, token_url, client_id) when
checking/setting the cache; propagate the username argument through all callers
of password_required() and any other places that call _cache_key (e.g., the
other occurrence handling token caching) so the prompt decision and stored
tokens are scoped per user.
- Around line 142-155: The get_token function currently calls
fetch_token_with_credentials even when user/password are None, causing a 400 and
traceback; update get_token to validate that user and password are provided
before invoking fetch_token_with_credentials and if they are missing raise a
sap.errors.SAPCliError (or a subclass) with a clear re-authentication message;
keep the existing cache and refresh logic intact and reference get_token,
get_cached_token, get_cached_refresh_token, refresh_access_token, and
fetch_token_with_credentials when making the change.
In `@sap/http/token_cache.py`:
- Around line 138-142: The token cache reader currently only catches OSError,
ValueError, and KeyError when calling Token.from_json, but Token.from_json can
also raise TypeError for malformed/missing fields; update the exception handling
in the token load block (where Token.from_json(...) is called in token_cache.py)
to also catch TypeError and return None so malformed cache entries are treated
as cache misses rather than crashing the OAuth flow.
---
Nitpick comments:
In `@test/unit/test_sap_http_oauth.py`:
- Around line 158-163: The test currently only checks that
client._session_initializer.build_unauthorized_error(...) returns
UnauthorizedError which is not BasicAuth-specific; change the assertion to
verify the concrete initializer type used by HTTPClient by asserting
isinstance(client._session_initializer, BasicAuthSessionInitializer) (or, if the
concrete class is named differently in your codebase, use that class name) to
ensure the default initializer is BasicAuth; alternatively assert a
BasicAuth-specific behavior such as calling
client._session_initializer.build_auth_header(...) and verifying the header
starts with "Basic ". Ensure you reference the HTTPClient and
_session_initializer symbols in the updated assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed2c5140-36a1-4e20-8f4c-826b47e57476
📒 Files selected for processing (11)
pyproject.tomlrequirements.txtsap/cli/__init__.pysap/cli/_entry.pysap/http/oauth.pysap/http/token_cache.pytest/unit/mock.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_http_oauth.pytest/unit/test_sap_http_token_cache.py
✅ Files skipped from review due to trivial changes (2)
- requirements.txt
- pyproject.toml
Comment on lines
+45
to
+46
| def _cache_key(token_url: str, client_id: str) -> str: | ||
| return f'{token_url}|{client_id}' |
There was a problem hiding this comment.
Scope the cache and prompt decision by username.
Keying only on token_url|client_id lets one user's password-grant token be reused for a different username that shares the same client, which can send requests under the wrong account. password_required() makes the same assumption, so the prompt decision can also be wrong. Include the username in the cache key and thread it through password_required() and its callers.
🛠️ Proposed fix
-def _cache_key(token_url: str, client_id: str) -> str:
- return f'{token_url}|{client_id}'
+def _cache_key(token_url: str, client_id: str, user: str) -> str:
+ return f'{token_url}|{client_id}|{user}'-def password_required(token_url: Optional[str], client_id: Optional[str]) -> bool:
+def password_required(token_url: Optional[str], client_id: Optional[str], user: Optional[str]) -> bool:Also applies to: 197-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/http/oauth.py` around lines 45 - 46, The cache key currently built by
_cache_key(token_url, client_id) must include the username to avoid reusing a
password-grant token across different users; change _cache_key to accept a
username and include it in the returned string, then update password_required()
to accept a username parameter and use the new _cache_key(username, token_url,
client_id) when checking/setting the cache; propagate the username argument
through all callers of password_required() and any other places that call
_cache_key (e.g., the other occurrence handling token caching) so the prompt
decision and stored tokens are scoped per user.
Comment on lines
+94
to
+135
| def refresh_access_token(token_url, client_id, client_secret, refresh_token): | ||
| """Try to swap a refresh token for a new access token. Returns None on failure.""" | ||
|
|
||
| response = requests.post( | ||
| token_url.rstrip('/') + '/oauth/token', | ||
| auth=(client_id, client_secret), | ||
| data={'grant_type': 'refresh_token', 'refresh_token': refresh_token}, | ||
| timeout=30, | ||
| ) | ||
| if not response.ok: | ||
| return None | ||
| token_data = response.json() | ||
| save_token_response(token_url, client_id, token_data) | ||
| return token_data['access_token'] | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Interactive password grant | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def fetch_token_with_credentials(token_url, client_id, client_secret, user, password): | ||
| """Obtain a Bearer token via OAuth 2.0 password grant using provided credentials.""" | ||
|
|
||
| response = requests.post( | ||
| token_url.rstrip('/') + '/oauth/token', | ||
| auth=(client_id, client_secret), | ||
| data={ | ||
| 'grant_type': 'password', | ||
| 'username': user, | ||
| 'password': password, | ||
| }, | ||
| timeout=30, | ||
| ) | ||
|
|
||
| if not response.ok: | ||
| raise OAuthTokenError( | ||
| f'OAuth login failed ({response.status_code}): {response.text}' | ||
| ) | ||
|
|
||
| token_data = response.json() | ||
| save_token_response(token_url, client_id, token_data) | ||
| return token_data['access_token'] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the oauth.py file
find . -name "oauth.py" -type f | head -20Repository: jfilak/sapcli
Length of output: 77
🏁 Script executed:
# Check the structure of the sap directory
ls -la sap/Repository: jfilak/sapcli
Length of output: 807
🏁 Script executed:
# Look at the errors module
find . -name "errors.py" -type f | head -20Repository: jfilak/sapcli
Length of output: 203
🏁 Script executed:
# Read the oauth.py file to see the current implementation
wc -l sap/http/oauth.pyRepository: jfilak/sapcli
Length of output: 79
🏁 Script executed:
# Read the oauth.py file content
cat -n sap/http/oauth.pyRepository: jfilak/sapcli
Length of output: 8126
🏁 Script executed:
# Check the main errors module
cat -n sap/errors.pyRepository: jfilak/sapcli
Length of output: 1260
🏁 Script executed:
# Check the http-specific errors module
cat -n sap/http/errors.pyRepository: jfilak/sapcli
Length of output: 3370
🏁 Script executed:
# Check if there are any tests for these functions to see expected behavior
find . -name "*test*oauth*" -o -name "*oauth*test*" | head -20Repository: jfilak/sapcli
Length of output: 92
🏁 Script executed:
# Check if there are any other places in the codebase that call response.json() without error handling
rg "response\.json\(\)" --type py -A 2 -B 2 | head -60Repository: jfilak/sapcli
Length of output: 2947
🏁 Script executed:
# Verify save_token_response doesn't have validation already
sed -n '74,88p' sap/http/oauth.pyRepository: jfilak/sapcli
Length of output: 706
🏁 Script executed:
# Check the test file to understand expected behavior
head -100 test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 3069
🏁 Script executed:
# Check for tests related to malformed responses or error handling
grep -n "json\|malform\|KeyError\|ValueError\|JSONDecodeError" test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 245
🏁 Script executed:
# Look for all test cases related to refresh_access_token and fetch_token_with_credentials
grep -n "def test.*refresh\|def test.*fetch\|def test.*credentials" test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 827
🏁 Script executed:
# Get more of the test file to see error handling tests
wc -l test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 94
🏁 Script executed:
# Read more of the test file around refresh_access_token and fetch_token_with_credentials tests
sed -n '100,300p' test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 7942
🏁 Script executed:
# Look at more of the test file to see if there are any malformed response tests
grep -n "JSONDecodeError\|test_.*malform\|test_.*error\|test_.*fail" test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 381
🏁 Script executed:
# Check the refresh_access_token test in detail
sed -n '326,380p' test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 2233
🏁 Script executed:
# Check the fetch_token_with_credentials tests
sed -n '370,450p' test/unit/test_sap_http_oauth.pyRepository: jfilak/sapcli
Length of output: 3177
Harden OAuth response parsing before caching.
Both token fetchers trust response.json() and the returned fields too much. A malformed 200 response will currently raise JSONDecodeError, KeyError, or ValueError and escape as a stack trace instead of a clean auth failure. Wrap parsing/validation in try-except and re-raise OAuthTokenError. The save_token_response() function also accesses token_response['access_token'] without validation—apply error handling there as well.
🛠️ Suggested hardening
def refresh_access_token(token_url, client_id, client_secret, refresh_token):
@@
- token_data = response.json()
- save_token_response(token_url, client_id, token_data)
- return token_data['access_token']
+ try:
+ token_data = response.json()
+ save_token_response(token_url, client_id, token_data)
+ return token_data['access_token']
+ except (ValueError, KeyError, TypeError) as exc:
+ raise OAuthTokenError(
+ f'OAuth token response was malformed: {response.text}'
+ ) from excApply the same pattern in fetch_token_with_credentials().
Comment on lines
+142
to
+155
| def get_token(token_url, client_id, client_secret, user=None, password=None): | ||
| """Return a valid Bearer token - from cache, refresh, or credentials grant.""" | ||
|
|
||
| token = get_cached_token(token_url, client_id) | ||
| if token: | ||
| return token | ||
|
|
||
| refresh_token = get_cached_refresh_token(token_url, client_id) | ||
| if refresh_token: | ||
| token = refresh_access_token(token_url, client_id, client_secret, refresh_token) | ||
| if token: | ||
| return token | ||
|
|
||
| return fetch_token_with_credentials(token_url, client_id, client_secret, user, password) |
There was a problem hiding this comment.
Fail fast when password-grant credentials are absent.
If neither a cached access token nor refresh token exists, this branch still calls the password grant with None user/password values. That produces an avoidable 400/stack-trace path instead of a clear re-authentication error. As per coding guidelines, use exception types derived from sap.errors.SAPCliError.
🛠️ Proposed fix
def get_token(token_url, client_id, client_secret, user=None, password=None):
@@
refresh_token = get_cached_refresh_token(token_url, client_id)
if refresh_token:
token = refresh_access_token(token_url, client_id, client_secret, refresh_token)
if token:
return token
+
+ if user is None or password is None:
+ raise OAuthTokenError(
+ 'No cached access token is available and password-grant credentials were not provided'
+ )
return fetch_token_with_credentials(token_url, client_id, client_secret, user, password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/http/oauth.py` around lines 142 - 155, The get_token function currently
calls fetch_token_with_credentials even when user/password are None, causing a
400 and traceback; update get_token to validate that user and password are
provided before invoking fetch_token_with_credentials and if they are missing
raise a sap.errors.SAPCliError (or a subclass) with a clear re-authentication
message; keep the existing cache and refresh logic intact and reference
get_token, get_cached_token, get_cached_refresh_token, refresh_access_token, and
fetch_token_with_credentials when making the change.
Comment on lines
+138
to
+142
| try: | ||
| return Token.from_json(path.read_text(encoding="utf-8")) | ||
| except (OSError, ValueError, KeyError): | ||
| # Corrupt or unreadable — treat as absent rather than crash. | ||
| return None |
There was a problem hiding this comment.
Treat malformed cache entries as cache misses.
Token.from_json() can still raise TypeError here for missing required fields or unexpected keys, so a corrupt token file will crash the OAuth path instead of being ignored.
🔧 Suggested fix
- except (OSError, ValueError, KeyError):
+ except (OSError, ValueError, KeyError, TypeError):
# Corrupt or unreadable — treat as absent rather than crash.
return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/http/token_cache.py` around lines 138 - 142, The token cache reader
currently only catches OSError, ValueError, and KeyError when calling
Token.from_json, but Token.from_json can also raise TypeError for
malformed/missing fields; update the exception handling in the token load block
(where Token.from_json(...) is called in token_cache.py) to also catch TypeError
and return None so malformed cache entries are treated as cache misses rather
than crashing the OAuth flow.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.