TheArchive/zitadel
1
0
Fork 0
mirror of https://github.com/zitadel/zitadel.git synced 2025-12-06 16:02:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: correct user self management on metadata and delete (#10666)

Browse Source 
# 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 5329d50509)
This commit is contained in:
Livio Spring
2025-09-16 14:26:21 +02:00
parent 389f908041
commit fa83c39510
31 changed files with 695 additions and 208 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
internal/api/grpc/admin/import.go
View File

@@ -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) {

8
internal/api/grpc/management/user.go
View File

@@ -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
}

48
internal/api/grpc/user/v2/integration_test/user_test.go
View File

@@ -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)

4
internal/api/grpc/user/v2/key.go
View File

@@ -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)

2
internal/api/grpc/user/v2/machine.go
View File

@@ -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 {

4
internal/api/grpc/user/v2/metadata.go
View File

@@ -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
}

4
internal/api/grpc/user/v2/pat.go
View File

@@ -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

4
internal/api/grpc/user/v2/secret.go
View File

@@ -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

55
internal/api/grpc/user/v2beta/integration_test/user_test.go
View File

@@ -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{