feat(api): close the gRPC/REST gap with GraphQL - #739
Conversation
GraphQL exposed 115 operations; gRPC/REST exposed 76. Every RPC already carried a google.api.http annotation, so gRPC and REST never diverged from each other — but 39 GraphQL ops had no RPC at all, and 26 of those were the multi-tenant control plane. A service provisioning tenants from a typed SDK could not create an organization, verify a domain, configure per-org SSO, or issue a SCIM token; it had to fall back to GraphQL. WebAuthn was the same story for native clients: passkeys were unreachable outside a browser talking GraphQL. Adds the 33 live ops (76 -> 109 RPCs). The 6 remaining are deprecated (_admin_signup, _env, _update_env, _generate_jwt_keys, mobile_login, mobile_signup) and are deliberately not ported. organizations + members 9 admin verified org domains 5 admin org OIDC connections 4 admin org SAML connections 4 admin SCIM endpoints 4 admin WebAuthn / passkeys 6 public TOTP setup 1 public Handlers are thin delegations; no authorization logic is reimplemented, so the service layer's load-then-check discipline (org id sourced from the loaded resource, never from caller input) carries over unchanged. RotateScimToken reuses CreateScimEndpointResponse, mirroring how RotateClientSecret reuses CreateClientResponse — the only messages that carry a secret. Two guards, because neither gap was catchable by the build: - TestEveryRPCHasAHandler. Both handler structs embed the generated Unimplemented server, so a forgotten handler compiles and fails only at runtime. Detection is behavioural, not name-based: Go synthesises an identically-named wrapper for promoted methods, so a name check passes vacuously. - Cross-surface conformance. Matching inventory is not matching behaviour. One logical operation is run over GraphQL, gRPC and REST against a single shared backend, asserting read parity, cross-surface write visibility, list parity, failure parity, and that an empty collection serializes as [] rather than null. Both were verified by injecting the fault they exist to catch.
gen/ts is vendored into authorizer-js and has its own staleness gate (proto-check-clients), separate from proto-gen/proto-check which cover only the Go and OpenAPI output. Adding RPCs without regenerating it left the TypeScript SDK missing all 33 new methods.
Principal review — PR #739Reviewed adversarially, verifying claims by running them rather than reading. CI is green (5/5) and the REST surface is complete, but there is one behavioural gap worth a decision before merge. 🔴 Finding 1 — org-admins cannot use the new admin surface at allThe service layer authorizes 28 admin operations via Verified with one org-admin, one operation ( Scope. Pre-existing for the 8 SAML-service-provider ops already in proto. This PR takes it from 8 to 27 affected operations — Severity: functional, not a security hole. It fails closed — too restrictive, never too permissive. Nothing is exposed that shouldn't be. Why it matters anyway: the stated purpose of this PR is letting SDK clients manage tenants, and org-admins are precisely the persona multi-tenant SSO exists for. As merged, the 26 new admin RPCs are usable only with the platform-wide admin secret — which is exactly the credential you do not hand to a tenant. Not fixable safely inside this PR. The fix is to stop blanket-requiring super-admin for the admin service and let the service layer decide per-op — a change to the auth interceptor, the single most security-sensitive file in the request path. It deserves its own PR, its own review, and its own tests. Merging this as-is is defensible provided the limitation is documented and tracked. The conformance suite did not catch this because it only exercises super-admin. Extending it to org-admin is the natural follow-up and would have caught it. ✅ Verified clean
🟡 Minor
VerdictApprove for merge on the strength of CI, the complete REST surface, and the field/partial-update/secret checks — conditional on Finding 1 being tracked as a known limitation, ideally noted in the PR body or an issue before merge. It is fail-closed, so it degrades capability rather than safety. I authored this PR, so this is a self-review and is not a substitute for a second reader — particularly on the proto surface, which is a public API contract that is expensive to change once released. |
Both surface on the admin API to any authenticated caller once the gRPC interceptor stops requiring super-admin for the whole admin service. The SAML SP and IdP-key lookups must precede authorization — the loaded resource's OrgID is what requireOrgAdmin checks against (design H2) — so an unmasked "not found" is a cross-tenant existence oracle, with the id echoed back. Mask for everyone but a super-admin, as the org OIDC, SAML and domain paths already do. normalizeDomain/guardVerifiableDomain returned untyped errors, which map to Internal: a request naming a malformed domain answered 500 instead of 400 on all four org-domain endpoints. Type them InvalidArgument at the declaration so every caller is fixed at once; they stay singletons, so the errors.Is comparisons are unaffected.
The admin surface accepts two identities: a platform super-admin for everything, and an org's own org-admin (authorizer:org_admin) for that org's scoped operations. The auth interceptor gated the entire AuthorizerAdminService on super-admin, so org-scoped ops worked over GraphQL and returned Unauthenticated over gRPC and REST — for exactly the persona multi-tenant SSO exists to serve. Resolve the caller, attach the principal, and let the service layer's requireSuperAdmin/requireOrgAdmin decide, as the public service already does. Anonymous callers are still refused at the interceptor. That delegation is only safe while every admin method carries its own gate, which the interceptor previously made unnecessary. Three tests pin it: - service.TestAdminMethodsAreGated parses every AdminProvider method and fails on any without a top-level gate (78 methods, AdminLogin exempt as the auth bootstrap). - integration_tests.TestAdminSurfaceDeniesPlainUser sweeps all 77 non-bootstrap admin RPCs over gRPC and REST with an ordinary user and requires each to refuse. Requests are filled and pre-checked against the same protovalidate rules the server enforces, because validation sits between auth and the handler — an invalid payload would be rejected before the gate and prove nothing. - e2e.TestReleaseSmokeOrgAdmin runs the scenario against the real binary across all three surfaces, including the negative cases: an org-admin is still refused another org's data and the platform-wide operations. TestEveryRPCHasARoutableRESTBinding additionally pins that all 109 RPCs (31 public, 78 admin) declare an HTTP rule that resolves in the gateway, complementing handlers.TestEveryRPCHasAHandler. Bearer calls in the in-process conformance harness now pin the issuing host across surfaces. Each ran on its own listener and derived its own issuer, so every bearer call failed on JWT issuer validation — which would have made the negative assertions pass vacuously.
|
Review findings addressed in bd7818c + 1a0c3be. The gapThe admin surface accepts two identities — platform super-admin, and an org's own org-admin ( The interceptor now resolves the caller, attaches the principal, and lets What that delegation costsThe interceptor was previously a blanket backstop: an admin method that forgot its own gate was still unreachable. It no longer is. Three tests carry that weight now:
Each was mutation-tested rather than assumed:
The negative cases matter as much as the positive one: simply deleting the interceptor check would satisfy the positive case alone. Two defects the sweep surfacedBoth pre-existing, both reachable over gRPC/REST only because of this change — so they are fixed here rather than deferred.
REST coverage for the new RPCs
One correctionI initially read the dropped Verification
Not run: |
A client_credentials token is authenticated, so the interceptor now admits it where it previously could not reach an admin handler at all. requireOrgAdmin rejects service accounts explicitly and the membership lookup rejects them implicitly, but nothing covered either path. The token is minted for real and asserted to resolve as a service account before the denials are checked — a malformed credential would be refused everywhere and prove nothing about authorization.
1 participant
Why
GraphQL exposed 115 operations; gRPC/REST exposed 76. Every RPC already
carried a
google.api.httpannotation, so gRPC and REST never diverged fromeach other — but 39 GraphQL ops had no RPC at all, and the reverse gap was
zero. gRPC/REST was a strict subset.
26 of those 39 were the multi-tenant control plane. A service provisioning
tenants from a generated SDK could not create an organization, verify a domain,
configure per-org SSO, or issue a SCIM token — it had to drop to GraphQL.
WebAuthn was the same story for native/mobile clients: passkeys were unreachable
outside a browser speaking GraphQL.
What this adds
33 live operations, 76 → 109 RPCs. Each carries an http annotation, so 26
new
/v1/admin/*and 7 new/v1/*REST paths land through grpc-gateway.The remaining 6 are deprecated (
_admin_signup,_env,_update_env,_generate_jwt_keys,mobile_login,mobile_signup) and are deliberately notported.
Conventions followed
Checked against the existing surface rather than assumed:
post: "/v1/admin/{snake_case}"withbody: "*";buf.validatemin_lenon requiredstrings and
max_lenon email/phone matching the existing OTP messages; proto3optionalfor every nullable GraphQL field so "unset" survives instead ofcollapsing to a zero value; one
…Request/…Responsepair per RPC; projectorsin the established
project*style.Handlers are thin delegations — no authorization logic is reimplemented, so the
service layer's load-then-check discipline (org id taken from the loaded
resource, never from caller input) carries over untouched.
RotateScimTokenreuses
CreateScimEndpointResponse, mirroring howRotateClientSecretreusesCreateClientResponse— the only two messages that carry a secret.Auth annotations follow the existing rule: WebAuthn options/verify are
public(a caller may be mid-login with no token);
WebauthnCredentialsandWebauthnDeleteCredentialact on the caller's own credentials and require auth.Mutating RPCs carry
audit_log.Two guards, because the build could not catch either gap
TestEveryRPCHasAHandler. Both handler structs embed the generatedUnimplemented…ServiceServer, which is what lets the package compile while RPCsare added incrementally — and which means a forgotten handler compiles fine
and fails only at runtime with
codes.Unimplemented. Adding 33 methods with nocompile-time check was not acceptable.
Detection is behavioural, not name-based. My first attempt compared
runtime.FuncForPCnames and was worthless: Go synthesises anidentically-named wrapper on the outer type for promoted methods, so a missing
handler reports the same name as a real one and the test passed vacuously. The
working version invokes each method with a nil Service — the fallback returns
Unimplementedwithout touching it; a real handler panics dereferencing it.Cross-surface conformance (
internal/integration_tests/surface_conformance_test.go).Matching inventory is not matching behaviour, and the three surfaces are wired
differently enough to drift:
newSurfaces(t)binds all three to a singletestSetup, then asserts readparity, cross-surface write visibility, list parity, failure parity, and that an
empty collection serializes as
[]rather thannull.It lives in
internal/integration_tests, not the smoke suite, so it runs onevery CI run rather than only at release — the harness needed (bufconn gRPC,
in-process gateway) already existed there.
Both guards were verified by injecting the fault
Dropping
display_namefrom the gRPC projector:Caught on both, correctly — REST rides the same handler.
Deleting a handler: build still passed, and the guard failed naming the exact
RPC.
A flaw I found in my own test while doing this: the empty-collection check
was originally
assert.Empty, which is satisfied by[], bynull, and byan absent key — it would have passed straight through the drift it was named
for. Fixed with
assert.NotNilfirst (only a literal[]decodes toempty-but-non-nil) and re-verified by simulating a
nullresponse.Verification
buf lint✅ ·buf generate✅ ·make proto-check✅ ·go build/vet✅ ·make lint-go0 issues ✅ ·go test ./...40 packages, exit 0 ✅ ·make smoke✅Plus a live REST exercise against a running binary: created an organization,
listed organizations/members/domains, generated a real DNS challenge
(
_authorizer-challenge.acme.testwith a TXT token), created a SCIM endpointreturning its one-time token, and updated the organization.
Notes for review
make proto-breakingfails locally withcannot find buf.validate.field in this scope. I checked whether this PR caused it by stashing everything andre-running on a clean tree: it fails identically. Pre-existing local
dep-resolution issue; CI's "Lint & breaking" job is the authority.
depth-over-breadth call — the harness and the five properties are the reusable
part, and extending to the other blocks is now table-shaped work. Five
properties that demonstrably catch drift beat 109 that do not.
GinContextrather than a per-call credential, so "send no credential" is notexpressible there the way it is for gRPC and REST.