mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-29 10:32:15 +00:00
Merge commit from fork
* Use slices.Contains over custom function * Correctly remove roles from granted roles * fix(setup): repair user grants with stale roles (GHSA-v859-c572-qh5p) Add setup step 73 that reconciles existing user grants whose roles were left too broad by the buggy cascade removal in removeRoleFromUserGrant. The corruption lives in the eventstore event payloads, so the step pushes a corrective user.grant.cascade.changed event per affected grant (roles intersected with the currently valid set) and re-triggers the user grant projection. Runs in the second setup slice, after the projection tables it reads have been created. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(setup): scope GHSA-v859-c572-qh5p repair to grant-based user grants Direct user grants can never be hit by this bug (only ChangeProjectGrant's multi-role cascade to grant-based grants can trigger it), so drop the direct-grant branch from the finder query to avoid stripping unrelated, legitimate roles that merely mismatch for other reasons (e.g. stale role_key drift). Also exclude removed instances from the migration scope, and log the number of grants fixed per instance. Co-authored-by: Cursor <[email protected]> --------- Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: Livio Spring <[email protected]> Co-authored-by: Cursor <[email protected]>
This commit is contained in:
co-authored by
Cursor
Claude Opus 4.8
Livio Spring
parent
8b9d3637fd
commit
9d60e83d6f
@@ -193,7 +193,7 @@ func (c *Commands) RemoveProjectRole(ctx context.Context, projectID, key, resour
|
||||
}
|
||||
|
||||
for _, grantID := range cascadeUserGrantIDs {
|
||||
event, err := c.removeRoleFromUserGrant(ctx, grantID, []string{key}, true)
|
||||
event, err := c.removeRoleFromUserGrant(ctx, grantID, map[string]bool{key:true}, true)
|
||||
if err != nil {
|
||||
logging.LogWithFields("COMMAND-mK0of", "usergrantid", grantID).WithError(err).Warn("could not cascade remove role on user grant")
|
||||
continue
|
||||
|
||||
@@ -111,7 +111,7 @@ func (c *Commands) ChangeUserGrant(ctx context.Context, userGrant *domain.UserGr
|
||||
return userGrantWriteModelToUserGrant(existingUserGrant), nil
|
||||
}
|
||||
|
||||
func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID string, roleKeys []string, cascade bool) (_ eventstore.Command, err error) {
|
||||
func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID string, roleKeysToRemove map[string]bool, cascade bool) (_ eventstore.Command, err error) {
|
||||
existingUserGrant, err := c.userGrantWriteModelByID(ctx, userGrantID, "")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -119,21 +119,16 @@ func (c *Commands) removeRoleFromUserGrant(ctx context.Context, userGrantID stri
|
||||
if existingUserGrant.State == domain.UserGrantStateUnspecified || existingUserGrant.State == domain.UserGrantStateRemoved {
|
||||
return nil, zerrors.ThrowNotFound(nil, "COMMAND-3M9sd", "Errors.UserGrant.NotFound")
|
||||
}
|
||||
keyExists := false
|
||||
for i, key := range existingUserGrant.RoleKeys {
|
||||
for _, roleKey := range roleKeys {
|
||||
if key == roleKey {
|
||||
keyExists = true
|
||||
copy(existingUserGrant.RoleKeys[i:], existingUserGrant.RoleKeys[i+1:])
|
||||
existingUserGrant.RoleKeys[len(existingUserGrant.RoleKeys)-1] = ""
|
||||
existingUserGrant.RoleKeys = existingUserGrant.RoleKeys[:len(existingUserGrant.RoleKeys)-1]
|
||||
continue
|
||||
}
|
||||
}
|
||||
}
|
||||
if !keyExists {
|
||||
|
||||
beforeLen := len(existingUserGrant.RoleKeys)
|
||||
existingUserGrant.RoleKeys = slices.DeleteFunc(existingUserGrant.RoleKeys, func(role string) bool {
|
||||
return roleKeysToRemove[role]
|
||||
})
|
||||
|
||||
if beforeLen == len(existingUserGrant.RoleKeys) {
|
||||
return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-5m8g9", "Errors.UserGrant.RoleKeyNotFound")
|
||||
}
|
||||
|
||||
changedUserGrant := NewUserGrantWriteModel(userGrantID, existingUserGrant.ResourceOwner)
|
||||
userGrantAgg := UserGrantAggregateFromWriteModel(&changedUserGrant.WriteModel)
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/text/language"
|
||||
|
||||
"github.com/zitadel/zitadel/internal/api/authz"
|
||||
@@ -3209,3 +3210,74 @@ func TestCommandSide_BulkRemoveUserGrant(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// PoC for GHSA-v859-c572-qh5p
|
||||
// Each case asserts the CORRECT post-removal RoleKeys;
|
||||
// against faulty implementations these assertions fail.
|
||||
func TestCommandSide_removeRoleFromUserGrant(t *testing.T) {
|
||||
t.Parallel()
|
||||
tests := []struct {
|
||||
name string
|
||||
existingRoleKeys []string
|
||||
roleKeysToRemove map[string]bool
|
||||
want []string
|
||||
}{
|
||||
{
|
||||
name: "advisory scenario, remove two adjacent leading roles",
|
||||
existingRoleKeys: []string{"admin", "viewer", "editor"},
|
||||
roleKeysToRemove: map[string]bool{"admin":true, "viewer":true},
|
||||
want: []string{"editor"},
|
||||
},
|
||||
{
|
||||
name: "remove first two of four",
|
||||
existingRoleKeys: []string{"a", "b", "c", "d"},
|
||||
roleKeysToRemove: map[string]bool{"a":true, "b":true},
|
||||
want: []string{"c", "d"},
|
||||
},
|
||||
{
|
||||
name: "remove two adjacent middle roles",
|
||||
existingRoleKeys: []string{"a", "b", "c", "d"},
|
||||
roleKeysToRemove: map[string]bool{"b":true, "c":true},
|
||||
want: []string{"a", "d"},
|
||||
},
|
||||
{
|
||||
name: "remove two non-adjacent roles",
|
||||
existingRoleKeys: []string{"a", "b", "c", "d"},
|
||||
roleKeysToRemove: map[string]bool{"a":true, "c":true},
|
||||
want: []string{"b", "d"},
|
||||
},
|
||||
{
|
||||
name: "remove all roles",
|
||||
existingRoleKeys: []string{"x", "y", "z"},
|
||||
roleKeysToRemove: map[string]bool{"x":true, "y":true, "z":true},
|
||||
want: []string{},
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
r := &Commands{
|
||||
eventstore: expectEventstore(
|
||||
expectFilter(
|
||||
eventFromEventPusher(
|
||||
usergrant.NewUserGrantAddedEvent(t.Context(),
|
||||
&usergrant.NewAggregate("usergrant1", "org1").Aggregate,
|
||||
"user1",
|
||||
"project1",
|
||||
"",
|
||||
tt.existingRoleKeys,
|
||||
),
|
||||
),
|
||||
),
|
||||
)(t),
|
||||
}
|
||||
|
||||
cmd, err := r.removeRoleFromUserGrant(t.Context(), "usergrant1", tt.roleKeysToRemove, true)
|
||||
require.NoError(t, err)
|
||||
|
||||
event, ok := cmd.(*usergrant.UserGrantCascadeChangedEvent)
|
||||
require.True(t, ok, "expected *usergrant.UserGrantCascadeChangedEvent, got %T", cmd)
|
||||
assert.Equal(t, tt.want, event.RoleKeys)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user