Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces a reusable authorization test helper framework ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Helper as authz.Test401/Test403
participant Harness as testutil.Harness
participant Route as Handler Route
participant KeyManager as Key Manager
Test->>Helper: Call Test401/Test403 with config
Helper->>Harness: Create test harness
Helper->>Route: Register route via SetupHandler
Route->>KeyManager: Initialize with DB, Keys, Logger
rect rgb(200, 220, 255)
Note over Helper: For each permission scenario (401 or 403)
Helper->>KeyManager: Create root key with specific permissions
Helper->>Route: Build Authorization header
Helper->>Route: Issue HTTP request
Route->>Route: Check authorization
Helper->>Helper: Assert Status and Body match expected
end
Helper-->>Test: Test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
|
||
| require.Equal(t, tc.expectedStatus, res.Status, | ||
| "expected %d, got %d, body: %s", tc.expectedStatus, res.Status, res.RawBody) | ||
| require.NotNil(t, res.Body) |
There was a problem hiding this comment.
can we also assert some of the body content? ie error.status, error.title and error.type?
| // | ||
| // func TestCreateApiForbidden(t *testing.T) { | ||
| // authz.Test403(t, | ||
| // authz.PermissionTestConfig[handler.Request, handler.Response]{ |
There was a problem hiding this comment.
the setup is inconsistent between Test401 and Test 403.
401 accepts multiple arguments and 403 accepts a struct
I don't care that much, but can we make it the same?
| // For each required permission, generate related test cases | ||
| for _, perm := range requiredPermissions { | ||
| parts := strings.Split(perm, ".") | ||
| if len(parts) >= 3 { |
There was a problem hiding this comment.
this should just fail if len(parts) is not 3. Cause that very likely means we have an issue somewhere, right now we're silently ignoring it
| // }, | ||
| // ) | ||
| // } | ||
| func Test401[TReq any, TRes any]( |
There was a problem hiding this comment.
"technically" this is authN and doesn't fit in a package called authz
it's a nit but I'd just rename the package to auth
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (33)
go/apps/api/routes/v2_permissions_get_role/403_test.go (3)
13-13: Enable test parallelization.Add t.Parallel() for faster suites, if the Harness is concurrency-safe.
func TestAuthorizationErrors(t *testing.T) { + t.Parallel() authz.Test403(t,Please confirm the Harness and seeded resources are safe under parallel execution.
31-33: Replace "role_id" magic string with a file‑scoped const.Reduces typo risk and improves readability.
@@ - return authz.TestResources{ + return authz.TestResources{ WorkspaceID: h.Resources().UserWorkspace.ID, Custom: map[string]string{ - "role_id": roleID, + roleIDKey: roleID, }, } @@ - CreateRequest: func(res authz.TestResources) handler.Request { + CreateRequest: func(res authz.TestResources) handler.Request { return handler.Request{ - Role: res.Custom["role_id"], + Role: res.Custom[roleIDKey], } },Add near the imports:
const roleIDKey = "role_id"Also applies to: 38-39
23-23: Permission string is correct and matches handler guard; string literal in test setup is acceptable but could use shared constant for consistency.The handler correctly verifies
rbac.*.read_roleviarbac.T(rbac.Tuple{ResourceType: rbac.Rbac, ResourceID: "*", Action: rbac.ReadRole})(lines 56–62 of handler.go). The test permission string aligns perfectly with this requirement.While constants
RbacandReadRoleexist ingo/pkg/rbac/permissions.go, the test setup uses a raw string literal. For test consistency with the handler's constant-based approach, consider creating a test helper or using the same Tuple pattern, though this is a minor refactoring—string literals for test setup are idiomatic.go/apps/api/routes/v2_permissions_delete_permission/401_test.go (1)
12-28: Consider standardizing test names across all 401 test files.The test naming is inconsistent across the refactored files: some use
TestAuthenticationErrorswhile others (e.g.,v2_ratelimit_list_overrides/401_test.go,v2_ratelimit_delete_override/401_test.go) useTestUnauthorizedAccess. Standardizing to one name would improve consistency.go/apps/api/routes/v2_ratelimit_limit/401_test.go (1)
12-12: Enable parallel execution if harness supports it.Add
t.Parallel()at the top of the test to speed up CI.func TestUnauthorizedAccess(t *testing.T) { + t.Parallel() authz.Test401[handler.Request, handler.Response](t,Please confirm
testutil.Harnessand shared fixtures are safe for parallel tests.go/apps/api/routes/v2_permissions_delete_permission/403_test.go (2)
13-13: Run test in parallel if possible.Add
t.Parallel()to improve throughput (assuming harness is concurrency-safe).func TestAuthorizationErrors(t *testing.T) { + t.Parallel() authz.Test403(t,If the harness uses shared DB state, confirm isolation before enabling.
24-24: Create a constant for the full permission string.The raw string
"rbac.*.delete_permission"should be replaced with a shared constant to prevent drift and typos. No centralized constant currently exists for the full permission strings (onlyDeletePermission ActionType = "delete_permission"for the action part).Consider defining full permission constants in
go/pkg/rbac/permissions.goalongside the ActionType constants, e.g.:const DeletePermissionFull = "rbac.*.delete_permission"Then use it here and in other test files that reference the same permission string.
go/apps/api/routes/v2_identities_list_identities/401_test.go (1)
12-12: Enable parallelism.Add
t.Parallel()for faster test runs if fixtures are parallel-safe.func TestAuthenticationErrors(t *testing.T) { + t.Parallel() authz.Test401[handler.Request, handler.Response](t,go/apps/api/routes/v2_identities_get_identity/403_test.go (2)
13-13: Parallelize the test if safe.Add
t.Parallel()to speed up CI (validate harness isolation first).func TestAuthorizationErrors(t *testing.T) { + t.Parallel() authz.Test403(t,
23-24: Use centralized permission constants to prevent string drift.The hard-coded string
"identity.*.read_identity"appears in multiple test files without centralization. Create a constant combining theIdentityresource type andReadIdentityaction (e.g.,IdentityReadIdentity = "identity.*.read_identity") ingo/pkg/rbac/permissions.go, then reference it in place of the string literal.go/apps/api/routes/v2_permissions_get_role/401_test.go (1)
12-12: Enable test parallelism.Add
t.Parallel()if the harness is concurrency-safe.func TestAuthenticationErrors(t *testing.T) { + t.Parallel() authz.Test401[handler.Request, handler.Response](t,go/apps/api/routes/v2_keys_set_roles/401_test.go (1)
14-30: Nice 401 refactor; KeyCache wiring and payload look right. Add t.Parallel.
- Dependencies sufficient; request meets validation.
- Minor: run in parallel.
func TestAuthenticationErrors(t *testing.T) { - authz.Test401[handler.Request, handler.Response](t, + t.Parallel() + authz.Test401[handler.Request, handler.Response](t,go/apps/api/routes/v2_permissions_list_roles/403_test.go (1)
13-27: Permission identifier confirmed as canonical; consider t.Parallel() for test optimization.The "rbac.*.read_role" permission matches the canonical form used throughout the codebase. The 403 test structure is correct and requires no resource setup. Consider adding
t.Parallel()to parallelize test execution if not already parallelized in a parent suite:func TestAuthorizationErrors(t *testing.T) { + t.Parallel() authz.Test403(t,go/apps/api/routes/v2_permissions_create_role/403_test.go (1)
13-30: Add t.Parallel() for consistency with test patterns in the same directory.Config is clean and permission usage is correct. Sibling tests in
go/apps/api/routes/v2_keys_*consistently uset.Parallel()as the first statement in test functions to enable parallel execution.func TestAuthorizationErrors(t *testing.T) { + t.Parallel() authz.Test403(t, authz.PermissionTestConfig[handler.Request, handler.Response]{ SetupHandler: func(h *testutil.Harness) zen.Route {go/apps/api/routes/v2_keys_reroll_key/401_test.go (1)
13-29: Test401 helper properly validates response contracts; migration approved. Consider adding t.Parallel() for test optimization.The helper validates both status codes (401 and 400 cases) and response body structure via typed parsing to
openapi.UnauthorizedErrorResponse. The 7 test scenarios comprehensively cover authentication failures with no regressions from prior behavior.Optional: Add
t.Parallel()at line 14 to parallelize test execution.go/apps/api/routes/v2_keys_set_permissions/401_test.go (1)
14-30: Good migration to authz.Test401; wiring looks correct.Route factory and minimal request are appropriate for 401-path coverage; dependencies are sensibly provided.
If you find yourself repeating the same handler wiring across multiple key-related tests, consider a tiny local helper to reduce duplication.
go/apps/api/routes/v2_keys_remove_permissions/401_test.go (1)
14-30: Consistent and clean 401 helper usage.Wiring and minimal request are appropriate; no issues spotted.
Consider a small shared builder for the key handlers across 401 tests to DRY up repeated wiring.
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
12-26: Minor: enable parallelism and rely on type inference.
- Add t.Parallel() for faster suites (if Harness is parallel-safe).
- Type args can be inferred; drop explicit [handler.Request, handler.Response].
Please confirm Harness isolation is safe for t.Parallel and that Test401 covers both “missing Authorization header” and “malformed/unsupported scheme” cases.
func TestAuthenticationErrors(t *testing.T) { - authz.Test401[handler.Request, handler.Response](t, + t.Parallel() + authz.Test401(t, func(h *testutil.Harness) zen.Route { return &handler.Handler{ DB: h.DB, Keys: h.Keys, Logger: h.Logger, } }, func() handler.Request { return handler.Request{ Identity: "test_identity", } }, ) }go/apps/api/routes/v2_ratelimit_set_override/403_test.go (2)
29-59: Reduce key typos: constantize Custom keys.Avoid stringly-typed map keys across SetupResources/CreateRequest.
func TestWorkspacePermissions(t *testing.T) { - authz.Test403(t, + authz.Test403(t, authz.PermissionTestConfig[handler.Request, handler.Response]{ SetupHandler: func(h *testutil.Harness) zen.Route { return &handler.Handler{ DB: h.DB, Keys: h.Keys, Logger: h.Logger, Auditlogs: h.Auditlogs, RatelimitNamespaceCache: h.Caches.RatelimitNamespace, } }, RequiredPermissions: []string{"ratelimit.*.set_override"}, SetupResources: func(h *testutil.Harness) authz.TestResources { + const keyNamespaceID = "namespace_id" ctx := context.Background() // Create a namespace namespaceID := uid.New(uid.RatelimitNamespacePrefix) err := db.Query.InsertRatelimitNamespace(ctx, h.DB.RW(), db.InsertRatelimitNamespaceParams{ ID: namespaceID, WorkspaceID: h.Resources().UserWorkspace.ID, Name: uid.New("name"), CreatedAt: time.Now().UnixMilli(), }) if err != nil { t.Fatalf("failed to create namespace: %v", err) } return authz.TestResources{ WorkspaceID: h.Resources().UserWorkspace.ID, Custom: map[string]string{ - "namespace_id": namespaceID, + keyNamespaceID: namespaceID, }, } }, CreateRequest: func(res authz.TestResources) handler.Request { + const keyNamespaceID = "namespace_id" return handler.Request{ - Namespace: res.Custom["namespace_id"], + Namespace: res.Custom[keyNamespaceID], Identifier: "test_identifier", Limit: 10, Duration: 1000, } }, }, ) }
16-28: Add t.Parallel() to align with codebase test conventions.The permission slug "ratelimit.*.set_override" is confirmed across OpenAPI specs, tests, and implementation files. However, this test lacks
t.Parallel()despite widespread adoption of this pattern in other tests within the same directory (e.g., v2_keys_reroll_key, v2_keys_update_key, v2_keys_create_key), suggesting the testutil.Harness infrastructure supports parallel execution. Addt.Parallel()at the start ofTestWorkspacePermissionsto align with project conventions.go/apps/api/routes/v2_ratelimit_limit/403_test.go (2)
3-10: Align test package naming with the rest for consistency.Other tests use package handler_test; consider renaming v2RatelimitLimit_test to handler_test to avoid mixed package names in the same module.
-package v2RatelimitLimit_test +package handler_test
26-47: Split scenarios: existing vs. non‑existing namespace.Comment notes create_namespace is needed only when namespace doesn’t exist. Consider a second test that seeds the namespace and sets RequiredPermissions to only "ratelimit.*.limit" to verify both paths.
I can draft a variant that seeds the namespace and asserts 403 with missing "ratelimit.*.limit" only.
go/apps/api/routes/v2_ratelimit_get_override/403_test.go (1)
28-71: Constantize Custom keys and enable parallelism.Avoid key typos and speed up tests.
func TestWorkspacePermissions(t *testing.T) { - authz.Test403(t, + t.Parallel() + authz.Test403(t, authz.PermissionTestConfig[handler.Request, handler.Response]{ SetupHandler: func(h *testutil.Harness) zen.Route { return &handler.Handler{ DB: h.DB, Keys: h.Keys, Logger: h.Logger, RatelimitNamespaceCache: h.Caches.RatelimitNamespace, } }, RequiredPermissions: []string{"ratelimit.*.read_override"}, SetupResources: func(h *testutil.Harness) authz.TestResources { + const ( + keyNamespaceID = "namespace_id" + keyOverrideID = "override_id" + ) ctx := context.Background() // Create a namespace namespaceID := uid.New(uid.RatelimitNamespacePrefix) err := db.Query.InsertRatelimitNamespace(ctx, h.DB.RW(), db.InsertRatelimitNamespaceParams{ ID: namespaceID, WorkspaceID: h.Resources().UserWorkspace.ID, Name: uid.New("test"), CreatedAt: time.Now().UnixMilli(), }) if err != nil { t.Fatalf("failed to create namespace: %v", err) } // Create an override overrideID := uid.New(uid.RatelimitOverridePrefix) err = db.Query.InsertRatelimitOverride(ctx, h.DB.RW(), db.InsertRatelimitOverrideParams{ ID: overrideID, WorkspaceID: h.Resources().UserWorkspace.ID, NamespaceID: namespaceID, Identifier: "test_identifier", Limit: 10, Duration: 1000, CreatedAt: time.Now().UnixMilli(), }) if err != nil { t.Fatalf("failed to create override: %v", err) } return authz.TestResources{ WorkspaceID: h.Resources().UserWorkspace.ID, Custom: map[string]string{ - "namespace_id": namespaceID, - "override_id": overrideID, + keyNamespaceID: namespaceID, + keyOverrideID: overrideID, }, } }, CreateRequest: func(res authz.TestResources) handler.Request { + const keyNamespaceID = "namespace_id" return handler.Request{ - Namespace: res.Custom["namespace_id"], + Namespace: res.Custom[keyNamespaceID], Identifier: "test_identifier", } }, }, ) }go/apps/api/routes/v2_keys_add_roles/403_test.go (1)
25-25: Add AdditionalPermissionTests for single-permission scenarios.The authorization test helper generates test cases for "no permissions" and wrong actions per-permission, but does not test subsets of multi-permission requirements. Add explicit AdditionalPermissionTests for:
- only
"api.*.update_key"- only
"rbac.*.add_role_to_key"to verify that each individual permission is required and not sufficient alone.
go/apps/api/routes/v2_keys_reroll_key/403_test.go (1)
59-70: Add a positive “recoverable key” case.You cover the 403 when encrypt_key is missing. Also assert the pass path (create_key + encrypt_key on the recoverable key) to prevent future regressions. This likely fits as an additional “combination” in the helper rather than a manual test here.
go/apps/api/routes/v2_keys_delete_key/403_test.go (2)
69-112: De-duplicate the cross-API test; move it into the helper.This manual subtest repeats harness setup and overlaps with what the helper can cover. Prefer expressing it via AdditionalPermissionTests if the helper supports dynamic permission strings (e.g., “api.{OtherApiID}.delete_key”) derived from TestResources. Consider extending PermissionTestCase to allow building permissions from resources (e.g., BuildPermissions func(res TestResources) []string), then drop this block.
3-12: Prune fmt import after removing the manual subtest.Once the manual block is gone, fmt becomes unused.
import ( - "fmt" "testing"go/pkg/testutil/authz/authz.go (2)
268-292: Add a “specific resource permission” pass case.When RequiredPermissions contain a wildcard and ApiID is present, also test the specific-id variant should pass.
testCases := []struct { name string permissions []string shouldPass bool }{ { name: "exact required permission", permissions: config.RequiredPermissions, shouldPass: true, }, + // If wildcard, also verify specific resource id permission passes + } + + // Append specific-id pass case when applicable + if resources.ApiID != "" { + specific := make([]string, 0, len(config.RequiredPermissions)) + for _, p := range config.RequiredPermissions { + parts := strings.Split(p, ".") + if len(parts) >= 3 && parts[1] == "*" { + parts[1] = resources.ApiID + specific = append(specific, strings.Join(parts, ".")) + } + } + if len(specific) > 0 { + testCases = append(testCases, struct { + name string + permissions []string + shouldPass bool + }{ + name: "specific resource permission", + permissions: specific, + shouldPass: true, + }) + } }
174-204: Enable dynamic permissions in custom 403 cases (optional).To eliminate route‑local manual tests (e.g., delete_key), consider extending PermissionTestCase to support building permissions from TestResources (e.g., BuildPermissions func(res TestResources) []string). Runner would prefer BuildPermissions over static Permissions when provided.
go/pkg/testutil/authz/types.go (4)
52-56: Document nil semantics for hooks.Make it explicit that nil is acceptable and what happens.
Doc-only change:
-// SetupResources optionally sets up test data before running tests. -// Returns resource IDs that can be used in CreateRequest and permission checks. -// If nil, uses default resources from harness.Resources(). +// SetupResources optionally sets up test data before running tests. +// Returns resource IDs that can be used in CreateRequest and permission checks. +// If nil, the harness default (harness.Resources()) is used. @@ -// ModifyRequest optionally modifies the request for this specific test +// ModifyRequest optionally modifies the request for this specific test. +// If nil, the original request is used unmodified.Also applies to: 69-71
61-77: Consider exposing typed error validation.If many endpoints return structured errors, allowing a typed validator can reduce parsing boilerplate later.
Option (non-breaking, additive):
type PermissionTestCase[TReq any] struct { @@ // ValidateError optionally validates the error response ValidateError func(t *testing.T, errorDetail string) + // ValidateResponse optionally validates the full raw body. + // Ignored if nil; keep errorDetail for simple cases. + ValidateResponse func(t *testing.T, body []byte) }This keeps string-based checks while enabling richer assertions when needed.
19-24: Refactor struct fields to use Go initialism convention: ApiID → APIID, OtherApiID → OtherAPIID.This refactor aligns with Go style guidelines for initialisms (API, ID). The struct fields are defined in
types.go(lines 19–24) and are used throughout test files ingo/apps/api/routes/*/and inauthz.go(line 239). Update all usages to match the renamed fields.- // ApiID is the primary API ID for testing - ApiID string + // APIID is the primary API ID for testing + APIID string - // OtherApiID is a secondary API for cross-resource testing - OtherApiID string + // OtherAPIID is a secondary API for cross-resource testing + OtherAPIID stringAffected locations:
go/pkg/testutil/authz/types.go,go/pkg/testutil/authz/authz.go:239, all test files initializing this struct ingo/apps/api/routes/*/, andgo/pkg/testutil/authz/README.md.
40-59: Based on my verification, I can now generate the rewritten review comment. The core finding is that TRes is indeed unused in both struct definitions and the test helper functions, confirming the review comment's central concern.
TRes is unused: document the intent or remove it.
Both
PermissionTestConfigandAuth401TestConfigdeclare TRes but never use it in any struct field. Similarly,Test401has TRes in its signature but doesn't use it, andTest403receives it via config but the config never references it.If TRes is retained for future extensibility or API design, add a comment to each type clarifying this. Otherwise, remove it:
-type PermissionTestConfig[TReq any, TRes any] struct { +type PermissionTestConfig[TReq any] struct {-type Auth401TestConfig[TReq any, TRes any] struct { +type Auth401TestConfig[TReq any] struct {-func Test401[TReq any, TRes any]( +func Test401[TReq any](-func Test403[TReq any, TRes any]( +func Test403[TReq any](Note: This requires updating all instantiation sites across the codebase (30+ test files currently pass
handler.Responseas the unused second argument).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
go/apps/api/routes/v2_apis_create_api/401_test.go(1 hunks)go/apps/api/routes/v2_apis_create_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/401_test.go(1 hunks)go/apps/api/routes/v2_apis_delete_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/401_test.go(1 hunks)go/apps/api/routes/v2_apis_get_api/403_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/401_test.go(1 hunks)go/apps/api/routes/v2_apis_list_keys/403_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_create_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_delete_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_get_identity/403_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/401_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/403_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/401_test.go(1 hunks)go/apps/api/routes/v2_identities_update_identity/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/401_test.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_create_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_get_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_get_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_roles/401_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_reroll_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_set_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_keys_set_roles/401_test.go(1 hunks)go/apps/api/routes/v2_keys_set_roles/403_test.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/401_test.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/403_test.go(1 hunks)go/apps/api/routes/v2_keys_update_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_update_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/401_test.go(1 hunks)go/apps/api/routes/v2_keys_whoami/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_create_permission/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_create_permission/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_create_role/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_create_role/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_delete_permission/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_delete_permission/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_delete_role/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_delete_role/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_get_permission/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_get_permission/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_get_role/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_get_role/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_list_permissions/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_list_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_permissions_list_roles/401_test.go(1 hunks)go/apps/api/routes/v2_permissions_list_roles/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_delete_override/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_delete_override/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_get_override/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_get_override/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_set_override/401_test.go(1 hunks)go/apps/api/routes/v2_ratelimit_set_override/403_test.go(1 hunks)go/pkg/testutil/authz/README.md(1 hunks)go/pkg/testutil/authz/authz.go(1 hunks)go/pkg/testutil/authz/types.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
PR: unkeyed/unkey#3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_add_permissions/401_test.gogo/apps/api/routes/v2_keys_update_credits/401_test.gogo/apps/api/routes/v2_keys_create_key/401_test.go
🔇 Additional comments (68)
go/apps/api/routes/v2_permissions_get_role/403_test.go (2)
8-10: Imports look good.New testutil/authz, seed, and zen imports are correct and are used.
13-42: Good migration to the shared authz harness.Clear reduction in boilerplate and centralized 403 behavior. Matches the PR objective to standardize 401/403 tests.
go/apps/api/routes/v2_apis_list_keys/403_test.go (1)
15-65: Test correctly validates all permission boundaries for the route.The 403 test properly covers:
- Missing
api.*.read_keypermission- Missing
api.*.read_apipermission- Attempting decrypt without
api.*.decrypt_keypermission (conditional check whenDecrypt=true)Handler enforcement verified: decrypt permission is only checked when
req.Decryptis true, matching the test's conditional case exactly.go/apps/api/routes/v2_permissions_create_role/401_test.go (1)
8-9: Handler dependencies verified; refactor is sound.All required services (Logger, DB, Keys, Auditlogs) are properly declared in the Handler struct and correctly wired in the test. The Handle() method implementation only uses these four declared fields—no Vault, KeyCache, or ApiCache references found. The auth helper migration is clean and complete.
go/apps/api/routes/v2_keys_create_key/403_test.go (1)
15-57: Encrypt_key enforcement verified—no issues found.The handler correctly enforces the
encrypt_keypermission whenRecoverable=true. At handler.go lines 144–152, the code callsauth.VerifyRootKeywithrbac.EncryptKeyaction within the encryption block, returning an error (403) if the permission is missing. The test case correctly validates this behavior.go/apps/api/routes/v2_identities_update_identity/401_test.go (1)
13-34: Excellent refactoring!The migration to the centralized
authz.Test401helper successfully reduces boilerplate while maintaining test coverage. The route and request factories are properly structured and provide appropriate test data.go/apps/api/routes/v2_permissions_create_permission/401_test.go (1)
12-29: LGTM!The refactoring correctly applies the centralized testing pattern with appropriate route and request factories.
go/apps/api/routes/v2_ratelimit_list_overrides/401_test.go (1)
12-27: Refactoring looks good.The implementation correctly uses the centralized helper pattern.
go/apps/api/routes/v2_ratelimit_delete_override/401_test.go (1)
13-31: LGTM!The helper integration is correct, and the route factory properly includes the cache dependency.
go/apps/api/routes/v2_permissions_list_permissions/401_test.go (1)
12-25: LGTM!The empty request factory is appropriate for a list endpoint that doesn't require specific identifiers.
go/apps/api/routes/v2_keys_add_roles/401_test.go (1)
13-31: LGTM!The refactoring correctly implements the helper pattern with appropriate test data for the roles endpoint.
go/apps/api/routes/v2_apis_delete_api/401_test.go (1)
13-30: LGTM!The helper integration is correct with appropriate test data for the API deletion endpoint.
go/apps/api/routes/v2_permissions_delete_permission/401_test.go (1)
12-28: LGTM!The refactoring correctly applies the helper pattern for the permission deletion endpoint.
go/apps/api/routes/v2_ratelimit_get_override/401_test.go (1)
13-29: LGTM! Clean refactoring to centralized auth helper.The test correctly uses the
authz.Test401helper with appropriate handler setup and request construction. The use ofuid.New("test")for the namespace is appropriate for generating a valid test identifier.go/apps/api/routes/v2_permissions_create_permission/403_test.go (1)
12-32: LGTM! Appropriate configuration for permission creation.The test correctly uses
authz.Test403with the required permission "rbac.*.create_permission". The absence ofSetupResourcesis appropriate here since permission creation likely only requires workspace context, which is provided by the default test harness.go/apps/api/routes/v2_ratelimit_set_override/401_test.go (1)
13-32: LGTM! Proper 401 test setup for set override endpoint.The handler setup appropriately includes
Auditlogs(needed for state-modifying operations) and the request factory correctly provides all required fields (Namespace, Identifier, Limit, Duration) for setting a rate limit override.go/apps/api/routes/v2_apis_delete_api/403_test.go (1)
13-43: LGTM! Well-structured authorization test for API deletion.The test properly sets up resources (creates an API to delete), specifies the correct permission ("api.*.delete_api"), and constructs a request that references the created API. The handler configuration appropriately includes Caches for potential cache invalidation during deletion.
go/apps/api/routes/v2_permissions_get_permission/401_test.go (1)
12-27: LGTM! Concise unauthorized access test.The test appropriately uses
authz.Test401with minimal handler setup (suitable for a read operation) and a straightforward test permission identifier.go/apps/api/routes/v2_apis_create_api/401_test.go (1)
12-32: LGTM! Well-documented unauthorized access test.The test includes helpful documentation explaining its purpose and correctly uses
authz.Test401with appropriate handler setup (including Auditlogs for the creation operation).go/apps/api/routes/v2_permissions_list_roles/401_test.go (1)
12-25: LGTM! Appropriate test for list endpoint.The test correctly uses an empty request (appropriate for a list endpoint with no required parameters) and minimal handler setup for a read-only operation.
go/apps/api/routes/v2_keys_remove_roles/403_test.go (1)
26-63: ****The test logic is correct and intentional. The handler implements idempotent role removal: it validates that requested roles exist in the workspace, but gracefully skips roles not attached to the key without error. The 200_test.go confirms this with an explicit test case "idempotent operation - removing non-assigned role", which validates this exact behavior. The 403_test.go correctly sets up an authorization test by creating a separate role and requesting its removal to verify the 403 permission check works regardless of attachment status.
Likely an incorrect or invalid review comment.
go/apps/api/routes/v2_ratelimit_limit/401_test.go (1)
13-33: LGTM: clean 401 harness wiring.Route factory and request builder look correct and minimal for auth-only coverage.
go/apps/api/routes/v2_permissions_delete_permission/403_test.go (1)
16-43: LGTM: cohesive 403 config.Handler setup, workspace-scoped seeding, and request mapping are consistent with the authz harness.
go/apps/api/routes/v2_identities_list_identities/401_test.go (1)
13-24: LGTM: minimal 401 test config.Route factory and empty request are appropriate since auth fails before payload validation.
go/apps/api/routes/v2_identities_get_identity/403_test.go (1)
16-41: LGTM: 403 harness wiring and resource seeding.Configuration is clear; workspace and identity setup align with RBAC checks.
go/apps/api/routes/v2_permissions_get_role/401_test.go (1)
13-26: LGTM: concise 401 coverage.Route factory and request builder are appropriate for auth-only validation.
go/apps/api/routes/v2_apis_list_keys/401_test.go (1)
14-29: Verified: 401 test helper validates header semantics and error responses comprehensively.The
authz.Test401helper performs extensive header validation (7 scenarios including malformed headers, missing auth, bearer spacing) and asserts response body is non-nil. Your test setup correctly delegates to this helper. Addingt.Parallel()is safe and optional since the test has no shared state.go/apps/api/routes/v2_permissions_delete_role/401_test.go (1)
13-27: 401 test is correctly structured; parity verification concern was unfounded.The test correctly uses
authz.Test401, notTest403. These helpers have intentionally different signatures by design—Test401accepts inline functions whileTest403uses a config struct—because they test different concerns (authentication vs. permission). Your minimal Request and Handler setup are appropriate for auth-only testing.Adding
t.Parallel()is optional and safe since the test is isolated.go/apps/api/routes/v2_apis_get_api/401_test.go (1)
13-28: LGTM! Clean migration to centralized auth test helper.The test correctly uses the
authz.Test401helper with appropriate handler dependencies and a valid request factory that generates a placeholder API ID.go/apps/api/routes/v2_keys_get_key/401_test.go (1)
14-32: LGTM! Proper migration to the auth helper framework.The test correctly migrates to
authz.Test401with all necessary handler dependencies (including Vault and Auditlogs) and creates a valid request with a placeholder KeyId and Decrypt flag.go/apps/api/routes/v2_keys_whoami/401_test.go (1)
12-27: LGTM! Appropriate test setup for the whoami endpoint.The test correctly uses the
authz.Test401helper with minimal handler dependencies and a placeholder invalid key string, which is appropriate for authentication error testing.go/apps/api/routes/v2_keys_create_key/401_test.go (1)
13-31: LGTM! Well-documented placeholder usage.The test correctly migrates to
authz.Test401with appropriate handler dependencies. The inline comment explaining the placeholder API ID is helpful for future maintainers.go/apps/api/routes/v2_keys_add_permissions/403_test.go (1)
13-60: LGTM! Comprehensive 403 test configuration.The test correctly uses
authz.Test403with appropriate:
- Handler dependencies including KeyCache for verification
- Required permissions covering both key updates and RBAC operations
- Resource setup creating all necessary entities (API, Key, Permission)
- Request construction using the created resources
go/apps/api/routes/v2_apis_get_api/403_test.go (1)
13-41: LGTM! Well-structured 403 test for read operations.The test correctly uses
authz.Test403with:
- Appropriate minimal handler dependencies for a read operation
- Single required permission
api.*.read_api- Simple resource setup creating just the API entity needed
- Request construction using the created API ID
go/apps/api/routes/v2_permissions_list_permissions/403_test.go (1)
12-28: LGTM! Minimal configuration appropriate for list endpoint.The test correctly uses
authz.Test403with therbac.*.read_permissionpermission. The empty request and absence ofSetupResourcesare appropriate for a list endpoint that operates at the workspace scope rather than requiring specific resource IDs.go/apps/api/routes/v2_keys_delete_key/401_test.go (1)
13-29: LGTM on the 401 refactor.Helper usage, handler wiring, and minimal request shape are consistent and clear.
go/apps/api/routes/v2_keys_remove_roles/401_test.go (1)
14-30: Looks good; helper-based 401 test is straightforward.Minimal request plus standard wiring is sufficient for auth-path checks.
go/apps/api/routes/v2_apis_create_api/403_test.go (1)
27-33: All concerns verified; no issues found.The test is correctly structured:
- Permission string "api.*.create_api" is consistently used across test files and matches the handler's authorization requirements
- Request requires only Name; WorkspaceID is correctly sourced from the auth context (not the request payload)
- SetupResources is not needed for creation endpoints (only for operations on existing resources, like delete/update)
The suggestion to define permission strings as constants is a stylistic improvement rather than a requirement; the hardcoded values are currently consistent across the codebase.
go/apps/api/routes/v2_keys_update_key/401_test.go (1)
13-28: LGTM! Clean refactoring to use shared authentication test helper.The test properly wires the handler dependencies and generates a valid test request. This refactoring successfully reduces boilerplate while maintaining test coverage.
go/apps/api/routes/v2_ratelimit_delete_override/403_test.go (1)
16-74: LGTM! Well-structured authorization test with proper resource setup.The test correctly:
- Wires all handler dependencies including the cache
- Creates necessary test resources (namespace and override)
- Uses the Custom map pattern to pass resource IDs to the request factory
- Specifies the required permission for the operation
The refactoring successfully consolidates the authorization test logic into a configuration-driven pattern.
go/apps/api/routes/v2_identities_list_identities/403_test.go (1)
13-42: LGTM! Appropriate test structure for list endpoint.The test correctly:
- Seeds test identities to ensure permission checks are triggered
- Uses the harness helper method for cleaner resource creation
- Returns an empty request, which is appropriate for list endpoints
- Includes a helpful comment explaining the resource setup rationale
The refactoring maintains test coverage while reducing boilerplate.
go/apps/api/routes/v2_identities_create_identity/401_test.go (1)
12-27: LGTM! Consistent refactoring to shared authentication helper.The test properly:
- Wires all handler dependencies (DB, Keys, Logger, Auditlogs)
- Provides a valid test request with ExternalId
- Follows the same clean pattern as other 401 tests in the PR
go/apps/api/routes/v2_identities_update_identity/403_test.go (1)
13-47: LGTM! Well-structured authorization test for identity updates.The test correctly:
- Uses the harness helper for cleaner identity creation
- Stores the identity ID in the Custom map for request construction
- Creates test metadata inline in the request factory
- Properly passes metadata as a pointer to match the expected type
- Specifies the required permission (identity.*.update_identity)
The refactoring successfully reduces boilerplate while maintaining clear test logic.
go/apps/api/routes/v2_identities_get_identity/401_test.go (1)
12-26: Nice migration to the shared 401 helper.Good use of authz.Test401 with a minimal handler factory and request builder. LGTM.
go/apps/api/routes/v2_ratelimit_set_override/403_test.go (1)
16-60: 403 harness usage looks solid.Handler wiring and resource seeding read clean. RequiredPermissions matches the operation. LGTM.
go/apps/api/routes/v2_ratelimit_limit/403_test.go (2)
13-49: Good 403 coverage with additional permission combinations.Shared harness config is clear; AdditionalPermissionTests make intent explicit. LGTM.
12-35: <!-- [scratchpad_start]
[task review_file_1/1 verification_2/2]
Status: Complete[observations]
- StartAllServices() is intentionally a no-op - services managed externally via docker-compose
- Containers run on fixed localhost addresses (MySQL:3306, Redis, ClickHouse)
- All test harnesses connect to same external services
- NEW HARNESS per test means potential database state conflicts if parallel
- Comments in containers.go note "tests should create and use specific database names to avoid conflicts"
[critical_findings]
Permission slugs ARE VALID:
- "ratelimit..limit" and "ratelimit..create_namespace" confirmed in 24+ locations
- Used consistently across codebase with no typos or variations
t.Parallel() CONTRADICTS CODEBASE PATTERN:
- 0 out of 34 403_test.go files use t.Parallel()
- This is a CONSISTENT PATTERN, not accidental
- Other tests (200_test.go, 400_test.go) in same directories DO use t.Parallel()
- Suggests deliberate exclusion for authorization tests
[analysis]
- Empirical evidence: 34/34 403_test.go files consistently avoid parallelization
- External database services shared by all tests increase risk of state conflicts
- authz.Test403() creates new harness each time, but harness connects to shared external DB
- Pattern suggests sequential execution is deliberate for authorization tests
- Original review comment contradicts established codebase convention
[conclusion]
Permission slugs: ✓ Valid
t.Parallel() suggestion: ✗ Contradicts established pattern across entire codebase
Recommendation: Update review comment to acknowledge pattern and note caution about parallelization[done]
</scratchpad_end] -->Confirm permission slugs are valid; note that adding t.Parallel() contradicts established codebase pattern.
The permission slugs "ratelimit..limit" and "ratelimit..create_namespace" are confirmed valid—they're used consistently across 24+ locations in tests, specs, documentation, and dashboard configuration with no variations.
However, adding
t.Parallel()contradicts an established codebase pattern: all 34 existing 403_test.go files in go/apps/api/routes/ deliberately omit parallelization, despite other test files in the same directories (200_test.go, 400_test.go) usingt.Parallel(). This suggests a deliberate convention for authorization tests, likely because test harnesses connect to shared external database services (MySQL, Redis, ClickHouse) that are centrally managed. Running authorization tests in parallel risks database state conflicts.Before adding
t.Parallel(), verify with the team whether this pattern should change or whether sequential execution remains intentional for authorization tests.go/apps/api/routes/v2_ratelimit_get_override/403_test.go (2)
17-73: Harness config looks correct and readable.Balanced SetupHandler/SetupResources/CreateRequest; LGTM.
27-28: Permission slug verified as correct.The permission slug "ratelimit.*.read_override" is consistently defined and used throughout the codebase for reading rate limit overrides across Go tests, TypeScript implementations, OpenAPI specifications, and dashboard permission configurations. No discrepancies found.
go/apps/api/routes/v2_permissions_get_permission/403_test.go (2)
13-43: Clean refactor to shared 403 test.Use of seed.CreatePermission keeps setup concise. LGTM.
23-41: Constantization is safe; permission slug is valid; t.Parallel() requires verification.The refactoring suggestions are mostly sound:
- Constantize "permission_id": Safe to apply. The string literal appears exactly twice (lines 33, 39) and consolidating to a const reduces duplication risk.
- Permission slug "rbac.*.read_permission": Valid and confirmed across the codebase in multiple endpoints and test fixtures.
- t.Parallel(): Unclear. While the test harness creates isolated resources per invocation (fresh containers, MySQL, Redis), note that tests in
v2_permissions_*routes consistently omitt.Parallel()despite using isolated harnesses—unlikev2_keys_*tests which do use it. This suggests an intentional pattern specific to the authz test framework. Recommend verifying with the maintainers whether theauthz.Test403framework supports parallelization before adding it.go/apps/api/routes/v2_ratelimit_list_overrides/403_test.go (3)
65-69: LGTM: request construction is correct for list-by-namespace.
26-26: Permission slug is correct and consistent.The permission
ratelimit.*.read_overrideat line 26 matches the canonical form used consistently throughout the codebase (tests, API routes, OpenAPI specs, and dashboard). No issues found.
27-40: No critical issues found; t.Fatalf concern is incorrect for this context, and seeding optimization is optional.The SetupResources closure runs synchronously on the main test thread (authz.go line 159), making t.Fatalf safe. No separate goroutine executes it. Permission enforcement happens before Handler.Handle() is invoked, so the namespace and override seeding is only consumed if a test case passes authorization. Since the 403 test contains no AdditionalPermissionTests, all scenarios remain 403-only, making the seeding unnecessary—but this is an optimization choice, not a correctness issue. The permission slug "ratelimit.*.read_override" is correctly defined (rbac/permissions.go:92).
go/apps/api/routes/v2_permissions_delete_role/403_test.go (2)
16-23: LGTM: concise 403 harness usage; handler and request wiring look solid.Also applies to: 25-43
24-24: Permission slug is correct and properly defined.The permission "rbac..delete_role" is correctly constructed from the RBAC tuple format. The handler implementation uses
rbac.Tuple{ResourceType: rbac.Rbac, ResourceID: "*", Action: rbac.DeleteRole}, which converts via theTuple.String()method to produce exactly "rbac..delete_role". TheDeleteRoleaction constant is properly defined, and the pattern is used consistently across tests and API specifications.go/apps/api/routes/v2_keys_update_credits/401_test.go (2)
15-34: LGTM: clear 401 harness; request payload is minimal and valid.
15-34: No issues found—Test401 helper adequately covers all 401 variants.The
Test401helper comprehensively exercises missing headers, invalid schemes/tokens, and signature validation: invalid bearer token (401), nonexistent key (401), malformed headers without Bearer prefix (400), Bearer-only (400), missing header (400), and empty header (400). The code in lines 15–34 correctly delegates to this reusable helper rather than writing bespoke tests, avoiding future coverage gaps.go/apps/api/routes/v2_keys_add_roles/403_test.go (2)
16-24: LGTM: good use of shared 403 harness; resources and request are correctly wired.Also applies to: 25-57
25-25: Permission slugs are correctly configured and enforced.Both "api..update_key" and "rbac..add_role_to_key" are properly defined in the RBAC system (go/pkg/rbac/permissions.go), actively enforced by the handler via
auth.VerifyRootKey(), and tested for authorization failures in the 403_test.go. The permissions follow the established pattern used across other similar routes (set_roles, remove_roles, add_permissions).go/apps/api/routes/v2_keys_get_key/403_test.go (2)
14-25: LGTM: solid 403 coverage including decrypt-specific permutations; handler wiring looks correct.Also applies to: 26-49, 50-69
26-26: Permission slugs confirmed as valid and consistently used throughout the system.Both "api..read_key" and "api..decrypt_key" are properly registered as permissions in the RBAC system. They're created dynamically during testing and used consistently across test files, API routes, OpenAPI specifications, and dashboard components. The permission slug naming follows the system's established pattern ("api.{scope}.{action}") and matches policy expectations. No conflicts or inconsistencies found.
go/apps/api/routes/v2_keys_update_credits/403_test.go (1)
15-56: LGTM: consolidated 403 test is correct and complete.Handler wiring, RequiredPermissions, seeded resources, and request body (Increment + nullable value) look consistent with the endpoint’s contract.
go/apps/api/routes/v2_keys_add_permissions/401_test.go (1)
14-30: LGTM: standardized 401 coverage via helper.Good shift to the shared Test401 harness; minimal boilerplate and clear factories. Based on learnings.
go/apps/api/routes/v2_identities_create_identity/403_test.go (1)
12-30: LGTM: 403 helper usage matches route semantics.Permission, handler wiring, and request are appropriate.
go/apps/api/routes/v2_keys_verify_key/401_test.go (1)
12-28: LGTM: concise 401 tests via helper.Route factory and request are minimal and correct.
go/apps/api/routes/v2_keys_update_key/403_test.go (1)
13-48: LGTM: standardized 403 path.Solid use of helper; resources include ApiID enabling cross-resource checks.
go/pkg/testutil/authz/types.go (1)
1-8: Good addition; clean, minimal surface.Package, imports, and public API placement look solid for a shared testutil module.
| func() handler.Request { | ||
| return handler.Request{ | ||
| Identity: uid.New("test"), | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the Identity UID prefix.
The request factory uses uid.New("test") for the Identity field. Verify that "test" is a valid UID prefix for identity objects, or if it should use a constant like uid.IdentityPrefix for consistency with other routes that use uid.APIPrefix and uid.KeyPrefix.
🏁 Script executed:
#!/bin/bash
# Description: Search for the correct identity UID prefix constant and verify usage patterns
# Search for IdentityPrefix constant definition
ast-grep --pattern 'const $_ = "id_"'
# Search for other identity UID generation patterns in test files
rg -n "Identity.*uid\.New" --type=go -A2 -B2
# Search for uid prefix constants
rg -n "Prefix.*=.*\"[a-z]+_\"" --type=go go/pkg/uid/Length of output: 6538
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for uid package constants and their values
# Find the uid package definition file
fd -e go "uid" go/pkg/ --type f
# Look at the uid package constants
rg -n "const.*Prefix.*=" --type=go go/pkg/uid/ -A1
# Check specifically for TestPrefix and IdentityPrefix
rg -n "(TestPrefix|IdentityPrefix|IdentityPrefix|=\"test\")" --type=go go/pkg/uid/
Length of output: 244
Replace hardcoded "test" with uid.TestPrefix constant for consistency.
The code at line 25 uses uid.New("test") as a hardcoded string literal. The uid package defines TestPrefix = "test" for tests only. Use uid.New(uid.TestPrefix) instead to align with the uid package conventions and match the pattern used throughout other test files (e.g., line 62 in v2_identities_get_identity/400_test.go).
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_identities_delete_identity/401_test.go around lines 23
to 27, replace the hardcoded uid.New("test") with uid.New(uid.TestPrefix) to use
the uid package's TestPrefix constant; update the handler.Request.Identity value
to call uid.New with uid.TestPrefix so tests follow the established convention
and remain consistent with other test files.
| RequiredPermissions: []string{"identity.*.delete_identity"}, | ||
| CreateRequest: func(res authz.TestResources) handler.Request { | ||
| return handler.Request{ | ||
| Identity: uid.New("test"), | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Validate permission key and identity ID; consider seeding if authz isn’t first.
- Use the real identity prefix constant instead of a raw "test" prefix to avoid format validation failures.
- Ensure the 403 path is exercised before any payload/resource validation. If not, seed a valid identity and pass its ID.
Suggested change (adjust the constant name to your actual one):
- return handler.Request{
- Identity: uid.New("test"),
- }
+ return handler.Request{
+ Identity: uid.New(uid.IdentityPrefix),
+ }Run to confirm the enforced permission name and whether the handler authorizes before validating the request, and to inspect the Request fields:
🏁 Script executed:
#!/bin/bash
# Check handler permission usage/naming
rg -nP -C2 '"identity\.\*\.[^"]*delete_identity"|Require(d)?Permissions|Authorize' go/apps/api/routes/v2_identities_delete_identity
# Inspect Request shape for this route
rg -nP -C3 'type\s+Request\s+struct' go/apps/api/routes/v2_identities_delete_identityLength of output: 6788
🏁 Script executed:
#!/bin/bash
# Find uid package and identity prefix constant
rg -nP -C3 'IdentityPrefix|const.*Prefix' go/apps/api/routes/v2_identities_delete_identity
# Search in broader uid package
fd 'uid' -t d go/apps/api
rg -nP 'package uid|const.*Prefix' --type goLength of output: 3368
🏁 Script executed:
#!/bin/bash
# Find Request struct definition in this route
fd -e go go/apps/api/routes/v2_identities_delete_identity -exec grep -l "type.*Request\s*struct" {} \;
# Look at handler registration/middleware
rg -nP 'RequiredPermissions|Middleware|Register' go/apps/api/routes/v2_identities_delete_identity/handler.go -A2 -B2Length of output: 257
🏁 Script executed:
#!/bin/bash
# Check test resources setup in 200_test.go and compare with 403_test.go
rg -nP 'SetupResources|CreateRequest' go/apps/api/routes/v2_identities_delete_identity/200_test.go -A5 -B2
rg -nP 'SetupResources|CreateRequest' go/apps/api/routes/v2_identities_delete_identity/403_test.go -A5 -B2Length of output: 236
🏁 Script executed:
#!/bin/bash
# Correct fd syntax - find files then search within them
fd '403_test.go|200_test.go' go/apps/api/routes/v2_identities_delete_identity
# Show full test case setup in 200_test.go
rg -nP 'func.*TestDeleteIdentity|SetupResources|CreateRequest' go/apps/api/routes/v2_identities_delete_identity/200_test.go -A10 | head -50Length of output: 381
🏁 Script executed:
#!/bin/bash
# Find Request struct definition
rg -nP 'type\s+Request\s+struct' go/apps/api/routes/v2_identities_delete_identity -A10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if 403_test.go is missing SetupResources compared to 200_test.go
echo "=== 403_test.go structure ==="
cat go/apps/api/routes/v2_identities_delete_identity/403_test.go | head -35
echo -e "\n=== 200_test.go first test case ==="
rg -nP 'SetupResources|CreateRequest' go/apps/api/routes/v2_identities_delete_identity/200_test.go -B10 -A10 | head -40Length of output: 906
Replace "test" prefix with uid.IdentityPrefix constant for proper UID formatting.
The code uses uid.New("test") but should use uid.New(uid.IdentityPrefix) to match the pattern in other tests within the same route (200_test.go:26 and 404_test.go:51, 87). The handler calls FindIdentity with the request identity, which validates UID format—using the correct prefix constant ensures proper formatting and consistency.
- return handler.Request{
- Identity: uid.New("test"),
- }
+ return handler.Request{
+ Identity: uid.New(uid.IdentityPrefix),
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RequiredPermissions: []string{"identity.*.delete_identity"}, | |
| CreateRequest: func(res authz.TestResources) handler.Request { | |
| return handler.Request{ | |
| Identity: uid.New("test"), | |
| } | |
| }, | |
| }, | |
| RequiredPermissions: []string{"identity.*.delete_identity"}, | |
| CreateRequest: func(res authz.TestResources) handler.Request { | |
| return handler.Request{ | |
| Identity: uid.New(uid.IdentityPrefix), | |
| } | |
| }, |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_identities_delete_identity/403_test.go around lines 24
to 30, the test constructs a UID with uid.New("test") which uses a literal
string prefix; replace that with uid.New(uid.IdentityPrefix) so the Identity UID
has the proper prefix format used elsewhere. Update the CreateRequest to return
handler.Request{ Identity: uid.New(uid.IdentityPrefix) } to match other tests
and satisfy FindIdentity validation.
| // Cross-API permission test | ||
| t.Run("permission for different API", func(t *testing.T) { | ||
| // Create root key with create permission for other API | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, fmt.Sprintf("api.%s.create_key", otherApiID)) | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| h := testutil.NewHarness(t) | ||
|
|
||
| route := &handler.Handler{ | ||
| DB: h.DB, | ||
| Keys: h.Keys, | ||
| Logger: h.Logger, | ||
| Auditlogs: h.Auditlogs, | ||
| Vault: h.Vault, | ||
| } | ||
| h.Register(route) | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("permission for specific API but requesting different API", func(t *testing.T) { | ||
| // Create root key with create permission for specific API | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, fmt.Sprintf("api.%s.create_key", otherApiID)) | ||
| // Create two APIs | ||
| api := h.CreateApi(seed.CreateApiRequest{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| }) | ||
| otherApi := h.CreateApi(seed.CreateApiRequest{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| }) | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
| // Create root key with permission for otherApi only | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, fmt.Sprintf("api.%s.create_key", otherApi.ID)) | ||
|
|
||
| // Try to create key for different API | ||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("unrelated permission", func(t *testing.T) { | ||
| // Create root key with completely unrelated permission | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, "workspace.read") | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("partial permission match", func(t *testing.T) { | ||
| // Create root key with permission that partially matches but isn't sufficient | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, "api.create") | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| }) | ||
|
|
||
| t.Run("create recoverable key without perms", func(t *testing.T) { | ||
| // Create root key with permission that partially matches but isn't sufficient | ||
| rootKey := h.CreateRootKey(h.Resources().UserWorkspace.ID, "api.*.create_key") | ||
|
|
||
| req := handler.Request{ | ||
| ApiId: apiID, | ||
| Recoverable: ptr.P(true), | ||
| ApiId: api.ID, | ||
| } | ||
|
|
||
| headers := http.Header{ | ||
| res := testutil.CallRoute[handler.Request, handler.Response](h, route, map[string][]string{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
| }, req) | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse](h, route, headers, req) | ||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| if res.Status != 403 { | ||
| t.Errorf("expected 403, got %d, body: %s", res.Status, res.RawBody) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Consolidate cross-API case into the authz helper (or extend helper to support dynamic permissions).
Mixing manual subtests with the helper defeats the standardization goal. Prefer expressing this as an AdditionalPermissionTest that grants permission for OtherApiID and targets ApiID. If the helper lacks a way to compute permissions from TestResources, consider extending PermissionTestCase with a BuildPermissions(res TestResources) []string hook so this scenario can live in the unified harness.
🏁 Script executed:
#!/bin/bash
# Check whether PermissionTestCase supports dynamic permissions today
rg -n 'type\s+PermissionTestCase' go/pkg/testutil/authz -C5
rg -n 'BuildPermissions|PermissionsFn' go/pkg/testutil/authz || trueLength of output: 748
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full PermissionTestCase struct definition and related code
rg -n 'type PermissionTestCase' go/pkg/testutil/authz -A20
Length of output: 1411
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where AdditionalPermissionTests is used/executed in the authz helper
rg -n 'AdditionalPermissionTests' go/pkg/testutil/authz -B2 -A5
Length of output: 2418
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the test execution loop that uses testCases and Permissions
rg -n 'for.*testCases' go/pkg/testutil/authz/authz.go -A40 | head -60
Length of output: 2067
Extend PermissionTestCase to support dynamic permission computation.
The authz helper currently lacks a way to build permissions dynamically from TestResources. While ModifyRequest already demonstrates callback-based extensibility, Permissions remains static. To consolidate the cross-API test into AdditionalPermissionTests, add a BuildPermissions func(TestResources) []string hook (optional, with static Permissions as fallback). This would allow the cross-API scenario—which grants permission for otherApi but targets api—to be expressed uniformly without manual subtests.
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_create_key/403_test.go around lines 59-96, the
test harness lacks a way to compute permissions dynamically from TestResources;
add an optional BuildPermissions func(TestResources) []string field to the
PermissionTestCase (falling back to the existing static Permissions slice when
nil), update the authz helper to call BuildPermissions(res) if present to
construct permissions for the test, and refactor the cross-API subtest into
AdditionalPermissionTests using this new hook so the test can grant permission
for otherApi while targeting api without manual subtests.
| Name: "decrypt permission without read permission", | ||
| Permissions: []string{"api.*.decrypt_key"}, // Has decrypt but not read | ||
| ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request { | ||
| req.Decrypt = ptr.P(false) | ||
| return req | ||
| }, | ||
| ExpectedStatus: 403, |
There was a problem hiding this comment.
Align test intent: set Decrypt=true to match the test name.
Name says "decrypt permission without read permission" but request sets Decrypt=false. Suggest setting it to true to actually exercise decrypt path while lacking read.
- ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request {
- req.Decrypt = ptr.P(false)
+ ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request {
+ req.Decrypt = ptr.P(true)
return req
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Name: "decrypt permission without read permission", | |
| Permissions: []string{"api.*.decrypt_key"}, // Has decrypt but not read | |
| ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request { | |
| req.Decrypt = ptr.P(false) | |
| return req | |
| }, | |
| ExpectedStatus: 403, | |
| Name: "decrypt permission without read permission", | |
| Permissions: []string{"api.*.decrypt_key"}, // Has decrypt but not read | |
| ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request { | |
| req.Decrypt = ptr.P(true) | |
| return req | |
| }, | |
| ExpectedStatus: 403, |
🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_get_key/403_test.go around lines 61 to 67, the
test named "decrypt permission without read permission" incorrectly sets
req.Decrypt to false; change the test's ModifyRequest to set Decrypt to true so
the request actually exercises the decrypt path while the permissions omit read,
and keep ExpectedStatus 403 unchanged to validate access denial.
| authz.Test403(t, | ||
| authz.PermissionTestConfig[handler.Request, handler.Response]{ | ||
| SetupHandler: func(h *testutil.Harness) zen.Route { | ||
| return &handler.Handler{ | ||
| DB: h.DB, | ||
| Keys: h.Keys, | ||
| Logger: h.Logger, | ||
| Auditlogs: h.Auditlogs, | ||
| KeyCache: h.Caches.VerificationKeyByHash, | ||
| } | ||
| }, | ||
| RequiredPermissions: []string{"api.*.update_key", "rbac.*.add_role_to_key", "rbac.*.remove_role_from_key"}, | ||
| SetupResources: func(h *testutil.Harness) authz.TestResources { | ||
| api := h.CreateApi(seed.CreateApiRequest{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| }) | ||
|
|
||
| roleID := h.CreateRole(seed.CreateRoleRequest{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| Name: "test-role", | ||
| }) | ||
|
|
||
| keyResp := h.CreateKey(seed.CreateKeyRequest{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| KeyAuthID: api.KeyAuthID.String, | ||
| }) | ||
|
|
||
| return authz.TestResources{ | ||
| WorkspaceID: h.Resources().UserWorkspace.ID, | ||
| ApiID: api.ID, | ||
| KeyAuthID: api.KeyAuthID.String, | ||
| KeyID: keyResp.KeyID, | ||
| Custom: map[string]string{ | ||
| "role_id": roleID, | ||
| }, | ||
| } | ||
| }, | ||
| CreateRequest: func(res authz.TestResources) handler.Request { | ||
| return handler.Request{ | ||
| KeyId: res.KeyID, | ||
| Roles: []string{res.Custom["role_id"]}, | ||
| } | ||
| }, | ||
| }, | ||
| }) | ||
| keyID := keyResponse.KeyID | ||
| roleID := keyResponse.RolesIds[0] | ||
|
|
||
| // Test case for insufficient permissions - missing update_key | ||
| t.Run("missing update_key permission", func(t *testing.T) { | ||
| // Create a root key with some permissions but not update_key | ||
| rootKey := h.CreateRootKey(workspace.ID, "api.*.create_key") // Only has create, not update | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| req := handler.Request{ | ||
| KeyId: keyID, | ||
| Roles: []string{roleID}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotNil(t, res.Body.Error) | ||
| require.Contains(t, res.Body.Error.Detail, "permission") | ||
| }) | ||
|
|
||
| // Test case for no permissions at all | ||
| t.Run("no permissions", func(t *testing.T) { | ||
| // Create a root key with no permissions | ||
| rootKey := h.CreateRootKey(workspace.ID) | ||
|
|
||
| headers := http.Header{ | ||
| "Content-Type": {"application/json"}, | ||
| "Authorization": {fmt.Sprintf("Bearer %s", rootKey)}, | ||
| } | ||
|
|
||
| req := handler.Request{ | ||
| KeyId: keyID, | ||
| Roles: []string{roleID}, | ||
| } | ||
|
|
||
| res := testutil.CallRoute[handler.Request, openapi.ForbiddenErrorResponse]( | ||
| h, | ||
| route, | ||
| headers, | ||
| req, | ||
| ) | ||
|
|
||
| require.Equal(t, 403, res.Status) | ||
| require.NotNil(t, res.Body) | ||
| require.NotNil(t, res.Body.Error) | ||
| require.Contains(t, res.Body.Error.Detail, "permission") | ||
| }) | ||
|
|
||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Permissions and seeding look correct; verify slugs and subset coverage.
Config matches the endpoint intent. Please verify the route enforces these exact slugs and that Test403 generates insufficient-subset cases for multi-permission endpoints.
Consider adding explicit cases for “only add” and “only remove” if the helper doesn’t already cover subsets.
🏁 Script executed:
#!/bin/bash
# Confirm enforced permissions in handler
rg -nP '(add_role_to_key|remove_role_from_key|update_key)' go/apps/api/routes/v2_keys_set_roles -C3
# Sanity-check helper generates subset tests (look for combos/subsets)
rg -n 'subset|combination|insufficient' go/pkg/testutil/authzLength of output: 2865
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect handler implementation
fd -t f 'handler\.go|handler\.Handler' go/apps/api/routes/v2_keys_set_roles
# Look for authorization checks in the handler
rg -nP 'CheckPermission|Authz|enforce' go/apps/api/routes/v2_keys_set_roles/handler.go -A5 -B2
Length of output: 211
---
🏁 Script executed:
```shell
#!/bin/bash
# List directory to see what files exist
ls -la go/apps/api/routes/v2_keys_set_roles/ 2>/dev/null || echo "Directory listing failed"
# Search for Handler struct definition in the route's package
rg -n 'type Handler struct' go/apps/api/routes/v2_keys_set_roles/ -A20
# Search more broadly for authorization checks near add_role_to_key
rg -n 'add_role_to_key.*CheckPermission|CheckPermission.*add_role_to_key' --type=go -C5
Length of output: 2182
---
🏁 Script executed:
```shell
#!/bin/bash
# View the Handle method to see authorization/permission checks
cat go/apps/api/routes/v2_keys_set_roles/handler.go | tail -n +47 | head -n 100
Length of output: 2818
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for add_role_to_key and remove_role_from_key checks in handler
rg -n 'AddRoleToKey|RemoveRoleFromKey|add_role_to_key|remove_role_from_key' go/apps/api/routes/v2_keys_set_roles/handler.go
# Also check what rbac constants exist
rg -n 'rbac\.(Add|Remove|Update)' go/apps/api/routes/v2_keys_set_roles/handler.go
Length of output: 71
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full handler.go line count and verify no other VerifyRootKey calls
wc -l go/apps/api/routes/v2_keys_set_roles/handler.go
# Search for all VerifyRootKey calls to confirm only one permission check
rg -n 'VerifyRootKey' go/apps/api/routes/v2_keys_set_roles/handler.go -A2
Length of output: 169
Fix RequiredPermissions to match handler enforcement.
The handler only enforces api.*.update_key. The add_role_to_key and remove_role_from_key permissions are never checked in the VerifyRootKey call. Update line 25 to reflect the actual enforced permission:
RequiredPermissions: []string{"api.*.update_key"},🤖 Prompt for AI Agents
In go/apps/api/routes/v2_keys_set_roles/403_test.go around lines 14 to 58, the
test's RequiredPermissions includes "rbac.*.add_role_to_key" and
"rbac.*.remove_role_from_key" but the handler only enforces "api.*.update_key";
update the RequiredPermissions entry to only include "api.*.update_key" so the
test matches the handler's actual permission checks.
| func Test401[TReq any, TRes any]( | ||
| t *testing.T, | ||
| setupHandler func(*testutil.Harness) zen.Route, | ||
| createRequest func() TReq, | ||
| ) { | ||
| t.Helper() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make expected 401/400 codes configurable.
Endpoints differ on whether missing/malformed Authorization yields 400 or 401. Add optional options to override defaults per route to reduce flakiness.
-func Test401[TReq any, TRes any](
- t *testing.T,
- setupHandler func(*testutil.Harness) zen.Route,
- createRequest func() TReq,
-) {
+type Auth401Option func(*auth401Config)
+type auth401Config struct {
+ MissingAuthStatus int
+ MalformedAuthStatus int
+}
+func WithMissingAuthStatus(status int) Auth401Option { return func(c *auth401Config) { c.MissingAuthStatus = status } }
+func WithMalformedAuthStatus(status int) Auth401Option { return func(c *auth401Config) { c.MalformedAuthStatus = status } }
+
+func Test401[TReq any, TRes any](
+ t *testing.T,
+ setupHandler func(*testutil.Harness) zen.Route,
+ createRequest func() TReq,
+ opts ...Auth401Option,
+) {
t.Helper()
+ cfg := &auth401Config{
+ MissingAuthStatus: http.StatusBadRequest,
+ MalformedAuthStatus: http.StatusBadRequest,
+ }
+ for _, opt := range opts {
+ opt(cfg)
+ }
testCases := []struct {
name string
authorization string
expectedStatus int
}{
{
name: "invalid bearer token",
authorization: "Bearer invalid_token_12345",
expectedStatus: http.StatusUnauthorized,
},
{
name: "nonexistent key",
authorization: fmt.Sprintf("Bearer %s", uid.New(uid.KeyPrefix)),
expectedStatus: http.StatusUnauthorized,
},
{
name: "bearer with extra spaces",
authorization: "Bearer invalid_key_with_spaces ",
expectedStatus: http.StatusUnauthorized,
},
{
name: "missing authorization header",
authorization: "",
- expectedStatus: http.StatusBadRequest, // Missing auth typically returns 400
+ expectedStatus: cfg.MissingAuthStatus,
},
{
name: "empty authorization header",
authorization: " ",
- expectedStatus: http.StatusBadRequest,
+ expectedStatus: cfg.MissingAuthStatus,
},
{
name: "malformed authorization header - no Bearer prefix",
authorization: "invalid_token_without_bearer",
- expectedStatus: http.StatusBadRequest,
+ expectedStatus: cfg.MalformedAuthStatus,
},
{
name: "malformed authorization header - Bearer only",
authorization: "Bearer",
- expectedStatus: http.StatusBadRequest,
+ expectedStatus: cfg.MalformedAuthStatus,
},
}Also applies to: 47-87
🤖 Prompt for AI Agents
In go/pkg/testutil/authz/authz.go around lines 40-45 (and also apply the same
change for 47-87), the Test401 helper hardcodes expected HTTP status for
missing/malformed Authorization; add an options parameter (e.g., a small struct
with fields like ExpectedMissingCode and ExpectedMalformedCode or a single
ExpectedCode) with sensible defaults (401) so callers can override per-route
behavior, update the function signature to accept the options (or a variadic
opts ...Option) and use the provided values when asserting responses, and
propagate the same configurable option pattern to the other helper(s) in lines
47-87 so tests can set 400 or 401 as needed without flakiness.
| // The test suite automatically generates test cases for: | ||
| // - No permissions | ||
| // - Wrong permissions (different action) | ||
| // - Unrelated permissions | ||
| // - Resource-specific permissions (if applicable) | ||
| // - Cross-workspace scenarios | ||
| // - Permission combinations | ||
| // |
There was a problem hiding this comment.
Doc/code mismatch: “Cross‑workspace scenarios” aren’t implemented.
The suite doesn’t generate a cross‑workspace case. Either drop this bullet or add a test that uses a root key from another workspace and expects 403.
| if tc.shouldPass { | ||
| require.NotEqual(t, http.StatusForbidden, res.Status, | ||
| "expected success, got %d, body: %s", res.Status, res.RawBody) | ||
| require.NotEqual(t, http.StatusUnauthorized, res.Status, | ||
| "expected success, got %d, body: %s", res.Status, res.RawBody) | ||
| } else { | ||
| require.Equal(t, http.StatusForbidden, res.Status, | ||
| "expected 403, got %d, body: %s", res.Status, res.RawBody) | ||
| } |
There was a problem hiding this comment.
Tighten “pass” assertions to catch 5xx regressions.
A 5xx currently passes. Assert non-error status (<400) for shouldPass cases.
- if tc.shouldPass {
- require.NotEqual(t, http.StatusForbidden, res.Status,
- "expected success, got %d, body: %s", res.Status, res.RawBody)
- require.NotEqual(t, http.StatusUnauthorized, res.Status,
- "expected success, got %d, body: %s", res.Status, res.RawBody)
+ if tc.shouldPass {
+ require.Less(t, res.Status, 400,
+ "expected non-error status, got %d, body: %s", res.Status, res.RawBody)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if tc.shouldPass { | |
| require.NotEqual(t, http.StatusForbidden, res.Status, | |
| "expected success, got %d, body: %s", res.Status, res.RawBody) | |
| require.NotEqual(t, http.StatusUnauthorized, res.Status, | |
| "expected success, got %d, body: %s", res.Status, res.RawBody) | |
| } else { | |
| require.Equal(t, http.StatusForbidden, res.Status, | |
| "expected 403, got %d, body: %s", res.Status, res.RawBody) | |
| } | |
| if tc.shouldPass { | |
| require.Less(t, res.Status, 400, | |
| "expected non-error status, got %d, body: %s", res.Status, res.RawBody) | |
| } else { | |
| require.Equal(t, http.StatusForbidden, res.Status, | |
| "expected 403, got %d, body: %s", res.Status, res.RawBody) | |
| } |
🤖 Prompt for AI Agents
In go/pkg/testutil/authz/authz.go around lines 304 to 312, the test currently
only checks that successful cases are not 401 or 403 which lets 5xx responses
slip through; change the assertions for tc.shouldPass to require that res.Status
is less than 400 (non-error) and keep the existing formatted failure message
including res.Status and res.RawBody so any 5xx will fail the test; retain the
else branch asserting 403 for shouldPass==false.
| AdditionalPermissionTests: []authz.PermissionTestCase[handler.Request]{ | ||
| { | ||
| Name: "special scenario", | ||
| Permissions: []string{"some.permission"}, | ||
| ModifyRequest: func(req handler.Request) handler.Request { | ||
| req.SomeField = "modified" | ||
| return req | ||
| }, | ||
| ExpectedStatus: 403, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
README/API mismatch: ModifyRequest signature differs from usage.
Examples show ModifyRequest(func(req TReq) TReq) but tests use ModifyRequest(func(req TReq, res TestResources) TReq). Update examples and the PermissionTestCase docs to the two-arg form to avoid confusion.
- AdditionalPermissionTests: []authz.PermissionTestCase[handler.Request]{
+ AdditionalPermissionTests: []authz.PermissionTestCase[handler.Request]{
{
Name: "special scenario",
Permissions: []string{"some.permission"},
- ModifyRequest: func(req handler.Request) handler.Request {
+ ModifyRequest: func(req handler.Request, res authz.TestResources) handler.Request {
req.SomeField = "modified"
return req
},
ExpectedStatus: 403,
},
},Also clarify in “PermissionTestCase” that ModifyRequest has signature:
ModifyRequest func(req TReq, res TestResources) TReqAlso applies to: 179-186
🤖 Prompt for AI Agents
In go/pkg/testutil/authz/README.md around lines 136-147 (and also update lines
179-186), the example and docs show ModifyRequest as func(req TReq) TReq but the
tests use the two-argument form; change the examples and the PermissionTestCase
documentation to show ModifyRequest as func(req TReq, res TestResources) TReq,
and explicitly state in the PermissionTestCase field description that
ModifyRequest has signature: ModifyRequest func(req TReq, res TestResources)
TReq so examples and docs match actual test usage.
2 participants
What does this PR do?
This change standardises the tests for 401 and 403 tests, while keeping the options to still do custom tests for any endpoint.
This heavily reduces the boilerplate needed to setup tests for endpoints
Fixes #3543
Type of change
How should this be tested?
Tests
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated