From 6b786f640fe8a818b32f0faff1c45a6584419d0f Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 14 Aug 2026 15:06:08 +0530 Subject: [PATCH 1/2] refactor(api): require permission key in create and update requests 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 --- cmd/seed.go | 7 +- cmd/seed/permissions.json | 30 +++--- internal/api/v1beta1connect/permission.go | 5 +- .../api/v1beta1connect/permission_test.go | 98 +++++-------------- .../regression/service_registration_test.go | 25 ++--- 5 files changed, 53 insertions(+), 112 deletions(-) diff --git a/cmd/seed.go b/cmd/seed.go index f63b8373f7..2e01c9791a 100644 --- a/cmd/seed.go +++ b/cmd/seed.go @@ -11,6 +11,7 @@ import ( "connectrpc.com/connect" "github.com/MakeNowJust/heredoc" "github.com/raystack/frontier/config" + "github.com/raystack/frontier/internal/bootstrap/schema" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" frontierv1beta1connect "github.com/raystack/frontier/proto/v1beta1/frontierv1beta1connect" "github.com/raystack/salt/cli/printer" @@ -112,10 +113,10 @@ func createCustomRolesAndPermissions(ctx context.Context, client frontierv1beta1 } str := "created custom permissions : " - //nolint:staticcheck for _, v := range permissionBodies { - str = fmt.Sprintf("%s %s:%s", str, v.GetNamespace(), v.GetName()) - resourceNamespaces = append(resourceNamespaces, v.GetNamespace()) + permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(v.GetKey()) + str = fmt.Sprintf("%s %s:%s", str, permNamespace, permName) + resourceNamespaces = append(resourceNamespaces, permNamespace) } fmt.Println(str) diff --git a/cmd/seed/permissions.json b/cmd/seed/permissions.json index 53de504843..16b4af625c 100644 --- a/cmd/seed/permissions.json +++ b/cmd/seed/permissions.json @@ -1,50 +1,42 @@ [ { - "name": "create", + "key": "compute.order.create", "title": "Create Order", - "namespace": "compute/order", "metadata": {} }, { - "name": "get", + "key": "compute.order.get", "title": "Read Order", - "namespace": "compute/order", "metadata": {} }, { - "name": "update", + "key": "compute.order.update", "title": "Update Order", - "namespace": "compute/order", "metadata": {} }, { - "name": "delete", + "key": "compute.order.delete", "title": "Delete Order", - "namespace": "compute/order", "metadata": {} }, { - "name": "list", + "key": "compute.order.list", "title": "List Orders", - "namespace": "compute/order", "metadata": {} }, { - "name": "get", + "key": "database.order.get", "title": "List Database Order", - "namespace": "database/order", - "metadata":{} + "metadata": {} }, { - "name": "update", + "key": "database.order.update", "title": "Update Database Order", - "namespace": "database/order", - "metadata":{} + "metadata": {} }, { - "name": "delete", + "key": "database.order.delete", "title": "Delete Database Order", - "namespace": "database/order", - "metadata":{} + "metadata": {} } ] diff --git a/internal/api/v1beta1connect/permission.go b/internal/api/v1beta1connect/permission.go index 5cf79e3187..e43f9dce1b 100644 --- a/internal/api/v1beta1connect/permission.go +++ b/internal/api/v1beta1connect/permission.go @@ -23,9 +23,6 @@ func (h *ConnectHandler) CreatePermission(ctx context.Context, request *connect. var permissionSlugs []string for _, permBody := range request.Msg.GetBodies() { permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(permBody.GetKey()) - if permNamespace == "" || permName == "" { - permNamespace, permName = permBody.GetNamespace(), permBody.GetName() //nolint:staticcheck - } if permName == "" || permNamespace == "" { return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) } @@ -116,7 +113,7 @@ func (h *ConnectHandler) UpdatePermission(ctx context.Context, request *connect. permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(request.Msg.GetBody().GetKey()) if permNamespace == "" || permName == "" { - permNamespace, permName = request.Msg.GetBody().GetNamespace(), request.Msg.GetBody().GetName() //nolint:staticcheck + return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) } updatedPermission, err := h.permissionService.Update(ctx, permission.Permission{ ID: request.Msg.GetId(), diff --git a/internal/api/v1beta1connect/permission_test.go b/internal/api/v1beta1connect/permission_test.go index 411aa40aef..d283747f42 100644 --- a/internal/api/v1beta1connect/permission_test.go +++ b/internal/api/v1beta1connect/permission_test.go @@ -71,8 +71,7 @@ func TestHandler_CreatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }, }), @@ -82,12 +81,12 @@ func TestHandler_CreatePermission(t *testing.T) { errors.New("test error"))), }, { - name: "should return bad request error if namespace id is empty", + name: "should return bad request error if key is missing", setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Name: testPermissions[testPermissionIdx].Name, + Title: "no key sent", }, }, }), @@ -95,12 +94,12 @@ func TestHandler_CreatePermission(t *testing.T) { wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), }, { - name: "should return bad request error if name is empty", + name: "should return bad request error if key is malformed", setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: "app.resource", }, }, }), @@ -143,12 +142,10 @@ func TestHandler_CreatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Name: testPermissions[testPermissionIdx].Name + "0", - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), }, { - Name: testPermissions[testPermissionIdx].Name + "1", - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"1"), }, }, }), @@ -174,50 +171,6 @@ func TestHandler_CreatePermission(t *testing.T) { }), wantErr: nil, }, - { - name: "should return success if permission service return nil error with permission key", - setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) { - bs.EXPECT().AppendSchema(mock.AnythingOfType("context.backgroundCtx"), schema.ServiceDefinition{ - Permissions: []schema.ResourcePermission{ - { - Name: testPermissions[testPermissionIdx].Name + "0", - Namespace: testPermissions[testPermissionIdx].NamespaceID, - }, - }, - }).Return(nil) - as.EXPECT().List(mock.Anything, permission.Filter{ - Slugs: []string{ - schema.FQPermissionNameFromNamespace(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), - }, - }).Return([]permission.Permission{ - { - ID: testPermissions[testPermissionIdx].ID, - Name: testPermissions[testPermissionIdx].Name + "0", - NamespaceID: testPermissions[testPermissionIdx].NamespaceID, - }, - }, nil) - }, - request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ - Bodies: []*frontierv1beta1.PermissionRequestBody{ - { - Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), - }, - }, - }), - want: connect.NewResponse(&frontierv1beta1.CreatePermissionResponse{ - Permissions: []*frontierv1beta1.Permission{ - { - Id: testPermissions[testPermissionIdx].ID, - Name: testPermissions[testPermissionIdx].Name + "0", - Namespace: testPermissions[testPermissionIdx].NamespaceID, - Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), - CreatedAt: timestamppb.New(testPermissions[testPermissionIdx].CreatedAt), - UpdatedAt: timestamppb.New(testPermissions[testPermissionIdx].UpdatedAt), - }, - }, - }), - wantErr: nil, - }, } for _, tt := range tests { @@ -255,8 +208,7 @@ func TestHandler_UpdatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, Body: &frontierv1beta1.PermissionRequestBody{ - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }), want: nil, @@ -275,8 +227,7 @@ func TestHandler_UpdatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, Body: &frontierv1beta1.PermissionRequestBody{ - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }), want: nil, @@ -291,8 +242,7 @@ func TestHandler_UpdatePermission(t *testing.T) { }, request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Body: &frontierv1beta1.PermissionRequestBody{ - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }), want: nil, @@ -309,24 +259,31 @@ func TestHandler_UpdatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, Body: &frontierv1beta1.PermissionRequestBody{ - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }), want: nil, wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), }, { - name: "should return bad request error if name is empty", - setup: func(as *mocks.PermissionService) { - as.EXPECT().Update(mock.AnythingOfType("context.backgroundCtx"), permission.Permission{ - ID: testPermissions[testPermissionIdx].ID, - NamespaceID: testPermissions[testPermissionIdx].NamespaceID}).Return(permission.Permission{}, permission.ErrInvalidDetail) - }, + name: "should return bad request error if key is missing", + setup: func(as *mocks.PermissionService) {}, request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, Body: &frontierv1beta1.PermissionRequestBody{ - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Title: "no key sent", + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + }, + { + name: "should return bad request error if key is malformed", + setup: func(as *mocks.PermissionService) {}, + request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ + Id: testPermissions[testPermissionIdx].ID, + Body: &frontierv1beta1.PermissionRequestBody{ + Key: "app.resource", }, }), want: nil, @@ -344,8 +301,7 @@ func TestHandler_UpdatePermission(t *testing.T) { request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, Body: &frontierv1beta1.PermissionRequestBody{ - Name: testPermissions[testPermissionIdx].Name, - Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name), }, }), want: connect.NewResponse(&frontierv1beta1.UpdatePermissionResponse{ diff --git a/test/e2e/regression/service_registration_test.go b/test/e2e/regression/service_registration_test.go index 2c7c1c1032..5c36c74d22 100644 --- a/test/e2e/regression/service_registration_test.go +++ b/test/e2e/regression/service_registration_test.go @@ -80,18 +80,15 @@ func (s *ServiceRegistrationRegressionTestSuite) TestServiceRegistration() { createPermResp, err := s.testBench.AdminClient.CreatePermission(ctx, connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Name: "get", - Namespace: "database/instance", - Title: "", + Key: "database.instance.get", + Title: "", }, { - Name: "update", - Namespace: "database/instance", - Title: "update db instance", + Key: "database.instance.update", + Title: "update db instance", }, { - Name: "delete", - Namespace: "database/instance", + Key: "database.instance.delete", Metadata: &structpb.Struct{ Fields: map[string]*structpb.Value{ "description": structpb.NewStringValue("bar"), @@ -117,13 +114,11 @@ func (s *ServiceRegistrationRegressionTestSuite) TestServiceRegistration() { createPermResp, err := s.testBench.AdminClient.CreatePermission(ctx, connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ { - Name: "update", - Namespace: "database/alert", - Title: "update db alert", + Key: "database.alert.update", + Title: "update db alert", }, { - Name: "delete", - Namespace: "database/alert", + Key: "database.alert.delete", Metadata: &structpb.Struct{ Fields: map[string]*structpb.Value{ "description": structpb.NewStringValue("bar"), @@ -166,7 +161,7 @@ func (s *ServiceRegistrationRegressionTestSuite) TestPermissionDeleteCascade() { createPermResp, err := s.testBench.AdminClient.CreatePermission(ctx, connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ - {Name: "act", Namespace: "permcascade/res"}, + {Key: "permcascade.res.act"}, }, })) s.Require().NoError(err) @@ -256,7 +251,7 @@ func (s *ServiceRegistrationRegressionTestSuite) TestPermissionDeleteBlockedByRe // authorizes the "delete" permission on the resource's namespace) createPermResp, err := s.testBench.AdminClient.CreatePermission(ctx, connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ Bodies: []*frontierv1beta1.PermissionRequestBody{ - {Name: "delete", Namespace: "orphanguard/widget"}, + {Key: "orphanguard.widget.delete"}, }, })) s.Require().NoError(err) From c28de61da077fc8811d38a0562188f24a031067e Mon Sep 17 00:00:00 2001 From: aman Date: Fri, 14 Aug 2026 16:11:40 +0530 Subject: [PATCH 2/2] refactor(api): validate permission key parts and name the rejection 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 --- internal/api/v1beta1connect/errors.go | 1 + internal/api/v1beta1connect/permission.go | 15 ++++++--- .../api/v1beta1connect/permission_test.go | 33 ++++++++++++++++--- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/internal/api/v1beta1connect/errors.go b/internal/api/v1beta1connect/errors.go index effd135b42..f8eddc17aa 100644 --- a/internal/api/v1beta1connect/errors.go +++ b/internal/api/v1beta1connect/errors.go @@ -56,6 +56,7 @@ var ( ErrAlreadyApplied = errors.New("credits already applied") ErrInvalidRoleID = errors.New("role id is invalid") ErrNamespaceSplitNotation = errors.New("subject/object should be provided as 'namespace:uuid'") + ErrPermissionKeyNotation = errors.New("permission key should be provided as 'service.resource.verb'") ErrPolicyNotFound = errors.New("policy doesn't exist") ErrProjectNotFound = errors.New("project doesn't exist") ErrGroupNotFound = errors.New("group doesn't exist") diff --git a/internal/api/v1beta1connect/permission.go b/internal/api/v1beta1connect/permission.go index e43f9dce1b..140e32639b 100644 --- a/internal/api/v1beta1connect/permission.go +++ b/internal/api/v1beta1connect/permission.go @@ -24,14 +24,13 @@ func (h *ConnectHandler) CreatePermission(ctx context.Context, request *connect. for _, permBody := range request.Msg.GetBodies() { permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(permBody.GetKey()) if permName == "" || permNamespace == "" { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) } if !schema.IsValidPermissionName(permName) { return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")) } - - if permNamespace == schema.DefaultNamespace { - return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("permission namespace cannot be "+schema.DefaultNamespace)) + if !schema.IsValidPermissionNamespace(permNamespace) { + return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) } permissionSlugs = append(permissionSlugs, schema.FQPermissionNameFromNamespace(permNamespace, permName)) @@ -113,7 +112,13 @@ func (h *ConnectHandler) UpdatePermission(ctx context.Context, request *connect. permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(request.Msg.GetBody().GetKey()) if permNamespace == "" || permName == "" { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) + } + if !schema.IsValidPermissionName(permName) { + return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")) + } + if !schema.IsValidPermissionNamespace(permNamespace) { + return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) } updatedPermission, err := h.permissionService.Update(ctx, permission.Permission{ ID: request.Msg.GetId(), diff --git a/internal/api/v1beta1connect/permission_test.go b/internal/api/v1beta1connect/permission_test.go index d283747f42..8670ac3950 100644 --- a/internal/api/v1beta1connect/permission_test.go +++ b/internal/api/v1beta1connect/permission_test.go @@ -91,7 +91,7 @@ func TestHandler_CreatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), }, { name: "should return bad request error if key is malformed", @@ -104,7 +104,20 @@ func TestHandler_CreatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), + }, + { + name: "should return bad request error if key namespace is invalid", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + { + Key: "app..get", + }, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), }, { name: "should return success if permission service return nil error", @@ -275,7 +288,7 @@ func TestHandler_UpdatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), }, { name: "should return bad request error if key is malformed", @@ -287,7 +300,19 @@ func TestHandler_UpdatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), + }, + { + name: "should return bad request error if key name has special characters", + setup: func(as *mocks.PermissionService) {}, + request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ + Id: testPermissions[testPermissionIdx].ID, + Body: &frontierv1beta1.PermissionRequestBody{ + Key: "app.resource.we$rd", + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")), }, { name: "should return success if permission service return nil error",