-
Notifications
You must be signed in to change notification settings - Fork 18.5k
fix: block normal users from viewing secret dataset API keys #28515
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?
fix: block normal users from viewing secret dataset API keys #28515
Conversation
Summary of ChangesHello @honjo-hiroaki-gtt, 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 significantly enhances security by implementing robust access control for secret API keys. It introduces both backend and frontend changes to ensure that only authorized users (specifically workspace editors or dataset operators for dataset API keys, and workspace editors for general secret keys) can view or generate these sensitive credentials. This prevents normal users or viewers from accessing API key information, thereby aligning the user interface with the new, stricter access policies. 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 adds permission checks for viewing API keys, a crucial security enhancement. For datasets, it restricts access to editors and dataset operators, while for other resources like apps, it limits access to editors. The UI is updated accordingly to hide API key management from unauthorized users. The implementation is mostly correct, but I've found a critical security vulnerability: the new permission checks only apply to listing keys (GET), while creating (POST) and deleting (DELETE) keys seem to be unprotected. I've also added a suggestion to improve the code's readability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…taset-apikey-visibility # Conflicts: # api/controllers/console/apikey.py
|
/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 effectively addresses the security concern of unauthorized access to dataset API keys. The backend changes correctly implement role-based access control for API key operations, ensuring that only users with appropriate permissions can view, create, or delete them. The frontend changes align with this by conditionally rendering the UI elements related to API key management, which improves the user experience by not showing options that are not available to them. The implementation is solid and follows best practices. I have one suggestion to improve the naming of a variable in the frontend for better code clarity and maintainability.
| const [isSecretKeyModalVisible, setIsSecretKeyModalVisible] = useState(false) | ||
|
|
||
| const isCurrentWorkspaceManager = useAppContextSelector(state => state.isCurrentWorkspaceManager) | ||
| const isDatasetApiKeyVisible = useAppContextSelector(state => state.isCurrentWorkspaceEditor || state.isCurrentWorkspaceDatasetOperator) |
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.
For better code clarity and maintainability, consider renaming isDatasetApiKeyVisible to something that more accurately reflects the permission being checked, such as canManageDatasetApiKeys. This name aligns better with the concept of checking a user's role for managing API keys, rather than just controlling UI visibility. You would need to update its usages in this file as well.
Additionally, the logic state.isCurrentWorkspaceEditor || state.isCurrentWorkspaceDatasetOperator is equivalent to the backend's is_dataset_editor permission. It would be ideal to encapsulate this logic within the AppContext as a single selector, for example isCurrentWorkspaceDatasetEditor, to promote reusability and keep permission logic centralized.
| const isDatasetApiKeyVisible = useAppContextSelector(state => state.isCurrentWorkspaceEditor || state.isCurrentWorkspaceDatasetOperator) | |
| const canManageDatasetApiKeys = useAppContextSelector(state => state.isCurrentWorkspaceEditor || state.isCurrentWorkspaceDatasetOperator) |
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.
@gemini-code-assist
Thanks for the feedback! I’ve renamed the variable to canManageDatasetApiKeys and added the centralized selector isCurrentWorkspaceDatasetEditor in AppContext to align the permission check. Changes are pushed—could you take another look?
/gemini review
|
/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 effectively restricts access to dataset API keys for users with normal/viewer roles, both on the backend and frontend. The changes are well-aligned with the goal of enhancing security. The backend logic correctly enforces permissions for creating, viewing, and deleting API keys based on user roles and resource type. The frontend conditionally renders UI elements for API key management, providing a consistent user experience.
I've included a couple of suggestions to improve code maintainability and readability. One is to refactor a new permission flag in the React context to be more declarative and avoid repeating logic. The other points out some code duplication in the Python backend's new permission-checking methods, which could be consolidated to prevent potential inconsistencies in the future.
| def _has_permission(self, user): | ||
| if self.resource_type == "dataset": | ||
| return user.is_dataset_editor | ||
| return user.is_admin_or_owner |
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.
This _has_permission method is very similar to the one in BaseApiKeyListResource on line 55. This duplication could lead to maintenance issues in the future (e.g., if permission logic for datasets changes, it would need to be updated in two places). Consider refactoring to a shared utility function to centralize the permission logic.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary
Fixes #28514.
Screenshots
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods