Add exclusion group#126
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| require ( | ||
| github.com/SAP/go-hdb v1.14.5 | ||
| github.com/conductorone/baton-sdk v0.9.10 | ||
| github.com/conductorone/baton-sdk v0.8.22-0.20260515185523-d9071b77d186 |
There was a problem hiding this comment.
🟠 Bug: SDK version downgraded from released v0.9.10 to unreleased pseudo-version v0.8.22-0.20260515185523-d9071b77d186. This is a major semver regression that could break existing functionality. This should be replaced with a proper tagged release that includes the EntitlementExclusionGroup and GrantReplaced types before merging.
| if replace != nil && replace.Query != "" { | ||
| var ret []*v2.Grant | ||
|
|
||
| _, err := s.runQuery(ctx, s.db, nil, replace.Query, nil, vars, func(ctx context.Context, rowMap map[string]any) (bool, error) { |
There was a problem hiding this comment.
🟡 Suggestion: The replace query reads from s.db directly while the subsequent revoke and grant writes use the transactional executor. This creates a TOCTOU window — the grant state could change between the read and the writes. The // Needs to fix the TX comment on line 704 acknowledges this; consider passing executor here instead of s.db so the entire replace-revoke-grant sequence runs within the same transaction.
| valid := result.Next() | ||
|
|
||
| if err := result.Err(); err != nil { | ||
| return anno, fmt.Errorf("failed to read validation query result: %w", err) |
There was a problem hiding this comment.
🟡 Suggestion: If result.Err() returns an error, result.Close() is never called, leaking the sql.Rows. Add defer result.Close() immediately after the QueryContext call (before calling result.Next()).
| return anno, errors.New("no revoke config found for entitlement") | ||
| } | ||
|
|
||
| provisioningVars, err := s.prepareProvisioningVars(ctx, provisioningConfig.Vars, grantToRevoke.GetPrincipal(), grantToRevoke.GetEntitlement()) |
There was a problem hiding this comment.
🟡 Suggestion: grantToRevoke.GetPrincipal() returns a *v2.Resource with only the Id field populated (built by sdkGrant.NewGrant from a *v2.ResourceId). If any provisioning var CEL expressions reference principal.ParentResourceId or profile fields, they will evaluate to nil/empty and the revoke query may use incorrect parameters. Consider whether the replace-revoke flow needs to look up the full principal resource first.
Connector PR Review: Add exclusion groupBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits address all four findings from the previous full review: Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
| executor, | ||
| ) | ||
| if err != nil { | ||
| if !errors.Is(err, ErrQueryAffectedZeroRows) { |
There was a problem hiding this comment.
🔴 Bug: Returning an error when the replace query finds 0 existing grants prevents first-time grants into an exclusion group. Previously this was a no-op fall-through that proceeded to run the grant queries. If no existing grant needs replacing, the function should continue to the grant step, not fail.
| if !errors.Is(err, ErrQueryAffectedZeroRows) { | |
| case len(ret) == 0: | |
| // No existing grant to replace — proceed with the new grant. |
| l.Info("running grant replace provisioning query", zap.String("query", replace.Query)) | ||
|
|
||
| var ret []*v2.Grant | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: The // Needs to fix the TX comment was removed in this commit, but s.runQuery(ctx, s.db, ...) still reads from the non-transactional s.db instead of executor. The TOCTOU race flagged in the previous review persists and is now undocumented.
| continue | ||
| } | ||
|
|
||
| if mapping.SkipIf != "" { |
There was a problem hiding this comment.
🟡 Suggestion: CEL evaluation errors on SkipIf are silently swallowed here. A typo or invalid expression in the config will be invisible. Consider logging the error or returning it instead of silently continuing.
…d baton-sql changes land); verified that it works for testing for now
MarcusGoldschmidt
force-pushed
the
goldschmidt/exclusion-group
branch
from
692cbc2 to
5d81a3b
Compare
| var committed bool | ||
| var executor executor = s.db | ||
|
|
||
| if useTx { | ||
| tx, err := s.db.BeginTx(ctx, nil) |
There was a problem hiding this comment.
🔴 Security: RunGrantProvisioning uses s.db directly instead of calling resolveProvisioningDB(vars) like RunProvisioningQueries does. This bypasses multi-database routing added in #124, so grant operations targeting a non-primary database will silently execute against the wrong database handle.
| skip, err := s.env.EvaluateBool(ctx, mapping.SkipIf, rowMap) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
🟡 Suggestion: SkipIf CEL evaluation errors are silently swallowed with continue. A config typo in skip_if expressions will be invisible during provisioning. Consider at minimum a debug-level log, or returning the error.
|
|
||
| var ret []*v2.Grant | ||
|
|
||
| _, err := s.runQuery(ctx, s.db, nil, replace.Query, nil, vars, func(ctx context.Context, rowMap map[string]any) (bool, error) { |
There was a problem hiding this comment.
🟡 Suggestion: The grant-replace read (s.runQuery(ctx, s.db, ...)) runs outside the transaction while the subsequent revoke and grant use executor (the tx). This is a TOCTOU race — a concurrent grant could change the result between the read and the revoke. Pass the transactional executor instead of s.db to make the read-revoke-grant sequence atomic.
| } | ||
|
|
||
| valid := result.Next() | ||
|
|
||
| if err := result.Err(); err != nil { | ||
| return anno, fmt.Errorf("failed to read validation query result: %w", err) |
There was a problem hiding this comment.
🟡 Suggestion: sql.Rows resource leak — if result.Err() returns an error on line 851, the function returns before result.Close() is called. Add defer result.Close() immediately after QueryContext succeeds. The same pattern exists in RunProvisioningQueriesWithExecutor around line 470.
laurenleach
dismissed
github-actions[bot]’s stale review
the bot reviewed after changes were addressed but the PR is still blocked #126 (review)
3 participants
Description
Useful links: