feat: add login authentication and rate limiting features#24
Open
feat: add login authentication and rate limiting features#24
Conversation
- Add _login.py module for login authentication - Add rate_limiter.py module for request rate limiting - Update auth.py with authentication improvements - Update yaml_loader.py and config.py for better configuration handling - Add test_login_auth.py for login authentication tests - Update existing tests and documentation - Update dependencies in uv.lock
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JWT and Basic authentication classes and a BaseAuthentication error-response helper; moves reserved JWT claim names to config; tightens YAML callable inspection; integrates a process-local RateLimiter into login flow; updates tests to use a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Req as Request Handler
participant JWTAuth as JWTAuthentication
participant JWTLib as JWT Library
participant State as request.state
Client->>Req: HTTP request with Authorization: Bearer <token>
Req->>JWTAuth: authenticate(request)
JWTAuth->>JWTAuth: skip OPTIONS, extract Bearer token
JWTAuth->>JWTLib: decode(token, secret, algorithm)
JWTLib-->>JWTAuth: payload or raise error
alt valid payload
JWTAuth->>State: set request.state.user = payload
JWTAuth-->>Req: return True
else invalid
JWTAuth-->>Req: get_auth_error_response -> 401
end
Req-->>Client: proceed or 401 response
sequenceDiagram
participant Client
participant Req as Request Handler
participant BasicAuth as BasicAuthentication
participant Validator as LoginValidator/Registry
participant State as request.state
Client->>Req: HTTP request with Authorization: Basic <base64>
Req->>BasicAuth: authenticate(request)
BasicAuth->>BasicAuth: skip OPTIONS, parse & base64-decode credentials
BasicAuth->>Validator: validate(username, password)
Validator-->>BasicAuth: user payload or None
alt valid payload
BasicAuth->>State: set request.state.user = payload
BasicAuth-->>Req: return True
else invalid
BasicAuth-->>Req: get_auth_error_response -> 401 with WWW-Authenticate: Basic
end
Req-->>Client: proceed or 401 response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Resolve conflicts in: - lightapi/_login.py: Fix rate limiter response call signature - lightapi/auth.py: Rewrite to combine both versions with proper JWT and Basic auth - lightapi/rate_limiter.py: Fix indentation and merge algorithm changes - lightapi/config.py: Keep RESERVED_CLAIMS as instance attribute - lightapi/yaml_loader.py: Keep improved signature inspection with better error handling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightapi/auth.py`:
- Around line 33-43: The change incorrectly modifies frozen class bodies in
BaseAuthentication and JWTAuthentication (including get_auth_error_response);
revert any edits to those classes and instead implement the new auth flow in a
new helper/backend module (e.g., helpers/new_auth or helpers/new_backends) that
provides the new functions/classes and an adapter to call from existing entry
points; move all new logic (error response creation, token parsing, validation
flow) out of lightapi/auth.py into the new module and update callers to use the
adapter or helper functions without changing BaseAuthentication or
JWTAuthentication bodies.
In `@lightapi/config.py`:
- Around line 59-60: Change the mutable set RESERVED_CLAIMS to an immutable
class-level frozenset with an explicit ClassVar[frozenset[str]] annotation so it
cannot be mutated at runtime; update the declaration of RESERVED_CLAIMS (the
class attribute used during JWT validation) to use ClassVar[frozenset[str]] and
construct it with frozenset({"exp", "iat", "nbf", "iss", "sub", "aud", "jti"}),
adding any needed typing import (ClassVar) so linters (RUF012) pass and
Authentication-related validation cannot be altered by accidental mutation.
In `@lightapi/rate_limiter.py`:
- Around line 144-146: The function get_rate_limit_response is currently defined
at module scope but must be an instance method on the RateLimiter class because
callers call rate_limiter.get_rate_limit_response(...); move the definition into
the RateLimiter class (e.g., under class RateLimiter:), adjust its signature to
accept self (if not already), and ensure any references to instance attributes
use self; update any imports/usages if necessary so RateLimiter instances expose
get_rate_limit_response and runtime AttributeError is avoided.
In `@lightapi/yaml_loader.py`:
- Around line 115-124: The except block currently lets uninspectable validators
(including lambdas) pass by returning fn; instead reject them during loading by
raising the loader's ConfigurationError instead of returning fn (and avoid
leaking a raw ValueError). Change the lambda branch so it does not return fn but
raises ConfigurationError with a clear message referencing the invalid validator
(fn) and the original exception (exc), so load_config() fails closed; keep
references to fn, load_config(), and ConfigurationError in the message to make
debugging straightforward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94f5a868-71fe-4dc1-9feb-263c737ba782
📒 Files selected for processing (4)
lightapi/auth.pylightapi/config.pylightapi/rate_limiter.pylightapi/yaml_loader.py
Comment on lines
+33
to
+43
| def get_auth_error_response(self, request: Request) -> JSONResponse: | ||
| """ | ||
| Get the response to return when authentication fails. | ||
|
|
||
| def get_auth_error_response(self, request: Request) -> JSONResponse: | ||
| """ | ||
| Get the response to return when authentication fails. | ||
|
|
||
| Args: | ||
| request: The HTTP request object. | ||
| Args: | ||
| request: The HTTP request object. | ||
|
|
||
| Returns: | ||
| Response object for authentication error. | ||
| """ | ||
| return JSONResponse( | ||
| {"error": "authentication failed"}, | ||
| status_code=401, | ||
| headers={"WWW-Authenticate": 'Basic realm="Restricted Area"'}, | ||
| ) | ||
| Returns: | ||
| Response object for authentication error. | ||
| """ | ||
| return JSONResponse({"error": "authentication failed"}, status_code=401) |
There was a problem hiding this comment.
Keep the frozen auth classes untouched.
This PR adds behavior directly to BaseAuthentication and rewrites JWTAuthentication, but both class bodies are explicitly frozen in this repo. Please move the new auth flow into helpers/new backends instead of editing these classes in place.
As per coding guidelines, "lightapi/auth.py: Do not modify lightapi/auth.py: JWTAuthentication and BaseAuthentication class bodies are FROZEN`."
Also applies to: 46-131
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 33-33: Unused method argument: request
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/auth.py` around lines 33 - 43, The change incorrectly modifies
frozen class bodies in BaseAuthentication and JWTAuthentication (including
get_auth_error_response); revert any edits to those classes and instead
implement the new auth flow in a new helper/backend module (e.g.,
helpers/new_auth or helpers/new_backends) that provides the new
functions/classes and an adapter to call from existing entry points; move all
new logic (error response creation, token parsing, validation flow) out of
lightapi/auth.py into the new module and update callers to use the adapter or
helper functions without changing BaseAuthentication or JWTAuthentication
bodies.
Comment on lines
+59
to
+60
| # Standard JWT reserved claims that cannot be used as extra claims | ||
| RESERVED_CLAIMS = {"exp", "iat", "nbf", "iss", "sub", "aud", "jti"} |
There was a problem hiding this comment.
Make RESERVED_CLAIMS immutable.
Ruff is correctly flagging this mutable class attribute (RUF012), and mutating it later would silently change JWT-claim validation for every Authentication instance. ClassVar[frozenset[str]] matches the current read-only usage.
Suggested fix
-from typing import Any
+from typing import Any, ClassVar
@@
- RESERVED_CLAIMS = {"exp", "iat", "nbf", "iss", "sub", "aud", "jti"}
+ RESERVED_CLAIMS: ClassVar[frozenset[str]] = frozenset(
+ {"exp", "iat", "nbf", "iss", "sub", "aud", "jti"}
+ )📝 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.
Suggested change
| # Standard JWT reserved claims that cannot be used as extra claims | |
| RESERVED_CLAIMS = {"exp", "iat", "nbf", "iss", "sub", "aud", "jti"} | |
| # Standard JWT reserved claims that cannot be used as extra claims | |
| RESERVED_CLAIMS: ClassVar[frozenset[str]] = frozenset( | |
| {"exp", "iat", "nbf", "iss", "sub", "aud", "jti"} | |
| ) |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 60-60: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/config.py` around lines 59 - 60, Change the mutable set
RESERVED_CLAIMS to an immutable class-level frozenset with an explicit
ClassVar[frozenset[str]] annotation so it cannot be mutated at runtime; update
the declaration of RESERVED_CLAIMS (the class attribute used during JWT
validation) to use ClassVar[frozenset[str]] and construct it with
frozenset({"exp", "iat", "nbf", "iss", "sub", "aud", "jti"}), adding any needed
typing import (ClassVar) so linters (RUF012) pass and Authentication-related
validation cannot be altered by accidental mutation.
Comment on lines
+115
to
+124
| except (ValueError, TypeError) as exc: | ||
| # Only allow specific cases where signature inspection legitimately fails | ||
| if hasattr(fn, "__name__") and fn.__name__ in ("<lambda>",): | ||
| # Lambdas can't be properly inspected in some Python versions | ||
| return fn | ||
| # For other cases, raise a clear error about the validation function | ||
| raise ValueError( | ||
| f"Login validation function {fn!r} cannot be inspected: {exc}. " | ||
| f"Ensure it's a regular Python function with inspectable signature." | ||
| ) from exc |
There was a problem hiding this comment.
Reject uninspectable login validators instead of letting them through.
lightapi/_registry.py:1-8 types this callable as (str, str) -> ..., and both lightapi/_login.py:8-45 and lightapi/auth.py:32-48 invoke it with exactly (username, password). Returning fn here skips that contract check, and the non-lambda path now leaks ValueError instead of the loader’s normal ConfigurationError. Keep this branch fail-closed so bad config is rejected during load_config().
Suggested fix
- except (ValueError, TypeError) as exc:
- # Only allow specific cases where signature inspection legitimately fails
- if hasattr(fn, "__name__") and fn.__name__ in ("<lambda>",):
- # Lambdas can't be properly inspected in some Python versions
- return fn
- # For other cases, raise a clear error about the validation function
- raise ValueError(
+ except (ValueError, TypeError) as exc:
+ raise ConfigurationError(
f"Login validation function {fn!r} cannot be inspected: {exc}. "
f"Ensure it's a regular Python function with inspectable signature."
) from exc🧰 Tools
🪛 Ruff (0.15.5)
[warning] 121-124: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/yaml_loader.py` around lines 115 - 124, The except block currently
lets uninspectable validators (including lambdas) pass by returning fn; instead
reject them during loading by raising the loader's ConfigurationError instead of
returning fn (and avoid leaking a raw ValueError). Change the lambda branch so
it does not return fn but raises ConfigurationError with a clear message
referencing the invalid validator (fn) and the original exception (exc), so
load_config() fails closed; keep references to fn, load_config(), and
ConfigurationError in the message to make debugging straightforward.
- Fix ruff linting: remove unused import from auth.py - Fix mypy type errors in rate_limiter.py: correct function indentation - Fix mypy type errors in _login.py: add proper type hints for JSON parsing - Add RateLimiter import and type annotation in _login.py - Create tests/__init__.py to fix mypy module resolution - Fix import sorting in _login.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightapi/auth.py (1)
134-196:⚠️ Potential issue | 🟡 Minor
get_auth_error_responsemethod is dead code—never invoked by the authentication middleware.The
BasicAuthenticationclass is a valid new addition (not frozen). However, theget_auth_error_responsemethod at lines 182-196 is never called. TheAuthenticationMiddlewareatlightapi/core.py:219-225returns a hardcodedJSONResponse({"error": "not allowed"}, status_code=403)instead of invokingauthenticator.get_auth_error_response(). This means the customWWW-Authenticate: Basic realm="Restricted Area"header is never sent to clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/auth.py` around lines 134 - 196, AuthenticationMiddleware currently returns a hardcoded JSONResponse on auth failure instead of using the authenticator's custom response, so BasicAuthentication.get_auth_error_response is never used; update AuthenticationMiddleware (the middleware that checks authenticator.authenticate) to call authenticator.get_auth_error_response(request) when authenticate(...) returns False (and ensure it handles None or missing method by falling back to the existing JSONResponse({"error":"not allowed"}, status_code=403)), so BasicAuthentication.get_auth_error_response can supply the WWW-Authenticate header and custom message.
🧹 Nitpick comments (2)
lightapi/_login.py (2)
68-77: Consider moving the fallback return to anelseblock.The
return {}at line 75 is inside thetryblock after all risky operations complete. For clarity, consider restructuring to use anelseclause, which signals that the fallback only executes when no exception occurred.♻️ Suggested refactor
async def _read_body(request: Request) -> dict[str, Any]: """Read JSON body; return {} on empty or invalid.""" try: body = await request.body() if body: result: dict[str, Any] = json.loads(body) return result - return {} except (json.JSONDecodeError, TypeError): return {} + else: + return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/_login.py` around lines 68 - 77, In _read_body, the fallback return {} currently sits inside the try after parsing; refactor the try/except to have the risky operations (await request.body() and json.loads) in the try, the json.loads success path return the parsed dict, and place the empty-body fallback return {} in an else block (i.e., execute when the body exists check fails) while keeping the except (json.JSONDecodeError, TypeError) returning {}; this clarifies control flow without changing behavior for the _read_body function.
97-100: Redundantwindow is not Nonecheck.Based on the
RateLimiter.is_rate_limited()implementation (seelightapi/rate_limiter.py:96-146), whenis_limited=True,windowis always a non-None string (the window name like "minute", "hour", "day"). Theand window is not Nonecondition is redundant.♻️ Suggested simplification
if rate_limiter is not None: is_limited, window = rate_limiter.is_rate_limited(request, endpoint="auth") - if is_limited and window is not None: + if is_limited: return rate_limiter.get_rate_limit_response(request, window)Note: If you prefer to keep it as defensive coding against future API changes, that's acceptable—just be aware it's currently unreachable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/_login.py` around lines 97 - 100, The check "and window is not None" is redundant because RateLimiter.is_rate_limited() guarantees a non-None window when is_limited is True; simplify the conditional in lightapi/_login.py by changing the block that calls rate_limiter.is_rate_limited(request, endpoint="auth") to only test "if is_limited:" and then return rate_limiter.get_rate_limit_response(request, window); reference symbols: rate_limiter.is_rate_limited, is_limited, window, and rate_limiter.get_rate_limit_response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lightapi/auth.py`:
- Around line 134-196: AuthenticationMiddleware currently returns a hardcoded
JSONResponse on auth failure instead of using the authenticator's custom
response, so BasicAuthentication.get_auth_error_response is never used; update
AuthenticationMiddleware (the middleware that checks authenticator.authenticate)
to call authenticator.get_auth_error_response(request) when authenticate(...)
returns False (and ensure it handles None or missing method by falling back to
the existing JSONResponse({"error":"not allowed"}, status_code=403)), so
BasicAuthentication.get_auth_error_response can supply the WWW-Authenticate
header and custom message.
---
Nitpick comments:
In `@lightapi/_login.py`:
- Around line 68-77: In _read_body, the fallback return {} currently sits inside
the try after parsing; refactor the try/except to have the risky operations
(await request.body() and json.loads) in the try, the json.loads success path
return the parsed dict, and place the empty-body fallback return {} in an else
block (i.e., execute when the body exists check fails) while keeping the except
(json.JSONDecodeError, TypeError) returning {}; this clarifies control flow
without changing behavior for the _read_body function.
- Around line 97-100: The check "and window is not None" is redundant because
RateLimiter.is_rate_limited() guarantees a non-None window when is_limited is
True; simplify the conditional in lightapi/_login.py by changing the block that
calls rate_limiter.is_rate_limited(request, endpoint="auth") to only test "if
is_limited:" and then return rate_limiter.get_rate_limit_response(request,
window); reference symbols: rate_limiter.is_rate_limited, is_limited, window,
and rate_limiter.get_rate_limit_response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea6d5a89-8318-43c2-90c4-43774a7554bf
📒 Files selected for processing (4)
lightapi/_login.pylightapi/auth.pylightapi/rate_limiter.pytests/__init__.py
✅ Files skipped from review due to trivial changes (1)
- lightapi/rate_limiter.py
- Fix rate limiting in tests by disabling auth_rate_limiter in fixtures - Fix JWT configuration errors: replace reserved claim 'sub' with 'user_id' in jwt_extra_claims - Update test assertions to check for 'user_id' instead of 'sub' - Fix YAML config test by adding login_validator parameter
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_yaml_config.py (1)
358-381:⚠️ Potential issue | 🔴 CriticalCritical: IndentationError breaks test execution.
Line 375 has incorrect indentation. The
app = _from_str(...)statement is indented to match the content inside the multiline string, but it should be dedented to align with thecontent = ...assignment at line 359. This causes the pipeline failure.🐛 Proposed fix
meta: methods: [GET] """ - app = _from_str(content, login_validator=_dummy_login_validator) + app = _from_str(content, login_validator=_dummy_login_validator) # The validator from YAML should be resolved and used assert app._login_validator is test_validator finally:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_config.py` around lines 358 - 381, The test has an IndentationError because the app = _from_str(...) line is over-indented inside the triple-quoted content block; move the app = _from_str(...) call to the same indentation level as the content = """...""" assignment so it is not part of the string literal, then keep the assertion assert app._login_validator is test_validator and the cleanup delattr(tests.test_yaml_config, "test_validator_func") at the same outer block level; this fixes the indentation issue around the _from_str, test_validator_func and _dummy_login_validator usage.
🧹 Nitpick comments (1)
tests/test_login_auth.py (1)
51-63: Consider using a public API or fixture parameter for disabling rate limiting.Directly mutating the private attribute
_auth_rate_limiterworks (the handler safely checks forNone), but accessing private internals couples tests to implementation details. Consider whether LightApi should expose a constructor parameter likeauth_rate_limiter=Noneor a test mode flag for cleaner test configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_login_auth.py` around lines 51 - 63, The test directly mutates the private attribute _auth_rate_limiter in the jwt_client fixture which couples tests to internals; update the LightApi constructor to accept a public parameter (e.g., auth_rate_limiter=None or disable_auth_rate_limiter=True) and default to the current behavior, then change the jwt_client fixture to pass that parameter (e.g., LightApi(engine=engine, login_validator=_valid_validator, auth_rate_limiter=None) or LightApi(..., disable_auth_rate_limiter=True)); ensure the LightApi class uses the new constructor argument instead of relying on a private attribute so tests can disable rate limiting via the public API rather than mutating _auth_rate_limiter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_yaml_config.py`:
- Around line 358-381: The test has an IndentationError because the app =
_from_str(...) line is over-indented inside the triple-quoted content block;
move the app = _from_str(...) call to the same indentation level as the content
= """...""" assignment so it is not part of the string literal, then keep the
assertion assert app._login_validator is test_validator and the cleanup
delattr(tests.test_yaml_config, "test_validator_func") at the same outer block
level; this fixes the indentation issue around the _from_str,
test_validator_func and _dummy_login_validator usage.
---
Nitpick comments:
In `@tests/test_login_auth.py`:
- Around line 51-63: The test directly mutates the private attribute
_auth_rate_limiter in the jwt_client fixture which couples tests to internals;
update the LightApi constructor to accept a public parameter (e.g.,
auth_rate_limiter=None or disable_auth_rate_limiter=True) and default to the
current behavior, then change the jwt_client fixture to pass that parameter
(e.g., LightApi(engine=engine, login_validator=_valid_validator,
auth_rate_limiter=None) or LightApi(..., disable_auth_rate_limiter=True));
ensure the LightApi class uses the new constructor argument instead of relying
on a private attribute so tests can disable rate limiting via the public API
rather than mutating _auth_rate_limiter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22966b9b-587c-414d-b083-68f47ad79113
📒 Files selected for processing (2)
tests/test_login_auth.pytests/test_yaml_config.py
- Fix indentation errors in test_yaml_config.py - Add login_validator parameter to YAML tests using authentication - Fix test_login_validator_dotted_path_from_yaml to not override YAML-specified validator - Update jwt_extra_claims from 'sub' to 'user_id' to avoid reserved claim error
- Update cache test mocks to patch 'lightapi.lightapi' instead of 'lightapi.cache' - Fixes issue where mocks weren't being called because functions are imported at module level - All cache tests now pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests