From fa83c39510c04dc709c9717d9970ee172138f6a0 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Tue, 16 Sep 2025 14:26:21 +0200 Subject: [PATCH] fix: correct user self management on metadata and delete (#10666) # Which Problems Are Solved This PR fixes the self-management of users for metadata and own removal and improves the corresponding permission checks. While looking into the problems, I also noticed that there's a bug in the metadata mapping when using `api.metadata.push` in actions v1 and that re-adding a previously existing key after its removal was not possible. # How the Problems Are Solved - Added a parameter `allowSelfManagement` to checkPermissionOnUser to not require a permission if a user is changing its own data. - Updated use of `NewPermissionCheckUserWrite` including prevention of self-management for metadata. - Pass permission check to the command side (for metadata functions) to allow it implicitly for login v1 and actions v1. - Use of json.Marshal for the metadata mapping (as with `AppendMetadata`) - Check the metadata state when comparing the value. # Additional Changes - added a variadic `roles` parameter to the `CreateOrgMembership` integration test helper function to allow defining specific roles. # Additional Context - noted internally while testing v4.1.x - requires backport to v4.x - closes https://github.com/zitadel/zitadel/issues/10470 - relates to https://github.com/zitadel/zitadel/pull/10426 (cherry picked from commit 5329d505099396e2f0cdd6d38ddddd633c235244) --- internal/actions/object/metadata.go | 25 +- internal/actions/object/metadata_test.go | 122 +++++++++ internal/api/grpc/admin/import.go | 2 +- internal/api/grpc/management/user.go | 8 +- .../user/v2/integration_test/user_test.go | 48 ++++ internal/api/grpc/user/v2/key.go | 4 +- internal/api/grpc/user/v2/machine.go | 2 +- internal/api/grpc/user/v2/metadata.go | 4 +- internal/api/grpc/user/v2/pat.go | 4 +- internal/api/grpc/user/v2/secret.go | 4 +- .../user/v2beta/integration_test/user_test.go | 55 ++++ internal/api/oidc/userinfo.go | 6 +- internal/api/saml/storage.go | 6 +- internal/api/ui/login/metadata.go | 2 +- .../eventsourcing/eventstore/auth_request.go | 4 +- internal/command/permission_checks.go | 23 +- internal/command/permission_checks_test.go | 101 ++++++- internal/command/user_human_otp.go | 6 +- internal/command/user_human_password.go | 2 +- internal/command/user_human_webauthn.go | 2 +- internal/command/user_metadata.go | 36 ++- internal/command/user_metadata_test.go | 257 +++++++++++++++--- internal/command/user_v2.go | 8 +- internal/command/user_v2_email.go | 4 +- internal/command/user_v2_human.go | 9 +- internal/command/user_v2_machine.go | 2 +- internal/command/user_v2_password.go | 2 +- internal/command/user_v2_phone.go | 4 +- internal/command/user_v2_test.go | 142 +++++----- internal/domain/permission.go | 1 + internal/integration/client.go | 8 +- 31 files changed, 695 insertions(+), 208 deletions(-) create mode 100644 internal/actions/object/metadata_test.go diff --git a/internal/actions/object/metadata.go b/internal/actions/object/metadata.go index a2d1f66a936..2a80d19f87d 100644 --- a/internal/actions/object/metadata.go +++ b/internal/actions/object/metadata.go @@ -204,7 +204,12 @@ func MetadataListToDomain(metadataList *MetadataList) []*domain.Metadata { for i, metadata := range metadataList.metadata { value := metadata.value if len(value) == 0 { - value = mapBytesToByteArray(metadata.Value.Export()) + var err error + value, err = json.Marshal(metadata.Value.Export()) + if err != nil { + logging.WithError(err).Debug("unable to marshal") + panic(err) + } } list[i] = &domain.Metadata{ Key: metadata.Key, @@ -214,21 +219,3 @@ func MetadataListToDomain(metadataList *MetadataList) []*domain.Metadata { return list } - -// mapBytesToByteArray is used for backwards compatibility of old metadata.push method -// converts the Javascript uint8 array which is exported as []interface{} to a []byte -func mapBytesToByteArray(i interface{}) []byte { - bytes, ok := i.([]interface{}) - if !ok { - return nil - } - value := make([]byte, len(bytes)) - for i, val := range bytes { - b, ok := val.(int64) - if !ok { - return nil - } - value[i] = byte(b) - } - return value -} diff --git a/internal/actions/object/metadata_test.go b/internal/actions/object/metadata_test.go new file mode 100644 index 00000000000..80dccfe46fd --- /dev/null +++ b/internal/actions/object/metadata_test.go @@ -0,0 +1,122 @@ +package object + +import ( + "testing" + + "github.com/dop251/goja" + "github.com/stretchr/testify/assert" + + "github.com/zitadel/zitadel/internal/domain" +) + +func TestMetadataListToDomain(t *testing.T) { + type args struct { + metadataList *MetadataList + } + tests := []struct { + name string + args args + want []*domain.Metadata + }{ + { + name: "nil", + args: args{metadataList: nil}, + want: nil, + }, + { + name: "empty", + args: args{metadataList: &MetadataList{}}, + want: []*domain.Metadata{}, + }, + { + name: "from mapped value", + args: args{metadataList: &MetadataList{ + metadata: []*Metadata{ + { + Key: "key1", + value: []byte("value1"), + }, + }, + }}, + want: []*domain.Metadata{ + { + Key: "key1", + Value: []byte("value1"), + }, + }, + }, + { + name: "from goja value string", + args: args{metadataList: &MetadataList{ + metadata: []*Metadata{ + { + Key: "key1", + Value: (&goja.Runtime{}).ToValue("value1"), + }, + }, + }}, + want: []*domain.Metadata{ + { + Key: "key1", + Value: []byte(`"value1"`), + }, + }, + }, + { + name: "from goja value int", + args: args{metadataList: &MetadataList{ + metadata: []*Metadata{ + { + Key: "key1", + Value: (&goja.Runtime{}).ToValue(1), + }, + }, + }}, + want: []*domain.Metadata{ + { + Key: "key1", + Value: []byte("1"), + }, + }, + }, + { + name: "from goja value float", + args: args{metadataList: &MetadataList{ + metadata: []*Metadata{ + { + Key: "key1", + Value: (&goja.Runtime{}).ToValue(1.2), + }, + }, + }}, + want: []*domain.Metadata{ + { + Key: "key1", + Value: []byte("1.2"), + }, + }, + }, + { + name: "from goja value bool", + args: args{metadataList: &MetadataList{ + metadata: []*Metadata{ + { + Key: "key1", + Value: (&goja.Runtime{}).ToValue(true), + }, + }, + }}, + want: []*domain.Metadata{ + { + Key: "key1", + Value: []byte("true"), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, MetadataListToDomain(tt.args.metadataList)) + }) + } +} diff --git a/internal/api/grpc/admin/import.go b/internal/api/grpc/admin/import.go index c2c8c748dc0..67802777e92 100644 --- a/internal/api/grpc/admin/import.go +++ b/internal/api/grpc/admin/import.go @@ -542,7 +542,7 @@ func importUserMetadata(ctx context.Context, s *Server, errors *[]*admin_pb.Impo } for _, userMetadata := range org.GetUserMetadata() { logging.Debugf("import usermetadata: %s", userMetadata.GetId()+"_"+userMetadata.GetKey()) - _, err := s.command.SetUserMetadata(ctx, &domain.Metadata{Key: userMetadata.GetKey(), Value: userMetadata.GetValue()}, userMetadata.GetId(), org.GetOrgId()) + _, err := s.command.SetUserMetadata(ctx, &domain.Metadata{Key: userMetadata.GetKey(), Value: userMetadata.GetValue()}, userMetadata.GetId(), org.GetOrgId(), nil) if err != nil { *errors = append(*errors, &admin_pb.ImportDataError{Type: "user_metadata", Id: userMetadata.GetId() + "_" + userMetadata.GetKey(), Message: errorToImportError(err)}) if isCtxTimeout(ctx) { diff --git a/internal/api/grpc/management/user.go b/internal/api/grpc/management/user.go index f40a29868ed..440213ae455 100644 --- a/internal/api/grpc/management/user.go +++ b/internal/api/grpc/management/user.go @@ -168,7 +168,7 @@ func (s *Server) GetUserMetadata(ctx context.Context, req *mgmt_pb.GetUserMetada func (s *Server) SetUserMetadata(ctx context.Context, req *mgmt_pb.SetUserMetadataRequest) (*mgmt_pb.SetUserMetadataResponse, error) { ctxData := authz.GetCtxData(ctx) - result, err := s.command.SetUserMetadata(ctx, &domain.Metadata{Key: req.Key, Value: req.Value}, req.Id, ctxData.OrgID) + result, err := s.command.SetUserMetadata(ctx, &domain.Metadata{Key: req.Key, Value: req.Value}, req.Id, ctxData.OrgID, nil) if err != nil { return nil, err } @@ -183,7 +183,7 @@ func (s *Server) SetUserMetadata(ctx context.Context, req *mgmt_pb.SetUserMetada func (s *Server) BulkSetUserMetadata(ctx context.Context, req *mgmt_pb.BulkSetUserMetadataRequest) (*mgmt_pb.BulkSetUserMetadataResponse, error) { ctxData := authz.GetCtxData(ctx) - result, err := s.command.BulkSetUserMetadata(ctx, req.Id, ctxData.OrgID, BulkSetUserMetadataToDomain(req)...) + result, err := s.command.BulkSetUserMetadata(ctx, req.Id, ctxData.OrgID, nil, BulkSetUserMetadataToDomain(req)...) // permission has already been checked if err != nil { return nil, err } @@ -194,7 +194,7 @@ func (s *Server) BulkSetUserMetadata(ctx context.Context, req *mgmt_pb.BulkSetUs func (s *Server) RemoveUserMetadata(ctx context.Context, req *mgmt_pb.RemoveUserMetadataRequest) (*mgmt_pb.RemoveUserMetadataResponse, error) { ctxData := authz.GetCtxData(ctx) - result, err := s.command.RemoveUserMetadata(ctx, req.Key, req.Id, ctxData.OrgID) + result, err := s.command.RemoveUserMetadata(ctx, req.Key, req.Id, ctxData.OrgID, nil) if err != nil { return nil, err } @@ -205,7 +205,7 @@ func (s *Server) RemoveUserMetadata(ctx context.Context, req *mgmt_pb.RemoveUser func (s *Server) BulkRemoveUserMetadata(ctx context.Context, req *mgmt_pb.BulkRemoveUserMetadataRequest) (*mgmt_pb.BulkRemoveUserMetadataResponse, error) { ctxData := authz.GetCtxData(ctx) - result, err := s.command.BulkRemoveUserMetadata(ctx, req.Id, ctxData.OrgID, req.Keys...) + result, err := s.command.BulkRemoveUserMetadata(ctx, req.Id, ctxData.OrgID, nil, req.Keys...) if err != nil { return nil, err } diff --git a/internal/api/grpc/user/v2/integration_test/user_test.go b/internal/api/grpc/user/v2/integration_test/user_test.go index 125cf41972f..0a66f3704ce 100644 --- a/internal/api/grpc/user/v2/integration_test/user_test.go +++ b/internal/api/grpc/user/v2/integration_test/user_test.go @@ -1344,6 +1344,17 @@ func TestServer_LockUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.LockUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.LockUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "lock, ok", args: args{ @@ -1452,6 +1463,17 @@ func TestServer_UnLockUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.UnlockUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.UnlockUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "unlock, not locked", args: args{ @@ -1560,6 +1582,17 @@ func TestServer_DeactivateUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.DeactivateUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.DeactivateUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "deactivate, ok", args: args{ @@ -1669,6 +1702,17 @@ func TestServer_ReactivateUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.ReactivateUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.ReactivateUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "reactivate, not deactivated", args: args{ @@ -1852,6 +1896,10 @@ func TestServer_DeleteUser(t *testing.T) { }, }) require.NoError(t, err) + + // allow self management incl. deletion + Instance.CreateOrgMembership(t, CTX, Instance.DefaultOrg.Id, removeUser.Id, "ORG_USER_SELF_MANAGER") + request.UserId = removeUser.Id Instance.RegisterUserPasskey(CTX, removeUser.Id) token := createVerifiedWebAuthNSession(LoginCTX, t, removeUser.Id) diff --git a/internal/api/grpc/user/v2/key.go b/internal/api/grpc/user/v2/key.go index 021f4be3884..243f163cad8 100644 --- a/internal/api/grpc/user/v2/key.go +++ b/internal/api/grpc/user/v2/key.go @@ -19,7 +19,7 @@ func (s *Server) AddKey(ctx context.Context, req *connect.Request[user.AddKeyReq }, ExpirationDate: req.Msg.GetExpirationDate().AsTime(), Type: domain.AuthNKeyTypeJSON, - PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx), + PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx, true), } newMachineKey.PublicKey = req.Msg.GetPublicKey() @@ -50,7 +50,7 @@ func (s *Server) RemoveKey(ctx context.Context, req *connect.Request[user.Remove ObjectRoot: models.ObjectRoot{ AggregateID: req.Msg.GetUserId(), }, - PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx), + PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx, true), KeyID: req.Msg.GetKeyId(), } objectDetails, err := s.command.RemoveUserMachineKey(ctx, machineKey) diff --git a/internal/api/grpc/user/v2/machine.go b/internal/api/grpc/user/v2/machine.go index e5126b90196..7889d2c6dcd 100644 --- a/internal/api/grpc/user/v2/machine.go +++ b/internal/api/grpc/user/v2/machine.go @@ -27,7 +27,7 @@ func (s *Server) createUserTypeMachine(ctx context.Context, machinePb *user.Crea ctx, cmd, nil, - s.command.NewPermissionCheckUserWrite(ctx), + s.command.NewPermissionCheckUserWrite(ctx, true), command.AddMachineWithUsernameToIDFallback(), ) if err != nil { diff --git a/internal/api/grpc/user/v2/metadata.go b/internal/api/grpc/user/v2/metadata.go index 338ce9fc45b..c7ce97e38e9 100644 --- a/internal/api/grpc/user/v2/metadata.go +++ b/internal/api/grpc/user/v2/metadata.go @@ -49,7 +49,7 @@ func (s *Server) listUserMetadataRequestToModel(req *user.ListUserMetadataReques } func (s *Server) SetUserMetadata(ctx context.Context, req *connect.Request[user.SetUserMetadataRequest]) (*connect.Response[user.SetUserMetadataResponse], error) { - result, err := s.command.BulkSetUserMetadata(ctx, req.Msg.UserId, "", setUserMetadataToDomain(req.Msg)...) + result, err := s.command.BulkSetUserMetadata(ctx, req.Msg.UserId, "", s.command.NewPermissionCheckUserWrite(ctx, false), setUserMetadataToDomain(req.Msg)...) if err != nil { return nil, err } @@ -70,7 +70,7 @@ func setUserMetadataToDomain(req *user.SetUserMetadataRequest) []*domain.Metadat } func (s *Server) DeleteUserMetadata(ctx context.Context, req *connect.Request[user.DeleteUserMetadataRequest]) (*connect.Response[user.DeleteUserMetadataResponse], error) { - result, err := s.command.BulkRemoveUserMetadata(ctx, req.Msg.UserId, "", req.Msg.Keys...) + result, err := s.command.BulkRemoveUserMetadata(ctx, req.Msg.UserId, "", s.command.NewPermissionCheckUserWrite(ctx, false), req.Msg.Keys...) if err != nil { return nil, err } diff --git a/internal/api/grpc/user/v2/pat.go b/internal/api/grpc/user/v2/pat.go index 0c90eeaebdf..36e8201362e 100644 --- a/internal/api/grpc/user/v2/pat.go +++ b/internal/api/grpc/user/v2/pat.go @@ -19,7 +19,7 @@ func (s *Server) AddPersonalAccessToken(ctx context.Context, req *connect.Reques ObjectRoot: models.ObjectRoot{ AggregateID: req.Msg.GetUserId(), }, - PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx), + PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx, true), ExpirationDate: req.Msg.GetExpirationDate().AsTime(), Scopes: []string{ oidc.ScopeOpenID, @@ -46,7 +46,7 @@ func (s *Server) RemovePersonalAccessToken(ctx context.Context, req *connect.Req ObjectRoot: models.ObjectRoot{ AggregateID: req.Msg.GetUserId(), }, - PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx), + PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx, true), }) if err != nil { return nil, err diff --git a/internal/api/grpc/user/v2/secret.go b/internal/api/grpc/user/v2/secret.go index acc7aef8cb3..6ab70089314 100644 --- a/internal/api/grpc/user/v2/secret.go +++ b/internal/api/grpc/user/v2/secret.go @@ -12,7 +12,7 @@ import ( func (s *Server) AddSecret(ctx context.Context, req *connect.Request[user.AddSecretRequest]) (*connect.Response[user.AddSecretResponse], error) { newSecret := &command.GenerateMachineSecret{ - PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx), + PermissionCheck: s.command.NewPermissionCheckUserWrite(ctx, true), } details, err := s.command.GenerateMachineSecret(ctx, req.Msg.GetUserId(), "", newSecret) if err != nil { @@ -29,7 +29,7 @@ func (s *Server) RemoveSecret(ctx context.Context, req *connect.Request[user.Rem ctx, req.Msg.GetUserId(), "", - s.command.NewPermissionCheckUserWrite(ctx), + s.command.NewPermissionCheckUserWrite(ctx, true), ) if err != nil { return nil, err diff --git a/internal/api/grpc/user/v2beta/integration_test/user_test.go b/internal/api/grpc/user/v2beta/integration_test/user_test.go index b655ae791fe..3b90759e984 100644 --- a/internal/api/grpc/user/v2beta/integration_test/user_test.go +++ b/internal/api/grpc/user/v2beta/integration_test/user_test.go @@ -1297,6 +1297,17 @@ func TestServer_LockUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.LockUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.LockUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "lock, ok", args: args{ @@ -1405,6 +1416,17 @@ func TestServer_UnLockUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.UnlockUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.UnlockUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "unlock, not locked", args: args{ @@ -1513,6 +1535,17 @@ func TestServer_DeactivateUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.DeactivateUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.DeactivateUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "deactivate, ok", args: args{ @@ -1621,6 +1654,17 @@ func TestServer_ReactivateUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.ReactivateUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.ReactivateUserRequest) error { return nil }, + }, + wantErr: true, + }, { name: "reactivate, not deactivated", args: args{ @@ -1730,6 +1774,17 @@ func TestServer_DeleteUser(t *testing.T) { }, wantErr: true, }, + { + name: "no permission, error", + args: args{ + UserCTX, + &user.DeleteUserRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + }, + func(request *user.DeleteUserRequest) {}, + }, + wantErr: true, + }, { name: "remove human, ok", args: args{ diff --git a/internal/api/oidc/userinfo.go b/internal/api/oidc/userinfo.go index 08ee4da3eb0..15ec5b21e02 100644 --- a/internal/api/oidc/userinfo.go +++ b/internal/api/oidc/userinfo.go @@ -292,8 +292,6 @@ func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, user ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - userCtx := authz.SetCtxData(ctx, authz.CtxData{UserID: userInfo.Subject, ResourceOwner: qu.User.ResourceOwner}) - queriedActions, err := s.query.GetActiveActionsByFlowAndTriggerType(ctx, domain.FlowTypeCustomiseToken, triggerType, qu.User.ResourceOwner) if err != nil { return err @@ -388,7 +386,7 @@ func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, user Key: key, Value: value, } - if _, err = s.command.SetUserMetadata(userCtx, metadata, userInfo.Subject, qu.User.ResourceOwner); err != nil { + if _, err = s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil { logging.WithError(err).Info("unable to set md in action") panic(err) } @@ -451,7 +449,7 @@ func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, user } claimLogs := make([]string, 0) for _, metadata := range contextInfoResponse.SetUserMetadata { - if _, err = s.command.SetUserMetadata(userCtx, metadata, userInfo.Subject, qu.User.ResourceOwner); err != nil { + if _, err = s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil { claimLogs = append(claimLogs, fmt.Sprintf("failed to set user metadata key %q", metadata.Key)) } } diff --git a/internal/api/saml/storage.go b/internal/api/saml/storage.go index b89fb69f9bc..94288142ccc 100644 --- a/internal/api/saml/storage.go +++ b/internal/api/saml/storage.go @@ -285,8 +285,6 @@ func setUserinfo(user *query.User, userinfo models.AttributeSetter, attributes [ } func (p *Storage) getCustomAttributes(ctx context.Context, user *query.User, userGrants *query.UserGrants) (map[string]*customAttribute, error) { - userCtx := authz.SetCtxData(ctx, authz.CtxData{UserID: user.ID, ResourceOwner: user.ResourceOwner}) - customAttributes := make(map[string]*customAttribute, 0) queriedActions, err := p.query.GetActiveActionsByFlowAndTriggerType(ctx, domain.FlowTypeCustomizeSAMLResponse, domain.TriggerTypePreSAMLResponseCreation, user.ResourceOwner) if err != nil { @@ -366,7 +364,7 @@ func (p *Storage) getCustomAttributes(ctx context.Context, user *query.User, use Key: key, Value: value, } - if _, err = p.command.SetUserMetadata(userCtx, metadata, user.ID, user.ResourceOwner); err != nil { + if _, err = p.command.SetUserMetadata(ctx, metadata, user.ID, user.ResourceOwner, nil); err != nil { logging.WithError(err).Info("unable to set md in action") panic(err) } @@ -413,7 +411,7 @@ func (p *Storage) getCustomAttributes(ctx context.Context, user *query.User, use } attributeLogs := make([]string, 0) for _, metadata := range contextInfoResponse.SetUserMetadata { - if _, err = p.command.SetUserMetadata(userCtx, metadata, user.ID, user.ResourceOwner); err != nil { + if _, err = p.command.SetUserMetadata(ctx, metadata, user.ID, user.ResourceOwner, nil); err != nil { attributeLogs = append(attributeLogs, fmt.Sprintf("failed to set user metadata key %q", metadata.Key)) } } diff --git a/internal/api/ui/login/metadata.go b/internal/api/ui/login/metadata.go index b645c9f4af0..ab58390c810 100644 --- a/internal/api/ui/login/metadata.go +++ b/internal/api/ui/login/metadata.go @@ -10,6 +10,6 @@ import ( func (l *Login) bulkSetUserMetadata(ctx context.Context, userID, orgID string, metadata []*domain.Metadata) error { // user context necessary due to permission check in command userCtx := authz.SetCtxData(ctx, authz.CtxData{UserID: userID, OrgID: orgID}) - _, err := l.command.BulkSetUserMetadata(userCtx, userID, orgID, metadata...) + _, err := l.command.BulkSetUserMetadata(userCtx, userID, orgID, nil, metadata...) return err } diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index ea260eeb1b8..d2e2bc62fb1 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -611,9 +611,7 @@ func (repo *AuthRequestRepo) AutoRegisterExternalUser(ctx context.Context, regis return err } if len(metadatas) > 0 { - // user context necessary due to permission check in command - userCtx := authz.SetCtxData(ctx, authz.CtxData{UserID: request.UserID, OrgID: request.UserOrgID}) - _, err := repo.Command.BulkSetUserMetadata(userCtx, request.UserID, request.UserOrgID, metadatas...) + _, err := repo.Command.BulkSetUserMetadata(ctx, request.UserID, request.UserOrgID, nil, metadatas...) if err != nil { return err } diff --git a/internal/command/permission_checks.go b/internal/command/permission_checks.go index 97bae79ba99..021107e6afa 100644 --- a/internal/command/permission_checks.go +++ b/internal/command/permission_checks.go @@ -39,29 +39,36 @@ func (c *Commands) newPermissionCheck(ctx context.Context, permission string, ag } } -func (c *Commands) checkPermissionOnUser(ctx context.Context, permission string) PermissionCheck { +func (c *Commands) checkPermissionOnUser(ctx context.Context, permission string, allowSelfManagement bool) PermissionCheck { return func(resourceOwner, aggregateID string) error { - if aggregateID != "" && aggregateID == authz.GetCtxData(ctx).UserID { + if allowSelfManagement && aggregateID != "" && aggregateID == authz.GetCtxData(ctx).UserID { return nil } return c.newPermissionCheck(ctx, permission, user.AggregateType)(resourceOwner, aggregateID) } } -func (c *Commands) NewPermissionCheckUserWrite(ctx context.Context) PermissionCheck { - return c.checkPermissionOnUser(ctx, domain.PermissionUserWrite) +func (c *Commands) NewPermissionCheckUserWrite(ctx context.Context, allowSelfManagement bool) PermissionCheck { + return c.checkPermissionOnUser(ctx, domain.PermissionUserWrite, allowSelfManagement) } func (c *Commands) checkPermissionDeleteUser(ctx context.Context, resourceOwner, userID string) error { - return c.checkPermissionOnUser(ctx, domain.PermissionUserDelete)(resourceOwner, userID) + err := c.checkPermissionOnUser(ctx, domain.PermissionUserDelete, false)(resourceOwner, userID) + if err == nil { + return nil + } + if userID != authz.GetCtxData(ctx).UserID { + return err + } + return c.checkPermissionOnUser(ctx, domain.PermissionUserDeleteSelf, false)(resourceOwner, userID) } -func (c *Commands) checkPermissionUpdateUser(ctx context.Context, resourceOwner, userID string) error { - return c.NewPermissionCheckUserWrite(ctx)(resourceOwner, userID) +func (c *Commands) checkPermissionUpdateUser(ctx context.Context, resourceOwner, userID string, allowSelfManagement bool) error { + return c.NewPermissionCheckUserWrite(ctx, allowSelfManagement)(resourceOwner, userID) } func (c *Commands) checkPermissionUpdateUserCredentials(ctx context.Context, resourceOwner, userID string) error { - return c.checkPermissionOnUser(ctx, domain.PermissionUserCredentialWrite)(resourceOwner, userID) + return c.checkPermissionOnUser(ctx, domain.PermissionUserCredentialWrite, true)(resourceOwner, userID) } func (c *Commands) checkPermissionCreateProject(ctx context.Context, resourceOwner, projectID string) error { diff --git a/internal/command/permission_checks_test.go b/internal/command/permission_checks_test.go index 5c36dc14f99..5afec753adf 100644 --- a/internal/command/permission_checks_test.go +++ b/internal/command/permission_checks_test.go @@ -147,6 +147,7 @@ func TestCommands_CheckPermissionUserWrite(t *testing.T) { type args struct { ctx context.Context resourceOwner, aggregateID string + allowSelfManagement bool } type want struct { err func(error) bool @@ -163,8 +164,29 @@ func TestCommands_CheckPermissionUserWrite(t *testing.T) { ctx: authz.SetCtxData(context.Background(), authz.CtxData{ UserID: "aggregateID", }), - resourceOwner: "resourceOwner", - aggregateID: "aggregateID", + resourceOwner: "resourceOwner", + aggregateID: "aggregateID", + allowSelfManagement: true, + }, + }, + { + name: "self, no selfManagementAllowed, permission check", + fields: fields{ + domainPermissionCheck: mockDomainPermissionCheck( + authz.SetCtxData(context.Background(), authz.CtxData{ + UserID: "aggregateID", + }), + "user.write", + "resourceOwner", + "aggregateID"), + }, + args: args{ + ctx: authz.SetCtxData(context.Background(), authz.CtxData{ + UserID: "aggregateID", + }), + resourceOwner: "resourceOwner", + aggregateID: "aggregateID", + allowSelfManagement: false, }, }, { @@ -194,7 +216,7 @@ func TestCommands_CheckPermissionUserWrite(t *testing.T) { if tt.fields.domainPermissionCheck != nil { c.checkPermission = tt.fields.domainPermissionCheck(t) } - err := c.NewPermissionCheckUserWrite(tt.args.ctx)(tt.args.resourceOwner, tt.args.aggregateID) + err := c.NewPermissionCheckUserWrite(tt.args.ctx, tt.args.allowSelfManagement)(tt.args.resourceOwner, tt.args.aggregateID) if tt.want.err != nil { assert.True(t, tt.want.err(err)) } @@ -223,7 +245,40 @@ func TestCommands_CheckPermissionUserDelete(t *testing.T) { want want }{ { - name: "self, no permission check", + name: "self permission allowed, permission check", + fields: fields{ + domainPermissionCheck: mockDomainPermissionCheck( + userCtx, + "user.delete", + "resourceOwner", + "aggregateID"), + }, + args: args{ + ctx: userCtx, + resourceOwner: "resourceOwner", + aggregateID: "aggregateID", + }, + }, + { + name: "self user.delete not allowed, user.self.delete permission check", + fields: fields{ + domainPermissionCheck: mockDomainPermissionChecks( + expectedCheck{ + userCtx, + "user.delete", + "resourceOwner", + "aggregateID", + zerrors.ThrowPermissionDenied(nil, "id", "permission denied"), + }, + expectedCheck{ + userCtx, + "user.self.delete", + "resourceOwner", + "aggregateID", + nil, + }, + ), + }, args: args{ ctx: userCtx, resourceOwner: "resourceOwner", @@ -276,3 +331,41 @@ func mockDomainPermissionCheck(expectCtx context.Context, expectPermission, expe } } } + +type expectedCheck struct { + ctx context.Context + permission string + resourceOwner string + resourceID string + err error +} + +func mockDomainPermissionChecks(checks ...expectedCheck) func(t *testing.T) domain.PermissionCheck { + var i int + return func(t *testing.T) domain.PermissionCheck { + t.Cleanup(func() { + t.Helper() + if i != len(checks) { + t.Logf("not all expected checks were called, expected: %d, got: %d", len(checks), i) + for ; i < len(checks); i++ { + t.Logf("missing call: %+v", checks[i]) + } + t.Fail() + } + }) + + return func(ctx context.Context, permission, orgID, resourceID string) (err error) { + if i >= len(checks) { + assert.Fail(t, "no more checks expected") + return nil + } + expect := checks[i] + assert.Equal(t, expect.ctx, ctx) + assert.Equal(t, expect.permission, permission) + assert.Equal(t, expect.resourceOwner, orgID) + assert.Equal(t, expect.resourceID, resourceID) + i++ + return expect.err + } + } +} diff --git a/internal/command/user_human_otp.go b/internal/command/user_human_otp.go index fca762cbabb..7c216b82801 100644 --- a/internal/command/user_human_otp.go +++ b/internal/command/user_human_otp.go @@ -237,7 +237,7 @@ func (c *Commands) HumanRemoveTOTP(ctx context.Context, userID, resourceOwner st if existingOTP.State == domain.MFAStateUnspecified || existingOTP.State == domain.MFAStateRemoved { return nil, zerrors.ThrowNotFound(nil, "COMMAND-Hd9sd", "Errors.User.MFA.OTP.NotExisting") } - if err := c.checkPermissionUpdateUser(ctx, existingOTP.ResourceOwner, userID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingOTP.ResourceOwner, userID, true); err != nil { return nil, err } userAgg := UserAggregateFromWriteModel(&existingOTP.WriteModel) @@ -309,7 +309,7 @@ func (c *Commands) RemoveHumanOTPSMS(ctx context.Context, userID, resourceOwner if err != nil { return nil, err } - if err := c.checkPermissionUpdateUser(ctx, existingOTP.WriteModel.ResourceOwner, userID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingOTP.WriteModel.ResourceOwner, userID, true); err != nil { return nil, err } if !existingOTP.otpAdded { @@ -439,7 +439,7 @@ func (c *Commands) RemoveHumanOTPEmail(ctx context.Context, userID, resourceOwne if err != nil { return nil, err } - if err := c.checkPermissionUpdateUser(ctx, existingOTP.WriteModel.ResourceOwner, userID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingOTP.WriteModel.ResourceOwner, userID, true); err != nil { return nil, err } if !existingOTP.otpAdded { diff --git a/internal/command/user_human_password.go b/internal/command/user_human_password.go index 1d91dc05edb..ad0123f1a01 100644 --- a/internal/command/user_human_password.go +++ b/internal/command/user_human_password.go @@ -112,7 +112,7 @@ type setPasswordVerification func(ctx context.Context) (newEncodedPassword strin // setPasswordWithPermission returns a permission check as [setPasswordVerification] implementation func (c *Commands) setPasswordWithPermission(userID, orgID string) setPasswordVerification { return func(ctx context.Context) (_ string, err error) { - return "", c.checkPermissionUpdateUser(ctx, orgID, userID) + return "", c.checkPermissionUpdateUser(ctx, orgID, userID, false) } } diff --git a/internal/command/user_human_webauthn.go b/internal/command/user_human_webauthn.go index 3b8a66e0d51..f6a85f73f78 100644 --- a/internal/command/user_human_webauthn.go +++ b/internal/command/user_human_webauthn.go @@ -601,7 +601,7 @@ func (c *Commands) removeHumanWebAuthN(ctx context.Context, userID, webAuthNID, return nil, zerrors.ThrowNotFound(nil, "COMMAND-DAfb2", "Errors.User.WebAuthN.NotFound") } - if err := c.checkPermissionUpdateUser(ctx, existingWebAuthN.ResourceOwner, existingWebAuthN.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingWebAuthN.ResourceOwner, existingWebAuthN.AggregateID, true); err != nil { return nil, err } diff --git a/internal/command/user_metadata.go b/internal/command/user_metadata.go index 294866e23bc..ac50f8a468f 100644 --- a/internal/command/user_metadata.go +++ b/internal/command/user_metadata.go @@ -11,7 +11,7 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) -func (c *Commands) SetUserMetadata(ctx context.Context, metadata *domain.Metadata, userID, resourceOwner string) (_ *domain.Metadata, err error) { +func (c *Commands) SetUserMetadata(ctx context.Context, metadata *domain.Metadata, userID, resourceOwner string, check PermissionCheck) (_ *domain.Metadata, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() @@ -20,8 +20,10 @@ func (c *Commands) SetUserMetadata(ctx context.Context, metadata *domain.Metadat return nil, err } - if err := c.checkPermissionUpdateUser(ctx, userResourceOwner, userID); err != nil { - return nil, err + if check != nil { + if err := check(userResourceOwner, userID); err != nil { + return nil, err + } } setMetadata, err := c.getUserMetadataModelByID(ctx, userID, userResourceOwner, metadata.Key) @@ -30,7 +32,7 @@ func (c *Commands) SetUserMetadata(ctx context.Context, metadata *domain.Metadat } userAgg := UserAggregateFromWriteModel(&setMetadata.WriteModel) // return if no change in the metadata - if bytes.Equal(setMetadata.Value, metadata.Value) { + if setMetadata.State == domain.MetadataStateActive && bytes.Equal(setMetadata.Value, metadata.Value) { return writeModelToUserMetadata(setMetadata), nil } @@ -50,7 +52,7 @@ func (c *Commands) SetUserMetadata(ctx context.Context, metadata *domain.Metadat return writeModelToUserMetadata(setMetadata), nil } -func (c *Commands) BulkSetUserMetadata(ctx context.Context, userID, resourceOwner string, metadatas ...*domain.Metadata) (_ *domain.ObjectDetails, err error) { +func (c *Commands) BulkSetUserMetadata(ctx context.Context, userID, resourceOwner string, check PermissionCheck, metadatas ...*domain.Metadata) (_ *domain.ObjectDetails, err error) { if len(metadatas) == 0 { return nil, zerrors.ThrowPreconditionFailed(nil, "META-9mm2d", "Errors.Metadata.NoData") } @@ -59,8 +61,10 @@ func (c *Commands) BulkSetUserMetadata(ctx context.Context, userID, resourceOwne return nil, err } - if err := c.checkPermissionUpdateUser(ctx, userResourceOwner, userID); err != nil { - return nil, err + if check != nil { + if err := check(userResourceOwner, userID); err != nil { + return nil, err + } } events := make([]eventstore.Command, 0) @@ -109,7 +113,7 @@ func (c *Commands) setUserMetadata(ctx context.Context, userAgg *eventstore.Aggr ), nil } -func (c *Commands) RemoveUserMetadata(ctx context.Context, metadataKey, userID, resourceOwner string) (_ *domain.ObjectDetails, err error) { +func (c *Commands) RemoveUserMetadata(ctx context.Context, metadataKey, userID, resourceOwner string, check PermissionCheck) (_ *domain.ObjectDetails, err error) { if metadataKey == "" { return nil, zerrors.ThrowInvalidArgument(nil, "META-2n0fs", "Errors.Metadata.Invalid") } @@ -117,9 +121,10 @@ func (c *Commands) RemoveUserMetadata(ctx context.Context, metadataKey, userID, if err != nil { return nil, err } - - if err := c.checkPermissionUpdateUser(ctx, userResourceOwner, userID); err != nil { - return nil, err + if check != nil { + if err := check(userResourceOwner, userID); err != nil { + return nil, err + } } removeMetadata, err := c.getUserMetadataModelByID(ctx, userID, userResourceOwner, metadataKey) @@ -146,7 +151,7 @@ func (c *Commands) RemoveUserMetadata(ctx context.Context, metadataKey, userID, return writeModelToObjectDetails(&removeMetadata.WriteModel), nil } -func (c *Commands) BulkRemoveUserMetadata(ctx context.Context, userID, resourceOwner string, metadataKeys ...string) (_ *domain.ObjectDetails, err error) { +func (c *Commands) BulkRemoveUserMetadata(ctx context.Context, userID, resourceOwner string, check PermissionCheck, metadataKeys ...string) (_ *domain.ObjectDetails, err error) { if len(metadataKeys) == 0 { return nil, zerrors.ThrowPreconditionFailed(nil, "META-9mm2d", "Errors.Metadata.NoData") } @@ -154,9 +159,10 @@ func (c *Commands) BulkRemoveUserMetadata(ctx context.Context, userID, resourceO if err != nil { return nil, err } - - if err := c.checkPermissionUpdateUser(ctx, userResourceOwner, userID); err != nil { - return nil, err + if check != nil { + if err := check(userResourceOwner, userID); err != nil { + return nil, err + } } events := make([]eventstore.Command, len(metadataKeys)) diff --git a/internal/command/user_metadata_test.go b/internal/command/user_metadata_test.go index e8fe25acd98..88cde4bfb39 100644 --- a/internal/command/user_metadata_test.go +++ b/internal/command/user_metadata_test.go @@ -16,8 +16,7 @@ import ( func TestCommandSide_SetUserMetadata(t *testing.T) { type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore - checkPermission domain.PermissionCheck + eventstore func(t *testing.T) *eventstore.Eventstore } type ( args struct { @@ -25,6 +24,7 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { orgID string userID string metadata *domain.Metadata + check PermissionCheck } ) type res struct { @@ -43,7 +43,6 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { eventstore: expectEventstore( expectFilter(), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -88,7 +87,6 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -123,7 +121,6 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckNotAllowed(), }, args: args{ ctx: context.Background(), @@ -133,6 +130,9 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { Key: "key", Value: []byte("value"), }, + check: func(resourceOwner, aggregateID string) error { + return zerrors.ThrowPermissionDenied(nil, "id", "permission denied") + }, }, res: res{ err: zerrors.IsPermissionDenied, @@ -167,7 +167,6 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -218,9 +217,13 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { []byte("value"), ), ), + eventFromEventPusher( + user.NewMetadataRemovedAllEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + ), + ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -235,7 +238,7 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { }, }, { - name: "add metadata, reset, ok", + name: "add metadata with same key, ok", fields: fields{ eventstore: expectEventstore( expectFilter( @@ -271,7 +274,6 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -294,14 +296,130 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { }, }, }, + { + name: "add metadata with same key and value, ok (ignore)", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "", + "firstname lastname", + language.Und, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + expectFilter( + eventFromEventPusher( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + []byte("value"), + ), + ), + ), + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + metadata: &domain.Metadata{ + Key: "key", + Value: []byte("value"), + }, + }, + res: res{ + want: &domain.Metadata{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + ResourceOwner: "org1", + }, + Key: "key", + Value: []byte("value"), + State: domain.MetadataStateActive, + }, + }, + }, + { + name: "add deleted metadata with same value, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "", + "firstname lastname", + language.Und, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + expectFilter( + eventFromEventPusher( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + []byte("value"), + ), + ), + eventFromEventPusher( + user.NewMetadataRemovedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + ), + ), + ), + expectPush( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + []byte("value"), + ), + ), + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + metadata: &domain.Metadata{ + Key: "key", + Value: []byte("value"), + }, + }, + res: res{ + want: &domain.Metadata{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "user1", + ResourceOwner: "org1", + }, + Key: "key", + Value: []byte("value"), + State: domain.MetadataStateActive, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := &Commands{ - eventstore: tt.fields.eventstore(t), - checkPermission: tt.fields.checkPermission, + eventstore: tt.fields.eventstore(t), } - got, err := r.SetUserMetadata(tt.args.ctx, tt.args.metadata, tt.args.userID, tt.args.orgID) + got, err := r.SetUserMetadata(tt.args.ctx, tt.args.metadata, tt.args.userID, tt.args.orgID, tt.args.check) if tt.res.err == nil { assert.NoError(t, err) } @@ -317,14 +435,14 @@ func TestCommandSide_SetUserMetadata(t *testing.T) { func TestCommandSide_BulkSetUserMetadata(t *testing.T) { type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore - checkPermission domain.PermissionCheck + eventstore func(t *testing.T) *eventstore.Eventstore } type ( args struct { ctx context.Context orgID string userID string + check PermissionCheck metadataList []*domain.Metadata } ) @@ -394,7 +512,6 @@ func TestCommandSide_BulkSetUserMetadata(t *testing.T) { ), expectFilter(), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -430,12 +547,14 @@ func TestCommandSide_BulkSetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckNotAllowed(), }, args: args{ ctx: context.Background(), orgID: "org1", userID: "user1", + check: func(resourceOwner, aggregateID string) error { + return zerrors.ThrowPermissionDenied(nil, "id", "permission-denied") + }, metadataList: []*domain.Metadata{ {Key: "key", Value: []byte("value")}, {Key: "key1", Value: []byte("value1")}, @@ -479,7 +598,6 @@ func TestCommandSide_BulkSetUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -496,14 +614,82 @@ func TestCommandSide_BulkSetUserMetadata(t *testing.T) { }, }, }, + { + name: "re add deleted metadata, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "", + "firstname lastname", + language.Und, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + expectFilter( + eventFromEventPusher( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + []byte("value"), + )), + eventFromEventPusher( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key1", + []byte("value1"), + )), + eventFromEventPusher( + user.NewMetadataRemovedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + )), + ), + expectPush( + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key", + []byte("value"), + ), + user.NewMetadataSetEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "key2", + []byte("value2"), + ), + ), + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + metadataList: []*domain.Metadata{ + {Key: "key", Value: []byte("value")}, + {Key: "key1", Value: []byte("value1")}, + {Key: "key2", Value: []byte("value2")}, + }, + }, + res: res{ + want: &domain.ObjectDetails{ + ResourceOwner: "org1", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := &Commands{ - eventstore: tt.fields.eventstore(t), - checkPermission: tt.fields.checkPermission, + eventstore: tt.fields.eventstore(t), } - got, err := r.BulkSetUserMetadata(tt.args.ctx, tt.args.userID, tt.args.orgID, tt.args.metadataList...) + got, err := r.BulkSetUserMetadata(tt.args.ctx, tt.args.userID, tt.args.orgID, tt.args.check, tt.args.metadataList...) if tt.res.err == nil { assert.NoError(t, err) } @@ -519,8 +705,7 @@ func TestCommandSide_BulkSetUserMetadata(t *testing.T) { func TestCommandSide_UserRemoveMetadata(t *testing.T) { type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore - checkPermission domain.PermissionCheck + eventstore func(t *testing.T) *eventstore.Eventstore } type ( args struct { @@ -528,6 +713,7 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { orgID string userID string metadataKey string + check PermissionCheck } ) type res struct { @@ -594,7 +780,6 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { ), expectFilter(), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -627,13 +812,15 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckNotAllowed(), }, args: args{ ctx: context.Background(), orgID: "org1", userID: "user1", metadataKey: "key", + check: func(resourceOwner, aggregateID string) error { + return zerrors.ThrowPermissionDenied(nil, "id", "permission denied") + }, }, res: res{ err: zerrors.IsPermissionDenied, @@ -675,7 +862,6 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -693,10 +879,9 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := &Commands{ - eventstore: tt.fields.eventstore(t), - checkPermission: tt.fields.checkPermission, + eventstore: tt.fields.eventstore(t), } - got, err := r.RemoveUserMetadata(tt.args.ctx, tt.args.metadataKey, tt.args.userID, tt.args.orgID) + got, err := r.RemoveUserMetadata(tt.args.ctx, tt.args.metadataKey, tt.args.userID, tt.args.orgID, tt.args.check) if tt.res.err == nil { assert.NoError(t, err) } @@ -712,14 +897,14 @@ func TestCommandSide_UserRemoveMetadata(t *testing.T) { func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { type fields struct { - eventstore func(t *testing.T) *eventstore.Eventstore - checkPermission domain.PermissionCheck + eventstore func(t *testing.T) *eventstore.Eventstore } type ( args struct { ctx context.Context orgID string userID string + check PermissionCheck metadataList []string } ) @@ -794,7 +979,6 @@ func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -843,7 +1027,6 @@ func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -876,13 +1059,15 @@ func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckNotAllowed(), }, args: args{ ctx: context.Background(), orgID: "org1", userID: "user1", metadataList: []string{"key", "key1"}, + check: func(resourceOwner, aggregateID string) error { + return zerrors.ThrowPermissionDenied(nil, "id", "permission denied") + }, }, res: res{ err: zerrors.IsPermissionDenied, @@ -935,7 +1120,6 @@ func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { ), ), ), - checkPermission: newMockPermissionCheckAllowed(), }, args: args{ ctx: context.Background(), @@ -953,10 +1137,9 @@ func TestCommandSide_BulkRemoveUserMetadata(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := &Commands{ - eventstore: tt.fields.eventstore(t), - checkPermission: tt.fields.checkPermission, + eventstore: tt.fields.eventstore(t), } - got, err := r.BulkRemoveUserMetadata(tt.args.ctx, tt.args.userID, tt.args.orgID, tt.args.metadataList...) + got, err := r.BulkRemoveUserMetadata(tt.args.ctx, tt.args.userID, tt.args.orgID, tt.args.check, tt.args.metadataList...) if tt.res.err == nil { assert.NoError(t, err) } diff --git a/internal/command/user_v2.go b/internal/command/user_v2.go index d6c5e7de53c..c9ae672cf10 100644 --- a/internal/command/user_v2.go +++ b/internal/command/user_v2.go @@ -27,7 +27,7 @@ func (c *Commands) LockUserV2(ctx context.Context, userID string) (*domain.Objec return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-lgws8wtsqf", "Errors.User.ShouldBeActiveOrInitial") } - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, false); err != nil { return nil, err } @@ -52,7 +52,7 @@ func (c *Commands) UnlockUserV2(ctx context.Context, userID string) (*domain.Obj if !hasUserState(existingHuman.UserState, domain.UserStateLocked) { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-olb9vb0oca", "Errors.User.NotLocked") } - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, false); err != nil { return nil, err } @@ -80,7 +80,7 @@ func (c *Commands) DeactivateUserV2(ctx context.Context, userID string) (*domain if isUserStateInactive(existingHuman.UserState) { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-5gunjw0cd7", "Errors.User.AlreadyInactive") } - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, false); err != nil { return nil, err } @@ -105,7 +105,7 @@ func (c *Commands) ReactivateUserV2(ctx context.Context, userID string) (*domain if !isUserStateInactive(existingHuman.UserState) { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-s5qqcz97hf", "Errors.User.NotInactive") } - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, false); err != nil { return nil, err } diff --git a/internal/command/user_v2_email.go b/internal/command/user_v2_email.go index f372d5846a3..59793e50bf8 100644 --- a/internal/command/user_v2_email.go +++ b/internal/command/user_v2_email.go @@ -148,7 +148,7 @@ func (c *Commands) changeUserEmailWithGeneratorEvents(ctx context.Context, userI if err != nil { return nil, err } - if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID); err != nil { + if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID, true); err != nil { return nil, err } if err = cmd.Change(ctx, domain.EmailAddress(email)); err != nil { @@ -170,7 +170,7 @@ func (c *Commands) sendUserEmailCodeWithGeneratorEvents(ctx context.Context, use if err != nil { return nil, err } - if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID); err != nil { + if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID, true); err != nil { return nil, err } if existingCheck && cmd.model.Code == nil { diff --git a/internal/command/user_v2_human.go b/internal/command/user_v2_human.go index 5e7973e6b33..558ad345889 100644 --- a/internal/command/user_v2_human.go +++ b/internal/command/user_v2_human.go @@ -138,7 +138,7 @@ func (c *Commands) AddUserHuman(ctx context.Context, resourceOwner string, human } // check for permission to create user on resourceOwner if !human.Register { - if err := c.checkPermissionUpdateUser(ctx, resourceOwner, human.ID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, resourceOwner, human.ID, true); err != nil { return err } } @@ -274,6 +274,7 @@ func (c *Commands) ChangeUserHuman(ctx context.Context, human *ChangeHuman, alg return err } + metadataChanged := len(human.Metadata) > 0 || len(human.MetadataKeysToRemove) > 0 existingHuman, err := c.UserHumanWriteModel( ctx, human.ID, @@ -284,14 +285,14 @@ func (c *Commands) ChangeUserHuman(ctx context.Context, human *ChangeHuman, alg human.Password != nil, false, // avatar not updateable false, // IDPLinks not updateable - len(human.Metadata) > 0 || len(human.MetadataKeysToRemove) > 0, + metadataChanged, ) if err != nil { return err } if human.Changed() { - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, !metadataChanged); err != nil { return err } } @@ -517,7 +518,7 @@ func (c *Commands) HumanMFAInitSkippedV2(ctx context.Context, userID string) (*d if !isUserStateExists(existingHuman.UserState) { return nil, zerrors.ThrowNotFound(nil, "COMMAND-auj6jeBei4", "Errors.User.NotFound") } - if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingHuman.ResourceOwner, existingHuman.AggregateID, true); err != nil { return nil, err } diff --git a/internal/command/user_v2_machine.go b/internal/command/user_v2_machine.go index 34079b7e6fa..3de1ea9af88 100644 --- a/internal/command/user_v2_machine.go +++ b/internal/command/user_v2_machine.go @@ -45,7 +45,7 @@ func (c *Commands) ChangeUserMachine(ctx context.Context, machine *ChangeMachine return err } if machine.Changed() { - if err := c.checkPermissionUpdateUser(ctx, existingMachine.ResourceOwner, existingMachine.AggregateID); err != nil { + if err := c.checkPermissionUpdateUser(ctx, existingMachine.ResourceOwner, existingMachine.AggregateID, true); err != nil { return err } } diff --git a/internal/command/user_v2_password.go b/internal/command/user_v2_password.go index e04b6ed7486..7961f740cf4 100644 --- a/internal/command/user_v2_password.go +++ b/internal/command/user_v2_password.go @@ -49,7 +49,7 @@ func (c *Commands) requestPasswordReset(ctx context.Context, userID string, retu if model.UserState == domain.UserStateInitial { return nil, nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-Sfe4g", "Errors.User.NotInitialised") } - if err = c.checkPermissionUpdateUser(ctx, model.ResourceOwner, userID); err != nil { + if err = c.checkPermissionUpdateUser(ctx, model.ResourceOwner, userID, true); err != nil { return nil, nil, err } var passwordCode *EncryptedCode diff --git a/internal/command/user_v2_phone.go b/internal/command/user_v2_phone.go index 8648f9a5649..9a84d2e9a0f 100644 --- a/internal/command/user_v2_phone.go +++ b/internal/command/user_v2_phone.go @@ -82,7 +82,7 @@ func (c *Commands) changeUserPhoneWithGenerator(ctx context.Context, userID, pho if err != nil { return nil, err } - if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID); err != nil { + if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID, true); err != nil { return nil, err } if err = cmd.Change(ctx, domain.PhoneNumber(phone)); err != nil { @@ -102,7 +102,7 @@ func (c *Commands) resendUserPhoneCodeWithGenerator(ctx context.Context, userID if err != nil { return nil, err } - if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID); err != nil { + if err = c.checkPermissionUpdateUser(ctx, cmd.aggregate.ResourceOwner, userID, true); err != nil { return nil, err } if cmd.model.Code == nil && cmd.model.GeneratorID == "" { diff --git a/internal/command/user_v2_test.go b/internal/command/user_v2_test.go index 685ad952535..4e590e9f8f8 100644 --- a/internal/command/user_v2_test.go +++ b/internal/command/user_v2_test.go @@ -18,6 +18,7 @@ import ( ) func TestCommandSide_LockUserV2(t *testing.T) { + userAgg := &user.NewAggregate("user1", "org1").Aggregate type fields struct { eventstore func(*testing.T) *eventstore.Eventstore checkPermission domain.PermissionCheck @@ -79,7 +80,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -93,7 +94,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -117,7 +118,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -127,7 +128,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -151,7 +152,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -166,7 +167,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { ), expectPush( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -189,7 +190,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -222,7 +223,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -233,7 +234,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { ), expectPush( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -271,6 +272,7 @@ func TestCommandSide_LockUserV2(t *testing.T) { } func TestCommandSide_UnlockUserV2(t *testing.T) { + userAgg := &user.NewAggregate("user1", "org1").Aggregate type fields struct { eventstore func(*testing.T) *eventstore.Eventstore checkPermission domain.PermissionCheck @@ -332,7 +334,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -364,7 +366,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { eventstore: expectEventstore( expectFilter( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -392,7 +394,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -406,12 +408,12 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), expectPush( user.NewUserUnlockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -434,7 +436,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -448,7 +450,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), ), @@ -471,7 +473,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -481,12 +483,12 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserLockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), expectPush( user.NewUserUnlockedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -524,6 +526,7 @@ func TestCommandSide_UnlockUserV2(t *testing.T) { } func TestCommandSide_DeactivateUserV2(t *testing.T) { + userAgg := &user.NewAggregate("user1", "org1").Aggregate type fields struct { eventstore func(*testing.T) *eventstore.Eventstore checkPermission domain.PermissionCheck @@ -585,7 +588,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -599,7 +602,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewHumanInitialCodeAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, nil, time.Hour*1, "", ), @@ -625,7 +628,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -639,7 +642,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -663,7 +666,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -677,13 +680,13 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewHumanInitializedCheckSucceededEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), expectPush( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -706,7 +709,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -720,7 +723,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewHumanInitializedCheckSucceededEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -744,7 +747,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -754,7 +757,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -778,7 +781,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -789,7 +792,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { ), expectPush( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -827,6 +830,7 @@ func TestCommandSide_DeactivateUserV2(t *testing.T) { } func TestCommandSide_ReactivateUserV2(t *testing.T) { + userAgg := &user.NewAggregate("user1", "org1").Aggregate type fields struct { eventstore func(*testing.T) *eventstore.Eventstore checkPermission domain.PermissionCheck @@ -888,7 +892,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -921,7 +925,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -950,7 +954,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -964,12 +968,12 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), expectPush( user.NewUserReactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -992,7 +996,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -1006,7 +1010,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), ), @@ -1029,7 +1033,7 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -1039,12 +1043,12 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserDeactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate), + userAgg), ), ), expectPush( user.NewUserReactivatedEvent(context.Background(), - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -1084,6 +1088,8 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) { func TestCommandSide_RemoveUserV2(t *testing.T) { ctxUserID := "ctxUserID" ctx := authz.SetCtxData(context.Background(), authz.CtxData{UserID: ctxUserID}) + userAgg := &user.NewAggregate("user1", "org1").Aggregate + orgAgg := &org.NewAggregate("org1").Aggregate type fields struct { eventstore func(*testing.T) *eventstore.Eventstore checkPermission domain.PermissionCheck @@ -1144,7 +1150,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -1158,7 +1164,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserRemovedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", nil, true, @@ -1184,7 +1190,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -1199,8 +1205,8 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), expectFilter( eventFromEventPusher( - org.NewDomainPolicyAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + org.NewDomainPolicyAddedEvent(context.Background(), + orgAgg, true, true, true, @@ -1209,7 +1215,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), expectPush( user.NewUserRemovedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", nil, true, @@ -1234,7 +1240,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "firstname", "lastname", @@ -1248,7 +1254,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), eventFromEventPusher( user.NewHumanInitializedCheckSucceededEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, ), ), ), @@ -1269,7 +1275,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -1279,7 +1285,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), eventFromEventPusher( user.NewUserRemovedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", nil, true, @@ -1304,7 +1310,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { expectFilter( eventFromEventPusher( user.NewMachineAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", "name", "description", @@ -1315,8 +1321,8 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), expectFilter( eventFromEventPusher( - org.NewDomainPolicyAddedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + org.NewDomainPolicyAddedEvent(context.Background(), + orgAgg, true, true, true, @@ -1325,7 +1331,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), expectPush( user.NewUserRemovedEvent(ctx, - &user.NewAggregate("user1", "org1").Aggregate, + userAgg, "username", nil, true, @@ -1344,7 +1350,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { }, }, { - name: "remove self, ok", + name: "remove self, permission denied", fields: fields{ eventstore: expectEventstore( expectFilter( @@ -1363,24 +1369,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), ), ), - expectFilter( - eventFromEventPusher( - org.NewDomainPolicyAddedEvent(ctx, - &user.NewAggregate(ctxUserID, "org1").Aggregate, - true, - true, - true, - ), - ), - ), - expectPush( - user.NewUserRemovedEvent(ctx, - &user.NewAggregate(ctxUserID, "org1").Aggregate, - "username", - nil, - true, - ), - ), ), checkPermission: newMockPermissionCheckNotAllowed(), }, @@ -1388,9 +1376,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { userID: ctxUserID, }, res: res{ - want: &domain.ObjectDetails{ - ResourceOwner: "org1", - }, + err: zerrors.IsPermissionDenied, }, }, } diff --git a/internal/domain/permission.go b/internal/domain/permission.go index 6c67841dfd2..42154930864 100644 --- a/internal/domain/permission.go +++ b/internal/domain/permission.go @@ -30,6 +30,7 @@ const ( PermissionUserWrite = "user.write" PermissionUserRead = "user.read" PermissionUserDelete = "user.delete" + PermissionUserDeleteSelf = "user.self.delete" PermissionUserCredentialWrite = "user.credential.write" PermissionSessionWrite = "session.write" PermissionSessionRead = "session.read" diff --git a/internal/integration/client.go b/internal/integration/client.go index aa3b16781c4..f829be6775d 100644 --- a/internal/integration/client.go +++ b/internal/integration/client.go @@ -978,13 +978,17 @@ func (i *Instance) DeleteInstanceMembership(t *testing.T, ctx context.Context, u require.NoError(t, err) } -func (i *Instance) CreateOrgMembership(t *testing.T, ctx context.Context, orgID, userID string) *internal_permission_v2beta.CreateAdministratorResponse { +// CreateOrgMembership creates an org membership with the given roles. If no roles are provided, the user will be assigned the "org.owner" role. +func (i *Instance) CreateOrgMembership(t *testing.T, ctx context.Context, orgID, userID string, roles ...string) *internal_permission_v2beta.CreateAdministratorResponse { + if len(roles) == 0 { + roles = []string{domain.RoleOrgOwner} + } resp, err := i.Client.InternalPermissionv2Beta.CreateAdministrator(ctx, &internal_permission_v2beta.CreateAdministratorRequest{ Resource: &internal_permission_v2beta.ResourceType{ Resource: &internal_permission_v2beta.ResourceType_OrganizationId{OrganizationId: orgID}, }, UserId: userID, - Roles: []string{domain.RoleOrgOwner}, + Roles: roles, }) require.NoError(t, err) return resp