Authplugins#165
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR adds an external auth-plugin system (kubectl-style) with a JSON plugin protocol, ConnectionInfo TLS fields, AuthPluginResponse serialization/expiration, a file-backed response store, an HTTP session initializer that applies plugin outputs (cookies/headers/certs), CLI flags to control cache behavior, a reference basic-auth plugin, and comprehensive unit tests. ChangesExternal Auth Plugin Feature
🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (2)
sap/cli/__init__.py (1)
548-551: ⚡ Quick winDocument the intentional swallow of
SAPCliConfigErrorin cache-key derivation.This catch currently disables caching silently. Please add an explicit inline reason so this is intentional and maintainers don’t treat it as accidental error masking.
Suggested patch
try: ctx = config_file.get_context(context_name) except SAPCliConfigError: + # Cache-key derivation is best-effort; auth flow should continue + # without cache if context lookup cannot be resolved here. return NoneAs 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/cli/__init__.py` around lines 548 - 551, The except block catching SAPCliConfigError around config_file.get_context(context_name) intentionally disables cache-key derivation when the requested context is invalid or missing; update the code at that try/except (the call to config_file.get_context(context_name) and the except SAPCliConfigError block) to include a clear inline comment stating this is deliberate (and, optionally, add a debug-level log like processLogger.debug or similar) so maintainers know the exception is intentionally swallowed rather than masked.test/unit/test_sap_http_external_session_initializer.py (1)
161-189: ⚡ Quick winAdd a regression test for non-boolean cookie
securevalues.Please add a case asserting
AuthPluginErrorwhen a cookie uses'secure': 'false'(string). It will guard against silent coercion regressions.✅ Suggested test
class TestCookieDispatch(unittest.TestCase): @@ def test_cookie_entry_not_object_raises(self): with self.assertRaisesRegex(AuthPluginError, "object"): self._initialize({ 'type': 'cookie', 'cookies': ['name=value; Path=/'], }) + + def test_cookie_secure_not_boolean_raises(self): + with self.assertRaisesRegex(AuthPluginError, "secure"): + self._initialize({ + 'type': 'cookie', + 'cookies': [{'name': 'X', 'value': '1', 'secure': 'false'}], + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_http_external_session_initializer.py` around lines 161 - 189, Add a new unit test method (e.g., test_cookie_secure_not_boolean_raises) alongside the other tests that calls self._initialize with {'type': 'cookie', 'cookies': [{'name': 'X', 'value': 'v', 'secure': 'false'}]} and assert that it raises AuthPluginError (use self.assertRaisesRegex(AuthPluginError, "secure")). This mirrors the existing tests (test_missing_cookies_field_raises, test_cookies_not_a_list_raises, test_cookie_without_name_raises, etc.) and ensures non-boolean secure values are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/auth/basic-auth-cookies.py`:
- Around line 88-90: After ensuring request.get('connection') is a dict,
validate that required fields (e.g., the keys used later such as 'url',
'client', and 'access') exist before they are accessed: create a required_keys
list, iterate and call _fail("Missing connection field: <key>") for any missing
key, and only proceed when all required keys are present to prevent KeyError
tracebacks; update the block around request.get('connection') to perform this
presence check using the existing _fail helper.
In `@sap/http/auth_plugin_cache.py`:
- Around line 42-52: cache_key_for currently concatenates context, connection
and user with '|' which allows collisions when any component contains '|' —
update cache_key_for to produce collision-safe keys by encoding each component
before joining (for example, apply a deterministic, filesystem-safe encoding
such as URL-safe base64 or percent-encoding to context, connection and user) and
then join the encoded parts (you can still use '|' as a separator after
encoding). Ensure the chosen encoding is deterministic and reversible/unique per
input so cache lookups remain correct across calls to cache_key_for.
In `@sap/http/external_session_initializer.py`:
- Around line 137-139: The current code coerces cookie['secure'] with bool(...)
which silently accepts strings like "false"; instead validate and reject
non-boolean values: in the block that checks cookie.get('secure') (in
external_session_initializer.py where cookie and kwargs are used), if
cookie['secure'] is not an instance of bool then raise a clear TypeError (or
ValueError) describing the bad value, otherwise set kwargs['secure'] =
cookie['secure'] without coercion so only real booleans are accepted.
In `@test/unit/test_sap_cli__entry.py`:
- Around line 393-400: Replace the loop-driven assertions that iterate over
variants for SAPCLI_AUTH_PLUGIN_DISABLE_CACHE with explicit, sequential checks:
for each variant string, set
os.environ['SAPCLI_AUTH_PLUGIN_DISABLE_CACHE']=variant, call
entry.parse_command_line(test_params), delete the env var, and assert
args.auth_plugin_disable_cache is False using a separate assertion (or create
separate test functions named e.g. test_auth_plugin_disable_cache_variant_n,
_no, _No, etc.); reference the environment key SAPCLI_AUTH_PLUGIN_DISABLE_CACHE,
the parser call entry.parse_command_line, and the result field
args.auth_plugin_disable_cache when implementing the explicit sequential
assertions/tests.
- Line 357: The with-context binds an unused variable `_env` from the patch.dict
call; remove the unused binding by changing the context manager from "with
patch.dict(os.environ, {}, clear=False) as _env:" to "with
patch.dict(os.environ, {}, clear=False):" in the test (look for the patch.dict
usage in test_sap_cli__entry.py) so the linter no longer complains about an
unused variable.
---
Nitpick comments:
In `@sap/cli/__init__.py`:
- Around line 548-551: The except block catching SAPCliConfigError around
config_file.get_context(context_name) intentionally disables cache-key
derivation when the requested context is invalid or missing; update the code at
that try/except (the call to config_file.get_context(context_name) and the
except SAPCliConfigError block) to include a clear inline comment stating this
is deliberate (and, optionally, add a debug-level log like processLogger.debug
or similar) so maintainers know the exception is intentionally swallowed rather
than masked.
In `@test/unit/test_sap_http_external_session_initializer.py`:
- Around line 161-189: Add a new unit test method (e.g.,
test_cookie_secure_not_boolean_raises) alongside the other tests that calls
self._initialize with {'type': 'cookie', 'cookies': [{'name': 'X', 'value': 'v',
'secure': 'false'}]} and assert that it raises AuthPluginError (use
self.assertRaisesRegex(AuthPluginError, "secure")). This mirrors the existing
tests (test_missing_cookies_field_raises, test_cookies_not_a_list_raises,
test_cookie_without_name_raises, etc.) and ensures non-boolean secure values are
rejected.
🪄 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: f15d4e38-19c7-4fcb-9dae-785ab38165c8
📒 Files selected for processing (15)
README.mddoc/configuration.mdplugins/auth/basic-auth-cookies.pysap/cli/__init__.pysap/cli/_entry.pysap/config.pysap/http/auth_plugin.pysap/http/auth_plugin_cache.pysap/http/external_session_initializer.pytest/unit/test_sap_cli.pytest/unit/test_sap_cli__entry.pytest/unit/test_sap_config.pytest/unit/test_sap_http_auth_plugin.pytest/unit/test_sap_http_auth_plugin_cache.pytest/unit/test_sap_http_external_session_initializer.py
|
|
||
| test_params = get_tested_parameters() | ||
|
|
||
| with patch.dict(os.environ, {}, clear=False) as _env: |
There was a problem hiding this comment.
Remove the unused _env binding in the patch context.
_env is assigned but never used, which trips lint unnecessarily.
Suggested patch
- with patch.dict(os.environ, {}, clear=False) as _env:
+ with patch.dict(os.environ, {}, clear=False):
os.environ.pop('SAPCLI_AUTH_PLUGIN_DISABLE_CACHE', None)
args = entry.parse_command_line(test_params)🧰 Tools
🪛 Flake8 (7.3.0)
[error] 357-357: local variable '_env' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/test_sap_cli__entry.py` at line 357, The with-context binds an
unused variable `_env` from the patch.dict call; remove the unused binding by
changing the context manager from "with patch.dict(os.environ, {}, clear=False)
as _env:" to "with patch.dict(os.environ, {}, clear=False):" in the test (look
for the patch.dict usage in test_sap_cli__entry.py) so the linter no longer
complains about an unused variable.
Comment on lines
+393
to
+400
| for variant in ('n', 'no', 'No', 'NO', 'false', 'FALSE', 'off', 'Off'): | ||
| os.environ['SAPCLI_AUTH_PLUGIN_DISABLE_CACHE'] = variant | ||
| try: | ||
| args = entry.parse_command_line(test_params) | ||
| finally: | ||
| del os.environ['SAPCLI_AUTH_PLUGIN_DISABLE_CACHE'] | ||
|
|
||
| self.assertFalse(args.auth_plugin_disable_cache, msg=variant) |
There was a problem hiding this comment.
Avoid loop-driven assertions in this unittest.
Please split these variant checks into sequential assertions/tests instead of iterating in a for loop.
As per coding guidelines "Write tests sequentially without loops".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/test_sap_cli__entry.py` around lines 393 - 400, Replace the
loop-driven assertions that iterate over variants for
SAPCLI_AUTH_PLUGIN_DISABLE_CACHE with explicit, sequential checks: for each
variant string, set os.environ['SAPCLI_AUTH_PLUGIN_DISABLE_CACHE']=variant, call
entry.parse_command_line(test_params), delete the env var, and assert
args.auth_plugin_disable_cache is False using a separate assertion (or create
separate test functions named e.g. test_auth_plugin_disable_cache_variant_n,
_no, _No, etc.); reference the environment key SAPCLI_AUTH_PLUGIN_DISABLE_CACHE,
the parser call entry.parse_command_line, and the result field
args.auth_plugin_disable_cache when implementing the explicit sequential
assertions/tests.
Wires the auth-plugin protocol from sap/http/auth_plugin.py into the
HTTPSessionInitializer seam. Step 2 of the auth-plugin track in
features/auth_plugins.md. Translates the plugin's response into the
requests.Session state HTTPClient expects: cookies, an Authorization
header, or a client certificate.
Design decisions:
- Dispatch on content.type via a small handler table (_HANDLERS) instead
of an if/elif ladder. Adding a new content type is one line in the
table plus the applier function; the dispatcher itself stays
unchanged. Mirrors how plugins evolve over time (new auth schemes
appear; the dispatcher should not be the bottleneck).
- Plugin invocation is lazy (in initialize_session, not __init__). Same
rationale as OAuthHTTPSessionInitializer: constructing a connection
must not perform subprocess I/O. Tests construct lots of initializers
for argument validation and do not want the side effect.
- Cookie objects (name/value/domain/path/expires/secure), not raw
"name=value; Path=/" header strings. Aligns with playwright's
storage_state["cookies"] format, which is what the reference
browser-auth plugin returns. Empty cookie value is technically valid
per RFC 6265 but requests' RequestsCookieJar.get() does not read
empty values back; we do not contort the API around a corner case
that does not occur in SAP session cookies.
- HttpOnly is intentionally ignored on the apply path. It is a
browser-only directive; once the cookie is round-tripped to the
server we are not a browser and the flag has no effect. Surfacing
it as a rejection or a warning would only confuse plugin authors.
- http_authorization_header takes a dict of header name/value pairs
rather than a list. The spec example is a single Authorization
header, but the dict shape extends naturally to plugins that need to
set additional headers (e.g. a session cookie alongside
Authorization) without a schema change.
- Certificates: file paths only. requests.Session.cert / .verify accept
either a path or a (cert, key) tuple of paths - no in-memory PEM.
Plugins reading from Windows Cert Store would need temp-file
handling with proper permissions and lifecycle management; deferred
until a real plugin exercises that path. The two reference plugins
(cookie proof-of-concept and playwright browser plugin) both return
cookies, so this is not blocking.
- Strict validation at the plugin trust boundary. The plugin is
external code returning JSON; missing or wrong-typed fields raise
AuthPluginError with a message naming the field. We do not silently
pass incomplete data to requests where it would manifest as a 401
much later. Same posture as auth_plugin.py's envelope validation.
- content.get('type') is Any; narrowed with isinstance(str) before the
_HANDLERS lookup so a non-string 'type' falls through to the same
"unsupported type" error rather than a TypeError on dict.get.
Also brings features/auth_plugins.md in line with the implemented
design: the HTTPSessionInitializer Protocol seam in sap/http/client.py
replaces the originally planned SessionManager in sap/http/session.py
- simpler and already covers every connection type. Refactoring
section is marked done; the plugin-development checklist is renumbered
to reflect the slice ordering (protocol module -> external initializer
-> response cache -> config -> CLI wiring -> reference plugins).
28 unit tests cover construction (lazy, returns same session, plugin
args, error propagation, build_unauthorized_error), each content
type's happy path plus its validation failures (missing fields, wrong
shapes, unknown type, missing type, non-object content), and HTTPClient
integration through session_initializer=.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 5 of the auth-plugin track in features/auth_plugins.md. With the
plugin protocol module (auth_plugin.py) and the session initializer
(external_session_initializer.py) already in place, this commit makes
the two reachable through the standard sapcli config + CLI flow:
users:
my-user:
auth_plugin:
command: /path/to/plugin
parameters: { ... }
When the resolved context carries an auth_plugin, sapcli builds an
HTTPExternalSessionInitializer and skips its own user/password
prompts - credential acquisition is the plugin's job.
Design decisions:
- Mutual exclusivity is enforced at config-resolution time (in
_resolve_auth_plugin_default), against config_values - not against
args. Plugins typically need SAP_PASSWORD in env so their
subprocess can inherit it; that env var lands on args.password
through resolve_default_connection_values, but that is a runtime
detail and has nothing to do with what the user actually
configured. Checking config_values keeps the validation honest and
lets users set SAP_USER/SAP_PASSWORD env vars for the plugin
without tripping a CLI-level conflict.
- auth_plugin is config-only (no CLI flag, no env var). It is a
structured value, not a scalar, and its presence flips the whole
authentication mode. Picking it up only from config keeps the
precedence rules simple - the existing CLI > env > config layering
for scalars would be confusing for a dict that flips behaviour.
- _build_session_initializer takes conn_type and conn_path as kwargs
rather than reading them from args. They are command-specific (ADT
vs. OData vs. REST need different login endpoints), so each
connection factory passes them explicitly. Today only ADT calls in;
rest/odata get wired up when their connection factories start
using session_initializer.
- ConnectionInfo gains 'verify: bool = True' and
'ssl_server_cert: Optional[str] = None'. Without them, every plugin
would have to read SAP_SSL_VERIFY from env independently, and
'ssl_verify: false' in config would silently fail against
self-signed-cert systems (the common test/dev case). Forwarding
args.verify into the request payload keeps the plugin honest
without env-var gymnastics.
- ADT login path is /sap/bc/adt/core/discovery (matches
sap.adt.core.Connection.login_path). The plugin gets that path so
the cookies it collects are the ones sapcli would have collected
through its built-in flow.
- _entry.py suppresses BOTH the user prompt and the getpass prompt
when auth_plugin is set. The plugin owns credentials end-to-end;
prompting for a user name that sapcli will not use would be
confusing.
Tests cover: auth_plugin field surfaced from resolve_context;
auth_plugin propagated into args from config (and absent when not
configured); HTTPExternalSessionInitializer constructed with the
right ConnectionInfo fields (proto, port str-coerced, type, path,
verify, ssl_server_cert); mutual exclusivity (config-level conflict
with password and with OAuth fields); env SAP_PASSWORD does NOT
trigger the mutex; user/password prompts suppressed in
parse_command_line.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds plugins/auth/basic-auth-cookies.py: the cookie proof-of-concept plugin called for in features/auth_plugins.md. It performs HTTP Basic auth against the ABAP system and returns the resulting session cookies - functionally equivalent to sapcli's built-in BasicAuth flow but going through the auth-plugin protocol end-to-end. Purpose: validate the plugin pipeline (subprocess invocation, JSON envelope, content.type='cookie' dispatch, ConnectionInfo TLS fields) against a real ABAP system without needing a browser-automation plugin set up. It also doubles as documentation-by-example for plugin authors. Design decisions: - Credentials come from SAP_USER / SAP_PASSWORD env vars, not from the plugin parameters or the request payload. The whole reason the auth-plugin path exists is to keep secrets out of the sapcli config file; defaulting to env keeps that property even for the reference plugin. - GET, not HEAD, against /sap/bc/adt/core/discovery. Newer ABAP systems return HTTP 400 on HEAD against that endpoint and only set the session cookie on GET. Mirrors sap.adt.core.Connection's login_method='GET' (which exists for the same reason). - Honors connection.verify and connection.ssl_server_cert from the request payload rather than reading SAP_SSL_VERIFY from env. With ssl_verify: false in config, sapcli forwards verify=False through the request, the plugin disables urllib3 InsecureRequestWarning explicitly (the user opted in), and the request goes through against a self-signed cert. - Cookie serialization keeps optional fields off the wire when not set (no 'domain: null' noise). HttpOnly is intentionally omitted - it is a browser-only directive and meaningless once we hand the cookie back to the server. - All failure modes (bad JSON, missing creds, network error, non-2xx response, no cookies returned) emit a 'message' field and exit non-zero, which is what HTTPExternalSessionInitializer surfaces as the AuthPluginError detail. Smoke-tested against a real ABAP at localhost:50001: 'sapcli abap systeminfo' through the plugin returns the same SID/userName/client as the built-in BasicAuth flow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 6 (final) of the auth-plugin track in features/auth_plugins.md. Without this commit every sapcli command spawns the plugin subprocess and re-authenticates - fine for BasicAuth, painful for browser-based SSO where the user has to click through every time. With the cache, the second and subsequent commands reuse the cookies (or token, or cert reference) the plugin returned, until it expires or the user passes --invalidate-cache. New AuthPluginResponseStore sits on top of the existing JSONFileStore primitive: atomic writes, 0o700/0o600 perms, corruption-tolerant reads. Same storage shape that backs the OAuth token cache. End-to-end measured against a real ABAP at localhost:50001: cold run 2.48s (plugin spawn + login), warm run 1.50s (cache hit, no subprocess), --invalidate-cache 2.08s (back to cold). The 1-second delta is the plugin's GET to /sap/bc/adt/core/discovery; eliminating it on every command is the whole point. Design decisions: - Cache key is the (context, connection, user) triple. The spec calls for the triple specifically so that changing the connection or user a context references mints a new key and the dev cookies do not get reused against prod. The pipe separator turns into '_' on disk via JSONFileStore's filename sanitiser - the on-disk filename is an implementation detail. - Cache is keyed at the CLI level, not deeper. The plugin protocol module (auth_plugin.py) and the session initializer (external_session_initializer.py) stay agnostic to the key shape; the CLI computes the triple and hands it to the initializer. Different frontends (e.g. a future Python API) can pick their own key derivation without forking the cache contract. - Missing expiration means "never expires" - same semantics as Token.is_expired. Cookie plugins typically have no idea when the server will invalidate the session; defaulting to "no expiration" caches indefinitely and lets the server be the source of truth. Plugins that DO know their token lifetime should set expiration explicitly; the cache will then mint a fresh response when it lapses. - The cache is process-wide via a singleton factory (get_response_store) - same pattern as get_token_store. Callers pass only the key; whether and how to persist is a deployment decision (today: file. Tomorrow: keyring, behind an env flag.) - cache_key=None disables caching entirely on the initializer. Tests and ad-hoc subprocess invocations that don't go through the CLI pipeline never accidentally write to disk. - Plugin error must not poison the cache. If run_plugin raises AuthPluginError, we propagate without calling store.set. Storing a half-built response would mask 'something is broken' on the next run and make diagnosis harder. - Expired entries are still readable from the store - the store does not police expiration. is_expired() is checked by the caller (the session initializer), which lets the same store later support 'use stale cookies + try refresh in background' if we want that. - --invalidate-cache lives at the top-level argument parser, not on subcommands. It is a system-wide concern (which credential blob am I using?) and a subcommand-level flag would be awkwardly duplicated. - Naive datetimes in expiration are interpreted as UTC. Better than crashing or silently treating them as local time, which would round-trip differently on different machines. - ConnectionInfo gained verify/ssl_server_cert in the previous commit; they round-trip through the cached AuthPluginResponse via content's to/from JSON the same way every other field does, no special handling needed. Tests cover: to_dict/to_json/is_expired on AuthPluginResponse (with naive datetimes, with/without expiration, leeway boundaries); the file store (round-trip, isolation between keys, delete, missing key, expired entries still readable, singleton factory); the initializer (no cache_key = no IO, hit, miss-and-store, expired-refresh, plugin error does not poison); CLI key derivation (triple changes with each component, None when auth_plugin not set, picks up --context, env override); --invalidate-cache (drops the entry before run, no-op without a key). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Until now, only adt_connection_from_args wired the
HTTPSessionInitializer seam. gcts_connection_from_args and
odata_connection_from_args constructed their Connection objects
directly with user/password and silently ignored args.auth_plugin and
the OAuth fields - the strategies were configured in sapcli but had
no effect on REST/OData commands.
Route both factories through _build_session_initializer so all three
HTTP-based connection types accept the same auth strategies:
adt -> /sap/bc/adt/core/discovery
rest -> /sap/bc/cts_abapvcs/system
odata -> /sap/opu/odata/<service>
End-to-end verified against the real ABAP at localhost:50001:
- 'sapcli gcts repolist' through the plugin returns the same data as
the BasicAuth flow (1.93s cold, 1.03s warm via the response cache).
- 'sapcli bsp stat' authenticates via the plugin against the OData
metadata endpoint and proceeds into the pyodata service call.
Design decisions:
- The cache key stays at (context, connection, user) - the same triple
serves all three connection types. ABAP session cookies are
server-wide, so a plugin response cached by an ADT command is reused
by the next REST or OData command in the same context. That is the
whole point of caching by logical session rather than by endpoint.
- conn_type values map to the design doc's enumeration ('adt', 'rest',
'odata'). Plugins that need to pick a protocol-specific auth flow
(e.g. a SAML plugin that hits ADT's discovery for ADT but the BSP
service root for OData) can dispatch on type; plugins that do not
care can ignore it.
- conn_path points at the connection's own login path. The
basic-auth-cookies plugin uses GET on this path; ABAP returns 200
with session cookies on all three. A plugin that wanted to be
cross-system safe could ignore the path and always hit
/sap/bc/adt/core/discovery, but defaulting to the type-native path
is the conservative choice when the system has any one of ADT, gCTS,
or OData disabled.
- RFC connections are intentionally NOT touched. RFC uses pyrfc with
its own auth model (SNC, native BasicAuth), and there is no session
cookie to deliver; the auth_plugin shape does not fit.
- This is also a fix for OAuth on gCTS/OData. Previously OAuth was
wired only for ADT; the same _build_session_initializer call now
unifies that behaviour. If a user has OAuth fully configured, they
now get bearer auth for gCTS and OData too.
Tests cover: BasicAuth fallback (session_initializer=None),
auth_plugin path (HTTPExternalSessionInitializer with the right
conn_type and conn_path), OAuth path (OAuthHTTPSessionInitializer) -
for both gcts_connection_from_args and odata_connection_from_args.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a brief mention of kubectl-style auth plugins to the README's configuration section (with a pointer to the deep dive) and a full 'Auth plugins' section to doc/configuration.md alongside the existing OAuth 2.0 section. The configuration.md additions: - New --invalidate-cache parameter entry. - 'auth_plugin' field on the users.<name> reference table, with a mutual-exclusivity note. - A new 'Use an auth plugin' bullet in the credentials-handling recommendations. - A complete 'Auth plugins' section covering: when to use a plugin (vs. OAuth/BasicAuth), how to enable one, response caching (location per OS, key shape, expiration semantics, --invalidate-cache), the request/response JSON envelope, the three content.type variants (cookie / http_authorization_header / certificates), failure handling, the bundled reference plugin, and a 'writing your own' guide that emphasises the language-agnostic stdin/stdout contract. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The on-disk response cache stores session cookies (effectively bearer credentials). Some deployments cannot tolerate that — corporate DLP, shared jump hosts, compliance regimes that forbid at-rest persistence of bearer material — and need a way to keep caching off entirely rather than scrubbing it after the fact with --invalidate-cache. Adds three coordinated overrides with the usual precedence (CLI > env > config > default false): - CLI flag: --auth-plugin-disable-cache - Env var: SAPCLI_AUTH_PLUGIN_DISABLE_CACHE (false tokens accepted) - Config: auth_plugin.disable_cache on the user definition When enabled, the initializer is constructed with cache_key=None (reusing the existing no-cache path in HTTPExternalSessionInitializer that already skips both reads and writes) and any pre-existing entry for the active context is deleted before the plugin runs — defense in depth, the user just told us they do not want secrets on disk. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.