feat: add OAuth 2.0 support for MCP#7720
Conversation
- Implemented OAuth 2.0 authorization flow for MCP servers, allowing users to log in directly through the WebUI. - Added new API endpoints for starting OAuth authorization, checking flow status, and handling callbacks. - Updated the MCP server configuration to include OAuth 2.0 settings, supporting both authorization code and client credentials grant types. - Enhanced the dashboard UI to display OAuth status and provide a QR code for mobile login. - Updated localization files to include new strings related to OAuth functionality. - Added documentation for configuring OAuth 2.0 for MCP servers.
dosubot
Bot
added
the
size:XXL
dosubot
Bot
added
area:provider
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The MCP WebUI changes introduce several hard-coded Chinese strings (e.g., QR code dialog title/button text and copy success/error messages) instead of using the existing i18n translation mechanism; consider moving these to the locale JSON files and using
tm/tfor consistency. - In multiple backend places you now have to manually strip the same set of MCP server meta-fields (
oauth2_enabled,oauth2_authorized,oauth2_grant_type, etc.); consider centralizing this list in a helper function/constant to avoid divergence and make future changes safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The MCP WebUI changes introduce several hard-coded Chinese strings (e.g., QR code dialog title/button text and copy success/error messages) instead of using the existing i18n translation mechanism; consider moving these to the locale JSON files and using `tm`/`t` for consistency.
- In multiple backend places you now have to manually strip the same set of MCP server meta-fields (`oauth2_enabled`, `oauth2_authorized`, `oauth2_grant_type`, etc.); consider centralizing this list in a helper function/constant to avoid divergence and make future changes safer.
## Individual Comments
### Comment 1
<location path="dashboard/src/components/extension/McpServersSection.vue" line_range="158-167" />
<code_context>
+ </div>
+
+ <div v-if="oauthUrl && oauthFlowStatus === 'awaiting_user'" class="mt-2 pa-3 bg-grey-lighten-4 rounded border">
+ <div class="text-caption text-grey-darken-1 mb-1">请复制以下链接在浏览器中打开：</div>
+ <div class="d-flex align-center">
+ <v-text-field
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use i18n translation keys instead of hardcoded Chinese strings in the new OAuth UI.
The existing labels in this component already use `tm(...)`, but the new OAuth texts (copy instructions, QR dialog title/description, and the copy success/error messages in `copyOauthUrl`) are hardcoded in Chinese. This will appear untranslated in other locales and complicates i18n maintenance. Please move these strings into the i18n bundles and use `tm(...)` consistently, including inside `copyOauthUrl()`.
Suggested implementation:
```
<div v-if="oauthUrl && oauthFlowStatus === 'awaiting_user'" class="mt-2 pa-3 bg-grey-lighten-4 rounded border">
<div class="text-caption text-grey-darken-1 mb-1">{{ tm('mcpServers.oauth.copyInstruction') }}</div>
<div class="d-flex align-center">
```
1. Update the `copyOauthUrl()` method in this same component so that any success/error messages currently hardcoded in Chinese are replaced with `tm(...)` calls, for example:
- `this.$toast.success(tm('mcpServers.oauth.copySuccess'))`
- `this.$toast.error(tm('mcpServers.oauth.copyError'))`
(Adjust for your actual notification mechanism and existing style in the file.)
2. For the QR dialog title and description that are currently hardcoded in Chinese, wrap their texts with `tm(...)` as well, using appropriately named keys, e.g.:
- `{{ tm('mcpServers.oauth.qrDialogTitle') }}`
- `{{ tm('mcpServers.oauth.qrDialogDescription') }}`
3. Add the new i18n keys to all locale bundles used by the dashboard, for example (Chinese shown here; add translations for other locales as needed):
- `mcpServers.oauth.copyInstruction`: `"请复制以下链接在浏览器中打开："`
- `mcpServers.oauth.copySuccess`: `"链接已复制到剪贴板"`
- `mcpServers.oauth.copyError`: `"复制链接失败，请手动复制"`
- `mcpServers.oauth.qrDialogTitle`: `"使用扫码完成授权"`
- `mcpServers.oauth.qrDialogDescription`: `"请使用授权设备扫描二维码完成 OAuth 授权流程。"`
4. Ensure `tm` is already available in the `<script setup>` (or equivalent) of `McpServersSection.vue`. If not, import it following the existing i18n convention used elsewhere in this component.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/mcp_oauth.py" line_range="73" />
<code_context>
+ client_metadata_url: str | None = None
+
+
+def _prepare_config(config: Mapping[str, Any]) -> dict[str, Any]:
+ prepared = dict(config)
+ if prepared.get("mcpServers"):
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing MCP config normalization into a single helper and simplifying MCPOAuthManager’s flow tracking to one mapping keyed by either flow_id or state to reduce moving parts.
You can keep all behavior but trim some of the moving parts with two small refactors:
---
### 1. Normalize MCP config once
Right now `_prepare_config`, `get_mcp_oauth_config`, `_get_storage_fingerprint`, `get_mcp_oauth_storage_path`, `create_mcp_http_auth`, `_probe_http_oauth_connection`, `MCPOAuthManager.start_authorization`, `get_mcp_oauth_state` each partially re‑implement “unwrap `mcpServers`, drop `active`, etc.” This makes it harder to reason about what “the config” actually is.
You can centralize this into a single normalizer and use it everywhere:
```python
def normalize_mcp_config(config: Mapping[str, Any]) -> dict[str, Any]:
"""Return a single, normalized MCP server config.
- Picks the first entry under 'mcpServers' if present
- Removes UI-only / volatile keys like 'active'
"""
cfg = dict(config)
if cfg.get("mcpServers"):
first_key = next(iter(cfg["mcpServers"]))
cfg = dict(cfg["mcpServers"][first_key])
cfg.pop("active", None)
return cfg
```
Then replace usages like:
```python
def _prepare_config(config: Mapping[str, Any]) -> dict[str, Any]:
prepared = dict(config)
if prepared.get("mcpServers"):
first_key = next(iter(prepared["mcpServers"]))
prepared = dict(prepared["mcpServers"][first_key])
prepared.pop("active", None)
return prepared
```
with:
```python
def get_mcp_oauth_config(config: Mapping[str, Any]) -> MCPOAuthConfig | None:
prepared = normalize_mcp_config(config)
oauth_config = prepared.get("oauth2") or prepared.get("oauth")
if not isinstance(oauth_config, dict):
return None
return MCPOAuthConfig.model_validate(oauth_config)
```
and similarly in:
- `_get_storage_fingerprint`
- `get_mcp_oauth_storage_path`
- `create_mcp_http_auth`
- `get_mcp_oauth_state`
- `_probe_http_oauth_connection`
- `MCPOAuthManager.start_authorization`
This removes duplicated implicit assumptions and makes it clear that all OAuth helpers operate on the same normalized shape.
---
### 2. Use a single mapping keyed by state (or flow_id), not both
`MCPOAuthManager` keeps both:
- `self._flows: dict[str, MCPOAuthPendingFlow]` keyed by `flow_id`
- `self._state_to_flow_id: dict[str, str]` keyed by `oauth_state`
For the current use (resolving a flow from callback), you can simplify by making the flow ID be the OAuth state once you know it, or by indexing flows directly by state.
Minimal change that keeps behavior: keep `_flows` but store under both keys, and drop `_state_to_flow_id`:
```python
class MCPOAuthManager:
_FLOW_TTL_SECONDS = 900
def __init__(self) -> None:
# map from identifier (flow_id or oauth_state) -> flow
self._flows: dict[str, MCPOAuthPendingFlow] = {}
self._lock = asyncio.Lock()
```
When you first create the flow:
```python
async with self._lock:
self._flows[flow_id] = flow
```
And when the OAuth `state` becomes known:
```python
if flow.oauth_state:
async with self._lock:
# store the same flow under its oauth_state as an alias
self._flows[flow.oauth_state] = flow
```
`submit_callback` becomes:
```python
async def submit_callback(
self,
flow_id: str | None = None,
*,
code: str | None,
state: str | None,
error: str | None,
) -> None:
key = flow_id or state
if not key:
raise KeyError("Missing flow_id and state")
async with self._lock:
flow = self._flows.get(key)
if flow is None:
raise KeyError(key)
flow.submit_callback(code=code, state=state, error=error)
```
And `_prune_flows` just needs to drop all aliases pointing to expired flows:
```python
async def _prune_flows(self) -> None:
threshold = time.time() - self._FLOW_TTL_SECONDS
async with self._lock:
expired_ids = {fid for fid, f in self._flows.items() if f.created_at < threshold}
self._flows = {k: f for k, f in self._flows.items() if f.flow_id not in expired_ids}
```
This keeps your existing behavior (you can still call `submit_callback` with either `flow_id` or `state`), but removes the dual `flow_id → flow` and `state → flow_id` structures and associated bookkeeping, making it easier to follow and reason about callback resolution and cleanup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <div class="text-caption text-grey-darken-1 mb-1">请复制以下链接在浏览器中打开：</div> | ||
| <div class="d-flex align-center"> | ||
| <v-text-field | ||
| :value="oauthUrl" | ||
| readonly | ||
| density="compact" | ||
| variant="plain" | ||
| hide-details | ||
| class="text-body-2" | ||
| ></v-text-field> |
There was a problem hiding this comment.
suggestion (bug_risk): Use i18n translation keys instead of hardcoded Chinese strings in the new OAuth UI.
The existing labels in this component already use tm(...), but the new OAuth texts (copy instructions, QR dialog title/description, and the copy success/error messages in copyOauthUrl) are hardcoded in Chinese. This will appear untranslated in other locales and complicates i18n maintenance. Please move these strings into the i18n bundles and use tm(...) consistently, including inside copyOauthUrl().
Suggested implementation:
<div v-if="oauthUrl && oauthFlowStatus === 'awaiting_user'" class="mt-2 pa-3 bg-grey-lighten-4 rounded border">
<div class="text-caption text-grey-darken-1 mb-1">{{ tm('mcpServers.oauth.copyInstruction') }}</div>
<div class="d-flex align-center">
- Update the
copyOauthUrl()method in this same component so that any success/error messages currently hardcoded in Chinese are replaced withtm(...)calls, for example:this.$toast.success(tm('mcpServers.oauth.copySuccess'))this.$toast.error(tm('mcpServers.oauth.copyError'))
(Adjust for your actual notification mechanism and existing style in the file.)
- For the QR dialog title and description that are currently hardcoded in Chinese, wrap their texts with
tm(...)as well, using appropriately named keys, e.g.:{{ tm('mcpServers.oauth.qrDialogTitle') }}{{ tm('mcpServers.oauth.qrDialogDescription') }}
- Add the new i18n keys to all locale bundles used by the dashboard, for example (Chinese shown here; add translations for other locales as needed):
mcpServers.oauth.copyInstruction:"请复制以下链接在浏览器中打开："mcpServers.oauth.copySuccess:"链接已复制到剪贴板"mcpServers.oauth.copyError:"复制链接失败，请手动复制"mcpServers.oauth.qrDialogTitle:"使用扫码完成授权"mcpServers.oauth.qrDialogDescription:"请使用授权设备扫描二维码完成 OAuth 授权流程。"
- Ensure
tmis already available in the<script setup>(or equivalent) ofMcpServersSection.vue. If not, import it following the existing i18n convention used elsewhere in this component.
| client_metadata_url: str | None = None | ||
|
|
||
|
|
||
| def _prepare_config(config: Mapping[str, Any]) -> dict[str, Any]: |
There was a problem hiding this comment.
issue (complexity): Consider centralizing MCP config normalization into a single helper and simplifying MCPOAuthManager’s flow tracking to one mapping keyed by either flow_id or state to reduce moving parts.
You can keep all behavior but trim some of the moving parts with two small refactors:
1. Normalize MCP config once
Right now _prepare_config, get_mcp_oauth_config, _get_storage_fingerprint, get_mcp_oauth_storage_path, create_mcp_http_auth, _probe_http_oauth_connection, MCPOAuthManager.start_authorization, get_mcp_oauth_state each partially re‑implement “unwrap mcpServers, drop active, etc.” This makes it harder to reason about what “the config” actually is.
You can centralize this into a single normalizer and use it everywhere:
def normalize_mcp_config(config: Mapping[str, Any]) -> dict[str, Any]:
"""Return a single, normalized MCP server config.
- Picks the first entry under 'mcpServers' if present
- Removes UI-only / volatile keys like 'active'
"""
cfg = dict(config)
if cfg.get("mcpServers"):
first_key = next(iter(cfg["mcpServers"]))
cfg = dict(cfg["mcpServers"][first_key])
cfg.pop("active", None)
return cfgThen replace usages like:
def _prepare_config(config: Mapping[str, Any]) -> dict[str, Any]:
prepared = dict(config)
if prepared.get("mcpServers"):
first_key = next(iter(prepared["mcpServers"]))
prepared = dict(prepared["mcpServers"][first_key])
prepared.pop("active", None)
return preparedwith:
def get_mcp_oauth_config(config: Mapping[str, Any]) -> MCPOAuthConfig | None:
prepared = normalize_mcp_config(config)
oauth_config = prepared.get("oauth2") or prepared.get("oauth")
if not isinstance(oauth_config, dict):
return None
return MCPOAuthConfig.model_validate(oauth_config)and similarly in:
_get_storage_fingerprintget_mcp_oauth_storage_pathcreate_mcp_http_authget_mcp_oauth_state_probe_http_oauth_connectionMCPOAuthManager.start_authorization
This removes duplicated implicit assumptions and makes it clear that all OAuth helpers operate on the same normalized shape.
2. Use a single mapping keyed by state (or flow_id), not both
MCPOAuthManager keeps both:
self._flows: dict[str, MCPOAuthPendingFlow]keyed byflow_idself._state_to_flow_id: dict[str, str]keyed byoauth_state
For the current use (resolving a flow from callback), you can simplify by making the flow ID be the OAuth state once you know it, or by indexing flows directly by state.
Minimal change that keeps behavior: keep _flows but store under both keys, and drop _state_to_flow_id:
class MCPOAuthManager:
_FLOW_TTL_SECONDS = 900
def __init__(self) -> None:
# map from identifier (flow_id or oauth_state) -> flow
self._flows: dict[str, MCPOAuthPendingFlow] = {}
self._lock = asyncio.Lock()When you first create the flow:
async with self._lock:
self._flows[flow_id] = flowAnd when the OAuth state becomes known:
if flow.oauth_state:
async with self._lock:
# store the same flow under its oauth_state as an alias
self._flows[flow.oauth_state] = flowsubmit_callback becomes:
async def submit_callback(
self,
flow_id: str | None = None,
*,
code: str | None,
state: str | None,
error: str | None,
) -> None:
key = flow_id or state
if not key:
raise KeyError("Missing flow_id and state")
async with self._lock:
flow = self._flows.get(key)
if flow is None:
raise KeyError(key)
flow.submit_callback(code=code, state=state, error=error)And _prune_flows just needs to drop all aliases pointing to expired flows:
async def _prune_flows(self) -> None:
threshold = time.time() - self._FLOW_TTL_SECONDS
async with self._lock:
expired_ids = {fid for fid, f in self._flows.items() if f.created_at < threshold}
self._flows = {k: f for k, f in self._flows.items() if f.flow_id not in expired_ids}This keeps your existing behavior (you can still call submit_callback with either flow_id or state), but removes the dual flow_id → flow and state → flow_id structures and associated bookkeeping, making it easier to follow and reason about callback resolution and cleanup.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OAuth 2.0 support for Model Context Protocol (MCP) servers, enabling both authorization_code and client_credentials grant types. Key changes include the addition of a dedicated mcp_oauth.py module for flow management and token storage, integration with the MCP client and tool manager, and WebUI updates to support interactive login via browser or QR code. Review feedback highlights opportunities to prevent resource leaks by ensuring background tasks are cancelled during timeouts or flow pruning, suggests refactoring duplicated configuration logic into shared helpers, and recommends adding unit tests for the new core authentication components.
| expired_ids = [ | ||
| flow_id | ||
| for flow_id, flow in self._flows.items() | ||
| if flow.created_at < threshold | ||
| ] | ||
| for flow_id in expired_ids: | ||
| expired_states = [ | ||
| state | ||
| for state, state_flow_id in self._state_to_flow_id.items() | ||
| if state_flow_id == flow_id | ||
| ] | ||
| for state in expired_states: | ||
| self._state_to_flow_id.pop(state, None) | ||
| self._flows.pop(flow_id, None) |
There was a problem hiding this comment.
在 _prune_flows 中清理过期的 flow 时，应当同时取消关联的 asyncio.Task。由于 _run_flow 可能会因为等待用户授权回调而长时间挂起，如果不手动取消任务，这些任务会一直残留在内存中导致泄漏。另外，根据 Rule 1，此类不含 await 的同步代码块在单线程 asyncio 事件循环中是原子执行的，因此无需额外的 lock。
expired_ids = [
flow_id
for flow_id, flow in self._flows.items()
if flow.created_at < threshold
]
for flow_id in expired_ids:
flow = self._flows[flow_id]
if flow.task and not flow.task.done():
flow.task.cancel()
expired_states = [
state
for state, state_flow_id in self._state_to_flow_id.items()
if state_flow_id == flow_id
]
for state in expired_states:
self._state_to_flow_id.pop(state, None)
self._flows.pop(flow_id, None)References
- In a single-threaded asyncio event loop, synchronous functions (code blocks without 'await') are executed atomically and will not be interrupted by other coroutines. Therefore, they are safe from race conditions when modifying shared state within that block.
| if not done: | ||
| raise MCPOAuthError( | ||
| "Timed out while preparing the OAuth 2.0 authorization flow.", | ||
| ) |
There was a problem hiding this comment.
如果在 start_authorization 中等待初始化超时，应当取消已经启动的 flow.task。否则，即使外部调用已经因为超时而返回错误，后台任务仍会继续运行并可能在之后尝试进行网络请求或继续等待回调，造成不必要的资源占用。
| if not done: | |
| raise MCPOAuthError( | |
| "Timed out while preparing the OAuth 2.0 authorization flow.", | |
| ) | |
| if not done: | |
| if flow.task and not flow.task.done(): | |
| flow.task.cancel() | |
| raise MCPOAuthError( | |
| "Timed out while preparing the OAuth 2.0 authorization flow.", | |
| ) |
| def _prepare_config(config: Mapping[str, Any]) -> dict[str, Any]: | ||
| prepared = dict(config) | ||
| if prepared.get("mcpServers"): | ||
| first_key = next(iter(prepared["mcpServers"])) | ||
| prepared = dict(prepared["mcpServers"][first_key]) | ||
| prepared.pop("active", None) | ||
| return prepared |
There was a problem hiding this comment.
_prepare_config 函数的实现与 astrbot/core/agent/mcp_client.py 和 astrbot/core/provider/func_tool_manager.py 中的实现完全一致。建议将此类重复逻辑重构为共享的辅助函数（例如放在 astrbot/core/agent/mcp_client.py 中并导出），以提高代码的可维护性并减少冗余。
References
- When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
| @@ -0,0 +1,709 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
新增的 OAuth 2.0 支持逻辑涉及复杂的异步流程、状态管理和文件持久化。建议为这些核心逻辑编写单元测试，特别是针对 MCPOAuthManager 的流程管理和 MCPFileTokenStorage 的存储逻辑，以确保其在各种边界情况下的正确性。
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
There was a problem hiding this comment.
Pull request overview
Adds OAuth 2.0 authorization support for HTTP-based MCP servers, enabling AstrBot to complete MCP OAuth flows via the WebUI and persist tokens for subsequent connections.
Changes:
- Introduce a new MCP OAuth manager/token storage implementation and wire it into MCP HTTP connections.
- Add backend endpoints for starting/polling OAuth flows and handling OAuth callbacks.
- Update the dashboard MCP server UI, templates, and i18n/docs to expose OAuth login status and flows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/zh/use/mcp.md | Documents OAuth 2.0 MCP configuration and WebUI login steps (ZH). |
| docs/en/use/mcp.md | Documents OAuth 2.0 MCP configuration and WebUI login steps (EN). |
| dashboard/src/i18n/locales/zh-CN/features/tool-use.json | Adds OAuth-related UI strings (ZH). |
| dashboard/src/i18n/locales/en-US/features/tool-use.json | Adds OAuth-related UI strings (EN). |
| dashboard/src/i18n/locales/ru-RU/features/tool-use.json | Adds OAuth-related UI strings (RU). |
| dashboard/src/components/extension/McpServersSection.vue | Adds OAuth status display, OAuth template, OAuth login UX (URL + QR), and polling. |
| astrbot/dashboard/routes/tools.py | Adds OAuth start/status endpoints and a callback handler route; exposes OAuth state in server list responses. |
| astrbot/core/provider/func_tool_manager.py | Integrates OAuth manager APIs and adjusts MCP connection testing behavior for OAuth configs. |
| astrbot/core/agent/mcp_oauth.py | New OAuth 2.0 flow manager, token storage, and httpx auth creation for MCP HTTP transports. |
| astrbot/core/agent/mcp_client.py | Injects OAuth auth into SSE/Streamable HTTP MCP connections and gates quick-test behavior for OAuth configs. |
| <div class="text-caption text-grey-darken-1 mb-1">请复制以下链接在浏览器中打开：</div> | ||
| <div class="d-flex align-center"> | ||
| <v-text-field | ||
| :value="oauthUrl" | ||
| readonly | ||
| density="compact" | ||
| variant="plain" | ||
| hide-details | ||
| class="text-body-2" | ||
| ></v-text-field> | ||
| <v-btn icon="mdi-content-copy" size="x-small" variant="text" @click="copyOauthUrl"></v-btn> | ||
| </div> | ||
| <v-btn | ||
| block | ||
| color="info" | ||
| variant="tonal" | ||
| size="small" | ||
| class="mt-3" | ||
| prepend-icon="mdi-qrcode-scan" | ||
| @click="showQrCodeDialog = true" | ||
| > | ||
| 扫描二维码登录 | ||
| </v-btn> |
There was a problem hiding this comment.
The OAuth UI adds hard-coded Chinese strings (e.g., the “copy this link” instruction and “scan QR code” button label), bypassing the existing i18n pattern used throughout this component. Please move these strings into the i18n files and reference them via tm(...) so the feature is usable in en-US/ru-RU locales as well.
| <v-card-title class="text-center py-4">扫描二维码登录</v-card-title> | ||
| <v-card-text class="text-center pb-6"> | ||
| <QrCodeViewer | ||
| v-if="oauthUrl" | ||
| :value="oauthUrl" | ||
| :size="240" | ||
| class="mx-auto border rounded-lg" | ||
| /> | ||
| <div class="mt-4 text-caption text-grey">请使用移动端或浏览器扫描此二维码完成授权</div> | ||
| </v-card-text> | ||
| <v-card-actions> | ||
| <v-spacer></v-spacer> | ||
| <v-btn color="primary" block @click="showQrCodeDialog = false">关闭</v-btn> | ||
| </v-card-actions> |
There was a problem hiding this comment.
The QR code dialog UI uses hard-coded Chinese strings (title, hint text, and the close button label). Please internationalize these strings using the existing module i18n (tm(...)) and add the corresponding keys for all supported locales.
| const response = await axios.post('/api/tools/mcp/oauth/start', { | ||
| mcp_server_config: configObj, | ||
| callback_base_url: window.location.origin, | ||
| force: true | ||
| }, { | ||
| params: { name: serverName } | ||
| }); |
There was a problem hiding this comment.
authorizeOauth() always sends force: true, which clears any previously stored tokens on every login attempt and forces re-authorization even when a valid token is already present. Consider defaulting force to false and only enabling forced re-auth via an explicit user action (e.g., “Re-authorize” button).
| 3. 在新打开的授权页面完成登录。 | ||
| 4. 返回 AstrBot，等待状态显示授权完成后再测试连接或保存。 |
There was a problem hiding this comment.
文档步骤写的是“在新打开的授权页面完成登录”，但当前 WebUI 逻辑并不会自动打开新窗口/标签页，而是展示可复制链接与二维码供用户自行打开。请更新这里的步骤描述以匹配实际交互，或在点击“OAuth 2.0 登录”时实现自动打开授权链接。
| 3. 在新打开的授权页面完成登录。 | |
| 4. 返回 AstrBot，等待状态显示授权完成后再测试连接或保存。 | |
| 3. 根据界面展示的授权链接或二维码，自行打开授权页面并完成登录。 | |
| 4. 完成授权后，回到或继续查看 AstrBot 当前界面，等待状态显示授权完成后再测试连接或保存。 |
| async def _prune_flows(self) -> None: | ||
| threshold = time.time() - self._FLOW_TTL_SECONDS | ||
| async with self._lock: | ||
| expired_ids = [ | ||
| flow_id | ||
| for flow_id, flow in self._flows.items() | ||
| if flow.created_at < threshold | ||
| ] | ||
| for flow_id in expired_ids: | ||
| expired_states = [ | ||
| state | ||
| for state, state_flow_id in self._state_to_flow_id.items() | ||
| if state_flow_id == flow_id | ||
| ] | ||
| for state in expired_states: | ||
| self._state_to_flow_id.pop(state, None) | ||
| self._flows.pop(flow_id, None) | ||
|
|
There was a problem hiding this comment.
_prune_flows() removes expired flows from the manager, but it does not cancel the associated background task (flow.task). Since _run_flow() can wait indefinitely for the user callback, pruning can leave orphaned tasks running forever, causing a memory/task leak over time. When pruning, please cancel flow.task (if still pending) and mark the flow as failed/expired (set error, signal events) before removing it.
| resolved_flow_id = flow_id | ||
| if resolved_flow_id is None and state: | ||
| resolved_flow_id = self._state_to_flow_id.get(state) | ||
|
|
||
| async with self._lock: |
There was a problem hiding this comment.
submit_callback() reads _state_to_flow_id outside of the manager lock, while _prune_flows() and start_authorization() mutate it under lock. This can race under concurrent requests (callback arriving while a new flow starts/prunes), leading to hard-to-reproduce KeyError/missed callbacks. Please do the state->flow_id lookup under the same lock used for flow mutations.
| resolved_flow_id = flow_id | |
| if resolved_flow_id is None and state: | |
| resolved_flow_id = self._state_to_flow_id.get(state) | |
| async with self._lock: | |
| async with self._lock: | |
| resolved_flow_id = flow_id | |
| if resolved_flow_id is None and state: | |
| resolved_flow_id = self._state_to_flow_id.get(state) |
| async def get_mcp_oauth_state(config: Mapping[str, Any]) -> dict[str, Any]: | ||
| oauth_config = get_mcp_oauth_config(config) | ||
| if oauth_config is None: | ||
| return { | ||
| "oauth2_enabled": False, | ||
| "oauth2_authorized": False, | ||
| "oauth2_grant_type": None, | ||
| } | ||
|
|
||
| if oauth_config.grant_type == "client_credentials": | ||
| return { | ||
| "oauth2_enabled": True, | ||
| "oauth2_authorized": True, | ||
| "oauth2_grant_type": oauth_config.grant_type, | ||
| } | ||
|
|
||
| storage = MCPFileTokenStorage.from_mcp_config(config) | ||
| tokens = await storage.get_tokens() | ||
| return { | ||
| "oauth2_enabled": True, | ||
| "oauth2_authorized": tokens is not None, | ||
| "oauth2_grant_type": oauth_config.grant_type, | ||
| } |
There was a problem hiding this comment.
get_mcp_oauth_state() treats any stored token as “authorized” without checking expiry. This can make the server list show “authorized” even when the token is already expired and the next connection will raise MCPOAuthAuthorizationRequiredError. Please incorporate token_expires_at (and refresh-token availability, if relevant) into the authorization state so the UI reflects reality.
| async def start_authorization( | ||
| self, | ||
| config: Mapping[str, Any], | ||
| *, | ||
| callback_base_url: str, | ||
| server_name: str | None = None, | ||
| force: bool = False, | ||
| ) -> MCPOAuthPendingFlow: |
There was a problem hiding this comment.
start_authorization(..., server_name=...) accepts server_name but never uses it. This makes the API misleading for callers (routes/UI) that pass a name expecting it to matter. Either remove the parameter or include it in the flow status/logging so it has a clear purpose.
| this.showSuccess('链接已复制到剪贴板'); | ||
| }).catch(err => { | ||
| this.showError('复制失败: ' + err); |
There was a problem hiding this comment.
copyOauthUrl() shows hard-coded Chinese toast/error messages. Please switch these to i18n keys (similar to the other messages.* entries) so non-zh locales get localized feedback and to keep messaging consistent.
| this.showSuccess('链接已复制到剪贴板'); | |
| }).catch(err => { | |
| this.showError('复制失败: ' + err); | |
| this.showSuccess(this.tm('messages.copyToClipboardSuccess')); | |
| }).catch(err => { | |
| this.showError(this.tm('messages.copyToClipboardError', { error: String(err) })); |
| 3. Complete the login flow in the newly opened authorization page. | ||
| 4. Return to AstrBot, wait until authorization completes, then test or save the server. |
There was a problem hiding this comment.
The steps say the authorization page is “newly opened”, but the current WebUI flow does not open a new window/tab automatically (it shows a URL/QR for the user to open). Please update the instructions to match the actual UX, or implement opening the authorization URL when clicking “OAuth 2.0 Login”.
| 3. Complete the login flow in the newly opened authorization page. | |
| 4. Return to AstrBot, wait until authorization completes, then test or save the server. | |
| 3. AstrBot will display an authorization URL or QR code. Open the URL in your browser, or scan the QR code, to continue the login flow. | |
| 4. After completing authorization, return to AstrBot and wait for the authorization status to finish updating, then test or save the server. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee58197b95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not done: | ||
| raise MCPOAuthError( | ||
| "Timed out while preparing the OAuth 2.0 authorization flow.", | ||
| ) |
There was a problem hiding this comment.
When start_authorization times out waiting for url_ready_event/done_event, it raises immediately but leaves the previously created background task and flow entry running. In the slow-provider path (authorization URL takes >15s), the caller gets an error and no usable flow_id, yet _run_flow keeps executing and may wait for callback without any client being able to complete it, which accumulates orphaned in-memory flows/tasks under repeated retries.
Useful? React with 👍 / 👎.
| tokens = await storage.get_tokens() | ||
| return { | ||
| "oauth2_enabled": True, | ||
| "oauth2_authorized": tokens is not None, | ||
| "oauth2_grant_type": oauth_config.grant_type, |
There was a problem hiding this comment.
get_mcp_oauth_state reports oauth2_authorized purely based on token presence, but create_mcp_http_auth later rejects expired tokens without refresh support. This creates a user-visible false-positive authorized state (UI shows authorized, but enable/test immediately fails) whenever a stored token has expired, so status should incorporate expiry/refresh viability instead of only tokens is not None.
Useful? React with 👍 / 👎.
实现了 MCP 的 OAuth 2.0 登录规范
Modifications / 改动点
新增
astrbot/core/agent/mcp_oauth.py
修改了
astrbot/core/agent/mcp_client.py
astrbot/core/provider/func_tool_manager.py
astrbot/dashboard/routes/tools.py
dashboard/src/components/extension/McpServersSection.vue
实现了自动发现功能
登录提供可复制的 url 与 可扫描的二维码(但是结合实测，此功能似乎有些鸡肋)
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add OAuth 2.0 support for HTTP-based MCP servers, including backend authorization flow management and WebUI integration for login and status display.
New Features:
Enhancements:
Documentation: