Skip to content

Add reject_if grant cancellation support#131

Merged
MarcusGoldschmidt merged 3 commits into
mainfrom
alanlee/fde-222-baton-sql-new-reject_if-config-support
Jun 2, 2026
Merged

Add reject_if grant cancellation support#131
MarcusGoldschmidt merged 3 commits into
mainfrom
alanlee/fde-222-baton-sql-new-reject_if-config-support

Conversation

@alan-lee-12

@alan-lee-12 alan-lee-12 commented May 27, 2026

Copy link
Copy Markdown
Contributor

Description

  • Add reject_if grant provisioning checks that cancel matching grants before any grant mutations run.

@alan-lee-12 alan-lee-12 requested a review from a team May 27, 2026 23:44
@linear-code

linear-code Bot commented May 27, 2026

Copy link
Copy Markdown

FDE-222

@@ -0,0 +1,26 @@
package grant

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: These vendor files (rejected.go and annotation_grant_rejected.pb.go) don't exist in baton-sdk v0.9.20 (the version in go.mod), nor on the SDK's main branch. Running go mod vendor will remove them. If the SDK feature is being developed in parallel, consider pointing go.mod at the SDK commit/branch that contains these types, or document the manual-vendor intent so a future go mod tidy && go mod vendor doesn't silently break the build.

Comment thread pkg/bsql/validate.go
Comment on lines +101 to +105
if grant == nil {
return nil
}

if grant.RejectIf != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The RejectIf.Query SQL is validated for variable references here, but the RejectIf.Reason CEL expression is not validated at startup. A syntax error in the Reason expression would only surface at runtime when a grant is actually rejected. Consider adding a CEL parse/check call here so misconfigured expressions fail early during validation.

@github-actions

github-actions Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: Add reject_if grant cancellation support

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Review mode: full
View review run

Review Summary

This PR adds a reject_if configuration option for grant provisioning that cancels a grant before mutations run when a policy query returns rows. The implementation is clean: the reject check runs inside the transaction boundary, the CEL reason expression is evaluated against the first returned row (callback correctly breaks after one row), validation covers the new query's variables, and existing ErrQueryAffectedZeroRows / GrantAlreadyExists behavior is preserved. Three focused tests cover the match, no-match, and backward-compatibility paths. No issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@alan-lee-12 alan-lee-12 marked this pull request as draft May 29, 2026 23:24

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@alan-lee-12 alan-lee-12 changed the title add support for reject_if Add reject_if grant cancellation support May 29, 2026
@alan-lee-12 alan-lee-12 force-pushed the alanlee/fde-222-baton-sql-new-reject_if-config-support branch from 38e9edf to 766c8bf Compare June 1, 2026 20:47
@alan-lee-12 alan-lee-12 marked this pull request as ready for review June 1, 2026 20:49

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@MarcusGoldschmidt MarcusGoldschmidt merged commit 2635540 into main Jun 2, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants