diff --git a/cmd/permission.go b/cmd/permission.go index c6efbe3770..c3da193d48 100644 --- a/cmd/permission.go +++ b/cmd/permission.go @@ -5,6 +5,7 @@ import ( "os" "github.com/MakeNowJust/heredoc" + "github.com/raystack/frontier/internal/bootstrap/schema" "github.com/raystack/frontier/pkg/file" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "github.com/raystack/salt/cli/printer" @@ -186,12 +187,13 @@ func viewPermissionCommand(cliConfig *Config) *cli.Command { spinner.Stop() + permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(action.GetKey()) + report = append(report, []string{"ID", "NAME", "NAMESPACE"}) - //nolint:staticcheck report = append(report, []string{ action.GetId(), - action.GetName(), - action.GetNamespace(), + permName, + permNamespace, }) printer.Table(os.Stdout, report) @@ -248,12 +250,12 @@ func listPermissionCommand(cliConfig *Config) *cli.Command { fmt.Printf(" \nShowing %d permission(s)\n \n", len(permissions)) report = append(report, []string{"ID", "NAME", "NAMESPACE"}) - //nolint:staticcheck for _, a := range permissions { + permNamespace, permName := schema.PermissionNamespaceAndNameFromKey(a.GetKey()) report = append(report, []string{ a.GetId(), - a.GetName(), - a.GetNamespace(), + permName, + permNamespace, }) } printer.Table(os.Stdout, report) diff --git a/internal/api/v1beta1connect/permission.go b/internal/api/v1beta1connect/permission.go index 5cf79e3187..75498c3d7a 100644 --- a/internal/api/v1beta1connect/permission.go +++ b/internal/api/v1beta1connect/permission.go @@ -94,6 +94,15 @@ func transformPermissionToPB(perm permission.Permission) (*frontierv1beta1.Permi } } + // key is the replacement for the deprecated namespace/name fields, so it + // must read back to the exact stored pair. A row that cannot round-trip + // (namespace without a slash, or with a dot in a part) fails loudly here + // instead of returning a key that reads back as a different permission. + key := schema.PermissionKeyFromNamespaceAndName(perm.NamespaceID, perm.Name) + if ns, name := schema.PermissionNamespaceAndNameFromKey(key); ns != perm.NamespaceID || name != perm.Name { + return nil, fmt.Errorf("permission namespace %q and name %q do not round-trip through key %q", perm.NamespaceID, perm.Name, key) + } + return &frontierv1beta1.Permission{ Id: perm.ID, Name: perm.Name, @@ -101,7 +110,7 @@ func transformPermissionToPB(perm permission.Permission) (*frontierv1beta1.Permi UpdatedAt: timestamppb.New(perm.UpdatedAt), Namespace: perm.NamespaceID, Metadata: metadata, - Key: schema.PermissionKeyFromNamespaceAndName(perm.NamespaceID, perm.Name), + Key: key, }, nil } diff --git a/internal/api/v1beta1connect/permission_test.go b/internal/api/v1beta1connect/permission_test.go index 411aa40aef..3c8262da64 100644 --- a/internal/api/v1beta1connect/permission_test.go +++ b/internal/api/v1beta1connect/permission_test.go @@ -522,3 +522,24 @@ func TestHandler_GetPermission(t *testing.T) { }) } } + +func TestTransformPermissionToPB_KeyRoundTrip(t *testing.T) { + t.Run("a name with dots survives the key round trip", func(t *testing.T) { + got, err := transformPermissionToPB(permission.Permission{ + ID: "p1", + Name: "soft.delete", + NamespaceID: "database/instance", + }) + assert.NoError(t, err) + assert.Equal(t, "database.instance.soft.delete", got.GetKey()) + }) + + t.Run("a namespace without a slash fails instead of emitting a wrong key", func(t *testing.T) { + _, err := transformPermissionToPB(permission.Permission{ + ID: "p2", + Name: "get", + NamespaceID: "foo", + }) + assert.ErrorContains(t, err, "round-trip") + }) +} diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index e52ea067c7..612f76be92 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -275,9 +275,13 @@ func FQPermissionNameFromNamespace(namespace, verb string) string { return fmt.Sprintf("%s_%s_%s", service, resource, verb) } +// PermissionNamespaceAndNameFromKey splits a "service.resource.verb" key into +// its namespace ("service/resource") and name ("verb"). Namespace parts can +// never contain a dot, so the first two segments are always the namespace and +// everything after them is the name — a name with dots survives the split. func PermissionNamespaceAndNameFromKey(key string) (string, string) { - parts := strings.Split(key, ".") - if len(parts) != 3 { + parts := strings.SplitN(key, ".", 3) + if len(parts) != 3 || parts[0] == "" || parts[1] == "" || parts[2] == "" { return "", "" } return fmt.Sprintf("%s/%s", parts[0], parts[1]), parts[2] diff --git a/internal/bootstrap/schema/schema_test.go b/internal/bootstrap/schema/schema_test.go index 6b75057cd7..aa03d6af95 100644 --- a/internal/bootstrap/schema/schema_test.go +++ b/internal/bootstrap/schema/schema_test.go @@ -50,6 +50,53 @@ func TestFQPermissionNameFromNamespace(t *testing.T) { } } +func TestPermissionNamespaceAndNameFromKey(t *testing.T) { + tests := []struct { + key string + wantNamespace string + wantName string + }{ + {"compute.instance.delete", "compute/instance", "delete"}, + {"app.organization.get", "app/organization", "get"}, + // dots after the namespace belong to the name + {"compute.instance.soft.delete", "compute/instance", "soft.delete"}, + {"compute.instance", "", ""}, // too few parts + {"compute", "", ""}, + {"", "", ""}, + {".instance.delete", "", ""}, // empty service + {"compute..delete", "", ""}, // empty resource + {"compute.instance.", "", ""}, // empty name + } + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + ns, name := schema.PermissionNamespaceAndNameFromKey(tt.key) + if ns != tt.wantNamespace || name != tt.wantName { + t.Errorf("PermissionNamespaceAndNameFromKey(%q) = (%q, %q), want (%q, %q)", tt.key, ns, name, tt.wantNamespace, tt.wantName) + } + }) + } +} + +func TestPermissionKeyRoundTrip(t *testing.T) { + tests := []struct { + namespace string + name string + }{ + {"compute/instance", "delete"}, + {"app/organization", "get"}, + {"database/instance", "soft.delete"}, + } + for _, tt := range tests { + t.Run(tt.namespace+":"+tt.name, func(t *testing.T) { + key := schema.PermissionKeyFromNamespaceAndName(tt.namespace, tt.name) + ns, name := schema.PermissionNamespaceAndNameFromKey(key) + if ns != tt.namespace || name != tt.name { + t.Errorf("round trip through key %q = (%q, %q), want (%q, %q)", key, ns, name, tt.namespace, tt.name) + } + }) + } +} + func TestIsValidPermissionNamespace(t *testing.T) { tests := []struct { ns string diff --git a/internal/reconcile/permission_reconciler.go b/internal/reconcile/permission_reconciler.go index a5807e299b..7fd28fe59b 100644 --- a/internal/reconcile/permission_reconciler.go +++ b/internal/reconcile/permission_reconciler.go @@ -117,15 +117,18 @@ func (r *PermissionReconciler) fetchCurrent(ctx context.Context) ([]currentPermi return nil, fmt.Errorf("list permissions: %w", err) } var current []currentPermission - //nolint:staticcheck for _, p := range resp.Msg.GetPermissions() { - if isBaseNamespace(p.GetNamespace()) { + ns, name := schema.PermissionNamespaceAndNameFromKey(p.GetKey()) + if ns == "" || name == "" { + return nil, fmt.Errorf("permission %s: cannot split key %q into namespace and name", p.GetId(), p.GetKey()) + } + if isBaseNamespace(ns) { continue } current = append(current, currentPermission{ ID: p.GetId(), - Namespace: p.GetNamespace(), - Name: p.GetName(), + Namespace: ns, + Name: name, }) } return current, nil diff --git a/internal/reconcile/permission_reconciler_test.go b/internal/reconcile/permission_reconciler_test.go index 7e53c89289..625bae92df 100644 --- a/internal/reconcile/permission_reconciler_test.go +++ b/internal/reconcile/permission_reconciler_test.go @@ -5,6 +5,7 @@ import ( "testing" "connectrpc.com/connect" + "github.com/raystack/frontier/internal/bootstrap/schema" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "github.com/stretchr/testify/assert" ) @@ -30,7 +31,7 @@ func (f *fakePermissionAPI) DeletePermission(_ context.Context, req *connect.Req } func permissionPB(id, namespace, name string) *frontierv1beta1.Permission { - return &frontierv1beta1.Permission{Id: id, Namespace: namespace, Name: name} + return &frontierv1beta1.Permission{Id: id, Key: schema.PermissionKeyFromNamespaceAndName(namespace, name)} } func TestPermissionReconciler(t *testing.T) { @@ -54,6 +55,19 @@ func TestPermissionReconciler(t *testing.T) { assert.Equal(t, []string{"p1"}, api.deleted) }) + t.Run("a permission whose key does not split fails instead of being misread", func(t *testing.T) { + api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ + {Id: "p1", Key: "notakey"}, + }} + spec := []byte("- {namespace: compute/order, name: get}\n") + + _, err := NewPermissionReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.ErrorContains(t, err, "notakey") + assert.Empty(t, api.created) + assert.Empty(t, api.deleted) + }) + t.Run("dry-run plans without applying", func(t *testing.T) { api := &fakePermissionAPI{} spec := []byte("- {namespace: compute/order, name: get}\n") diff --git a/test/e2e/regression/service_registration_test.go b/test/e2e/regression/service_registration_test.go index 2c7c1c1032..65e89949d5 100644 --- a/test/e2e/regression/service_registration_test.go +++ b/test/e2e/regression/service_registration_test.go @@ -146,8 +146,7 @@ func (s *ServiceRegistrationRegressionTestSuite) TestServiceRegistration() { var lastPermCount int for _, perm := range []string{"get", "update", "delete"} { for _, listPerm := range listPermResp.Msg.GetPermissions() { - //nolint:staticcheck - if listPerm.GetName() == perm && listPerm.GetNamespace() == "database/instance" { + if listPerm.GetKey() == "database.instance."+perm { lastPermCount++ } } @@ -229,8 +228,7 @@ func (s *ServiceRegistrationRegressionTestSuite) TestPermissionDeleteCascade() { s.Require().NoError(err) var builtinID string for _, p := range listResp.Msg.GetPermissions() { - //nolint:staticcheck - if p.GetNamespace() == "app/organization" && p.GetName() == "get" { + if p.GetKey() == "app.organization.get" { builtinID = p.GetId() break }