feat: 凭证失效时自动禁用 #1250
feat: 凭证失效时自动禁用 #1250
Conversation
Introduce a flexible auto-delete mechanism that removes OAuth credential files based on configurable HTTP status codes per provider. - Add AutoDeleteConfig with per-provider rules and customizable status codes - Implement TOCTOU-safe deletion with decisionTime validation - Delete memory state before file to ensure consistency on partial failure - Add config.example.yaml template for auto-delete settings
Add independent auto-delete configuration for the /api-call endpoint, allowing management API to automatically delete OAuth credentials when receiving specific HTTP status codes (default 401/403). - Add APICallAutoDelete config field reusing AutoDeleteConfig type - Add tryAPICallAutoDelete() delegating to Manager.RequestAutoDelete() - Add RequestAutoDelete() public method with proper nil checks - Export DefaultAutoDeleteStatusCodes constant for reuse - Fix debug log redundancy in shouldAutoDeleteCredential()
FileSynthesizer.Synthesize() was creating Auth objects without setting the FileName field. This caused api-call-auto-delete to fail because tryAPICallAutoDelete checks auth.FileName before proceeding. The watcher's snapshotCoreAuths() uses FileSynthesizer to create Auth objects, which then overwrites Manager.auths via AuthUpdate mechanism, replacing correctly-loaded auths (with FileName) with synthesized auths (without FileName). Fix: Set FileName=ID for primary auths and FileName=primary.FileName for Gemini virtual auths.
Merge api-call-auto-delete into auto-delete config with scope control:
- Add 'proxy' field (default: true) for proxy request auto-delete
- Add 'management' field (default: false) for management API auto-delete
- Remove separate APICallAutoDelete config field
This provides a cleaner, more intuitive configuration:
auto-delete:
providers:
codex:
enabled: true
proxy: true # during proxy requests (default)
management: true # during management API calls
Summary of ChangesHello @xycld, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求引入了一项关键功能，旨在提高系统的健壮性和自动化管理能力。通过在检测到 OAuth 凭证失效（例如，收到 401 或 403 HTTP 错误码）时自动禁用这些凭证，系统可以避免持续使用无效凭证进行请求，从而减少错误并优化资源使用。这一改进通过灵活的配置选项，允许管理员根据具体需求为不同服务提供商定制自动禁用策略，确保了在凭证出现问题时能够及时响应并采取纠正措施。 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| if h == nil || h.cfg == nil || h.authManager == nil { | ||
| return | ||
| } | ||
| if auth == nil || auth.FileName == "" { |
There was a problem hiding this comment.
在 tryAPICallAutoDisable 函数中，检查 auth.FileName == "" 可能会限制自动禁用功能仅适用于基于文件的 OAuth 凭证。如果存在非文件支持的 OAuth 凭证（例如，通过其他方式注入的运行时凭证），并且它们也需要自动禁用，则此检查可能会阻止它们被处理。请确认这是否是预期行为，即只有文件支持的凭证才应被自动禁用。如果需要支持其他类型的凭证，可能需要调整此条件。
| if err := m.persist(ctx, auth); err != nil { | ||
| log.WithFields(log.Fields{ | ||
| "auth_id": auth.ID, | ||
| "error": err, | ||
| }).Error("failed to persist auth state") | ||
| } |
There was a problem hiding this comment.
将 m.persist(ctx, auth) 的错误处理从 _ = 更改为显式日志记录是一个很好的改进。这提高了代码的可观察性和可维护性，有助于在持久化认证状态失败时快速识别问题。
| if err := m.persist(ctx, auth); err != nil { | |
| log.WithFields(log.Fields{ | |
| "auth_id": auth.ID, | |
| "error": err, | |
| }).Error("failed to persist auth state") | |
| } | |
| if err := m.persist(ctx, auth); err != nil { | |
| log.WithFields(log.Fields{ | |
| "auth_id": auth.ID, | |
| "error": err, | |
| }).Error("failed to persist auth state") | |
| } |
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic disabling of OAuth credentials when they receive HTTP 401, 402, or 403 status codes, indicating permanent authentication or authorization failures. The feature is configuration-driven and allows separate control for proxy requests and management API calls.
Changes:
- Added auto-disable configuration structure with provider-specific rules and separate controls for proxy vs management API paths
- Implemented auto-disable logic in the proxy request path (MarkResult function) and management API path
- Added SuspendAllClientModels function to suspend all models for a credential when it's auto-disabled
- Set FileName field for file-based OAuth credentials to enable file-based credential tracking
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/cliproxy/auth/conductor.go | Implements core auto-disable logic including condition checking, execution in MarkResult for proxy requests, and RequestAutoDisable/executeAutoDisable for management API |
| internal/api/handlers/management/api_tools.go | Adds tryAPICallAutoDisable to handle auto-disable for management API calls |
| internal/registry/model_registry.go | Implements SuspendAllClientModels to suspend all models for a disabled credential |
| internal/config/config.go | Defines AutoDisableConfig and AutoDisableRule structures with validation |
| config.example.yaml | Documents auto-disable configuration options with examples |
| internal/watcher/synthesizer/file.go | Sets FileName field for file-based and virtual auth credentials |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auth.Disabled = true | ||
| auth.Status = StatusDisabled | ||
| auth.StatusMessage = fmt.Sprintf("auto-disabled: HTTP %d", statusCode) | ||
| } |
There was a problem hiding this comment.
The auth.Status is set to StatusDisabled here when auto-disable conditions are met, but it's unconditionally overwritten to StatusError at line 1218, effectively making the auto-disable status change ineffective. This means the credential will appear as having an error rather than being disabled. To fix this, the code should only set auth.Status to StatusError if the credential was not auto-disabled, or move the auto-disable logic after line 1219 but before line 1220.
| autoDisableStatusCode = statusCode | ||
| auth.Disabled = true | ||
| auth.Status = StatusDisabled | ||
| auth.StatusMessage = fmt.Sprintf("auto-disabled: HTTP %d", statusCode) |
There was a problem hiding this comment.
When auto-disabling the credential, the auth.UpdatedAt field should be set to 'now' to maintain consistency with other state updates in this function. Currently, UpdatedAt is only set at line 1219, which happens after the auto-disable logic. Consider setting auth.UpdatedAt = now here along with the other auto-disable state changes, or ensure the code path continues to line 1219 where UpdatedAt is set.
| auth.StatusMessage = fmt.Sprintf("auto-disabled: HTTP %d", statusCode) | |
| auth.StatusMessage = fmt.Sprintf("auto-disabled: HTTP %d", statusCode) | |
| auth.UpdatedAt = now |
| rule, ok := h.cfg.AutoDisable.Providers[strings.ToLower(auth.Provider)] | ||
| if !ok || !rule.Enabled || !rule.Management { | ||
| return | ||
| } | ||
|
|
||
| statusCodes := rule.StatusCodes | ||
| if len(statusCodes) == 0 { | ||
| statusCodes = coreauth.DefaultAutoDisableStatusCodes | ||
| } | ||
|
|
||
| if !slices.Contains(statusCodes, statusCode) { | ||
| return | ||
| } |
There was a problem hiding this comment.
There's significant code duplication between this function and checkAutoDisableConditionInternal in conductor.go (lines 1456-1476). Both functions check the provider configuration and status codes in nearly identical ways. Consider refactoring to eliminate this duplication - for example, tryAPICallAutoDisable could call checkAutoDisableConditionInternal with forManagement=true to determine whether to proceed with the auto-disable.
| if m.checkAutoDisableCondition(auth, statusCode) { | ||
| shouldAutoDisable = true | ||
| autoDisableAuthID = auth.ID | ||
| autoDisableStatusCode = statusCode | ||
| auth.Disabled = true | ||
| auth.Status = StatusDisabled | ||
| auth.StatusMessage = fmt.Sprintf("auto-disabled: HTTP %d", statusCode) | ||
| } |
There was a problem hiding this comment.
The new auto-disable functionality lacks test coverage. Given that this feature automatically disables OAuth credentials based on HTTP error codes, tests should be added to verify: (1) auto-disable triggers correctly for configured status codes and providers, (2) auto-disable respects the proxy vs management configuration flags, (3) already-disabled credentials are not processed again, (4) the status and state changes are persisted correctly, (5) the integration with SuspendAllClientModels works as expected. The codebase has comprehensive test coverage for other features (as seen in files like api_key_model_alias_test.go, oauth_model_alias_test.go, selector_test.go), and this critical feature should follow the same pattern.
| } else { | ||
| if rule.Proxy != nil && !*rule.Proxy { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The default behavior for the Proxy field relies on implicit nil handling. When Proxy is nil (not configured), auto-disable is enabled by default. While this works correctly, consider adding an explicit comment explaining this default-true behavior for better code clarity, or refactoring to make the default more explicit, such as: 'if rule.Proxy == nil || *rule.Proxy' to check if proxy auto-disable should be enabled.
- Rename AutoDeleteConfig -> AutoDisableConfig, auto-delete -> auto-disable - Change behavior from deleting credentials to disabling them (Disabled=true, Status=StatusDisabled) - Add SuspendAllClientModels to registry for suspending all models of a client - Persist disabled state to disk instead of deleting credential files - Add proper error logging for persist operations - Merge duplicate case branches (401/402/403) and check functions (DRY) BREAKING CHANGE: config field renamed from auto-delete to auto-disable
xycld
force-pushed
the
feature/auto-delete-management-api
branch
from
4ddf27a to
fea80c6
Compare
|
没有对该功能的支持计划 |
2 participants
需求
将 OAuth 凭证添加自动禁用机制。当收到 401/403 等错误时，将其标记为禁用状态。
配置变更
在配置中新增以下字段：
proxy: 控制代理请求的自动删除，默认开启management: 控制管理 API 的自动删除，默认关闭，需要显式开启此功能在配置样例默认注释，没有此配置时默认关闭