Repository navigation
Conversation
Read-only tokens are denied on gRPC and authorized as viewers on the REST API, so a credential can inspect a tenant without connecting a worker.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
[High impact] The PR changes how API tokens are authorized across tenant REST and gRPC requests. Fix the migration order and outstanding repository requirements before merging. SummaryAdds optional read-only API tokens while keeping existing tokens unrestricted.
Reviews (2) · Last reviewed commit: "merge" · Reviewed by Greptile |
|
|
||
| func (r bearerRepo) APIToken() repository.APITokenRepository { return r.tokens } | ||
|
|
||
| func TestBearerTokenPermissions(t *testing.T) { |
There was a problem hiding this comment.
The new exported test functions have no godoc comments. The repository requires comments on exported Go functions, so this must be fixed before merging. Add short comments explaining what each test protects. The same pattern appears in TestReadOnlyTokenAuthentication, TestGenerateTenantTokenReadOnly, and the new persistence test.
Context Used: Comment and logging conventions for the Hatchet codebase (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ctx, err := NewAuthN(config).Middleware(context.Background(), http.Header{"Authorization": []string{"Bearer " + testToken}}) | ||
| if readOnly { | ||
| require.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) | ||
| require.Nil(t, ctx) |
There was a problem hiding this comment.
Client rejection lacks coverage
TestReadOnlyTokenAuthentication calls Middleware directly. It does not check that clients receive PermissionDenied through the actual request gate. Add read-only cases to the transport tests for a unary call and a stream, and assert that neither reaches the handler. This would catch future changes that break the promised client-facing error.
|
|
||
| ## PermissionDenied with a read-only token | ||
|
|
||
| Read-only API tokens can use viewer-authorized REST endpoints, but every gRPC call returns `PermissionDenied`, including worker registration and task completion. |
There was a problem hiding this comment.
Not sure about this, but don't we use gRPC for some of the read operations on the SDKs?
There was a problem hiding this comment.
on the sdk yes, but not on the MCP which is the current ask... and frankly a good idea to probably restrict bc if the agent does weird stuff it wont blow up core workloads...
i was thinking about calling these agent read only tokens potentially to make this more clear
There was a problem hiding this comment.
but not on the MCP which is the current ask
also im assuming this is true.... perhaps we should verify
mnafees
left a comment
There was a problem hiding this comment.
LGTM on first glance, we might want to run a Claude security analysis on the code.
|
|
| -- +goose Up | ||
| ALTER TABLE "APIToken" ADD COLUMN "readOnly" BOOLEAN NOT NULL DEFAULT false; |
There was a problem hiding this comment.
The new migration uses version 20261007120000, but the target branch already contains 20261028120000_v1_0_183.sql. This violates the repository requirement that new migrations have a version higher than the latest migration on main. Existing installations may fail to upgrade because they have already applied later migrations. Rename this migration to a version above 20261028120000.
Rule Used: What: Ensure every new SQL migration in cmd/hatchet-migrate/migrations/* has a version/date higher than the latest migration present on main. Why: Prevents out-of-order migrations that can fail or corrupt database schema state during deployment.... (source)
| Before diving into specific issues, run through these checks: | ||
|
|
||
| 1. **Verify your API token** — make sure `HATCHET_CLIENT_TOKEN` matches the token generated in the Hatchet dashboard for your tenant. | ||
| 1. **Verify your API token** — make sure `HATCHET_CLIENT_TOKEN` matches an unrestricted token generated in the Hatchet dashboard for your tenant. Read-only tokens cannot connect workers. |
There was a problem hiding this comment.
Checklist breaks the punctuation rule
The rewritten checklist item still uses an em dash. The documentation guide forbids em dashes, so this requirement must be satisfied before merging. Replace it with a colon or a separate sentence.
| 1. **Verify your API token** — make sure `HATCHET_CLIENT_TOKEN` matches an unrestricted token generated in the Hatchet dashboard for your tenant. Read-only tokens cannot connect workers. | |
| 1. **Verify your API token**: make sure `HATCHET_CLIENT_TOKEN` matches an unrestricted token generated in the Hatchet dashboard for your tenant. Read-only tokens cannot connect workers. |
Context Used: Writing style conventions for Hatchet documentation (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
API tokens can be created as read-only. Those tokens are rejected on every gRPC call and authorized as VIEWER for tenant REST operations. Existing tokens stay unrestricted.
Type of change
What's Changed
readOnlyflag on API tokens, defaulting to falsePermissionDeniedChecklist
Changes have been:
🤖 AI Disclosure
I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchet's AI_POLICY.md.
Details: Cursor (Grok) drafted the pull request description. The implementation was already in the working tree.