feat(authz): publish challenges through pubsub - #5070
Conversation
🦋 Changeset detectedLatest commit: d89e621 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 87 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/organizations/setup_test.go">
<violation number="1" location="server/internal/organizations/setup_test.go:138">
P3: After switching authz.NewEngine to a NoopPublisher, the organizations test package no longer uses ClickHouse at all, but TestMain still launches a ClickHouse container every run (setup_test.go line 82, ClickHouse: true). Consider removing the ClickHouse launch option so the suite no longer spins up unused infra.</violation>
</file>
<file name="server/internal/authz/challenge_writer.go">
<violation number="1" location="server/internal/authz/challenge_writer.go:90">
P2: Malformed challenge messages with a trace ID longer than 32 bytes or span ID longer than 16 bytes are treated as transient ClickHouse failures rather than poison records: `FixedString` rejects the insert, so the handler returns an error and Pub/Sub keeps redelivering the message. Validate these field sizes in `challengeRowFromMessage` before calling `InsertChallenge` so invalid records are logged and acknowledged.</violation>
</file>
<file name="server/internal/authz/challenge_writer_test.go">
<violation number="1" location="server/internal/authz/challenge_writer_test.go:46">
P2: The PR's core promise is idempotent persistence across redelivery, but no test exercises it. Add a case that calls writer.Handle twice with the same message ID (same challenge ID) and asserts authz_challenges still contains exactly one row for that id.</violation>
</file>
<file name="server/internal/access/list_challenge_buckets_test.go">
<violation number="1" location="server/internal/access/list_challenge_buckets_test.go:122">
P3: This test proves only that the buckets query returns one challenge for a duplicated challenge ID; it never exercises the feature this PR claims to add (idempotent ClickHouse writes on pubsub redelivery), because both rows are inserted straight into the table. If the SQL already dedups by challenge ID, the test passes even when the redelivery guard is missing, giving false confidence. Consider driving the rows through the real subscriber/publish path, or asserting on the underlying table state, so the test actually shields the write-side dedup.</violation>
</file>
<file name="server/cmd/gram/streams.go">
<violation number="1" location="server/cmd/gram/streams.go:329">
P2: This change reverses the resilience posture that the removed code deliberately established. Previously, a ClickHouse connect/ping failure at startup degraded gracefully: it logged the error, disabled only the ClickHouse risk_findings receiver, and let the rest of the streams process (ping, gitleaks, prompt-injection/policy scanners, webhook/Svix relay, etc.) keep running. The removed comment explicitly stated, "A ClickHouse connect/ping failure must NOT abort streams: taking the process down would also kill every other receiver."
Now a transient ClickHouse outage (or a misconfigured/unreachable host) causes the whole streams command to fail startup, so no subscriber starts at all. In addition, ClickHouse is now always required regardless of the `disable-clickhouse-risk-writes` kill switch: deployments that set that flag to avoid depending on ClickHouse still go through `newClickhouseClient` (which validates the CH config flags and pings the server) and will hard-fail at boot. Consider restoring the degrade-instead-of-abort behavior, or if the challenge writer genuinely requires ClickHouse, gate its registration separately so a ClickHouse failure disables the CH-bound receivers but not the rest of the process.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| Timestamp: timestamp.UTC(), | ||
| OrganizationID: message.GetOrganizationId(), | ||
| ProjectID: message.GetProjectId(), | ||
| TraceID: message.GetTraceId(), |
There was a problem hiding this comment.
P2: Malformed challenge messages with a trace ID longer than 32 bytes or span ID longer than 16 bytes are treated as transient ClickHouse failures rather than poison records: FixedString rejects the insert, so the handler returns an error and Pub/Sub keeps redelivering the message. Validate these field sizes in challengeRowFromMessage before calling InsertChallenge so invalid records are logged and acknowledged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/authz/challenge_writer.go, line 90:
<comment>Malformed challenge messages with a trace ID longer than 32 bytes or span ID longer than 16 bytes are treated as transient ClickHouse failures rather than poison records: `FixedString` rejects the insert, so the handler returns an error and Pub/Sub keeps redelivering the message. Validate these field sizes in `challengeRowFromMessage` before calling `InsertChallenge` so invalid records are logged and acknowledged.</comment>
<file context>
@@ -0,0 +1,115 @@
+ Timestamp: timestamp.UTC(),
+ OrganizationID: message.GetOrganizationId(),
+ ProjectID: message.GetProjectId(),
+ TraceID: message.GetTraceId(),
+ SpanID: message.GetSpanId(),
+ RequestID: conv.PtrEmpty(message.GetRequestId()),
</file context>
| }}, row.MatchedGrants) | ||
| } | ||
|
|
||
| func TestChallengeCHWriterPersistsMessage(t *testing.T) { |
There was a problem hiding this comment.
P2: The PR's core promise is idempotent persistence across redelivery, but no test exercises it. Add a case that calls writer.Handle twice with the same message ID (same challenge ID) and asserts authz_challenges still contains exactly one row for that id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/authz/challenge_writer_test.go, line 46:
<comment>The PR's core promise is idempotent persistence across redelivery, but no test exercises it. Add a case that calls writer.Handle twice with the same message ID (same challenge ID) and asserts authz_challenges still contains exactly one row for that id.</comment>
<file context>
@@ -0,0 +1,154 @@
+ }}, row.MatchedGrants)
+}
+
+func TestChallengeCHWriterPersistsMessage(t *testing.T) {
+ t.Parallel()
+
</file context>
| } | ||
| chConn, shutdown, err := newClickhouseClient(ctx, logger, c) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create clickhouse client: %w", err) |
There was a problem hiding this comment.
P2: This change reverses the resilience posture that the removed code deliberately established. Previously, a ClickHouse connect/ping failure at startup degraded gracefully: it logged the error, disabled only the ClickHouse risk_findings receiver, and let the rest of the streams process (ping, gitleaks, prompt-injection/policy scanners, webhook/Svix relay, etc.) keep running. The removed comment explicitly stated, "A ClickHouse connect/ping failure must NOT abort streams: taking the process down would also kill every other receiver."
Now a transient ClickHouse outage (or a misconfigured/unreachable host) causes the whole streams command to fail startup, so no subscriber starts at all. In addition, ClickHouse is now always required regardless of the disable-clickhouse-risk-writes kill switch: deployments that set that flag to avoid depending on ClickHouse still go through newClickhouseClient (which validates the CH config flags and pings the server) and will hard-fail at boot. Consider restoring the degrade-instead-of-abort behavior, or if the challenge writer genuinely requires ClickHouse, gate its registration separately so a ClickHouse failure disables the CH-bound receivers but not the rest of the process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/gram/streams.go, line 329:
<comment>This change reverses the resilience posture that the removed code deliberately established. Previously, a ClickHouse connect/ping failure at startup degraded gracefully: it logged the error, disabled only the ClickHouse risk_findings receiver, and let the rest of the streams process (ping, gitleaks, prompt-injection/policy scanners, webhook/Svix relay, etc.) keep running. The removed comment explicitly stated, "A ClickHouse connect/ping failure must NOT abort streams: taking the process down would also kill every other receiver."
Now a transient ClickHouse outage (or a misconfigured/unreachable host) causes the whole streams command to fail startup, so no subscriber starts at all. In addition, ClickHouse is now always required regardless of the `disable-clickhouse-risk-writes` kill switch: deployments that set that flag to avoid depending on ClickHouse still go through `newClickhouseClient` (which validates the CH config flags and pings the server) and will hard-fail at boot. Consider restoring the degrade-instead-of-abort behavior, or if the challenge writer genuinely requires ClickHouse, gate its registration separately so a ClickHouse failure disables the CH-bound receivers but not the rest of the process.</comment>
<file context>
@@ -322,26 +323,12 @@ func newStreamsCommand() *cli.Command {
- }
+ chConn, shutdown, err := newClickhouseClient(ctx, logger, c)
+ if err != nil {
+ return fmt.Errorf("failed to create clickhouse client: %w", err)
}
+ shutdownFuncs = append(shutdownFuncs, shutdown)
</file context>
| require.NoError(t, err) | ||
|
|
||
| authzEngine := authz.NewEngine(logger, conn, chConn, authztest.ChallengeLoggingAlwaysDisabled, thirdpartyworkos.NewStubClient()) | ||
| authzEngine := authz.NewEngine(logger, conn, gcp.NewNoopPublisher[*authzv1.Challenge](), authztest.ChallengeLoggingAlwaysDisabled, thirdpartyworkos.NewStubClient()) |
There was a problem hiding this comment.
P3: After switching authz.NewEngine to a NoopPublisher, the organizations test package no longer uses ClickHouse at all, but TestMain still launches a ClickHouse container every run (setup_test.go line 82, ClickHouse: true). Consider removing the ClickHouse launch option so the suite no longer spins up unused infra.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/organizations/setup_test.go, line 138:
<comment>After switching authz.NewEngine to a NoopPublisher, the organizations test package no longer uses ClickHouse at all, but TestMain still launches a ClickHouse container every run (setup_test.go line 82, ClickHouse: true). Consider removing the ClickHouse launch option so the suite no longer spins up unused infra.</comment>
<file context>
@@ -133,10 +135,7 @@ func newTestOrganizationsService(t *testing.T) (context.Context, *testInstance)
- require.NoError(t, err)
-
- authzEngine := authz.NewEngine(logger, conn, chConn, authztest.ChallengeLoggingAlwaysDisabled, thirdpartyworkos.NewStubClient())
+ authzEngine := authz.NewEngine(logger, conn, gcp.NewNoopPublisher[*authzv1.Challenge](), authztest.ChallengeLoggingAlwaysDisabled, thirdpartyworkos.NewStubClient())
auditLogger := audit.NewLogger()
</file context>
|
|
||
| // A subscriber can replay a message after an ambiguous acknowledgement. | ||
| // The stable event id keeps that transport replay from inflating counts. | ||
| insertCHChallenge(t, ti, authCtx.ActiveOrganizationID, challengeID, "allow", "user:u1", "org:read") |
There was a problem hiding this comment.
P3: This test proves only that the buckets query returns one challenge for a duplicated challenge ID; it never exercises the feature this PR claims to add (idempotent ClickHouse writes on pubsub redelivery), because both rows are inserted straight into the table. If the SQL already dedups by challenge ID, the test passes even when the redelivery guard is missing, giving false confidence. Consider driving the rows through the real subscriber/publish path, or asserting on the underlying table state, so the test actually shields the write-side dedup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/internal/access/list_challenge_buckets_test.go, line 122:
<comment>This test proves only that the buckets query returns one challenge for a duplicated challenge ID; it never exercises the feature this PR claims to add (idempotent ClickHouse writes on pubsub redelivery), because both rows are inserted straight into the table. If the SQL already dedups by challenge ID, the test passes even when the redelivery guard is missing, giving false confidence. Consider driving the rows through the real subscriber/publish path, or asserting on the underlying table state, so the test actually shields the write-side dedup.</comment>
<file context>
@@ -110,6 +110,41 @@ func TestListChallengeBuckets_GroupsByDimensions(t *testing.T) {
+
+ // A subscriber can replay a message after an ambiguous acknowledgement.
+ // The stable event id keeps that transport replay from inflating counts.
+ insertCHChallenge(t, ti, authCtx.ActiveOrganizationID, challengeID, "allow", "user:u1", "org:read")
+ insertCHChallenge(t, ti, authCtx.ActiveOrganizationID, challengeID, "allow", "user:u1", "org:read")
+
</file context>
1 participant
Summary
Why
Authorization challenge logging previously wrote to ClickHouse from request paths. Routing events through Pub/Sub decouples authorization handling from ClickHouse availability and gives failed writes durable retry behavior.
Impact
Authorization decisions are unchanged. Challenge logging is asynchronous, and the streams process owns ClickHouse persistence.
Validation
mise run gen:inframise exec -- go test ./server/internal/authz ./server/cmd/grammise exec -- go test -run=^$ ./server/internal/...mise lint:server