Limit github onboarding to team users #2729
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new mechanism to control GitHub connection button interactions based on team user status. By implementing an Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthStore
participant GithubConnect
participant Permissions
User->>AuthStore: Login
AuthStore->>GithubConnect: Provide user object
GithubConnect->>Permissions: Check isTeamUser(user)
Permissions-->>GithubConnect: Return team status
GithubConnect->>GithubConnect: Update button state
Possibly related PRs
Poem
🪧 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: 2
🔭 Outside diff range comments (1)
frontend/src/config/integrations/github/components/github-connect.vue (1)
Line range hint
8-17: Improve user feedback and error handlingThe current implementation has several UX concerns:
- The tooltip message about "disabled due to issues" is misleading for team users
- No visual feedback during permission checks
- Commented out "Details" button without explanation
Consider these improvements:
<el-tooltip - content="Onboarding new data for GitHub is currently disabled due to some issues we are experiencing. - Please contact support if you need to onboard new data or update settings." + :content="isTeam ? 'Connect your GitHub account' : 'GitHub integration is currently limited to team members only. Please contact support for access.'" placement="top" :disabled="isTeam" > - <lf-button type="secondary" :disabled="!isTeam" @click="isSettingsDrawerOpen = true"> + <lf-button + type="secondary" + :disabled="!isTeam" + :loading="isCheckingPermissions" + @click="handleConnect" + > <lf-icon name="link-simple" /> Connect </lf-button> </el-tooltip>
🧹 Nitpick comments (2)
frontend/src/config/permissions/index.ts (1)
31-31: Add input validation for user ID formatThe
isTeamUserfunction should validate the UUID format before checking inclusion.Consider adding validation:
-export const isTeamUser = (user: User | null) => user && teamUsers.includes(user.id); +export const isTeamUser = (user: User | null) => { + const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + return user && UUID_REGEX.test(user.id) && teamUsers.includes(user.id); +};frontend/src/config/integrations/github/components/github-connect.vue (1)
Line range hint
4-8: Document or remove commented codeThe commented out "Details" button lacks context. Either document why it's commented out or remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/config/integrations/github/components/github-connect.vue(1 hunks)frontend/src/config/permissions/index.ts(2 hunks)
| const teamUsers: string[] = [ | ||
| 'fc795a2e-9bd6-4b2d-817b-c357d1d59a2a', | ||
| 'df7f38f0-e994-4954-b680-edd9930a2e02', | ||
| '5418959c-7410-40a8-bce7-a3635291bd29', | ||
| 'f1004da4-f99c-4a9d-b718-6447128d961d', | ||
| 'af964295-bc82-436c-8cce-843cd0a6b2ef', | ||
| 'a9745d27-1d2e-4f6f-adda-aeb7c9553cbb', | ||
| 'd2e45678-b867-4a14-950a-b6f72319d44b', | ||
| '208c0a98-6290-42c5-a1a4-b2ec3451d97b', | ||
| '10cc0c77-2ed8-487d-9e40-688a46786fe5', | ||
| '5e127804-4382-4648-bed7-12c34846d5ce', | ||
| '893c09e1-8f33-4464-80fe-9db431d82ba3', | ||
| 'cbc5ec8f-76c2-49f0-8288-c61c5c238d6d', | ||
| '815a59ef-bb99-42ea-b6b8-fe30a1c1e2e1', | ||
| // '0fbf4c1b-3774-4a7a-a126-7a722ed8ff93', | ||
| ]; |
There was a problem hiding this comment.
Consider moving team user management to the backend
Hardcoding user IDs in the frontend raises several concerns:
- Security: User IDs are exposed in client-side code
- Maintainability: Adding/removing users requires code changes
- Documentation: No context for the listed IDs or the commented out ID
Consider these alternatives:
- Move team user management to the backend with proper access control
- Implement a configuration endpoint to fetch authorized users
- Add role-based access control instead of user-based restrictions
Would you like me to propose a detailed implementation for any of these alternatives?
|
|
||
| const { user } = storeToRefs(useAuthStore()); | ||
|
|
||
| const isTeam = computed(() => isTeamUser(user.value)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state
The component should handle potential store errors and loading states.
Add error handling and loading state:
const isSettingsDrawerOpen = ref(false);
+const isCheckingPermissions = ref(false);
+const error = ref<Error | null>(null);
const { user } = storeToRefs(useAuthStore());
-const isTeam = computed(() => isTeamUser(user.value));
+const isTeam = computed(() => {
+ try {
+ return isTeamUser(user.value);
+ } catch (e) {
+ error.value = e as Error;
+ return false;
+ }
+});
+
+const handleConnect = async () => {
+ isCheckingPermissions.value = true;
+ try {
+ // Verify permissions before opening drawer
+ if (await verifyGithubPermissions()) {
+ isSettingsDrawerOpen.value = true;
+ }
+ } catch (e) {
+ error.value = e as Error;
+ } finally {
+ isCheckingPermissions.value = false;
+ }
+};Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| const teamUsers: string[] = [ | ||
| 'fc795a2e-9bd6-4b2d-817b-c357d1d59a2a', | ||
| 'df7f38f0-e994-4954-b680-edd9930a2e02', | ||
| '5418959c-7410-40a8-bce7-a3635291bd29', | ||
| 'f1004da4-f99c-4a9d-b718-6447128d961d', | ||
| 'af964295-bc82-436c-8cce-843cd0a6b2ef', | ||
| 'a9745d27-1d2e-4f6f-adda-aeb7c9553cbb', | ||
| 'd2e45678-b867-4a14-950a-b6f72319d44b', | ||
| '208c0a98-6290-42c5-a1a4-b2ec3451d97b', | ||
| '10cc0c77-2ed8-487d-9e40-688a46786fe5', | ||
| '5e127804-4382-4648-bed7-12c34846d5ce', | ||
| '893c09e1-8f33-4464-80fe-9db431d82ba3', | ||
| 'cbc5ec8f-76c2-49f0-8288-c61c5c238d6d', | ||
| '815a59ef-bb99-42ea-b6b8-fe30a1c1e2e1', | ||
| // '0fbf4c1b-3774-4a7a-a126-7a722ed8ff93', | ||
| ]; |
There was a problem hiding this comment.
Hey @gaspergrom , why not adding this to .env.override.local in crowd-kube?
I think it will be better for security reasons
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
New Features
Bug Fixes