refactor(api): require resource in CheckResourcePermission - #1886
refactor(api): require resource in CheckResourcePermission#1886AmanGIT07 wants to merge 3 commits into
Conversation
CheckResourcePermission reads the object only from the resource field
("namespace:id") and returns InvalidArgument when it is missing or
malformed. The deprecated object_id/object_namespace request fields are
no longer read. E2E tests send the resource form.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary by CodeRabbit
WalkthroughPermission handlers now require namespace-qualified values in ChangesPermission resource identifier migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The API validation refactor and its callers are updated to use the required resource format; no actionable merge-blocking risk remains after normal checks and review. An additional empty-namespace edge-case test is a non-blocking follow-up. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a381cba-390a-4d76-b8c0-f350fcbc32c1
📒 Files selected for processing (5)
internal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.gotest/e2e/regression/api_test.gotest/e2e/regression/onboarding_test.gotest/e2e/regression/serviceusers_test.go
Coverage Report for CI Build 32038923635Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.05%) to 48.746%Details
Uncovered Changes
Coverage Regressions60 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
BatchCheckPermission rejects a resource with an empty namespace or id as InvalidArgument instead of failing at SpiceDB as internal. All three resource guards in the file, and the federated subject guard, return the namespace-notation error so callers see the expected format. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49074c84-636d-440d-a48b-0457f5e47b3a
📒 Files selected for processing (2)
internal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/permission_check_test.go
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review.
| name: "should return bad request error if a body resource id part is empty", | ||
| request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ | ||
| Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ | ||
| {Resource: "organization:", Permission: schema.UpdatePermission}, | ||
| }, | ||
| }), | ||
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), | ||
| }, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an empty namespace test for BatchCheckPermission.
SplitNamespaceAndResourceID(":id") returns an empty namespace without a parsing error. The handler rejects this branch, but the batch tests only cover malformed input and an empty ID. Add a body with Resource: ":" + testRelationV2.Object.ID and expect CodeInvalidArgument with ErrNamespaceSplitNotation.
Proposed test case
{
name: "should return bad request error if a body resource id part is empty",
request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{
Bodies: []*frontierv1beta1.BatchCheckPermissionBody{
{Resource: "organization:", Permission: schema.UpdatePermission},
},
}),
wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation),
},
+ {
+ name: "should return bad request error if a body resource namespace part is empty",
+ request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{
+ Bodies: []*frontierv1beta1.BatchCheckPermissionBody{
+ {Resource: ":" + testRelationV2.Object.ID, Permission: schema.UpdatePermission},
+ },
+ }),
+ wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation),
+ },📝 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: "should return bad request error if a body resource id part is empty", | |
| request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ | |
| Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ | |
| {Resource: "organization:", Permission: schema.UpdatePermission}, | |
| }, | |
| }), | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), | |
| }, | |
| } | |
| name: "should return bad request error if a body resource id part is empty", | |
| request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ | |
| Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ | |
| {Resource: "organization:", Permission: schema.UpdatePermission}, | |
| }, | |
| }), | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), | |
| }, | |
| { | |
| name: "should return bad request error if a body resource namespace part is empty", | |
| request: connect.NewRequest(&frontierv1beta1.BatchCheckPermissionRequest{ | |
| Bodies: []*frontierv1beta1.BatchCheckPermissionBody{ | |
| {Resource: ":" + testRelationV2.Object.ID, Permission: schema.UpdatePermission}, | |
| }, | |
| }), | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrNamespaceSplitNotation), | |
| }, | |
| } |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
CheckResourcePermission accepts the object only via the
resourcefield (namespace:id). The deprecatedobject_id/object_namespacerequest fields are no longer read; requests sending only those fields now receive InvalidArgument. Namespace aliases keep working insideresource. Part of #1782.Changes
internal/api/v1beta1connect/permission_check.go: remove the split-field fallback; reject a missing or malformedresourcetest/e2e/regression/onboarding_test.go,serviceusers_test.go,api_test.go: check requests sendresourceinternal/api/v1beta1connect/permission_check_test.go: success cases sendresource; add missing-resource caseTest Plan
go test ./internal/api/v1beta1connect/passesmake lintpasses (0 issues)🤖 Generated with Claude Code