feat: report side-effect principal deletion from revoke - #141
Conversation
Connector PR Review: feat: report side-effect principal deletion from revokeBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change adds an optional Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Some apps delete the user when their last role is revoked. Add an optional provisioning.revoke.principal_deleted_check probe that runs after the revoke queries on the same transaction; when it finds the principal gone, the revoke response carries a ResourceDeleted annotation so ConductorOne marks the account deleted immediately instead of waiting for a full sync. - config: PrincipalDeletedCheck struct/field + validation - query.go: RunRevokeProvisioning + runPrincipalDeletedCheck probe - provisioning.go: attach ResourceDeleted (combined with GrantAlreadyRevoked on the already-revoked retry path) - examples/mssql-revoke-deletes-user.yml + tests Depends on the baton-sdk ResourceDeleted annotation; the go.mod/vendor bump to the released SDK is handled separately. Refs FDE-296 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Group revoke-only probe config under revoke.revoke_options so future revoke knobs have a clear home without polluting the shared provisioning query fields. Co-authored-by: Cursor <cursoragent@cursor.com>
The probe is a positive existence query — it returns the principal when it
still exists, mirroring account_provisioning.validate. The previous name
described the downstream consequence rather than the query, which forced a
double-negative read ("write an exists query, but zero rows means deleted").
Behavior is unchanged: the exists-check returning no rows still means the
principal was deleted by the revoke, yielding a ResourceDeleted annotation.
Refs FDE-296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pin to the released SDK that provides the ResourceDeleted annotation used by revoke principal-exists-check reporting. Co-authored-by: Cursor <cursoragent@cursor.com>
f45dc4a to
2e8342c
Compare
| useTx, | ||
| ) | ||
|
|
||
| anno := annotations.Annotations{} |
There was a problem hiding this comment.
I think you need to check the error on line 142 before we use principalDeleted.
| var exists bool | ||
| exists, err = s.runPrincipalExistsCheck(ctx, executor, existsCheck, vars) | ||
| if err != nil { | ||
| return false, err |
There was a problem hiding this comment.
I'm wondering if we should return a typed error here so the caller knows if the actual revoke query failed versus the optional check. Also we probably need to wait to run this until after we commit the transaction so that our existence check runs after we do the revoke.
There was a problem hiding this comment.
- run check after transaction: fixed in 24647f6
- looking into the typed error suggestion..
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if existsCheck != nil { | ||
| exists, err := s.runPrincipalExistsCheck(ctx, target, existsCheck, vars) | ||
| if err != nil { | ||
| l.Error( |
There was a problem hiding this comment.
🟡 Suggestion: This is graceful degradation — the revoke already committed and the connector continues, treating the failure as "not deleted." Per the repo log-level criteria, a gracefully-handled probe failure (often a customer-supplied query or transient DB issue) should be Warn rather than Error to avoid alert noise and retained OTEL error spans. The connector doesn't stop and the condition isn't a connector code bug, so l.Warn(...) fits better here.
There was a problem hiding this comment.
Yeah, I think set this to warn, and explicitly set exists = false if there is an error, and this is good to go.
Connector PR Review: feat: report side-effect principal deletion from revokeBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This adds an optional Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Manual e2e verification (local postgres) — Stage AVerified Setup
Test config yamlapp_name: PostgreSQL Revoke-Deletes-User Test
app_description: Demonstrates revoke_options.principal_exists_check for apps that delete a user when their last role is revoked
connect:
dsn: "postgres://${DB_HOST}:${DB_PORT}/${DB_DATABASE}?sslmode=disable"
user: "${DB_USER}"
password: "${DB_PASSWORD}"
resource_types:
user:
name: "User"
description: "A user within the PostgreSQL system"
list:
query: |
SELECT
u.id,
u.username,
u.email,
u.status
FROM users u
ORDER BY u.id
pagination:
strategy: "offset"
primary_key: "id"
map:
id: ".username"
display_name: ".username"
description: ".username"
traits:
user:
emails:
- ".email"
status: ".status"
login: ".username"
# Account provisioning so that a re-grant immediately after deletion
# recreates the user before the new role is assigned.
account_provisioning:
schema:
- name: "username"
description: "The username for the new user"
type: "string"
placeholder: "new_user"
required: true
- name: "email"
description: "Email address for the new user"
type: "string"
placeholder: "user@example.com"
required: true
credentials:
no_password:
preferred: true
validate:
vars:
username: "username"
query: |
SELECT u.id, u.username, u.email
FROM users u
WHERE u.username = ?<username>
create:
vars:
username: "input.username"
email: "input.email"
queries:
- |
INSERT INTO users (username, email, status, created_at)
VALUES (?<username>, ?<email>, 'active', NOW())
ON CONFLICT (username) DO NOTHING
role:
name: "Role"
description: "A role within the PostgreSQL system"
list:
query: |
SELECT
id,
role_name,
description
FROM roles
ORDER BY id
pagination:
strategy: "offset"
primary_key: "id"
map:
id: ".role_name"
display_name: ".role_name"
description: ".description"
traits:
role:
profile:
role_name: ".role_name"
static_entitlements:
- id: "member"
display_name: "resource.DisplayName + ' Membership'"
description: "'Member of the ' + resource.DisplayName + ' role'"
purpose: "assignment"
grantable_to:
- "user"
provisioning:
vars:
username: "principal.ID"
role_name: "resource.ID"
grant:
queries:
- |
INSERT INTO user_roles (user_id, role_id)
SELECT u.id, r.id
FROM users u, roles r
WHERE u.username = ?<username>
AND r.role_name = ?<role_name>
ON CONFLICT (user_id, role_id) DO NOTHING
revoke:
# Revoke removes the membership row, then deletes the user if this
# was their last role. The queries run in a single transaction.
queries:
- |
DELETE FROM user_roles ur
USING users u, roles r
WHERE ur.user_id = u.id
AND ur.role_id = r.id
AND u.username = ?<username>
AND r.role_name = ?<role_name>
- |
DELETE FROM users
WHERE username = ?<username>
AND NOT EXISTS (
SELECT 1 FROM user_roles ur
JOIN users u2 ON ur.user_id = u2.id
WHERE u2.username = ?<username>
)
# After the revoke queries run, probe whether the principal still
# exists. No rows means the app deleted the user, so the connector
# reports a ResourceDeleted annotation on the revoke response.
revoke_options:
principal_exists_check:
query: |
SELECT 1 FROM users WHERE username = ?<username>
grants:
- query: |
SELECT
u.username,
r.role_name
FROM user_roles ur
JOIN users u ON ur.user_id = u.id
JOIN roles r ON ur.role_id = r.id
ORDER BY r.role_name, u.username
map:
- skip_if: ".role_name != resource.ID"
principal_id: ".username"
principal_type: "user"
entitlement_id: "member"
pagination:
strategy: "offset"
primary_key: "username"Test fixtureCREATE TABLE users (
id SERIAL PRIMARY KEY,
username TEXT NOT NULL UNIQUE,
email TEXT NOT NULL,
status TEXT NOT NULL DEFAULT 'active',
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
);
CREATE TABLE roles (
id SERIAL PRIMARY KEY,
role_name TEXT NOT NULL UNIQUE,
description TEXT
);
CREATE TABLE user_roles (
user_id INT NOT NULL REFERENCES users (id) ON DELETE CASCADE,
role_id INT NOT NULL REFERENCES roles (id),
PRIMARY KEY (user_id, role_id)
);
INSERT INTO roles (role_name, description) VALUES
('analyst', 'Read-only analyst role'),
('admin', 'Administrator role'),
('auditor', 'Compliance auditor role');
INSERT INTO users (username, email) VALUES
('alice', 'alice@example.com'),
('bob', 'bob@example.com'),
('carol', 'carol@example.com');
INSERT INTO user_roles (user_id, role_id)
SELECT u.id, r.id FROM users u, roles r
WHERE (u.username, r.role_name) IN (
('alice', 'analyst'),
('alice', 'admin'),
('bob', 'analyst'),
('carol', 'admin')
);Results
sample logs when last role was deleted{"level":"debug","ts":1785274092.702752,"caller":"bsql/provisioning.go:109","msg":"revoking entitlement","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","grant_id":"role:analyst:member:user:bob"}
{"level":"info","ts":1785274092.702773,"caller":"bsql/provisioning.go:31","msg":"provisioning is enabled for entitlement","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","entitlement_id":"member"}
{"level":"debug","ts":1785274092.7158709,"caller":"bsql/query.go:674","msg":"query executed","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","use_tx":true,"peer.address":"127.0.0.1:53502","query":"DELETE FROM user_roles ur\nUSING users u, roles r\nWHERE ur.user_id = u.id\nAND ur.role_id = r.id\nAND u.username = $1\nAND r.role_name = $2\n","args":["bob","analyst"],"rows_affected":1}
{"level":"debug","ts":1785274092.71793,"caller":"bsql/query.go:674","msg":"query executed","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","use_tx":true,"peer.address":"127.0.0.1:53502","query":"DELETE FROM users\nWHERE username = $1\nAND NOT EXISTS (\n SELECT 1 FROM user_roles ur\n JOIN users u2 ON ur.user_id = u2.id\n WHERE u2.username = $2\n)\n","args":["bob","bob"],"rows_affected":1}
{"level":"debug","ts":1785274092.7189739,"caller":"bsql/query.go:579","msg":"running principal exists check query","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","use_tx":true,"peer.address":"127.0.0.1:53502","query":"SELECT 1 FROM users WHERE username = $1\n","args":["bob"]}
{"level":"info","ts":1785274092.720174,"caller":"bsql/provisioning.go:160","msg":"revoke deleted principal; reporting ResourceDeleted","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","grant_id":"role:analyst:member:user:bob","principal_id":"bob"}
{"level":"debug","ts":1785274092.720187,"caller":"bsql/provisioning.go:172","msg":"revoked grant","grpc.start_time":"2026-07-28T14:28:12-07:00","grpc.service":"c1.connector.v2.GrantManagerService","grpc.method":"Revoke","peer.address":"127.0.0.1:53502","grant_id":"role:analyst:member:user:bob"}Unit coverage for the remaining edges (no Next: Stage B — service mode against a dev tenant with the c1-side consumer + feature flag, exercising revoke-from-C1, immediate re-provision, and FF-off control. |
3 participants
What
Some downstream apps delete the user record when their last role is revoked. Today C1 doesn't learn the account is gone until the next full sync (~1h), so an immediate role change (revoke old → request new) fails downstream because the account no longer exists.
This adds an optional
provisioning.revoke.revoke_options.principal_exists_checkprobe. After the revoke queries run (same transaction), the probe checks whether the principal still exists — a positive existence query, the same shape asaccount_provisioning.validate. If it returns no rows, the principal was deleted as a side effect, and the connector attaches aResourceDeletedannotation to the revoke response so C1 can mark the account deleted immediately.Changes
pkg/bsql/config.go,pkg/bsql/validate.go): a revoke-onlyRevokeEntitlementProvisioningQueriestype with arevoke_optionsblock carryingprincipal_exists_check { query }, plus validation (non-empty query + var check)pkg/bsql/query.go:RunRevokeProvisioning+runPrincipalExistsCheck— runs the probe on the same executor/tx after the revoke queries; no rows ⇒ principal deleted. Tolerates the all-zero-rows (already-revoked) case so retried revokes still report the deletion.pkg/bsql/provisioning.go:Revoke()attachesResourceDeleted{principal.Id}, combined withGrantAlreadyRevokedon the already-revoked retry path.examples/mssql-revoke-deletes-user.yml: worked example (delete-user-on-last-role revoke + probe + re-provisioning)pkg/bsql/provisioning_revoke_deleted_test.go+ config round-trip inconfig_test.goDesign note
The probe is a positive existence query (rows present = principal still exists), mirroring
account_provisioning.validate; "no rows ⇒ deleted" is the single interpretation. This keeps the SQL natural (noNOT EXISTSgymnastics) and consistent with the existing validate pattern.Dependency / merge order
Depends on the
ResourceDeletedannotation in ConductorOne/baton-sdk#1010. This branch is intentionally source-only —go.mod/go.sum/vendor/are not included, so it won't build standalone until the SDK PR is released and the dependency is bumped here (dropping the localreplacedirective used during development).Test plan (built against the local SDK via
replace)go build ./... && go test ./... -count=1— passgofmtclean;golangci-lint run ./pkg/bsql/...— clean aside from the dev-onlyreplacedirectiveRefs FDE-296
🤖 Generated with Claude Code