Add OAuth 2.0 support for BTP ABAP Environment (Steampunk)#157
Conversation
📝 WalkthroughWalkthroughAdds OAuth 2.0 token-based authentication across the CLI and HTTP stack: new token parameters propagate from CLI → Connection → HTTPClient; HTTPClient can use Bearer auth; new oauth module handles token fetch, refresh, and disk cache with a password-grant fallback. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant CLI
participant Connection
participant HTTPClient
participant OAuthModule as OAuth Module
participant Cache as Token Cache
participant OAuthServer as OAuth Server
participant SAPBackend as SAP Backend
User->>CLI: invoke command (may include token_url, client_id, client_secret)
CLI->>Connection: Connection(..., token_url, client_id, client_secret)
Connection->>HTTPClient: init(token_url, client_id, client_secret, user?, password?)
HTTPClient->>OAuthModule: get_token(token_url, client_id, client_secret, user?, password?)
OAuthModule->>Cache: get_cached_token(token_url, client_id)
alt cached valid token
Cache-->>OAuthModule: access_token
else missing/expired
OAuthModule->>Cache: get_cached_refresh_token(token_url, client_id)
alt refresh token present
OAuthModule->>OAuthServer: POST /oauth/token (grant_type=refresh_token)
OAuthServer-->>OAuthModule: token_response
OAuthModule->>Cache: save_token_response(...)
else no refresh token
OAuthModule->>User: prompt for password (if needed)
User-->>OAuthModule: credentials
OAuthModule->>OAuthServer: POST /oauth/token (grant_type=password)
OAuthServer-->>OAuthModule: token_response
OAuthModule->>Cache: save_token_response(...)
end
end
OAuthModule-->>HTTPClient: access_token
HTTPClient->>SAPBackend: request with Authorization: Bearer <token>
SAPBackend-->>HTTPClient: response
HTTPClient-->>Connection: response
Connection-->>CLI: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
sap/http/oauth.py (1)
20-24: Don’t silently discard token-cache read failures without context.Line 23–24 currently masks cache corruption/permission problems as a cache miss. Add at least a short explanatory comment and debug logging so auth failures are diagnosable.
As per coding guidelines "Avoid silently swallowing caught exceptions - if necessary, add a comment explaining why it is needed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sap/http/oauth.py` around lines 20 - 24, The token cache reader currently swallows OSError and json.JSONDecodeError which hides permission or corruption issues; update the except block around the TOKEN_CACHE_PATH json.load call to add a brief comment stating why exceptions are treated as a cache miss and emit a debug-level log with the exception details (e.g., using the module logger) so failures are diagnosable; keep returning {} for compatibility but ensure the log message references TOKEN_CACHE_PATH and the caught exception to aid debugging.
🤖 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/cli/_entry.py`:
- Around line 160-161: The current gate uses only args.token_url to skip
prompting for args.password (see args.password, args.token_url,
getpass.getpass), which suppresses password collection in partial/OAuth-mixed
modes; change the conditional so the password is skipped only when a full OAuth
configuration is present (check token_url plus required OAuth fields such as
client_id and client_secret) and when the ADT connection factory is being used,
otherwise call getpass.getpass() to collect args.password; update the condition
around the getpass.getpass() call to explicitly detect the full-OAuth tuple and
ADT connection-factory presence before skipping the prompt.
In `@sap/config.py`:
- Line 26: The secret-exposure check in _has_passwords() still only looks for
'password' so newly-stored OAuth secrets like 'client_secret' are missed; update
_has_passwords() (and any helper that enumerates secret keys) to include
'client_secret' alongside 'password' (and other secret keys if present) so
world-readable/shared-config warnings fire for OAuth configs, and adjust any
tests or calls that rely on the original set of keys (e.g., places referencing
_has_passwords or SECRET_KEYS) to use the expanded list.
In `@sap/http/oauth.py`:
- Around line 71-79: The current refresh_access_token function swallows all
non-2xx responses by returning None; change it to surface network/5xx and
unexpected 4xx errors as typed CLI errors while only returning None for expected
token-expiry cases (e.g., invalid_grant). Concretely: wrap the requests.post
call to catch requests.exceptions.RequestException and raise a CLIError (or a
new RefreshTokenError) with the exception details; after getting response, if
status_code >= 500 raise the same typed error including response.status_code and
response.text; if status_code is 400/401 parse response.json() and return None
only when the error indicates an expired/invalid_grant token, otherwise raise
the typed error with the error payload. Reference refresh_access_token,
token_url, response, refresh_token and use a CLI-specific exception type for
callers to distinguish fallback vs fatal.
- Around line 89-110: In fetch_token_via_password, avoid raising RuntimeError
and avoid unconditional prompting in non-interactive runs: import and raise an
SAPCliError-derived exception (e.g., SAPCliError or a suitable subclass from
sap.errors) instead of RuntimeError for the failed response, and before calling
input()/getpass.getpass() check interactivity (e.g., if not sys.stdin.isatty():
raise SAPCliError('BTP OAuth login required but input is not interactive') ) so
non-interactive callers fail with a CLI-friendly error; keep the rest of the
POST logic using token_url, client_id, client_secret unchanged.
In `@test/unit/test_sap_http_oauth.py`:
- Around line 3-6: Remove the unused imports causing lint failures at the top of
the test file: drop the symbols `json`, `MagicMock`, and `call` from the import
list in test_sap_http_oauth.py so only required imports (e.g., `time`,
`unittest`, `Mock`, `patch`) remain; alternatively, if those symbols are
actually needed later, replace their unused occurrences with real usage or
assert statements in the tests referencing `MagicMock`/`call` instead of
removing them.
---
Nitpick comments:
In `@sap/http/oauth.py`:
- Around line 20-24: The token cache reader currently swallows OSError and
json.JSONDecodeError which hides permission or corruption issues; update the
except block around the TOKEN_CACHE_PATH json.load call to add a brief comment
stating why exceptions are treated as a cache miss and emit a debug-level log
with the exception details (e.g., using the module logger) so failures are
diagnosable; keep returning {} for compatibility but ensure the log message
references TOKEN_CACHE_PATH and the caught exception to aid debugging.
🪄 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: e5b370b0-ae70-49f8-bf6a-46a3e9b19906
📒 Files selected for processing (7)
sap/adt/core.pysap/cli/__init__.pysap/cli/_entry.pysap/config.pysap/http/client.pysap/http/oauth.pytest/unit/test_sap_http_oauth.py
| if not args.password and not getattr(args, 'token_url', None): | ||
| args.password = getpass.getpass() |
There was a problem hiding this comment.
Gate password prompt on full OAuth mode, not just token_url.
Line 160 skips password prompt when only token_url is present. That can suppress password collection for RFC/basic-auth commands (or partial OAuth config), causing avoidable auth failures. Use a stricter condition (full OAuth tuple + ADT connection factory).
💡 Proposed fix
- if not args.password and not getattr(args, 'token_url', None):
+ oauth_enabled = (
+ args.connection_factory is sap.cli.adt_connection_from_args and
+ all(getattr(args, key, None) for key in ('token_url', 'client_id', 'client_secret'))
+ )
+ if not args.password and not oauth_enabled:
args.password = getpass.getpass()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not args.password and not getattr(args, 'token_url', None): | |
| args.password = getpass.getpass() | |
| oauth_enabled = ( | |
| args.connection_factory is sap.cli.adt_connection_from_args and | |
| all(getattr(args, key, None) for key in ('token_url', 'client_id', 'client_secret')) | |
| ) | |
| if not args.password and not oauth_enabled: | |
| args.password = getpass.getpass() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/cli/_entry.py` around lines 160 - 161, The current gate uses only
args.token_url to skip prompting for args.password (see args.password,
args.token_url, getpass.getpass), which suppresses password collection in
partial/OAuth-mixed modes; change the conditional so the password is skipped
only when a full OAuth configuration is present (check token_url plus required
OAuth fields such as client_id and client_secret) and when the ADT connection
factory is being used, otherwise call getpass.getpass() to collect
args.password; update the condition around the getpass.getpass() call to
explicitly detect the full-OAuth tuple and ADT connection-factory presence
before skipping the prompt.
| 'ashost', 'sysnr', 'client', 'port', 'ssl', 'ssl_verify', | ||
| 'ssl_server_cert', 'mshost', 'msserv', 'sysid', 'group', | ||
| 'snc_qop', 'snc_myname', 'snc_partnername', 'snc_lib', | ||
| 'token_url', 'client_id', 'client_secret', |
There was a problem hiding this comment.
client_secret is now storable, but secret-exposure checks don’t cover it.
After adding client_secret in Line 26, _has_passwords() still only inspects password. World-readable/shared-config warnings can be missed when only OAuth secrets are present.
💡 Proposed fix
-def _has_passwords(config_data: dict) -> bool:
- """Check if the config contains any password fields."""
+def _has_passwords(config_data: dict) -> bool:
+ """Check if the config contains any credential/secret fields."""
+
+ secret_fields = ('password', 'client_secret')
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'):
+ if isinstance(user_def, dict) and any(user_def.get(k) for k in secret_fields):
return True
+ connections = config_data.get('connections', {})
+ if isinstance(connections, dict):
+ for conn_def in connections.values():
+ if isinstance(conn_def, dict) and any(conn_def.get(k) for k in secret_fields):
+ 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 any(context_def.get(k) for k in secret_fields):
return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/config.py` at line 26, The secret-exposure check in _has_passwords()
still only looks for 'password' so newly-stored OAuth secrets like
'client_secret' are missed; update _has_passwords() (and any helper that
enumerates secret keys) to include 'client_secret' alongside 'password' (and
other secret keys if present) so world-readable/shared-config warnings fire for
OAuth configs, and adjust any tests or calls that rely on the original set of
keys (e.g., places referencing _has_passwords or SECRET_KEYS) to use the
expanded list.
| def refresh_access_token(token_url, client_id, client_secret, refresh_token): | ||
| 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 |
There was a problem hiding this comment.
Refresh failure handling is too silent and can cause confusing fallback prompts.
Line 78–79 returns None for every non-2xx response, which triggers password prompting later without surfacing why refresh failed. Consider raising a typed CLI error for non-recoverable cases (e.g., 5xx/network), and only fallback on expected token-expiry scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sap/http/oauth.py` around lines 71 - 79, The current refresh_access_token
function swallows all non-2xx responses by returning None; change it to surface
network/5xx and unexpected 4xx errors as typed CLI errors while only returning
None for expected token-expiry cases (e.g., invalid_grant). Concretely: wrap the
requests.post call to catch requests.exceptions.RequestException and raise a
CLIError (or a new RefreshTokenError) with the exception details; after getting
response, if status_code >= 500 raise the same typed error including
response.status_code and response.text; if status_code is 400/401 parse
response.json() and return None only when the error indicates an
expired/invalid_grant token, otherwise raise the typed error with the error
payload. Reference refresh_access_token, token_url, response, refresh_token and
use a CLI-specific exception type for callers to distinguish fallback vs fatal.
| def fetch_token_via_password(token_url, client_id, client_secret): | ||
| """Prompt for email + password once, cache the resulting token.""" | ||
|
|
||
| print('\nBTP OAuth login required.') | ||
| username = input('Email: ') | ||
| password = getpass.getpass('Password: ') | ||
|
|
||
| response = requests.post( | ||
| token_url.rstrip('/') + '/oauth/token', | ||
| auth=(client_id, client_secret), | ||
| data={ | ||
| 'grant_type': 'password', | ||
| 'username': username, | ||
| 'password': password, | ||
| }, | ||
| timeout=30, | ||
| ) | ||
|
|
||
| if not response.ok: | ||
| raise RuntimeError( | ||
| f'OAuth login failed ({response.status_code}): {response.text}' | ||
| ) |
There was a problem hiding this comment.
Use SAPCliError-derived exceptions and guard non-interactive input.
Line 108 raises RuntimeError, which bypasses the CLI’s friendly SAPCliError handling. Also, Line 93–94 unconditionally prompts, which is brittle in non-interactive runs.
💡 Proposed fix
+import sys
+from sap.errors import SAPCliError
+
+class OAuthLoginError(SAPCliError):
+ """OAuth authentication failed."""
+
def fetch_token_via_password(token_url, client_id, client_secret):
"""Prompt for email + password once, cache the resulting token."""
+ if not sys.stdin.isatty():
+ raise OAuthLoginError('OAuth login requires an interactive terminal.')
+
print('\nBTP OAuth login required.')
- username = input('Email: ')
- password = getpass.getpass('Password: ')
+ try:
+ username = input('Email: ')
+ password = getpass.getpass('Password: ')
+ except EOFError as ex:
+ raise OAuthLoginError('OAuth login input was interrupted.') from ex
- response = requests.post(
- token_url.rstrip('/') + '/oauth/token',
- auth=(client_id, client_secret),
- data={
- 'grant_type': 'password',
- 'username': username,
- 'password': password,
- },
- timeout=30,
- )
+ try:
+ response = requests.post(
+ token_url.rstrip('/') + '/oauth/token',
+ auth=(client_id, client_secret),
+ data={
+ 'grant_type': 'password',
+ 'username': username,
+ 'password': password,
+ },
+ timeout=30,
+ )
+ except requests.RequestException as ex:
+ raise OAuthLoginError(f'OAuth request failed: {ex}') from ex
if not response.ok:
- raise RuntimeError(
+ raise OAuthLoginError(
f'OAuth login failed ({response.status_code}): {response.text}'
)As per coding guidelines "Use exception types derived from sap.errors.SAPCliError to ensure the command line entry point intercepts them and prints nice error messages instead of stack traces".
🤖 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 - 110, In fetch_token_via_password, avoid
raising RuntimeError and avoid unconditional prompting in non-interactive runs:
import and raise an SAPCliError-derived exception (e.g., SAPCliError or a
suitable subclass from sap.errors) instead of RuntimeError for the failed
response, and before calling input()/getpass.getpass() check interactivity
(e.g., if not sys.stdin.isatty(): raise SAPCliError('BTP OAuth login required
but input is not interactive') ) so non-interactive callers fail with a
CLI-friendly error; keep the rest of the POST logic using token_url, client_id,
client_secret unchanged.
| print('\nBTP OAuth login required.') | ||
| username = input('Email: ') | ||
| password = getpass.getpass('Password: ') |
There was a problem hiding this comment.
IOn the ideal world, anyh user input is done only in the modules at the directory sap/cli/ and nowhere else. Can we say that the already existing "user" and "password" configuration options are used here instead of asking for email and password?
There was a problem hiding this comment.
All user interaction stays in the CLI layer. The password prompt is now OAuth-aware: it checks the token cache first and only asks for a password when neither a valid access token nor a refresh token exists.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/unit/test_sap_http_oauth.py (1)
3-6:⚠️ Potential issue | 🟡 MinorRemove unused imports to fix lint failures.
json,MagicMock, andcallare imported but not used (Line 3, Line 6).Proposed fix
-import json import time import unittest -from unittest.mock import MagicMock, Mock, patch, call +from unittest.mock import Mock, patch🤖 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 3 - 6, Remove the unused imports causing lint failures from test_sap_http_oauth.py: drop "json" from the top import line and remove "MagicMock" and "call" from the unittest.mock import list, leaving only the mocks actually used (e.g., Mock, patch) so the file only imports symbols referenced in the tests.
🧹 Nitpick comments (1)
test/unit/test_sap_http_oauth.py (1)
267-291: Strengthen orchestration tests with collaborator call assertions.These tests only validate returned tokens; they should also assert that refresh/password paths are called (or not called) with expected arguments to lock in priority behavior.
Example assertions to add
def test_uses_refresh_token_when_access_token_expired( self, mock_cached, mock_refresh_tok, mock_refresh, mock_password): token = get_token('https://auth.example.com', 'client-id', 'client-secret') self.assertEqual(token, 'refreshed-token') + mock_refresh.assert_called_once_with( + 'https://auth.example.com', 'client-id', 'client-secret', 'old-refresh' + ) mock_password.assert_not_called() @@ def test_falls_back_to_password_when_refresh_fails( self, mock_cached, mock_refresh_tok, mock_refresh, mock_password): token = get_token('https://auth.example.com', 'client-id', 'client-secret') self.assertEqual(token, 'new-login-token') + mock_refresh.assert_called_once_with( + 'https://auth.example.com', 'client-id', 'client-secret', 'stale-refresh' + ) + mock_password.assert_called_once()🤖 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 267 - 291, Add assertions in the three tests to verify collaborator calls and enforce the priority flow: in test_uses_refresh_token_when_access_token_expired assert refresh_access_token (mock_refresh) was called once with the cached refresh token ('stale-refresh') and the client creds/URL and that fetch_token_with_credentials (mock_refresh_tok) and mock_password were not called; in test_falls_back_to_password_when_refresh_fails assert refresh_access_token (mock_refresh) was called and, because it returned None, fetch_token_with_credentials (mock_refresh_tok) was called with the client-id/client-secret and auth URL (and mock_password not called if not expected), and in test_prompts_login_when_no_cache_at_all assert get_cached_refresh_token/get_cached_token (mock_cached) returned None and that fetch_token_with_credentials (mock_password/mock_refresh_tok depending on patch names in that test) was called once with the expected credentials while refresh_access_token was not called; use the mock objects present in each test (mock_cached, mock_refresh_tok, mock_refresh, mock_password) and their call/assert methods to lock in the orchestration behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/test_sap_http_oauth.py`:
- Around line 69-90: The tests patch the wrong symbol path causing false
positives; change the `@patch` decorators in
test_uses_basic_auth_when_no_token_url and
test_uses_basic_auth_when_token_url_missing to patch the actual call site used
by HTTPClient (patch 'sap.http.client.get_token' instead of
'sap.http.oauth.get_token') so that HTTPClient's use of get_token is correctly
intercepted; keep the rest of the test logic and imports (HTTPClient,
HTTPBasicAuth) unchanged.
---
Duplicate comments:
In `@test/unit/test_sap_http_oauth.py`:
- Around line 3-6: Remove the unused imports causing lint failures from
test_sap_http_oauth.py: drop "json" from the top import line and remove
"MagicMock" and "call" from the unittest.mock import list, leaving only the
mocks actually used (e.g., Mock, patch) so the file only imports symbols
referenced in the tests.
---
Nitpick comments:
In `@test/unit/test_sap_http_oauth.py`:
- Around line 267-291: Add assertions in the three tests to verify collaborator
calls and enforce the priority flow: in
test_uses_refresh_token_when_access_token_expired assert refresh_access_token
(mock_refresh) was called once with the cached refresh token ('stale-refresh')
and the client creds/URL and that fetch_token_with_credentials
(mock_refresh_tok) and mock_password were not called; in
test_falls_back_to_password_when_refresh_fails assert refresh_access_token
(mock_refresh) was called and, because it returned None,
fetch_token_with_credentials (mock_refresh_tok) was called with the
client-id/client-secret and auth URL (and mock_password not called if not
expected), and in test_prompts_login_when_no_cache_at_all assert
get_cached_refresh_token/get_cached_token (mock_cached) returned None and that
fetch_token_with_credentials (mock_password/mock_refresh_tok depending on patch
names in that test) was called once with the expected credentials while
refresh_access_token was not called; use the mock objects present in each test
(mock_cached, mock_refresh_tok, mock_refresh, mock_password) and their
call/assert methods to lock in the orchestration behavior.
🪄 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: 904ac9f7-b9ac-481c-b452-18d5453894e5
📒 Files selected for processing (3)
sap/http/client.pysap/http/oauth.pytest/unit/test_sap_http_oauth.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sap/http/client.py
- sap/http/oauth.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/unit/test_sap_http_oauth.py (1)
3-6:⚠️ Potential issue | 🟡 MinorRemove unused imports to keep lint green.
json,MagicMock, andcallare not used, so this will fail static checks.💡 Proposed fix
-import json import time import unittest -from unittest.mock import MagicMock, Mock, patch, call +from unittest.mock import Mock, patch🤖 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 3 - 6, Remove the unused imports in test_sap_http_oauth.py: delete json, MagicMock, and call from the import line (the file currently imports json, time, unittest, MagicMock, Mock, patch, call) so only the actually used symbols remain (e.g., time, unittest, Mock, patch); update the import statement accordingly to satisfy linters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/unit/test_sap_http_oauth.py`:
- Around line 3-6: Remove the unused imports in test_sap_http_oauth.py: delete
json, MagicMock, and call from the import line (the file currently imports json,
time, unittest, MagicMock, Mock, patch, call) so only the actually used symbols
remain (e.g., time, unittest, Mock, patch); update the import statement
accordingly to satisfy linters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 712316cc-c278-440a-b587-d398c968408f
📒 Files selected for processing (2)
sap/cli/_entry.pytest/unit/test_sap_http_oauth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- sap/cli/_entry.py
|
I took you PR and added some hardenings: #159 |
sapcli previously only supported HTTP Basic Auth, which BTP Steampunk rejects entirely. This change adds OAuth 2.0 password grant flow so sapcli can authenticate against BTP ABAP Environment systems.