Conversation
WalkthroughAdds API key–based authentication for ingress routes: a new AuthConfig model and Settings.auth, utilities to resolve auth keys per URL, and route registration changes that inject an API key dependency to enforce auth on protected endpoints. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIIngress
participant FastAPIRoute as "FastAPI Route"
participant Verify as "_verify_api_key"
participant Utils as "get_auth_keys_by_url"
participant Settings
Client->>APIIngress: HTTP request to path
APIIngress->>FastAPIRoute: Register/Invoke route (with dependencies)
FastAPIRoute->>Verify: Dependency executed (if route protected)
Verify->>Client: Read Authorization/API-Key header
Verify->>Utils: get_auth_keys_by_url(path)
Utils->>Settings: Read auth configuration
Settings-->>Utils: Return auth rules/keys
Utils-->>Verify: Return allowed keys (or None)
alt allowed keys and header matches
Verify-->>FastAPIRoute: Pass (dependency satisfied)
FastAPIRoute-->>Client: Handler executes, returns 200
else missing header
Verify-->>Client: HTTP 403 Not authenticated
else invalid key
Verify-->>Client: HTTP 401 Invalid API Key
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/framex/config.py (2)
59-64: Consider extracting error message to a custom exception class.The validation logic is correct and ensures special_auth_keys URLs are covered by auth_urls rules. However, the static analysis tool flags a style concern about inline error messages.
You may optionally refactor to address the TRY003 hint:
+class AuthConfigError(ValueError): + """Raised when authentication configuration is invalid.""" + pass + class AuthConfig(BaseModel): ... @model_validator(mode="after") def validate_special_auth_urls(self) -> Self: for special_url in self.special_auth_keys: if not self._is_url_allowed(special_url): - raise ValueError(f"special_auth_keys url '{special_url}' is not covered by any auth_urls rule") + raise AuthConfigError(f"special_auth_keys url '{special_url}' is not covered by any auth_urls rule") return self
66-76: Consider reducing code duplication with get_auth_keys_by_url.The URL matching logic in
_is_url_allowedis nearly identical to the protection check inget_auth_keys_by_url(lines 63-69 in src/framex/utils.py). Consider extracting this logic to a shared helper to maintain consistency and reduce duplication.For example, you could add a shared helper in
src/framex/utils.py:def is_url_protected(url: str, auth_urls: list[str]) -> bool: """Check if a URL is protected by any auth_urls rule.""" for rule in auth_urls: if rule == url: return True if rule.endswith("/*") and url.startswith(rule[:-1]): return True return FalseThen use it in both locations:
- In
AuthConfig._is_url_allowed:return is_url_protected(url, self.auth_urls)- In
get_auth_keys_by_url:is_protected = is_url_protected(url, auth_config.auth_urls)src/framex/driver/ingress.py (2)
22-22: Consider using a more conventional header name for API keys.While "Authorization" works, it's typically reserved for Bearer tokens in REST APIs. The conventional header name for API keys is "X-API-Key". Consider whether this aligns with your API design standards.
If you prefer to follow the API key convention:
-api_key_header = APIKeyHeader(name="Authorization", auto_error=True) +api_key_header = APIKeyHeader(name="X-API-Key", auto_error=True)Note: This would require updating test files accordingly.
101-113: Auth dependency implementation is correct.The dependency injection pattern properly validates API keys and raises HTTP 401 for unauthorized requests. The closure correctly captures
auth_keysper route.Minor optimization: Line 107's
not api_keycheck is redundant sinceAPIKeyHeader(auto_error=True)already ensures the header is present. You can simplify to:- if not api_key or api_key not in auth_keys: + if api_key not in auth_keys:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pytest.ini(1 hunks)src/framex/config.py(3 hunks)src/framex/driver/ingress.py(6 hunks)src/framex/utils.py(1 hunks)tests/api/test_echo.py(1 hunks)tests/test_config.py(1 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/framex/driver/ingress.py (1)
src/framex/utils.py (1)
get_auth_keys_by_url(58-92)
tests/test_config.py (1)
src/framex/config.py (1)
AuthConfig(54-76)
tests/api/test_echo.py (2)
book/theme/pagetoc.js (1)
headers(34-34)tests/conftest.py (1)
client(55-57)
🪛 Ruff (0.14.8)
src/framex/driver/ingress.py
108-111: Abstract raise to an inner function
(TRY301)
src/framex/config.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
pytest.ini (1)
29-30: LGTM!The test configuration correctly sets up authentication keys and protected URLs for testing the new authentication system.
tests/api/test_echo.py (1)
8-13: LGTM!The test correctly includes the Authorization header matching the configured general auth key, properly exercising the new authentication flow.
tests/test_config.py (2)
16-33: LGTM!The test cases effectively validate AuthConfig with various URL patterns including wildcards and exact matches, ensuring proper configuration validation.
35-42: LGTM!The negative test case properly validates that misconfigured special_auth_keys (URLs not covered by auth_urls) raise a ValidationError with an informative message.
tests/test_utils.py (1)
37-52: LGTM!The test comprehensively validates
get_auth_keys_by_urllogic across multiple scenarios: unprotected URLs, wildcard matching, exact matching, special keys, and general fallback. Line 52 correctly verifies that exact URL matches don't protect subpaths.src/framex/utils.py (2)
58-72: LGTM!The function correctly determines URL protection status using both exact matching and wildcard prefix matching (for rules ending with
/*). The local import ofsettingsappropriately avoids potential circular dependency issues.
74-90: LGTM!The special key resolution logic is well-implemented: exact matches take highest priority, followed by the longest matching wildcard prefix, ensuring more specific rules override general ones.
src/framex/config.py (2)
54-57: LGTM!The AuthConfig fields are well-defined with appropriate default factories to avoid mutable default issues. Field names clearly convey their purposes.
95-95: LGTM!The integration of AuthConfig into Settings is correct and follows the existing pattern for other configuration models.
src/framex/driver/ingress.py (2)
47-57: LGTM!The authentication key resolution is correctly performed for each plugin API and properly passed to the route registration method.
115-122: LGTM!The dependencies are correctly passed to
add_api_route, ensuring authentication is enforced before the route handler executes when required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/framex/driver/ingress.py (1)
101-114: Consider extracting the exception raising logic (optional refactor).The authentication dependency injection logic is correct and functional. However, the static analysis tool suggests abstracting the
raisestatement to an inner function for better testability and separation of concerns.🔎 View suggested refactor
+ def _raise_invalid_key_error(api_key: str, path: str) -> None: + logger.error(f"Unauthorized access attempt with API Key({api_key}) for API({path})") + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail=f"Invalid API Key({api_key}) for API({path})", + ) + # Inject auth dependency if needed dependencies = [] if auth_keys is not None: logger.debug(f"API({path}) with tags {tags} requires auth.") def _verify_api_key(api_key: str = Depends(api_key_header)) -> None: if api_key not in auth_keys: - logger.error(f"Unauthorized access attempt with API Key({api_key}) for API({path})") - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail=f"Invalid API Key({api_key}) for API({path})", - ) + _raise_invalid_key_error(api_key, path) dependencies.append(Depends(_verify_api_key))src/framex/config.py (1)
56-66: LGTM! Excellent defensive validation for auth configuration.The
AuthConfigmodel is well-structured with appropriate defaults and a validator that ensures allspecial_auth_keysURLs are covered byauth_urlsrules. This prevents configuration errors at startup.The static analysis tool suggests moving the error message to a constant or using a shorter inline message, but the current implementation is clear and acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/framex/config.py(4 hunks)src/framex/driver/ingress.py(6 hunks)src/framex/utils.py(1 hunks)tests/api/test_echo.py(1 hunks)tests/test_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/api/test_echo.py (1)
tests/conftest.py (1)
client(55-57)
src/framex/driver/ingress.py (2)
src/framex/utils.py (2)
escape_tag(26-28)get_auth_keys_by_url(68-95)src/framex/driver/application.py (1)
create_fastapi_application(23-128)
src/framex/config.py (1)
src/framex/utils.py (1)
is_url_protected(58-65)
🪛 Ruff (0.14.8)
src/framex/driver/ingress.py
109-112: Abstract raise to an inner function
(TRY301)
src/framex/config.py
65-65: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
tests/api/test_echo.py (4)
8-13: LGTM! Clean test structure with authentication header.The test properly includes the Authorization header and validates the successful response.
16-20: LGTM! Good coverage for missing API key scenario.The test correctly validates that a request without an API key returns 403 with "Not authenticated" message.
23-28: LGTM! Good coverage for invalid API key scenario.The test correctly validates that an invalid API key returns 401 with a specific error message including both the invalid key and the API path.
31-56: No action needed. Thetest_echo_modelandtest_echo_streamendpoints are not protected by authentication becauseauth_urlsis empty by default. The tests correctly omit Authorization headers.src/framex/utils.py (2)
58-65: LGTM! Clean URL protection checking logic.The function correctly handles both exact matches and wildcard patterns ending with
/*. The implementation is straightforward and correct.
68-95: LGTM! Well-structured auth key resolution with proper fallback logic.The function correctly:
- Returns None for unprotected URLs
- Handles exact matches in special_auth_keys
- Finds the longest matching wildcard prefix
- Falls back to general_auth_keys
The runtime import of settings is a good pattern to avoid circular dependencies.
src/framex/driver/ingress.py (4)
22-22: Verify that auto_error=True produces the expected error message.The
APIKeyHeaderis configured withauto_error=True, which automatically raises a 403 error when the Authorization header is missing. Ensure this produces the exact error message expected by the test: "Not authenticated".The FastAPI library should handle this correctly, but please verify the error message format matches test expectations.
47-58: LGTM! Clean integration of auth key resolution into route registration.The code properly retrieves auth keys for each URL and passes them to the route registration. The logic is clear and correct.
60-70: LGTM! Clean signature extension.The
auth_keysparameter is properly added with the correct type annotation.
116-123: LGTM! Proper dependency injection into route registration.The dependencies list is correctly passed to
add_api_route, enabling authentication enforcement.src/framex/config.py (1)
85-85: LGTM! Clean integration of AuthConfig into Settings.The
authfield is properly added with a default AuthConfig instance.
1 participant
#27
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.