codeant-ai-for-open-source[bot] commented on code in PR #40861:
URL: https://github.com/apache/superset/pull/40861#discussion_r3375070571
##########
superset/mcp_service/auth.py:
##########
@@ -369,14 +371,11 @@ def get_user_from_request() -> User:
"MCP_DEV_USERNAME is not configured",
"g.user was not set by external middleware",
]
- configured_prefixes = current_app.config.get("FAB_API_KEY_PREFIXES",
["sst_"])
- prefix_example = configured_prefixes[0] if configured_prefixes else "sst_"
- raise ValueError(
- "No authenticated user found. Tried:\n"
- + "\n".join(f" - {d}" for d in details)
- + f"\n\nEither pass a valid API key (Bearer {prefix_example}...), "
- "JWT token, or configure MCP_DEV_USERNAME for development."
+ logger.warning(
+ "MCP request could not be authenticated. Tried: %s",
+ "; ".join(details),
)
+ raise ValueError("Authentication required. No valid credentials provided.")
Review Comment:
**Suggestion:** Replacing the unauthenticated `ValueError` text with a
completely new string breaks existing consumers that match on the previous
message (for example
`tests/unit_tests/mcp_service/test_auth_user_resolution.py::test_raises_when_no_auth_source`
still expects `"No authenticated user found"`). Keep backward compatibility by
preserving a stable error identifier/message contract (or update all dependent
call sites/tests in the same PR). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP auth user resolution unit test currently fails.
- ⚠️ CI runs including this test will be blocked.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From the PR branch, run `pytest
tests/unit_tests/mcp_service/test_auth_user_resolution.py::test_raises_when_no_auth_source`.
2. The test `test_raises_when_no_auth_source` at
`tests/unit_tests/mcp_service/test_auth_user_resolution.py:217-234` enters
an app context,
ensures `MCP_DEV_USERNAME` is unset and `g.user` is cleared, and patches
`fastmcp.server.dependencies.get_access_token` to return `None`.
3. Inside the innermost context manager, the test calls
`get_user_from_request()` imported
from `superset.mcp_service.auth` (see definition at
`superset/mcp_service/auth.py:312`),
which reaches the unauthenticated path and executes `raise
ValueError("Authentication
required. No valid credentials provided.")` at
`superset/mcp_service/auth.py:378`.
4. The test currently asserts `with pytest.raises(ValueError, match="No
authenticated user
found")` at `tests/unit_tests/mcp_service/test_auth_user_resolution.py:232`,
so the regex
does not match the new message string, causing the assertion to fail and the
test (and any
CI run including it) to break; no production callers were found that depend
on the old
message, only this test.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=68a74b223f4044038f495df491b54b2a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=68a74b223f4044038f495df491b54b2a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 378:378
**Comment:**
*Api Mismatch: Replacing the unauthenticated `ValueError` text with a
completely new string breaks existing consumers that match on the previous
message (for example
`tests/unit_tests/mcp_service/test_auth_user_resolution.py::test_raises_when_no_auth_source`
still expects `"No authenticated user found"`). Keep backward compatibility by
preserving a stable error identifier/message contract (or update all dependent
call sites/tests in the same PR).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40861&comment_hash=d6d1cc5e4a1233ec9d2cbbbf20f3104ed8b169858c51711932845c69c2b65326&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40861&comment_hash=d6d1cc5e4a1233ec9d2cbbbf20f3104ed8b169858c51711932845c69c2b65326&reaction=dislike'>👎</a>
--
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]
