fix: resolve OAuth compatibility issues for login-flow deployment

[cbcoutinho]

Summary

• Drop OIDC fork: Use upstream v1.16.3 from Nextcloud app store instead of third_party/oidc fork (upstream PR fix: Preserve OIDC params across SAML session regeneration H2CK/oidc#631 merged the same consent redirect fix plus SAML session regeneration fixes)
• Support client_secret_basic auth: Claude Code's TS MCP SDK sends client_id via HTTP Basic Auth header, not form body — added _extract_basic_auth() helper to token endpoint
• Multi-issuer JWT validation: AS proxy obtains tokens server-to-server (issuer http://app:80) but verifier expected public URL (http://localhost:8080) — now accepts both
• Introspection fallback: When JWT verification fails, try token introspection before rejecting (supports both JWT and opaque token types)
• Register all tool scopes in DCR: Added semantic:read, collectives:read/write to OIDC client allowed_scopes so semantic search tools appear for authenticated clients
• Auto-create Astrolabe OAuth client: New app-hook (26-configure-astrolabe-oauth.sh) creates OIDC client and stores credentials in config.php so the "Authorize via OAuth" button works without manual setup

Context

Claude Code couldn't authenticate to the login-flow MCP server due to three cascading issues:

1. Token endpoint rejected client_secret_basic (400: "client_id is required")
2. JWT issuer mismatch between Docker-internal and public URLs (401: "Invalid issuer")
3. Semantic search tools filtered out because semantic:read scope wasn't registered in OIDC client

Additionally, the Astrolabe "Authorize via OAuth" button in Nextcloud settings redirected to /app because no OAuth client was configured for Astrolabe.

Test plan

• uv run pytest -m login_flow -v --browser firefox — 12 passed, 1 skipped, 1 xfailed
• Claude Code successfully authenticates to http://localhost:8004/mcp
• nc_semantic_search, nc_semantic_search_answer, nc_get_vector_sync_status tools visible
• Collectives tools visible
• Astrolabe OAuth client auto-created during container startup
• Astrolabe "Authorize via OAuth" button completes full OAuth flow (not just redirect)

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

PR Review: fix OAuth compatibility issues for login-flow deployment

This PR addresses three real issues that were blocking Claude Code from authenticating to the login-flow MCP server. The fixes are targeted and the root causes are well-explained in the description. A few items worth discussing before merging:

---

Security

client_secret from Basic Auth is discarded without validation (oauth_routes.py:976)

if not client_id:
    client_id, _ = _extract_basic_auth(request)  # client_secret is thrown away

For public clients (like Claude Code) that use PKCE, this is fine — they have no secret. But for confidential clients (--type confidential as configured in the new app-hook), the secret SHOULD be validated against the stored client record if one is provided. The current code accepts Authorization: Basic <any-client-id>:anything equally. Consider adding validation: if a client_secret is present in the Basic Auth header AND the stored client entry has a secret, verify them.

Introspection fallback on JWT validation failure (unified_verifier.py:223-230)

When JWT verification returns None (which covers signature failures, expiry, issuer mismatch, etc.), the code falls back to introspection. The intent is to handle the issuer-mismatch case, but the same path is taken for a token with an invalid signature. This is safe only because the AS-side introspection independently re-validates the token — but it means a token with a bad signature no longer produces a clear rejection. I'd recommend logging the specific JWT failure reason before falling back, to preserve debuggability:

except jwt.InvalidSignatureError:
    logger.warning("JWT signature invalid, skipping introspection fallback")
    return None
except jwt.InvalidIssuerError as e:
    logger.debug(f"JWT issuer mismatch: {e}, trying introspection")
    # fall through to introspection

---

Correctness

nextcloud_host used as an OIDC issuer (unified_verifier.py:91-94)

if hasattr(settings, "nextcloud_host") and settings.nextcloud_host:
    host = settings.nextcloud_host.rstrip("/")
    if host not in self.valid_issuers:
        self.valid_issuers.append(host)

nextcloud_host is the base URL of the Nextcloud instance, but OIDC issuers are specific values set in the discovery document — typically <nextcloud_host> itself but not guaranteed. If a Nextcloud instance has overwritewebroot or a proxy that changes the issuer path, this will silently accept tokens from a different issuer. It would be safer to add an explicit NEXTCLOUD_INTERNAL_ISSUER_URL config variable (or read it from the discovery document) rather than assuming the host is the issuer.

hasattr() checks on settings (multiple places in unified_verifier.py)

hasattr(settings, "oidc_issuer"), hasattr(settings, "nextcloud_host") — if Settings is a Pydantic model, these attributes always exist (possibly as None). The hasattr guard is misleading; just check truthiness: if settings.oidc_issuer:.

---

Code Quality

Commented-out code in CI workflow (.github/workflows/test.yml:122-130)

Dead code in CI is noise. If the OIDC fork is definitively dropped, remove the commented block rather than preserving it. The git history captures the deletion.

Hardcoded CLIENT_ID in app-hook (26-configure-astrolabe-oauth.sh:42)

CLIENT_ID="astrolabeMcpClientOAuth00000000000"

A fixed client ID is intentional for idempotency (the EXISTING_CLIENT_ID check prevents re-creation), but it's worth a brief comment explaining that this is by design. Also, a typo: the string is 34 chars — comment says 32-64 but it's within range, just worth double-checking the OIDC app's client_id length requirements.

semantic:read only added when vector_sync_enabled (app.py:463-464)

This is the right design, but the comment says collectives:read collectives:write were also added to the DCR scopes — those appear unconditionally in the string. If Collectives isn't always enabled, they should follow the same conditional pattern as semantic:read. If Collectives is always available, a brief note explaining the asymmetry would help.

---

Minor

• The emoji in logger.info("✓ semantic:read scope enabled...") is fine given the existing codebase style
• _extract_basic_auth is clean and well-referenced to RFC 6749 §2.3.1 — good addition
• Multi-issuer validation with manual iss check after signature verification is the correct approach when PyJWT's built-in issuer check is too narrow

---

Summary

The core fixes (Basic Auth support, multi-issuer JWT validation, scope registration) are correct and well-motivated. The main items I'd address before merging are: (1) the unanswered question of whether confidential client secrets from Basic Auth should be validated, and (2) replacing the nextcloud_host-as-issuer assumption with either a dedicated config variable or a discovery-document lookup. The introspection fallback granularity is worth improving for observability.

[claude]

PR Review: fix: resolve OAuth compatibility issues for login-flow deployment

Good set of fixes that address real authentication failures. The overall approach is sound. A few issues worth addressing before merge:

---

Bugs / Security Issues

1. Client secret from Basic Auth is not validated (oauth_routes.py:975–979)

_extract_basic_auth extracts both client_id and client_secret, but the call site discards the secret:

if not client_id:
    client_id, _ = _extract_basic_auth(request)

For confidential clients authenticating via client_secret_basic, the secret should be validated against the stored client credential. Without this, any caller that knows a valid client_id can authenticate as that client without knowing the secret. At minimum, either validate the secret or add a comment explaining why it's safe to skip here (e.g., because PKCE is sufficient for this flow).

2. Silent issuer bypass when valid_issuers is empty (unified_verifier.py:433–437)

if not skip_issuer_check and self.valid_issuers:
    token_issuer = payload.get("iss")
    if token_issuer not in self.valid_issuers:
        raise jwt.InvalidIssuerError(...)

If oidc_issuer and nextcloud_host are both unset/empty, self.valid_issuers is [] and the condition short-circuits — issuer validation is silently skipped. This is a security regression from the original code which required oidc_issuer to be set before doing issuer validation. Consider logging a warning when valid_issuers ends up empty after __init__, or raising a configuration error at startup if neither is available.

3. Hardcoded OAuth client ID in 26-configure-astrolabe-oauth.sh

CLIENT_ID="astrolabeMcpClientOAuth00000000000"

A predictable, fixed client ID across all deployments makes enumeration trivial. Consider generating a random one (e.g., openssl rand -hex 16 | tr -d '\n' | head -c 32) or at least documenting why a fixed ID is acceptable here (e.g., the secret is still randomized and the redirect URI is validated). The idempotency skip check (EXISTING_CLIENT_ID) depends on this being stable, so if you change it, note the tradeoff.

---

Minor Issues

4. IPv6 loopback not handled in redirect URI validation (oauth_routes.py:196–205)

RFC 8252 §8.3 permits ::1 as a loopback address alongside 127.0.0.1 and localhost. Current code would reject http://[::1]:port/callback. Low priority unless you expect native clients on IPv6-only systems.

5. Commented-out code in test.yml should be removed

# - name: Build OIDC app
#   run: |
#     cd third_party/oidc
#     ...

Git history preserves this; the comment block just adds noise. Either delete it or replace with a single-line comment explaining the change.

6. hasattr checks in unified_verifier.py are overly defensive

if hasattr(settings, "oidc_issuer") and settings.oidc_issuer:
    ...
if hasattr(settings, "nextcloud_host") and settings.nextcloud_host:
    ...

Settings is a known, typed Pydantic model — these fields either exist or they don't at class definition time. Using hasattr suggests uncertainty about the model's shape. Prefer getattr(settings, "oidc_issuer", None) or just direct attribute access with a None check.

---

Positive Notes

• _extract_basic_auth is clean and handles percent-encoding correctly with unquote.
• Relaxing the redirect URI check from localhost-only to allow HTTPS for remote hosts is the right call for cloud clients.
• Adding collectives:read/write and conditional semantic:read to DCR scopes is a clean, well-structured fix.
• The introspection fallback logic is well-placed and won't silently swallow hard errors — it only triggers after JWT validation returns None.
• App-hook scripts are well-guarded (idempotency checks, set -e, missing-config exits).

---

Overall: the correctness issues in #1 and #2 should be addressed before merging given their security implications. The rest are polish.