-
Notifications
You must be signed in to change notification settings - Fork 33
feat(http-power): support HTTP Digest authentication #1001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import requests | ||
| from jumpstarter_driver_power.common import PowerReading | ||
| from jumpstarter_driver_power.driver import PowerInterface | ||
| from requests.auth import HTTPBasicAuth, HTTPDigestAuth | ||
|
|
||
| from jumpstarter.driver import Driver, export | ||
|
|
||
|
|
@@ -34,9 +35,16 @@ class HttpBasicAuth: | |
| password: str = field(default="") | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class HttpDigestAuth: | ||
| user: str = field(default="") | ||
| password: str = field(default="") | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class HttpAuthConfig: | ||
| basic: Optional[HttpBasicAuth] = field(default=None) | ||
| digest: Optional[HttpDigestAuth] = field(default=None) | ||
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
|
|
@@ -68,13 +76,24 @@ def __post_init__(self): | |
| self.auth = HttpAuthConfig(**self.auth) | ||
| if self.auth and self.auth.basic and isinstance(self.auth.basic, dict): | ||
| self.auth.basic = HttpBasicAuth(**self.auth.basic) | ||
|
|
||
| if self.auth and self.auth.digest and isinstance(self.auth.digest, dict): | ||
| self.auth.digest = HttpDigestAuth(**self.auth.digest) | ||
| if self.auth and self.auth.basic and self.auth.digest: | ||
| raise ValueError("auth.basic and auth.digest are mutually exclusive, configure only one of them") | ||
|
Comment on lines
+79
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Check authentication blocks by presence, not truthiness. An empty mapping is falsy. With Use Suggested presence checks- if self.auth and self.auth.basic and isinstance(self.auth.basic, dict):
+ if self.auth and self.auth.basic is not None and isinstance(self.auth.basic, dict):
self.auth.basic = HttpBasicAuth(**self.auth.basic)
- if self.auth and self.auth.digest and isinstance(self.auth.digest, dict):
+ if self.auth and self.auth.digest is not None and isinstance(self.auth.digest, dict):
self.auth.digest = HttpDigestAuth(**self.auth.digest)
- if self.auth and self.auth.basic and self.auth.digest:
+ if self.auth and self.auth.basic is not None and self.auth.digest is not None:
raise ValueError(...)
...
- if self.auth.basic:
+ if self.auth.basic is not None:
...
- if self.auth.digest:
+ if self.auth.digest is not None:Also applies to: 86-92 🤖 Prompt for AI Agents
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that when running this we get dicts instead of the structs and we need to reconstruct the structs, the code for basic auth already works and follows the same pattern. If we switch to "is not None" we should do it for all L75-L81
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this change is made, update all relevant checks in 🐇🔎 ✏️ Learnings added
You are interacting with an AI system. |
||
|
|
||
| def _build_auth(self): | ||
| """Build the requests auth handler from the configured credentials""" | ||
| if not self.auth: | ||
| return None | ||
| if self.auth.basic: | ||
| return HTTPBasicAuth(self.auth.basic.user, self.auth.basic.password) | ||
| if self.auth.digest: | ||
| return HTTPDigestAuth(self.auth.digest.user, self.auth.digest.password) | ||
| return None | ||
|
|
||
| def _make_http_request(self, endpoint_config: HttpEndpointConfig) -> str: | ||
| """Make HTTP request to the specified endpoint""" | ||
| auth = None | ||
| if self.auth and self.auth.basic: | ||
| auth = (self.auth.basic.user, self.auth.basic.password) | ||
| auth = self._build_auth() | ||
|
Comment on lines
+79
to
+96
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Add regression tests for Digest authentication. The supplied Run As per coding guidelines, “After generation, review generated files, implement driver logic in 🤖 Prompt for AI AgentsSource: Coding guidelines
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to the specific tests for the digest authentication.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline python/packages/jumpstarter-driver-http-power/jumpstarter_driver_http_power/driver_test.py --items all
rg -n -C 5 '(HTTPBasicAuth|HTTPDigestAuth|_build_auth|_make_http_request|requests\.request|HttpAuthConfig|HttpPower)' \
python/packages/jumpstarter-driver-http-power/jumpstarter_driver_http_power/driver_test.py
rg -n -C 3 'pkg-test-jumpstarter-driver-http-power|pkg-ty-jumpstarter-driver-http-power|^test:|^lint-fix:' \
MakefileLength of output: 5661 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,120p' python/packages/jumpstarter-driver-http-power/jumpstarter_driver_http_power/driver_test.py
sed -n '120,190p' python/packages/jumpstarter-driver-http-power/jumpstarter_driver_http_power/driver_test.py
rg -n -C 4 'pkg-test|pkg-ty|lint-fix' \
python/packages/jumpstarter-driver-http-power/Makefile MakefileLength of output: 6427 @mangelajo, the test setup supports the requested coverage. The tests can remain localized to
🧬 Code Graph Analysis Results
|
||
| method = endpoint_config.method.upper() | ||
| url = endpoint_config.url | ||
| kwargs = { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.