From ecfb9d0d6de4975be908fa5b04ef0f177596f1ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Thu, 4 Jul 2024 19:18:43 +0300 Subject: [PATCH] perf(command): user grant pre-condition check using the search table (#8230) # Which Problems Are Solved Imporve the performance of user grant addition, especially for import. # How the Problems Are Solved Use the search table to query for the project grant state. This could easily be done by making the search used in `checkProjectGrantPreCondition` reusable. # Additional Changes Chanded event declerations to `const` in the `internal/repository/project` package. # Additional Context - Performance improvements for import are evaluated and acted upon internally at the moment --------- Co-authored-by: adlerhurst Co-authored-by: Livio Spring --- internal/api/grpc/feature/v2/converter.go | 4 + internal/command/project_grant.go | 36 +++--- internal/command/user_grant.go | 138 +++++++++++++++++++++ internal/domain/project_grant.go | 6 + internal/feature/feature.go | 1 + internal/repository/project/role.go | 2 +- pkg/grpc/protoc/v2/options.pb.go | 10 +- proto/zitadel/feature/v2beta/feature.proto | 1 + 8 files changed, 178 insertions(+), 20 deletions(-) diff --git a/internal/api/grpc/feature/v2/converter.go b/internal/api/grpc/feature/v2/converter.go index 15c43655a..b428d20d7 100644 --- a/internal/api/grpc/feature/v2/converter.go +++ b/internal/api/grpc/feature/v2/converter.go @@ -113,6 +113,8 @@ func improvedPerformanceTypeToPb(typ feature.ImprovedPerformanceType) feature_pb return feature_pb.ImprovedPerformance_IMPROVED_PERFORMANCE_PROJECT_GRANT case feature.ImprovedPerformanceTypeProject: return feature_pb.ImprovedPerformance_IMPROVED_PERFORMANCE_PROJECT + case feature.ImprovedPerformanceTypeUserGrant: + return feature_pb.ImprovedPerformance_IMPROVED_PERFORMANCE_USER_GRANT default: return feature_pb.ImprovedPerformance(typ) } @@ -141,6 +143,8 @@ func improvedPerformanceToDomain(typ feature_pb.ImprovedPerformance) feature.Imp return feature.ImprovedPerformanceTypeProjectGrant case feature_pb.ImprovedPerformance_IMPROVED_PERFORMANCE_PROJECT: return feature.ImprovedPerformanceTypeProject + case feature_pb.ImprovedPerformance_IMPROVED_PERFORMANCE_USER_GRANT: + return feature.ImprovedPerformanceTypeUserGrant default: return feature.ImprovedPerformanceTypeUnknown } diff --git a/internal/command/project_grant.go b/internal/command/project_grant.go index 59f98e376..ef602c4b3 100644 --- a/internal/command/project_grant.go +++ b/internal/command/project_grant.go @@ -259,38 +259,49 @@ func (c *Commands) checkProjectGrantPreCondition(ctx context.Context, projectGra if !authz.GetFeatures(ctx).ShouldUseImprovedPerformance(feature.ImprovedPerformanceTypeProjectGrant) { return c.checkProjectGrantPreConditionOld(ctx, projectGrant) } + existingRoleKeys, err := c.searchProjectGrantState(ctx, projectGrant.AggregateID, projectGrant.GrantedOrgID) + if err != nil { + return err + } + + if projectGrant.HasInvalidRoles(existingRoleKeys) { + return zerrors.ThrowPreconditionFailed(err, "COMMAND-6m9gd", "Errors.Project.Role.NotFound") + } + return nil +} + +func (c *Commands) searchProjectGrantState(ctx context.Context, projectID, grantedOrgID string) (existingRoleKeys []string, err error) { results, err := c.eventstore.Search( ctx, // project state query map[eventstore.FieldType]any{ eventstore.FieldTypeAggregateType: project.AggregateType, - eventstore.FieldTypeAggregateID: projectGrant.AggregateID, + eventstore.FieldTypeAggregateID: projectID, eventstore.FieldTypeFieldName: project.ProjectStateSearchField, eventstore.FieldTypeObjectType: project.ProjectSearchType, }, // granted org query map[eventstore.FieldType]any{ eventstore.FieldTypeAggregateType: org.AggregateType, - eventstore.FieldTypeAggregateID: projectGrant.GrantedOrgID, + eventstore.FieldTypeAggregateID: grantedOrgID, eventstore.FieldTypeFieldName: org.OrgStateSearchField, eventstore.FieldTypeObjectType: org.OrgSearchType, }, // role query map[eventstore.FieldType]any{ eventstore.FieldTypeAggregateType: project.AggregateType, - eventstore.FieldTypeAggregateID: projectGrant.AggregateID, + eventstore.FieldTypeAggregateID: projectID, eventstore.FieldTypeFieldName: project.ProjectRoleKeySearchField, eventstore.FieldTypeObjectType: project.ProjectRoleSearchType, }, ) if err != nil { - return err + return nil, err } var ( existsProject bool existsGrantedOrg bool - existingRoleKeys []string ) for _, result := range results { @@ -299,34 +310,31 @@ func (c *Commands) checkProjectGrantPreCondition(ctx context.Context, projectGra var role string err := result.Value.Unmarshal(&role) if err != nil { - return err + return nil, err } existingRoleKeys = append(existingRoleKeys, role) case org.OrgSearchType: var state domain.OrgState err := result.Value.Unmarshal(&state) if err != nil { - return err + return nil, err } existsGrantedOrg = state.Valid() && state != domain.OrgStateRemoved case project.ProjectSearchType: var state domain.ProjectState err := result.Value.Unmarshal(&state) if err != nil { - return err + return nil, err } existsProject = state.Valid() && state != domain.ProjectStateRemoved } } if !existsProject { - return zerrors.ThrowPreconditionFailed(err, "COMMAND-m9gsd", "Errors.Project.NotFound") + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-m9gsd", "Errors.Project.NotFound") } if !existsGrantedOrg { - return zerrors.ThrowPreconditionFailed(err, "COMMAND-3m9gg", "Errors.Org.NotFound") + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-3m9gg", "Errors.Org.NotFound") } - if projectGrant.HasInvalidRoles(existingRoleKeys) { - return zerrors.ThrowPreconditionFailed(err, "COMMAND-6m9gd", "Errors.Project.Role.NotFound") - } - return nil + return existingRoleKeys, nil } diff --git a/internal/command/user_grant.go b/internal/command/user_grant.go index b37571cd3..e17fece11 100644 --- a/internal/command/user_grant.go +++ b/internal/command/user_grant.go @@ -4,8 +4,12 @@ import ( "context" "reflect" + "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/feature" + "github.com/zitadel/zitadel/internal/repository/org" + "github.com/zitadel/zitadel/internal/repository/project" "github.com/zitadel/zitadel/internal/repository/usergrant" "github.com/zitadel/zitadel/internal/telemetry/tracing" "github.com/zitadel/zitadel/internal/zerrors" @@ -288,6 +292,140 @@ func (c *Commands) userGrantWriteModelByID(ctx context.Context, userGrantID, res } func (c *Commands) checkUserGrantPreCondition(ctx context.Context, usergrant *domain.UserGrant, resourceOwner string) (err error) { + if !authz.GetFeatures(ctx).ShouldUseImprovedPerformance(feature.ImprovedPerformanceTypeUserGrant) { + return c.checkUserGrantPreConditionOld(ctx, usergrant, resourceOwner) + } + + ctx, span := tracing.NewSpan(ctx) + defer func() { span.EndWithError(err) }() + + if err := c.checkUserExists(ctx, usergrant.UserID, ""); err != nil { + return err + } + existingRoleKeys, err := c.searchUserGrantPreConditionState(ctx, usergrant, resourceOwner) + if err != nil { + return err + } + if usergrant.HasInvalidRoles(existingRoleKeys) { + return zerrors.ThrowPreconditionFailed(err, "COMMAND-mm9F4", "Errors.Project.Role.NotFound") + } + return nil +} + +// this code needs to be rewritten anyways as soon as we improved the fields handling +// +//nolint:gocognit +func (c *Commands) searchUserGrantPreConditionState(ctx context.Context, userGrant *domain.UserGrant, resourceOwner string) (existingRoleKeys []string, err error) { + criteria := []map[eventstore.FieldType]any{ + // project state query + { + eventstore.FieldTypeAggregateType: project.AggregateType, + eventstore.FieldTypeAggregateID: userGrant.ProjectID, + eventstore.FieldTypeFieldName: project.ProjectStateSearchField, + eventstore.FieldTypeObjectType: project.ProjectSearchType, + }, + // granted org query + { + eventstore.FieldTypeAggregateType: org.AggregateType, + eventstore.FieldTypeAggregateID: resourceOwner, + eventstore.FieldTypeFieldName: org.OrgStateSearchField, + eventstore.FieldTypeObjectType: org.OrgSearchType, + }, + } + if userGrant.ProjectGrantID != "" { + criteria = append(criteria, map[eventstore.FieldType]any{ + eventstore.FieldTypeAggregateType: project.AggregateType, + eventstore.FieldTypeAggregateID: userGrant.ProjectID, + eventstore.FieldTypeObjectType: project.ProjectGrantSearchType, + eventstore.FieldTypeObjectID: userGrant.ProjectGrantID, + }) + } else { + criteria = append(criteria, map[eventstore.FieldType]any{ + eventstore.FieldTypeAggregateType: project.AggregateType, + eventstore.FieldTypeAggregateID: userGrant.ProjectID, + eventstore.FieldTypeObjectType: project.ProjectRoleSearchType, + eventstore.FieldTypeFieldName: project.ProjectRoleKeySearchField, + }) + } + results, err := c.eventstore.Search(ctx, criteria...) + if err != nil { + return nil, err + } + + var ( + existsProject bool + existsGrantedOrg bool + existsGrant bool + ) + + for _, result := range results { + switch result.Object.Type { + case project.ProjectRoleSearchType: + var role string + err := result.Value.Unmarshal(&role) + if err != nil { + return nil, err + } + existingRoleKeys = append(existingRoleKeys, role) + case org.OrgSearchType: + var state domain.OrgState + err := result.Value.Unmarshal(&state) + if err != nil { + return nil, err + } + existsGrantedOrg = state.Valid() && state != domain.OrgStateRemoved + case project.ProjectSearchType: + var state domain.ProjectState + err := result.Value.Unmarshal(&state) + if err != nil { + return nil, err + } + existsProject = state.Valid() && state != domain.ProjectStateRemoved + case project.ProjectGrantSearchType: + switch result.FieldName { + case project.ProjectGrantGrantedOrgIDSearchField: + var orgID string + err := result.Value.Unmarshal(&orgID) + if err != nil || orgID != resourceOwner { + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-3m9gg", "Errors.Org.NotFound") + } + case project.ProjectGrantStateSearchField: + var state domain.ProjectGrantState + err := result.Value.Unmarshal(&state) + if err != nil { + return nil, err + } + existsGrant = state.Valid() && state != domain.ProjectGrantStateRemoved + case project.ProjectGrantRoleKeySearchField: + var role string + err := result.Value.Unmarshal(&role) + if err != nil { + return nil, err + } + existingRoleKeys = append(existingRoleKeys, role) + case project.ProjectGrantGrantIDSearchField: + var grantID string + err := result.Value.Unmarshal(&grantID) + if err != nil || grantID != userGrant.ProjectGrantID { + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-huvKF", "Errors.Project.Grant.NotFound") + } + } + } + } + + if !existsProject { + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-m9gsd", "Errors.Project.NotFound") + } + if !existsGrantedOrg { + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-3m9gg", "Errors.Org.NotFound") + } + if userGrant.ProjectGrantID != "" && !existsGrant { + return nil, zerrors.ThrowPreconditionFailed(err, "COMMAND-huvKF", "Errors.Project.Grant.NotFound") + } + return existingRoleKeys, nil +} + +func (c *Commands) checkUserGrantPreConditionOld(ctx context.Context, usergrant *domain.UserGrant, resourceOwner string) (err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() diff --git a/internal/domain/project_grant.go b/internal/domain/project_grant.go index fecf38ec6..31c6d9edb 100644 --- a/internal/domain/project_grant.go +++ b/internal/domain/project_grant.go @@ -18,8 +18,14 @@ const ( ProjectGrantStateActive ProjectGrantStateInactive ProjectGrantStateRemoved + + projectGrantStateMax ) +func (s ProjectGrantState) Valid() bool { + return s > ProjectGrantStateUnspecified && s < projectGrantStateMax +} + func (p *ProjectGrant) IsValid() bool { return p.GrantedOrgID != "" } diff --git a/internal/feature/feature.go b/internal/feature/feature.go index d0f8a9f7a..395efa14f 100644 --- a/internal/feature/feature.go +++ b/internal/feature/feature.go @@ -44,6 +44,7 @@ const ( ImprovedPerformanceTypeOrgByID ImprovedPerformanceTypeProjectGrant ImprovedPerformanceTypeProject + ImprovedPerformanceTypeUserGrant ) func (f Features) ShouldUseImprovedPerformance(typ ImprovedPerformanceType) bool { diff --git a/internal/repository/project/role.go b/internal/repository/project/role.go index 2c7836da2..92d5f5198 100644 --- a/internal/repository/project/role.go +++ b/internal/repository/project/role.go @@ -8,7 +8,7 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) -var ( +const ( UniqueRoleType = "project_role" roleEventTypePrefix = projectEventTypePrefix + "role." RoleAddedType = roleEventTypePrefix + "added" diff --git a/pkg/grpc/protoc/v2/options.pb.go b/pkg/grpc/protoc/v2/options.pb.go index 890856e43..cdb10700c 100644 --- a/pkg/grpc/protoc/v2/options.pb.go +++ b/pkg/grpc/protoc/v2/options.pb.go @@ -1,6 +1,6 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.33.0 +// protoc-gen-go v1.34.2 // protoc (unknown) // source: zitadel/protoc_gen_zitadel/v2/options.proto @@ -250,7 +250,7 @@ func file_zitadel_protoc_gen_zitadel_v2_options_proto_rawDescGZIP() []byte { } var file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes = make([]protoimpl.MessageInfo, 3) -var file_zitadel_protoc_gen_zitadel_v2_options_proto_goTypes = []interface{}{ +var file_zitadel_protoc_gen_zitadel_v2_options_proto_goTypes = []any{ (*Options)(nil), // 0: zitadel.protoc_gen_zitadel.v2.Options (*AuthOption)(nil), // 1: zitadel.protoc_gen_zitadel.v2.AuthOption (*CustomHTTPResponse)(nil), // 2: zitadel.protoc_gen_zitadel.v2.CustomHTTPResponse @@ -274,7 +274,7 @@ func file_zitadel_protoc_gen_zitadel_v2_options_proto_init() { return } if !protoimpl.UnsafeEnabled { - file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[0].Exporter = func(v interface{}, i int) interface{} { + file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[0].Exporter = func(v any, i int) any { switch v := v.(*Options); i { case 0: return &v.state @@ -286,7 +286,7 @@ func file_zitadel_protoc_gen_zitadel_v2_options_proto_init() { return nil } } - file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[1].Exporter = func(v interface{}, i int) interface{} { + file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[1].Exporter = func(v any, i int) any { switch v := v.(*AuthOption); i { case 0: return &v.state @@ -298,7 +298,7 @@ func file_zitadel_protoc_gen_zitadel_v2_options_proto_init() { return nil } } - file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[2].Exporter = func(v interface{}, i int) interface{} { + file_zitadel_protoc_gen_zitadel_v2_options_proto_msgTypes[2].Exporter = func(v any, i int) any { switch v := v.(*CustomHTTPResponse); i { case 0: return &v.state diff --git a/proto/zitadel/feature/v2beta/feature.proto b/proto/zitadel/feature/v2beta/feature.proto index b000092fa..6e51eace5 100644 --- a/proto/zitadel/feature/v2beta/feature.proto +++ b/proto/zitadel/feature/v2beta/feature.proto @@ -59,4 +59,5 @@ enum ImprovedPerformance { // correctnes of data. IMPROVED_PERFORMANCE_PROJECT_GRANT = 2; IMPROVED_PERFORMANCE_PROJECT = 3; + IMPROVED_PERFORMANCE_USER_GRANT = 4; } \ No newline at end of file