LCORE-598: Add authorization e2e tests #636
Conversation
WalkthroughAdds JWK-based end-to-end authentication tests and supporting infra: new test JWK server service, E2E configuration/feature/step and environment wiring to generate and mount JWKS, parameterizes container CLI via CONTAINER_CMD, updates unit test JWKS/JWT helpers, docs, and ignore rules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as E2E Tests
participant J as Test JWK Server (http://localhost:16161/jwk.json)
participant L as Lightspeed Core Service (LCS)
participant U as Upstream Llama Stack
rect rgba(200,230,255,0.25)
note over T: Before feature (JWKAuth)
T->>J: Write generated jwk.json (mounted volume)
T->>L: Swap to lightspeed-stack-auth-jwk.yaml and restart container
L->>J: Fetch JWKS from /jwk.json
J-->>L: JWKS keys
end
rect rgba(220,255,220,0.25)
note over T,L: Scenario request
T->>T: Create RS256 JWT with roles
T->>L: HTTP request with Authorization: Bearer <jwt>
L->>L: Validate JWT via JWKS and evaluate role_rules
alt Authorized
L->>U: Optional upstream call
U-->>L: Response
L-->>T: 200 OK
else Forbidden
L-->>T: 403 Insufficient permissions
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/features/environment.py (1)
25-28: Remove unused try-except block.Lines 25-28 contain a try-except block that attempts to import
os, which is already imported at line 10. This appears to be legacy code and serves no purpose.Apply this diff to remove the unused code:
-try: - import os # noqa: F401 -except ImportError as e: - print("Warning: unable to import module:", e) -tests/unit/authentication/test_jwk_token.py (1)
31-38: Add type annotations.The function lacks a return type annotation.
As per coding guidelines, all functions require complete type annotations. Apply this diff:
-def create_token_payload(): +def create_token_payload() -> dict[str, str | int]: """Create a sample token payload with the default user_id and username claims.""" return { "user_id": TEST_USER_ID, "username": TEST_USER_NAME, "exp": int(time.time()) + 3600, "iat": int(time.time()), }Based on coding guidelines.
🧹 Nitpick comments (3)
docker-compose.yaml (1)
45-61: Consider adding a healthcheck for the test-jwk-server.While the simple http.server is likely to start quickly, adding a healthcheck would ensure consistency with other services and allow dependent services to wait for JWK server readiness if needed.
Example healthcheck:
healthcheck: test: ["CMD", "curl", "-f", "http://localhost:16161/"] interval: 5s timeout: 3s retries: 2 start_period: 5stests/e2e/features/steps/jwk_auth.py (1)
12-18: Consider restructuring imports to avoid sys.path manipulation.The
sys.path.appendworkaround and the comment about "module load issues" suggest a structural concern. While this works for tests, it would be cleaner to ensure the test package structure allows direct imports.Consider one of these approaches:
- Install the package in editable mode (
pip install -e .) so all modules are importable- Use relative imports if the test structure permits
- Add an
__init__.pyto make tests a proper packageThat said, this pattern is acceptable for E2E tests if restructuring is not feasible at this time.
tests/e2e/features/environment.py (1)
115-126: Refactor to use context.feature_config consistently.Line 125 hardcodes the config path that's already stored in
context.feature_configon line 117. Additionally, the file write operation (lines 119-122) lacks error handling.Apply this diff to improve consistency and add error handling:
elif "JWKAuth" in feature.tags: context.feature_config = ( "tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml" ) - with open("tests/e2e/configuration/test_jwk/jwk.json", "w") as f: - context.test_key = make_key() - keys = create_jwks_keys([context.test_key], ["RS256"]) - f.write(json.dumps(keys)) + try: + with open("tests/e2e/configuration/test_jwk/jwk.json", "w") as f: + context.test_key = make_key() + keys = create_jwks_keys([context.test_key], ["RS256"]) + f.write(json.dumps(keys)) + except (IOError, OSError) as e: + print(f"Failed to write JWK file: {e}") + raise context.default_config_backup = create_config_backup("lightspeed-stack.yaml") - switch_config("tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml") + switch_config(context.feature_config) restart_container("lightspeed-stack")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)docker-compose.yaml(1 hunks)docs/testing.md(1 hunks)tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml(1 hunks)tests/e2e/configuration/test_jwk/.gitignore(1 hunks)tests/e2e/features/authorization_jwk.feature(1 hunks)tests/e2e/features/environment.py(5 hunks)tests/e2e/features/steps/health.py(3 hunks)tests/e2e/features/steps/jwk_auth.py(1 hunks)tests/e2e/test_list.txt(1 hunks)tests/e2e/utils/utils.py(2 hunks)tests/unit/authentication/test_jwk_token.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/features/steps/jwk_auth.pytests/e2e/utils/utils.pytests/e2e/features/steps/health.pytests/unit/authentication/test_jwk_token.pytests/e2e/features/environment.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/jwk_auth.pytests/e2e/features/steps/health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/features/steps/jwk_auth.pytests/e2e/utils/utils.pytests/e2e/features/steps/health.pytests/unit/authentication/test_jwk_token.pytests/e2e/features/environment.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/authentication/test_jwk_token.py
tests/e2e/test_list.txt
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the E2E test list in tests/e2e/test_list.txt
Files:
tests/e2e/test_list.txt
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/authorization_jwk.feature
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/e2e/test_list.txt : Maintain the E2E test list in tests/e2e/test_list.txt
Applied to files:
tests/e2e/test_list.txt
🧬 Code graph analysis (2)
tests/e2e/features/steps/jwk_auth.py (1)
tests/unit/authentication/test_jwk_token.py (2)
create_token_header(20-22)create_token_payload(31-38)
tests/e2e/features/environment.py (2)
tests/unit/authentication/test_jwk_token.py (2)
create_jwks_keys(94-105)make_key(47-54)tests/e2e/utils/utils.py (3)
create_config_backup(108-117)switch_config(97-105)restart_container(129-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (14)
docs/testing.md (1)
144-177: LGTM! Well-documented E2E test prerequisites.The new section clearly explains how to run E2E tests with both docker-compose and podman-compose, including the CONTAINER_CMD environment variable override. The practical example of running a specific test with live output is particularly helpful.
tests/e2e/test_list.txt (1)
11-11: LGTM! Test list correctly updated.The new JWK authorization test has been appropriately added to the E2E test list.
tests/e2e/configuration/test_jwk/.gitignore (1)
1-2: LGTM! Appropriate ignore rule for generated test artifacts.The gitignore pattern correctly excludes generated JWK JSON files from version control, with a clear comment explaining the purpose.
.gitignore (1)
191-193: LGTM! Backup file correctly ignored.The ignore rule appropriately excludes the temporary backup file created during E2E tests. The comment clearly explains the purpose.
tests/e2e/utils/utils.py (2)
40-40: LGTM! Parameterized container command enables podman support.The change to use
os.getenv("CONTAINER_CMD", "docker")appropriately enables runtime selection of the container command while maintaining backward compatibility.
133-133: LGTM! Consistent container command parameterization.The same pattern applied to
restart_containermaintains consistency across the codebase.tests/e2e/features/steps/health.py (1)
3-3: LGTM! Consistent container command parameterization in test steps.The changes appropriately enable runtime selection of the container command for both inspection and stop operations, maintaining consistency with other E2E test utilities.
Also applies to: 18-24, 33-36
tests/e2e/features/steps/jwk_auth.py (2)
1-11: LGTM!The module docstring and imports are appropriate for a Behave step definition file.
21-41: No guard needed forcontext.test_key. Thebefore_scenariohook initializescontext.test_keyfor every@JWKAuthscenario (as seen in environment.py), and all uses ofcreate_role_tokenare within those scenarios.tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml (1)
1-52: LGTM!The JWK authentication configuration is well-structured:
- The JWK server URL points to the test server container
- Role mappings correctly transform external claims to internal roles
- Authorization rules properly map roles to permitted actions
This configuration aligns well with the test infrastructure in environment.py and the step definitions in jwk_auth.py.
tests/e2e/features/environment.py (2)
62-74: LGTM!The parameterization of the container command via the
CONTAINER_CMDenvironment variable is a good improvement that enables support for alternative container runtimes like Podman.
134-137: LGTM!The cleanup logic correctly includes JWKAuth scenarios alongside Authorized scenarios, ensuring proper restoration of configuration and container state.
tests/unit/authentication/test_jwk_token.py (2)
26-44: LGTM!Good refactoring to extract token creation logic into reusable helpers. The fixtures now properly delegate to the helper functions, improving maintainability and enabling reuse in E2E tests.
108-115: LGTM!The refactoring to use
create_jwks_keysimproves code consistency and reduces duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/authentication/test_jwk_token.py (1)
31-38: Add type annotations.The function lacks type annotations for its return type, which violates the coding guidelines. This is similar to the issues already flagged for other helper functions in this file.
As per coding guidelines, apply this diff:
-def create_token_payload(): +def create_token_payload() -> dict[str, str | int]: """Create a sample token payload with the default user_id and username claims.""" return { "user_id": TEST_USER_ID, "username": TEST_USER_NAME, "exp": int(time.time()) + 3600, "iat": int(time.time()), }Based on coding guidelines.
🧹 Nitpick comments (9)
tests/e2e/features/environment.py (1)
115-127: Add error handling for file operations.The code writes to a JWK configuration file without checking if the directory exists or handling potential I/O errors. If the directory
tests/e2e/configuration/test_jwk/doesn't exist, the test will fail with an unclear error.Apply this diff to add directory creation and error handling:
elif "JWKAuth" in feature.tags: context.feature_config = ( "tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml" ) - with open("tests/e2e/configuration/test_jwk/jwk.json", "w") as f: + jwk_dir = "tests/e2e/configuration/test_jwk" + os.makedirs(jwk_dir, exist_ok=True) + jwk_path = os.path.join(jwk_dir, "jwk.json") + with open(jwk_path, "w") as f: context.test_key = make_key() keys = create_jwks_keys([context.test_key], ["RS256"]) f.write(json.dumps(keys))tests/unit/authentication/test_jwk_token.py (1)
41-44: Consider removing redundant fixture wrapper.The
token_payloadfixture simply wraps a call tocreate_token_payload()without adding any value. Sincecreate_token_payload()is already a standalone function, tests that need a fresh payload can call it directly, reducing indirection.If you prefer to keep the fixture for consistency with the existing test structure, you can ignore this suggestion. Otherwise, consider removing the fixture and updating call sites to use
create_token_payload()directly.docker-compose.yaml (7)
58-58: Bind explicitly to 0.0.0.0 for predictability.Make the bind address explicit to avoid surprises across Python versions/runtimes.
- entrypoint: ["python3", "-m", "http.server", "16161"] + entrypoint: ["python3", "-m", "http.server", "16161", "--bind", "0.0.0.0"]
60-60: Mount JWK directory read-only.The server only needs to read JWKS. Tighten perms with ro,Z.
- - ./tests/e2e/configuration/test_jwk:/app-root/test_jwk:Z + - ./tests/e2e/configuration/test_jwk:/app-root/test_jwk:ro,Z
53-59: Add a healthcheck for the JWK server.Improves readiness gating and allows adding depends_on conditions if needed.
ports: - "16161:16161" + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:16161/"] + interval: 5s + timeout: 3s + retries: 5 + start_period: 2s
53-56: Do you need to publish port 16161 to the host?If lightspeed-stack resolves JWKS via service DNS (http://test-jwk-server:16161/...), you can avoid host port conflicts by dropping ports and using expose.
- ports: - - "16161:16161" + expose: + - "16161"If lightspeed needs localhost access, keep the published port.
52-52: container_name may collide in parallel CI runs.Consider removing container_name and rely on Compose’s project scoping to avoid name clashes.
49-52: Optional: use a lighter image for the static JWKS server.Reusing the main Containerfile is convenient but heavy. A tiny python:alpine stage (or busybox httpd) would speed build/pull for tests.
58-58: Heads-up: JSON Content-Type.python http.server may serve .json with a generic type on some environments. If any client enforces application/json, swap to a tiny handler that sets the header. Only act if tests complain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)docker-compose.yaml(1 hunks)docs/testing.md(1 hunks)tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml(1 hunks)tests/e2e/configuration/test_jwk/.gitignore(1 hunks)tests/e2e/features/authorization_jwk.feature(1 hunks)tests/e2e/features/environment.py(5 hunks)tests/e2e/features/steps/health.py(3 hunks)tests/e2e/features/steps/jwk_auth.py(1 hunks)tests/e2e/test_list.txt(1 hunks)tests/e2e/utils/utils.py(2 hunks)tests/unit/authentication/test_jwk_token.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/test_list.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- .gitignore
- tests/e2e/configuration/test_jwk/.gitignore
- tests/e2e/utils/utils.py
- tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml
- tests/e2e/features/authorization_jwk.feature
- docs/testing.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/features/environment.pytests/e2e/features/steps/jwk_auth.pytests/unit/authentication/test_jwk_token.pytests/e2e/features/steps/health.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/features/environment.pytests/e2e/features/steps/jwk_auth.pytests/unit/authentication/test_jwk_token.pytests/e2e/features/steps/health.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/jwk_auth.pytests/e2e/features/steps/health.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/authentication/test_jwk_token.py
🧬 Code graph analysis (2)
tests/e2e/features/environment.py (2)
tests/unit/authentication/test_jwk_token.py (2)
create_jwks_keys(94-105)make_key(47-54)tests/e2e/utils/utils.py (3)
create_config_backup(108-117)switch_config(97-105)restart_container(129-143)
tests/e2e/features/steps/jwk_auth.py (1)
tests/unit/authentication/test_jwk_token.py (2)
create_token_header(20-22)create_token_payload(31-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e_tests (ci)
- GitHub Check: build-pr
🔇 Additional comments (3)
tests/e2e/features/steps/health.py (1)
3-3: LGTM! Container command parameterization is well-implemented.The changes correctly introduce the
CONTAINER_CMDenvironment variable to support both Docker and Podman, maintaining backward compatibility with a sensible default of "docker". The implementation is consistent across both the inspect and stop operations.Also applies to: 18-24, 33-36
tests/e2e/features/environment.py (1)
10-10: LGTM! Consistent use ofCONTAINER_CMDand proper test imports.The changes correctly introduce the
CONTAINER_CMDenvironment variable for container operations and import the necessary JWK test utilities. This maintains consistency with the broader parameterization effort across the e2e test suite.Also applies to: 14-14, 30-30, 62-62, 74-74
tests/e2e/features/steps/jwk_auth.py (1)
21-41: Add complete type annotations.The function lacks type annotations for its parameters and return type, violating the coding guidelines.
As per coding guidelines, apply this diff:
@given("I have a valid JWT token with the {role} role") -def create_role_token(context: Context, role: str) -> None: +def create_role_token(context: Context, role: str) -> None: """Create token with role using the shared test key."""Wait, I see the function already has type annotations (
context: Context, role: str) -> None). Let me re-check... Yes, it does have them. This comment would be incorrect. Let me skip this.Actually, looking more carefully, the function signature at line 22 already has proper type annotations:
def create_role_token(context: Context, role: str) -> None:. So this is already compliant with coding guidelines.Let me check if there are other issues with this function.
- The docstring is brief but adequate
- The logic is sound
- Context mutation is expected in behave steps
No issues to flag here beyond the sys.path.append issue already covered.
Likely an incorrect or invalid review comment.
|
|
||
| from authlib.jose import JsonWebToken | ||
|
|
||
| sys.path.append(str(Path(__file__).resolve().parents[4])) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid sys.path.append for imports.
Manipulating sys.path at runtime is fragile and pollutes the global import state. Instead, structure your imports using relative imports or ensure the package is properly installed.
Apply this diff to use a relative import:
-sys.path.append(str(Path(__file__).resolve().parents[4]))
-
-# Import at runtime to avoid module load issues
from tests.unit.authentication.test_jwk_token import (
create_token_header,
create_token_payload,
)If this approach causes import errors, verify that the test runner is invoked from the repository root or that the package structure includes proper __init__.py files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sys.path.append(str(Path(__file__).resolve().parents[4])) | |
| from tests.unit.authentication.test_jwk_token import ( | |
| create_token_header, | |
| create_token_payload, | |
| ) |
🤖 Prompt for AI Agents
In tests/e2e/features/steps/jwk_auth.py around line 12, remove the runtime
sys.path.append modification and replace it with proper package-relative imports
(or absolute imports assuming the package is installed); update the import
statements in this file to use relative form (e.g., from ...module import X) or
full package paths, ensure any needed __init__.py files exist in the package
directories, and confirm the test runner is executed from the repository root
(or install the package in editable mode) so imports resolve without modifying
sys.path.
This commit adds end-to-end tests for the authorization functionality. # Implementation details - Modified `.gitignore` to ignore leftover config backup files (unrelated to this change but useful) - Updated testing documentation to include instructions for running e2e tests - Added a JWK server container to `docker-compose.yaml` which is needed for e2e testing JWK auth - Added a `tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml` config file for lightspeed-stack with JWK auth enabled to be used in the JWK e2e tests - Removed hard dependency on the `docker` command in e2e tests and instead use the `CONTAINER_CMD` environment variable if set (to allow using `podman` instead of `docker`) - Added `tests/e2e/features/authorization_jwk.feature` which contains the actual e2e tests for JWK authz - Added `tests/e2e/features/steps/jwk_auth.py` which implements the steps for the JWK authz tests - Modified `tests/e2e/features/environment.py` to handle the new JWK authz tests, including creating a temporary JWK key pair for the tests and writing the public key to a file served by the JWK server container (which lightspeed-stack is directed to access through the config file mentioned above) functionality to functions that can be reused in the e2e tests - Added `tests/e2e/configuration/test_jwk/.gitignore` to ignore generated JWK files - Updated `tests/e2e/test_list.txt` to include the new JWK authz tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/unit/authentication/test_jwk_token.py (1)
23-25: Add missing parameter type annotation.The
create_token_headerfunction lacks a type annotation for thekey_idparameter. This was flagged in a previous review but remains unaddressed.Based on coding guidelines, apply this diff:
-def create_token_header(key_id) -> dict[str, str]: +def create_token_header(key_id: str) -> dict[str, str]: """Create a sample token header.""" return {"alg": "RS256", "typ": "JWT", "kid": key_id}Based on coding guidelines.
🧹 Nitpick comments (1)
tests/e2e/features/steps/health.py (1)
18-36: Consider extracting the container command to reduce duplication.The
os.getenv("CONTAINER_CMD", "docker")call is duplicated across lines 19 and 33. While this is acceptable for test code, you could extract it to a module-level constant or helper function for better maintainability.For example:
CONTAINER_CMD = os.getenv("CONTAINER_CMD", "docker")Then use
CONTAINER_CMDdirectly in the subprocess calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)docker-compose.yaml(1 hunks)docs/testing.md(1 hunks)tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml(1 hunks)tests/e2e/configuration/test_jwk/.gitignore(1 hunks)tests/e2e/features/authorization_jwk.feature(1 hunks)tests/e2e/features/environment.py(5 hunks)tests/e2e/features/steps/health.py(3 hunks)tests/e2e/features/steps/jwk_auth.py(1 hunks)tests/e2e/test_list.txt(1 hunks)tests/e2e/utils/utils.py(2 hunks)tests/unit/authentication/test_jwk_token.py(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/e2e/configuration/test_jwk/.gitignore
- .gitignore
- docker-compose.yaml
- tests/e2e/test_list.txt
- tests/e2e/utils/utils.py
- docs/testing.md
- tests/e2e/features/steps/jwk_auth.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/e2e/features/environment.pytests/e2e/features/steps/health.pytests/unit/authentication/test_jwk_token.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/e2e/features/environment.pytests/e2e/features/steps/health.pytests/unit/authentication/test_jwk_token.py
tests/e2e/features/steps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place behave step definitions under tests/e2e/features/steps/
Files:
tests/e2e/features/steps/health.py
tests/e2e/features/**/*.feature
📄 CodeRabbit inference engine (CLAUDE.md)
Write E2E tests as Gherkin feature files for behave
Files:
tests/e2e/features/authorization_jwk.feature
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/authentication/test_jwk_token.py
🧬 Code graph analysis (2)
tests/e2e/features/environment.py (2)
tests/unit/authentication/test_jwk_token.py (2)
create_jwks_keys(89-102)make_key(50-57)tests/e2e/utils/utils.py (3)
create_config_backup(108-117)switch_config(97-105)restart_container(129-143)
tests/unit/authentication/test_jwk_token.py (1)
src/models/config.py (1)
JwkConfiguration(396-400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (12)
tests/e2e/features/authorization_jwk.feature (1)
1-39: LGTM! Well-structured authorization test scenarios.The feature file provides comprehensive coverage of JWK-based authorization across all three roles (admin, config, readonly) with appropriate success and failure cases. The scenarios clearly test the access control rules defined in the configuration.
tests/e2e/features/steps/health.py (1)
18-24: Good parameterization for container command flexibility.The change to use
CONTAINER_CMDenvironment variable enables podman support. The implementation correctly defaults to "docker" when the variable is not set.tests/e2e/configuration/lightspeed-stack-auth-jwk.yaml (1)
1-52: LGTM! Configuration correctly implements role-based access control.The configuration properly maps JWT roles to internal roles and defines appropriate authorization rules that align with the test scenarios:
administratorrole →["admin"]actionsconfigrole →["get_config", "info"]actionsreadonlyrole →["info"]actionsThe JWK configuration points to the test JWK server on port 16161, which integrates correctly with the e2e test infrastructure.
tests/e2e/features/environment.py (4)
10-10: LGTM! Appropriate imports added for JWK support.The new imports enable JWK key generation and file operations required for the JWKAuth test flow.
Also applies to: 14-14, 30-30
62-62: LGTM! Container command parameterization implemented.Consistent use of
os.getenv("CONTAINER_CMD", "docker")throughout the file enables podman support while maintaining backward compatibility.Also applies to: 74-74
134-137: LGTM! Cleanup properly handles both auth types.The after_feature hook correctly restores the original configuration and removes the backup for both
AuthorizedandJWKAuthfeatures.
115-126: No directory creation needed The directorytests/e2e/configuration/test_jwkalready exists, so writingjwk.jsonwill succeed.tests/unit/authentication/test_jwk_token.py (5)
6-6: LGTM! Type support enhanced for test helpers.The addition of
Anyfrom typing and theGeneratedKeytype alias improves type clarity for the test helper functions.Also applies to: 20-20
34-41: LGTM! Properly typed payload generator.The
create_token_payloadfunction has complete type annotations and creates a standard JWT payload with appropriate claims.
50-57: LGTM! Key generation with clear return type.The
make_keyfunction properly returns aGeneratedKeytype, improving type clarity for downstream consumers.
89-102: LGTM! JWKS helper function with proper typing.The
create_jwks_keysfunction has appropriate type annotations and correctly transforms the key set into JWKS format for the mock server.
139-147: LGTM! Type annotations added throughout test helpers.All helper functions now have proper type annotations for parameters and return values, improving type safety and code clarity.
Also applies to: 150-158, 162-170, 174-182, 185-191
1 participant
Description
This commit adds end-to-end tests for the authorization functionality.
Implementation details
.gitignoreto ignore leftover config backup files(unrelated to this change but useful)
tests
docker-compose.yamlwhich is neededfor e2e testing JWK auth
tests/e2e/configuration/lightspeed-stack-auth-jwk.yamlconfig file for lightspeed-stack with JWK auth enabled to be used in
the JWK e2e tests
dockercommand in e2e tests andinstead use the
CONTAINER_CMDenvironment variable if set (to allowusing
podmaninstead ofdocker)tests/e2e/features/authorization_jwk.featurewhich containsthe actual e2e tests for JWK authz
tests/e2e/features/steps/jwk_auth.pywhich implements thesteps for the JWK authz tests
tests/e2e/features/environment.pyto handle the new JWKauthz tests, including creating a temporary JWK key pair for the
tests and writing the public key to a file served by the JWK server
container (which lightspeed-stack is directed to access through the
config file mentioned above)
functionality to functions that can be reused in the e2e tests
tests/e2e/configuration/test_jwk/.gitignoreto ignoregenerated JWK files
tests/e2e/test_list.txtto include the new JWK authz testsType of change
Related Tickets & Documents
LCORE-598
Checklist before requesting a review
Testing
Summary by CodeRabbit
Documentation
Tests
Chores