gabotorresruiz commented on code in PR #45403:
URL: https://github.com/apache/superset/pull/45403#discussion_r4318503037
##########
superset/mcp_service/mcp_config.py:
##########
@@ -251,10 +251,10 @@ class MCPAuthConfigError(ValueError):
MCP_API_KEY_ENABLED: bool | None = None
# URL surfaced to users when an API key is rejected, pointing them at the
-# place to create or rotate a key. Defaults to the FAB user profile page;
-# deployments that manage keys elsewhere can override this to point at their
-# own key-management UI without forking the auth code.
-MCP_API_KEY_CREATE_URL = "/profile/"
+# place to create or rotate a key. Defaults to the user info page, where API
+# keys are managed; deployments that manage keys elsewhere can override this to
+# point at their own key-management UI without forking the auth code.
+MCP_API_KEY_CREATE_URL = "/user_info/"
Review Comment:
Not a blocker: I think this constant is inert today. `get_mcp_config()`
further down doesn't list `MCP_API_KEY_CREATE_URL` in its `defaults` dict, and
that call (`flask_singleton.py:91`) is the only thing that copies this module's
values into `app.config`, so the `auth.py` fallback is the only live default.
Could `auth.py` import it instead, so the two can't drift?
```python
from superset.mcp_service.mcp_config import (
MCP_API_KEY_CREATE_URL,
...
)
create_url = app.config.get("MCP_API_KEY_CREATE_URL", MCP_API_KEY_CREATE_URL)
```
##########
superset/mcp_service/auth.py:
##########
@@ -766,7 +766,7 @@ def _validate_api_key_fallback(app: Any, api_key_string:
str | None) -> User:
user = sm.validate_api_key(api_key_string)
if not user:
- create_url = app.config.get("MCP_API_KEY_CREATE_URL", "/profile/")
+ create_url = app.config.get("MCP_API_KEY_CREATE_URL", "/user_info/")
raise PermissionError(
f"Invalid or expired API key. Create a new key at {create_url}."
Review Comment:
Not a blocker, and possibly out of scope for a default change, but does this
string ever reach a client? `GlobalErrorHandlerMiddleware._handle_error` takes
the generic `PermissionError` branch and rewrites it to `Permission denied for
<tool>: You don't have access to this resource.`, and
`_sanitize_error_for_logging` flattens it to `Access denied`, so the URL
survives in neither the response nor the log. I drove that path on this branch
and the caller only sees the generic text.
This fallback also only runs when `CompositeTokenVerifier` was built without
a Flask app; with one, an invalid key is already rejected at the transport
layer with a generic 401. Worth a follow-up to raise something the middleware
passes through, so the link actually lands?
--
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]
