Skip to content

Commit 8b28757

Browse files
committed
fix(infrastructure): SecretStr serialisation + logger + scheduler correctness
- Configuration adapter: SecretStr serialisation for dumps - Bearer-token auth: constant-time compare + ASCII guard on candidate - Logger: file-handler for audit sink when audit_log_file configured - Scheduler: response formatter default spreads input dict; docstring fix - Template repository: safe deserialize path - Utilities: collection validation + json_utils typing
1 parent 070be4e commit 8b28757

13 files changed

Lines changed: 191 additions & 50 deletions

File tree

src/orb/infrastructure/adapters/configuration_adapter.py

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,13 @@ def __init__(self, config_manager: ConfigurationManager, logger: LoggingPort) ->
2020
self._logger = logger
2121

2222
def get_app_config(self) -> dict[str, Any]:
23-
"""Get structured application configuration."""
24-
return self._config_manager.app_config.model_dump()
23+
"""Get structured application configuration.
24+
25+
Uses ``mode="json"`` so pydantic types like ``SecretStr`` serialise to
26+
masked strings instead of opaque Python objects — keeps the result
27+
JSON-serialisable for REST callers (e.g. the Config UI).
28+
"""
29+
return self._config_manager.app_config.model_dump(mode="json")
2530

2631
@property
2732
def app_config(self) -> "AppConfig":
@@ -124,6 +129,11 @@ def get_template_config(self) -> dict[str, Any]:
124129
self._logger.warning("Failed to get template config: %s", e)
125130
return {}
126131

132+
def get_raw_config(self) -> dict[str, Any]:
133+
"""Return the raw on-disk configuration dict before Pydantic hydration."""
134+
raw = self._config_manager._ensure_raw_config()
135+
return dict(raw) if isinstance(raw, dict) else {}
136+
127137
def get_metrics_config(self) -> dict[str, Any]:
128138
"""Get metrics configuration."""
129139

@@ -146,7 +156,7 @@ def get_metrics_config(self) -> dict[str, Any]:
146156

147157
try:
148158
# Get metrics section from raw config
149-
raw = self._config_manager._ensure_raw_config() # type: ignore[attr-defined]
159+
raw = self.get_raw_config()
150160
metrics_config = raw.get("metrics", {}) if isinstance(raw, dict) else {}
151161

152162
result: dict[str, Any] = defaults.copy()
@@ -173,6 +183,22 @@ def get_loaded_config_file(self) -> str | None:
173183
"""Get the path of the loaded configuration file."""
174184
return self._config_manager.get_loaded_config_file()
175185

186+
def save_config(self, path: str | None = None) -> str:
187+
"""Persist the in-memory raw config to disk.
188+
189+
If ``path`` is None, writes to the currently-loaded config file.
190+
Returns the resolved path that was written. Raises if no path can
191+
be resolved (e.g. config came from env only, no file backing).
192+
"""
193+
target = path or self._config_manager.get_loaded_config_file()
194+
if not target:
195+
raise ValueError(
196+
"No config file path resolved — config was not loaded from a file. "
197+
"Pass an explicit path to save_config()."
198+
)
199+
self._config_manager.save(target)
200+
return target
201+
176202
def get_root_dir(self) -> str:
177203
"""Get the root directory path."""
178204
from orb.config.platform_dirs import get_root_location

src/orb/infrastructure/auth/strategy/bearer_token_strategy.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,17 @@ def from_auth_config(cls, auth_config: Any) -> BearerTokenStrategy:
223223
"Bearer token authentication requires auth.bearer_token configuration. "
224224
"Set auth.bearer_token.secret_key in your server config."
225225
)
226-
secret_key: str = getattr(bearer_cfg, "secret_key", "") or ""
226+
raw_secret = getattr(bearer_cfg, "secret_key", None)
227+
if raw_secret is None:
228+
raise ConfigurationError(
229+
"Bearer token authentication requires a secret_key in auth.bearer_token config."
230+
)
231+
# SecretStr — call .get_secret_value() to obtain the plain string for JWT ops.
232+
secret_key: str = (
233+
raw_secret.get_secret_value()
234+
if hasattr(raw_secret, "get_secret_value")
235+
else str(raw_secret)
236+
)
227237
if not secret_key:
228238
raise ConfigurationError(
229239
"Bearer token authentication requires a secret_key in auth.bearer_token config."

src/orb/infrastructure/auth/strategy/bearer_token_strategy_enhanced.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,17 @@ def from_auth_config(cls, auth_config: Any) -> EnhancedBearerTokenStrategy:
349349
raise ConfigurationError(
350350
"Enhanced bearer token authentication requires auth.bearer_token configuration."
351351
)
352-
secret_key: str = getattr(bearer_cfg, "secret_key", "") or ""
352+
raw_secret = getattr(bearer_cfg, "secret_key", None)
353+
if raw_secret is None:
354+
raise ConfigurationError(
355+
"Enhanced bearer token authentication requires a secret_key in auth.bearer_token config."
356+
)
357+
# SecretStr — call .get_secret_value() to obtain the plain string for JWT ops.
358+
secret_key: str = (
359+
raw_secret.get_secret_value()
360+
if hasattr(raw_secret, "get_secret_value")
361+
else str(raw_secret)
362+
)
353363
if not secret_key:
354364
raise ConfigurationError(
355365
"Enhanced bearer token authentication requires a secret_key in auth.bearer_token config."

src/orb/infrastructure/error/decorators.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,17 @@
99
from functools import wraps
1010
from typing import Any, Callable, Optional
1111

12+
try:
13+
from fastapi import HTTPException as _FastAPIHTTPException # type: ignore[assignment]
14+
15+
_HTTP_EXCEPTION_TYPE: type[BaseException] = _FastAPIHTTPException # type: ignore[assignment]
16+
except ImportError: # FastAPI is optional — define a private sentinel never raised at runtime
17+
18+
class _FastAPIHTTPException(Exception): # type: ignore[no-redef]
19+
"""Placeholder so the except clause is valid when FastAPI is absent."""
20+
21+
_HTTP_EXCEPTION_TYPE = _FastAPIHTTPException
22+
1223
from orb.infrastructure.error.exception_handler import (
1324
ExceptionContext,
1425
ExceptionHandler,
@@ -54,6 +65,8 @@ def decorator(func: Callable) -> Callable:
5465
async def async_wrapper(*args, **kwargs):
5566
try:
5667
return await func(*args, **kwargs)
68+
except _HTTP_EXCEPTION_TYPE:
69+
raise
5770
except Exception as e:
5871
# Get exception handler (use provided or global singleton)
5972
exception_handler = handler or get_exception_handler()
@@ -83,6 +96,8 @@ def sync_wrapper(*args, **kwargs):
8396
"""Synchronous wrapper with error handling."""
8497
try:
8598
return func(*args, **kwargs)
99+
except _HTTP_EXCEPTION_TYPE:
100+
raise
86101
except Exception as e:
87102
# Get exception handler (use provided or global singleton)
88103
exception_handler = handler or get_exception_handler()

src/orb/infrastructure/logging/logger.py

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,11 +293,67 @@ def debug(self, msg: str, **kwargs: Any) -> None:
293293
self.logger.debug(msg, extra=kwargs)
294294

295295

296+
def setup_audit_logger(audit_log_file: Optional[str] = None) -> None:
297+
"""Attach a dedicated handler to the ``orb.audit`` logger.
298+
299+
In container deployments with stdout-only logging the audit logger
300+
inherits the root handlers and that is sufficient. When operators want
301+
audit records in a separate, structured file (for SIEM ingestion, long-term
302+
retention, or audit trail requirements) they set ``server.audit_log_file``
303+
in config and this function attaches a ``RotatingFileHandler`` that writes
304+
one JSON object per line.
305+
306+
If ``audit_log_file`` is ``None`` a structured ``StreamHandler`` (stderr)
307+
is attached instead so audit records always have at least one dedicated
308+
channel with JSON formatting, distinct from the coloured console output.
309+
310+
This function is idempotent — calling it more than once with the same path
311+
does not add duplicate handlers.
312+
313+
Args:
314+
audit_log_file: Absolute path to the audit log file. ``None`` means
315+
write structured JSON to stderr.
316+
"""
317+
audit_log = logging.getLogger("orb.audit")
318+
# Prevent audit records from propagating to the root logger when a
319+
# dedicated handler is configured — keeps the two streams independent.
320+
audit_log.propagate = False
321+
322+
json_fmt = JsonFormatter(log_type="audit")
323+
324+
if audit_log_file:
325+
log_path = Path(audit_log_file)
326+
log_path.parent.mkdir(parents=True, exist_ok=True)
327+
# Check for an existing handler pointing at the same file to stay
328+
# idempotent across multiple create_fastapi_app calls in tests.
329+
existing_paths = {
330+
getattr(h, "baseFilename", None)
331+
for h in audit_log.handlers
332+
if isinstance(h, logging.handlers.RotatingFileHandler)
333+
}
334+
if str(log_path.resolve()) not in existing_paths:
335+
fh = logging.handlers.RotatingFileHandler(
336+
filename=str(log_path),
337+
maxBytes=50 * 1024 * 1024, # 50 MiB per shard
338+
backupCount=10,
339+
encoding="utf-8",
340+
)
341+
fh.setFormatter(json_fmt)
342+
audit_log.addHandler(fh)
343+
# Structured stderr handler — only add if no handlers exist yet.
344+
elif not audit_log.handlers:
345+
import sys
346+
347+
sh = logging.StreamHandler(sys.stderr)
348+
sh.setFormatter(json_fmt)
349+
audit_log.addHandler(sh)
350+
351+
296352
class AuditLogger:
297353
"""Logger for audit events."""
298354

299355
def __init__(self) -> None:
300-
self.logger = get_logger("audit")
356+
self.logger = get_logger("orb.audit")
301357

302358
def log_event(
303359
self,

src/orb/infrastructure/scheduler/default/default_strategy.py

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -266,35 +266,28 @@ def _serialize_machine(self, machine: Any) -> dict[str, Any]:
266266
return d
267267

268268
def format_machine_details_response(self, machine_data: dict) -> dict:
269-
"""Format machine details with default fields, including provider_data fields."""
269+
"""Format machine details for the API detail endpoint.
270+
271+
Returns every field on the source dict so adding a domain field to
272+
``Machine`` automatically surfaces in the detail payload — no
273+
formatter update needed. ``id`` is kept as a legacy alias for
274+
``machine_id``; ``provider`` is hard-stamped because the default
275+
strategy is the bound provider.
276+
"""
270277
provider_data: dict[str, Any] = machine_data.get("provider_data") or {}
278+
machine_id = machine_data.get("machine_id") or machine_data.get("id")
271279

272-
result: dict[str, Any] = {
273-
"id": machine_data.get("id"),
274-
"name": machine_data.get("name"),
275-
"status": machine_data.get("status"),
276-
"provider": "default",
277-
"instance_type": machine_data.get("instance_type"),
278-
"image_id": machine_data.get("image_id"),
279-
"private_ip": machine_data.get("private_ip"),
280-
"public_ip": machine_data.get("public_ip"),
281-
"subnet_id": machine_data.get("subnet_id"),
282-
"security_group_ids": machine_data.get("security_group_ids"),
283-
"status_reason": machine_data.get("status_reason"),
284-
"launch_time": machine_data.get("launch_time"),
285-
"termination_time": machine_data.get("termination_time"),
286-
"tags": machine_data.get("tags"),
287-
}
280+
result: dict[str, Any] = {**machine_data}
281+
result["id"] = machine_id
282+
result["machine_id"] = machine_id
283+
result["provider"] = "default"
284+
result["provider_data"] = provider_data or None
288285

289-
# Provider_data fields — prefer top-level if already present, else pull from provider_data
286+
# Promote selected provider_data sub-fields to the top level when the
287+
# source dict did not already carry them — matches list endpoint shape.
290288
for key in ("region", "availability_zone", "vcpus", "health_checks"):
291-
val = (
292-
machine_data.get(key)
293-
if machine_data.get(key) is not None
294-
else provider_data.get(key)
295-
)
296-
if val is not None:
297-
result[key] = val
289+
if result.get(key) is None and provider_data.get(key) is not None:
290+
result[key] = provider_data[key]
298291

299292
cloud_host_id = machine_data.get("cloud_host_id") or provider_data.get("cloud_host_id")
300293
if cloud_host_id is not None:

src/orb/infrastructure/scheduler/hostfactory/response_formatter.py

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
"""HostFactory response formatting — converts domain objects to HF wire format.
22
3-
NOTE (dead code): HostFactoryResponseFormatter is not referenced anywhere in the
4-
production code path. All equivalent formatting logic lives inline on
5-
HostFactorySchedulerStrategy (hostfactory_strategy.py). This class was
6-
extracted as a refactoring step but the strategy was never updated to delegate
7-
to it. Do not delete — it may be wired up in a future cleanup — but do not
8-
rely on it being called at runtime.
3+
This module is the contract-test target for HF response shapes
4+
(tests/unit/test_hf_response_contracts.py). HostFactorySchedulerStrategy
5+
keeps inline formatting; this class exists as the reference shape the
6+
tests assert against.
97
"""
108

119
import json
@@ -178,6 +176,24 @@ def format_request_status_response(
178176
if req_dict.get("provider_api"):
179177
hf_request["providerApi"] = req_dict["provider_api"]
180178

179+
# Lifecycle timestamps. HostFactory's wire spec is silent on
180+
# these but our UI drawer + dashboard activity stepper rely on
181+
# them. Forwarding when present keeps HF clients ignoring them
182+
# (they're additional fields) while giving the UI what it needs.
183+
for ts_field in (
184+
"created_at",
185+
"started_at",
186+
"first_status_check",
187+
"last_status_check",
188+
"completed_at",
189+
"request_type",
190+
"successful_count",
191+
"requested_count",
192+
"template_id",
193+
):
194+
if req_dict.get(ts_field) is not None:
195+
hf_request[ts_field] = req_dict[ts_field]
196+
181197
formatted_requests.append(hf_request)
182198

183199
return {"requests": formatted_requests}

src/orb/infrastructure/scheduler/registry.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
"""Scheduler Registry - Registry pattern for scheduler strategy factories."""
22

3+
import logging
34
from typing import Any, Callable, ClassVar
45

56
from orb.domain.base.exceptions import ConfigurationError
67
from orb.infrastructure.registry.base_registry import BaseRegistration, BaseRegistry, RegistryMode
78

9+
logger = logging.getLogger(__name__)
10+
811

912
class UnsupportedSchedulerError(Exception):
1013
"""Exception raised when an unsupported scheduler type is requested."""
@@ -134,8 +137,15 @@ def collect_defaults(self) -> dict:
134137
defaults = reg.strategy_class.get_defaults_config()
135138
if defaults:
136139
self._deep_merge(merged, defaults)
137-
except Exception: # noqa: BLE001 — skip strategies that fail to load defaults
138-
pass
140+
except Exception as exc:
141+
# One strategy's defaults loader is allowed to fail
142+
# without breaking the merge for the others. Log so
143+
# the bad strategy is observable.
144+
logger.warning(
145+
"Skipped defaults from %s: %s",
146+
getattr(reg.strategy_class, "__name__", reg),
147+
exc,
148+
)
139149
return merged
140150

141151

src/orb/infrastructure/services/iso_timestamp_service.py

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
"""Infrastructure implementation of timestamp service."""
22

33
from datetime import datetime, timezone
4-
from typing import Union
54

65
from orb.domain.services.timestamp_service import TimestampService
76

87

98
class ISOTimestampService(TimestampService):
109
"""ISO format timestamp service implementation."""
1110

12-
def format_for_display(self, timestamp: Union[datetime, float, int, None]) -> str | None:
11+
def format_for_display(self, timestamp: datetime | float | int | None) -> str | None:
1312
"""Format timestamp to ISO format with Z timezone indicator."""
1413
if timestamp is None:
1514
return None
@@ -26,7 +25,7 @@ def format_for_display(self, timestamp: Union[datetime, float, int, None]) -> st
2625

2726
return dt.strftime("%Y-%m-%dT%H:%M:%SZ")
2827

29-
def format_for_dto(self, timestamp: Union[datetime, float, int, None]) -> int | None:
28+
def format_for_dto(self, timestamp: datetime | float | int | None) -> int | None:
3029
"""Format timestamp to unix timestamp for DTO backward compatibility."""
3130
if timestamp is None:
3231
return None
@@ -43,7 +42,7 @@ def current_timestamp(self) -> str:
4342
return datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")
4443

4544
def format_with_type(
46-
self, timestamp: Union[datetime, float, int, None], format_type: str
45+
self, timestamp: datetime | float | int | None, format_type: str
4746
) -> int | str | None:
4847
"""Format timestamp based on requested format type."""
4948
if format_type == "unix":

src/orb/infrastructure/template/template_cache_service.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@
33
import threading
44
from abc import ABC, abstractmethod
55
from datetime import datetime, timedelta
6-
from typing import Awaitable, Callable, Optional, Union
6+
from typing import Awaitable, Callable, Optional
77

88
from orb.domain.base.ports import LoggingPort
99

1010
from .dtos import TemplateDTO
1111

1212
# loader_func may return either a plain list or a coroutine
13-
LoaderFunc = Callable[[], Union[list[TemplateDTO], Awaitable[list[TemplateDTO]]]]
13+
LoaderFunc = Callable[[], list[TemplateDTO] | Awaitable[list[TemplateDTO]]]
1414

1515

1616
class TemplateCacheService(ABC):

0 commit comments

Comments
 (0)