refactor(api): require permission key in create and update requests - #1885
refactor(api): require permission key in create and update requests#1885AmanGIT07 wants to merge 3 commits into
Conversation
|
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. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe permission model now uses a consolidated ChangesPermission key migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The API now requires a valid permission key and rejects deprecated namespace/name-only payloads; UpdatePermission still lacks a direct regression case proving that legacy input is rejected, so the change is mergeable with explicit owner awareness or a follow-up test. Possibly related issues
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
🧹 Nitpick comments (1)
internal/api/v1beta1connect/permission_test.go (1)
84-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve that deprecated fields are ignored.
The missing-key cases leave
NameandNamespaceempty. A future fallback to populated deprecated fields could pass these cases. Add CreatePermission and UpdatePermission cases withKeyempty,NameandNamespacepopulated, and expectCodeInvalidArgument.This follows the PR objective that deprecated request fields are no longer read.
Also applies to: 268-286
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a841b60-53d3-4101-b77a-15ff9171c906
📒 Files selected for processing (5)
cmd/seed.gocmd/seed/permissions.jsoninternal/api/v1beta1connect/permission.gointernal/api/v1beta1connect/permission_test.gotest/e2e/regression/service_registration_test.go
Coverage Report for CI Build 32038815790Coverage increased (+0.004%) to 48.739%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/v1beta1connect/permission_test.go (1)
275-315: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd an invalid-namespace case for
UpdatePermission.
internal/api/v1beta1connect/permission.goLine 120 has a separate namespace-validation branch.TestHandler_UpdatePermissioncovers missing keys, malformed keys, and invalid permission names, but it does not cover a three-part key with an invalid namespace. Add anapp..getcase and expectconnect.CodeInvalidArgumentwithErrPermissionKeyNotation.Based on the changed handler branch and the supplied test cases, this is the only new validation path without a direct regression case.
Proposed test case
+ { + name: "should return bad request error if key namespace is invalid", + setup: func(as *mocks.PermissionService) {}, + request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ + Id: testPermissions[testPermissionIdx].ID, + Body: &frontierv1beta1.PermissionRequestBody{ + Key: "app..get", + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), + },
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d7469106-ebfb-4b52-94ba-265fdc69e424
📒 Files selected for processing (3)
internal/api/v1beta1connect/errors.gointernal/api/v1beta1connect/permission.gointernal/api/v1beta1connect/permission_test.go
| @@ -1,50 +1,42 @@ | |||
| [ | |||
| { | |||
| "name": "create", | |||
There was a problem hiding this comment.
What is the purpose of these seed JSON files? It looks like dummy data. If they are not needed, we can think of removing them.
There was a problem hiding this comment.
The six JSON files under cmd/seed/ (permissions, roles, users, organizations, projects, resource) are compiled into the binary with go:embed and used by exactly one thing: the frontier seed CLI command, documented in the CLI reference. Running it against a live server creates a sample world through the public API — the compute/order and database/order permissions, two custom roles, a sample org with users, projects, a resource, and policies. Its purpose is stated in its own help text: making it easier to get started — demo data for someone evaluating Frontier locally.
Removing the command and its fixtures is doable, but that's its own discussion — keeping this PR to the request-field change.
|
@AmanGIT07, did you test that the reconcile flow is working correctly for permissions? |
CreatePermission and UpdatePermission read the permission identity only from the key field and return InvalidArgument when it is missing or malformed. The deprecated namespace/name request fields are no longer read. The seed command and e2e tests send keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Missing or malformed keys now fail with a format-hint error instead of a generic bad request. Create validates the namespace form and Update applies the same name and namespace checks as Create. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Create and Update reject a body that sends only namespace/name, an empty key part, or an invalid key namespace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d3b2cf6 to
ca5b944
Compare
|
@whoAbhishekSah Verified live against a server built from this branch: reconcile dry-run plans the adds, apply creates them through the key-only CreatePermission, an immediate re-run converges to "no changes", export reads back the exact namespace/name pair, and Also rebased the branch onto current main (post #1887, so the key-reading reconciler was in the loop for the run above) and dropped the namespace-grammar commit from this PR — |
3 participants
Summary
CreatePermission and UpdatePermission accept the permission identity only via the
keyfield (service.resource.verb). The deprecatednamespace/namerequest body fields are no longer read; requests sending only those fields now receive InvalidArgument. Part of #1782.Changes
internal/api/v1beta1connect/permission.go: remove the namespace/name fallback; reject a missing or malformed key in both CreatePermission and UpdatePermissioncmd/seed/permissions.json+cmd/seed.go: seed data sendskeytest/e2e/regression/service_registration_test.go: request bodies sendkeyinternal/api/v1beta1connect/permission_test.go: request bodies sendkey; add missing-key and malformed-key cases for both handlersTest Plan
go test ./internal/api/v1beta1connect/passesmake lintpasses (0 issues)🤖 Generated with Claude Code