Skip to content

LCORE-390: authentication configuration is not optional #395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 13, 2025
Merged

LCORE-390: authentication configuration is not optional #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/models/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,7 @@ class Configuration(BaseModel):
user_data_collection: UserDataCollection
database: DatabaseConfiguration = DatabaseConfiguration()
mcp_servers: list[ModelContextProtocolServer] = []
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix mutable default list for mcp_servers

mcp_servers uses a mutable default ([]), which will be shared across Configuration instances. Use a default factory to prevent cross-instance leakage.

Apply this diff:

-mcp_servers: list[ModelContextProtocolServer] = []
+mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list)
📝 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
mcp_servers: list[ModelContextProtocolServer] = []
mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list)
🤖 Prompt for AI Agents 
In src/models/config.py around line 294, mcp_servers is defined with a mutable
default list ([]), which will be shared across instances; change the definition
to use a default factory instead (e.g., if this is a dataclass use
field(default_factory=list) from dataclasses, or if this is a Pydantic model use
Field(default_factory=list) from pydantic) so each Configuration instance gets
its own new list; update the import if needed and remove the bare [] default.

authentication: Optional[AuthenticationConfiguration] = (
AuthenticationConfiguration()
)
authentication: AuthenticationConfiguration = AuthenticationConfiguration()
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Avoid shared default instance for authentication; use Field(default_factory=...)

Using a constructed BaseModel instance as a class-level default risks sharing the same instance across all Configuration objects. Prefer a factory to ensure a fresh instance per model.

Apply this diff to the authentication field:

-authentication: AuthenticationConfiguration = AuthenticationConfiguration()
+authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration)

Additionally, ensure Field is imported:

# at import line with other pydantic symbols
from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field

This change aligns the code with the PR’s goal (authentication is not optional) but may uncover existing configs passing authentication: null. Run this to find potential offenders:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching YAML for 'authentication: null' or '~' or empty mapping lines"
rg -n --glob '*.yaml' --glob '*.yml' -e $'^\s*authentication\s*:\s*(null|~)\s*$' -e $'^\s*authentication\s*:\s*$' || true

echo "Searching Python for explicit authentication=None"
rg -n --type py -e $'authentication\s*=\s*None' -A 2 || true

Length of output: 634

Use a fresh AuthenticationConfiguration via default_factory

Sharing a single AuthenticationConfiguration() across all Configuration instances risks cross-instance state. Update the field and imports as follows, and adjust your example YAMLs to remove any explicit authentication: entries (which currently parse as null and will now error):

  • In src/models/config.py, replace:

    - authentication: AuthenticationConfiguration = AuthenticationConfiguration()
+ authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration)

  • In the same file’s imports, add Field:

    - from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl
+ from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field

  • Remove or replace the blank/null authentication: lines in your YAML examples so they don’t explicitly set null:

    • examples/lightspeed-stack.yaml:24
    • examples/lightspeed-stack-lls-external.yaml:26
    • examples/lightspeed-stack-lls-library.yaml:25

    You can either delete those lines (letting the default factory apply) or set them to an empty mapping (authentication: {}) if you need an explicit no-overrides placeholder.

📝 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
authentication: AuthenticationConfiguration = AuthenticationConfiguration()
# In src/models/config.py
from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field
class Configuration(BaseModel):
# …
authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration)
# …

customization: Optional[Customization] = None
inference: InferenceConfiguration = InferenceConfiguration()

Expand Down
47 changes: 47 additions & 0 deletions tests/unit/models/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,53 @@ def test_dump_configuration_with_more_mcp_servers(tmp_path) -> None:
]


def test_authentication_configuration_in_config() -> None:
"""Test the authentication configuration in main config."""
cfg = Configuration(
name="test_name",
service=ServiceConfiguration(),
llama_stack=LlamaStackConfiguration(
use_as_library_client=True,
library_client_config_path="tests/configuration/run.yaml",
),
user_data_collection=UserDataCollection(
feedback_enabled=False, feedback_storage=None
),
mcp_servers=[],
)
assert cfg.authentication is not None
assert cfg.authentication.module == AUTH_MOD_NOOP
assert cfg.authentication.skip_tls_verification is False
assert cfg.authentication.k8s_ca_cert_path is None
assert cfg.authentication.k8s_cluster_api is None

cfg2 = Configuration(
name="test_name",
service=ServiceConfiguration(),
llama_stack=LlamaStackConfiguration(
use_as_library_client=True,
library_client_config_path="tests/configuration/run.yaml",
),
user_data_collection=UserDataCollection(
feedback_enabled=False, feedback_storage=None
),
mcp_servers=[],
authentication=AuthenticationConfiguration(
module=AUTH_MOD_K8S,
skip_tls_verification=True,
k8s_ca_cert_path="tests/configuration/server.crt",
k8s_cluster_api=None,
),
)
assert cfg2.authentication is not None
assert cfg2.authentication.module == AUTH_MOD_K8S
assert cfg2.authentication.skip_tls_verification is True
assert cfg2.authentication.k8s_ca_cert_path == Path(
"tests/configuration/server.crt"
)
assert cfg2.authentication.k8s_cluster_api is None


def test_authentication_configuration() -> None:
"""Test the AuthenticationConfiguration constructor."""

Expand Down