Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions sap/adt/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ class Connection:
"""

# pylint: disable=too-many-arguments
def __init__(self, host, client, user, password, port=None, ssl=True, verify=True, ssl_server_cert=None):
def __init__(self, host, client, user, password, port=None, ssl=True, verify=True, ssl_server_cert=None,
token_url=None, client_id=None, client_secret=None):
"""Parameters:
- host: string host name
- client: string SAP client
Expand All @@ -124,10 +125,13 @@ def __init__(self, host, client, user, password, port=None, ssl=True, verify=Tru
port=port,
user=user,
password=password,
saml2=False,
saml2=None if (token_url and client_id and client_secret) else False,
client=client,
verify=verify,
ssl_server_cert=ssl_server_cert,
token_url=token_url,
client_id=client_id,
client_secret=client_secret,
# This must be the default login path because newer ABAP systems
# did not return cookies and CSRF token with the old default login
# path (GET /sap/bc/adt/discovery) and thus did not work with
Expand Down
17 changes: 16 additions & 1 deletion sap/cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ def adt_connection_from_args(args):
return sap.adt.Connection(
args.ashost, args.client, args.user, args.password,
port=args.port, ssl=args.ssl, verify=args.verify,
ssl_server_cert=args.ssl_server_cert)
ssl_server_cert=args.ssl_server_cert,
token_url=getattr(args, 'token_url', None),
client_id=getattr(args, 'client_id', None),
client_secret=getattr(args, 'client_secret', None))


def rfc_connection_from_args(args):
Expand Down Expand Up @@ -202,6 +205,9 @@ def build_empty_connection_values():
ssl_server_cert=None,
user=None,
password=None,
token_url=None,
client_id=None,
client_secret=None,
)


Expand Down Expand Up @@ -285,6 +291,15 @@ def resolve_default_connection_values(args):
if not args.password:
args.password = os.getenv('SAP_PASSWORD') or config_values.get('password')

if not getattr(args, 'token_url', None):
args.token_url = os.getenv('SAP_TOKEN_URL') or config_values.get('token_url')

if not getattr(args, 'client_id', None):
args.client_id = os.getenv('SAP_CLIENT_ID') or config_values.get('client_id')

if not getattr(args, 'client_secret', None):
args.client_secret = os.getenv('SAP_CLIENT_SECRET') or config_values.get('client_secret')

if hasattr(args, 'corrnr') and args.corrnr is None:
args.corrnr = os.getenv('SAP_CORRNR')

Expand Down
11 changes: 10 additions & 1 deletion sap/cli/_entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import sap.rfc
from sap.config import ConfigFile
from sap.http import TimedOutRequestError as HttpTimedOutRequestError
from sap.http.oauth import get_cached_token, get_cached_refresh_token
from sap.odata.errors import TimedOutRequestError as ODataTimedOutRequestError

# pylint: disable=invalid-name
Expand Down Expand Up @@ -157,7 +158,15 @@ def parse_command_line(argv):
if not args.user:
args.user = input('Login:')

if not args.password:
token_url = getattr(args, 'token_url', None)
client_id = getattr(args, 'client_id', None)
has_valid_token = (
token_url and client_id and (
get_cached_token(token_url, client_id) or
get_cached_refresh_token(token_url, client_id)
)
)
if not args.password and not has_valid_token:
args.password = getpass.getpass()

return args
Expand Down
1 change: 1 addition & 0 deletions sap/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class SAPCliConfigError(SAPCliError):
'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',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

)

USER_FIELDS = (
Expand Down
25 changes: 22 additions & 3 deletions sap/http/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,26 @@

import requests
import requests.exceptions
from requests.auth import HTTPBasicAuth
from requests.auth import AuthBase, HTTPBasicAuth

from sap import get_logger, config_get
from sap.http.errors import (
HTTPRequestError,
UnauthorizedError,
TimedOutRequestError,
)
from sap.http.oauth import get_token


class BearerAuth(AuthBase):
"""Requests auth handler that injects an OAuth 2.0 Bearer token."""

def __init__(self, token):
self._token = token

def __call__(self, r):
r.headers['Authorization'] = f'Bearer {self._token}'
return r


def build_query_args(client=None, saml2=None):
Expand Down Expand Up @@ -63,7 +75,10 @@ def __init__(self,
verify=None,
ssl_server_cert=None,
login_path='',
login_method='HEAD'
login_method='HEAD',
token_url=None,
client_id=None,
client_secret=None,
):

self.ssl = ssl
Expand Down Expand Up @@ -91,7 +106,11 @@ def __init__(self,

self.timeout = config_get('http_timeout')

self._auth = HTTPBasicAuth(user, password)
if token_url and client_id and client_secret:
token = get_token(token_url, client_id, client_secret, user=user, password=password)
self._auth = BearerAuth(token)
else:
self._auth = HTTPBasicAuth(user, password)

self.error_handlers = [default_http_error_handler]
self._connection_error_handler = None
Expand Down
129 changes: 129 additions & 0 deletions sap/http/oauth.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
"""OAuth 2.0 password grant flow with token caching for BTP Steampunk."""

import json
import os
import time
from pathlib import Path

import requests

TOKEN_CACHE_PATH = Path('~/.sapcli/tokens.json').expanduser()
REFRESH_MARGIN = 60


# ---------------------------------------------------------------------------
# Token cache
# ---------------------------------------------------------------------------

def _load_token_cache():
try:
with open(TOKEN_CACHE_PATH, 'r', encoding='utf-8') as f:
return json.load(f)
except (OSError, json.JSONDecodeError):
return {}


def _save_token_cache(cache):
TOKEN_CACHE_PATH.parent.mkdir(parents=True, exist_ok=True)
fd = os.open(TOKEN_CACHE_PATH, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
with os.fdopen(fd, 'w', encoding='utf-8') as f:
json.dump(cache, f, indent=2)


def _cache_key(token_url, client_id):
return f'{token_url}|{client_id}'


def get_cached_token(token_url, client_id):
cache = _load_token_cache()
entry = cache.get(_cache_key(token_url, client_id))
if not entry:
return None
if time.time() > entry.get('expires_at', 0) - REFRESH_MARGIN:
return None
return entry['access_token']


def get_cached_refresh_token(token_url, client_id):
cache = _load_token_cache()
entry = cache.get(_cache_key(token_url, client_id))
if not entry:
return None
return entry.get('refresh_token')


def save_token_response(token_url, client_id, token_response):
cache = _load_token_cache()
expires_in = token_response.get('expires_in', 3600)
cache[_cache_key(token_url, client_id)] = {
'access_token': token_response['access_token'],
'refresh_token': token_response.get('refresh_token'),
'expires_at': time.time() + expires_in,
}
_save_token_cache(cache)


# ---------------------------------------------------------------------------
# Token refresh
# ---------------------------------------------------------------------------

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
Comment on lines +70 to +78

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

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 RuntimeError(
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']


# ---------------------------------------------------------------------------
# Entry point
# ---------------------------------------------------------------------------

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)
Loading
Loading