TheArchive/zitadel
1
0
Fork 0
mirror of https://github.com/zitadel/zitadel.git synced 2025-08-11 17:27:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: allow user self deletion (#9828)

Browse Source 
# Which Problems Are Solved

Currently, users can't delete themselves using the V2 RemoveUser API
because of the redunant API middleware permission check.

On main, using a machine user PAT to delete the same machine user:

```bash
grpcurl -plaintext -H "Authorization: Bearer ${ZITADEL_ACCESS_TOKEN}" -d '{"userId": "318838604669387137"}' localhost:8080 zitadel.user.v2.UserService.DeleteUser
ERROR:
  Code: NotFound
  Message: membership not found (AUTHZ-cdgFk)
  Details:
  1)	{
    	  "@type": "type.googleapis.com/zitadel.v1.ErrorDetail",
    	  "id": "AUTHZ-cdgFk",
    	  "message": "membership not found"
    	}
```

Same on this PRs branch:

```bash
grpcurl -plaintext -H "Authorization: Bearer ${ZITADEL_ACCESS_TOKEN}" -d '{"userId": "318838604669387137"}' localhost:8080 zitadel.user.v2.UserService.DeleteUser
{
  "details": {
    "sequence": "3",
    "changeDate": "2025-05-06T13:44:54.349048Z",
    "resourceOwner": "318838541083804033"
  }
}
```

Repeated call
```bash
grpcurl -plaintext -H "Authorization: Bearer ${ZITADEL_ACCESS_TOKEN}" -d '{"userId": "318838604669387137"}' localhost:8080 zitadel.user.v2.UserService.DeleteUser
ERROR:
  Code: Unauthenticated
  Message: Errors.Token.Invalid (AUTH-7fs1e)
  Details:
  1)	{
    	  "@type": "type.googleapis.com/zitadel.v1.ErrorDetail",
    	  "id": "AUTH-7fs1e",
    	  "message": "Errors.Token.Invalid"
    	}
```

# How the Problems Are Solved

The middleware permission check is disabled and the
domain.PermissionCheck is used exclusively.

# Additional Changes

A new type command.PermissionCheck allows to optionally accept a
permission check for commands, so APIs with middleware permission checks
can omit redundant permission checks by passing nil while APIs without
middleware permission checks can pass one to the command.

# Additional Context

This is a subtask of #9763

---------

Co-authored-by: Livio Spring <livio.a@gmail.com>
This commit is contained in:
Elio Bischof
2025-05-07 15:24:24 +02:00
committed by GitHub
parent 0d7d4e6af0
commit 898366c537
9 changed files with 459 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
internal/command/permission_checks.go Normal file
View File

@@ -0,0 +1,60 @@
package command
import (
"context"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/eventstore"
"github.com/zitadel/zitadel/internal/v2/user"
"github.com/zitadel/zitadel/internal/zerrors"
)
type PermissionCheck func(resourceOwner, aggregateID string) error
func (c *Commands) newPermissionCheck(ctx context.Context, permission string, aggregateType eventstore.AggregateType) PermissionCheck {
return func(resourceOwner, aggregateID string) error {
if aggregateID == "" {
return zerrors.ThrowInternal(nil, "COMMAND-ulBlS", "Errors.IDMissing")
}
// For example if a write model didn't query any events, the resource owner is probably empty.
// In this case, we have to query an event on the given aggregate to get the resource owner.
if resourceOwner == "" {
r := NewResourceOwnerModel(authz.GetInstance(ctx).InstanceID(), aggregateType, aggregateID)
err := c.eventstore.FilterToQueryReducer(ctx, r)
if err != nil {
return err
}
resourceOwner = r.resourceOwner
}
if resourceOwner == "" {
return zerrors.ThrowNotFound(nil, "COMMAND-4g3xq", "Errors.NotFound")
}
return c.checkPermission(ctx, permission, resourceOwner, aggregateID)
}
}
func (c *Commands) checkPermissionOnUser(ctx context.Context, permission string) PermissionCheck {
return func(resourceOwner, aggregateID string) error {
if 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) checkPermissionDeleteUser(ctx context.Context, resourceOwner, userID string) error {
return c.checkPermissionOnUser(ctx, domain.PermissionUserDelete)(resourceOwner, userID)
}
func (c *Commands) checkPermissionUpdateUser(ctx context.Context, resourceOwner, userID string) error {
return c.NewPermissionCheckUserWrite(ctx)(resourceOwner, userID)
}
func (c *Commands) checkPermissionUpdateUserCredentials(ctx context.Context, resourceOwner, userID string) error {
return c.checkPermissionOnUser(ctx, domain.PermissionUserCredentialWrite)(resourceOwner, userID)
}

278
internal/command/permission_checks_test.go Normal file
View File

@@ -0,0 +1,278 @@
package command
import (
"context"
"database/sql"
"errors"
"testing"
"github.com/stretchr/testify/assert"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/eventstore"
"github.com/zitadel/zitadel/internal/eventstore/repository"
"github.com/zitadel/zitadel/internal/zerrors"
)
func TestCommands_CheckPermission(t *testing.T) {
type fields struct {
eventstore func(*testing.T) *eventstore.Eventstore
domainPermissionCheck func(*testing.T) domain.PermissionCheck
}
type args struct {
ctx context.Context
permission string
aggregateType eventstore.AggregateType
resourceOwner, aggregateID string
}
type want struct {
err func(error) bool
}
ctx := context.Background()
filterErr := errors.New("filter error")
tests := []struct {
name string
fields fields
args args
want want
}{
{
name: "resource owner is given, no query",
fields: fields{
domainPermissionCheck: mockDomainPermissionCheck(
ctx,
"permission",
"resourceOwner",
"aggregateID"),
},
args: args{
ctx: ctx,
permission: "permission",
resourceOwner: "resourceOwner",
aggregateID: "aggregateID",
},
},
{
name: "resource owner is empty, query for resource owner",
fields: fields{
eventstore: expectEventstore(
expectFilter(&repository.Event{
AggregateID: "aggregateID",
ResourceOwner: sql.NullString{String: "resourceOwner"},
}),
),
domainPermissionCheck: mockDomainPermissionCheck(ctx, "permission", "resourceOwner", "aggregateID"),
},
args: args{
ctx: ctx,
permission: "permission",
resourceOwner: "",
aggregateID: "aggregateID",
},
},
{
name: "resource owner is empty, query for resource owner, error",
fields: fields{
eventstore: expectEventstore(
expectFilterError(filterErr),
),
},
args: args{
ctx: ctx,
permission: "permission",
resourceOwner: "",
aggregateID: "aggregateID",
},
want: want{
err: func(err error) bool {
return errors.Is(err, filterErr)
},
},
},
{
name: "resource owner is empty, query for resource owner, no events",
fields: fields{
eventstore: expectEventstore(
expectFilter(),
),
},
args: args{
ctx: ctx,
permission: "permission",
resourceOwner: "",
aggregateID: "aggregateID",
},
want: want{
err: zerrors.IsNotFound,
},
},
{
name: "no aggregateID, internal error",
args: args{
ctx: ctx,
},
want: want{
err: zerrors.IsInternal,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := &Commands{
checkPermission: func(ctx context.Context, permission, orgID, resourceID string) (err error) {
assert.Failf(t, "Domain permission check should not be called", "Called c.checkPermission(%v,%v,%v,%v)", ctx, permission, orgID, resourceID)
return nil
},
eventstore: expectEventstore()(t),
}
if tt.fields.domainPermissionCheck != nil {
c.checkPermission = tt.fields.domainPermissionCheck(t)
}
if tt.fields.eventstore != nil {
c.eventstore = tt.fields.eventstore(t)
}
err := c.newPermissionCheck(tt.args.ctx, tt.args.permission, tt.args.aggregateType)(tt.args.resourceOwner, tt.args.aggregateID)
if tt.want.err != nil {
assert.True(t, tt.want.err(err))
}
})
}
}
func TestCommands_CheckPermissionUserWrite(t *testing.T) {
type fields struct {
domainPermissionCheck func(*testing.T) domain.PermissionCheck
}
type args struct {
ctx context.Context
resourceOwner, aggregateID string
}
type want struct {
err func(error) bool
}
tests := []struct {
name string
fields fields
args args
want want
}{
{
name: "self, no permission check",
args: args{
ctx: authz.SetCtxData(context.Background(), authz.CtxData{
UserID: "aggregateID",
}),
resourceOwner: "resourceOwner",
aggregateID: "aggregateID",
},
},
{
name: "not self, permission check",
fields: fields{
domainPermissionCheck: mockDomainPermissionCheck(
context.Background(),
"user.write",
"resourceOwner",
"foreignAggregateID"),
},
args: args{
ctx: context.Background(),
resourceOwner: "resourceOwner",
aggregateID: "foreignAggregateID",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := &Commands{
checkPermission: func(ctx context.Context, permission, orgID, resourceID string) (err error) {
assert.Failf(t, "Domain permission check should not be called", "Called c.checkPermission(%v,%v,%v,%v)", ctx, permission, orgID, resourceID)
return nil
},
}
if tt.fields.domainPermissionCheck != nil {
c.checkPermission = tt.fields.domainPermissionCheck(t)
}
err := c.NewPermissionCheckUserWrite(tt.args.ctx)(tt.args.resourceOwner, tt.args.aggregateID)
if tt.want.err != nil {
assert.True(t, tt.want.err(err))
}
})
}
}
func TestCommands_CheckPermissionUserDelete(t *testing.T) {
type fields struct {
domainPermissionCheck func(*testing.T) domain.PermissionCheck
}
type args struct {
ctx context.Context
resourceOwner, aggregateID string
}
type want struct {
err func(error) bool
}
userCtx := authz.SetCtxData(context.Background(), authz.CtxData{
UserID: "aggregateID",
})
tests := []struct {
name string
fields fields
args args
want want
}{
{
name: "self, no permission check",
args: args{
ctx: userCtx,
resourceOwner: "resourceOwner",
aggregateID: "aggregateID",
},
},
{
name: "not self, permission check",
fields: fields{
domainPermissionCheck: mockDomainPermissionCheck(
context.Background(),
"user.delete",
"resourceOwner",
"foreignAggregateID"),
},
args: args{
ctx: context.Background(),
resourceOwner: "resourceOwner",
aggregateID: "foreignAggregateID",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := &Commands{
checkPermission: func(ctx context.Context, permission, orgID, resourceID string) (err error) {
assert.Failf(t, "Domain permission check should not be called", "Called c.checkPermission(%v,%v,%v,%v)", ctx, permission, orgID, resourceID)
return nil
},
}
if tt.fields.domainPermissionCheck != nil {
c.checkPermission = tt.fields.domainPermissionCheck(t)
}
err := c.checkPermissionDeleteUser(tt.args.ctx, tt.args.resourceOwner, tt.args.aggregateID)
if tt.want.err != nil {
assert.True(t, tt.want.err(err))
}
})
}
}
func mockDomainPermissionCheck(expectCtx context.Context, expectPermission, expectResourceOwner, expectResourceID string) func(t *testing.T) domain.PermissionCheck {
return func(t *testing.T) domain.PermissionCheck {
return func(ctx context.Context, permission, orgID, resourceID string) (err error) {
assert.Equal(t, expectCtx, ctx)
assert.Equal(t, expectPermission, permission)
assert.Equal(t, expectResourceOwner, orgID)
assert.Equal(t, expectResourceID, resourceID)
return nil
}
}
}

31
internal/command/user_v2.go
View File

@@ -5,7 +5,6 @@ import (
"github.com/zitadel/logging"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/eventstore"
"github.com/zitadel/zitadel/internal/repository/user"
@@ -117,36 +116,6 @@ func (c *Commands) ReactivateUserV2(ctx context.Context, userID string) (*domain
return writeModelToObjectDetails(&existingHuman.WriteModel), nil
}
func (c *Commands) checkPermissionUpdateUser(ctx context.Context, resourceOwner, userID string) error {
if userID != "" && userID == authz.GetCtxData(ctx).UserID {
return nil
}
if err := c.checkPermission(ctx, domain.PermissionUserWrite, resourceOwner, userID); err != nil {
return err
}
return nil
}
func (c *Commands) checkPermissionUpdateUserCredentials(ctx context.Context, resourceOwner, userID string) error {
if userID != "" && userID == authz.GetCtxData(ctx).UserID {
return nil
}
if err := c.checkPermission(ctx, domain.PermissionUserCredentialWrite, resourceOwner, userID); err != nil {
return err
}
return nil
}
func (c *Commands) checkPermissionDeleteUser(ctx context.Context, resourceOwner, userID string) error {
if userID != "" && userID == authz.GetCtxData(ctx).UserID {
return nil
}
if err := c.checkPermission(ctx, domain.PermissionUserDelete, resourceOwner, userID); err != nil {
return err
}
return nil
}
func (c *Commands) userStateWriteModel(ctx context.Context, userID string) (writeModel *UserV2WriteModel, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()

6
internal/command/user_v2_invite.go
View File

@@ -73,12 +73,12 @@ func (c *Commands) ResendInviteCode(ctx context.Context, userID, resourceOwner,
if err != nil {
return nil, err
}
if err := c.checkPermissionUpdateUser(ctx, existingCode.ResourceOwner, userID); err != nil {
return nil, err
}
if !existingCode.UserState.Exists() {
return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-H3b2a", "Errors.User.NotFound")
}
if err := c.checkPermissionUpdateUser(ctx, existingCode.ResourceOwner, userID); err != nil {
return nil, err
}
if !existingCode.CreationAllowed() {
return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-Gg42s", "Errors.User.AlreadyInitialised")
}

16
internal/command/user_v2_invite_test.go
View File

@@ -323,7 +323,21 @@ func TestCommands_ResendInviteCode(t *testing.T) {
"missing permission",
fields{
eventstore: expectEventstore(
expectFilter(),
expectFilter(
eventFromEventPusher(
user.NewHumanAddedEvent(context.Background(),
&user.NewAggregate("userID", "org1").Aggregate,
"username", "firstName",
"lastName",
"nickName",
"displayName",
language.Afrikaans,
domain.GenderUnspecified,
"email",
false,
),
),
),
),
checkPermission: newMockPermissionCheckNotAllowed(),
},

93
internal/command/user_v2_test.go
View File

@@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/assert"
"golang.org/x/text/language"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/eventstore"
"github.com/zitadel/zitadel/internal/repository/org"
@@ -1081,13 +1082,14 @@ func TestCommandSide_ReactivateUserV2(t *testing.T) {
}
func TestCommandSide_RemoveUserV2(t *testing.T) {
ctxUserID := "ctxUserID"
ctx := authz.SetCtxData(context.Background(), authz.CtxData{UserID: ctxUserID})
type fields struct {
eventstore func(*testing.T) *eventstore.Eventstore
checkPermission domain.PermissionCheck
}
type (
args struct {
ctx context.Context
userID string
cascadingMemberships []*CascadingMembership
grantIDs []string
@@ -1110,7 +1112,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "",
},
res: res{
@@ -1128,7 +1129,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
@@ -1143,7 +1143,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewHumanAddedEvent(context.Background(),
user.NewHumanAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
"firstname",
@@ -1157,7 +1157,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
eventFromEventPusher(
user.NewUserRemovedEvent(context.Background(),
user.NewUserRemovedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
nil,
@@ -1169,7 +1169,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
@@ -1184,7 +1183,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewHumanAddedEvent(context.Background(),
user.NewHumanAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
"firstname",
@@ -1200,7 +1199,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
expectFilter(
eventFromEventPusher(
org.NewDomainPolicyAddedEvent(context.Background(),
org.NewDomainPolicyAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
true,
true,
@@ -1209,7 +1208,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
expectPush(
user.NewUserRemovedEvent(context.Background(),
user.NewUserRemovedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
nil,
@@ -1220,7 +1219,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
@@ -1235,7 +1233,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewHumanAddedEvent(context.Background(),
user.NewHumanAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
"firstname",
@@ -1249,7 +1247,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
eventFromEventPusher(
user.NewHumanInitializedCheckSucceededEvent(context.Background(),
user.NewHumanInitializedCheckSucceededEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
),
),
@@ -1258,13 +1256,10 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckNotAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
err: func(err error) bool {
return errors.Is(err, zerrors.ThrowPermissionDenied(nil, "AUTHZ-HKJD33", "Errors.PermissionDenied"))
},
err: zerrors.IsPermissionDenied,
},
},
{
@@ -1273,7 +1268,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewMachineAddedEvent(context.Background(),
user.NewMachineAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
"name",
@@ -1283,7 +1278,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
eventFromEventPusher(
user.NewUserRemovedEvent(context.Background(),
user.NewUserRemovedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
nil,
@@ -1292,10 +1287,8 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
),
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
@@ -1310,7 +1303,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewMachineAddedEvent(context.Background(),
user.NewMachineAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
"name",
@@ -1322,7 +1315,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
expectFilter(
eventFromEventPusher(
org.NewDomainPolicyAddedEvent(context.Background(),
org.NewDomainPolicyAddedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
true,
true,
@@ -1331,7 +1324,7 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
),
),
expectPush(
user.NewUserRemovedEvent(context.Background(),
user.NewUserRemovedEvent(ctx,
&user.NewAggregate("user1", "org1").Aggregate,
"username",
nil,
@@ -1342,7 +1335,6 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
checkPermission: newMockPermissionCheckAllowed(),
},
args: args{
ctx: context.Background(),
userID: "user1",
},
res: res{
@@ -1351,6 +1343,56 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
},
},
},
{
name: "remove self, ok",
fields: fields{
eventstore: expectEventstore(
expectFilter(
eventFromEventPusher(
user.NewHumanAddedEvent(ctx,
&user.NewAggregate(ctxUserID, "org1").Aggregate,
"username",
"firstname",
"lastname",
"nickname",
"displayname",
language.German,
domain.GenderUnspecified,
"email@test.ch",
true,
),
),
),
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(),
},
args: args{
userID: ctxUserID,
},
res: res{
want: &domain.ObjectDetails{
ResourceOwner: "org1",
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@@ -1358,7 +1400,8 @@ func TestCommandSide_RemoveUserV2(t *testing.T) {
eventstore: tt.fields.eventstore(t),
checkPermission: tt.fields.checkPermission,
}
got, err := r.RemoveUserV2(tt.args.ctx, tt.args.userID, "", tt.args.cascadingMemberships, tt.args.grantIDs...)
got, err := r.RemoveUserV2(ctx, tt.args.userID, "", tt.args.cascadingMemberships, tt.args.grantIDs...)
if tt.res.err == nil {
assert.NoError(t, err)
}