Feature/Add Permissions To API Keys #5590
Conversation
…during creation/update
Summary of ChangesHello @yau-wd, 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 delivers a crucial security and access control feature by integrating permissions directly into API keys. This allows for a more secure and controlled environment where each API key can be tailored with specific access rights, moving beyond a broad, all-or-nothing access model. The changes span database schema modifications, updates to the backend API logic for managing keys, and a comprehensive overhaul of the frontend UI to support permission selection and display. 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 permissions system for API keys, a significant security enhancement. The changes span across the backend and UI, allowing for granular control over what each API key can do.
My review focuses on the security implications and maintainability of the new implementation. Here are the key points:
- A new
permissionscolumn is added to theApiKeyentity, and database migrations are included for all supported databases. - The API key controller and service layers are updated to validate and filter based on these new permissions, preventing privilege escalation.
- The UI is updated with a new dialog to manage API key permissions.
I've identified a critical security concern in the database migration scripts, which grant all permissions to existing API keys by default. I've also pointed out a potential data integrity issue with a null value in the permissions list and a potential runtime error in a new database utility function. Additionally, I've suggested some improvements for code maintainability by addressing duplicated logic and silent error handling.
Overall, this is a great feature addition. Addressing the highlighted issues will make it more robust and secure.
packages/server/src/database/migrations/postgres/1765360298674-AddApiKeyPermission.ts
Outdated
Show resolved
Hide resolved
packages/server/src/database/migrations/postgres/1765360298674-AddApiKeyPermission.ts
Outdated
Show resolved
Hide resolved
… and platform type
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a permissions system for API keys, which is a significant and valuable feature. The changes span across the backend (database, services, controllers) and the frontend (UI dialogs, API calls). Key changes include adding a permissions column to the ApiKey entity, updating services and controllers to handle permissions with validation, modifying the UI to allow selecting permissions, and defining permissions for different platforms. The implementation is mostly solid, but I've found a few issues that need attention, including a potential security vulnerability, a bug in the frontend logic, and some areas for improvement in error handling and database migrations.
packages/server/src/database/migrations/mariadb/1765360298674-AddApiKeyPermission.ts
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature by adding granular permissions to API keys, which greatly enhances security and flexibility. The implementation is thorough, spanning database migrations, backend services, and a well-designed UI for permission management. I appreciate the careful thought given to permission validation, especially the restrictions on workspace and admin permissions and the platform-aware filtering. The deprecation of the old JSON-based API key import is also a welcome cleanup. My review has identified a recurring pattern of unsafe JSON.parse calls in several places, which could lead to runtime errors or failed migrations if data is malformed. I've provided specific suggestions to make the parsing more robust. Overall, this is an excellent contribution, and addressing the identified issues will make it even more solid.
packages/server/src/database/migrations/mariadb/1765360298674-AddApiKeyPermission.ts
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
yau-wd
requested review from
0xi4o,
HenryHengZJ,
harshit-flowise,
igor-magun-wd,
jocelynlin-wd,
muratkaratekeli,
simon-song-wd and
tianwei-liu
packages/server/src/database/migrations/mariadb/1765360298674-AddApiKeyPermission.ts
Show resolved
Hide resolved
| @Column({ type: 'text' }) | ||
| keyName: string | ||
|
|
||
| @Column({ nullable: false, type: 'text', default: '[]' }) |
There was a problem hiding this comment.
also is it possible to use simple-json instead?
@Column({
type: 'simple-json',
nullable: false,
default: '[]'
})
permissions: string[]; // TypeORM handles the stringify/parse automatically
I am under the impression that TypeORM can automatically pick the best column type for the underlying DB (it will use JSONB in Postgres, JSON in MySQL, and TEXT in SQLite), would you be able to help verify?
This would allow us to mitigate the performance and maintenance bottleneck especially if in the future we need to scale to combine JWT scopes with other services
There was a problem hiding this comment.
I'm not sure, but this PR is to address Jira2 Ticket Flowise-12 quickly. Migrating to simple-json could be a separate initiative if this becomes a bottleneck.
Priority is testing and ensuring no unintended side effects in other parts of the codebase.
There was a problem hiding this comment.
depends on how much effort it would take to try out simple-json since changing the data type later on will be much more expensive (and the type was a recommendation by gemini for scalability and to avoid tech debt)
There was a problem hiding this comment.
Test PostgreSQL
1. Initial attempt with LIKE (fails on PostgreSQL)
const apiKeysWithChatflowsView = await this.AppDataSource.getRepository(ApiKey)
.createQueryBuilder('apikey')
.where('apikey.permissions LIKE :permission', { permission: '%chatflows:view%' })
.getMany()
// Log or use the filtered API keys as needed
logger.info(`Found ${apiKeysWithChatflowsView.length} API keys with chatflows:view permission`)Error:
query failed: SELECT "apikey"."id" AS "apikey_id", "apikey"."apiKey" AS "apikey_apiKey", "apikey"."apiSecret" AS "apikey_apiSecret", "apikey"."keyName" AS "apikey_keyName", "apikey"."permissions" AS "apikey_permissions", "apikey"."updatedDate" AS "apikey_updatedDate", "apikey"."workspaceId" AS "apikey_workspaceId" FROM "apikey" "apikey" WHERE "apikey"."permissions" LIKE $1 -- PARAMETERS: ["%chatflows:view%"]
error: error: operator does not exist: jsonb ~~ unknown2. Fixed with PostgreSQL JSONB operator (works)
const apiKeysWithChatflowsView = await this.AppDataSource.getRepository(ApiKey)
.createQueryBuilder('apikey')
.where('apikey.permissions::jsonb @> :permission', { permission: '"chatflows:view"' })
.getMany()
// Log or use the filtered API keys as needed
logger.info(`Found ${apiKeysWithChatflowsView.length} API keys with chatflows:view permission`)Success:
2026-01-06 20:55:44 [INFO]: Found 1 API keys with chatflows:view permissionMy opinion
Without maintaining multiple queries, we'd have to fall back to loading all records and filtering in-memory, which defeats the purpose of database-level optimization, or using TEXT across different database platforms.
SQLite does not support the jsonb type and PostgreSQL's JSON operators (?, @>, etc.).
Database-specific solutions:
- PostgreSQL: Use
jsonboperators (?,@>) - SQLite: Use
LIKEwith text pattern matching - MySQL & MariaDB: Use
JSON_CONTAINS()
There was a problem hiding this comment.
| * @param operation - The operation being performed (for error message) | ||
| * @throws InternalFlowiseError if validation fails | ||
| */ | ||
| function validatePermissions(user: LoggedInUser, requestedPermissions: string[], operation: string) { |
There was a problem hiding this comment.
[minor] do we want to limit operation with a type or enum?
There was a problem hiding this comment.
Probably in the next phase, as we also need to take Stripe product metadata into consideration so everything can be standardized and we can determine whether a type, enum, or other declaration or method is most appropriate.
Co-authored-by: Jocelyn Lin <[email protected]>
|
Edge Case: Pro plan users downgrading while retaining API keys with sharing permissions Done: feat(PricingDialog.jsx): handle downgrade with excessive apikey permissions TestUI
Database
|
Description
API Key
permissionscolumn intoapikeytable.workspaceandadmincategories.Permissions
apikeys import.sso.API Key
Database
Before
After
UI
Create/Update
Admin
Non-Admin Users
Read
Admin
Non-Admin Users
Postman
View
Create
Update
Delete
Role
Database
Before
After
UI
Create/Update
Admin
Non-Admin Users
Read
Admin
Non-Admin Users