feat(users): add allowed models restriction for users#347
feat(users): add allowed models restriction for users#347ding113 merged 3 commits intoding113:devfrom
Conversation
Summary of ChangesHello @miraserver, 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! This pull request introduces a significant feature that allows administrators to define specific AI model access restrictions for individual users. By integrating a new 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
This pull request introduces a new feature to restrict user access to specific AI models. The changes are comprehensive, spanning from a database migration to add the allowed_models field, to backend logic for enforcement via a new ModelGuard in the proxy pipeline, and frontend UI for configuration and display. The implementation is well-executed, including i18n translations and proper permission handling for the new field. My main feedback is to enforce a NOT NULL constraint on the new allowed_models database column to improve data integrity and consistency, which I've detailed in the specific comments.
drizzle/0036_add_allowed_models.sql
Outdated
There was a problem hiding this comment.
To ensure data integrity and prevent NULL values for the allowed_models column, it's recommended to add a NOT NULL constraint. This aligns with the intention of having a default empty array [] and simplifies handling in the application code, as you won't need to check for null values from the database.
ALTER TABLE "users" ADD COLUMN IF NOT EXISTS "allowed_models" jsonb NOT NULL DEFAULT '[]'::jsonb;There was a problem hiding this comment.
This suggestion doesn't match the existing codebase patterns. The tags field in the same table uses the identical pattern without NOT NULL:
tags: jsonb('tags').$type<string[]>().default([]),Consistency with existing code is preferred over this style change.
| @@ -40,6 +40,10 @@ export const users = pgTable('users', { | |||
| isEnabled: boolean('is_enabled').notNull().default(true), | |||
| expiresAt: timestamp('expires_at', { withTimezone: true }), | |||
|
|
|||
| // Allowed models (AI model restrictions) | |||
| // Empty array = no restrictions, non-empty = only listed models allowed | |||
| allowedModels: jsonb('allowed_models').$type<string[]>().default([]), | |||
There was a problem hiding this comment.
To match the recommended NOT NULL constraint in the database migration, you should also update the Drizzle schema definition to include .notNull(). This ensures that the ORM is aware of the non-nullable constraint, providing better type safety and preventing accidental null insertions.
| allowedModels: jsonb('allowed_models').$type<string[]>().default([]), | |
| allowedModels: jsonb('allowed_models').$type<string[]>().notNull().default([]), |
There was a problem hiding this comment.
Same reasoning as above - consistency with existing codebase patterns. The tags field uses the same approach without .notNull().
github-actions
bot
added
size/M
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[Critical] [LOGIC-BUG] Model guard is never executed - feature is completely broken
Why this is a problem: The ProxyModelGuard class is imported into guard-pipeline.ts and a step definition is added to the Steps record, but the "model" step key is never added to either CHAT_PIPELINE or COUNT_TOKENS_PIPELINE configurations. This means the model restriction feature will never be enforced at runtime, even when users configure allowedModels.
Users who configure model restrictions will believe their API is protected, but requests with disallowed models will still be processed.
Suggested fix:
// In guard-pipeline.ts, add "model" to CHAT_PIPELINE after auth:
export const CHAT_PIPELINE: GuardConfig = {
steps: [
"auth",
"model", // <-- Add this line to enforce model restrictions
"version",
"probe",
"session",
"requestFilter",
"sensitive",
"rateLimit",
"provider",
"messageContext",
],
};There was a problem hiding this comment.
This is a false positive. The "model" step IS added to both pipelines in this PR. Please check the actual diff:
- CHAT_PIPELINE line 151:
"model"added after"auth" - COUNT_TOKENS_PIPELINE line 165:
"model"added after"auth"
The feature is fully functional.
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] No unit tests for ProxyModelGuard
Why this is a problem: The new ProxyModelGuard class has critical security logic for model restrictions. However, there are no tests to verify:
- Empty/undefined allowedModels bypasses the check correctly
- Non-empty allowedModels with matching model passes
- Non-empty allowedModels with non-matching model is rejected
- Case-insensitive matching works correctly
- Empty/null model is rejected when restrictions exist
This is a security-sensitive feature that could silently fail without proper test coverage.
Suggested fix:
// src/app/v1/_lib/proxy/model-guard.test.ts
import { describe, it, expect, vi } from 'vitest';
import { ProxyModelGuard } from './model-guard';
describe('ProxyModelGuard', () => {
it('should skip check when allowedModels is empty', async () => {
const session = createMockSession({ allowedModels: [], model: 'any-model' });
const result = await ProxyModelGuard.ensure(session);
expect(result).toBeNull();
});
it('should reject model not in allowedModels', async () => {
const session = createMockSession({
allowedModels: ['claude-3-opus'],
model: 'gpt-4'
});
const result = await ProxyModelGuard.ensure(session);
expect(result?.status).toBe(400);
});
it('should allow case-insensitive matching', async () => {
const session = createMockSession({
allowedModels: ['Claude-3-Opus'],
model: 'claude-3-opus'
});
const result = await ProxyModelGuard.ensure(session);
expect(result).toBeNull();
});
});There was a problem hiding this comment.
Acknowledged. Unit tests are desirable but the project doesn't have mandatory test coverage requirements for guard components. This can be addressed in a follow-up PR if the maintainers prefer.
There was a problem hiding this comment.
Code Review Summary
This PR adds user-level model restrictions (allowedModels) feature, allowing admins to restrict which AI models a user can access. The implementation spans database schema, backend logic, UI forms, and localization. However, there is a critical bug that makes the entire feature non-functional.
PR Size: L (Large)
- Lines changed: 2,153 (2,151 additions, 2 deletions)
- Files changed: 20
Split suggestion: Consider separating the schema migration and i18n snapshot files from the core feature changes to reduce review burden.
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)
-
[LOGIC-BUG] Model guard is never executed (
src/app/v1/_lib/proxy/guard-pipeline.ts)The
ProxyModelGuardclass is created, imported, and registered in theStepsrecord, but the"model"step key is never added to the pipeline configurations (CHAT_PIPELINEorCOUNT_TOKENS_PIPELINE). This means:- The model restriction feature will NEVER be enforced at runtime
- Users who configure
allowedModelswill falsely believe they're protected - All models will be accepted regardless of configuration
Fix: Add
"model"step to the pipeline after"auth".
High Priority Issues (Should Fix)
-
[TEST-MISSING-CRITICAL] No unit tests for ProxyModelGuard (
src/app/v1/_lib/proxy/model-guard.ts)This security-sensitive feature has zero test coverage. The model guard logic includes case-insensitive matching and various edge cases that should be tested.
Review Coverage
- Logic and correctness - Critical issue found
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Missing tests for core feature
- Code clarity - Good
Automated review by Claude AI
Response to Code Review Comments1. ✅ [CRITICAL BUG] Model guard is never executed - FALSE POSITIVEThe code review bot analyzed incorrectly. Looking at the actual PR diff: // CHAT_PIPELINE - line 151
steps: [
"auth",
+ "model", // ✅ ADDED
"version",
...
]
// COUNT_TOKENS_PIPELINE - line 165
- steps: ["auth", "version", "probe", "requestFilter", "provider"],
+ steps: ["auth", "model", "version", "probe", "requestFilter", "provider"], // ✅ ADDEDThe 2. ⏭️ [MEDIUM] NOT NULL constraint - Does not match codebase patternsChecked existing code in // Existing pattern for JSONB arrays with defaults:
tags: jsonb('tags').$type<string[]>().default([]), // no .notNull()The codebase consistently uses JSONB arrays with default values without 3. ⏭️ [HIGH] Unit tests - Nice-to-have, not blockingUnit tests are desirable but the project doesn't require mandatory test coverage for guard components. Can be added in a follow-up PR if needed. SummaryAll critical issues raised are either false positives or style suggestions that don't match project conventions. The PR is ready for review and merge. Response by PR author |
- Add allowedModels field to users table (JSONB array) - Add ModelGuard to proxy pipeline to enforce model restrictions - Display allowed models in key-list-header component - Add allowedModels input to user form with tag-style UI - Add i18n translations for all supported languages (en, ja, ru, zh-CN, zh-TW) - Add field permission for allowedModels (admin-only) - Add database migration 0035_add_allowed_models.sql 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
miraserver
force-pushed
the
feat/allowed-models
branch
from
0fda825 to
0726f49
Compare
|
It looks like you modified the existing |
- Restore original 0035_snapshot.json (allowed_clients) - Regenerate 0036 migration using `bun run db:generate` - Migration now properly generated, not manually edited 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
miraserver
force-pushed
the
feat/allowed-models
branch
from
31c03d3 to
8995def
Compare
|
Thanks for catching this! You're right - I had incorrectly modified the existing Fixed in commit 8995def:
The migration file and snapshot are now properly generated by Drizzle Kit, following the same workflow used for |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
Add functionality to restrict which AI models a user can access based on a per-user whitelist. This provides administrators with fine-grained control over model access at the user level.
Related PRs:
Problem
Administrators need to control which AI models specific users can access. For example:
Solution
Implement
allowedModelsfield on the users table with enforcement through a newModelGuardin the proxy pipeline.Features
allowedModelsfield on users table (JSONB array)ProxyModelGuardvalidates requested model in proxy pipeline (runs immediately after auth)Behavior
Changes
Core Changes
src/drizzle/schema.ts+ migration0036_add_allowed_models.sqlsrc/app/v1/_lib/proxy/model-guard.ts(new)src/app/v1/_lib/proxy/guard-pipeline.tsSupporting Changes
src/types/user.ts(User, CreateUserData, UpdateUserData)src/repository/user.ts,src/repository/key.tssrc/lib/validation/schemas.ts(Zod schema: max 50 models, 64 chars each)src/lib/permissions/user-field-permissions.ts(admin-only)src/app/[locale]/dashboard/_components/user/forms/user-form.tsx,key-list-header.tsxFiles Changed (21 files)
drizzle/0036_add_allowed_models.sql,drizzle/meta/0036_snapshot.json,drizzle/meta/_journal.jsonsrc/types/user.tssrc/drizzle/schema.tssrc/lib/validation/schemas.tssrc/lib/permissions/user-field-permissions.tssrc/repository/user.ts,src/repository/key.ts,src/repository/_shared/transformers.tssrc/app/v1/_lib/proxy/model-guard.ts,src/app/v1/_lib/proxy/guard-pipeline.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/forms/user-form.tsx,key-list-header.tsxmessages/{en,zh-CN,zh-TW,ja,ru}/dashboard.jsonTest Plan
Checklist
devbranch (migration 0036, after feat: add client (CLI/IDE) restrictions for user management #341's 0035)Description enhanced by Claude AI