Code review: issues to address before v1.0

[lukwam]

Code Review Summary

Overall assessment: 🟢 Excellent foundation — architecture, typing, and provider implementations are production-quality. The following items should be addressed before v1.0.

---

🔴 High Priority

1. No FastAPI Adapter Tests

The FastAPI adapter is ~384 lines with error mapping, telemetry, OIDC dependency factory, and all Auth.* methods — all untested. This is the largest code gap.

2. No Django Ninja Adapter Tests

Same issue — no tests for any of the Ninja auth classes (ApiKeyAuth, FirebaseAuth, GoogleAuth, OIDCAuth).

3. Flask Adapter Stores client_secret in Session

flask/__init__.py stores the OAuth2 client secret in the Flask session. If sessions are cookie-based (even encrypted), this is a security risk. The secret is already available from the client_secrets_file and does not need to be stored in the session.

4. Flask Adapter Bypasses AuthService

The Flask adapter directly uses google-auth and google-auth-oauthlib instead of going through the Provider → Service → Adapter pattern. This is architecturally inconsistent with the other adapters.

---

🟡 Medium Priority

5. No Telemetry in Django Ninja Adapter

The FastAPI adapter has _log_failure() and _set_request_auth_context() for structured auth telemetry. The Ninja adapter silently swallows errors (except AuthError: return None) with no logging. Should at least log failures for observability.

6. AuthCascade Does Not Handle Exceptions

If an auth method raises an exception instead of returning None, the cascade stops with an unhandled error. The Ninja auth classes return None on failure so this works today, but it is fragile.

Suggested fix:

# Current (fragile):
for method in self.auth_methods:
    result = method.authenticate(request)
    if result is not None:
        return result

# Safer:
for method in self.auth_methods:
    try:
        result = method.authenticate(request)
        if result is not None:
            return result
    except AuthError:
        continue

7. AuthPrincipal Model is Dead Code

core/models.py defines AuthPrincipal but it is never referenced in service, adapters, or tests. Either integrate it or remove it.

8. No Coverage Threshold in CI

Tests run with --cov but no --cov-fail-under. Adding --cov-fail-under=80 would prevent coverage regression.

9. CHANGELOG Missing Flask Adapter

The [0.1.0] entry mentions FastAPI and Django Ninja adapters but not the Flask adapter. The date 2024-01-01 also appears to be a placeholder.

---

🟢 Low Priority / Nitpicks

Issue	Notes
AuthCascade typed as list[Any]	Could use list[Callable] for type safety
No reset() for test isolation	Module-level _service singleton in FastAPI adapter has no reset function
IAP provider is header-only	Does not verify IAP JWT tokens — documented but should be noted more prominently
No token refresh in Flask adapter	Access token expiry is not handled
Poetry version not pinned in CI	pipx install poetry should pin a version for reproducibility
GoogleTokenInfo.jlt field	Unusual JWT claim name — is this jti? Worth documenting

---

Test Coverage Summary

Area	Status
Models	✅ Good
Exceptions	✅ Good
Policies	✅ Good
Service	✅ Good
Cascade	✅ Adequate
All 6 Providers	✅ Good to Excellent
Flask Adapter	✅ Good
FastAPI Adapter	❌ No tests
Django Ninja Adapter	❌ No tests
Telemetry	❌ No tests

---

Recommendations

1. Add FastAPI and Django Ninja adapter tests
2. Fix the Flask session client_secret storage
3. Add telemetry to the Django Ninja adapter
4. Add exception handling to AuthCascade
5. Add --cov-fail-under=80 to CI
6. Either use AuthPrincipal or remove it
7. Update CHANGELOG to include Flask adapter and correct the date

[lukwam]

Resolved in 3d60c94

[lukwam]

Re-Review (Round 2)

Good progress — 7 of 11 items fully fixed, 3 partially fixed, 1 not fixed. However, a second pass found additional issues.

Original Issues Status

#	Issue	Status
1	FastAPI adapter tests	✅ Fixed
2	Django Ninja adapter tests	✅ Fixed
3	Flask client_secret in session	⚠️ Partially — client_secret removed but refresh_token now stored in session (same security risk)
4	Flask bypasses AuthService	❌ Not fixed — OAuth2FlowManager still uses google-auth directly
5	No telemetry in Ninja adapter	✅ Fixed
6	AuthCascade exception handling	✅ Fixed
7	AuthPrincipal dead code	⚠️ Partially — removed from models but still in __all__ (see below)
8	Coverage threshold in CI	✅ Fixed (--cov-fail-under=80)
9	CHANGELOG missing Flask	✅ Fixed
10	AuthCascade typing	✅ Improved (list[Callable])
11	Poetry version pinned	✅ Fixed (poetry==2.0.1)

---

New Issues Found

🔴 Bug: AuthPrincipal still in __all__

src/altissimo/auth/__init__.py line 34 still lists "AuthPrincipal" in __all__, but the class was removed from models.py. This will cause an ImportError for anyone doing:

from altissimo.auth import AuthPrincipal  # ImportError!

Fix: Remove "AuthPrincipal" from __all__.

🔴 Bug: Wrong field name in test fixtures

Both test_fastapi.py:73 and test_ninja.py:72 create:

FirebaseUser(uid="user-123", email="user@example.com", email_verified=True, disabled=False, claims={})

The actual field on FirebaseUser is custom_claims, not claims. Because FirebaseUser has extra="allow", this silently stores claims as extra data and leaves custom_claims as None.

Fix: Change claims={} to custom_claims={}.

🟡 Flask adapter stores refresh_token in session cookie

flask/__init__.py lines 94-97 now stores google_token, google_refresh_token, google_id_token, and google_client_id in the session. Refresh tokens are long-lived — if the cookie signing key is compromised, all refresh tokens are exposed. The refresh token should be stored server-side (database or encrypted store), not in cookies.

🟡 Ninja _log_failure fallback uses wrong reason code

ninja/__init__.py _log_failure(): if auth_source is not API_KEY, FIREBASE, or GOOGLE, the function falls back to AuthReasonCode.INVALID_API_KEY. This is incorrect for SERVICE_ACCOUNT or WEBHOOK auth sources. Should use a generic fallback reason code or handle all source types.

🟡 Adapter test coverage is still thin

The new tests only cover API key + Firebase scenarios. Missing coverage:

FastAPI adapter — no tests for:

• Auth.validate_firebase_admin
• Auth.validate_google_user / Auth.validate_google_admin
• Auth.create_oidc_dependency
• Auth.get_iap_identity
• Auth.verify_webhook
• Auth.validate_api_key_optional
• _handle_error 403/404 paths

Ninja adapter — no tests for:

• ApiKeyQueryAuth
• ApiKeyAuth (combined header+query)
• FirebaseAdminAuth
• GoogleAuth / GoogleAdminAuth
• OIDCAuth
• _log_failure() helper
• configure() / _get_service() error path

🟡 Flask test_logout incomplete

The test only checks that oauth_state and user_info are cleared, but logout() also clears google_token, google_refresh_token, google_id_token, and google_client_id. These should be verified.

🟢 Minor: Version mismatch

pyproject.toml has version = "0.1.0" but CHANGELOG.md says [1.0.0] - 2026-06-07. These should be consistent.