pub_soft/WeKnora
3
0
Fork 0
mirror of https://github.com/Tencent/WeKnora.git synced 2026-06-04 13:30:32 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(rbac): close org-tenant gate gaps from Plan 3 review

Browse Source 
Three follow-ups from #1350 review:

1. DELETE /organizations/:id/members/:tenant_id was un-gated, so a
   tenant Viewer could remove their own tenant from an org via the
   members endpoint and bypass the Admin+ requirement that POST
   /:id/leave already enforces. Gate it with g.Admin() — symmetric
   with /leave.

2. isOwnerTenant() returned (false, err) when the owner user lookup
   failed, and the two call sites only checked the bool. A deleted /
   unreadable owner user would silently fall through and let admins
   remove or re-role the would-be owner tenant. Make the helper
   fail-closed: any lookup error now returns (true, err), and both
   call sites refuse the destructive op on error.

3. POST /:id/request-upgrade was un-gated. Approval of the request
   changes the whole tenant's org role, so it is in the same trust
   tier as join/leave/create. Add g.Admin().
This commit is contained in:
wizardchen
2026-05-15 22:25:31 +08:00
committed by lyingbug
parent 928f5abe3f
commit eda6b1011a
2 changed files with 37 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
internal/application/service/organization.go
View File

@@ -390,9 +390,13 @@ func (s *organizationService) RemoveTenantMember(ctx context.Context, orgID stri
return err
}
// Owner-tenant resolution: derive owner tenant from Organization.OwnerID's
// own tenant. If the OwnerID-user has been deleted (very rare during the
// org's lifetime) we fall through and refuse the destructive action.
if isOwnerTenant, err := s.isOwnerTenant(ctx, org, memberTenantID); err == nil && isOwnerTenant {
// own tenant. isOwnerTenant is fail-closed — on any lookup error we
// also refuse, so a deleted owner user can't be used to bypass the
// "cannot remove owner tenant" invariant.
if isOwnerTenant, ownerErr := s.isOwnerTenant(ctx, org, memberTenantID); isOwnerTenant || ownerErr != nil {
if ownerErr != nil {
logger.Warnf(ctx, "RemoveTenantMember: owner-tenant resolution failed for org %s: %v; refusing", orgID, ownerErr)
}
return ErrCannotRemoveOwner
}
@@ -431,7 +435,10 @@ func (s *organizationService) UpdateTenantMemberRole(ctx context.Context, orgID
if err != nil {
return err
}
if isOwnerTenant, err := s.isOwnerTenant(ctx, org, memberTenantID); err == nil && isOwnerTenant {
if isOwnerTenant, ownerErr := s.isOwnerTenant(ctx, org, memberTenantID); isOwnerTenant || ownerErr != nil {
if ownerErr != nil {
logger.Warnf(ctx, "UpdateTenantMemberRole: owner-tenant resolution failed for org %s: %v; refusing", orgID, ownerErr)
}
return ErrCannotChangeOwnerRole
}
_ = operatorUserID
@@ -579,13 +586,27 @@ func (s *organizationService) GetTenantRoleInOrg(ctx context.Context, orgID stri
// org.OwnerID. The owner tenant is undeletable / unchangeable in the
// org membership table — that lets us preserve the "owner can never be
// orphaned" invariant without adding a separate column.
//
// Fail-closed: when we can't resolve the owner user (lookup error, owner
// soft-deleted, userRepo unwired), we return (true, err) so callers
// refuse the destructive action rather than silently fall through.
// Callers MUST treat any non-nil error as "treat as owner-tenant".
func (s *organizationService) isOwnerTenant(ctx context.Context, org *types.Organization, tenantID uint64) (bool, error) {
if s.userRepo == nil || org == nil {
if org == nil {
// No org to compare against — caller is operating on a request
// that already failed validation; nothing to protect here.
return false, nil
}
if s.userRepo == nil {
// Owner identity unknowable; refuse the destructive op.
return true, errors.New("owner tenant cannot be resolved: userRepo unavailable")
}
owner, err := s.userRepo.GetUserByID(ctx, org.OwnerID)
if err != nil || owner == nil {
return false, err
if err != nil {
return true, err
}
if owner == nil {
return true, errors.New("owner user not found")
}
return owner.TenantID == tenantID, nil
}

13
internal/router/router.go
View File

@@ -823,8 +823,10 @@ func RegisterOrganizationRoutes(r *gin.RouterGroup, orgHandler *handler.Organiza
orgs.DELETE("/:id", orgHandler.DeleteOrganization)
// Leave organization (Admin+ in caller's tenant only)
orgs.POST("/:id/leave", g.Admin(), orgHandler.LeaveOrganization)
// Request role upgrade (for existing members)
orgs.POST("/:id/request-upgrade", orgHandler.RequestRoleUpgrade)
// Request role upgrade (Admin+ in caller's tenant only).
// An upgrade approval changes the whole tenant's org role, so it
// must not be initiated by a tenant Viewer/Contributor.
orgs.POST("/:id/request-upgrade", g.Admin(), orgHandler.RequestRoleUpgrade)
// Generate invite code
orgs.POST("/:id/invite-code", orgHandler.GenerateInviteCode)
// Search users for invite (admin only)
@@ -835,8 +837,11 @@ func RegisterOrganizationRoutes(r *gin.RouterGroup, orgHandler *handler.Organiza
orgs.GET("/:id/members", orgHandler.ListMembers)
// Update member role (path parameter is the member tenant_id)
orgs.PUT("/:id/members/:tenant_id", orgHandler.UpdateMemberRole)
// Remove member (path parameter is the member tenant_id)
orgs.DELETE("/:id/members/:tenant_id", orgHandler.RemoveMember)
// Remove member (path parameter is the member tenant_id).
// Both self-removal (caller's own tenant) and admin-removal-of-other
// take a whole tenant out of the org, so the route must be Admin+
// in the caller's tenant — symmetric with POST /:id/leave above.
orgs.DELETE("/:id/members/:tenant_id", g.Admin(), orgHandler.RemoveMember)
// List join requests (admin only)
orgs.GET("/:id/join-requests", orgHandler.ListJoinRequests)
// Review join request (admin only)