AAP-59476: API Key (Bearer) token auth module. #855
Conversation
WalkthroughAdds an API Key Token authentication module ("api-key-token") that validates a Bearer token from the Authorization header against a configured API key, wires the module into auth and authorization flows, adds config/schema/docs/examples, and includes unit tests. (28 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant AuthDep as APIKeyTokenAuthDependency
participant Config as AuthenticationConfiguration
participant AuthZ as AuthorizationResolvers
Client->>FastAPI: HTTP request (Authorization: Bearer <token>)
FastAPI->>AuthDep: call dependency(request)
AuthDep->>FastAPI: extract Authorization header/token
AuthDep->>Config: read api_key_config.api_key
alt token == config.api_key
AuthDep-->>FastAPI: return (user_id, username, skip_userid_check, token)
FastAPI->>AuthZ: resolve roles/access (noop)
FastAPI-->>Client: proceed to endpoint (200)
else invalid or missing
AuthDep-->>FastAPI: raise HTTP 401 (error detail)
FastAPI-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
|
Hi @romartin. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/authentication/api_key_token.py (2)
27-33: Consider removing unused virtual_path parameter.The
virtual_pathparameter is stored inself.virtual_pathbut never used within this class. Based on the pattern from other authentication modules and the relevant code snippets, it appears this parameter may have been copied from another authentication implementation but isn't needed for API key validation.If
virtual_pathis not needed for future enhancements, consider removing it:def __init__( - self, config: APIKeyTokenConfiguration, virtual_path: str = DEFAULT_VIRTUAL_PATH + self, config: APIKeyTokenConfiguration ) -> None: - """Initialize the required allowed paths for authorization checks.""" - self.virtual_path: str = virtual_path + """Initialize the API key authentication dependency.""" self.config: APIKeyTokenConfiguration = config self.skip_userid_check = True
44-52: Remove redundant None check for user_token.The condition
user_token is Noneis redundant becauseextract_user_token(fromauthentication.utils) never returnsNone— it either returns a string or raises anHTTPException. The None check adds unnecessary complexity.Apply this diff:
# try to extract user token from request user_token = extract_user_token(request.headers) # API Key validation. - if user_token is None or user_token != self.config.api_key: + if user_token != self.config.api_key: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid API Key", )src/models/config.py (1)
741-749: Simplify redundant None check for api_key.Line 746 checks if
self.api_key_config.api_key is None, but according to theAPIKeyTokenConfigurationmodel definition (line 705),api_keyhas a default value ofconstants.DEFAULT_API_KEY. With Pydantic, this field can never beNoneunless explicitly set toNone, which would requireOptional[str].Consider removing the redundant check:
if self.module == constants.AUTH_MOD_APIKEY_TOKEN: if self.api_key_config is None: raise ValueError( "API_KEY configuration section must be specified when using API_KEY token authentication" ) - if self.api_key_config.api_key is None: - raise ValueError( - "api_key parameter must be specified when using API_KEY token authentication" - )Alternatively, if you want to prevent users from using the default value in production, you could add a more specific validation that checks if the key equals
constants.DEFAULT_API_KEY.docs/openapi.json (3)
4530-4539: api_key_config wiring looks fine; document/validate module value.The new anyOf is good. Please either:
- document allowed values for authentication.module including "api-key-token", or
- add a non-breaking enum that includes it.
If enums are used elsewhere, ensure "api-key-token" matches the implementation string exactly to avoid drift.
4361-4362: Add a Bearer security scheme to components.Define a reusable bearer scheme so UIs show the expected “Authorization: Bearer ” flow. You can keep security application per‑endpoint or conditional at runtime.
"components": { - "schemas": { + "securitySchemes": { + "BearerAuth": { + "type": "http", + "scheme": "bearer", + "bearerFormat": "API Key" + } + }, + "schemas": {
7566-7640: Add a 401 example for invalid API key (Bearer mismatch).New module returns 401 when the Bearer token ≠ configured api_key. Add an example to make this explicit.
"UnauthorizedResponse": { ... "examples": [ { "label": "missing header", "detail": { "cause": "No Authorization header found", "response": "Missing or invalid credentials provided by client" } }, { "label": "missing token", "detail": { "cause": "No token found in Authorization header", "response": "Missing or invalid credentials provided by client" } }, + { + "label": "invalid api key", + "detail": { + "cause": "Bearer token does not match configured API key", + "response": "Missing or invalid credentials provided by client" + } + }, { "label": "expired token", "detail": { "cause": "Token has expired", "response": "Missing or invalid credentials provided by client" } },docs/output.md (1)
4009-4026: Minor markdown formatting issues flagged by linting tools.Static analysis identified several markdown formatting issues in this file:
- List indentation inconsistencies at lines 4015-4017, 4517-4519, 4965-4967 (MD007)
- Bare URL at line 4756 without link formatting (MD034)
Since this file is auto-generated documentation, these are low priority. However, if the source generation tool can be configured to emit proper markdown, it would improve compliance with linting standards.
Also applies to: 4020-4046, 4036-4048
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/auth.md(1 hunks)docs/openapi.json(27 hunks)docs/output.md(14 hunks)examples/lightspeed-stack-api-key-auth.yaml(1 hunks)src/authentication/README.md(1 hunks)src/authentication/__init__.py(2 hunks)src/authentication/api_key_token.py(1 hunks)src/authorization/middleware.py(1 hunks)src/constants.py(2 hunks)src/models/config.py(4 hunks)src/utils/README.md(1 hunks)tests/unit/authentication/README.md(1 hunks)tests/unit/authentication/test_api_key_token.py(1 hunks)tests/unit/utils/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.pysrc/authentication/__init__.pysrc/constants.pysrc/authentication/api_key_token.pysrc/authorization/middleware.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/authentication/test_api_key_token.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/authentication/test_api_key_token.py
src/**/__init__.py
📄 CodeRabbit inference engine (CLAUDE.md)
Package
__init__.pyfiles must contain brief package descriptions
Files:
src/authentication/__init__.py
src/**/constants.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define shared constants in central
constants.pyfile with descriptive comments
Files:
src/constants.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
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/unit/authentication/test_api_key_token.pydocs/auth.mdexamples/lightspeed-stack-api-key-auth.yamlsrc/authorization/middleware.py
🧬 Code graph analysis (3)
tests/unit/authentication/test_api_key_token.py (2)
src/authentication/api_key_token.py (1)
APIKeyTokenAuthDependency(23-54)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(702-705)
src/authentication/__init__.py (3)
src/authentication/api_key_token.py (1)
APIKeyTokenAuthDependency(23-54)src/configuration.py (2)
configuration(73-77)authentication_configuration(108-113)src/models/config.py (1)
api_key_configuration(776-784)
src/authentication/api_key_token.py (4)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(8-30)src/log.py (1)
get_logger(7-13)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(702-705)
🪛 LanguageTool
docs/output.md
[style] ~4041-~4041: Consider using “inaccessible” to avoid wordiness.
Context: ...figured in the llama-stack run.yaml are not accessible to lightspeed-core agents. | | authenti...
(NOT_ABLE_PREMIUM)
[style] ~4539-~4539: Consider using “inaccessible” to avoid wordiness.
Context: ...figured in the llama-stack run.yaml are not accessible to lightspeed-core agents. Useful reso...
(NOT_ABLE_PREMIUM)
[style] ~4599-~4599: It’s more common nowadays to write this noun as one word.
Context: ...connect to | | user | string | Database user name used to authenticate | | password | str...
(RECOMMENDED_COMPOUNDS)
[style] ~4769-~4769: ‘on a daily basis’ might be wordy. Consider a shorter alternative.
Context: ...ific value periodically (for example on a daily basis), set quota_increase. | Field | T...
(EN_WORDINESS_PREMIUM_ON_A_DAILY_BASIS)
[grammar] ~4775-~4775: Use a hyphen to join words.
Context: ...luster_limiter | | name | string | Human readable quota limiter name | | initial_...
(QB_NEW_EN_HYPHEN)
[grammar] ~4778-~4778: Use a hyphen to join words.
Context: ...iod | string | Period specified in human readable form | ## QuotaSchedulerConfi...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/output.md
4015-4015: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4016-4016: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4017-4017: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4517-4517: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4518-4518: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4519-4519: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4756-4756: Bare URL used
(MD034, no-bare-urls)
4965-4965: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4966-4966: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
4967-4967: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ 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: e2e_tests (azure)
🔇 Additional comments (9)
src/authorization/middleware.py (1)
36-45: LGTM! Correct integration with authorization system.The API key token authentication module is correctly mapped to
NoopRolesResolverandNoopAccessResolver, consistent with the other noop-like authentication modules.docs/auth.md (1)
145-160: LGTM! Clear and consistent documentation.The API Key Token authentication documentation is well-structured and follows the same format as other authentication modules. The configuration example and behavior description are clear.
tests/unit/authentication/test_api_key_token.py (1)
19-122: LGTM: Comprehensive test coverage.The test suite provides excellent coverage for the API key authentication flow, including:
- Successful authentication with valid Bearer token
- Missing Authorization header scenario
- Invalid Bearer scheme handling
- Invalid API key validation
src/models/config.py (2)
702-705: LGTM: Configuration class follows Pydantic patterns.The
APIKeyTokenConfigurationclass correctly extendsConfigurationBaseand uses the default API key from constants, consistent with the codebase's configuration patterns.
775-784: LGTM: Property accessor follows established patterns.The
api_key_configurationproperty accessor is consistent with the existingjwk_configurationandrh_identity_configurationaccessors, providing appropriate validation and type safety.src/constants.py (1)
117-117: Ensure users override the default API key in production.The default value
"some-api-key"is a weak placeholder. While this is acceptable for development and testing, ensure that documentation clearly warns users to configure a strong, unique API key in production environments. Consider adding validation or warnings when this default value is detected.⛔ Skipped due to learnings
Learnt from: CR Repo: lightspeed-core/lightspeed-stack PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T16:58:04.410Z Learning: Applies to src/**/constants.py : Define shared constants in central `constants.py` file with descriptive commentsdocs/openapi.json (1)
4696-4735: LGTM: helpful doc/title/description enrichments.The expanded titles/descriptions (CORS, Configuration, LlamaStack, Service, TLS) improve clarity without changing signatures.
Also applies to: 4744-4787, 6047-6093, 7295-7345, 7488-7489
docs/output.md (2)
3871-3880: New APIKeyTokenConfiguration schema properly documented.The
APIKeyTokenConfigurationdocumentation is minimal but appropriate for this authentication module. The singleapi_keyfield is clearly documented, and the placement within the components section is consistent with other configuration types.
3943-3943: api_key_config field correctly integrated into AuthenticationConfiguration.The optional
api_key_configfield is properly added toAuthenticationConfigurationalongside existing auth config options (k8s_cluster_api, jwk_config, rh_identity_config), maintaining consistency with the configuration hierarchy.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/authentication/api_key_token.py (1)
1-7: Update the module docstring to reflect the correct return tuple.The module docstring on line 6 still documents the return tuple incorrectly. The first element should be
DEFAULT_USER_UID, notDEFAULT_USER_NAME, to match the actual return statement on line 54.
🧹 Nitpick comments (1)
examples/lightspeed-stack-api-key-auth.yaml (1)
1-15: Consider adding a comment about production API key requirements.The example configuration is correct and well-structured. However, since this file serves as a template, consider adding a comment above the
api_keyfield to remind users to use a strong, randomly-generated key in production environments.Example addition:
authentication: module: "api-key-token" api_key_config: + # WARNING: Use a strong, randomly-generated API key in production. + # Example: Use `openssl rand -base64 32` to generate a secure key. api_key: "some-api-key"This helps prevent users from deploying with weak keys.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/lightspeed-stack-api-key-auth.yaml(1 hunks)src/authentication/__init__.py(2 hunks)src/authentication/api_key_token.py(1 hunks)tests/unit/authentication/test_api_key_token.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/authentication/test_api_key_token.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/authentication/init.py
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/authentication/api_key_token.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
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:
examples/lightspeed-stack-api-key-auth.yaml
🧬 Code graph analysis (1)
src/authentication/api_key_token.py (4)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(8-30)src/log.py (1)
get_logger(7-13)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(702-705)
⏰ 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 (azure)
- GitHub Check: e2e_tests (ci)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/authentication/api_key_token.py (1)
1-7: Complete the module docstring correction.The return tuple documentation still lists
DEFAULT_USER_NAMEtwice instead ofDEFAULT_USER_UID, DEFAULT_USER_NAME. The previous review comment was marked as addressed, but the fix was only partial (addedskip_userid_checkbut didn't correct the first element).Apply this diff:
"""Authentication flow for FastAPI endpoints with a provided API key. Behavior: - Reads a user token from request headers via `authentication.utils.extract_user_token` and verifies the value equals to the API Key, given from configuration parameter. -- Returns a tuple: (DEFAULT_USER_NAME, DEFAULT_USER_NAME, skip_userid_check, user_token). +- Returns a tuple: (DEFAULT_USER_UID, DEFAULT_USER_NAME, skip_userid_check, user_token). """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/authentication/api_key_token.py(1 hunks)src/constants.py(2 hunks)src/models/config.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/constants.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.pysrc/authentication/api_key_token.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/*.py : All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Applied to files:
src/authentication/api_key_token.py
🧬 Code graph analysis (1)
src/authentication/api_key_token.py (4)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(8-30)src/log.py (1)
get_logger(7-13)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(703-713)
|
Please reformat the files to pass the checks (see contribution guide) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/authentication/api_key_token.py(1 hunks)src/models/config.py(4 hunks)tests/unit/authentication/test_api_key_token.py(1 hunks)tests/unit/models/config/test_dump_configuration.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/authentication/api_key_token.py
- src/models/config.py
🧰 Additional context used
📓 Path-based instructions (2)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/authentication/test_api_key_token.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/authentication/test_api_key_token.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
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/unit/authentication/test_api_key_token.py
🧬 Code graph analysis (1)
tests/unit/authentication/test_api_key_token.py (2)
src/authentication/api_key_token.py (1)
APIKeyTokenAuthDependency(24-67)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(703-714)
⏰ 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 (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (4)
tests/unit/models/config/test_dump_configuration.py (1)
152-152: LGTM! Test expectations correctly updated for new schema field.The addition of
"api_key_config": Noneto the expected authentication section in all three test functions correctly reflects the new API key configuration field introduced in this PR. The placement is consistent across all tests and aligns with the schema changes.Also applies to: 452-452, 637-637
tests/unit/authentication/test_api_key_token.py (3)
20-44: LGTM! Success case properly tested.The test correctly validates successful authentication when a valid Bearer token matching the configured API key is provided. All returned values are properly asserted.
47-76: LGTM! Missing Authorization header case properly tested.The test correctly validates that an HTTPException with status 401 is raised when no Authorization header is present. The assertion structure matches the expected error format.
78-99: LGTM! Malformed Authorization header case properly tested.The test correctly validates that an HTTPException with status 401 is raised when the Authorization header doesn't use the Bearer scheme. The test covers an important security validation.
|
Added more tests, applied suggestions and code reformatted. So as far as I am concerned, just pending on feedback on these comments from @eranco74 , please let me know your thoughts, they may imply code changes:
Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/authentication/api_key_token.py (1)
6-6: Correct the module docstring return tuple.The docstring still states the return tuple has
DEFAULT_USER_NAMEtwice, but the actual return at line 72 is(DEFAULT_USER_UID, DEFAULT_USER_NAME, self.skip_userid_check, user_token). The first element should beDEFAULT_USER_UID, notDEFAULT_USER_NAME.Apply this diff:
-- Returns a tuple: (DEFAULT_USER_NAME, DEFAULT_USER_NAME, skip_userid_check, user_token). +- Returns a tuple: (DEFAULT_USER_UID, DEFAULT_USER_NAME, skip_userid_check, user_token).
🧹 Nitpick comments (1)
tests/unit/models/config/test_authentication_configuration.py (1)
380-392: Consider adding a test for accessing api_key_configuration with the wrong module.The test coverage is good, but consider adding a test similar to lines 46-50 that verifies
ValueErroris raised when accessingauth_config.api_key_configurationwith a non-API-key-token module (e.g.,AUTH_MOD_NOOP).Example test:
def test_authentication_configuration_api_key_wrong_module() -> None: """Test that accessing api_key_configuration with wrong module raises ValueError.""" auth_config = AuthenticationConfiguration( module=AUTH_MOD_NOOP, skip_tls_verification=False, k8s_ca_cert_path=None, k8s_cluster_api=None, ) with pytest.raises( ValueError, match="API Key configuration is only available for API Key token authentication module", ): _ = auth_config.api_key_configuration
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/authentication/__init__.py(2 hunks)src/authentication/api_key_token.py(1 hunks)src/models/config.py(4 hunks)tests/unit/authentication/test_api_key_token.py(1 hunks)tests/unit/models/config/test_authentication_configuration.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/authentication/test_api_key_token.py
- src/authentication/init.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/authentication/api_key_token.pysrc/models/config.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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_authentication_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_authentication_configuration.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
🧠 Learnings (4)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/*.py : All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Applied to files:
src/authentication/api_key_token.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Applied to files:
tests/unit/models/config/test_authentication_configuration.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : Use type hints `Optional[FilePath]`, `PositiveInt`, `SecretStr` for Pydantic configuration models
Applied to files:
tests/unit/models/config/test_authentication_configuration.pysrc/models/config.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
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/unit/models/config/test_authentication_configuration.py
🧬 Code graph analysis (2)
src/authentication/api_key_token.py (4)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(8-30)src/log.py (1)
get_logger(7-13)src/models/config.py (2)
config(279-285)APIKeyTokenConfiguration(703-715)
tests/unit/models/config/test_authentication_configuration.py (1)
src/models/config.py (3)
APIKeyTokenConfiguration(703-715)AuthenticationConfiguration(718-795)api_key_configuration(787-795)
⏰ 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 (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (6)
src/authentication/api_key_token.py (1)
9-72: LGTM!The implementation follows security best practices:
- Uses
secrets.compare_digestfor constant-time comparison to prevent timing attacks- Properly accesses the SecretStr via
get_secret_value()- Includes complete type annotations and Google-style docstrings
- Follows the established authentication interface pattern
tests/unit/models/config/test_authentication_configuration.py (1)
356-378: LGTM!The test properly validates the API key token authentication configuration, including:
- Correct module assignment
- SecretStr usage for the API key
- Property accessor validation (
api_key_configuration)- Field presence verification
src/models/config.py (4)
703-716: LGTM!The
APIKeyTokenConfigurationclass properly:
- Extends
ConfigurationBase(which enforcesextra="forbid")- Uses
SecretStrto prevent accidental exposure in logs/errors- Includes proper Field configuration with
min_length=1validation- Provides JSON schema metadata for API documentation
As per coding guidelines, SecretStr is correctly used for sensitive configuration fields.
752-761: LGTM!The validation logic correctly enforces that:
api_key_configis provided when using API key token authentication- The
api_keyfield is present (though line 757's check is technically redundant due to Pydantic'smin_length=1constraint, it serves as defensive programming)
786-796: LGTM!The
api_key_configurationproperty accessor:
- Follows the established pattern used by
jwk_configurationandrh_identity_configuration- Includes proper validation with clear error messages
- Has complete type annotations
- Ensures type safety when accessing the API key configuration
726-726: LGTM!The
api_key_configfield is properly integrated intoAuthenticationConfiguration, following the same pattern asjwk_configandrh_identity_config.
romartin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/authentication/api_key_token.py (1)
1-7: Fix the module docstring - previous correction was not applied.The module docstring on line 6 still incorrectly states the return tuple as
(DEFAULT_USER_NAME, DEFAULT_USER_NAME, skip_userid_check, user_token). The actual return on line 74 is(DEFAULT_USER_UID, DEFAULT_USER_NAME, skip_userid_check, user_token).This was flagged in a previous review and marked as "done", but the fix was not correctly applied. The first element should be
DEFAULT_USER_UID, notDEFAULT_USER_NAME.Apply this diff:
"""Authentication flow for FastAPI endpoints with a provided API key. Behavior: - Reads a user token from request headers via `authentication.utils.extract_user_token` and verifies the value equals to the API Key, given from configuration parameter. -- Returns a tuple: (DEFAULT_USER_NAME, DEFAULT_USER_NAME, skip_userid_check, user_token). +- Returns a tuple: (DEFAULT_USER_UID, DEFAULT_USER_NAME, skip_userid_check, user_token). """
🧹 Nitpick comments (1)
src/models/config.py (1)
752-761: Consider removing the redundant None check.The check on line 758 for
api_key.get_secret_value() is Noneis redundant. Sinceapi_keyis a required field withmin_length=1validation (line 708), Pydantic ensures it cannot be None or empty. The validation on line 753 (checking ifapi_key_configis None) is sufficient.Additionally, this creates an inconsistency with the JWK and RH Identity validation logic (lines 739-750), which only check if the config object is None, not the inner fields.
You can simplify this to match the pattern used for other authentication modules:
if self.module == constants.AUTH_MOD_APIKEY_TOKEN: if self.api_key_config is None: raise ValueError( "API Key configuration section must be specified " "when using API Key token authentication" ) - if self.api_key_config.api_key.get_secret_value() is None: - raise ValueError( - "api_key parameter must be specified when using API_KEY token authentication" - )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/authentication/api_key_token.py(1 hunks)src/models/config.py(4 hunks)tests/unit/authentication/test_api_key_token.py(1 hunks)tests/unit/models/config/test_authentication_configuration.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/authentication/test_api_key_token.py
🧰 Additional context used
📓 Path-based instructions (5)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_authentication_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_authentication_configuration.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.pysrc/authentication/api_key_token.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
🧠 Learnings (4)
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Applied to files:
tests/unit/models/config/test_authentication_configuration.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : Use type hints `Optional[FilePath]`, `PositiveInt`, `SecretStr` for Pydantic configuration models
Applied to files:
tests/unit/models/config/test_authentication_configuration.pysrc/models/config.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
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/unit/models/config/test_authentication_configuration.py
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/**/*.py : All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Applied to files:
src/authentication/api_key_token.py
🧬 Code graph analysis (1)
tests/unit/models/config/test_authentication_configuration.py (1)
src/models/config.py (3)
APIKeyTokenConfiguration(703-715)AuthenticationConfiguration(718-796)api_key_configuration(788-796)
⏰ 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). (5)
- GitHub Check: build-pr
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (6)
src/models/config.py (2)
703-715: LGTM! SecretStr usage addresses previous feedback.The
APIKeyTokenConfigurationclass correctly usesSecretStrfor theapi_keyfield to prevent accidental exposure in logs or error messages. The field validation withmin_length=1and JSON schema metadata (format: password, writeOnly: true) are appropriate for sensitive configuration data.
787-796: LGTM! Property accessor follows established patterns.The
api_key_configurationproperty correctly follows the same implementation pattern asjwk_configurationandrh_identity_configuration, ensuring consistency across authentication module accessors.src/authentication/api_key_token.py (2)
26-33: LGTM! Class docstring follows guidelines.The class docstring correctly describes the purpose and behavior of
APIKeyTokenAuthDependency, following Google Python docstring conventions as required by the coding guidelines.
35-74: LGTM! Authentication implementation is secure and well-documented.The implementation correctly:
- Documents parameters and return values following Google Python conventions
- Uses
secrets.compare_digest()for constant-time comparison to prevent timing attacks- Properly extracts the secret value from
SecretStrusingget_secret_value()- Returns the correct 4-tuple with user authentication information
tests/unit/models/config/test_authentication_configuration.py (2)
356-378: LGTM! Test coverage follows established patterns.The test correctly validates:
- API key token authentication configuration creation
- Property accessor functionality via
api_key_configuration- Field accessibility and proper SecretStr usage
The implementation follows the same testing pattern used for JWK and RH Identity authentication modules, ensuring consistency across the test suite.
380-393: LGTM! Error case is properly tested.The test correctly validates that a
ValidationErroris raised with the appropriate message whenapi_key_configis not provided but the authentication module is set toAUTH_MOD_APIKEY_TOKEN. This follows the same validation pattern used for other authentication modules.
|
/ok-to-test |
|
@romartin: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
thank you @tisnik ! |
|
/ok-to-test |
|
thank you all!! |
4 participants
Description
Added a basic API Key authentication mechanism, really similar to the
noop_with_token, but it checks theBearertoken value is equal to a given API Key, from configuration parameters.See https://issues.redhat.com/browse/AAP-59476
It allows production deployment, although it is a basic mechanism, so no expiration and revocation, and token is not self-contained, no information on payload.
Type of change
Tools used to create PR
None
Related Tickets & Documents
Checklist before requesting a review
Testing
1.- In
lightspeed-stack.yaml, change theauthenticationsection as:2.- Run lightspeed-stack and use Baerer token to authenticate
3.- Otherwise expect an HTTP 401
Tested using Postman.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.