From e60f933b93aec06daabbce03802f3be05eea248f Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Thu, 4 Jul 2024 16:51:26 +0530 Subject: [PATCH 1/9] feat: make servicedata key public --- cmd/serve.go | 2 +- internal/adapter/relation.go | 21 ++++++++++++++++++- internal/schema/predefined.go | 7 ++++--- internal/store/postgres/role_repository.go | 3 ++- .../spicedb/schema_generator/generator.go | 5 +++-- test/integration_test/rest_test.go | 3 ++- 6 files changed, 32 insertions(+), 9 deletions(-) diff --git a/cmd/serve.go b/cmd/serve.go index c006e3ba4..042d61c80 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -243,7 +243,7 @@ func BuildAPIDependencies( serviceDataRepository := postgres.NewServiceDataRepository(dbc) serviceDataService := servicedata.NewService(logger, serviceDataRepository, resourceService, relationService, projectService, userService, activityService) - relationAdapter := adapter.NewRelation(groupService, userService, relationService) + relationAdapter := adapter.NewRelation(groupService, userService, relationService, roleService) dependencies := api.Deps{ OrgService: organizationService, diff --git a/internal/adapter/relation.go b/internal/adapter/relation.go index c222b9fb1..600cd33f0 100644 --- a/internal/adapter/relation.go +++ b/internal/adapter/relation.go @@ -2,30 +2,38 @@ package adapter import ( "context" + "errors" "fmt" + "slices" "github.com/goto/shield/core/group" "github.com/goto/shield/core/relation" + "github.com/goto/shield/core/role" "github.com/goto/shield/core/user" "github.com/goto/shield/internal/schema" "github.com/goto/shield/pkg/uuid" ) +const WILDCARD = "*" + type Relation struct { groupService *group.Service userService *user.Service relationService *relation.Service + roleService *role.Service } func NewRelation( groupService *group.Service, userService *user.Service, relationService *relation.Service, + roleService *role.Service, ) *Relation { return &Relation{ groupService: groupService, userService: userService, relationService: relationService, + roleService: roleService, } } @@ -36,7 +44,18 @@ func (a Relation) TransformRelation(ctx context.Context, rlt relation.RelationV2 if rel.Subject.Namespace == schema.UserPrincipal || rel.Subject.Namespace == "user" { userID := rel.Subject.ID - if !uuid.IsValid(userID) { + if userID == WILDCARD { + roleID := rel.Object.NamespaceID + ":" + rel.Subject.RoleID + role, err := a.roleService.Get(ctx, roleID) + if err != nil { + return relation.RelationV2{}, err + } + if !slices.Contains(role.Types, schema.UserPrincipalWildcard) { + return relation.RelationV2{}, errors.New("this does not allow wildcard") + } + } + + if !uuid.IsValid(userID) && userID != WILDCARD { fetchedUser, err := a.userService.GetByEmail(ctx, rel.Subject.ID) if err != nil { return relation.RelationV2{}, fmt.Errorf("%w: %s", relation.ErrFetchingUser, err.Error()) diff --git a/internal/schema/predefined.go b/internal/schema/predefined.go index b90cf6a07..6c29fb4d2 100644 --- a/internal/schema/predefined.go +++ b/internal/schema/predefined.go @@ -29,8 +29,9 @@ const ( MembershipPermission = "membership" // principals - UserPrincipal = "shield/user" - GroupPrincipal = "shield/group" + UserPrincipal = "shield/user" + GroupPrincipal = "shield/group" + UserPrincipalWildcard = "shield/user:*" ) var InheritedRelations = map[string]bool{ @@ -131,7 +132,7 @@ var ServiceDataKeyConfig = NamespaceConfig{ }, Roles: map[string][]string{ EditorRole: {UserPrincipal, GroupPrincipal}, - ViewerRole: {UserPrincipal, GroupPrincipal}, + ViewerRole: {UserPrincipal, GroupPrincipal, UserPrincipalWildcard}, OwnerRole: {UserPrincipal, GroupPrincipal}, }, Permissions: map[string][]string{ diff --git a/internal/store/postgres/role_repository.go b/internal/store/postgres/role_repository.go index e9a38e7ff..d0d5fe57b 100644 --- a/internal/store/postgres/role_repository.go +++ b/internal/store/postgres/role_repository.go @@ -116,7 +116,8 @@ func (r RoleRepository) Upsert(ctx context.Context, rl role.Role) (string, error "metadata": goqu.L("$5"), }).OnConflict( goqu.DoUpdate("id", goqu.Record{ - "name": goqu.L("$2"), + "types": goqu.L("$3"), + "metadata": goqu.L("$5"), }, )).Returning("id").ToSQL() if err != nil { diff --git a/internal/store/spicedb/schema_generator/generator.go b/internal/store/spicedb/schema_generator/generator.go index 7fb0ca6ab..4f1e6e35d 100644 --- a/internal/store/spicedb/schema_generator/generator.go +++ b/internal/store/spicedb/schema_generator/generator.go @@ -49,7 +49,8 @@ func GenerateSchema(namespaceConfig schema.NamespaceConfigMapType) []string { func processPrincipal(s string) string { return map[string]string{ - "shield/group": "shield/group#membership", - "shield/user": "shield/user", + "shield/group": "shield/group#membership", + "shield/user": "shield/user", + "shield/user:*": "shield/user:*", }[s] } diff --git a/test/integration_test/rest_test.go b/test/integration_test/rest_test.go index cf2292c2c..73f828691 100644 --- a/test/integration_test/rest_test.go +++ b/test/integration_test/rest_test.go @@ -15,6 +15,7 @@ import ( "github.com/goto/shield/core/project" "github.com/goto/shield/core/relation" "github.com/goto/shield/core/resource" + "github.com/goto/shield/core/role" "github.com/goto/shield/core/rule" "github.com/goto/shield/core/user" "github.com/goto/shield/internal/adapter" @@ -309,7 +310,7 @@ func buildPipeline(logger log.Logger, proxy http.Handler, ruleService *rule.Serv func hookPipeline(log log.Logger) hook.Service { rootHook := hook.New() - relationAdapter := adapter.NewRelation(&group.Service{}, &user.Service{}, &relation.Service{}) + relationAdapter := adapter.NewRelation(&group.Service{}, &user.Service{}, &relation.Service{}, &role.Service{}) return authz_hook.New(log, rootHook, rootHook, &resource.Service{}, &relation.Service{}, relationAdapter, "X-Auth-Email") } From 657f084f60cbe0a15db29d7443ee8c2907e67d40 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 11:46:03 +0530 Subject: [PATCH 2/9] fix: schema generator test --- internal/store/spicedb/schema_generator/generator_test.go | 2 +- internal/store/spicedb/schema_generator/predefined_schema | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/store/spicedb/schema_generator/generator_test.go b/internal/store/spicedb/schema_generator/generator_test.go index 87818c80b..102d53d70 100644 --- a/internal/store/spicedb/schema_generator/generator_test.go +++ b/internal/store/spicedb/schema_generator/generator_test.go @@ -33,5 +33,5 @@ func TestPredefinedSchema(t *testing.T) { schema.PreDefinedSystemNamespaceConfig[schema.ServiceDataKeyNamespace] = schema.ServiceDataKeyConfig actualPredefinedConfigs := makeDefnMap(GenerateSchema(schema.PreDefinedSystemNamespaceConfig)) expectedPredefinedConfigs := makeDefnMap(strings.Split(string(content), "\n--\n")) - assert.Equal(t, actualPredefinedConfigs, expectedPredefinedConfigs) + assert.Equal(t, expectedPredefinedConfigs, actualPredefinedConfigs) } diff --git a/internal/store/spicedb/schema_generator/predefined_schema b/internal/store/spicedb/schema_generator/predefined_schema index 63ba8020e..c2ae7cb96 100644 --- a/internal/store/spicedb/schema_generator/predefined_schema +++ b/internal/store/spicedb/schema_generator/predefined_schema @@ -30,7 +30,7 @@ definition shield/group { -- definition shield/servicedata_key { relation editor: shield/user | shield/group#membership - relation viewer: shield/user | shield/group#membership + relation viewer: shield/user | shield/group#membership | shield/user:* relation owner: shield/user | shield/group#membership permission edit = owner + editor + organization->owner + organization->editor + project->owner + project->editor permission view = owner + editor + viewer + organization->owner + organization->editor + organization->viewer + project->owner + project->editor + project->viewer From 970b5a1363e52bbee6219a97f5a775f0a4bf9184 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 11:58:48 +0530 Subject: [PATCH 3/9] test: spicedb image tab --- test/e2e_test/testbench/spicedb.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e_test/testbench/spicedb.go b/test/e2e_test/testbench/spicedb.go index 16789ff2a..bb5c32b6d 100644 --- a/test/e2e_test/testbench/spicedb.go +++ b/test/e2e_test/testbench/spicedb.go @@ -68,7 +68,7 @@ func migrateSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest func startSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest.Pool, pgConnString string, preSharedKey string) (extPort string, res *dockertest.Resource, err error) { res, err = pool.RunWithOptions(&dockertest.RunOptions{ Repository: "quay.io/authzed/spicedb", - Tag: "v1.0.0", + Tag: "v1.32.0", Cmd: []string{"spicedb", "serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--grpc-no-tls", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, ExposedPorts: []string{"50051/tcp"}, NetworkID: network.ID, From f3f55d4bd1e41a8e7a72f94a4f19db63c75ea846 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 12:02:52 +0530 Subject: [PATCH 4/9] test: fix image repo --- test/e2e_test/testbench/spicedb.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e_test/testbench/spicedb.go b/test/e2e_test/testbench/spicedb.go index bb5c32b6d..422e86191 100644 --- a/test/e2e_test/testbench/spicedb.go +++ b/test/e2e_test/testbench/spicedb.go @@ -67,7 +67,7 @@ func migrateSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest func startSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest.Pool, pgConnString string, preSharedKey string) (extPort string, res *dockertest.Resource, err error) { res, err = pool.RunWithOptions(&dockertest.RunOptions{ - Repository: "quay.io/authzed/spicedb", + Repository: "authzed/spicedb", Tag: "v1.32.0", Cmd: []string{"spicedb", "serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--grpc-no-tls", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, ExposedPorts: []string{"50051/tcp"}, From fc56cabb17b94b1f6b7fff941cf9b6da4e8fb537 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 12:35:46 +0530 Subject: [PATCH 5/9] test: fix image version --- .github/workflows/test.yml | 2 +- test/e2e_test/testbench/spicedb.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6d4882234..c7be8e608 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -36,7 +36,7 @@ jobs: ports: - "4000:80" spicedb: - image: quay.io/authzed/spicedb:v1.0.0 + image: authzed/spicedb:v1.32.0 ports: - "8080:8080" - "50051:50051" diff --git a/test/e2e_test/testbench/spicedb.go b/test/e2e_test/testbench/spicedb.go index 422e86191..550841e2c 100644 --- a/test/e2e_test/testbench/spicedb.go +++ b/test/e2e_test/testbench/spicedb.go @@ -14,8 +14,8 @@ import ( func migrateSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest.Pool, pgConnString string) error { resource, err := pool.RunWithOptions(&dockertest.RunOptions{ - Repository: "quay.io/authzed/spicedb", - Tag: "v1.0.0", + Repository: "authzed/spicedb", + Tag: "v1.32.0", Cmd: []string{"spicedb", "migrate", "head", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, NetworkID: network.ID, }, func(config *docker.HostConfig) { From 3bce0137412e950cc61b33a82eaeae4aad528ead Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 12:46:33 +0530 Subject: [PATCH 6/9] test: fix command --- test/e2e_test/testbench/spicedb.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e_test/testbench/spicedb.go b/test/e2e_test/testbench/spicedb.go index 550841e2c..80e85b250 100644 --- a/test/e2e_test/testbench/spicedb.go +++ b/test/e2e_test/testbench/spicedb.go @@ -16,7 +16,7 @@ func migrateSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest resource, err := pool.RunWithOptions(&dockertest.RunOptions{ Repository: "authzed/spicedb", Tag: "v1.32.0", - Cmd: []string{"spicedb", "migrate", "head", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, + Cmd: []string{"migrate", "head", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, NetworkID: network.ID, }, func(config *docker.HostConfig) { config.RestartPolicy = docker.RestartPolicy{ @@ -69,7 +69,7 @@ func startSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest.P res, err = pool.RunWithOptions(&dockertest.RunOptions{ Repository: "authzed/spicedb", Tag: "v1.32.0", - Cmd: []string{"spicedb", "serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--grpc-no-tls", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, + Cmd: []string{"serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--grpc-no-tls", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, ExposedPorts: []string{"50051/tcp"}, NetworkID: network.ID, }, func(config *docker.HostConfig) { From 652e968cb1988dec90d79d685e47bbca2f50a233 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 13:03:04 +0530 Subject: [PATCH 7/9] test: remove command flag --- test/e2e_test/testbench/spicedb.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e_test/testbench/spicedb.go b/test/e2e_test/testbench/spicedb.go index 80e85b250..1751a9beb 100644 --- a/test/e2e_test/testbench/spicedb.go +++ b/test/e2e_test/testbench/spicedb.go @@ -69,7 +69,7 @@ func startSpiceDB(logger log.Logger, network *docker.Network, pool *dockertest.P res, err = pool.RunWithOptions(&dockertest.RunOptions{ Repository: "authzed/spicedb", Tag: "v1.32.0", - Cmd: []string{"serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--grpc-no-tls", "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, + Cmd: []string{"serve", "--log-level", "debug", "--grpc-preshared-key", preSharedKey, "--datastore-engine", "postgres", "--datastore-conn-uri", pgConnString}, ExposedPorts: []string{"50051/tcp"}, NetworkID: network.ID, }, func(config *docker.HostConfig) { From f14932258dac20987665415abca9f620218ccc65 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Tue, 9 Jul 2024 14:03:39 +0530 Subject: [PATCH 8/9] test: add e2e-tests --- internal/adapter/relation.go | 9 +++----- test/e2e_test/smoke/api_test.go | 37 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/adapter/relation.go b/internal/adapter/relation.go index 600cd33f0..8abfcbfd2 100644 --- a/internal/adapter/relation.go +++ b/internal/adapter/relation.go @@ -2,7 +2,6 @@ package adapter import ( "context" - "errors" "fmt" "slices" @@ -48,14 +47,12 @@ func (a Relation) TransformRelation(ctx context.Context, rlt relation.RelationV2 roleID := rel.Object.NamespaceID + ":" + rel.Subject.RoleID role, err := a.roleService.Get(ctx, roleID) if err != nil { - return relation.RelationV2{}, err + return relation.RelationV2{}, fmt.Errorf("error fetching role: %s", err.Error()) } if !slices.Contains(role.Types, schema.UserPrincipalWildcard) { - return relation.RelationV2{}, errors.New("this does not allow wildcard") + return relation.RelationV2{}, fmt.Errorf("%s does not allow wildcard for subject %s", rlt.Object.NamespaceID, rlt.Subject.Namespace) } - } - - if !uuid.IsValid(userID) && userID != WILDCARD { + } else if !uuid.IsValid(userID) { fetchedUser, err := a.userService.GetByEmail(ctx, rel.Subject.ID) if err != nil { return relation.RelationV2{}, fmt.Errorf("%w: %s", relation.ErrFetchingUser, err.Error()) diff --git a/test/e2e_test/smoke/api_test.go b/test/e2e_test/smoke/api_test.go index c13992503..a0df9a38a 100644 --- a/test/e2e_test/smoke/api_test.go +++ b/test/e2e_test/smoke/api_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/goto/shield/config" + "github.com/goto/shield/internal/schema" shieldv1beta1 "github.com/goto/shield/proto/v1beta1" "github.com/goto/shield/test/e2e_test/testbench" "github.com/stretchr/testify/suite" @@ -91,6 +92,42 @@ func (s *EndToEndAPISmokeTestSuite) TestUserAPI() { }) } +func (s *EndToEndAPISmokeTestSuite) TestRelationsAPI() { + ctxOrgAdminAuth := metadata.NewOutgoingContext(context.Background(), metadata.New(map[string]string{ + testbench.IdentityHeader: testbench.OrgAdminEmail, + })) + + s.Run("1. should fail when trying to create wildcard relation", func() { + oRes, err := s.client.ListOrganizations(ctxOrgAdminAuth, &shieldv1beta1.ListOrganizationsRequest{}) + s.Require().NoError(err) + + _, err = s.client.CreateRelation(ctxOrgAdminAuth, &shieldv1beta1.CreateRelationRequest{ + Body: &shieldv1beta1.RelationRequestBody{ + ObjectId: oRes.Organizations[0].Id, + ObjectNamespace: schema.OrganizationNamespace, + Subject: schema.UserPrincipalWildcard, + RoleName: schema.OwnerRole, + }, + }) + s.Assert().Error(err) + }) + + s.Run("2. should allow relation creation with wildcard", func() { + res, err := s.client.ListResources(ctxOrgAdminAuth, &shieldv1beta1.ListResourcesRequest{}) + s.Require().NoError(err) + + _, err = s.client.CreateRelation(ctxOrgAdminAuth, &shieldv1beta1.CreateRelationRequest{ + Body: &shieldv1beta1.RelationRequestBody{ + ObjectId: res.Resources[0].Id, + ObjectNamespace: schema.ServiceDataKeyNamespace, + Subject: schema.UserPrincipalWildcard, + RoleName: schema.ViewerRole, + }, + }) + s.Assert().NoError(err) + }) +} + func TestEndToEndAPISmokeTestSuite(t *testing.T) { suite.Run(t, new(EndToEndAPISmokeTestSuite)) } From 1a370ee039d79b80f33f0536eb51a9d31f90d908 Mon Sep 17 00:00:00 2001 From: Ishan Arya Date: Wed, 10 Jul 2024 12:22:20 +0530 Subject: [PATCH 9/9] refactor: wildcard role check --- internal/adapter/relation.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/adapter/relation.go b/internal/adapter/relation.go index 8abfcbfd2..db3dd430c 100644 --- a/internal/adapter/relation.go +++ b/internal/adapter/relation.go @@ -44,13 +44,9 @@ func (a Relation) TransformRelation(ctx context.Context, rlt relation.RelationV2 userID := rel.Subject.ID if userID == WILDCARD { - roleID := rel.Object.NamespaceID + ":" + rel.Subject.RoleID - role, err := a.roleService.Get(ctx, roleID) + err := a.isWildCardAllowed(ctx, rel) if err != nil { - return relation.RelationV2{}, fmt.Errorf("error fetching role: %s", err.Error()) - } - if !slices.Contains(role.Types, schema.UserPrincipalWildcard) { - return relation.RelationV2{}, fmt.Errorf("%s does not allow wildcard for subject %s", rlt.Object.NamespaceID, rlt.Subject.Namespace) + return relation.RelationV2{}, err } } else if !uuid.IsValid(userID) { fetchedUser, err := a.userService.GetByEmail(ctx, rel.Subject.ID) @@ -95,3 +91,16 @@ func (a Relation) TransformRelation(ctx context.Context, rlt relation.RelationV2 return rel, nil } + +func (a Relation) isWildCardAllowed(ctx context.Context, rlt relation.RelationV2) error { + roleID := rlt.Object.NamespaceID + ":" + rlt.Subject.RoleID + role, err := a.roleService.Get(ctx, roleID) + if err != nil { + return fmt.Errorf("error fetching role: %s", err.Error()) + } + if !slices.Contains(role.Types, schema.UserPrincipalWildcard) { + return fmt.Errorf("%s does not allow wildcard for subject %s", rlt.Object.NamespaceID, rlt.Subject.Namespace) + } + + return nil +}