diff --git a/internal/actions/object/metadata.go b/internal/actions/object/metadata.go index a2d1f66a9..2a80d19f8 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 000000000..80dccfe46 --- /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 c2c8c748d..67802777e 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 f40a29868..440213ae4 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 64ab39668..cb7ca8d9b 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 021f4be38..243f163ca 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 e5126b901..7889d2c6d 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 338ce9fc4..c7ce97e38 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 0c90eeaeb..36e820136 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 acc7aef8c..6ab700893 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 b655ae791..3b90759e9 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 08ee4da3e..15ec5b21e 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 b89fb69f9..94288142c 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 b645c9f4a..ab58390c8 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 ea260eeb1..d2e2bc62f 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 97bae79ba..021107e6a 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 5c36dc14f..5afec753a 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 fca762cba..7c216b828 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 1d91dc05e..ad0123f1a 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 3b8a66e0d..f6a85f73f 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 294866e23..ac50f8a46 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 e8fe25acd..88cde4bfb 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 028fda7f4..1963e5e30 100644 --- a/internal/command/user_v2.go +++ b/internal/command/user_v2.go @@ -28,7 +28,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 } @@ -53,7 +53,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 } @@ -81,7 +81,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 } @@ -106,7 +106,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 f372d5846..59793e50b 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 572aa2962..8bbd8b674 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 } } @@ -279,6 +279,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, @@ -289,14 +290,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 } } @@ -522,7 +523,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 34079b7e6..3de1ea9af 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 e04b6ed74..7961f740c 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 8648f9a56..9a84d2e9a 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 f878569a0..80408e5f9 100644 --- a/internal/command/user_v2_test.go +++ b/internal/command/user_v2_test.go @@ -1352,7 +1352,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { }, }, { - name: "remove self, ok", + name: "remove self, permission denied", fields: fields{ eventstore: expectEventstore( expectFilter( @@ -1371,25 +1371,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) { ), ), ), - expectFilter( - eventFromEventPusher( - org.NewDomainPolicyAddedEvent(ctx, - orgAgg, - true, - true, - true, - ), - ), - ), - expectFilterOrganizationSettings("org1", false, false), - expectPush( - user.NewUserRemovedEvent(ctx, - &user.NewAggregate(ctxUserID, "org1").Aggregate, - "username", - nil, - true, - ), - ), ), checkPermission: newMockPermissionCheckNotAllowed(), }, @@ -1397,9 +1378,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 cbea7028a..ff21530af 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 848262a51..78f07425c 100644 --- a/internal/integration/client.go +++ b/internal/integration/client.go @@ -999,13 +999,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