fix: managers should not be allowed to create api keys#5409
fix: managers should not be allowed to create api keys#5409victorvhs017 merged 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
""" WalkthroughThis change restricts API key management actions within the organization settings to users with the "owner" role by removing the "manager" role from authorization checks in the relevant backend actions. The frontend permission checks and UI components were updated accordingly: access is now limited exclusively to owners, with non-owners immediately blocked from page rendering. The navigation menu for organization settings was modified to hide the API keys section from non-owners. Additionally, the translation key for "powered by Formbricks" was removed from multiple localization files, including German, English, French, Brazilian Portuguese, Portuguese (Portugal), and Traditional Chinese. No changes were made to the signatures or declarations of exported or public entities, except for the removal of the translation key in the localization files. Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/modules/organization/settings/api-keys/page.tsx (1)
45-45: Consider renaming prop for consistency with variable naming.While the permission check works correctly, there's a minor inconsistency between the variable name
isNotOwnerand the prop nameisReadOnly. Consider renaming for better code clarity.- isReadOnly={isNotOwner} + isNotOwner={isNotOwner}This would require updating the prop name in the
ApiKeyListcomponent as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/modules/organization/settings/api-keys/actions.ts(3 hunks)apps/web/modules/organization/settings/api-keys/page.tsx(3 hunks)packages/lib/messages/de-DE.json(0 hunks)packages/lib/messages/en-US.json(0 hunks)packages/lib/messages/fr-FR.json(0 hunks)packages/lib/messages/pt-BR.json(0 hunks)packages/lib/messages/pt-PT.json(0 hunks)packages/lib/messages/zh-Hant-TW.json(0 hunks)
💤 Files with no reviewable changes (6)
- packages/lib/messages/de-DE.json
- packages/lib/messages/en-US.json
- packages/lib/messages/fr-FR.json
- packages/lib/messages/pt-BR.json
- packages/lib/messages/zh-Hant-TW.json
- packages/lib/messages/pt-PT.json
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Run E2E Tests / Run E2E Tests
- GitHub Check: Build Formbricks-web / Build Formbricks-web
- GitHub Check: Run Linters / Linters
- GitHub Check: Run Unit Tests / Unit Tests
- GitHub Check: SonarQube
- GitHub Check: Validate Docker Build
- GitHub Check: Analyze (javascript)
- GitHub Check: Tusk Tester
🔇 Additional comments (5)
apps/web/modules/organization/settings/api-keys/actions.ts (3)
28-28: Permission restriction applied correctly.The authorization roles for deleting API keys have been restricted to only "owner", removing "manager" from the allowed roles. This change aligns with the PR objective to limit API key management to owners only.
50-50: Permission restriction applied correctly.The authorization roles for creating API keys have been restricted to only "owner", removing "manager" from the allowed roles. This change directly addresses the core objective of the PR to prevent managers from creating API keys.
72-72: Permission restriction applied correctly.The authorization roles for updating API keys have been restricted to only "owner", removing "manager" from the allowed roles. This change consistently applies the owner-only restriction across all API key operations.
apps/web/modules/organization/settings/api-keys/page.tsx (2)
22-22: Variable name change is appropriate for the new permission model.The variable has been renamed to
isNotOwnerwhich clearly indicates the permission check being performed. This boolean will be true for any non-owner roles (including managers).
34-37: Alert warning updated correctly for non-owners.The condition for displaying the warning now uses
isNotOwner, correctly showing the alert to managers and other non-owner roles. The alert formatting with size "small" makes the warning less intrusive while still being clear about the permission restriction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/modules/organization/settings/api-keys/page.tsx (1)
47-47: Inconsistent prop namingThe prop name
isReadOnlydoesn't match the variable nameisNotOwner, which could be confusing. Consider renaming the prop to match the variable name for better readability and maintainability.- isReadOnly={isNotOwner} + isNotOwner={isNotOwner}This would require updating the
ApiKeyListcomponent to use theisNotOwnerprop instead ofisReadOnly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(app)/environments/[environmentId]/settings/(organization)/components/OrganizationSettingsNavbar.tsx(2 hunks)apps/web/modules/organization/settings/api-keys/page.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Run E2E Tests / Run E2E Tests
- GitHub Check: Build Formbricks-web / Build Formbricks-web
- GitHub Check: Run Linters / Linters
- GitHub Check: Run Unit Tests / Unit Tests
- GitHub Check: SonarQube
- GitHub Check: Validate Docker Build
- GitHub Check: Analyze (javascript)
- GitHub Check: Tusk Tester
🔇 Additional comments (3)
apps/web/app/(app)/environments/[environmentId]/settings/(organization)/components/OrganizationSettingsNavbar.tsx (2)
25-25: Extraction of owner role for access controlGood addition to extract
isOwneralongsideisMemberfromgetAccessFlags. This properly separates the role-based access control concerns.
62-62: Effective restriction of API keys navigation to owners onlyThis change properly restricts the visibility of the API keys navigation item to owners only, which aligns with the PR objective to prevent managers from creating API keys.
apps/web/modules/organization/settings/api-keys/page.tsx (1)
22-24: Unauthorized access handling implemented correctlyGood implementation of the server-side check that prevents non-owners from accessing the API keys page. This effectively addresses the comment from the previous review about preventing access when the route is accessed directly.
|
✅ Generated 3 tests - 3 passed (ea4d650) View tests ↗Test Summary
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
What does this PR do?
Fixes #(issue)
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit