[CXH-1974] - Label grants by access path and sync invited-group relationships - GitLab Connector#92
Conversation
Connector PR Review: [CXH-1974] - Label grants by access path and sync invited-group relationships - GitLab ConnectorBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review Summary Security Issues Correctness Issues Suggestions
Prompt for AI agentsSuggestions (pkg/connector/helpers.go):
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ff29ef6 to
ce7ac19
Compare
mateoHernandez123
assigned
btipling,
johnallers and
mateoHernandez123 and unassigned
johnallers
There was a problem hiding this comment.
On the first re-sync after deploy, the previously flattened direct grants for inherited/shared members on projects and subgroups are replaced by path-specific expandable grants. The same users retain access (validated: the expansion resolves to the same effective set), but access is now attributed to its actual path.
We cannot break everyone's gitlab just for a trade desk request yalll.
|
Either work with customer success and get their buy in to ship this or make it configurable. This changes grant counts, shape. We can't just sneak it in. Maybe it hasn't. If that's all been established, dismiss my rejection or request a re-review from me. |
|
Just for context we have repeatedly iterated on gitlab group pagination, and we also have enterprise/cloud gitlab. This is a very important connector for a bunch of important customers. This has be done carefully. |
|
@btipling you're right — this changed grant shape/counts on the default path, which we shouldn't ship to every GitLab tenant. Fixed in 1dd042e: all of the path-visibility behavior is now gated behind a new opt-in flag
Existing deployments upgrade with zero change; only tenants that explicitly enable the flag get the new model. Nothing is snuck in and no counts move by default. Could you re-review / dismiss the change request? |
… 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) <noreply@anthropic.com>
3f01f00 to
da9c736
Compare
…base 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
This should be a careful rollout
…h, prefix, docs) 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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
… paths 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Follow-up re-review at HEAD a550269 (delta since our last review at bf8ab09).
Both previously-tracked concerns resolved as follows:
Invited-group PROJECT under-sync (verifier-flagged, ae79df2): FIXED — new accessPathInvitedProjectGrants now uses effective members (/groups/:id/members/all, direct+inherited) instead of direct-only, so inherited-only members at a level are no longer dropped.
Prior MAJOR revoke findings (groups.go/projects.go inherited/invited rejecting with InvalidArgument): still fixed, and this round further improves the default-mode branch to preserve accumulated rate-limit annotations (outputAnnotations.Update instead of annotations.New).
[Major, carried over from last review, still open] config_schema.json still omits the sync-access-paths field — and isn't touched anywhere in this PR's diff, confirming it was never regenerated. pkg/config/config.go declares SyncAccessPaths = field.BoolField("sync-access-paths", ...) and includes it in ConfigurationFields, wired into the connector via main.go — but the generated schema's fields array still ends at sync-direct-members-only (config_schema.json:125) with no sync-access-paths entry. The C1 platform consumes this generated schema, so the flagship opt-in flag this whole PR is built around still isn't exposed in the platform UI. Please regenerate config_schema.json (go generate ./... / make generate) and commit it. (Verifier: CONFIRM — real regression, unresolved at current HEAD; can't be left as a line comment since the file has no diff hunk in this PR.)
One inline Minor finding below.
6 participants
Description
[CXH-1974] Labels grants by access path (opt-in via
--sync-access-paths) so reviewers can see how access was obtained — direct, inherited from a parent/top-level group, or via an invited (shared) group — instead of a single flattened list. This feeds C1's native "grant source: direct vs inherited" model used in access reviews and automations.The default is unchanged (byte-for-byte). With the flag off, the connector emits the same flattened effective-membership grants as before (verified: 81 grants / 120 entitlements on the test tenant, identical shape to v0.0.31). Existing deployments are unaffected until they explicitly opt in.
Model (flag on)
Shallow=true— one crisp hop per ancestor; the platform resolves deeper nesting.min(level, share level), expanding the invited group's matching per-level entitlements.Design goals:
member/effective-memberanchors).InvalidArgument(pointing at the source) instead of a silent no-op that reappears on the next sync.Validation
Verified end-to-end in a C1 service-mode tenant: inherited access resolves and deduplicates on a depth-2 nested project, invited-group paths surface at the capped level, and the account view exposes each path.
--sync-direct-members-onlystill applies (suppresses the inherited/invited paths).Docs updated:
README.md,docs/doc-info.md,docs/connector.mdx.Useful links