Skip to content

http: introduce auth-plugin protocol module#161

Merged
jfilak merged 1 commit into
masterfrom
auth_http_plugin
May 9, 2026
Merged

http: introduce auth-plugin protocol module#161
jfilak merged 1 commit into
masterfrom
auth_http_plugin

Conversation

@jfilak

@jfilak jfilak commented May 9, 2026

Copy link
Copy Markdown
Owner

Adds sap/http/auth_plugin.py: the request/response shape and the run_plugin() driver for kubectl-style external authentication helpers. This is step 1 of the auth-plugin track in features/auth_plugins.md and is independent of CLI wiring and the response cache - those land in follow-up commits.

Why a kubectl-style plugin (vs. building auth methods in-tree): sapcli must support SAML2/SSO and Windows-cert SSO without taking on browser-automation or Win32-API dependencies. Spawning an external command and exchanging JSON over stdin/stdout keeps sapcli's dependency surface minimal and lets each plugin be implemented in whatever language fits the platform (e.g. playwright on Windows).

Design decisions:

  • subprocess.run with check=False and no timeout. Browser-based plugins legitimately wait minutes for the user to log in; a default timeout would kill them mid-flow. check=False so we surface our own AuthPluginError with stdout+stderr included instead of a bare CalledProcessError.

  • AuthPluginError derives from SAPCliError so the CLI entry point prints a friendly message instead of a stack trace, matching every other user-visible error in the project.

  • OSError catches both FileNotFoundError (plugin not on PATH) and PermissionError (plugin not executable); wrapping at OSError keeps the failure path uniform without enumerating each subclass.

  • Response validation happens here for the envelope (message, content, optional expiration) but does NOT inspect content's internal shape - that is the session-initializer's job, which dispatches on content.type. Keeps this module's contract narrow.

  • Expiration normalises a trailing 'Z' to '+00:00' before datetime.fromisoformat. Python 3.10's parser does not understand the Z suffix; doing it here lets the rest of the codebase stay portable.

  • ConnectionInfo carries port as str (matching the wire format in the spec, e.g. "44300") rather than int. The CLI parses port as int, so the caller is responsible for coercion - we do not silently mutate user input.

  • ConnectionInfo.sysnr is Optional[str] (default None). HTTP-only plugins have no use for the SAP system / instance number; making it required would force every caller to pass fake data. RFC-aware plugins (which need it to derive the gateway port 33) set it explicitly. The field is always emitted in to_dict (as null when unset) so plugin authors see a stable wire shape.

  • parameters defaults to {} when None is passed, so callers do not have to special-case "no plugin parameters" before calling.

Adds sap/http/auth_plugin.py: the request/response shape and the
run_plugin() driver for kubectl-style external authentication helpers.
This is step 1 of the auth-plugin track in features/auth_plugins.md and
is independent of CLI wiring and the response cache - those land in
follow-up commits.

Why a kubectl-style plugin (vs. building auth methods in-tree):
sapcli must support SAML2/SSO and Windows-cert SSO without taking on
browser-automation or Win32-API dependencies. Spawning an external
command and exchanging JSON over stdin/stdout keeps sapcli's
dependency surface minimal and lets each plugin be implemented in
whatever language fits the platform (e.g. playwright on Windows).

Design decisions:

- subprocess.run with check=False and no timeout. Browser-based plugins
  legitimately wait minutes for the user to log in; a default timeout
  would kill them mid-flow. check=False so we surface our own
  AuthPluginError with stdout+stderr included instead of a bare
  CalledProcessError.

- AuthPluginError derives from SAPCliError so the CLI entry point
  prints a friendly message instead of a stack trace, matching every
  other user-visible error in the project.

- OSError catches both FileNotFoundError (plugin not on PATH) and
  PermissionError (plugin not executable); wrapping at OSError keeps
  the failure path uniform without enumerating each subclass.

- Response validation happens here for the envelope (message, content,
  optional expiration) but does NOT inspect content's internal shape -
  that is the session-initializer's job, which dispatches on
  content.type. Keeps this module's contract narrow.

- Expiration normalises a trailing 'Z' to '+00:00' before
  datetime.fromisoformat. Python 3.10's parser does not understand the
  Z suffix; doing it here lets the rest of the codebase stay portable.

- ConnectionInfo carries port as str (matching the wire format in the
  spec, e.g. "44300") rather than int. The CLI parses port as int, so
  the caller is responsible for coercion - we do not silently mutate
  user input.

- ConnectionInfo.sysnr is Optional[str] (default None). HTTP-only
  plugins have no use for the SAP system / instance number; making it
  required would force every caller to pass fake data. RFC-aware
  plugins (which need it to derive the gateway port 33<sysnr>) set
  it explicitly. The field is always emitted in to_dict (as null when
  unset) so plugin authors see a stable wire shape.

- parameters defaults to {} when None is passed, so callers do not
  have to special-case "no plugin parameters" before calling.

31 unit tests cover serialization (including sysnr present/absent),
response parsing (happy path plus schema-drift edge cases like
array-instead-of-object, null/empty expiration, Z-suffix and offset
forms), the subprocess invocation contract (argv shape, stdin payload,
capture_output, encoding, no-timeout, no-check), and every failure
branch (missing executable, permission denied, non-zero exit, invalid
JSON, missing required fields).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b59dd9da-dddf-4353-9a51-6a95a2f78fc4

📥 Commits

Reviewing files that changed from the base of the PR and between 19a8a91 and a7ccdde.

📒 Files selected for processing (2)
  • sap/http/auth_plugin.py
  • test/unit/test_sap_http_auth_plugin.py

📝 Walkthrough

Walkthrough

This pull request introduces a new external authentication plugin protocol for sapcli. It defines typed data models (ConnectionInfo, AuthPluginRequest, AuthPluginResponse), implements subprocess execution via run_plugin, and provides comprehensive unit test coverage. The implementation handles JSON serialization, ISO-8601 expiration parsing, subprocess communication via stdin/stdout, and error handling.

Changes

External Auth Plugin Protocol

Layer / File(s) Summary
Module Foundation & Exception
sap/http/auth_plugin.py
Module introduces auth plugin subprocess protocol; AuthPluginError exception signals plugin invocation or output validation failures.
Request/Response Data Models
sap/http/auth_plugin.py
ConnectionInfo holds connection metadata with optional sysnr and to_dict() serializer. AuthPluginRequest wraps connection and parameters with to_dict() and to_json(). AuthPluginResponse parses stdout with required message and content fields, optional expiration, and from_dict()/from_json() classmethods.
Expiration Parsing
sap/http/auth_plugin.py
_parse_expiration converts ISO-8601 timestamps to datetime, normalizes Z suffix to +00:00, returns None for falsy input, and raises ValueError for invalid format.
Plugin Subprocess Execution
sap/http/auth_plugin.py
run_plugin executes external command, sends AuthPluginRequest JSON via stdin, captures UTF-8 stdout, parses response, and wraps subprocess exceptions and validation failures into AuthPluginError with diagnostic output.
Unit Tests
test/unit/test_sap_http_auth_plugin.py
Test helpers validate ConnectionInfo and AuthPluginRequest serialization, AuthPluginResponse parsing from dict/JSON with required-field validation, AuthPluginError inheritance, subprocess invocation contract (argv, stdin, UTF-8 text capture), success response parsing, and failure handling (missing executable, permission errors, non-zero exit codes, invalid JSON, malformed responses).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'http: introduce auth-plugin protocol module' accurately and concisely summarizes the main change: adding a new auth-plugin protocol module to the http package.
Description check ✅ Passed The description provides detailed context about the auth-plugin module implementation, design rationale, and architectural decisions, all directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch auth_http_plugin

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jfilak jfilak merged commit 8eddf92 into master May 9, 2026
3 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant