feat(git-provider): enhance sharing and permissions management#4135
feat(git-provider): enhance sharing and permissions management#4135Siumauricio merged 5 commits intocanaryfrom
Conversation
- Added functionality to toggle sharing of Git providers with the organization. - Introduced a new column "sharedWithOrganization" in the git_provider table to track sharing status. - Updated user permissions to include accessedGitProviders, allowing for more granular access control. - Enhanced API routes to support fetching accessible Git providers based on user roles and permissions. - Implemented UI components for managing Git provider sharing and permissions in the dashboard.
dosubot
bot
added
the
size:L
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| await audit(ctx, { | ||
| action: "update", | ||
| resourceType: "gitProvider", | ||
| resourceId: provider.gitProviderId, | ||
| resourceName: provider.name ?? provider.gitProviderId, | ||
| }); | ||
|
|
||
| return await updateGitProvider(input.gitProviderId, { | ||
| sharedWithOrganization: input.sharedWithOrganization, |
There was a problem hiding this comment.
toggleShare
toggleShare only validates that provider.userId === ctx.session.userId, but never checks provider.organizationId === ctx.session.activeOrganizationId. A user who owns providers across multiple organizations can call this endpoint while operating in a different org context and toggle sharing on a provider that belongs to another org — causing unintended exposure.
| } | |
| await audit(ctx, { | |
| action: "update", | |
| resourceType: "gitProvider", | |
| resourceId: provider.gitProviderId, | |
| resourceName: provider.name ?? provider.gitProviderId, | |
| }); | |
| return await updateGitProvider(input.gitProviderId, { | |
| sharedWithOrganization: input.sharedWithOrganization, | |
| if ( | |
| provider.userId !== ctx.session.userId || | |
| provider.organizationId !== ctx.session.activeOrganizationId | |
| ) { | |
| throw new TRPCError({ | |
| code: "UNAUTHORIZED", | |
| message: "Only the owner can share this provider", | |
| }); | |
| } |
| .returning() | ||
| .then((response) => response[0]); | ||
| }; | ||
|
|
||
| export const getAccessibleGitProviderIds = async (session: { | ||
| userId: string; | ||
| activeOrganizationId: string; | ||
| }): Promise<Set<string>> => { | ||
| const { userId, activeOrganizationId } = session; | ||
|
|
||
| const allOrgProviders = await db.query.gitProvider.findMany({ | ||
| where: eq(gitProvider.organizationId, activeOrganizationId), | ||
| columns: { | ||
| gitProviderId: true, | ||
| userId: true, | ||
| sharedWithOrganization: true, | ||
| }, | ||
| }); | ||
|
|
||
| const memberRecord = await db.query.member.findFirst({ | ||
| where: and( | ||
| eq(member.userId, userId), | ||
| eq(member.organizationId, activeOrganizationId), | ||
| ), | ||
| columns: { accessedGitProviders: true, role: true }, | ||
| }); | ||
|
|
||
| if (memberRecord?.role === "owner" || memberRecord?.role === "admin") { | ||
| return new Set(allOrgProviders.map((p) => p.gitProviderId)); | ||
| } | ||
|
|
||
| const licensed = await hasValidLicense(activeOrganizationId); | ||
| const assignedSet = licensed | ||
| ? new Set(memberRecord?.accessedGitProviders ?? []) | ||
| : new Set<string>(); |
There was a problem hiding this comment.
getAccessibleGitProviderIds is now called in five separate routers (gitProvider.getAll, bitbucketProviders, githubProviders, gitlabProviders, giteaProviders). Each call issues two queries — one to fetch all org providers and one to fetch the member record. For a dashboard page that renders all provider types, this adds 10 extra round-trips. Consider caching the result within a request context (e.g. via tRPC context) or combining it into a single query with a join.
| }); | ||
| } | ||
|
|
||
| await deactivateLicenseKey(currentUser.licenseKey); | ||
| try { | ||
| await deactivateLicenseKey(currentUser.licenseKey); | ||
| } catch (_) { | ||
| // Always clean up locally even if the license server is unreachable |
There was a problem hiding this comment.
The comment says "even if the license server is unreachable", implying only transient network errors should be ignored. The bare catch (_) will also suppress programming errors, unexpected response shapes, or authentication failures from the license server. If those silent failures occur, the license will be cleaned up locally but remain active on the server side — potentially blocking its reuse on another installation.
Consider narrowing the catch to network/connection errors, or at least logging the failure:
try {
await deactivateLicenseKey(currentUser.licenseKey);
} catch (err) {
// Log but continue — clean up locally even if the server is unreachable
console.error("Failed to deactivate license key remotely:", err);
}| Tooltip, | ||
| TooltipContent, |
There was a problem hiding this comment.
isPending is not destructured from the toggleShare mutation. While the mutation is in-flight the Switch remains interactive, so a user can double-fire conflicting toggle calls. Destructuring isPending and passing it as the disabled prop on the Switch would prevent that.
| Tooltip, | |
| TooltipContent, | |
| const { mutateAsync: toggleShare, isPending: isToggling } = | |
| api.gitProvider.toggleShare.useMutation(); |
- Added loading state for the sharing toggle in the UI to prevent user interaction during processing. - Enhanced authorization logic in the API to ensure that both user and organization ownership are validated before allowing sharing of Git providers. - Improved error handling in the license key deactivation process to log failures for better debugging.
- Replaced `createSchema.pick` with `z.object` for `apiFindOneLibsql` and `apiFindMountByApplicationId` to enforce stricter validation. - Ensured `libsqlId`, `serviceType`, and `serviceId` are required strings with minimum length constraints.
|
@Siumauricio Good job! 🙏 |
2 participants
What is this PR about?
Please describe in a short paragraph what this PR is about.
Checklist
Before submitting this PR, please make sure that:
canarybranch.Issues related (if applicable)
#2220
Screenshots (if applicable)
Greptile Summary
This PR adds git provider sharing (per-organization opt-in via a new
sharedWithOrganizationboolean) and granular per-user access control (accessedGitProviderson themembertable), gated behind an enterprise license. The migration is non-destructive, thegetAccessibleGitProviderIdshelper correctly handles owner/admin full-access and shared/assigned providers, and the UI properly hides edit controls from non-owners.toggleShareonly checksprovider.userId === ctx.session.userIdbut notprovider.organizationId === ctx.session.activeOrganizationId. A user who owns providers in multiple organizations can flip sharing on a provider that belongs to a different org context.getAccessibleGitProviderIdsissues 2 extra DB queries and is called in 5 separate routers; on pages that load all provider types this adds 10 extra round-trips.try/catcharounddeactivateLicenseKeycatches all error types, not just network failures, hiding unexpected server-side deactivation errors.Confidence Score: 4/5
Mostly safe to merge after the missing organization scope check in toggleShare is addressed.
One P1 finding: toggleShare can be called cross-organization since there is no activeOrganizationId guard. The remaining findings are P2 (performance, error visibility, missing loading state) and do not affect correctness of the happy path.
apps/dokploy/server/api/routers/git-provider.ts (toggleShare authorization logic)
Reviews (1): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile