Skip to content

fix(api): make AuthSettings fields mandatory#25

Merged
olivermeyer merged 2 commits into
mainfrom
fix/auth-settings
Mar 31, 2026
Merged

fix(api): make AuthSettings fields mandatory#25
olivermeyer merged 2 commits into
mainfrom
fix/auth-settings

Conversation

@olivermeyer
Copy link
Copy Markdown
Collaborator

This PR makes all AuthSettings fields mandatory, and removes references to the default value for Bridge.

@olivermeyer olivermeyer requested a review from a team as a code owner March 31, 2026 14:37
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

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/__init__.py 100.00% <ø> (ø)
src/aignostics_foundry_core/api/auth.py 100.00% <ø> (ø)

@olivermeyer olivermeyer added the claude Trigger Claude Code automation label Mar 31, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Mar 31, 2026

Claude finished @olivermeyer's task in 2m 21s —— View job


PR Review Complete ✅

This PR successfully makes AuthSettings fields mandatory and removes the Bridge-specific default value. The implementation is clean, well-tested, and follows all project conventions.


Summary

Type: Breaking change (intentional and documented)
Scope: Authentication settings (api.auth)
Changes:

  • Made internal_org_id and auth0_role_claim required fields
  • Removed DEFAULT_AUTH0_ROLE_CLAIM constant and all references
  • Updated tests with proper fixtures and validation
  • Updated documentation to reflect mandatory fields

Status: ✅ Approved - Ready to merge


Quality Metrics

SonarQube: Quality Gate passed (0 new issues)
Codecov: 100% coverage maintained
Tests: All passing (unit + integration markers properly applied)
Conventional Commit: fix(api): make AuthSettings fields mandatory
TODOs: Resolved 3 TODO comments from commit 7915e19


Detailed Review

✅ Code Changes (src/aignostics_foundry_core/api/auth.py)

Lines 30-31: Removed DEFAULT_AUTH0_ROLE_CLAIM constant

  • Clean removal, no lingering references found

Lines 45-46: Made fields mandatory

internal_org_id: str
auth0_role_claim: str
  • Correct approach - forces explicit configuration
  • Good validation will occur at instantiation time via Pydantic

Lines 39-40: Added clear docstring

  • Documents that both fields are required
  • States no defaults are provided

Verification: ✅ No remaining references to removed constant


✅ Test Updates (tests/aignostics_foundry_core/api/auth_test.py)

Lines 34-47: Added _auth_context autouse fixture

  • ✅ Excellent pattern - sets required env vars automatically for all tests
  • ✅ Proper cleanup with os.environ.pop() and reset_context()
  • ✅ Uses constants from tests/aignostics_foundry_core/api/__init__.py for consistency

Lines 102-116: Updated TestAuthSettings

  • ✅ Removed obsolete tests for default values
  • ✅ Added test_auth_settings_raises_when_required_fields_absent - validates ValidationError
  • test_auth_settings_uses_context_env_prefix now verifies both fields

Lines 213-244: Updated test methods

  • ✅ Cleaned up monkeypatch usage
  • ✅ Removed redundant env var setup (now handled by autouse fixture)
  • ✅ Tests use _TEST_ROLE_CLAIM constant instead of removed default

Integration marker: ✅ Properly marked with @pytest.mark.integration


✅ Test Constants (tests/aignostics_foundry_core/api/__init__.py)

Lines 3-6: Added shared constants

INTERNAL_ORG_ID_VAR_NAME = f"{TEST_PROJECT_PREFIX}AUTH_INTERNAL_ORG_ID"
AUTH0_ROLE_CLAIM_VAR_NAME = f"{TEST_PROJECT_PREFIX}AUTH_AUTH0_ROLE_CLAIM"
  • ✅ DRY principle - single source of truth for env var names
  • ✅ Used consistently across auth_test.py and gui_test.py

✅ GUI Test Updates (tests/aignostics_foundry_core/gui/gui_test.py)

Lines 454-463 & 541-549: Added _gui_context fixtures

  • ✅ Same pattern as auth tests - consistent approach
  • ✅ Required because GUI auth depends on AuthSettings
  • ✅ Proper isolation and cleanup

✅ Documentation (src/aignostics_foundry_core/AGENTS.md)

Lines 105-106: Updated module description

  • ✅ Clearly states both fields are required
  • ✅ Explicitly mentions "Both fields are mandatory — no defaults are provided"
  • ✅ Removed DEFAULT_AUTH0_ROLE_CLAIM from constants list

✅ API Exports (src/aignostics_foundry_core/api/__init__.py)

Line 15: Removed import
Line 69: Removed from __all__

  • ✅ Clean removal from public API

Breaking Change Impact

⚠️ Breaking Change: This change requires all users to set these environment variables:

  • {PREFIX}AUTH_INTERNAL_ORG_ID
  • {PREFIX}AUTH_AUTH0_ROLE_CLAIM

Why this is good:

  1. Removes Bridge-specific hardcoded default (https://aignostics-platform-bridge/role)
  2. Forces explicit configuration (better security posture)
  3. Makes dependencies visible and testable
  4. Resolves TODO comments indicating this was planned

Migration path: Users must set both environment variables before upgrading. Clear Pydantic validation errors will guide them if they forget.


Security Considerations

No security issues identified

  • Removing hardcoded defaults improves security
  • Explicit configuration makes auth boundaries clear
  • No secrets in code or tests

Best Practices Compliance

Code Quality Principles (CLAUDE.md):

  • DRY: Constants extracted to __init__.py
  • SonarQube compliant: 0 issues
  • Clean code: No duplication, clear intent

Testing Standards (tests/CLAUDE.md):

  • All tests have category markers (@pytest.mark.unit, @pytest.mark.integration)
  • Fixtures follow best practices (autouse for common setup)
  • 100% coverage maintained

Conventional Commits:

  • Format: fix(api): make AuthSettings fields mandatory
  • Correct type (fix for breaking changes is acceptable here)
  • Clear scope and description

Suggestions

None - This PR is production-ready as-is.

The implementation is clean, well-tested, properly documented, and follows all project conventions. The breaking change is intentional and improves the codebase by removing platform-specific defaults.


Recommendation

✅ APPROVE - Ready to merge

Checklist:

  • ✅ Code quality (SonarQube passed)
  • ✅ Test coverage (100% maintained)
  • ✅ All tests passing
  • ✅ Breaking change documented and intentional
  • ✅ Conventional commit format
  • ✅ Documentation updated
  • ✅ No security issues

Great work! This PR successfully addresses the TODOs and improves the auth configuration model by making dependencies explicit. 🎉


Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonarqubecloud
Copy link
Copy Markdown

@olivermeyer olivermeyer merged commit c028774 into main Mar 31, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude Trigger Claude Code automation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant