added e2e tests for authorized endpoint #485
Conversation
|
Warning Rate limit exceeded@radofuchs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughAdds a noop-with-token Lightspeed config, replaces the legacy authorized.feature with two noop-focused feature files, updates Behave environment hooks to swap configs for Authorized-tagged features, revises auth-related step definitions and HTTP POST handling, updates the test list, and adds a utility to swap configs and restart the service container. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Behave as "Behave Runner"
participant Env as "environment.py"
participant Utils as "utils.switch_config_and_restart"
participant Svc as "LCS Container"
participant Steps as "Step defs"
rect rgba(200,230,255,0.25)
note over Behave,Env: Before Authorized-tagged feature
Behave->>Env: before_feature(Feature[tag=Authorized])
Env->>Utils: switch_config_and_restart(orig_cfg, noop_token_cfg, "lightspeed-core")
Utils->>Svc: docker restart lightspeed-core
Svc-->>Utils: restarted
Utils-->>Env: backup_path
end
rect rgba(200,255,200,0.25)
note over Tester,Steps: Execute scenarios
Tester->>Steps: Given Authorization header / When POST /v1/authorized (±user_id)
Steps->>Svc: HTTP POST /v1/authorized
Svc-->>Steps: 200/400 + JSON body
Steps-->>Tester: Assert status & body
end
rect rgba(255,230,200,0.25)
note over Behave,Env: After feature cleanup
Behave->>Env: after_feature(Feature[tag=Authorized])
Env->>Utils: switch_config_and_restart(orig_cfg, backup, "lightspeed-core", cleanup=true)
Utils->>Svc: docker restart lightspeed-core
Svc-->>Utils: restarted
Utils-->>Env: cleanup_done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
tests/e2e/features/environment.py (2)
89-95: Hard-coded paths/container; parameterize for CI.lightspeed-stack.yaml and container name are baked in. Read from env to support local vs CI.
Example:
- context.backup_file = switch_config_and_restart( - "lightspeed-stack.yaml", - "tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml", - "lightspeed-stack", - ) + import os + base_cfg = os.getenv("LS_STACK_CONFIG", "lightspeed-stack.yaml") + token_cfg = os.getenv( + "LS_STACK_AUTH_TOKEN_CONFIG", + "tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml", + ) + container = os.getenv("LS_STACK_CONTAINER", "lightspeed-stack") + context.backup_file = switch_config_and_restart(base_cfg, token_cfg, container)…and mirror in after_feature.
Also applies to: 99-105
99-105: Guard against missing backup.If before_feature failed, after_feature will error. Add hasattr/context check.
- if "Authorized" in feature.tags: + if "Authorized" in feature.tags and hasattr(context, "backup_file"):tests/e2e/utils/utils.py (2)
41-51: Clarify cleanup semantics in docstring.
“after restoration” is misleading since this function doesn’t restore; callers pass cleanup=True during the restoration call. Tighten wording.- cleanup: If True, remove the backup file after restoration (default: False) + cleanup: If True, delete the existing backup file after a successful copy/restart. + Intended for use when restoring the original config. (default: False)
79-81: Replace fixed sleep with readiness check.
A fixed 5s delay is brittle; poll container health/state or an HTTP health endpoint.Example helper (outside this file’s scope, but recommended):
def wait_container_healthy(name: str, timeout_s: int = 60): start = time.time() while time.time() - start < timeout_s: r = subprocess.run( ["docker", "inspect", "-f", "{{.State.Health.Status}}", name], capture_output=True, text=True ) if r.returncode == 0 and r.stdout.strip() in ("healthy", "starting"): if r.stdout.strip() == "healthy": return time.sleep(1) raise TimeoutError(f"{name} not healthy within {timeout_s}s")Call it after restart and remove the static sleep.
tests/e2e/features/authorized_noop_token.feature (4)
4-9: Prefix step appears unused by the new POST steps.
Your new step implementations build URLs without api_prefix. Either remove the prefix line to avoid confusion or ensure all steps consistently honor it.- And REST API service prefix is /v1Please confirm that steps in steps/common_http.py also ignore api_prefix; otherwise endpoints may diverge across features.
24-24: Use an explicit dummy token; avoid logging/secret-scan noise.
The current token is long and looks real. A shorter placeholder improves clarity.- And I set the Authorization header to Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6Ikpva + And I set the Authorization header to Bearer dummy.token.valueApply to all similar steps.
Also applies to: 34-34, 44-44
52-52: Fix indentation of Scenario keyword.
Minor formatting nit; one extra leading space.- Scenario: Check if the authorized endpoint works with proper user_id but bearer token is not present + Scenario: Check if the authorized endpoint works with proper user_id but bearer token is not present
61-69: Add a case for “Bearer ” with no token.
You already cover “Bearerey...” (no space). Consider also “Authorization: Bearer ” (space, empty token) if the service distinguishes the two.tests/e2e/features/steps/auth.py (4)
9-15: Avoid printing full Authorization tokens.
Even in tests, emitting tokens to logs is noisy and can trigger scanners. Redact most of the value.- print(f"🔑 Set Authorization header to: {header_value}") + redacted = (header_value[:12] + "…") if len(header_value) > 12 else header_value + print(f"🔑 Set Authorization header to: {redacted}")
18-35: Validate inputs and environment before switching configs.
Add checks that the files exist and optionally assert the expected container is running; failing early saves CI time.+ # Validate paths before attempting switch + for p in ("lightspeed-stack.yaml", "tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml"): + assert os.path.exists(p), f"Config file not found: {p}"Note: you’ll need
import osat the top.+import osPlease confirm the container name is actually “lightspeed-stack” in CI; if it differs, consider pulling it from config/env instead of hardcoding.
37-44: Docstring mismatch.
The function claims “payload in the endpoint as parameter,” but the call sends an empty JSON body. Adjust wording or use context.text if provided.
3-7: Centralize URL building to avoid drift across step modules.
Consider a small helper (e.g., build_url(context, endpoint)) that handles prefix + normalization once.I can draft the helper and update both auth/common_http steps if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml(1 hunks)tests/e2e/features/authorized.feature(0 hunks)tests/e2e/features/authorized_noop.feature(1 hunks)tests/e2e/features/authorized_noop_token.feature(1 hunks)tests/e2e/features/environment.py(2 hunks)tests/e2e/features/steps/auth.py(1 hunks)tests/e2e/features/steps/common_http.py(1 hunks)tests/e2e/test_list.txt(1 hunks)tests/e2e/utils/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/features/authorized.feature
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/features/environment.py (1)
tests/e2e/utils/utils.py (1)
switch_config_and_restart(35-89)
tests/e2e/features/steps/auth.py (2)
tests/e2e/utils/utils.py (2)
normalize_endpoint(12-17)switch_config_and_restart(35-89)tests/e2e/features/steps/feedback.py (1)
access_rest_api_endpoint_post(14-31)
⏰ 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
🔇 Additional comments (8)
tests/e2e/test_list.txt (1)
2-3: Replacement looks correct.List correctly splits the old authorized.feature into two focused features.
tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml (2)
5-5: auth_enabled=false with module=noop-with-token—confirm intent.If auth is disabled globally, the module may be ignored; token-related scenarios could pass without actually exercising token logic.
Consider:
- auth_enabled: false + auth_enabled: true…if the service expects enabled auth to invoke the noop-with-token module.
Also applies to: 24-25
3-7: Config aligns with test background (localhost:8080).Host/port match the features’ Background. Good.
tests/e2e/features/authorized_noop.feature (1)
32-32: Bearer token strings are truncated.If any JWT parsing occurs, use a well-formed 3-part token (header.payload.signature).
I can generate a dummy HS256 token if needed.
Also applies to: 42-42, 52-52
tests/e2e/utils/utils.py (1)
3-6: LGTM on new imports.
Used by the helper; no redundancies.tests/e2e/features/authorized_noop_token.feature (2)
22-30: Verify expected default user_id; likely truncated UUID.
The body expects "00000000-0000-0000-0000-000" which is not a full UUID. If the service returns a zero UUID, use 00000000-0000-0000-0000-000000000000.- {"user_id": "00000000-0000-0000-0000-000","username": "lightspeed-user"} + {"user_id": "00000000-0000-0000-0000-000000000000","username": "lightspeed-user"}
10-16: No action required: POST-with-body step maps to common_http.py
The step text “I access endpoint … using HTTP POST method” only matches the definition in common_http.py (which loadscontext.textas JSON), not the auth.py variants (which require “with user_id” or “without user_id”). Payload handling is therefore correct.tests/e2e/features/steps/auth.py (1)
18-31: Remove unusedsetup_authorized_test_environmentstep. No feature invokes the “I set up authorized test environment” step and config switching is already handled inenvironment.py, so deletesetup_authorized_test_environmentfromtests/e2e/features/steps/auth.py.
| # Restart container | ||
| try: | ||
| subprocess.run( | ||
| ["docker", "restart", container_name], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| except subprocess.CalledProcessError as e: | ||
| print(f"Failed to restart container {container_name}: {e.stderr}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Docker CLI presence check and a restart timeout.
Preemptive fail if docker isn’t available; also avoid indefinite hangs on restart.
- # Restart container
- try:
- subprocess.run(
- ["docker", "restart", container_name],
- capture_output=True,
- text=True,
- check=True,
- )
- except subprocess.CalledProcessError as e:
- print(f"Failed to restart container {container_name}: {e.stderr}")
- raise
+ # Restart container
+ if shutil.which("docker") is None:
+ raise RuntimeError("Docker CLI not found in PATH")
+ try:
+ subprocess.run(
+ ["docker", "restart", container_name],
+ capture_output=True,
+ text=True,
+ check=True,
+ timeout=60,
+ )
+ except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
+ err = getattr(e, "stderr", str(e))
+ print(f"Failed to restart container {container_name}: {err}")
+ raiseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/utils/utils.py around lines 67 to 77, add a pre-check that the
Docker CLI is available (e.g., use shutil.which("docker") and raise a clear
error if not found) and add a timeout to the subprocess.run call that restarts
the container to prevent hanging; also handle subprocess.TimeoutExpired
separately with a clear error message and keep the existing CalledProcessError
handling (including printing/propagating stderr). Ensure the subprocess.run uses
the timeout parameter and that both absence of docker and restart timeout raise
informative exceptions.
radofuchs
force-pushed
the
LCORE_493_auth_e2e_tests
branch
from
ae207c4 to
27d99e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/e2e/features/steps/auth.py (2)
37-58: Use requests params for user_id and send a valid JSON body.Manual query concat can mis-encode; json="" isn’t valid JSON. Prefer params and json={}.
- path = f"{endpoint}?user_id={user_id}".replace("//", "/") + path = f"{endpoint}".replace("//", "/") @@ - context.response = requests.post( - url, json="", headers=context.auth_headers, timeout=10 - ) + context.response = requests.post( + url, params={"user_id": user_id}, json={}, headers=context.auth_headers, timeout=10 + )
60-79: Mirror the same request conventions for the no-user_id variant.Send json={} for consistency and valid content type.
- context.response = requests.post( - url, json="", headers=context.auth_headers, timeout=10 - ) + context.response = requests.post( + url, json={}, headers=context.auth_headers, timeout=10 + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml(1 hunks)tests/e2e/features/authorized.feature(0 hunks)tests/e2e/features/authorized_noop.feature(1 hunks)tests/e2e/features/authorized_noop_token.feature(1 hunks)tests/e2e/features/environment.py(2 hunks)tests/e2e/features/steps/auth.py(1 hunks)tests/e2e/features/steps/common_http.py(1 hunks)tests/e2e/test_list.txt(1 hunks)tests/e2e/utils/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/features/authorized.feature
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/features/authorized_noop.feature
- tests/e2e/configuration/lightspeed-stack-auth-noop-token.yaml
- tests/e2e/features/environment.py
- tests/e2e/utils/utils.py
- tests/e2e/features/steps/common_http.py
- tests/e2e/features/authorized_noop_token.feature
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.365Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.364Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
📚 Learning: 2025-09-02T11:15:02.365Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.365Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Applied to files:
tests/e2e/test_list.txttests/e2e/features/steps/auth.py
📚 Learning: 2025-09-02T11:09:40.364Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.364Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/e2e/test_list.txttests/e2e/features/steps/auth.py
📚 Learning: 2025-09-02T11:14:17.077Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/steps/common_http.py:244-255
Timestamp: 2025-09-02T11:14:17.077Z
Learning: The POST step in tests/e2e/features/steps/common_http.py (`access_rest_api_endpoint_post`) is intentionally designed as a general-purpose HTTP POST method, not specifically for REST API endpoints, so it should not include context.api_prefix in the URL construction.
Applied to files:
tests/e2e/features/steps/auth.py
📚 Learning: 2025-09-02T11:09:23.077Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/authorized_noop.feature:11-11
Timestamp: 2025-09-02T11:09:23.077Z
Learning: The authorized endpoint in the lightspeed-stack project does not use the /v1 API prefix and should be accessed directly as "authorized" rather than "/v1/authorized".
Applied to files:
tests/e2e/features/steps/auth.py
🧬 Code graph analysis (1)
tests/e2e/features/steps/auth.py (1)
tests/e2e/utils/utils.py (2)
normalize_endpoint(12-17)switch_config_and_restart(35-89)
⏰ 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
🔇 Additional comments (1)
tests/e2e/test_list.txt (1)
2-3: Tag strategy verified:authorized_noop.featurecontains no@Authorized;authorized_noop_token.featureincludes@Authorized.
| @given("I set the Authorization header to {header_value}") | ||
| def set_authorization_header_custom(context: Context, header_value: str) -> None: | ||
| """Set a custom Authorization header value.""" | ||
| if not hasattr(context, "auth_headers"): | ||
| context.auth_headers = {} | ||
| context.auth_headers["Authorization"] = header_value | ||
| print(f"🔑 Set Authorization header to: {header_value}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t log secrets; optionally strip quotes from header value.
Avoid printing Authorization values to CI logs; also consider stripping wrapping quotes.
Apply:
- context.auth_headers["Authorization"] = header_value
- print(f"🔑 Set Authorization header to: {header_value}")
+ header_value = header_value.strip('"')
+ context.auth_headers["Authorization"] = header_value
+ print("🔑 Authorization header set.")📝 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.
| @given("I set the Authorization header to {header_value}") | |
| def set_authorization_header_custom(context: Context, header_value: str) -> None: | |
| """Set a custom Authorization header value.""" | |
| if not hasattr(context, "auth_headers"): | |
| context.auth_headers = {} | |
| context.auth_headers["Authorization"] = header_value | |
| print(f"🔑 Set Authorization header to: {header_value}") | |
| @given("I set the Authorization header to {header_value}") | |
| def set_authorization_header_custom(context: Context, header_value: str) -> None: | |
| """Set a custom Authorization header value.""" | |
| if not hasattr(context, "auth_headers"): | |
| context.auth_headers = {} | |
| header_value = header_value.strip('"') | |
| context.auth_headers["Authorization"] = header_value | |
| print("🔑 Authorization header set.") |
🤖 Prompt for AI Agents
In tests/e2e/features/steps/auth.py around lines 9 to 15, remove the print that
logs the Authorization header (do not output secrets to CI logs) and instead log
a non-sensitive confirmation message or nothing; additionally, sanitize the
incoming header_value by stripping surrounding single or double quotes before
storing it in context.auth_headers["Authorization"] so quoted inputs become
unquoted while still avoiding printing the secret.
90f8ed3 to
81244e1
Compare
2 participants
…auth modules
Description
added e2e tests for authorized endpoint for noop and noop-with-token auth modules
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests
Chores