refactor(api): require resource in CheckResourcePermission - #1886
refactor(api): require resource in CheckResourcePermission#1886AmanGIT07 wants to merge 1 commit 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.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesPermission resource identifier migration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change enforces the new resource-based request format, with deprecated-only requests rejected as invalid. No actionable merge-blocking risk remains beyond an optional test hardening follow-up. 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
| name: "should return bad request error if resource is missing", | ||
| request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ | ||
| Permission: schema.UpdatePermission, | ||
| }), | ||
| want: nil, | ||
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add a regression case for deprecated fields.
The current missing-resource case sets neither Resource nor ObjectId/ObjectNamespace. It would pass even if the legacy fallback were reintroduced. Add a case that sets only ObjectId and ObjectNamespace and expects CodeInvalidArgument.
Proposed test case
{
name: "should return bad request error if resource is missing",
request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{
Permission: schema.UpdatePermission,
}),
want: nil,
wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest),
},
+ {
+ name: "should reject deprecated object fields when resource is missing",
+ request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{
+ ObjectId: testRelationV2.Object.ID,
+ ObjectNamespace: testRelationV2.Object.Namespace,
+ Permission: schema.UpdatePermission,
+ }),
+ want: nil,
+ wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest),
+ },📝 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 resource is missing", | |
| request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ | |
| Permission: schema.UpdatePermission, | |
| }), | |
| want: nil, | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), | |
| }, | |
| name: "should return bad request error if resource is missing", | |
| request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ | |
| Permission: schema.UpdatePermission, | |
| }), | |
| want: nil, | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), | |
| }, | |
| { | |
| name: "should reject deprecated object fields when resource is missing", | |
| request: connect.NewRequest(&frontierv1beta1.CheckResourcePermissionRequest{ | |
| ObjectId: testRelationV2.Object.ID, | |
| ObjectNamespace: testRelationV2.Object.Namespace, | |
| Permission: schema.UpdatePermission, | |
| }), | |
| want: nil, | |
| wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), | |
| }, |
Coverage Report for CI Build 31791295387Coverage decreased (-0.007%) to 48.691%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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