feat(auth): consolidate auth config in AuthSettings

[olivermeyer]

Why?
Auth configuration was split between the foundry-python template (seven fields under {APP}_AUTH0_* / {APP}_SESSION_*) and the foundry-python-core library (two required fields under {APP}_AUTH_*), making operator configuration inconsistent and error-prone.

How?
All nine auth fields are moved into AuthSettings in the library under the uniform {APP}_AUTH_ prefix, with empty-string defaults and a @model_validator that enforces cross-field dependencies (e.g. enabled requires session_enabled, client_secret, non-empty domain, etc.) only when auth is actually enabled. The old auth0_role_claim field is renamed to role_claim, all internal references updated, and the README auth table expanded to document all nine fields.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines	Coverage Δ
src/aignostics_foundry_core/api/auth.py	100.00% <100.00%> (ø)
src/aignostics_foundry_core/gui/auth.py	93.05% <100.00%> (ø)

[claude]

Claude finished @olivermeyer's task in 3m 25s —— View job

---

Code Review Complete ✅

Comprehensive review of PR #57: feat(auth): consolidate auth config in AuthSettings

Overall Assessment: Strong implementation with excellent test coverage (100%) and zero SonarQube issues. One blocking concern regarding breaking changes and migration risk.

---

🔴 Blocking Issues

1. Breaking Change - Migration Risk Not Addressed

Location: src/aignostics_foundry_core/api/auth.py:59-72

Issue: This PR introduces a breaking change that could silently disable authentication in existing deployments:

Before this PR:

• internal_org_id and auth0_role_claim were required (no defaults)
• Deployments would fail fast if these weren't configured
• Auth was implicitly "always enabled"

After this PR:

• All fields optional with enabled=False as default
• internal_org_id and auth0_role_claim (now role_claim) have empty defaults
• Auth could be silently disabled if operators don't set {PREFIX}AUTH_ENABLED=true

Migration Impact:

1. Environment variable rename: AUTH_AUTH0_ROLE_CLAIM → AUTH_ROLE_CLAIM requires config updates
2. New required field: Existing deployments must add {PREFIX}AUTH_ENABLED=true
3. Silent failure mode: If operators only update the renamed field, auth validation won't run (enabled=False)

Recommendation: Add migration documentation:

• Document the breaking changes in CHANGELOG or PR description
• Provide migration checklist for operators
• Consider whether enabled should default to True when any auth field is set (fail-safe design)

Example Migration Guide:

# Required changes for existing deployments:
# 1. Rename env var
-MYAPP_AUTH_AUTH0_ROLE_CLAIM=https://example.com/role
+MYAPP_AUTH_ROLE_CLAIM=https://example.com/role

# 2. Add new required fields
+MYAPP_AUTH_ENABLED=true
+MYAPP_AUTH_SESSION_SECRET=<generate-secret>
+MYAPP_AUTH_SESSION_EXPIRATION=86400  # optional, has default
+MYAPP_AUTH_DOMAIN=example.auth0.com
+MYAPP_AUTH_CLIENT_ID=<client-id>
+MYAPP_AUTH_CLIENT_SECRET=<client-secret>

---

💡 Suggestions (Non-Blocking)

2. DRY Violation in Validator ⚠️ Code Quality

Location: src/aignostics_foundry_core/api/auth.py:89-107

Issue: The @model_validator has 6 nearly identical if-blocks checking required fields when enabled=True. Per CLAUDE.md Code Quality Principles (SonarQube S1192), duplicated code should be refactored.

Current Code:

if self.enabled and self.session_secret is None:
    msg = "AUTH_SESSION_SECRET must not be None when AUTH_ENABLED is True"
    raise ValueError(msg)
if self.enabled and self.client_secret is None:
    msg = "AUTH_CLIENT_SECRET must not be None when AUTH_ENABLED is True"
    raise ValueError(msg)
# ... 4 more similar blocks

Suggested Refactor:

@model_validator(mode="after")
def validate_auth_dependencies(self) -> "AuthSettings":
    """Validate cross-field auth dependencies."""
    if not self.enabled:
        return self
    
    required_fields = {
        "session_secret": (self.session_secret is None, "AUTH_SESSION_SECRET must not be None"),
        "client_secret": (self.client_secret is None, "AUTH_CLIENT_SECRET must not be None"),
        "domain": (not self.domain, "AUTH_DOMAIN must not be empty"),
        "client_id": (not self.client_id, "AUTH_CLIENT_ID must not be empty"),
        "internal_org_id": (not self.internal_org_id, "AUTH_INTERNAL_ORG_ID must not be empty"),
        "role_claim": (not self.role_claim, "AUTH_ROLE_CLAIM must not be empty"),
    }
    
    for field_name, (is_invalid, error_msg) in required_fields.items():
        if is_invalid:
            raise ValueError(f"{error_msg} when AUTH_ENABLED is True")
    
    return self

Why: Reduces code duplication, easier to maintain, follows DRY principle.

---

3. Missing "Happy Path" Test ⚠️ Test Coverage

Location: tests/aignostics_foundry_core/api/auth_test.py:113-171

Issue: Tests cover all failure cases (7 tests for invalid states) but no test validates the successful case where enabled=True with all required fields set.

Suggested Addition:

def test_enabled_with_all_required_fields_passes(self) -> None:
    """enabled=True with all required fields set validates successfully."""
    settings = AuthSettings(
        enabled=True,
        session_secret=_TEST_SESSION_SECRET,
        client_secret=_TEST_CLIENT_SECRET,
        domain=_TEST_DOMAIN,
        client_id=_TEST_CLIENT_ID,
        internal_org_id=_INTERNAL_ORG_ID,
        role_claim=_TEST_ROLE_CLAIM,
    )
    assert settings.enabled is True
    assert settings.session_secret is not None
    assert settings.client_secret is not None
    # Validates that the model_validator doesn't raise

Why: Confirms the validator allows valid configurations (not just rejects invalid ones).

---

✅ What's Good

1. Test Coverage: 100% coverage on new code (Codecov), all tests properly marked with @pytest.mark.unit/@pytest.mark.integration
2. Security: Proper use of SecretStr with PlainSerializer for sensitive fields
3. Type Safety: Excellent type annotations, Pydantic v2 best practices
4. Validation: Comprehensive cross-field validation logic
5. Documentation: README table expanded with all 8 auth fields clearly documented
6. Conventional Commits: ✅ Both commits follow conventional commit format correctly
7. Code Style: ✅ SonarQube passed (0 issues), Ruff compliant
8. Field Constraints: Sensible validation rules (session_expiration bounds, client_secret length, domain max_length)

---

📋 Summary

Commits Reviewed:

• 9f6a3be feat(auth): consolidate auth config in AuthSettings
• e7effd2 refactor(auth): drop redundant session_enabled field

Files Changed: 8 files (+185/-63)
Test Coverage: 100% on new code ✅
Quality Gates: All passing ✅
Breaking Changes: Yes ⚠️ (migration path needed)

Blocking: 1 issue
Suggestions: 2 improvements

---

Recommendation

Request changes to address the breaking change migration risk (#1). Once migration documentation is added (either in PR description, CHANGELOG, or a migration guide), this PR is ready to merge.

The code quality suggestions (#2, #3) are nice-to-have improvements but not blockers.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[arne-aignx]

LGTM.