-
-
Notifications
You must be signed in to change notification settings - Fork 23.5k
Feature/Add Permissions To API Keys #5590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…ensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
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