From 8c7000d439dcf65bf687591465db792ae323662a Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Mon, 13 Jul 2026 15:27:48 -0300 Subject: [PATCH 01/15] feat(gitlab): label grants by access path and sync invited-group relationships --- docs/connector.mdx | 6 ++ docs/doc-info.md | 18 ++++++ go.mod | 3 + pkg/config/config.go | 2 +- pkg/connector/client/client.go | 2 +- pkg/connector/client/models.go | 39 ++++++++----- pkg/connector/groups.go | 100 ++++++++++++++++++++++++++------ pkg/connector/helpers.go | 102 +++++++++++++++++++++++++++++++++ pkg/connector/projects.go | 69 ++++++++++++---------- pkg/connector/users.go | 2 +- vendor/modules.txt | 6 ++ 11 files changed, 286 insertions(+), 63 deletions(-) diff --git a/docs/connector.mdx b/docs/connector.mdx index 998a4183..c5d5e34a 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -22,6 +22,12 @@ The GitLab connector supports [automatic account provisioning and deprovisioning Information on last login is synced from self-hosted GitLab instances; this capability is not supported on GitLab.com due to permissions limitations. +### Access paths + +In GitLab a user can obtain access to a project or group in several ways: directly as a member, inherited from a parent or top-level group, or through another group that has been invited (shared) into the project or group. The connector surfaces each of these as a distinct, reviewable access path in C1 rather than flattening them into identical direct grants — so during an access review you can see *how* each user obtained their access. + +To sync only direct memberships and exclude these indirect access paths, enable the **Sync direct members only** setting. + ## Gather GitLab credentials Configuring the connector requires you to pass in credentials generated in GitLab. Gather these credentials before you move on. diff --git a/docs/doc-info.md b/docs/doc-info.md index 8089cd64..e74aad85 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -3,6 +3,24 @@ While developing the connector, please fill out this form. This information is n ## Connector capabilities - Sync Users, projects and groups. +- Labels grants by access path: memberships are surfaced as distinct, reviewable + paths — direct membership, inheritance from a parent/top-level group, and + access through an invited (shared) group — instead of a flattened effective + list. Indirect paths are expressed via expandable grants (group-as-principal + pointing at a group's membership entitlement). The `--sync-direct-members-only` + flag restricts syncing to direct memberships only. + + Invited (shared) group resolution follows GitLab's own sharing semantics, + which differ by target: a group shared into another **group** grants access + only to the invited group's *direct* members, so that path expands to the + invited group's `member` entitlement; a group shared into a **project** grants + access to the invited group's *direct and inherited* members, so that path + expands to the invited group's `effective-member` entitlement (composed from + the group's direct members plus its ancestor-inherited members via expansion, + no extra API calls). Sharing is non-transitive — a group's own inbound (shared) + members are not re-shared onward — so `effective-member` deliberately excludes + them. Parent/top-level inheritance is fully transitive in both cases. + - Supports Account provisioning: When you creating and new account, the following fields are required: - Name: The name of the user. diff --git a/go.mod b/go.mod index 6f91922a..d67136f0 100644 --- a/go.mod +++ b/go.mod @@ -65,6 +65,7 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/protobuf v1.5.4 // indirect github.com/golang/snappy v0.0.5-0.20231225225746-43d5d4cd4e0e // indirect + github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e // indirect github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.27.3 // indirect github.com/hashicorp/hcl v1.0.0 // indirect @@ -123,6 +124,7 @@ require ( go.opentelemetry.io/otel/sdk/log v0.15.0 // indirect go.opentelemetry.io/otel/trace v1.43.0 // indirect go.opentelemetry.io/proto/otlp v1.9.0 // indirect + go.uber.org/goleak v1.3.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.uber.org/ratelimit v0.3.1 // indirect golang.org/x/crypto v0.50.0 // indirect @@ -138,6 +140,7 @@ require ( gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + modernc.org/fileutil v1.4.0 // indirect modernc.org/libc v1.72.0 // indirect modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.11.0 // indirect diff --git a/pkg/config/config.go b/pkg/config/config.go index 7bdb4436..95a3ef44 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -28,7 +28,7 @@ var ( SyncDirectMembersOnly = field.BoolField( "sync-direct-members-only", field.WithDisplayName("Sync direct members only"), - field.WithDescription("When enabled, only sync direct members of groups and projects. Inherited members from parent groups will be excluded from membership grants."), + field.WithDescription("When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded."), ) // ConfigurationFields defines the external configuration required for the diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 803b2a51..542dd981 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -53,7 +53,7 @@ func New(ctx context.Context, accessToken, baseURL, accountCreationGroup string, }, nil } -func (c *GitlabClient) doRequest(ctx context.Context, method string, endpoint string, target interface{}, body interface{}) (*http.Header, *v2.RateLimitDescription, error) { +func (c *GitlabClient) doRequest(ctx context.Context, method string, endpoint string, target any, body any) (*http.Header, *v2.RateLimitDescription, error) { endpoint = fmt.Sprintf("%s%s", c.baseURL, endpoint) relativeURL, err := url.Parse(endpoint) if err != nil { diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index 7f3ab78e..b3b4efcd 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -20,14 +20,26 @@ type PendingInviteUser struct { } type Group struct { - ID int `json:"id"` - Name string `json:"name"` - Description string `json:"description"` - FullName string `json:"full_name"` - ParentID int `json:"parent_id"` - Archived bool `json:"archived"` - Visibility string `json:"visibility"` - MarkedForDeletion *ISOTime `json:"marked_for_deletion"` + ID int `json:"id"` + Name string `json:"name"` + Description string `json:"description"` + FullName string `json:"full_name"` + ParentID int `json:"parent_id"` + Archived bool `json:"archived"` + Visibility string `json:"visibility"` + MarkedForDeletion *ISOTime `json:"marked_for_deletion"` + SharedWithGroups []SharedGroup `json:"shared_with_groups"` +} + +// SharedGroup represents a group that has been invited (shared) into another +// group or project, granting its members access at group_access_level. +// Returned by GET /groups/:id and GET /projects/:id (not by the list endpoints). +type SharedGroup struct { + GroupID int `json:"group_id"` + GroupName string `json:"group_name"` + GroupFullPath string `json:"group_full_path"` + GroupAccessLevel int `json:"group_access_level"` + ExpiresAt *ISOTime `json:"expires_at"` } type Namespace struct { @@ -40,11 +52,12 @@ type Namespace struct { } type Project struct { - ID int `json:"id"` - Name string `json:"name"` - Description string `json:"description"` - NameWithNamespace string `json:"name_with_namespace"` - Namespace *Namespace `json:"namespace"` + ID int `json:"id"` + Name string `json:"name"` + Description string `json:"description"` + NameWithNamespace string `json:"name_with_namespace"` + Namespace *Namespace `json:"namespace"` + SharedWithGroups []SharedGroup `json:"shared_with_groups"` } type GroupMember struct { diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index ac79c095..4af7134e 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -18,6 +18,8 @@ import ( resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -34,6 +36,25 @@ var groupAccessLevels = []client.AccessLevelValue{ client.OwnerPermissions, } +// groupMemberEntitlement is the expansion-only entitlement for a group's DIRECT +// members (the group→group sharing target). Not grantable; Grant is rejected. +const groupMemberEntitlement = "member" + +// groupEffectiveMemberEntitlement is the expansion-only entitlement for a group's +// EFFECTIVE membership (direct + inherited), the group→project sharing target. +// Not grantable; Grant is rejected. See docs/docs-info.md. +const groupEffectiveMemberEntitlement = "effective-member" + +// groupMemberEntitlementID returns a group's member entitlement ID (e.g. "group:g/28:member"). +func groupMemberEntitlementID(groupResourceID string) string { + return fmt.Sprintf("%s:%s:%s", groupResourceType.Id, groupResourceID, groupMemberEntitlement) +} + +// groupEffectiveMemberEntitlementID returns a group's effective-member entitlement ID. +func groupEffectiveMemberEntitlementID(groupResourceID string) string { + return fmt.Sprintf("%s:%s:%s", groupResourceType.Id, groupResourceID, groupEffectiveMemberEntitlement) +} + func (o *groupBuilder) ResourceType(ctx context.Context) *v2.ResourceType { return groupResourceType } @@ -98,7 +119,7 @@ func getParentGroupFromNamespace(namespace *client.Namespace) *v2.ResourceId { } func (o *groupBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { - rv := make([]*v2.Entitlement, 0, len(groupAccessLevels)) + rv := make([]*v2.Entitlement, 0, len(groupAccessLevels)+2) for _, level := range groupAccessLevels { levelName := level.String() @@ -110,14 +131,34 @@ func (o *groupBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ entitlement.WithDescription(fmt.Sprintf("%s on the %s group in Gitlab", levelName, resource.DisplayName)), )) } + + // Expansion-only membership anchors (not grantable, immutable): member = + // direct, effective-member = direct + inherited. Grant on them is rejected. + rv = append(rv, + entitlement.NewAssignmentEntitlement( + resource, + groupMemberEntitlement, + entitlement.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("Direct member of the %s group in Gitlab", resource.DisplayName)), + entitlement.WithAnnotation(&v2.EntitlementImmutable{}), + ), + entitlement.NewAssignmentEntitlement( + resource, + groupEffectiveMemberEntitlement, + entitlement.WithDisplayName(fmt.Sprintf("%s Group Effective Member", resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("Effective member (direct or inherited) of the %s group in Gitlab", resource.DisplayName)), + entitlement.WithAnnotation(&v2.EntitlementImmutable{}), + ), + ) return rv, "", nil, nil } +// Grants emits direct members (access-level + member grants) and, unless +// SyncDirectMembersOnly is set, the indirect access paths (parent inheritance and +// invited groups) as expandable grants so each path stays distinct in C1. func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() - var users []*client.GroupMember - var err error var pageToken string if pToken != nil { @@ -129,17 +170,10 @@ func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken return nil, "", nil, fmt.Errorf("error parsing group resource id: %w", err) } - var nextPageToken string - var rateLimitDesc *v2.RateLimitDescription - if o.client.SyncDirectMembersOnly { - users, nextPageToken, rateLimitDesc, err = o.client.ListGroupMembers(ctx, groupId, pageToken) - } else { - users, nextPageToken, rateLimitDesc, err = o.client.ListAllGroupMembers(ctx, groupId, pageToken) - } + users, nextPageToken, rateLimitDesc, err := o.client.ListGroupMembers(ctx, groupId, pageToken) if rateLimitDesc != nil { outputAnnotations.WithRateLimiting(rateLimitDesc) } - if err != nil { isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupId) if unhandledErr != nil { @@ -156,12 +190,37 @@ func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) } - outGrants = append(outGrants, grant.NewGrant( - resource, - client.AccessLevelValue(user.AccessLevel).String(), - principalId, - )) + outGrants = append(outGrants, + grant.NewGrant(resource, client.AccessLevelValue(user.AccessLevel).String(), principalId), + grant.NewGrant(resource, groupMemberEntitlement, principalId), + ) } + + // Indirect access paths, emitted once on the first page when enabled. + if pageToken == "" && !o.client.SyncDirectMembersOnly { + if parentGroup := resource.ParentResourceId; parentGroup != nil { + outGrants = append(outGrants, parentGroupInheritanceGrants(resource, parentGroup)...) + } + + // This group's effective membership, for when it is invited into a project. + outGrants = append(outGrants, effectiveMemberChainGrants(resource, resource.ParentResourceId)...) + + group, rlDesc, err := o.client.GetGroup(ctx, groupId) + if rlDesc != nil { + outputAnnotations.WithRateLimiting(rlDesc) + } + if err != nil { + _, unhandledErr := handlePermissionError(ctx, err, "group", groupId) + if unhandledErr != nil { + return nil, "", outputAnnotations, unhandledErr + } + // Permission error: skip invited-group grants and continue. + } else { + // Inbound shares (group→group): expand to the invited group's direct members. + outGrants = append(outGrants, sharedGroupGrants(resource, group.SharedWithGroups, groupMemberEntitlement)...) + } + } + return outGrants, nextPageToken, outputAnnotations, nil } @@ -180,6 +239,15 @@ func (o *groupBuilder) Grant( return nil, fmt.Errorf("entitlement cannot be granted: user %q has not yet accepted the invitation to gitlab", principal.Id.Resource) } + // The member/effective-member entitlements are expansion-only; assign a + // specific access level instead. + groupResourceID := entitlement.Resource.Id.Resource + if entitlement.Id == groupMemberEntitlementID(groupResourceID) || entitlement.Id == groupEffectiveMemberEntitlementID(groupResourceID) { + return nil, status.Errorf(codes.InvalidArgument, + "baton-gitlab: the %q entitlement is expansion-only and cannot be granted directly; assign a specific access level instead", + strings.TrimPrefix(entitlement.Id, groupResourceType.Id+":"+groupResourceID+":")) + } + groupIdAndName := entitlement.Resource.Id.Resource groupId, err := fromGroupResourceId(groupIdAndName) if err != nil { diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 6ce2fcfa..04e914fe 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -4,9 +4,12 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "github.com/conductorone/baton-gitlab/pkg/connector/client" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/types/grant" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" "google.golang.org/grpc/codes" @@ -39,6 +42,105 @@ func parseAccessLevelFromEntitlementID(entitlementID string) (int, error) { return int(levelValue), nil } +// inheritedAccessLevels are the levels a parent group confers on child +// subgroups/projects (Minimal and None excluded — they confer no child access). +var inheritedAccessLevels = []client.AccessLevelValue{ + client.GuestPermissions, + client.ReporterPermissions, + client.DeveloperPermissions, + client.MaintainerPermissions, + client.OwnerPermissions, +} + +// parentGroupInheritanceGrants surfaces "access via parent/top-level group" as a +// distinct path: per level, the parent group as principal, expandable to the +// parent's members at that level (non-shallow, so ancestors resolve transitively). +func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { + grants := make([]*v2.Grant, 0, len(inheritedAccessLevels)) + for _, level := range inheritedAccessLevels { + levelName := level.String() + grants = append(grants, grant.NewGrant( + target, + levelName, + parentGroup, + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, parentGroup.Resource, levelName)}, + Shallow: false, + ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, + }), + )) + } + return grants +} + +// sharedGroupGrants emits, for each invited (shared) group, that group as +// principal on the target's access-level entitlement, expandable to the invited +// group's membership. expansionSlug picks which membership entitlement to expand +// into (group→group = member/direct, group→project = effective-member); see +// docs/docs-info.md for the sharing-semantics rationale. +func sharedGroupGrants(target *v2.Resource, shared []client.SharedGroup, expansionSlug string) []*v2.Grant { + grants := make([]*v2.Grant, 0, len(shared)) + for _, sg := range shared { + level := client.AccessLevelValue(sg.GroupAccessLevel) + levelName := level.String() + // Skip levels with no matching entitlement (only Minimal..Owner exist), + // which would otherwise orphan the grant. + if levelName == "" || level < client.MinimalAccessPermissions || level > client.OwnerPermissions { + continue + } + + invitedGroupResourceID := toGroupResourceId(strconv.Itoa(sg.GroupID)) + principalID := &v2.ResourceId{ResourceType: groupResourceType.Id, Resource: invitedGroupResourceID} + + grants = append(grants, grant.NewGrant( + target, + levelName, + principalID, + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, invitedGroupResourceID, expansionSlug)}, + Shallow: false, + ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, + }), + )) + } + return grants +} + +// effectiveMemberChainGrants composes a group's effective-member entitlement +// (direct + ancestor-inherited members) purely via expansion — no extra API call. +// It excludes the group's own inbound shares because group→project sharing is +// non-transitive. See docs/docs-info.md for the semantics. +func effectiveMemberChainGrants(group *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { + expandable := func(srcEntitlementID string) grant.GrantOption { + return grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{srcEntitlementID}, + Shallow: false, + ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, + }) + } + + grants := make([]*v2.Grant, 0, 2) + + // Direct members. + grants = append(grants, grant.NewGrant( + group, + groupEffectiveMemberEntitlement, + group.Id, + expandable(groupMemberEntitlementID(group.Id.Resource)), + )) + + // Inherited members (transitive up the ancestor chain). + if parentGroup != nil { + grants = append(grants, grant.NewGrant( + group, + groupEffectiveMemberEntitlement, + parentGroup, + expandable(groupEffectiveMemberEntitlementID(parentGroup.Resource)), + )) + } + return grants +} + func handlePermissionError(ctx context.Context, err error, resourceType, resourceId string) (bool, error) { if err == nil { return false, nil diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index a468fe5c..b71f3432 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -69,7 +69,7 @@ func (o *projectBuilder) List(ctx context.Context, parentResourceID *v2.Resource parentGroup = parentResourceID } - resource, err := projectResource(project, parentGroup, o.client.IsOnPremise) + resource, err := projectResource(project, parentGroup) if err != nil { return nil, "", outputAnnotations, err } @@ -95,57 +95,64 @@ func (o *projectBuilder) Entitlements(ctx context.Context, resource *v2.Resource return rv, "", nil, nil } +// Grants emits direct members and, unless SyncDirectMembersOnly is set, the +// indirect access paths (parent inheritance and invited groups) as expandable +// grants so each path stays distinct in C1. func (o *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() - var users []*client.ProjectMember - var err error - var nextPageToken string - var rateLimitDesc *v2.RateLimitDescription - if o.client.SyncDirectMembersOnly { - users, nextPageToken, rateLimitDesc, err = o.client.ListProjectMembers(ctx, resource.Id.Resource, pToken.Token) - } else { - users, nextPageToken, rateLimitDesc, err = o.client.ListAllProjectMembers(ctx, resource.Id.Resource, pToken.Token) + var pageToken string + if pToken != nil { + pageToken = pToken.Token } + + users, nextPageToken, rateLimitDesc, err := o.client.ListProjectMembers(ctx, resource.Id.Resource, pageToken) if rateLimitDesc != nil { outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - return nil, "", outputAnnotations, err - } - - groupId := resource.ParentResourceId - if groupId == nil { - return nil, "", outputAnnotations, fmt.Errorf("project resource has no parent group") + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "project", resource.Id.Resource) + if unhandledErr != nil { + return nil, "", outputAnnotations, unhandledErr + } + if isPermissionError { + return nil, "", outputAnnotations, nil + } } for _, user := range users { - entitlementId := fmt.Sprintf("group:%s:%s", groupId.Resource, client.AccessLevelValue(user.AccessLevel).String()) principalId, err := resourceSdk.NewResourceID(userResourceType, user.ID) if err != nil { return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) } - - grantOptions := []grant.GrantOption{ - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{entitlementId}, - Shallow: false, - }), - } - outGrants = append(outGrants, grant.NewGrant( resource, client.AccessLevelValue(user.AccessLevel).String(), principalId, )) + } - outGrants = append(outGrants, grant.NewGrant( - resource, - client.AccessLevelValue(user.AccessLevel).String(), - groupId, - grantOptions..., - )) + // Indirect access paths, emitted once on the first page when enabled. + if pageToken == "" && !o.client.SyncDirectMembersOnly { + if parentGroup := resource.ParentResourceId; parentGroup != nil { + outGrants = append(outGrants, parentGroupInheritanceGrants(resource, parentGroup)...) + } + + project, rlDesc, err := o.client.GetProject(ctx, resource.Id.Resource) + if rlDesc != nil { + outputAnnotations.WithRateLimiting(rlDesc) + } + if err != nil { + _, unhandledErr := handlePermissionError(ctx, err, "project", resource.Id.Resource) + if unhandledErr != nil { + return nil, "", outputAnnotations, unhandledErr + } + // Permission error: skip invited-group grants and continue. + } else { + // Invited groups (group→project): expand to effective membership. + outGrants = append(outGrants, sharedGroupGrants(resource, project.SharedWithGroups, groupEffectiveMemberEntitlement)...) + } } return outGrants, nextPageToken, outputAnnotations, nil @@ -212,7 +219,7 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio return outputAnnotations, nil } -func projectResource(project *client.Project, parentResourceID *v2.ResourceId, isOnPremise bool) (*v2.Resource, error) { +func projectResource(project *client.Project, parentResourceID *v2.ResourceId) (*v2.Resource, error) { var annotations []proto.Message return resourceSdk.NewGroupResource( diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 3a60b1ed..59d807c4 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -464,7 +464,7 @@ func userResource(user any) (*v2.Resource, error) { name = pendingInvitationUser + strings.ToLower(email) } - profile := map[string]interface{}{ + profile := map[string]any{ "first_name": name, "username": username, profileFieldEmail: email, diff --git a/vendor/modules.txt b/vendor/modules.txt index ee8be648..e85849b5 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -420,6 +420,8 @@ github.com/golang/protobuf/ptypes/timestamp # github.com/golang/snappy v0.0.5-0.20231225225746-43d5d4cd4e0e ## explicit github.com/golang/snappy +# github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e +## explicit; go 1.23 # github.com/google/uuid v1.6.0 ## explicit github.com/google/uuid @@ -730,6 +732,8 @@ go.opentelemetry.io/proto/otlp/common/v1 go.opentelemetry.io/proto/otlp/logs/v1 go.opentelemetry.io/proto/otlp/resource/v1 go.opentelemetry.io/proto/otlp/trace/v1 +# go.uber.org/goleak v1.3.0 +## explicit; go 1.20 # go.uber.org/multierr v1.11.0 ## explicit; go 1.19 go.uber.org/multierr @@ -947,6 +951,8 @@ gopkg.in/yaml.v2 # gopkg.in/yaml.v3 v3.0.1 ## explicit gopkg.in/yaml.v3 +# modernc.org/fileutil v1.4.0 +## explicit; go 1.24 # modernc.org/libc v1.72.0 ## explicit; go 1.25.0 modernc.org/libc From d4a26d10cbd53054e8308f0313eec56b494b715b Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Mon, 13 Jul 2026 15:29:42 -0300 Subject: [PATCH 02/15] docs(gitlab): fix doc-info.md path in code comment references The condensed comments pointed at docs/docs-info.md, but the file is docs/doc-info.md (singular). Fixes the three broken references flagged in review. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/groups.go | 2 +- pkg/connector/helpers.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 4af7134e..d8cd1f04 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -42,7 +42,7 @@ const groupMemberEntitlement = "member" // groupEffectiveMemberEntitlement is the expansion-only entitlement for a group's // EFFECTIVE membership (direct + inherited), the group→project sharing target. -// Not grantable; Grant is rejected. See docs/docs-info.md. +// Not grantable; Grant is rejected. See docs/doc-info.md. const groupEffectiveMemberEntitlement = "effective-member" // groupMemberEntitlementID returns a group's member entitlement ID (e.g. "group:g/28:member"). diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 04e914fe..5a426a3e 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -77,7 +77,7 @@ func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceI // principal on the target's access-level entitlement, expandable to the invited // group's membership. expansionSlug picks which membership entitlement to expand // into (group→group = member/direct, group→project = effective-member); see -// docs/docs-info.md for the sharing-semantics rationale. +// docs/doc-info.md for the sharing-semantics rationale. func sharedGroupGrants(target *v2.Resource, shared []client.SharedGroup, expansionSlug string) []*v2.Grant { grants := make([]*v2.Grant, 0, len(shared)) for _, sg := range shared { @@ -109,7 +109,7 @@ func sharedGroupGrants(target *v2.Resource, shared []client.SharedGroup, expansi // effectiveMemberChainGrants composes a group's effective-member entitlement // (direct + ancestor-inherited members) purely via expansion — no extra API call. // It excludes the group's own inbound shares because group→project sharing is -// non-transitive. See docs/docs-info.md for the semantics. +// non-transitive. See docs/doc-info.md for the semantics. func effectiveMemberChainGrants(group *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { expandable := func(srcEntitlementID string) grant.GrantOption { return grant.WithAnnotation(&v2.GrantExpandable{ From 5d9c2c1a1e4586b99ee2b881a77d21ea2762841c Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Mon, 13 Jul 2026 15:48:03 -0300 Subject: [PATCH 03/15] docs(gitlab): document shared-group access-level upper-bound limitation On the invited-group path, expanded members are shown at the share's group_access_level (an upper bound); GitLab caps effective access at min(member role, group_access_level). Documenting the trade-off inherent to the expansion model, per review feedback. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/doc-info.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/doc-info.md b/docs/doc-info.md index e74aad85..ea8286c3 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -21,6 +21,16 @@ While developing the connector, please fill out this form. This information is n members are not re-shared onward — so `effective-member` deliberately excludes them. Parent/top-level inheritance is fully transitive in both cases. + Known limitation: on the invited (shared) group path, expanded members are + shown at the share's `group_access_level`, which is an upper bound. GitLab caps + a shared member's effective access at `min(role in the invited group, + group_access_level)`, so a member whose role in the invited group is below the + share level may be shown one level higher on the shared path. This is a + consequence of expressing the path via expansion (`GrantExpandable` copies + principals without recomputing per-member levels). The direct and + parent/top-level inheritance paths preserve exact access levels; only the + invited-group path uses the share level. + - Supports Account provisioning: When you creating and new account, the following fields are required: - Name: The name of the user. From 0fce1595fdc33adc629bacecb6ceb7bf06e6b092 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Mon, 13 Jul 2026 16:32:52 -0300 Subject: [PATCH 04/15] chore(gitlab): log invited groups skipped for unsupported access level Surface, at Debug level, invited (shared) groups dropped in sharedGroupGrants when their group_access_level falls outside the Minimal..Owner range (previously a silent continue), so a share whose grant would otherwise be orphaned is observable. Thread ctx through sharedGroupGrants for the logger, and document why the shared path accepts the full Minimal..Owner range while parent inheritance excludes Minimal. Also downgrade two non-fatal Warn logs to Debug (permission- denied skip and the grant email-invite fallback) per the log-level convention. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/groups.go | 4 ++-- pkg/connector/helpers.go | 15 +++++++++++++-- pkg/connector/projects.go | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index d8cd1f04..1174ee70 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -217,7 +217,7 @@ func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken // Permission error: skip invited-group grants and continue. } else { // Inbound shares (group→group): expand to the invited group's direct members. - outGrants = append(outGrants, sharedGroupGrants(resource, group.SharedWithGroups, groupMemberEntitlement)...) + outGrants = append(outGrants, sharedGroupGrants(ctx, resource, group.SharedWithGroups, groupMemberEntitlement)...) } } @@ -275,7 +275,7 @@ func (o *groupBuilder) Grant( userId, err := strconv.Atoi(principal.Id.Resource) if err != nil { - l.Warn("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) + l.Debug("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) ut, err := resourceSdk.GetUserTrait(principal) if err != nil { return nil, fmt.Errorf("baton-gitlab: error getting user trait: %w", err) diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 5a426a3e..afd9f67a 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -78,7 +78,13 @@ func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceI // group's membership. expansionSlug picks which membership entitlement to expand // into (group→group = member/direct, group→project = effective-member); see // docs/doc-info.md for the sharing-semantics rationale. -func sharedGroupGrants(target *v2.Resource, shared []client.SharedGroup, expansionSlug string) []*v2.Grant { +// +// Unlike parent inheritance (which excludes Minimal because it confers no child +// access), a share carries whatever group_access_level GitLab reports, so the +// full Minimal..Owner range is accepted here — both groups and projects expose a +// Minimal access-level entitlement, so the grant still resolves. +func sharedGroupGrants(ctx context.Context, target *v2.Resource, shared []client.SharedGroup, expansionSlug string) []*v2.Grant { + l := ctxzap.Extract(ctx) grants := make([]*v2.Grant, 0, len(shared)) for _, sg := range shared { level := client.AccessLevelValue(sg.GroupAccessLevel) @@ -86,6 +92,11 @@ func sharedGroupGrants(target *v2.Resource, shared []client.SharedGroup, expansi // Skip levels with no matching entitlement (only Minimal..Owner exist), // which would otherwise orphan the grant. if levelName == "" || level < client.MinimalAccessPermissions || level > client.OwnerPermissions { + l.Debug("baton-gitlab: skipping shared group with unsupported access level", + zap.String("target", target.Id.Resource), + zap.Int("shared_group_id", sg.GroupID), + zap.Int("group_access_level", sg.GroupAccessLevel), + ) continue } @@ -148,7 +159,7 @@ func handlePermissionError(ctx context.Context, err error, resourceType, resourc if status.Code(err) == codes.PermissionDenied || errors.Is(err, client.ErrForbidden) { l := ctxzap.Extract(ctx) - l.Warn( + l.Debug( fmt.Sprintf("Permission denied while listing members for %s. Skipping.", resourceType), zap.String(fmt.Sprintf("%s_id", resourceType), resourceId), ) diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index b71f3432..c10d334b 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -151,7 +151,7 @@ func (o *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pTok // Permission error: skip invited-group grants and continue. } else { // Invited groups (group→project): expand to effective membership. - outGrants = append(outGrants, sharedGroupGrants(resource, project.SharedWithGroups, groupEffectiveMemberEntitlement)...) + outGrants = append(outGrants, sharedGroupGrants(ctx, resource, project.SharedWithGroups, groupEffectiveMemberEntitlement)...) } } From 4b85231ce2aa05fe2cf5c21f5cd8e84bcb78bef2 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 14 Jul 2026 17:30:14 -0300 Subject: [PATCH 05/15] feat(gitlab): gate access-path labeling behind opt-in sync-access-paths flag The path-visibility model (label grants by access path + sync invited groups, CXH-1974) changed grant shape and counts on the default path. Gate all of it behind a new sync-access-paths flag (default off) so existing deployments upgrade with zero change: flag-off reproduces the pre-existing flattened effective-membership behavior byte-for-byte (verified against released v0.0.31), and only flag-on emits the path-labeled expandable grants and the member/effective-member anchors. Regenerate config schema; align code comments, README, connector.mdx and doc-info.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 11 ++++ cmd/baton-gitlab/main.go | 1 + config_schema.json | 2 +- docs/connector.mdx | 6 +- docs/doc-info.md | 18 ++++-- pkg/config/conf.gen.go | 1 + pkg/config/config.go | 6 ++ pkg/connector/client/client.go | 4 +- pkg/connector/connector.go | 4 +- pkg/connector/groups.go | 113 +++++++++++++++++++++++++++------ pkg/connector/helpers.go | 7 +- pkg/connector/projects.go | 84 +++++++++++++++++++++++- 12 files changed, 220 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index c9d2bf5f..fd997354 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,15 @@ To use this connector, you will need different things depending on which version ## Connector capabilities - Sync Users, projects and groups. +- Optional access-path labeling (opt-in via `--sync-access-paths`, off by default): + when enabled, group/project access is surfaced as distinct, reviewable paths — + direct membership, inheritance from a parent/top-level group, and access via an + invited (shared) group — instead of a flattened effective list. When disabled + (the default), effective membership is emitted as flattened direct grants, + preserving the previous grant shape and counts. The `--sync-direct-members-only` + flag is separate and, in either mode, restricts syncing to direct memberships + only (excluding all inherited/shared access). + - Supports Account provisioning: When you creating and new account, the following fields are required: - Name: The name of the user. @@ -130,6 +139,8 @@ Flags: --otel-collector-endpoint string The endpoint of the OpenTelemetry collector to send observability data to ($BATON_OTEL_COLLECTOR_ENDPOINT) -p, --provisioning This must be set in order for provisioning actions to be enabled ($BATON_PROVISIONING) --skip-full-sync This must be set to skip a full sync ($BATON_SKIP_FULL_SYNC) + --sync-access-paths Label grants by access path (direct, inherited, or invited group). Disabled (default) keeps the previous flattened effective-membership grants. ($BATON_SYNC_ACCESS_PATHS) + --sync-direct-members-only When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded. ($BATON_SYNC_DIRECT_MEMBERS_ONLY) --ticketing This must be set to enable ticketing support ($BATON_TICKETING) -v, --version version for baton-gitlab diff --git a/cmd/baton-gitlab/main.go b/cmd/baton-gitlab/main.go index 638a2190..2c1e50db 100644 --- a/cmd/baton-gitlab/main.go +++ b/cmd/baton-gitlab/main.go @@ -54,6 +54,7 @@ func getConnector(ctx context.Context, glc *cfg.Gitlab) (types.ConnectorServer, glc.BaseUrl, glc.AccountCreationGroup, glc.SyncDirectMembersOnly, + glc.SyncAccessPaths, ) if err != nil { diff --git a/config_schema.json b/config_schema.json index 4814afb9..55fa2041 100644 --- a/config_schema.json +++ b/config_schema.json @@ -124,7 +124,7 @@ { "name": "sync-direct-members-only", "displayName": "Sync direct members only", - "description": "When enabled, only sync direct members of groups and projects. Inherited members from parent groups will be excluded from membership grants.", + "description": "When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded.", "boolField": {} } ], diff --git a/docs/connector.mdx b/docs/connector.mdx index c5d5e34a..e892c933 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -24,9 +24,11 @@ Information on last login is synced from self-hosted GitLab instances; this capa ### Access paths -In GitLab a user can obtain access to a project or group in several ways: directly as a member, inherited from a parent or top-level group, or through another group that has been invited (shared) into the project or group. The connector surfaces each of these as a distinct, reviewable access path in C1 rather than flattening them into identical direct grants — so during an access review you can see *how* each user obtained their access. +In GitLab a user can obtain access to a project or group in several ways: directly as a member, inherited from a parent or top-level group, or through another group that has been invited (shared) into the project or group. -To sync only direct memberships and exclude these indirect access paths, enable the **Sync direct members only** setting. +By default the connector emits effective membership as flattened direct grants, so every access path looks the same. Enable the **Sync access paths** setting to instead surface each path as a distinct, reviewable access path in C1 — so during an access review you can see *how* each user obtained their access. This setting is opt-in because it changes the shape and number of grants; existing connectors are unaffected until it is enabled. + +Separately, enable the **Sync direct members only** setting to sync only direct memberships and exclude indirect access (parent-group inheritance and invited/shared groups) entirely. It applies whether or not **Sync access paths** is enabled. ## Gather GitLab credentials diff --git a/docs/doc-info.md b/docs/doc-info.md index ea8286c3..23ece39a 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -3,12 +3,18 @@ While developing the connector, please fill out this form. This information is n ## Connector capabilities - Sync Users, projects and groups. -- Labels grants by access path: memberships are surfaced as distinct, reviewable - paths — direct membership, inheritance from a parent/top-level group, and - access through an invited (shared) group — instead of a flattened effective - list. Indirect paths are expressed via expandable grants (group-as-principal - pointing at a group's membership entitlement). The `--sync-direct-members-only` - flag restricts syncing to direct memberships only. +- Optionally labels grants by access path (opt-in via `--sync-access-paths`, + disabled by default): when enabled, memberships are surfaced as distinct, + reviewable paths — direct membership, inheritance from a parent/top-level + group, and access through an invited (shared) group — instead of a flattened + effective list. Indirect paths are expressed via expandable grants + (group-as-principal pointing at a group's membership entitlement), and two + expansion-only entitlements (`member`, `effective-member`) are added to groups. + When disabled (the default), the connector emits effective membership as + flattened direct grants, preserving the previous grant shape and counts — so + existing deployments see no change unless they opt in. The + `--sync-direct-members-only` flag restricts syncing to direct memberships only + and applies in both modes. Invited (shared) group resolution follows GitLab's own sharing semantics, which differ by target: a group shared into another **group** grants access diff --git a/pkg/config/conf.gen.go b/pkg/config/conf.gen.go index b0fa6667..2c082647 100644 --- a/pkg/config/conf.gen.go +++ b/pkg/config/conf.gen.go @@ -8,6 +8,7 @@ type Gitlab struct { BaseUrl string `mapstructure:"base-url"` AccountCreationGroup string `mapstructure:"account-creation-group"` SyncDirectMembersOnly bool `mapstructure:"sync-direct-members-only"` + SyncAccessPaths bool `mapstructure:"sync-access-paths"` } func (c* Gitlab) findFieldByTag(tagValue string) (any, bool) { diff --git a/pkg/config/config.go b/pkg/config/config.go index 95a3ef44..fc817c2c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -30,6 +30,11 @@ var ( field.WithDisplayName("Sync direct members only"), field.WithDescription("When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded."), ) + SyncAccessPaths = field.BoolField( + "sync-access-paths", + field.WithDisplayName("Sync access paths"), + field.WithDescription("Label grants by access path (direct, inherited, or invited group). Disabled (default) keeps the previous flattened effective-membership grants."), + ) // ConfigurationFields defines the external configuration required for the // connector to run. Note: these fields can be marked as optional or @@ -39,6 +44,7 @@ var ( BaseURL, AccountCreationGroup, SyncDirectMembersOnly, + SyncAccessPaths, } // FieldRelationships defines relationships between the fields listed in diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 542dd981..8317a780 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -26,9 +26,10 @@ type GitlabClient struct { IsOnPremise bool accessToken string SyncDirectMembersOnly bool + SyncAccessPaths bool } -func New(ctx context.Context, accessToken, baseURL, accountCreationGroup string, syncDirectMembersOnly bool) (*GitlabClient, error) { +func New(ctx context.Context, accessToken, baseURL, accountCreationGroup string, syncDirectMembersOnly, syncAccessPaths bool) (*GitlabClient, error) { options := []uhttp.Option{uhttp.WithLogger(true, ctxzap.Extract(ctx)), uhttp.WithUserAgent("baton-gitlab/1.0")} client, err := uhttp.NewClient(ctx, options...) @@ -50,6 +51,7 @@ func New(ctx context.Context, accessToken, baseURL, accountCreationGroup string, IsOnPremise: baseURLTrimmed != "https://gitlab.com", accessToken: accessToken, SyncDirectMembersOnly: syncDirectMembersOnly, + SyncAccessPaths: syncAccessPaths, }, nil } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 198f33dd..9779fe41 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -131,10 +131,10 @@ func (d *Connector) Validate(ctx context.Context) (annotations.Annotations, erro } // New returns a new instance of the connector. -func New(ctx context.Context, accessToken string, baseURL string, accountCreationGroup string, syncDirectMembersOnly bool) (*Connector, error) { +func New(ctx context.Context, accessToken string, baseURL string, accountCreationGroup string, syncDirectMembersOnly, syncAccessPaths bool) (*Connector, error) { l := ctxzap.Extract(ctx) - gitlabClient, err := client.New(ctx, accessToken, baseURL, accountCreationGroup, syncDirectMembersOnly) + gitlabClient, err := client.New(ctx, accessToken, baseURL, accountCreationGroup, syncDirectMembersOnly, syncAccessPaths) if err != nil { l.Error("error creating gitlab client", zap.Error(err)) return nil, err diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 1174ee70..78ec2690 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -38,11 +38,13 @@ var groupAccessLevels = []client.AccessLevelValue{ // groupMemberEntitlement is the expansion-only entitlement for a group's DIRECT // members (the group→group sharing target). Not grantable; Grant is rejected. +// Only emitted when the SyncAccessPaths flag is enabled. const groupMemberEntitlement = "member" // groupEffectiveMemberEntitlement is the expansion-only entitlement for a group's // EFFECTIVE membership (direct + inherited), the group→project sharing target. -// Not grantable; Grant is rejected. See docs/doc-info.md. +// Not grantable; Grant is rejected. Only emitted when the SyncAccessPaths flag is +// enabled. See docs/doc-info.md. const groupEffectiveMemberEntitlement = "effective-member" // groupMemberEntitlementID returns a group's member entitlement ID (e.g. "group:g/28:member"). @@ -134,29 +136,100 @@ func (o *groupBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ // Expansion-only membership anchors (not grantable, immutable): member = // direct, effective-member = direct + inherited. Grant on them is rejected. - rv = append(rv, - entitlement.NewAssignmentEntitlement( - resource, - groupMemberEntitlement, - entitlement.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), - entitlement.WithDescription(fmt.Sprintf("Direct member of the %s group in Gitlab", resource.DisplayName)), - entitlement.WithAnnotation(&v2.EntitlementImmutable{}), - ), - entitlement.NewAssignmentEntitlement( - resource, - groupEffectiveMemberEntitlement, - entitlement.WithDisplayName(fmt.Sprintf("%s Group Effective Member", resource.DisplayName)), - entitlement.WithDescription(fmt.Sprintf("Effective member (direct or inherited) of the %s group in Gitlab", resource.DisplayName)), - entitlement.WithAnnotation(&v2.EntitlementImmutable{}), - ), - ) + // Only emitted in access-path mode; without it the connector's entitlement + // surface stays identical to the flattened-membership behavior. + if o.client.SyncAccessPaths { + rv = append(rv, + entitlement.NewAssignmentEntitlement( + resource, + groupMemberEntitlement, + entitlement.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("Direct member of the %s group in Gitlab", resource.DisplayName)), + entitlement.WithAnnotation(&v2.EntitlementImmutable{}), + ), + entitlement.NewAssignmentEntitlement( + resource, + groupEffectiveMemberEntitlement, + entitlement.WithDisplayName(fmt.Sprintf("%s Group Effective Member", resource.DisplayName)), + entitlement.WithDescription(fmt.Sprintf("Effective member (direct or inherited) of the %s group in Gitlab", resource.DisplayName)), + entitlement.WithAnnotation(&v2.EntitlementImmutable{}), + ), + ) + } return rv, "", nil, nil } -// Grants emits direct members (access-level + member grants) and, unless -// SyncDirectMembersOnly is set, the indirect access paths (parent inheritance and -// invited groups) as expandable grants so each path stays distinct in C1. +// Grants dispatches on the SyncAccessPaths flag. With it disabled (default) the +// connector emits flattened effective membership as before; with it enabled each +// access path is surfaced as a distinct expandable grant. func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { + if o.client.SyncAccessPaths { + return o.grantsWithAccessPaths(ctx, resource, pToken) + } + return o.grantsFlattened(ctx, resource, pToken) +} + +// grantsFlattened emits effective membership as flat direct grants — the +// pre-access-path behavior, preserved unchanged for existing customers. When +// SyncDirectMembersOnly is set, only direct members are listed; otherwise the +// full effective set (direct + inherited) is flattened via ListAllGroupMembers. +func (o *groupBuilder) grantsFlattened(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { + var outGrants []*v2.Grant + var outputAnnotations = annotations.New() + + var pageToken string + if pToken != nil { + pageToken = pToken.Token + } + + groupId, err := fromGroupResourceId(resource.Id.Resource) + if err != nil { + return nil, "", nil, fmt.Errorf("error parsing group resource id: %w", err) + } + + var ( + users []*client.GroupMember + nextPageToken string + rateLimitDesc *v2.RateLimitDescription + ) + if o.client.SyncDirectMembersOnly { + users, nextPageToken, rateLimitDesc, err = o.client.ListGroupMembers(ctx, groupId, pageToken) + } else { + users, nextPageToken, rateLimitDesc, err = o.client.ListAllGroupMembers(ctx, groupId, pageToken) + } + if rateLimitDesc != nil { + outputAnnotations.WithRateLimiting(rateLimitDesc) + } + if err != nil { + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupId) + if unhandledErr != nil { + return nil, "", outputAnnotations, unhandledErr + } + if isPermissionError { + return nil, "", outputAnnotations, nil + } + } + + for _, user := range users { + principalId, err := resourceSdk.NewResourceID(userResourceType, user.ID) + if err != nil { + return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) + } + outGrants = append(outGrants, grant.NewGrant( + resource, + client.AccessLevelValue(user.AccessLevel).String(), + principalId, + )) + } + + return outGrants, nextPageToken, outputAnnotations, nil +} + +// grantsWithAccessPaths emits direct members (access-level + member grants) and, +// unless SyncDirectMembersOnly is set, the indirect access paths (parent +// inheritance and invited groups) as expandable grants so each path stays +// distinct in C1. +func (o *groupBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index afd9f67a..2414cea3 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -55,6 +55,7 @@ var inheritedAccessLevels = []client.AccessLevelValue{ // parentGroupInheritanceGrants surfaces "access via parent/top-level group" as a // distinct path: per level, the parent group as principal, expandable to the // parent's members at that level (non-shallow, so ancestors resolve transitively). +// Only used in access-path mode (SyncAccessPaths). func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { grants := make([]*v2.Grant, 0, len(inheritedAccessLevels)) for _, level := range inheritedAccessLevels { @@ -77,7 +78,8 @@ func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceI // principal on the target's access-level entitlement, expandable to the invited // group's membership. expansionSlug picks which membership entitlement to expand // into (group→group = member/direct, group→project = effective-member); see -// docs/doc-info.md for the sharing-semantics rationale. +// docs/doc-info.md for the sharing-semantics rationale. Only used in access-path +// mode (SyncAccessPaths). // // Unlike parent inheritance (which excludes Minimal because it confers no child // access), a share carries whatever group_access_level GitLab reports, so the @@ -120,7 +122,8 @@ func sharedGroupGrants(ctx context.Context, target *v2.Resource, shared []client // effectiveMemberChainGrants composes a group's effective-member entitlement // (direct + ancestor-inherited members) purely via expansion — no extra API call. // It excludes the group's own inbound shares because group→project sharing is -// non-transitive. See docs/doc-info.md for the semantics. +// non-transitive. See docs/doc-info.md for the semantics. Only used in access-path +// mode (SyncAccessPaths). func effectiveMemberChainGrants(group *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { expandable := func(srcEntitlementID string) grant.GrantOption { return grant.WithAnnotation(&v2.GrantExpandable{ diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index c10d334b..57d043df 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -95,10 +95,88 @@ func (o *projectBuilder) Entitlements(ctx context.Context, resource *v2.Resource return rv, "", nil, nil } -// Grants emits direct members and, unless SyncDirectMembersOnly is set, the -// indirect access paths (parent inheritance and invited groups) as expandable -// grants so each path stays distinct in C1. +// Grants dispatches on the SyncAccessPaths flag. With it disabled (default) the +// connector emits flattened effective membership as before; with it enabled each +// access path is surfaced as a distinct expandable grant. func (o *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { + if o.client.SyncAccessPaths { + return o.grantsWithAccessPaths(ctx, resource, pToken) + } + return o.grantsFlattened(ctx, resource, pToken) +} + +// grantsFlattened emits effective membership as flat direct grants plus a +// parent-group expandable grant per member — the pre-access-path behavior, +// preserved unchanged for existing customers. When SyncDirectMembersOnly is set, +// only direct members are listed; otherwise the full effective set is flattened +// via ListAllProjectMembers. +func (o *projectBuilder) grantsFlattened(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { + var outGrants []*v2.Grant + var outputAnnotations = annotations.New() + + var pageToken string + if pToken != nil { + pageToken = pToken.Token + } + + var ( + users []*client.ProjectMember + nextPageToken string + rateLimitDesc *v2.RateLimitDescription + err error + ) + if o.client.SyncDirectMembersOnly { + users, nextPageToken, rateLimitDesc, err = o.client.ListProjectMembers(ctx, resource.Id.Resource, pageToken) + } else { + users, nextPageToken, rateLimitDesc, err = o.client.ListAllProjectMembers(ctx, resource.Id.Resource, pageToken) + } + if rateLimitDesc != nil { + outputAnnotations.WithRateLimiting(rateLimitDesc) + } + if err != nil { + return nil, "", outputAnnotations, err + } + + groupId := resource.ParentResourceId + if groupId == nil { + return nil, "", outputAnnotations, fmt.Errorf("project resource has no parent group") + } + + for _, user := range users { + entitlementId := fmt.Sprintf("group:%s:%s", groupId.Resource, client.AccessLevelValue(user.AccessLevel).String()) + principalId, err := resourceSdk.NewResourceID(userResourceType, user.ID) + if err != nil { + return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) + } + + grantOptions := []grant.GrantOption{ + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: []string{entitlementId}, + Shallow: false, + }), + } + + outGrants = append(outGrants, grant.NewGrant( + resource, + client.AccessLevelValue(user.AccessLevel).String(), + principalId, + )) + + outGrants = append(outGrants, grant.NewGrant( + resource, + client.AccessLevelValue(user.AccessLevel).String(), + groupId, + grantOptions..., + )) + } + + return outGrants, nextPageToken, outputAnnotations, nil +} + +// grantsWithAccessPaths emits direct members and, unless SyncDirectMembersOnly is +// set, the indirect access paths (parent inheritance and invited groups) as +// expandable grants so each path stays distinct in C1. +func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() From 872ce028584020ccdd0ff890ed1a9451f978b4d8 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 15 Jul 2026 10:00:01 -0300 Subject: [PATCH 06/15] fix(gitlab): emit pure-expansion access-path anchors on member permission-error and guard expansion-only revoke On a member-listing permission error, grantsWithAccessPaths returned early and dropped the pure-expansion indirect anchors (parent inheritance, effective-member chain) even though those need no member data. Since the access-path path no longer flattens via /members/all, a project/subgroup inheriting from or invited by a group whose members are unreadable lost that path silently. Skip only the direct-member enumeration on permission error and still emit the anchors. Also add a defensive guard in groupBuilder.Revoke rejecting the expansion-only member/effective-member entitlements with InvalidArgument, mirroring Grant, so a revoke can never call RemoveGroupMember on an attribution-only anchor that carries real user principals. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/groups.go | 22 ++++++++++++++++++---- pkg/connector/projects.go | 11 +++++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 78ec2690..91d1e4c2 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -248,13 +248,16 @@ func (o *groupBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.R outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupId) + _, unhandledErr := handlePermissionError(ctx, err, "group", groupId) if unhandledErr != nil { return nil, "", outputAnnotations, unhandledErr } - if isPermissionError { - return nil, "", outputAnnotations, nil - } + // Permission error listing members: skip direct members but still emit the + // pure-expansion indirect anchors below, which need no member data. Otherwise + // projects/subgroups that inherit from or are invited by this group silently + // lose those paths, since they now resolve solely by expanding this group's + // entitlements rather than by flattening via /members/all. + users, nextPageToken = nil, "" } for _, user := range users { @@ -401,6 +404,17 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations var outputAnnotations = annotations.New() groupIdAndName := grant.Entitlement.Resource.Id.Resource + + // The member/effective-member entitlements are expansion-only attribution + // anchors that carry real user principals; revoking them must never remove a + // real group membership. Mirror the guard in Grant (EntitlementImmutable should + // already prevent this path, but guard defensively). + if grant.Entitlement.Id == groupMemberEntitlementID(groupIdAndName) || grant.Entitlement.Id == groupEffectiveMemberEntitlementID(groupIdAndName) { + return nil, status.Errorf(codes.InvalidArgument, + "baton-gitlab: the %q entitlement is expansion-only and cannot be revoked directly; it maps to no assignable access level", + strings.TrimPrefix(grant.Entitlement.Id, groupResourceType.Id+":"+groupIdAndName+":")) + } + groupId, err := fromGroupResourceId(groupIdAndName) if err != nil { return nil, fmt.Errorf("error parsing group resource id: %w", err) diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index 57d043df..84fdf3f5 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -190,13 +190,16 @@ func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2 outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - isPermissionError, unhandledErr := handlePermissionError(ctx, err, "project", resource.Id.Resource) + _, unhandledErr := handlePermissionError(ctx, err, "project", resource.Id.Resource) if unhandledErr != nil { return nil, "", outputAnnotations, unhandledErr } - if isPermissionError { - return nil, "", outputAnnotations, nil - } + // Permission error listing members: skip direct members but still emit the + // pure-expansion parent-inheritance anchor below, which needs no member data. + // Otherwise a project whose members are unreadable loses its inherited-access + // path entirely, since it now resolves by expanding group:: + // rather than by flattening via /members/all. + users, nextPageToken = nil, "" } for _, user := range users { From e712ebcdb4020aa11f658aae71b527536dbd09a2 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 15 Jul 2026 15:47:20 -0300 Subject: [PATCH 07/15] feat(gitlab): rework access-path labeling to a lean expandable-grant model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reimplements the CXH-1974 access-path feature (opt-in via --sync-access-paths) with a leaner model that adds no synthetic entitlements and emits no empty expansions. - Inheritance: emit one expandable grant per ancestor group, only for the access levels that group actually has members at, expanding into the existing per-level entitlements (Shallow=true — one crisp hop per ancestor; the platform resolves deeper nesting). - Invited (shared) groups: map each member's level to min(level, share level) and expand the invited group's matching per-level entitlements. No synthetic member/effective-member anchor entitlements are added. - Revoke of inherited/invited access now rejects with InvalidArgument instead of a silent no-op that reappears on the next sync (verifies effective membership via GET .../members/all/:user_id). The default (flag off) output is unchanged byte-for-byte (81 grants / 120 entitlements on the test tenant). With the flag on: 108 grants / 120 entitlements, 0 orphans, 0 empty expansions. Also extracts repeated string literals into constants (goconst) and documents the model in README.md, docs/doc-info.md and docs/connector.mdx. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 25 +-- config_schema.json | 2 +- docs/connector.mdx | 10 +- docs/doc-info.md | 74 ++++---- pkg/config/config.go | 4 +- pkg/connector/client/client.go | 34 +++- pkg/connector/client/helper.go | 3 + pkg/connector/client/models.go | 15 +- pkg/connector/groups.go | 151 ++++++--------- pkg/connector/helpers.go | 319 ++++++++++++++++++++++---------- pkg/connector/projects.go | 83 +++++---- pkg/connector/resource_types.go | 7 + pkg/connector/users.go | 2 +- 13 files changed, 432 insertions(+), 297 deletions(-) diff --git a/README.md b/README.md index fd997354..7b456984 100644 --- a/README.md +++ b/README.md @@ -25,15 +25,6 @@ To use this connector, you will need different things depending on which version ## Connector capabilities - Sync Users, projects and groups. -- Optional access-path labeling (opt-in via `--sync-access-paths`, off by default): - when enabled, group/project access is surfaced as distinct, reviewable paths — - direct membership, inheritance from a parent/top-level group, and access via an - invited (shared) group — instead of a flattened effective list. When disabled - (the default), effective membership is emitted as flattened direct grants, - preserving the previous grant shape and counts. The `--sync-direct-members-only` - flag is separate and, in either mode, restricts syncing to direct memberships - only (excluding all inherited/shared access). - - Supports Account provisioning: When you creating and new account, the following fields are required: - Name: The name of the user. @@ -57,6 +48,18 @@ To use this connector, you will need different things depending on which version because admin permissions are needed and in the cloud version they do not exist. https://docs.gitlab.com/api/users/ +- Optional access-path labeling (`--sync-access-paths`, disabled by default): + When enabled, grants are labeled by HOW access was obtained instead of a single + flattened list. Direct membership, inheritance from a parent/top-level group, and + access via an invited (shared) group each become a distinct, reviewable path + (expandable grants). The default output is unchanged — identical grant shape and + counts — so existing deployments are unaffected until they opt in. It reuses the + existing per-access-level entitlements (no new entitlements) and emits paths only + for access levels that actually have members (no empty expansions). + `--sync-direct-members-only` restricts the sync to direct members only and + suppresses the inherited/invited paths. See `docs/doc-info.md` for the full model + and its one known limitation. + ## Where can I find my API Key? 1- Log in gitlab.com o in your base url, then go to the top left, click on the user emoticon, a popup menu will open, click on edit profile. 2- In the dashboard to the left of User settings, click on access tokens, and in the list of tokens that appears, click on add new token. @@ -139,8 +142,8 @@ Flags: --otel-collector-endpoint string The endpoint of the OpenTelemetry collector to send observability data to ($BATON_OTEL_COLLECTOR_ENDPOINT) -p, --provisioning This must be set in order for provisioning actions to be enabled ($BATON_PROVISIONING) --skip-full-sync This must be set to skip a full sync ($BATON_SKIP_FULL_SYNC) - --sync-access-paths Label grants by access path (direct, inherited, or invited group). Disabled (default) keeps the previous flattened effective-membership grants. ($BATON_SYNC_ACCESS_PATHS) - --sync-direct-members-only When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded. ($BATON_SYNC_DIRECT_MEMBERS_ONLY) + --sync-access-paths Label grants by access path (direct, inherited, or via an invited group). Disabled by default. ($BATON_SYNC_ACCESS_PATHS) + --sync-direct-members-only When enabled, only sync direct members of groups and projects. ($BATON_SYNC_DIRECT_MEMBERS_ONLY) --ticketing This must be set to enable ticketing support ($BATON_TICKETING) -v, --version version for baton-gitlab diff --git a/config_schema.json b/config_schema.json index 55fa2041..4814afb9 100644 --- a/config_schema.json +++ b/config_schema.json @@ -124,7 +124,7 @@ { "name": "sync-direct-members-only", "displayName": "Sync direct members only", - "description": "When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded.", + "description": "When enabled, only sync direct members of groups and projects. Inherited members from parent groups will be excluded from membership grants.", "boolField": {} } ], diff --git a/docs/connector.mdx b/docs/connector.mdx index e892c933..ad65dfcf 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -22,13 +22,9 @@ The GitLab connector supports [automatic account provisioning and deprovisioning Information on last login is synced from self-hosted GitLab instances; this capability is not supported on GitLab.com due to permissions limitations. -### Access paths - -In GitLab a user can obtain access to a project or group in several ways: directly as a member, inherited from a parent or top-level group, or through another group that has been invited (shared) into the project or group. - -By default the connector emits effective membership as flattened direct grants, so every access path looks the same. Enable the **Sync access paths** setting to instead surface each path as a distinct, reviewable access path in C1 — so during an access review you can see *how* each user obtained their access. This setting is opt-in because it changes the shape and number of grants; existing connectors are unaffected until it is enabled. - -Separately, enable the **Sync direct members only** setting to sync only direct memberships and exclude indirect access (parent-group inheritance and invited/shared groups) entirely. It applies whether or not **Sync access paths** is enabled. + +**Access path visibility (optional).** By default, C1 shows effective membership as a single flattened list of grants. Enable the **Sync access paths** setting to instead label each grant by how the access was obtained — direct membership, inheritance from a parent or top-level group, or access through an invited (shared) group — so reviewers can see and act on each access path separately. This setting is off by default and does not change existing grants until you enable it. + ## Gather GitLab credentials diff --git a/docs/doc-info.md b/docs/doc-info.md index 23ece39a..b179addd 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -3,40 +3,6 @@ While developing the connector, please fill out this form. This information is n ## Connector capabilities - Sync Users, projects and groups. -- Optionally labels grants by access path (opt-in via `--sync-access-paths`, - disabled by default): when enabled, memberships are surfaced as distinct, - reviewable paths — direct membership, inheritance from a parent/top-level - group, and access through an invited (shared) group — instead of a flattened - effective list. Indirect paths are expressed via expandable grants - (group-as-principal pointing at a group's membership entitlement), and two - expansion-only entitlements (`member`, `effective-member`) are added to groups. - When disabled (the default), the connector emits effective membership as - flattened direct grants, preserving the previous grant shape and counts — so - existing deployments see no change unless they opt in. The - `--sync-direct-members-only` flag restricts syncing to direct memberships only - and applies in both modes. - - Invited (shared) group resolution follows GitLab's own sharing semantics, - which differ by target: a group shared into another **group** grants access - only to the invited group's *direct* members, so that path expands to the - invited group's `member` entitlement; a group shared into a **project** grants - access to the invited group's *direct and inherited* members, so that path - expands to the invited group's `effective-member` entitlement (composed from - the group's direct members plus its ancestor-inherited members via expansion, - no extra API calls). Sharing is non-transitive — a group's own inbound (shared) - members are not re-shared onward — so `effective-member` deliberately excludes - them. Parent/top-level inheritance is fully transitive in both cases. - - Known limitation: on the invited (shared) group path, expanded members are - shown at the share's `group_access_level`, which is an upper bound. GitLab caps - a shared member's effective access at `min(role in the invited group, - group_access_level)`, so a member whose role in the invited group is below the - share level may be shown one level higher on the shared path. This is a - consequence of expressing the path via expansion (`GrantExpandable` copies - principals without recomputing per-member levels). The direct and - parent/top-level inheritance paths preserve exact access levels; only the - invited-group path uses the share level. - - Supports Account provisioning: When you creating and new account, the following fields are required: - Name: The name of the user. @@ -71,6 +37,46 @@ While developing the connector, please fill out this form. This information is n — Group entitlements for Users — Project entitlements for Users +## Access-path labeling (--sync-access-paths, opt-in) + +By default the connector emits effective membership as flattened direct grants: a +user who can access a group/project shows up as a single grant, regardless of +whether that access is direct, inherited from a parent group, or granted through an +invited (shared) group. All paths look the same. + +When `--sync-access-paths` is enabled, each way a principal obtained access is +surfaced as its own expandable grant, so the access path is visible and reviewable: + + - Direct membership — a plain user grant on the resource's access-level entitlement. + - Inheritance from a parent/top-level group — one expandable grant per ancestor + group and per access level that ancestor actually has members at, with the + ancestor group as principal, expanding into that group's matching per-level + entitlement. Each ancestor (immediate subgroup up to the top-level group) is a + distinct path, so a top-level group member is shown as having access to nested + projects through the top-level group specifically. + - Access via an invited (shared) group — the invited group as principal on the + target, expanding into the invited group's per-level entitlements. GitLab caps a + shared member's effective access at min(their level in the invited group, the + share's access level); the connector applies that cap per level, so no member is + shown at a higher level than GitLab actually grants. + +Design notes: + - Expandable grants use Shallow=true: each grant is a single, crisp hop, and the + ConductorOne platform resolves deeper nesting by walking the connected edges. + - No new entitlements are added — the model reuses the existing per-access-level + entitlements, and only emits paths for access levels that actually have members, + so no grant expands to an empty set. + - The default (flag off) output is unchanged (identical grant shape and counts), + so existing deployments are unaffected until they opt in. + - `--sync-direct-members-only` still applies: with it set, only direct members are + synced and the inheritance/invited paths are not emitted. + +Known limitation: GitLab group→project sharing also grants access to the invited +group's inherited members. When the invited group is itself a subgroup, those +inherited members are surfaced via their own group's path rather than the invited +path. Invited top-level groups (the common case) are unaffected, since their direct +and effective membership are the same. + ## Connector credentials 1. What credentials or information are needed to set up the connector? (For example, API key, client ID and secret, domain, etc.) diff --git a/pkg/config/config.go b/pkg/config/config.go index fc817c2c..85cd40e2 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -28,12 +28,12 @@ var ( SyncDirectMembersOnly = field.BoolField( "sync-direct-members-only", field.WithDisplayName("Sync direct members only"), - field.WithDescription("When enabled, only direct members of groups and projects are synced. Access inherited from parent groups or granted via invited (shared) groups is excluded."), + field.WithDescription("When enabled, only sync direct members of groups and projects. Inherited members from parent groups will be excluded from membership grants."), ) SyncAccessPaths = field.BoolField( "sync-access-paths", field.WithDisplayName("Sync access paths"), - field.WithDescription("Label grants by access path (direct, inherited, or invited group). Disabled (default) keeps the previous flattened effective-membership grants."), + field.WithDescription("Label grants by access path (direct, inherited, or via an invited group). Disabled (default) keeps the previous flattened effective-membership grants."), ) // ConfigurationFields defines the external configuration required for the diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 8317a780..7a128512 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -55,7 +55,7 @@ func New(ctx context.Context, accessToken, baseURL, accountCreationGroup string, }, nil } -func (c *GitlabClient) doRequest(ctx context.Context, method string, endpoint string, target any, body any) (*http.Header, *v2.RateLimitDescription, error) { +func (c *GitlabClient) doRequest(ctx context.Context, method string, endpoint string, target interface{}, body interface{}) (*http.Header, *v2.RateLimitDescription, error) { endpoint = fmt.Sprintf("%s%s", c.baseURL, endpoint) relativeURL, err := url.Parse(endpoint) if err != nil { @@ -192,6 +192,22 @@ func (c *GitlabClient) ListAllGroupMembers(ctx context.Context, groupID string, return members, nextToken, rateLimitDesc, nil } +// GetGroupMemberAll retrieves a single member of a group including inherited and +// invited (shared) members (GET /groups/:id/members/all/:user_id). Returns +// ErrNotFound when the user has no effective access to the group. Used to tell a +// direct membership apart from inherited/invited access on revoke. +func (c *GitlabClient) GetGroupMemberAll(ctx context.Context, groupID, userID string) (*GroupMember, *v2.RateLimitDescription, error) { + var member GroupMember + + path := fmt.Sprintf("/api/v4/groups/%s/members/all/%s", PathEscape(groupID), PathEscape(userID)) + _, rateLimitDesc, err := c.doRequest(ctx, http.MethodGet, path, &member, nil) + if err != nil { + return nil, rateLimitDesc, err + } + + return &member, rateLimitDesc, nil +} + // ListGroupMembers retrieves members of a specific group. func (c *GitlabClient) ListGroupMembers(ctx context.Context, groupID string, nextPageToken string) ([]*GroupMember, string, *v2.RateLimitDescription, error) { var members []*GroupMember @@ -333,6 +349,22 @@ func (c *GitlabClient) ListAllProjectMembers(ctx context.Context, projectID stri return members, nextToken, rateLimitDesc, nil } +// GetProjectMemberAll retrieves a single member of a project including inherited and +// invited (shared) members (GET /projects/:id/members/all/:user_id). Returns +// ErrNotFound when the user has no effective access to the project. Used to tell a +// direct membership apart from inherited/invited access on revoke. +func (c *GitlabClient) GetProjectMemberAll(ctx context.Context, projectID, userID string) (*ProjectMember, *v2.RateLimitDescription, error) { + var member ProjectMember + + path := fmt.Sprintf("/api/v4/projects/%s/members/all/%s", PathEscape(projectID), PathEscape(userID)) + _, rateLimitDesc, err := c.doRequest(ctx, http.MethodGet, path, &member, nil) + if err != nil { + return nil, rateLimitDesc, err + } + + return &member, rateLimitDesc, nil +} + // AddProjectMember adds a member to a project. func (c *GitlabClient) AddProjectMember(ctx context.Context, projectID string, memberRequest *AddProjectMemberRequest) (*ProjectMember, *v2.RateLimitDescription, error) { var member ProjectMember diff --git a/pkg/connector/client/helper.go b/pkg/connector/client/helper.go index 5856d4d5..d51e1492 100644 --- a/pkg/connector/client/helper.go +++ b/pkg/connector/client/helper.go @@ -22,6 +22,9 @@ func WithQueryParam(key string, value string) ReqOpt { } } +// sortOrderAsc is the ascending sort direction used for keyset pagination. +const sortOrderAsc = "asc" + type KeysetPaginationOpts struct { OrderBy string Sort string diff --git a/pkg/connector/client/models.go b/pkg/connector/client/models.go index b3b4efcd..adc018da 100644 --- a/pkg/connector/client/models.go +++ b/pkg/connector/client/models.go @@ -31,15 +31,14 @@ type Group struct { SharedWithGroups []SharedGroup `json:"shared_with_groups"` } -// SharedGroup represents a group that has been invited (shared) into another -// group or project, granting its members access at group_access_level. -// Returned by GET /groups/:id and GET /projects/:id (not by the list endpoints). +// SharedGroup is a group invited (shared) into another group or project. GitLab +// reports it under shared_with_groups on the group/project object. GroupAccessLevel +// is the ceiling access the invited group's members receive on the target. type SharedGroup struct { - GroupID int `json:"group_id"` - GroupName string `json:"group_name"` - GroupFullPath string `json:"group_full_path"` - GroupAccessLevel int `json:"group_access_level"` - ExpiresAt *ISOTime `json:"expires_at"` + GroupID int `json:"group_id"` + GroupName string `json:"group_name"` + GroupFullPath string `json:"group_full_path"` + GroupAccessLevel int `json:"group_access_level"` } type Namespace struct { diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 91d1e4c2..73452b5b 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -18,8 +18,6 @@ import ( resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -36,27 +34,6 @@ var groupAccessLevels = []client.AccessLevelValue{ client.OwnerPermissions, } -// groupMemberEntitlement is the expansion-only entitlement for a group's DIRECT -// members (the group→group sharing target). Not grantable; Grant is rejected. -// Only emitted when the SyncAccessPaths flag is enabled. -const groupMemberEntitlement = "member" - -// groupEffectiveMemberEntitlement is the expansion-only entitlement for a group's -// EFFECTIVE membership (direct + inherited), the group→project sharing target. -// Not grantable; Grant is rejected. Only emitted when the SyncAccessPaths flag is -// enabled. See docs/doc-info.md. -const groupEffectiveMemberEntitlement = "effective-member" - -// groupMemberEntitlementID returns a group's member entitlement ID (e.g. "group:g/28:member"). -func groupMemberEntitlementID(groupResourceID string) string { - return fmt.Sprintf("%s:%s:%s", groupResourceType.Id, groupResourceID, groupMemberEntitlement) -} - -// groupEffectiveMemberEntitlementID returns a group's effective-member entitlement ID. -func groupEffectiveMemberEntitlementID(groupResourceID string) string { - return fmt.Sprintf("%s:%s:%s", groupResourceType.Id, groupResourceID, groupEffectiveMemberEntitlement) -} - func (o *groupBuilder) ResourceType(ctx context.Context) *v2.ResourceType { return groupResourceType } @@ -121,7 +98,7 @@ func getParentGroupFromNamespace(namespace *client.Namespace) *v2.ResourceId { } func (o *groupBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { - rv := make([]*v2.Entitlement, 0, len(groupAccessLevels)+2) + rv := make([]*v2.Entitlement, 0, len(groupAccessLevels)) for _, level := range groupAccessLevels { levelName := level.String() @@ -133,35 +110,13 @@ func (o *groupBuilder) Entitlements(_ context.Context, resource *v2.Resource, _ entitlement.WithDescription(fmt.Sprintf("%s on the %s group in Gitlab", levelName, resource.DisplayName)), )) } - - // Expansion-only membership anchors (not grantable, immutable): member = - // direct, effective-member = direct + inherited. Grant on them is rejected. - // Only emitted in access-path mode; without it the connector's entitlement - // surface stays identical to the flattened-membership behavior. - if o.client.SyncAccessPaths { - rv = append(rv, - entitlement.NewAssignmentEntitlement( - resource, - groupMemberEntitlement, - entitlement.WithDisplayName(fmt.Sprintf("%s Group Member", resource.DisplayName)), - entitlement.WithDescription(fmt.Sprintf("Direct member of the %s group in Gitlab", resource.DisplayName)), - entitlement.WithAnnotation(&v2.EntitlementImmutable{}), - ), - entitlement.NewAssignmentEntitlement( - resource, - groupEffectiveMemberEntitlement, - entitlement.WithDisplayName(fmt.Sprintf("%s Group Effective Member", resource.DisplayName)), - entitlement.WithDescription(fmt.Sprintf("Effective member (direct or inherited) of the %s group in Gitlab", resource.DisplayName)), - entitlement.WithAnnotation(&v2.EntitlementImmutable{}), - ), - ) - } return rv, "", nil, nil } // Grants dispatches on the SyncAccessPaths flag. With it disabled (default) the // connector emits flattened effective membership as before; with it enabled each -// access path is surfaced as a distinct expandable grant. +// access path (direct, inherited from a parent group, or via an invited group) is +// surfaced as a distinct expandable grant. func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { if o.client.SyncAccessPaths { return o.grantsWithAccessPaths(ctx, resource, pToken) @@ -171,11 +126,13 @@ func (o *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken // grantsFlattened emits effective membership as flat direct grants — the // pre-access-path behavior, preserved unchanged for existing customers. When -// SyncDirectMembersOnly is set, only direct members are listed; otherwise the -// full effective set (direct + inherited) is flattened via ListAllGroupMembers. +// SyncDirectMembersOnly is set, only direct members are listed; otherwise the full +// effective set (direct + inherited) is flattened via ListAllGroupMembers. func (o *groupBuilder) grantsFlattened(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() + var users []*client.GroupMember + var err error var pageToken string if pToken != nil { @@ -187,11 +144,8 @@ func (o *groupBuilder) grantsFlattened(ctx context.Context, resource *v2.Resourc return nil, "", nil, fmt.Errorf("error parsing group resource id: %w", err) } - var ( - users []*client.GroupMember - nextPageToken string - rateLimitDesc *v2.RateLimitDescription - ) + var nextPageToken string + var rateLimitDesc *v2.RateLimitDescription if o.client.SyncDirectMembersOnly { users, nextPageToken, rateLimitDesc, err = o.client.ListGroupMembers(ctx, groupId, pageToken) } else { @@ -200,6 +154,7 @@ func (o *groupBuilder) grantsFlattened(ctx context.Context, resource *v2.Resourc if rateLimitDesc != nil { outputAnnotations.WithRateLimiting(rateLimitDesc) } + if err != nil { isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupId) if unhandledErr != nil { @@ -215,20 +170,21 @@ func (o *groupBuilder) grantsFlattened(ctx context.Context, resource *v2.Resourc if err != nil { return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) } + outGrants = append(outGrants, grant.NewGrant( resource, client.AccessLevelValue(user.AccessLevel).String(), principalId, )) } - return outGrants, nextPageToken, outputAnnotations, nil } -// grantsWithAccessPaths emits direct members (access-level + member grants) and, -// unless SyncDirectMembersOnly is set, the indirect access paths (parent -// inheritance and invited groups) as expandable grants so each path stays -// distinct in C1. +// grantsWithAccessPaths emits a group's direct members plus its indirect access +// paths — inheritance from ancestor groups and access via invited (shared) groups — +// as expandable grants, so each path stays distinct and reviewable in C1. It reuses +// the existing per-access-level entitlements (no synthetic membership entitlement) +// and emits paths only for levels that actually have members (no empty expansions). func (o *groupBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() @@ -253,34 +209,33 @@ func (o *groupBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.R return nil, "", outputAnnotations, unhandledErr } // Permission error listing members: skip direct members but still emit the - // pure-expansion indirect anchors below, which need no member data. Otherwise - // projects/subgroups that inherit from or are invited by this group silently - // lose those paths, since they now resolve solely by expanding this group's - // entitlements rather than by flattening via /members/all. + // pure-expansion indirect anchors below, which need no member data of this group. users, nextPageToken = nil, "" } + // Path 1/2: direct members. for _, user := range users { principalId, err := resourceSdk.NewResourceID(userResourceType, user.ID) if err != nil { return nil, "", outputAnnotations, fmt.Errorf("error creating principal ID: %w", err) } - - outGrants = append(outGrants, - grant.NewGrant(resource, client.AccessLevelValue(user.AccessLevel).String(), principalId), - grant.NewGrant(resource, groupMemberEntitlement, principalId), - ) + outGrants = append(outGrants, grant.NewGrant( + resource, + client.AccessLevelValue(user.AccessLevel).String(), + principalId, + )) } - // Indirect access paths, emitted once on the first page when enabled. + // Indirect access paths, emitted once on the first page (unless direct-only). if pageToken == "" && !o.client.SyncDirectMembersOnly { - if parentGroup := resource.ParentResourceId; parentGroup != nil { - outGrants = append(outGrants, parentGroupInheritanceGrants(resource, parentGroup)...) + if resource.ParentResourceId != nil { + inherited, err := accessPathInheritanceGrants(ctx, o.client, resource, resource.ParentResourceId, &outputAnnotations) + if err != nil { + return nil, "", outputAnnotations, err + } + outGrants = append(outGrants, inherited...) } - // This group's effective membership, for when it is invited into a project. - outGrants = append(outGrants, effectiveMemberChainGrants(resource, resource.ParentResourceId)...) - group, rlDesc, err := o.client.GetGroup(ctx, groupId) if rlDesc != nil { outputAnnotations.WithRateLimiting(rlDesc) @@ -292,8 +247,12 @@ func (o *groupBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.R } // Permission error: skip invited-group grants and continue. } else { - // Inbound shares (group→group): expand to the invited group's direct members. - outGrants = append(outGrants, sharedGroupGrants(ctx, resource, group.SharedWithGroups, groupMemberEntitlement)...) + // Path 4: groups invited into this group (group→group = direct members). + invited, err := accessPathInvitedGrants(ctx, o.client, resource, group.SharedWithGroups, &outputAnnotations) + if err != nil { + return nil, "", outputAnnotations, err + } + outGrants = append(outGrants, invited...) } } @@ -315,15 +274,6 @@ func (o *groupBuilder) Grant( return nil, fmt.Errorf("entitlement cannot be granted: user %q has not yet accepted the invitation to gitlab", principal.Id.Resource) } - // The member/effective-member entitlements are expansion-only; assign a - // specific access level instead. - groupResourceID := entitlement.Resource.Id.Resource - if entitlement.Id == groupMemberEntitlementID(groupResourceID) || entitlement.Id == groupEffectiveMemberEntitlementID(groupResourceID) { - return nil, status.Errorf(codes.InvalidArgument, - "baton-gitlab: the %q entitlement is expansion-only and cannot be granted directly; assign a specific access level instead", - strings.TrimPrefix(entitlement.Id, groupResourceType.Id+":"+groupResourceID+":")) - } - groupIdAndName := entitlement.Resource.Id.Resource groupId, err := fromGroupResourceId(groupIdAndName) if err != nil { @@ -351,7 +301,7 @@ func (o *groupBuilder) Grant( userId, err := strconv.Atoi(principal.Id.Resource) if err != nil { - l.Debug("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) + l.Warn("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) ut, err := resourceSdk.GetUserTrait(principal) if err != nil { return nil, fmt.Errorf("baton-gitlab: error getting user trait: %w", err) @@ -404,17 +354,6 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations var outputAnnotations = annotations.New() groupIdAndName := grant.Entitlement.Resource.Id.Resource - - // The member/effective-member entitlements are expansion-only attribution - // anchors that carry real user principals; revoking them must never remove a - // real group membership. Mirror the guard in Grant (EntitlementImmutable should - // already prevent this path, but guard defensively). - if grant.Entitlement.Id == groupMemberEntitlementID(groupIdAndName) || grant.Entitlement.Id == groupEffectiveMemberEntitlementID(groupIdAndName) { - return nil, status.Errorf(codes.InvalidArgument, - "baton-gitlab: the %q entitlement is expansion-only and cannot be revoked directly; it maps to no assignable access level", - strings.TrimPrefix(grant.Entitlement.Id, groupResourceType.Id+":"+groupIdAndName+":")) - } - groupId, err := fromGroupResourceId(groupIdAndName) if err != nil { return nil, fmt.Errorf("error parsing group resource id: %w", err) @@ -426,7 +365,21 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations } if err != nil { if errors.Is(err, client.ErrNotFound) { - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + // Not a direct member. If the principal still has effective access, it is + // inherited/invited — reject instead of falsely reporting success (which + // would reappear next sync). Otherwise the membership is genuinely gone. + member, rlDesc, checkErr := o.client.GetGroupMemberAll(ctx, groupId, grant.Principal.Id.Resource) + if rlDesc != nil { + outputAnnotations.WithRateLimiting(rlDesc) + } + switch { + case checkErr == nil && member != nil: + return outputAnnotations, revokeInheritedError("group", groupIdAndName) + case checkErr != nil && !errors.Is(checkErr, client.ErrNotFound): + return outputAnnotations, fmt.Errorf("error verifying group membership: %w", checkErr) + default: + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } } return outputAnnotations, fmt.Errorf("error removing user from group: %w", err) } diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 2414cea3..00ea2d97 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -4,11 +4,13 @@ import ( "context" "errors" "fmt" + "slices" "strconv" "strings" "github.com/conductorone/baton-gitlab/pkg/connector/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/types/grant" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" @@ -16,6 +18,10 @@ import ( "google.golang.org/grpc/status" ) +// maxAncestorDepth bounds the parent-group walk so a pathological/cyclic group +// hierarchy can never deadlock a sync. +const maxAncestorDepth = 20 + func toGroupResourceId(groupId string) string { return fmt.Sprintf("g/%s", groupId) } @@ -42,132 +48,245 @@ func parseAccessLevelFromEntitlementID(entitlementID string) (int, error) { return int(levelValue), nil } -// inheritedAccessLevels are the levels a parent group confers on child -// subgroups/projects (Minimal and None excluded — they confer no child access). -var inheritedAccessLevels = []client.AccessLevelValue{ - client.GuestPermissions, - client.ReporterPermissions, - client.DeveloperPermissions, - client.MaintainerPermissions, - client.OwnerPermissions, +// revokeInheritedError rejects a revoke of access the principal holds indirectly +// (inherited from a parent group or granted via an invited/shared group) rather than +// as a direct membership. Removing a non-existent direct membership is a silent no-op +// that reappears on the next sync (a revoke loop), so we reject with an actionable +// message instead of falsely reporting success. The indirect access must be revoked +// at its source (the parent or invited group). +func revokeInheritedError(resourceType, resourceID string) error { + return status.Errorf(codes.InvalidArgument, + "baton-gitlab: cannot revoke: principal is not a direct member of %s %q; access is inherited from a parent group or granted via an invited group and must be revoked at its source", + resourceType, resourceID) } -// parentGroupInheritanceGrants surfaces "access via parent/top-level group" as a -// distinct path: per level, the parent group as principal, expandable to the -// parent's members at that level (non-shallow, so ancestors resolve transitively). -// Only used in access-path mode (SyncAccessPaths). -func parentGroupInheritanceGrants(target *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { - grants := make([]*v2.Grant, 0, len(inheritedAccessLevels)) - for _, level := range inheritedAccessLevels { - levelName := level.String() - grants = append(grants, grant.NewGrant( - target, - levelName, - parentGroup, - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, parentGroup.Resource, levelName)}, - Shallow: false, - ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, - }), - )) +func handlePermissionError(ctx context.Context, err error, resourceType, resourceId string) (bool, error) { + if err == nil { + return false, nil } - return grants + + if status.Code(err) == codes.PermissionDenied || errors.Is(err, client.ErrForbidden) { + l := ctxzap.Extract(ctx) + l.Debug( + fmt.Sprintf("Permission denied while listing members for %s. Skipping.", resourceType), + zap.String(fmt.Sprintf("%s_id", resourceType), resourceId), + ) + return true, nil + } + + return false, err } -// sharedGroupGrants emits, for each invited (shared) group, that group as -// principal on the target's access-level entitlement, expandable to the invited -// group's membership. expansionSlug picks which membership entitlement to expand -// into (group→group = member/direct, group→project = effective-member); see -// docs/doc-info.md for the sharing-semantics rationale. Only used in access-path -// mode (SyncAccessPaths). +// --- Access-path helpers (only used when SyncAccessPaths is enabled) --- // -// Unlike parent inheritance (which excludes Minimal because it confers no child -// access), a share carries whatever group_access_level GitLab reports, so the -// full Minimal..Owner range is accepted here — both groups and projects expose a -// Minimal access-level entitlement, so the grant still resolves. -func sharedGroupGrants(ctx context.Context, target *v2.Resource, shared []client.SharedGroup, expansionSlug string) []*v2.Grant { - l := ctxzap.Extract(ctx) - grants := make([]*v2.Grant, 0, len(shared)) - for _, sg := range shared { - level := client.AccessLevelValue(sg.GroupAccessLevel) - levelName := level.String() - // Skip levels with no matching entitlement (only Minimal..Owner exist), - // which would otherwise orphan the grant. - if levelName == "" || level < client.MinimalAccessPermissions || level > client.OwnerPermissions { - l.Debug("baton-gitlab: skipping shared group with unsupported access level", - zap.String("target", target.Id.Resource), - zap.Int("shared_group_id", sg.GroupID), - zap.Int("group_access_level", sg.GroupAccessLevel), - ) - continue +// The access-path model labels every grant by HOW access was obtained, using +// expandable grants that reuse the existing per-access-level entitlements. It adds +// NO new entitlements and emits paths only for access levels that actually have +// members, so no expandable grant resolves to an empty set. See docs/doc-info.md. + +// sortedLevelSlugs returns the slugs of the given access levels sorted ascending by +// level value, for deterministic grant emission. +func sortedLevelSlugs(set map[client.AccessLevelValue]struct{}) []string { + levels := make([]client.AccessLevelValue, 0, len(set)) + for l := range set { + levels = append(levels, l) + } + slices.Sort(levels) + out := make([]string, 0, len(levels)) + for _, l := range levels { + if slug := l.String(); slug != "" { + out = append(out, slug) } + } + return out +} - invitedGroupResourceID := toGroupResourceId(strconv.Itoa(sg.GroupID)) - principalID := &v2.ResourceId{ResourceType: groupResourceType.Id, Resource: invitedGroupResourceID} +// inheritableLevelSlugs returns the distinct access levels actually held by the +// given members, restricted to the inheritable range (Guest..Owner). Minimal and +// None confer no access to child subgroups/projects, so they are excluded. +func inheritableLevelSlugs(members []*client.GroupMember) []string { + present := make(map[client.AccessLevelValue]struct{}) + for _, member := range members { + lvl := client.AccessLevelValue(member.AccessLevel) + if lvl < client.GuestPermissions || lvl > client.OwnerPermissions { + continue + } + present[lvl] = struct{}{} + } + return sortedLevelSlugs(present) +} +// inheritanceGrants emits one expandable grant per level the ancestor group holds: +// principal = the ancestor group, expandable into that group's per-level entitlement. +// Shallow keeps each path a single crisp hop ("access via membership"); +// the ancestor's own inheritance grants carry deeper paths as their own edges. +func inheritanceGrants(target *v2.Resource, ancestor *v2.ResourceId, levelSlugs []string) []*v2.Grant { + grants := make([]*v2.Grant, 0, len(levelSlugs)) + for _, slug := range levelSlugs { grants = append(grants, grant.NewGrant( target, - levelName, - principalID, + slug, + ancestor, grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, invitedGroupResourceID, expansionSlug)}, - Shallow: false, - ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, + EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, ancestor.Resource, slug)}, + Shallow: true, + ResourceTypeIds: []string{userResourceType.Id}, }), )) } return grants } -// effectiveMemberChainGrants composes a group's effective-member entitlement -// (direct + ancestor-inherited members) purely via expansion — no extra API call. -// It excludes the group's own inbound shares because group→project sharing is -// non-transitive. See docs/doc-info.md for the semantics. Only used in access-path -// mode (SyncAccessPaths). -func effectiveMemberChainGrants(group *v2.Resource, parentGroup *v2.ResourceId) []*v2.Grant { - expandable := func(srcEntitlementID string) grant.GrantOption { - return grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{srcEntitlementID}, - Shallow: false, - ResourceTypeIds: []string{userResourceType.Id, groupResourceType.Id}, - }) - } - - grants := make([]*v2.Grant, 0, 2) +// invitedGroupGrants emits access-path grants for groups invited (shared) into the +// target. GitLab caps a shared member's effective access at min(their level in the +// invited group, the share's group_access_level); we map each level present in the +// invited group to that capped level and emit one expandable grant per resulting +// target level, expanding the invited group's matching per-level entitlements. +// Shallow, only real levels — no empty paths, no synthetic membership entitlement. +func invitedGroupGrants(target *v2.Resource, shared []client.SharedGroup, membersByGroup map[int][]*client.GroupMember) []*v2.Grant { + var grants []*v2.Grant + for _, sharedGroup := range shared { + share := client.AccessLevelValue(sharedGroup.GroupAccessLevel) + invitedResourceID := toGroupResourceId(strconv.Itoa(sharedGroup.GroupID)) + principalID := &v2.ResourceId{ResourceType: groupResourceType.Id, Resource: invitedResourceID} - // Direct members. - grants = append(grants, grant.NewGrant( - group, - groupEffectiveMemberEntitlement, - group.Id, - expandable(groupMemberEntitlementID(group.Id.Resource)), - )) + // capped target level -> set of the invited group's per-level entitlements + // whose members land on that level. + sources := make(map[client.AccessLevelValue]map[string]struct{}) + for _, m := range membersByGroup[sharedGroup.GroupID] { + lvl := client.AccessLevelValue(m.AccessLevel) + if lvl < client.MinimalAccessPermissions || lvl > client.OwnerPermissions { + continue + } + eff := lvl + if share >= client.MinimalAccessPermissions && share < eff { + eff = share + } + src := fmt.Sprintf("%s:%s:%s", groupResourceType.Id, invitedResourceID, lvl.String()) + if sources[eff] == nil { + sources[eff] = make(map[string]struct{}) + } + sources[eff][src] = struct{}{} + } - // Inherited members (transitive up the ancestor chain). - if parentGroup != nil { - grants = append(grants, grant.NewGrant( - group, - groupEffectiveMemberEntitlement, - parentGroup, - expandable(groupEffectiveMemberEntitlementID(parentGroup.Resource)), - )) + effLevels := make([]client.AccessLevelValue, 0, len(sources)) + for l := range sources { + effLevels = append(effLevels, l) + } + slices.Sort(effLevels) + for _, eff := range effLevels { + srcIDs := make([]string, 0, len(sources[eff])) + for s := range sources[eff] { + srcIDs = append(srcIDs, s) + } + slices.Sort(srcIDs) + grants = append(grants, grant.NewGrant( + target, + eff.String(), + principalID, + grant.WithAnnotation(&v2.GrantExpandable{ + EntitlementIds: srcIDs, + Shallow: true, + ResourceTypeIds: []string{userResourceType.Id}, + }), + )) + } } return grants } -func handlePermissionError(ctx context.Context, err error, resourceType, resourceId string) (bool, error) { - if err == nil { - return false, nil +// allDirectGroupMembers pages through a group's direct members, accumulating rate-limit +// annotations. Used by the access-path helpers to enumerate ancestor/invited-group +// membership levels. +func allDirectGroupMembers(ctx context.Context, c *client.GitlabClient, groupID string, annos *annotations.Annotations) ([]*client.GroupMember, error) { + var all []*client.GroupMember + pageToken := "" + for { + members, next, rateLimitDesc, err := c.ListGroupMembers(ctx, groupID, pageToken) + if rateLimitDesc != nil { + annos.WithRateLimiting(rateLimitDesc) + } + if err != nil { + return all, err + } + all = append(all, members...) + if next == "" { + return all, nil + } + pageToken = next } +} - if status.Code(err) == codes.PermissionDenied || errors.Is(err, client.ErrForbidden) { - l := ctxzap.Extract(ctx) - l.Debug( - fmt.Sprintf("Permission denied while listing members for %s. Skipping.", resourceType), - zap.String(fmt.Sprintf("%s_id", resourceType), resourceId), - ) - return true, nil +// accessPathInheritanceGrants walks the target's ancestor group chain and emits, per +// ancestor, "access via membership" expandable grants for the levels that +// ancestor actually has direct members at. Together the per-ancestor edges surface +// both intermediate-subgroup and top-level access as distinct, reviewable paths. +func accessPathInheritanceGrants(ctx context.Context, c *client.GitlabClient, target *v2.Resource, immediateParent *v2.ResourceId, annos *annotations.Annotations) ([]*v2.Grant, error) { + var grants []*v2.Grant + cursor := immediateParent + for depth := 0; cursor != nil && depth < maxAncestorDepth; depth++ { + groupID, err := fromGroupResourceId(cursor.Resource) + if err != nil { + return nil, err + } + + members, err := allDirectGroupMembers(ctx, c, groupID, annos) + if err != nil { + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupID) + if unhandledErr != nil { + return nil, unhandledErr + } + if isPermissionError { + members = nil + } + } + grants = append(grants, inheritanceGrants(target, cursor, inheritableLevelSlugs(members))...) + + group, rateLimitDesc, err := c.GetGroup(ctx, groupID) + if rateLimitDesc != nil { + annos.WithRateLimiting(rateLimitDesc) + } + if err != nil { + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupID) + if unhandledErr != nil { + return nil, unhandledErr + } + if isPermissionError { + return grants, nil + } + } + if group == nil || group.ParentID == 0 { + return grants, nil + } + cursor = getParentGroup(group.ParentID) } + return grants, nil +} - return false, err +// accessPathInvitedGrants resolves each invited (shared) group's direct members and +// emits the invited-group access paths for the target. GitLab group→group sharing +// confers access only to the invited group's direct members; group→project sharing +// also confers it to the invited group's inherited members, so for an invited +// *subgroup* shared into a project those inherited members are surfaced via their own +// group's path rather than this one (see docs/doc-info.md). +func accessPathInvitedGrants(ctx context.Context, c *client.GitlabClient, target *v2.Resource, shared []client.SharedGroup, annos *annotations.Annotations) ([]*v2.Grant, error) { + if len(shared) == 0 { + return nil, nil + } + membersByGroup := make(map[int][]*client.GroupMember, len(shared)) + for _, sg := range shared { + groupID := strconv.Itoa(sg.GroupID) + members, err := allDirectGroupMembers(ctx, c, groupID, annos) + if err != nil { + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", groupID) + if unhandledErr != nil { + return nil, unhandledErr + } + if isPermissionError { + continue + } + } + membersByGroup[sg.GroupID] = members + } + return invitedGroupGrants(target, shared, membersByGroup), nil } diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index 84fdf3f5..1f4af6c3 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -69,7 +69,7 @@ func (o *projectBuilder) List(ctx context.Context, parentResourceID *v2.Resource parentGroup = parentResourceID } - resource, err := projectResource(project, parentGroup) + resource, err := projectResource(project, parentGroup, o.client.IsOnPremise) if err != nil { return nil, "", outputAnnotations, err } @@ -97,7 +97,8 @@ func (o *projectBuilder) Entitlements(ctx context.Context, resource *v2.Resource // Grants dispatches on the SyncAccessPaths flag. With it disabled (default) the // connector emits flattened effective membership as before; with it enabled each -// access path is surfaced as a distinct expandable grant. +// access path (direct, inherited from a parent group, or via an invited group) is +// surfaced as a distinct expandable grant. func (o *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { if o.client.SyncAccessPaths { return o.grantsWithAccessPaths(ctx, resource, pToken) @@ -106,29 +107,22 @@ func (o *projectBuilder) Grants(ctx context.Context, resource *v2.Resource, pTok } // grantsFlattened emits effective membership as flat direct grants plus a -// parent-group expandable grant per member — the pre-access-path behavior, -// preserved unchanged for existing customers. When SyncDirectMembersOnly is set, -// only direct members are listed; otherwise the full effective set is flattened -// via ListAllProjectMembers. +// parent-group expandable grant per member — the pre-access-path behavior, preserved +// unchanged for existing customers. When SyncDirectMembersOnly is set, only direct +// members are listed; otherwise the full effective set is flattened via +// ListAllProjectMembers. func (o *projectBuilder) grantsFlattened(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() - var pageToken string - if pToken != nil { - pageToken = pToken.Token - } - - var ( - users []*client.ProjectMember - nextPageToken string - rateLimitDesc *v2.RateLimitDescription - err error - ) + var users []*client.ProjectMember + var err error + var nextPageToken string + var rateLimitDesc *v2.RateLimitDescription if o.client.SyncDirectMembersOnly { - users, nextPageToken, rateLimitDesc, err = o.client.ListProjectMembers(ctx, resource.Id.Resource, pageToken) + users, nextPageToken, rateLimitDesc, err = o.client.ListProjectMembers(ctx, resource.Id.Resource, pToken.Token) } else { - users, nextPageToken, rateLimitDesc, err = o.client.ListAllProjectMembers(ctx, resource.Id.Resource, pageToken) + users, nextPageToken, rateLimitDesc, err = o.client.ListAllProjectMembers(ctx, resource.Id.Resource, pToken.Token) } if rateLimitDesc != nil { outputAnnotations.WithRateLimiting(rateLimitDesc) @@ -173,9 +167,11 @@ func (o *projectBuilder) grantsFlattened(ctx context.Context, resource *v2.Resou return outGrants, nextPageToken, outputAnnotations, nil } -// grantsWithAccessPaths emits direct members and, unless SyncDirectMembersOnly is -// set, the indirect access paths (parent inheritance and invited groups) as -// expandable grants so each path stays distinct in C1. +// grantsWithAccessPaths emits a project's direct members plus its indirect access +// paths — inheritance from ancestor groups (up to and including the top-level group) +// and access via invited (shared) groups — as expandable grants. It reuses the +// existing per-access-level entitlements (no synthetic membership entitlement) and +// emits paths only for levels that actually have members (no empty expansions). func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2.Resource, pToken *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { var outGrants []*v2.Grant var outputAnnotations = annotations.New() @@ -195,13 +191,11 @@ func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2 return nil, "", outputAnnotations, unhandledErr } // Permission error listing members: skip direct members but still emit the - // pure-expansion parent-inheritance anchor below, which needs no member data. - // Otherwise a project whose members are unreadable loses its inherited-access - // path entirely, since it now resolves by expanding group:: - // rather than by flattening via /members/all. + // pure-expansion indirect anchors below, which need no project member data. users, nextPageToken = nil, "" } + // Path 1: direct members. for _, user := range users { principalId, err := resourceSdk.NewResourceID(userResourceType, user.ID) if err != nil { @@ -214,12 +208,18 @@ func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2 )) } - // Indirect access paths, emitted once on the first page when enabled. + // Indirect access paths, emitted once on the first page (unless direct-only). if pageToken == "" && !o.client.SyncDirectMembersOnly { - if parentGroup := resource.ParentResourceId; parentGroup != nil { - outGrants = append(outGrants, parentGroupInheritanceGrants(resource, parentGroup)...) + // Path 2: inheritance from ancestor groups (immediate subgroup up to top-level). + if resource.ParentResourceId != nil { + inherited, err := accessPathInheritanceGrants(ctx, o.client, resource, resource.ParentResourceId, &outputAnnotations) + if err != nil { + return nil, "", outputAnnotations, err + } + outGrants = append(outGrants, inherited...) } + // Path 3: groups invited into this project. project, rlDesc, err := o.client.GetProject(ctx, resource.Id.Resource) if rlDesc != nil { outputAnnotations.WithRateLimiting(rlDesc) @@ -231,8 +231,11 @@ func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2 } // Permission error: skip invited-group grants and continue. } else { - // Invited groups (group→project): expand to effective membership. - outGrants = append(outGrants, sharedGroupGrants(ctx, resource, project.SharedWithGroups, groupEffectiveMemberEntitlement)...) + invited, err := accessPathInvitedGrants(ctx, o.client, resource, project.SharedWithGroups, &outputAnnotations) + if err != nil { + return nil, "", outputAnnotations, err + } + outGrants = append(outGrants, invited...) } } @@ -292,7 +295,21 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio } if err != nil { if errors.Is(err, client.ErrNotFound) { - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + // Not a direct member. If the principal still has effective access, it is + // inherited/invited — reject instead of falsely reporting success (which + // would reappear next sync). Otherwise the membership is genuinely gone. + member, rlDesc, checkErr := o.client.GetProjectMemberAll(ctx, projectId, strconv.Itoa(userId)) + if rlDesc != nil { + outputAnnotations.WithRateLimiting(rlDesc) + } + switch { + case checkErr == nil && member != nil: + return outputAnnotations, revokeInheritedError("project", projectId) + case checkErr != nil && !errors.Is(checkErr, client.ErrNotFound): + return outputAnnotations, fmt.Errorf("error verifying project membership: %w", checkErr) + default: + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } } return outputAnnotations, fmt.Errorf("error removing user from project: %w", err) } @@ -300,7 +317,7 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio return outputAnnotations, nil } -func projectResource(project *client.Project, parentResourceID *v2.ResourceId) (*v2.Resource, error) { +func projectResource(project *client.Project, parentResourceID *v2.ResourceId, isOnPremise bool) (*v2.Resource, error) { var annotations []proto.Message return resourceSdk.NewGroupResource( diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index 5a93080c..01001a36 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -23,3 +23,10 @@ var projectResourceType = &v2.ResourceType{ Id: "project", DisplayName: "Project", } + +// Shared field-name keys used by the account-creation schema and resource +// profiles. Extracted so the literal is defined once (goconst). +const ( + fieldName = "name" + fieldEmail = "email" +) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 59d807c4..3a60b1ed 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -464,7 +464,7 @@ func userResource(user any) (*v2.Resource, error) { name = pendingInvitationUser + strings.ToLower(email) } - profile := map[string]any{ + profile := map[string]interface{}{ "first_name": name, "username": username, profileFieldEmail: email, From da9c73639196b4105a977686439da9f2842e21be Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Thu, 16 Jul 2026 12:53:54 -0300 Subject: [PATCH 08/15] fix(gitlab): fix revoke/grant idempotency detection and mark indirect grants immutable The Revoke and Grant idempotency branches keyed off errors.Is(err, client.ErrNotFound) and errors.As(err, *client.ErrorResponse), but the uhttp client returns a gRPC-status error for 4xx and doRequest never reaches CheckResponse (the only place those sentinels are produced), so neither check ever matched: revoking inherited/invited access fell through to a NotFound error and re-adding an existing member surfaced as a hard error instead of a no-op. Detect the status via status.Code(err) (isNotFoundError / isAlreadyExistsError) so the revoke rejection and grant idempotency actually fire. Gate the inherited/invited revoke rejection behind SyncAccessPaths so the default path keeps its historical GrantAlreadyRevoked no-op. Mark the inheritance and invited-group expandable grants GrantImmutable so the platform never offers a direct revoke of access that is held elsewhere. Guard invitedGroupGrants against non-standard access levels that have no entitlement slug. Wrap the projects user-ID parse error as InvalidArgument, downgrade a Warn log to Debug, and document the revoke/immutability semantics in docs/doc-info.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/doc-info.md | 13 +++++++++ pkg/connector/groups.go | 33 ++++++++++++---------- pkg/connector/helpers.go | 58 +++++++++++++++++++++++++++++++-------- pkg/connector/projects.go | 27 ++++++++++++++---- 4 files changed, 100 insertions(+), 31 deletions(-) diff --git a/docs/doc-info.md b/docs/doc-info.md index b179addd..35e83f63 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -66,6 +66,12 @@ Design notes: - No new entitlements are added — the model reuses the existing per-access-level entitlements, and only emits paths for access levels that actually have members, so no grant expands to an empty set. + - Inherited and invited (shared) paths are attribution-only: their expandable grants + are marked immutable, so the platform never offers a direct revoke of access that + is really held elsewhere. Revoking such access at the target is rejected + (InvalidArgument) and points at its source group, since removing a non-existent + direct membership is a no-op that would reappear on the next sync. Direct + memberships revoke normally. - The default (flag off) output is unchanged (identical grant shape and counts), so existing deployments are unaffected until they opt in. - `--sync-direct-members-only` still applies: with it set, only direct members are @@ -77,6 +83,13 @@ inherited members are surfaced via their own group's path rather than the invite path. Invited top-level groups (the common case) are unaffected, since their direct and effective membership are the same. +Known limitation: an inheritance/invited access path is expandable into the ancestor or +invited group's per-level entitlements. If that group is not itself in the connector's +sync scope (for example, the token can see the share but the group is not returned as a +top-level synced resource), the expandable grant has nothing to resolve into. This is +non-destructive — the path simply does not expand — but the members reached only through +that group will not surface until the group is in scope. + ## Connector credentials 1. What credentials or information are needed to set up the connector? (For example, API key, client ID and secret, domain, etc.) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 73452b5b..32af69e8 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -2,9 +2,7 @@ package connector import ( "context" - "errors" "fmt" - "net/http" "strconv" "strings" "time" @@ -301,7 +299,7 @@ func (o *groupBuilder) Grant( userId, err := strconv.Atoi(principal.Id.Resource) if err != nil { - l.Warn("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) + l.Debug("baton-gitlab grant: unable to parse user ID. falling back to email invite", zap.Error(err)) ut, err := resourceSdk.GetUserTrait(principal) if err != nil { return nil, fmt.Errorf("baton-gitlab: error getting user trait: %w", err) @@ -336,14 +334,13 @@ func (o *groupBuilder) Grant( outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - var errResp *client.ErrorResponse - if errors.As(err, &errResp) { - if errResp.Response != nil && errResp.Response.StatusCode == http.StatusConflict { - return annotations.New(&v2.GrantAlreadyExists{}), nil - } - if strings.Contains(err.Error(), "should be greater than or equal to") { - return annotations.New(&v2.GrantAlreadyExists{}), nil - } + // Idempotency: GitLab returns 409 (uhttp → codes.AlreadyExists) when the user is + // already a member, and a 400 "should be greater than or equal to" when they already + // hold an equal/higher role. Both mean the desired grant already holds. (The gRPC + // code is checked directly because the hand-rolled *ErrorResponse never reaches the + // error chain for doRequest calls — uhttp errors on 4xx before CheckResponse runs.) + if isAlreadyExistsError(err) || strings.Contains(err.Error(), "should be greater than or equal to") { + return annotations.New(&v2.GrantAlreadyExists{}), nil } return outputAnnotations, fmt.Errorf("error adding user to group: %w", err) } @@ -364,7 +361,14 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - if errors.Is(err, client.ErrNotFound) { + if isNotFoundError(err) { + // With access-path labeling off (default), preserve the historical behavior: + // an absent direct membership is reported as already revoked. The effective- + // access check below only applies in access-path mode, so default-mode + // deployments see no provisioning behavior change. + if !o.client.SyncAccessPaths { + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which // would reappear next sync). Otherwise the membership is genuinely gone. @@ -375,10 +379,11 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations switch { case checkErr == nil && member != nil: return outputAnnotations, revokeInheritedError("group", groupIdAndName) - case checkErr != nil && !errors.Is(checkErr, client.ErrNotFound): + case checkErr != nil && !isNotFoundError(checkErr): return outputAnnotations, fmt.Errorf("error verifying group membership: %w", checkErr) default: - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) + return outputAnnotations, nil } } return outputAnnotations, fmt.Errorf("error removing user from group: %w", err) diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 00ea2d97..96b54c31 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -60,6 +60,23 @@ func revokeInheritedError(resourceType, resourceID string) error { resourceType, resourceID) } +// isNotFoundError reports whether err is a GitLab 404. The uhttp client surfaces a 404 +// as a gRPC status with codes.NotFound; the client.ErrNotFound sentinel is only returned +// by CheckResponse, which doRequest bypasses because uhttp already errors on 4xx. We +// check both so the detection is robust regardless of which path produced the error. +// (Dominant idiom across baton-openai/segment/microsoft-entra/active-directory.) +func isNotFoundError(err error) bool { + return errors.Is(err, client.ErrNotFound) || status.Code(err) == codes.NotFound +} + +// isAlreadyExistsError reports whether err is a GitLab 409 Conflict ("Member already +// exists"). uhttp maps HTTP 409 → codes.AlreadyExists; the hand-rolled *ErrorResponse +// path never fires for doRequest calls (uhttp errors on 4xx before CheckResponse runs), +// so idempotency must gate on the gRPC code, mirroring isNotFoundError. +func isAlreadyExistsError(err error) bool { + return status.Code(err) == codes.AlreadyExists +} + func handlePermissionError(ctx context.Context, err error, resourceType, resourceId string) (bool, error) { if err == nil { return false, nil @@ -127,11 +144,17 @@ func inheritanceGrants(target *v2.Resource, ancestor *v2.ResourceId, levelSlugs target, slug, ancestor, - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, ancestor.Resource, slug)}, - Shallow: true, - ResourceTypeIds: []string{userResourceType.Id}, - }), + // GrantExpandable surfaces the ancestor's members as access-through-this-path; + // GrantImmutable marks the edge non-revocable — inherited access is structural + // and can only be removed at its source, so C1 must not offer a direct revoke. + grant.WithAnnotation( + &v2.GrantExpandable{ + EntitlementIds: []string{fmt.Sprintf("%s:%s:%s", groupResourceType.Id, ancestor.Resource, slug)}, + Shallow: true, + ResourceTypeIds: []string{userResourceType.Id}, + }, + &v2.GrantImmutable{}, + ), )) } return grants @@ -155,13 +178,20 @@ func invitedGroupGrants(target *v2.Resource, shared []client.SharedGroup, member sources := make(map[client.AccessLevelValue]map[string]struct{}) for _, m := range membersByGroup[sharedGroup.GroupID] { lvl := client.AccessLevelValue(m.AccessLevel) - if lvl < client.MinimalAccessPermissions || lvl > client.OwnerPermissions { + // A member level in range but not one of GitLab's defined levels has no + // entitlement slug; skip it so we never build a malformed source ID. + if lvl < client.MinimalAccessPermissions || lvl > client.OwnerPermissions || lvl.String() == "" { continue } eff := lvl if share >= client.MinimalAccessPermissions && share < eff { eff = share } + // A non-standard share level (in range but undefined) has no slug either; + // skip rather than emit a grant on an empty-slug entitlement. + if eff.String() == "" { + continue + } src := fmt.Sprintf("%s:%s:%s", groupResourceType.Id, invitedResourceID, lvl.String()) if sources[eff] == nil { sources[eff] = make(map[string]struct{}) @@ -184,11 +214,17 @@ func invitedGroupGrants(target *v2.Resource, shared []client.SharedGroup, member target, eff.String(), principalID, - grant.WithAnnotation(&v2.GrantExpandable{ - EntitlementIds: srcIDs, - Shallow: true, - ResourceTypeIds: []string{userResourceType.Id}, - }), + // Invited-group access flows through the invited group's membership + // (GrantExpandable) and is not revocable at the target (GrantImmutable): + // it must be removed by un-sharing or editing the invited group. + grant.WithAnnotation( + &v2.GrantExpandable{ + EntitlementIds: srcIDs, + Shallow: true, + ResourceTypeIds: []string{userResourceType.Id}, + }, + &v2.GrantImmutable{}, + ), )) } } diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index 1f4af6c3..d8b4fde6 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -2,7 +2,6 @@ package connector import ( "context" - "errors" "fmt" "strconv" "strings" @@ -14,6 +13,8 @@ import ( "github.com/conductorone/baton-sdk/pkg/types/entitlement" "github.com/conductorone/baton-sdk/pkg/types/grant" resourceSdk "github.com/conductorone/baton-sdk/pkg/types/resource" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -262,7 +263,7 @@ func (o *projectBuilder) Grant( } userId, err := strconv.Atoi(principal.Id.Resource) if err != nil { - return nil, fmt.Errorf("error converting user ID to int: %w", err) + return nil, status.Errorf(codes.InvalidArgument, "baton-gitlab: invalid user ID: %v", err) } memberRequest := &client.AddProjectMemberRequest{ @@ -274,6 +275,12 @@ func (o *projectBuilder) Grant( outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { + // Idempotency: GitLab returns 409 (uhttp → codes.AlreadyExists) when the user is + // already a member, and a 400 "should be greater than or equal to" when they already + // hold an equal/higher role. Both mean the desired grant already holds. + if isAlreadyExistsError(err) || strings.Contains(err.Error(), "should be greater than or equal to") { + return annotations.New(&v2.GrantAlreadyExists{}), nil + } return outputAnnotations, fmt.Errorf("error adding user to project: %w", err) } @@ -286,7 +293,7 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio projectId := grant.Entitlement.Resource.Id.Resource userId, err := strconv.Atoi(grant.Principal.Id.Resource) if err != nil { - return nil, fmt.Errorf("error converting user ID to int: %w", err) + return nil, status.Errorf(codes.InvalidArgument, "baton-gitlab: invalid user ID: %v", err) } rateLimitDesc, err := o.client.RemoveProjectMember(ctx, projectId, strconv.Itoa(userId)) @@ -294,7 +301,14 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio outputAnnotations.WithRateLimiting(rateLimitDesc) } if err != nil { - if errors.Is(err, client.ErrNotFound) { + if isNotFoundError(err) { + // With access-path labeling off (default), preserve the historical behavior: + // an absent direct membership is reported as already revoked. The effective- + // access check below only applies in access-path mode, so default-mode + // deployments see no provisioning behavior change. + if !o.client.SyncAccessPaths { + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which // would reappear next sync). Otherwise the membership is genuinely gone. @@ -305,10 +319,11 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio switch { case checkErr == nil && member != nil: return outputAnnotations, revokeInheritedError("project", projectId) - case checkErr != nil && !errors.Is(checkErr, client.ErrNotFound): + case checkErr != nil && !isNotFoundError(checkErr): return outputAnnotations, fmt.Errorf("error verifying project membership: %w", checkErr) default: - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) + return outputAnnotations, nil } } return outputAnnotations, fmt.Errorf("error removing user from project: %w", err) From 5df18f7848193e302110d5f1a9fb3c3c81eea5f8 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Thu, 16 Jul 2026 14:07:44 -0300 Subject: [PATCH 09/15] chore(gitlab): remove constants left unused by main's rename after rebase Rebasing onto main kept the branch's original sortOrderAsc (helper.go) and fieldName/fieldEmail (resource_types.go) constants after main had renamed them to sortAscending and profileFieldName/profileFieldEmail. Every usage now references main's names, so the originals were dead code and failed golangci-lint (unused). Remove them. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/client/helper.go | 3 --- pkg/connector/resource_types.go | 7 ------- 2 files changed, 10 deletions(-) diff --git a/pkg/connector/client/helper.go b/pkg/connector/client/helper.go index d51e1492..5856d4d5 100644 --- a/pkg/connector/client/helper.go +++ b/pkg/connector/client/helper.go @@ -22,9 +22,6 @@ func WithQueryParam(key string, value string) ReqOpt { } } -// sortOrderAsc is the ascending sort direction used for keyset pagination. -const sortOrderAsc = "asc" - type KeysetPaginationOpts struct { OrderBy string Sort string diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index 01001a36..5a93080c 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -23,10 +23,3 @@ var projectResourceType = &v2.ResourceType{ Id: "project", DisplayName: "Project", } - -// Shared field-name keys used by the account-creation schema and resource -// profiles. Extracted so the literal is defined once (goconst). -const ( - fieldName = "name" - fieldEmail = "email" -) From ae79df2b22f502e16af8e8bb85465801d4c91a88 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Thu, 16 Jul 2026 14:30:09 -0300 Subject: [PATCH 10/15] chore(gitlab): remove orphaned client-go replace directive from go.mod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The replace of gitlab.com/gitlab-org/api/client-go with the jirwin fork had no corresponding require and no import anywhere in the connector (a leftover from when the connector used the GitLab SDK before switching to a hand-rolled uhttp client). A replace directive with no requirer is dead, and golangci-lint's gomoddirectives flags it, failing the go-lint CI job. Remove it (and its vendor/modules.txt annotation, via go mod vendor) so the lint passes. No build or behavior change — nothing referenced the replaced module. Co-Authored-By: Claude Opus 4.8 (1M context) --- go.mod | 2 -- vendor/modules.txt | 1 - 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index d67136f0..900798e7 100644 --- a/go.mod +++ b/go.mod @@ -146,5 +146,3 @@ require ( modernc.org/memory v1.11.0 // indirect modernc.org/sqlite v1.50.0 // indirect ) - -replace gitlab.com/gitlab-org/api/client-go => gitlab.com/jirwin/client-go v0.123.1-0.20250228021302-c3f0af7d3169 diff --git a/vendor/modules.txt b/vendor/modules.txt index e85849b5..5aa58656 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -992,4 +992,3 @@ modernc.org/memory modernc.org/sqlite modernc.org/sqlite/lib modernc.org/sqlite/vtab -# gitlab.com/gitlab-org/api/client-go => gitlab.com/jirwin/client-go v0.123.1-0.20250228021302-c3f0af7d3169 From bf8ab09405b2b6011602c8c1dc7b0abb710382d9 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Fri, 17 Jul 2026 10:42:02 -0300 Subject: [PATCH 11/15] refactor(gitlab): address revoke-path review nits (helper, dead branch, prefix, docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up cleanups from PR #92 review, all behavior-preserving: - Extract isAlreadyAtOrAboveLevelError so GitLab's "should be greater than or equal to" 400 idempotency match lives in one place instead of a copy-pasted strings.Contains in both groups.go and projects.go — a GitLab wording change now only has to be fixed once. - Drop the always-true `member != nil` guard in group/project Revoke: GetGroupMemberAll/GetProjectMemberAll never return a nil member on a nil error, so the switch keys on checkErr alone. - Add the baton-gitlab: prefix to the new "error verifying membership" wraps per CLAUDE.md's error convention (only the new lines; pre-existing inconsistencies left for a separate pass). - Correct the GetGroupMemberAll/GetProjectMemberAll doc comments: they surface a gRPC NotFound status, not the ErrNotFound sentinel (doRequest errors on 4xx via uhttp before CheckResponse runs), matching the isNotFoundError logic. Validated live against the tenant: default and --sync-access-paths full syncs clean (expansion resolves with no orphans), and group + project grant->revoke round-trips pass with tenant state restored. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/client/client.go | 14 ++++++++------ pkg/connector/groups.go | 12 +++++++----- pkg/connector/helpers.go | 10 ++++++++++ pkg/connector/projects.go | 12 +++++++----- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 7a128512..bf478f17 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -193,9 +193,10 @@ func (c *GitlabClient) ListAllGroupMembers(ctx context.Context, groupID string, } // GetGroupMemberAll retrieves a single member of a group including inherited and -// invited (shared) members (GET /groups/:id/members/all/:user_id). Returns -// ErrNotFound when the user has no effective access to the group. Used to tell a -// direct membership apart from inherited/invited access on revoke. +// invited (shared) members (GET /groups/:id/members/all/:user_id). Returns a gRPC +// NotFound status (not the ErrNotFound sentinel — doRequest surfaces uhttp's 4xx +// error before CheckResponse runs) when the user has no effective access to the +// group. Used to tell a direct membership apart from inherited/invited access on revoke. func (c *GitlabClient) GetGroupMemberAll(ctx context.Context, groupID, userID string) (*GroupMember, *v2.RateLimitDescription, error) { var member GroupMember @@ -350,9 +351,10 @@ func (c *GitlabClient) ListAllProjectMembers(ctx context.Context, projectID stri } // GetProjectMemberAll retrieves a single member of a project including inherited and -// invited (shared) members (GET /projects/:id/members/all/:user_id). Returns -// ErrNotFound when the user has no effective access to the project. Used to tell a -// direct membership apart from inherited/invited access on revoke. +// invited (shared) members (GET /projects/:id/members/all/:user_id). Returns a gRPC +// NotFound status (not the ErrNotFound sentinel — doRequest surfaces uhttp's 4xx +// error before CheckResponse runs) when the user has no effective access to the +// project. Used to tell a direct membership apart from inherited/invited access on revoke. func (c *GitlabClient) GetProjectMemberAll(ctx context.Context, projectID, userID string) (*ProjectMember, *v2.RateLimitDescription, error) { var member ProjectMember diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 32af69e8..1ac979b3 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -339,7 +339,7 @@ func (o *groupBuilder) Grant( // hold an equal/higher role. Both mean the desired grant already holds. (The gRPC // code is checked directly because the hand-rolled *ErrorResponse never reaches the // error chain for doRequest calls — uhttp errors on 4xx before CheckResponse runs.) - if isAlreadyExistsError(err) || strings.Contains(err.Error(), "should be greater than or equal to") { + if isAlreadyExistsError(err) || isAlreadyAtOrAboveLevelError(err) { return annotations.New(&v2.GrantAlreadyExists{}), nil } return outputAnnotations, fmt.Errorf("error adding user to group: %w", err) @@ -372,15 +372,17 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which // would reappear next sync). Otherwise the membership is genuinely gone. - member, rlDesc, checkErr := o.client.GetGroupMemberAll(ctx, groupId, grant.Principal.Id.Resource) + _, rlDesc, checkErr := o.client.GetGroupMemberAll(ctx, groupId, grant.Principal.Id.Resource) if rlDesc != nil { outputAnnotations.WithRateLimiting(rlDesc) } + // GetGroupMemberAll always returns a non-nil member when checkErr is nil, + // so a nil-check would be dead here; keying on checkErr is sufficient. switch { - case checkErr == nil && member != nil: + case checkErr == nil: return outputAnnotations, revokeInheritedError("group", groupIdAndName) - case checkErr != nil && !isNotFoundError(checkErr): - return outputAnnotations, fmt.Errorf("error verifying group membership: %w", checkErr) + case !isNotFoundError(checkErr): + return outputAnnotations, fmt.Errorf("baton-gitlab: error verifying group membership: %w", checkErr) default: outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) return outputAnnotations, nil diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 96b54c31..0d933262 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -77,6 +77,16 @@ func isAlreadyExistsError(err error) bool { return status.Code(err) == codes.AlreadyExists } +// isAlreadyAtOrAboveLevelError reports whether err is GitLab's 400 rejection raised when +// the user already holds an equal or higher role than the one being granted ("access level +// should be greater than or equal to ..."). Like a 409, this means the desired grant already +// holds, so it is treated as idempotent success. Matched by message text because GitLab does +// not expose a distinct status code for it; centralized so a GitLab wording change is fixed +// in one place for both groups and projects. +func isAlreadyAtOrAboveLevelError(err error) bool { + return err != nil && strings.Contains(err.Error(), "should be greater than or equal to") +} + func handlePermissionError(ctx context.Context, err error, resourceType, resourceId string) (bool, error) { if err == nil { return false, nil diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index d8b4fde6..04dcab80 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -278,7 +278,7 @@ func (o *projectBuilder) Grant( // Idempotency: GitLab returns 409 (uhttp → codes.AlreadyExists) when the user is // already a member, and a 400 "should be greater than or equal to" when they already // hold an equal/higher role. Both mean the desired grant already holds. - if isAlreadyExistsError(err) || strings.Contains(err.Error(), "should be greater than or equal to") { + if isAlreadyExistsError(err) || isAlreadyAtOrAboveLevelError(err) { return annotations.New(&v2.GrantAlreadyExists{}), nil } return outputAnnotations, fmt.Errorf("error adding user to project: %w", err) @@ -312,15 +312,17 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which // would reappear next sync). Otherwise the membership is genuinely gone. - member, rlDesc, checkErr := o.client.GetProjectMemberAll(ctx, projectId, strconv.Itoa(userId)) + _, rlDesc, checkErr := o.client.GetProjectMemberAll(ctx, projectId, strconv.Itoa(userId)) if rlDesc != nil { outputAnnotations.WithRateLimiting(rlDesc) } + // GetProjectMemberAll always returns a non-nil member when checkErr is nil, + // so a nil-check would be dead here; keying on checkErr is sufficient. switch { - case checkErr == nil && member != nil: + case checkErr == nil: return outputAnnotations, revokeInheritedError("project", projectId) - case checkErr != nil && !isNotFoundError(checkErr): - return outputAnnotations, fmt.Errorf("error verifying project membership: %w", checkErr) + case !isNotFoundError(checkErr): + return outputAnnotations, fmt.Errorf("baton-gitlab: error verifying project membership: %w", checkErr) default: outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) return outputAnnotations, nil From 8443f03710fcb84e1a60ab7394d423a0d53722f9 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Fri, 17 Jul 2026 13:44:09 -0300 Subject: [PATCH 12/15] fix(gitlab): preserve rate-limit annotations on the flag-off already-revoked path The default (SyncAccessPaths off) branch of Revoke returned a fresh `annotations.New(&v2.GrantAlreadyRevoked{})`, discarding `outputAnnotations` and with it any rate-limit descriptor already attached from the RemoveGroupMember / RemoveProjectMember call. The access-path branch a few lines down already uses `outputAnnotations.Update(...)`; match it so the no-op revoke path keeps feeding the SDK's throttling. Same fix in groups.go and projects.go. No behavior change beyond annotation fidelity. Addresses sergiocorral-conductorone review nit on PR #92. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/groups.go | 3 ++- pkg/connector/projects.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 1ac979b3..a75e5c32 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -367,7 +367,8 @@ func (o *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations // access check below only applies in access-path mode, so default-mode // deployments see no provisioning behavior change. if !o.client.SyncAccessPaths { - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) + return outputAnnotations, nil } // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index 04dcab80..226dcf3f 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -307,7 +307,8 @@ func (o *projectBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotatio // access check below only applies in access-path mode, so default-mode // deployments see no provisioning behavior change. if !o.client.SyncAccessPaths { - return annotations.New(&v2.GrantAlreadyRevoked{}), nil + outputAnnotations.Update(&v2.GrantAlreadyRevoked{}) + return outputAnnotations, nil } // Not a direct member. If the principal still has effective access, it is // inherited/invited — reject instead of falsely reporting success (which From f89bc64f865129ff5967693c2c465ee34235f09f Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Fri, 17 Jul 2026 14:00:24 -0300 Subject: [PATCH 13/15] fix(gitlab): sync invited groups' inherited members on project access paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under --sync-access-paths, group->project sharing was under-synced: the invited path built its grants from ListGroupMembers (direct members only), so an inherited member of the invited group whose level had no direct member in that group was dropped entirely. GitLab group->project sharing confers access to the invited group's *effective* members (direct AND inherited), so those users had real access that never surfaced. Add accessPathInvitedProjectGrants (+ allEffectiveGroupMembers over /members/all): for a project target, emit one user-principal grant per effective member of each invited group, capped at min(memberLevel, shareLevel), marked GrantImmutable (the access is removed by un-sharing or editing the invited group, not at the project). Group targets keep accessPathInvitedGrants unchanged (group->group sharing confers to direct members only). Update docs/doc-info.md: describe the project vs group invited model and drop the now-resolved 'inherited members surfaced elsewhere' limitation. Opt-in only: default (flag-off) output is byte-identical. Validated live against a test tenant with an invited subgroup carrying inherited members — the inherited member (level 30, capped under a level-40 share) is absent before this change and present at Developer after, with the default-mode grant set unchanged (88 == 88). Addresses sergiocorral-conductorone [Major] review on PR #92. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/doc-info.md | 23 +++++------ pkg/connector/helpers.go | 85 ++++++++++++++++++++++++++++++++++++--- pkg/connector/projects.go | 2 +- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/docs/doc-info.md b/docs/doc-info.md index 35e83f63..2eead33d 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -54,11 +54,14 @@ surfaced as its own expandable grant, so the access path is visible and reviewab entitlement. Each ancestor (immediate subgroup up to the top-level group) is a distinct path, so a top-level group member is shown as having access to nested projects through the top-level group specifically. - - Access via an invited (shared) group — the invited group as principal on the - target, expanding into the invited group's per-level entitlements. GitLab caps a - shared member's effective access at min(their level in the invited group, the - share's access level); the connector applies that cap per level, so no member is - shown at a higher level than GitLab actually grants. + - Access via an invited (shared) group. For a group target, the invited group is the + principal on the target, expanding into the invited group's per-level entitlements — + group→group sharing confers access to the invited group's direct members only. For a + project target, group→project sharing also confers access to the invited group's + inherited members, so each effective member (direct or inherited) is emitted as a + user-principal grant. Either way GitLab caps a shared member's effective access at + min(their level in the invited group, the share's access level); the connector + applies that cap, so no member is shown at a higher level than GitLab actually grants. Design notes: - Expandable grants use Shallow=true: each grant is a single, crisp hop, and the @@ -66,8 +69,8 @@ Design notes: - No new entitlements are added — the model reuses the existing per-access-level entitlements, and only emits paths for access levels that actually have members, so no grant expands to an empty set. - - Inherited and invited (shared) paths are attribution-only: their expandable grants - are marked immutable, so the platform never offers a direct revoke of access that + - Inherited and invited (shared) paths are attribution-only: their grants are marked + immutable, so the platform never offers a direct revoke of access that is really held elsewhere. Revoking such access at the target is rejected (InvalidArgument) and points at its source group, since removing a non-existent direct membership is a no-op that would reappear on the next sync. Direct @@ -77,12 +80,6 @@ Design notes: - `--sync-direct-members-only` still applies: with it set, only direct members are synced and the inheritance/invited paths are not emitted. -Known limitation: GitLab group→project sharing also grants access to the invited -group's inherited members. When the invited group is itself a subgroup, those -inherited members are surfaced via their own group's path rather than the invited -path. Invited top-level groups (the common case) are unaffected, since their direct -and effective membership are the same. - Known limitation: an inheritance/invited access path is expandable into the ancestor or invited group's per-level entitlements. If that group is not itself in the connector's sync scope (for example, the token can see the share but the group is not returned as a diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 0d933262..5d82490c 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -263,6 +263,29 @@ func allDirectGroupMembers(ctx context.Context, c *client.GitlabClient, groupID } } +// allEffectiveGroupMembers pages through a group's effective members — direct plus +// inherited (GET /groups/:id/members/all) — accumulating rate-limit annotations. Used +// for group→project invited access, which confers on the invited group's inherited +// members too, not just its direct ones. +func allEffectiveGroupMembers(ctx context.Context, c *client.GitlabClient, groupID string, annos *annotations.Annotations) ([]*client.GroupMember, error) { + var all []*client.GroupMember + pageToken := "" + for { + members, next, rateLimitDesc, err := c.ListAllGroupMembers(ctx, groupID, pageToken) + if rateLimitDesc != nil { + annos.WithRateLimiting(rateLimitDesc) + } + if err != nil { + return all, err + } + all = append(all, members...) + if next == "" { + return all, nil + } + pageToken = next + } +} + // accessPathInheritanceGrants walks the target's ancestor group chain and emits, per // ancestor, "access via membership" expandable grants for the levels that // ancestor actually has direct members at. Together the per-ancestor edges surface @@ -309,12 +332,12 @@ func accessPathInheritanceGrants(ctx context.Context, c *client.GitlabClient, ta return grants, nil } -// accessPathInvitedGrants resolves each invited (shared) group's direct members and -// emits the invited-group access paths for the target. GitLab group→group sharing -// confers access only to the invited group's direct members; group→project sharing -// also confers it to the invited group's inherited members, so for an invited -// *subgroup* shared into a project those inherited members are surfaced via their own -// group's path rather than this one (see docs/doc-info.md). +// accessPathInvitedGrants resolves each invited (shared) group's DIRECT members and emits +// group-as-principal expandable grants for a GROUP target. GitLab group→group sharing +// confers access only to the invited group's direct members, so direct enumeration is +// correct here. For a PROJECT target use accessPathInvitedProjectGrants instead — +// group→project sharing also confers access to the invited group's inherited members +// (see docs/doc-info.md). func accessPathInvitedGrants(ctx context.Context, c *client.GitlabClient, target *v2.Resource, shared []client.SharedGroup, annos *annotations.Annotations) ([]*v2.Grant, error) { if len(shared) == 0 { return nil, nil @@ -336,3 +359,53 @@ func accessPathInvitedGrants(ctx context.Context, c *client.GitlabClient, target } return invitedGroupGrants(target, shared, membersByGroup), nil } + +// accessPathInvitedProjectGrants emits grants for groups invited (shared) into a project. +// GitLab group→project sharing confers access to the invited group's EFFECTIVE members — +// direct AND inherited — each capped at min(memberLevel, shareLevel). Emitted as +// user-principal grants (marked immutable: the access is removed by un-sharing or editing +// the invited group, not at the project), so an inherited member whose level has no direct +// member in the invited group is not dropped (the group→group path only covers directs). +func accessPathInvitedProjectGrants(ctx context.Context, c *client.GitlabClient, target *v2.Resource, shared []client.SharedGroup, annos *annotations.Annotations) ([]*v2.Grant, error) { + if len(shared) == 0 { + return nil, nil + } + var grants []*v2.Grant + for _, sg := range shared { + share := client.AccessLevelValue(sg.GroupAccessLevel) + members, err := allEffectiveGroupMembers(ctx, c, strconv.Itoa(sg.GroupID), annos) + if err != nil { + isPermissionError, unhandledErr := handlePermissionError(ctx, err, "group", strconv.Itoa(sg.GroupID)) + if unhandledErr != nil { + return nil, unhandledErr + } + if isPermissionError { + continue + } + } + for _, m := range members { + lvl := client.AccessLevelValue(m.AccessLevel) + // A level outside GitLab's defined set has no entitlement slug; skip it so we + // never build a grant on an empty-slug entitlement. + if lvl < client.MinimalAccessPermissions || lvl > client.OwnerPermissions || lvl.String() == "" { + continue + } + eff := lvl + if share >= client.MinimalAccessPermissions && share < eff { + eff = share + } + if eff.String() == "" { + continue + } + grants = append(grants, grant.NewGrant( + target, + eff.String(), + &v2.ResourceId{ResourceType: userResourceType.Id, Resource: strconv.Itoa(m.ID)}, + // Invited-group access is not revocable at the target: it is removed by + // un-sharing or editing the invited group. + grant.WithAnnotation(&v2.GrantImmutable{}), + )) + } + } + return grants, nil +} diff --git a/pkg/connector/projects.go b/pkg/connector/projects.go index 226dcf3f..f3c9f4be 100644 --- a/pkg/connector/projects.go +++ b/pkg/connector/projects.go @@ -232,7 +232,7 @@ func (o *projectBuilder) grantsWithAccessPaths(ctx context.Context, resource *v2 } // Permission error: skip invited-group grants and continue. } else { - invited, err := accessPathInvitedGrants(ctx, o.client, resource, project.SharedWithGroups, &outputAnnotations) + invited, err := accessPathInvitedProjectGrants(ctx, o.client, resource, project.SharedWithGroups, &outputAnnotations) if err != nil { return nil, "", outputAnnotations, err } From a550269ce627a55f451012bc036c0f3d24ee3329 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Fri, 17 Jul 2026 14:24:13 -0300 Subject: [PATCH 14/15] fix(gitlab): dedupe invited-project grants against direct members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The invited-project access path emits user-principal grants marked GrantImmutable. Because a grant ID is target + entitlement + principal, a user who is BOTH a direct project member and an effective member of a group shared into the same project, at the same (capped) level, produces the identical grant ID from both paths. The direct membership is revocable; the invited grant is immutable — and depending on dedup order the immutable annotation could win, marking a legitimately-revocable direct membership immutable and blocking its revoke. accessPathInvitedProjectGrants now drains the project's direct members and skips emitting an invited grant when the user is already a direct member at that exact level, so the revocable direct grant stays authoritative. The full drain is required for correctness (a colliding direct member can land on a later SDK page). Also dedupe a user who is an effective member of multiple invited groups capping to the same level, so the level's grant is emitted once. Opt-in only (--sync-access-paths); default output unchanged. Validated live: a user who is direct@Developer and invited-effective@Developer is immutable=true before this change (revoke blocked) and immutable=false after (direct wins); an invited-only member stays immutable. Addresses the github-actions review bot's collision comment on PR #92. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/connector/helpers.go | 57 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 5d82490c..0561ab6e 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -286,6 +286,27 @@ func allEffectiveGroupMembers(ctx context.Context, c *client.GitlabClient, group } } +// allDirectProjectMembers pages through a project's direct members, accumulating +// rate-limit annotations. Used to dedupe invited-group grants against direct membership. +func allDirectProjectMembers(ctx context.Context, c *client.GitlabClient, projectID string, annos *annotations.Annotations) ([]*client.ProjectMember, error) { + var all []*client.ProjectMember + pageToken := "" + for { + members, next, rateLimitDesc, err := c.ListProjectMembers(ctx, projectID, pageToken) + if rateLimitDesc != nil { + annos.WithRateLimiting(rateLimitDesc) + } + if err != nil { + return all, err + } + all = append(all, members...) + if next == "" { + return all, nil + } + pageToken = next + } +} + // accessPathInheritanceGrants walks the target's ancestor group chain and emits, per // ancestor, "access via membership" expandable grants for the levels that // ancestor actually has direct members at. Together the per-ancestor edges surface @@ -370,6 +391,26 @@ func accessPathInvitedProjectGrants(ctx context.Context, c *client.GitlabClient, if len(shared) == 0 { return nil, nil } + // A user who is a direct project member at a given level already has a revocable grant + // from the direct-members path. An immutable invited grant with the identical grant ID + // (target + entitlement + user) could override it and block a legitimate revoke, so we + // skip that exact collision — the direct membership wins. On a permission error we + // simply can't build the map; the direct-members path then emits no grants either, so + // there is nothing to collide with. + directLevel := make(map[int]client.AccessLevelValue) + directMembers, err := allDirectProjectMembers(ctx, c, target.Id.Resource, annos) + if err != nil { + if _, unhandledErr := handlePermissionError(ctx, err, "project", target.Id.Resource); unhandledErr != nil { + return nil, unhandledErr + } + } + for _, member := range directMembers { + directLevel[member.ID] = client.AccessLevelValue(member.AccessLevel) + } + + // A user can be an effective member of more than one invited group that caps to the + // same level; emit their grant on that level once. + emitted := make(map[string]struct{}) var grants []*v2.Grant for _, sg := range shared { share := client.AccessLevelValue(sg.GroupAccessLevel) @@ -383,8 +424,8 @@ func accessPathInvitedProjectGrants(ctx context.Context, c *client.GitlabClient, continue } } - for _, m := range members { - lvl := client.AccessLevelValue(m.AccessLevel) + for _, member := range members { + lvl := client.AccessLevelValue(member.AccessLevel) // A level outside GitLab's defined set has no entitlement slug; skip it so we // never build a grant on an empty-slug entitlement. if lvl < client.MinimalAccessPermissions || lvl > client.OwnerPermissions || lvl.String() == "" { @@ -397,10 +438,20 @@ func accessPathInvitedProjectGrants(ctx context.Context, c *client.GitlabClient, if eff.String() == "" { continue } + // Exact collision with a direct membership at the same level: the direct + // (revocable) grant is authoritative — skip the immutable invited duplicate. + if directLevel[member.ID] == eff { + continue + } + dedupeKey := fmt.Sprintf("%d:%s", member.ID, eff.String()) + if _, ok := emitted[dedupeKey]; ok { + continue + } + emitted[dedupeKey] = struct{}{} grants = append(grants, grant.NewGrant( target, eff.String(), - &v2.ResourceId{ResourceType: userResourceType.Id, Resource: strconv.Itoa(m.ID)}, + &v2.ResourceId{ResourceType: userResourceType.Id, Resource: strconv.Itoa(member.ID)}, // Invited-group access is not revocable at the target: it is removed by // un-sharing or editing the invited group. grant.WithAnnotation(&v2.GrantImmutable{}), From 46725c164377a4e3ba724330e8f0d2d2b6c2435d Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Fri, 17 Jul 2026 15:53:16 -0300 Subject: [PATCH 15/15] fix(gitlab): regenerate config schema to expose sync-access-paths flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sync-access-paths is declared in pkg/config/config.go and included in ConfigurationFields, but config_schema.json — consumed by the C1 platform to render the connector config UI — was never regenerated after the flag was added (a rebase reset it to main's bot-generated copy), so the opt-in flag this PR is built around was not exposed. Regenerated via `./connector config`, the same command capabilities_and_config.yaml runs. Co-Authored-By: Claude Opus 4.8 (1M context) --- config_schema.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/config_schema.json b/config_schema.json index 4814afb9..6686b319 100644 --- a/config_schema.json +++ b/config_schema.json @@ -126,6 +126,12 @@ "displayName": "Sync direct members only", "description": "When enabled, only sync direct members of groups and projects. Inherited members from parent groups will be excluded from membership grants.", "boolField": {} + }, + { + "name": "sync-access-paths", + "displayName": "Sync access paths", + "description": "Label grants by access path (direct, inherited, or via an invited group). Disabled (default) keeps the previous flattened effective-membership grants.", + "boolField": {} } ], "displayName": "GitLab",