Conversation
📝 Walkthrough概览此 PR 向多个 API 路由添加了 变更清单
代码审查工作量评估🎯 3 (Moderate) | ⏱️ ~25 分钟 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AptS-1547, 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! 此拉取请求旨在增强API的只读密钥功能，通过扩展其对多个用户数据相关端点的访问权限。这一改进允许持有只读密钥的用户检索自己的信息，从而提升了系统的可用性，同时严格维护了数据隔离。为确保扩展访问的安全性和正确性，本次实现包含了全面的测试。 Highlights
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}
There was a problem hiding this comment.
Code Review
这个 PR 旨在为多个 API 端点扩展只读密钥的访问权限，相关的测试文件也已相应更新。这是一个很好的功能增强。
然而，在审查中发现一个关键问题：实现此功能所需的核心逻辑变更似乎缺失了。根据 PR 描述，getUsers、getUserLimitUsage、getUserStatistics、getUsageLogs、getOverviewData 和 getActiveSessions 等端点应该允许只读密钥访问。这通常需要将这些 Action 中的 getSession() 调用修改为 getSession({ allowReadOnlyAccess: true })。但在所提供的代码文件中，这些 Action 并没有进行相应的修改。如果这些变更确实缺失，那么新添加和修改的测试将会失败，该功能也无法按预期工作。请确认相关 Action 文件是否已正确修改并包含在此 PR 中。
此外，在新的测试文件中发现了一些可以改进的地方，以提高代码质量和可维护性，具体请见代码行评论。
| async function createTestUser(name: string): Promise<TestUser> { | ||
| const [row] = await db | ||
| .insert(users) | ||
| .values({ name }) | ||
| .returning({ id: users.id, name: users.name }); | ||
|
|
||
| if (!row) { | ||
| throw new Error("Failed to create test user"); | ||
| } | ||
| return row; | ||
| } | ||
|
|
||
| async function createTestKey(params: { | ||
| userId: number; | ||
| key: string; | ||
| name: string; | ||
| canLoginWebUi: boolean; | ||
| }): Promise<TestKey> { | ||
| const [row] = await db | ||
| .insert(keys) | ||
| .values({ | ||
| userId: params.userId, | ||
| key: params.key, | ||
| name: params.name, | ||
| canLoginWebUi: params.canLoginWebUi, | ||
| dailyResetMode: "rolling", | ||
| dailyResetTime: "00:00", | ||
| }) | ||
| .returning({ id: keys.id, userId: keys.userId, key: keys.key, name: keys.name }); | ||
|
|
||
| if (!row) { | ||
| throw new Error("Failed to create test key"); | ||
| } | ||
| return row; | ||
| } |
There was a problem hiding this comment.
这两个辅助函数 createTestUser 和 createTestKey 与 tests/api/my-usage-readonly.test.ts 中的函数几乎完全相同。为了遵循 DRY (Don't Repeat Yourself) 原则并提高可维护性，建议将这些通用的测试辅助函数提取到一个共享文件中，例如 tests/test-utils.ts。
| return row; | ||
| } | ||
|
|
||
| describe("allowReadOnlyAccess endpoints (Issue #687)", () => { |
There was a problem hiding this comment.
这个测试套件中的许多测试用例都包含了重复的设置代码，用于创建测试用户和只读密钥。虽然每个测试用例都创建了带有特定名称的唯一实体，有助于调试，但这种重复性影响了代码的简洁性和可维护性。
可以考虑将这部分逻辑提取到一个辅助函数中，例如：
async function setupReadonlyUserAndKey(name: string) {
const unique = `${name}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
const user = await createTestUser(`Test ${unique}`);
createdUserIds.push(user.id);
const readonlyKey = await createTestKey({
userId: user.id,
key: `test-readonly-key-${unique}`,
name: `readonly-${unique}`,
canLoginWebUi: false,
});
createdKeyIds.push(readonlyKey.id);
currentAuthToken = readonlyKey.key;
return { user, readonlyKey };
}然后在每个测试用例中调用它：
test("readonly key can access getUsers endpoint", async () => {
const { user, readonlyKey } = await setupReadonlyUserAndKey("readonly-getusers");
// ... 测试逻辑
});这样既能保持测试的独立性和调试信息的清晰度，又能显著减少重复代码。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: M
- Lines changed: 392
- Files changed: 3
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Excellent (358 lines of new integration tests)
- Code clarity - Good
Analysis
This PR extends readonly API key access to 6 additional endpoints by adding allowReadOnlyAccess: true flags. After comprehensive review through 6 specialized perspectives:
Security & Access Control ✅
All affected endpoints already implement proper user-based access control in their business logic:
getUsers(src/actions/users.ts:184-190) - Non-admin users only see their own datagetUserLimitUsage(src/actions/users.ts:1268-1273) - Permission check prevents cross-user accessgetUserStatistics(src/actions/statistics.ts:54-94) - Properly scoped by user rolegetUsageLogs(src/actions/usage-logs.ts:51-53) - Forced userId filter for non-admingetOverviewData(src/actions/overview.ts:52-56) - Properly scoped based on permissionsgetActiveSessions(src/actions/active-sessions.ts:43-44) - Filtered by user permissions
Test Coverage ✅
- 358 lines of comprehensive integration tests verify all 6 endpoints
- Data isolation verified (tests/integration/readonly-access-endpoints.test.ts:255-289)
- Admin-only endpoint protection verified (tests/integration/readonly-access-endpoints.test.ts:291-313)
- Bearer token authentication tested
- Existing tests properly updated
Implementation ✅
- Minimal, focused changes following established pattern from PR #532
- No over-engineering or unnecessary complexity
- Consistent with 7 other endpoints already using this mechanism
- Solves real user problem (Issue #687)
Automated review by Claude AI
| summary: "获取用户列表", | ||
| tags: ["用户管理"], | ||
| allowReadOnlyAccess: true, | ||
| } |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] allowReadOnlyAccess is only applied in the adapter; these actions still call getSession() (default allowReadOnlyAccess=false), so readonly keys remain unauthenticated at the action layer.
Why this is a problem: The adapter validates with validateKey(authToken, { allowReadOnlyAccess }) (src/lib/api/action-adapter-openapi.ts:312), but the newly-whitelisted actions still do const session = await getSession(); (e.g. src/actions/users.ts:172, src/actions/users.ts:1257, src/actions/statistics.ts:39, src/actions/usage-logs.ts:46, src/actions/overview.ts:43, src/actions/active-sessions.ts:27). With a canLoginWebUi=false key, adapter auth will pass while the action returns { ok: false, error: "未登录"/"UNAUTHORIZED" } (=> HTTP 400 via src/lib/api/action-adapter-openapi.ts:377-391) or empty data, contradicting the new expectations in tests/api/my-usage-readonly.test.ts:208-223 and tests/integration/readonly-access-endpoints.test.ts:124-237.
Suggested fix (apply to each affected action):
// Example: src/actions/users.ts
const session = await getSession({ allowReadOnlyAccess: true });
if (!session) return [];
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(json).toMatchObject({ ok: false }); |
There was a problem hiding this comment.
[HIGH] [TEST-INCOMPLETE] This test expects HTTP 200 for an { ok: false } ActionResult, but the Actions adapter returns HTTP 400 when ok=false.
Why this is a problem: createActionRoute returns return c.json(..., 400) for result.ok === false (src/lib/api/action-adapter-openapi.ts:377-391), and this behavior is enforced by tests/api/action-adapter-openapi.unit.test.ts:106-127. getUserLimitUsage returns { ok: false, errorCode: "PERMISSION_DENIED" } on cross-user access (src/actions/users.ts:1267-1274), so this assertion should be 400 unless you change adapter semantics globally.
Suggested fix:
expect(response.status).toBe(400);
expect(json).toMatchObject({ ok: false, errorCode: "PERMISSION_DENIED" });
There was a problem hiding this comment.
Code Review Summary
This PR widens allowReadOnlyAccess to several Actions endpoints and adds tests, but the change currently only relaxes adapter-level auth. The underlying server actions still call getSession() without { allowReadOnlyAccess: true }, so readonly keys will still fail at the action layer and the new tests/behavior will not match.
PR Size: M
- Lines changed: 392
- Files changed: 3
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/app/api/actions/[...route]/route.ts:allowReadOnlyAccessis enabled for endpoints whose actions still usegetSession()(default denies readonly keys), leading to 400/empty responses and failing expectations.
High Priority Issues (Should Fix)
tests/integration/readonly-access-endpoints.test.ts: expects HTTP 200 for{ ok: false }even though the Actions adapter contract returns HTTP 400 forok=false.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Applied PR size label: size/M (392 lines changed across 3 files).
Posted 2 inline review comments + a summary review on PR #704:
- [CRITICAL] [LOGIC-BUG]
src/app/api/actions/[...route]/route.ts(diff hunk at+ allowReadOnlyAccess: true, PR comment position 5): adapter auth is relaxed, but the underlying actions still usegetSession()(defaultallowReadOnlyAccess=false), so readonly keys will still fail at the action layer (400/empty) and the new tests won’t match. - [HIGH] [TEST-INCOMPLETE]
tests/integration/readonly-access-endpoints.test.ts(line 302 in diff): test expects HTTP 200 for{ ok: false }, butcreateActionRoutereturns HTTP 400 whenok=false(src/lib/api/action-adapter-openapi.ts:377-391).
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary | 概述
This PR extends readonly API key access permissions to support more user data endpoints, fixing Issue #687 where regular users' API tokens were getting 401 errors.
本 PR 扩展只读 API 密钥访问权限，支持更多用户数据端点，修复了 Issue #687 中普通用户 API token 返回 401 错误的问题。
Problem | 问题描述
Issue: Regular users' API tokens (with
canLoginWebUi=false) were getting 401 authentication errors when calling user data endpoints like/api/actions/users/getUsers, even though:问题：普通用户的 API token（
canLoginWebUi=false）在调用用户数据端点（如/api/actions/users/getUsers）时返回 401 认证错误，即使：Related Issues:
allowReadOnlyAccessmechanism introduced in PR fix: 修复 my-usage 今日统计与只读 API 自助查询 #532Solution | 解决方案
Added
allowReadOnlyAccess: trueflag to 6 API endpoints insrc/app/api/actions/[...route]/route.ts:在
src/app/api/actions/[...route]/route.ts中为 6 个 API 端点添加allowReadOnlyAccess: true标志：How
allowReadOnlyAccessWorks | 工作原理When
allowReadOnlyAccess: trueis set:canLoginWebUi=falsecan authenticate successfully设置
allowReadOnlyAccess: true后：canLoginWebUi=false的密钥可以成功认证Changes | 变更内容
Core Changes | 核心变更
src/app/api/actions/[...route]/route.tsallowReadOnlyAccess: trueto 6 endpointsTest Changes | 测试变更
tests/api/my-usage-readonly.test.tstests/integration/readonly-access-endpoints.test.tsTest Coverage | 测试覆盖
New Integration Tests | 新增集成测试
The new
tests/integration/readonly-access-endpoints.test.tsfile provides comprehensive coverage:新增的
tests/integration/readonly-access-endpoints.test.ts文件提供全面覆盖：Updated Existing Tests | 更新现有测试
Updated
tests/api/my-usage-readonly.test.ts:Security Considerations | 安全考虑
Data Isolation Verified:
tests/integration/readonly-access-endpoints.test.ts:255-289)listSensitiveWords) remain protected (verified in tests)数据隔离已验证：
tests/integration/readonly-access-endpoints.test.ts:255-289中验证）listSensitiveWords）仍然受保护（在测试中验证）Breaking Changes | 破坏性变更
None. This is a backwards-compatible enhancement that allows more access, not less.
无破坏性变更。 这是一个向后兼容的增强，允许更多访问而非更少。
Checklist | 检查清单
Description enhanced by Claude AI