aminghadersohi commented on PR #45639: URL: https://github.com/apache/superset/pull/45639#issuecomment-6376091436
@gabotorresruiz Thanks for the review. I followed up on all four inline asks in 272e4a61c091eddec8e47caa73fe820e1ffec0a2, retained at merge head 124b49628647cc9df2bead1f676ead7fae73e65b: - Expiry/type coverage: required `exp`, added its structural parameter, and pinned non-guest types. `test_guest_reverification_requires_core_structure[exp]` failed before and passes after; removing the type condition makes both new `test_guest_reverification_rejects_non_guest_type` cases fail. - Repeated verification cost: I answered the question with the DAO/context-lifecycle evidence and documented the deliberate uncached freshness policy. `test_guest_reverification_observes_later_dashboard_revocation` checks a successful resolution followed by revocation across direct resolution and both discovery paths. Remaining: no per-request cache is introduced; I left that question open for your response. - Order dependence: moved the middleware import to module scope. The exact two-file order went from 1 failed/70 passed to 80 passed, including the new cases; the original failing regression is `test_disabled_api_key_assertion_hides_tools[tool_list]`. - Upgrade guidance: added both behavior changes under `UPDATING.md`'s `Next` and updated the resolver table. Four documentation assertions fail against the previous head and pass against this head. The runtime contracts are pinned by `test_default_user_resolver_uses_unqualified_client_id`, `test_api_key_disabled_rejects_explicit_auth`, and `test_disabled_api_key_assertion_hides_tools`. The sibling search also found discovery treating invalid guest credentials as absent auth and mishandling revocation-backend errors. The tool-search revocation case plus all three `test_guest_reverification_backend_failure_is_credential_failure` cases failed before and pass after the changes. Siblings checked: - superset/mcp_service/auth.py:599 — fixed in 272e4a61c091eddec8e47caa73fe820e1ffec0a2 - superset/mcp_service/server.py:450 — fixed in 272e4a61c091eddec8e47caa73fe820e1ffec0a2 - tests/unit_tests/mcp_service/test_multi_auth_client_id.py:56 — fixed in 272e4a61c091eddec8e47caa73fe820e1ffec0a2 - UPDATING.md:26 — fixed in 272e4a61c091eddec8e47caa73fe820e1ffec0a2 - docs/admin_docs/configuration/mcp-server.mdx:984 — fixed in 272e4a61c091eddec8e47caa73fe820e1ffec0a2 - superset/mcp_service/guest_token_verifier.py:74 — not affected: transport already rejects missing expiry, wrong type, and verification/backend errors - superset/mcp_service/middleware.py:1282 — not affected: credential ValueError/PermissionError already hides all tools - superset/mcp_service/composite_token_verifier.py:266 — not affected: delegates guest transport admission; other sources are still checked at resolution - superset/security/manager.py:6336 — not affected: per-dashboard lookup semantics are unchanged and intentionally uncached - superset/mcp_service/worker.py:366 — not affected: production deferred import and worker-context lifecycle are unchanged - superset/mcp_service/mcp_config.py:961 — not affected: default resolver already uses the unqualification helper -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
