diff --git a/internal/cmd/import-test/prefixed-relations-validation-file.yaml b/internal/cmd/import-test/prefixed-relations-validation-file.yaml new file mode 100644 index 00000000..1012c9f5 --- /dev/null +++ b/internal/cmd/import-test/prefixed-relations-validation-file.yaml @@ -0,0 +1,3 @@ +relationships: |- + prefixtest/resource:doc1#viewer@prefixtest/user:alice + prefixtest/resource:doc2#viewer@prefixtest/user:bob[prefixtest/somecaveat] diff --git a/internal/cmd/import-test/prefixed-schema.zed b/internal/cmd/import-test/prefixed-schema.zed new file mode 100644 index 00000000..ab6d1920 --- /dev/null +++ b/internal/cmd/import-test/prefixed-schema.zed @@ -0,0 +1,10 @@ +definition prefixtest/user {} + +caveat prefixtest/somecaveat(allowed bool) { + allowed == true +} + +definition prefixtest/resource { + relation viewer: prefixtest/user | prefixtest/user with prefixtest/somecaveat + permission view = viewer +} diff --git a/internal/cmd/import.go b/internal/cmd/import.go index 8ce71fb9..2bec610e 100644 --- a/internal/cmd/import.go +++ b/internal/cmd/import.go @@ -118,6 +118,15 @@ func importSchema(ctx context.Context, client v1.SchemaServiceClient, schema str return nil } +// addPrefixIfAbsent prepends prefix to name unless name already carries a +// definition prefix (i.e. already contains a "/"). +func addPrefixIfAbsent(name, prefix string) string { + if strings.Contains(name, "/") { + return name + } + return fmt.Sprintf("%s/%s", prefix, name) +} + func importRelationships(ctx context.Context, client v1.PermissionsServiceClient, relationships string, definitionPrefix string, batchSize int, workers int) error { relationshipUpdates := make([]*v1.RelationshipUpdate, 0) scanner := bufio.NewScanner(strings.NewReader(relationships)) @@ -135,12 +144,15 @@ func importRelationships(ctx context.Context, client v1.PermissionsServiceClient } log.Trace().Str("line", line).Send() - // Rewrite the prefix on the references, if any. + // Rewrite the prefix on the references, if any. Only prefix names that + // are not already prefixed, so importing relationships that already + // carry the prefix (e.g. from a prefixed schemaFile) does not double it + // (`studio/x` becoming `studio/studio/x`). See issue #460. if len(definitionPrefix) > 0 { - rel.Resource.ObjectType = fmt.Sprintf("%s/%s", definitionPrefix, rel.Resource.ObjectType) - rel.Subject.Object.ObjectType = fmt.Sprintf("%s/%s", definitionPrefix, rel.Subject.Object.ObjectType) + rel.Resource.ObjectType = addPrefixIfAbsent(rel.Resource.ObjectType, definitionPrefix) + rel.Subject.Object.ObjectType = addPrefixIfAbsent(rel.Subject.Object.ObjectType, definitionPrefix) if rel.OptionalCaveat != nil { - rel.OptionalCaveat.CaveatName = fmt.Sprintf("%s/%s", definitionPrefix, rel.OptionalCaveat.CaveatName) + rel.OptionalCaveat.CaveatName = addPrefixIfAbsent(rel.OptionalCaveat.CaveatName, definitionPrefix) } } diff --git a/internal/cmd/import_test.go b/internal/cmd/import_test.go index c837ef7e..085ddb47 100644 --- a/internal/cmd/import_test.go +++ b/internal/cmd/import_test.go @@ -17,6 +17,17 @@ import ( var fullyConsistent = &v1.Consistency{Requirement: &v1.Consistency_FullyConsistent{FullyConsistent: true}} +func TestAddPrefixIfAbsent(t *testing.T) { + // Unprefixed names get the prefix. + require.Equal(t, "studio/permissions", addPrefixIfAbsent("permissions", "studio")) + require.Equal(t, "studio/user", addPrefixIfAbsent("user", "studio")) + // Already-prefixed names are left alone (regression for #460: importing + // relationships that already carry the prefix must not double it). + require.Equal(t, "studio/permissions", addPrefixIfAbsent("studio/permissions", "studio")) + // A different existing prefix is also preserved, not stacked. + require.Equal(t, "other/permissions", addPrefixIfAbsent("other/permissions", "studio")) +} + func TestImportCmd(t *testing.T) { testcases := map[string]struct { importSchema bool @@ -181,3 +192,75 @@ func TestImportCmdRelationsOnly(t *testing.T) { require.Contains(t, schemaResp.SchemaText, `permission view = user`) }) } + +// TestImportCmdRelationsOnlyAlreadyPrefixed is a regression test for #460: +// importing relationships that are already prefixed (e.g. from a prefixed +// schemaFile) with --schema=false must not double the auto-detected prefix +// (`prefixtest/resource` becoming `prefixtest/prefixtest/resource`). +func TestImportCmdRelationsOnlyAlreadyPrefixed(t *testing.T) { + cmd := zedtesting.CreateTestCobraCommandWithFlagValue( + t, + zedtesting.StringFlag{FlagName: "schema-definition-prefix"}, + zedtesting.BoolFlag{FlagName: "schema", FlagValue: false}, + zedtesting.BoolFlag{FlagName: "relationships", FlagValue: true}, + zedtesting.IntFlag{FlagName: "batch-size", FlagValue: 100}, + zedtesting.IntFlag{FlagName: "workers", FlagValue: 1}, + ) + + ctx := t.Context() + srv := zedtesting.NewTestServer(ctx, t) + go func() { + assert.NoError(t, srv.Run(ctx)) + }() + conn, err := srv.NewClient() + require.NoError(t, err) + t.Cleanup(func() { + conn.Close() + }) + + c, err := zedtesting.ClientFromConn(conn)(cmd) + require.NoError(t, err) + + // Write a prefixed schema so prefix auto-detection returns "prefixtest". + schemaBytes, err := os.ReadFile(filepath.Join("import-test", "prefixed-schema.zed")) + require.NoError(t, err) + _, err = c.WriteSchema(ctx, &v1.WriteSchemaRequest{Schema: string(schemaBytes)}) + require.NoError(t, err) + + // Mirror the import command: auto-detect the prefix from the running + // server, then import already-prefixed relationships with --schema=false. + prefix, err := determinePrefixForSchema(ctx, "", c, nil) + require.NoError(t, err) + require.Equal(t, "prefixtest", prefix) + + f := filepath.Join("import-test", "prefixed-relations-validation-file.yaml") + require.NoError(t, importCmdFunc(cmd, c, c, prefix, f)) + + // The relationship must land at prefixtest/resource, not the doubled + // prefixtest/prefixtest/resource (which would fail with FailedPrecondition). + resp, err := c.CheckPermission(ctx, &v1.CheckPermissionRequest{ + Consistency: fullyConsistent, + Subject: &v1.SubjectReference{Object: &v1.ObjectReference{ObjectType: "prefixtest/user", ObjectId: "alice"}}, + Permission: "view", + Resource: &v1.ObjectReference{ObjectType: "prefixtest/resource", ObjectId: "doc1"}, + }) + require.NoError(t, err) + require.Equal(t, v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, resp.Permissionship) + + // The already-prefixed caveat name must also be preserved (not doubled to + // prefixtest/prefixtest/somecaveat). With the caveat context satisfied, the + // caveated relationship grants the permission. + caveatedResp, err := c.CheckPermission(ctx, &v1.CheckPermissionRequest{ + Consistency: fullyConsistent, + Subject: &v1.SubjectReference{Object: &v1.ObjectReference{ObjectType: "prefixtest/user", ObjectId: "bob"}}, + Permission: "view", + Resource: &v1.ObjectReference{ObjectType: "prefixtest/resource", ObjectId: "doc2"}, + Context: func() *structpb.Struct { + s, serr := structpb.NewStruct(map[string]any{"allowed": true}) + require.NoError(t, serr) + return s + }(), + }) + require.NoError(t, err) + require.Equal(t, v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, caveatedResp.Permissionship) +}