Skip to content
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.

Sign up for GitHub

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

Limit github onboarding to team users #2729

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Limit github onboarding to team users #2729

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb6a648
Limit github onboarding to team users
gaspergrom Dec 16, 2024
946ccb8
Fix texts
gaspergrom Dec 16, 2024
27602d8
Team users from env
gaspergrom Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions frontend/src/config/integrations/github/components/github-connect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,32 @@
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."
placement="top"
:disabled="isTeam"
>
<span>
<lf-button type="secondary" disabled @click="isSettingsDrawerOpen = true">
<lf-icon name="link-simple" />
Connect
</lf-button>
</span>
<lf-button type="secondary" :disabled="!isTeam" @click="isSettingsDrawerOpen = true">
<lf-icon name="link-simple" />
Connect
</lf-button>
</el-tooltip>
</div>
<lf-github-settings-drawer v-if="isSettingsDrawerOpen" v-model="isSettingsDrawerOpen" :integration="null" />
</div>
</template>

<script setup lang="ts">
import { ref } from 'vue';
import { ref, computed } from 'vue';
import LfButton from '@/ui-kit/button/Button.vue';
import LfIcon from '@/ui-kit/icon/Icon.vue';
import LfGithubSettingsDrawer from '@/config/integrations/github/components/settings/github-settings-drawer.vue';
import { isTeamUser } from '@/config/permissions';
import { useAuthStore } from '@/modules/auth/store/auth.store';
import { storeToRefs } from 'pinia';

const isSettingsDrawerOpen = ref(false);

const { user } = storeToRefs(useAuthStore());

const isTeam = computed(() => isTeamUser(user.value));
Comment on lines +34 to +37
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

</script>

<script lang="ts">
Expand Down
20 changes: 20 additions & 0 deletions frontend/src/config/permissions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { LfRole } from '@/shared/modules/permissions/types/Roles';
import { LfPermission } from '@/shared/modules/permissions/types/Permissions';
import { User } from '@/modules/auth/types/User.type';
import readonly from './readonly';
import admin from './admin';
import projectAdmin from './projectAdmin';
Expand All @@ -9,3 +10,22 @@ export const lfPermissions: Record<LfRole, Record<LfPermission, boolean>> = {
admin,
projectAdmin,
};

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',
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider moving team user management to the backend

Hardcoding user IDs in the frontend raises several concerns:

  1. Security: User IDs are exposed in client-side code
  2. Maintainability: Adding/removing users requires code changes
  3. Documentation: No context for the listed IDs or the commented out ID

Consider these alternatives:

  1. Move team user management to the backend with proper access control
  2. Implement a configuration endpoint to fetch authorized users
  3. Add role-based access control instead of user-based restrictions

Would you like me to propose a detailed implementation for any of these alternatives?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gaspergrom , why not adding this to .env.override.local in crowd-kube?
I think it will be better for security reasons


export const isTeamUser = (user: User | null) => user && teamUsers.includes(user.id);
Loading