From 1949d1546af4ea3231204fead58546ee9e831027 Mon Sep 17 00:00:00 2001 From: Silvan <27845747+adlerhurst@users.noreply.github.com> Date: Wed, 15 Jan 2025 11:22:16 +0100 Subject: [PATCH] fix: set correct owner on project grants (#9089) # Which Problems Are Solved In versions previous to v2.66 it was possible to set a different resource owner on project grants. This was introduced with the new resource based API. The resource owner was possible to overwrite using the x-zitadel-org header. Because of this issue project grants got the wrong resource owner, instead of the owner of the project it got the granted org which is wrong because a resource owner of an aggregate is not allowed to change. # How the Problems Are Solved - The wrong owners of the events are set to the original owner of the project. - A new event is pushed to these aggregates `project.owner.corrected` - The projection updates the owners of the user grants if that event was written # Additional Changes The eventstore push function (replaced in version 2.66) writes the correct resource owner. # Additional Context closes https://github.com/zitadel/zitadel/issues/9072 --- cmd/setup/45.go | 111 +++++++++++++++++++ cmd/setup/45.sql | 79 +++++++++++++ cmd/setup/config.go | 1 + cmd/setup/setup.go | 2 + docs/docs/support/advisory/a10014.md | 26 +++++ docs/docs/support/technical_advisory.mdx | 12 ++ internal/eventstore/handler/v2/statement.go | 6 + internal/eventstore/v3/sequence.go | 13 ++- internal/query/projection/project_grant.go | 18 +++ internal/repository/owner/owner_corrected.go | 40 +++++++ internal/repository/project/project.go | 1 + 11 files changed, 308 insertions(+), 1 deletion(-) create mode 100644 cmd/setup/45.go create mode 100644 cmd/setup/45.sql create mode 100644 docs/docs/support/advisory/a10014.md create mode 100644 internal/repository/owner/owner_corrected.go diff --git a/cmd/setup/45.go b/cmd/setup/45.go new file mode 100644 index 0000000000..d8318a6d59 --- /dev/null +++ b/cmd/setup/45.go @@ -0,0 +1,111 @@ +package setup + +import ( + "context" + _ "embed" + "encoding/json" + "fmt" + + "github.com/zitadel/logging" + + "github.com/zitadel/zitadel/internal/api/authz" + "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/query/projection" + "github.com/zitadel/zitadel/internal/repository/instance" + "github.com/zitadel/zitadel/internal/repository/owner" + "github.com/zitadel/zitadel/internal/repository/project" +) + +var ( + //go:embed 45.sql + correctProjectOwnerEvents string +) + +type CorrectProjectOwners struct { + eventstore *eventstore.Eventstore +} + +func (mig *CorrectProjectOwners) Execute(ctx context.Context, _ eventstore.Event) error { + instances, err := mig.eventstore.InstanceIDs( + ctx, + eventstore.NewSearchQueryBuilder(eventstore.ColumnsInstanceIDs). + OrderDesc(). + AddQuery(). + AggregateTypes("instance"). + EventTypes(instance.InstanceAddedEventType). + Builder(), + ) + if err != nil { + return err + } + + ctx = authz.SetCtxData(ctx, authz.CtxData{UserID: "SETUP"}) + for i, instance := range instances { + ctx = authz.WithInstanceID(ctx, instance) + logging.WithFields("instance_id", instance, "migration", mig.String(), "progress", fmt.Sprintf("%d/%d", i+1, len(instances))).Info("correct owners of projects") + didCorrect, err := mig.correctInstanceProjects(ctx, instance) + if err != nil { + return err + } + if !didCorrect { + continue + } + _, err = projection.ProjectGrantProjection.Trigger(ctx) + logging.OnError(err).Debug("failed triggering project grant projection to update owners") + } + return nil +} + +func (mig *CorrectProjectOwners) correctInstanceProjects(ctx context.Context, instance string) (didCorrect bool, err error) { + var correctedOwners []eventstore.Command + + tx, err := mig.eventstore.Client().BeginTx(ctx, nil) + if err != nil { + return false, err + } + defer func() { + if err != nil { + _ = tx.Rollback() + return + } + err = tx.Commit() + }() + + rows, err := tx.QueryContext(ctx, correctProjectOwnerEvents, instance) + if err != nil { + return false, err + } + defer rows.Close() + + for rows.Next() { + aggregate := &eventstore.Aggregate{ + InstanceID: instance, + Type: project.AggregateType, + Version: project.AggregateVersion, + } + var payload json.RawMessage + err := rows.Scan( + &aggregate.ID, + &aggregate.ResourceOwner, + &payload, + ) + if err != nil { + return false, err + } + previousOwners := make(map[uint32]string) + if err := json.Unmarshal(payload, &previousOwners); err != nil { + return false, err + } + correctedOwners = append(correctedOwners, owner.NewCorrected(ctx, aggregate, previousOwners)) + } + if rows.Err() != nil { + return false, rows.Err() + } + + _, err = mig.eventstore.PushWithClient(ctx, tx, correctedOwners...) + return len(correctedOwners) > 0, err +} + +func (*CorrectProjectOwners) String() string { + return "43_correct_project_owners" +} diff --git a/cmd/setup/45.sql b/cmd/setup/45.sql new file mode 100644 index 0000000000..0e90a2683d --- /dev/null +++ b/cmd/setup/45.sql @@ -0,0 +1,79 @@ +WITH corrupt_streams AS ( + select + e.instance_id + , e.aggregate_type + , e.aggregate_id + , min(e.sequence) as min_sequence + , count(distinct e.owner) as owner_count + from + eventstore.events2 e + where + e.instance_id = $1 + and aggregate_type = 'project' + group by + e.instance_id + , e.aggregate_type + , e.aggregate_id + having + count(distinct e.owner) > 1 +), correct_owners AS ( + select + e.instance_id + , e.aggregate_type + , e.aggregate_id + , e.owner + from + eventstore.events2 e + join + corrupt_streams cs + on + e.instance_id = cs.instance_id + and e.aggregate_type = cs.aggregate_type + and e.aggregate_id = cs.aggregate_id + and e.sequence = cs.min_sequence +), wrong_events AS ( + select + e.instance_id + , e.aggregate_type + , e.aggregate_id + , e.sequence + , e.owner wrong_owner + , co.owner correct_owner + from + eventstore.events2 e + join + correct_owners co + on + e.instance_id = co.instance_id + and e.aggregate_type = co.aggregate_type + and e.aggregate_id = co.aggregate_id + and e.owner <> co.owner +), updated_events AS ( + UPDATE eventstore.events2 e + SET owner = we.correct_owner + FROM + wrong_events we + WHERE + e.instance_id = we.instance_id + and e.aggregate_type = we.aggregate_type + and e.aggregate_id = we.aggregate_id + and e.sequence = we.sequence + RETURNING + we.aggregate_id + , we.correct_owner + , we.sequence + , we.wrong_owner +) +SELECT + ue.aggregate_id + , ue.correct_owner + , jsonb_object_agg( + ue.sequence::TEXT --formant to string because crdb is not able to handle int + , ue.wrong_owner + ) payload +FROM + updated_events ue +GROUP BY + ue.aggregate_id + , ue.correct_owner +; diff --git a/cmd/setup/config.go b/cmd/setup/config.go index 9f34c2baa5..0a5493b771 100644 --- a/cmd/setup/config.go +++ b/cmd/setup/config.go @@ -130,6 +130,7 @@ type Steps struct { s42Apps7OIDCConfigsLoginVersion *Apps7OIDCConfigsLoginVersion s43CreateFieldsDomainIndex *CreateFieldsDomainIndex s44ReplaceCurrentSequencesIndex *ReplaceCurrentSequencesIndex + s45CorrectProjectOwners *CorrectProjectOwners } func MustNewSteps(v *viper.Viper) *Steps { diff --git a/cmd/setup/setup.go b/cmd/setup/setup.go index 4ffef441af..d55ea0f3fe 100644 --- a/cmd/setup/setup.go +++ b/cmd/setup/setup.go @@ -173,6 +173,7 @@ func Setup(ctx context.Context, config *Config, steps *Steps, masterKey string) steps.s42Apps7OIDCConfigsLoginVersion = &Apps7OIDCConfigsLoginVersion{dbClient: esPusherDBClient} steps.s43CreateFieldsDomainIndex = &CreateFieldsDomainIndex{dbClient: queryDBClient} steps.s44ReplaceCurrentSequencesIndex = &ReplaceCurrentSequencesIndex{dbClient: esPusherDBClient} + steps.s45CorrectProjectOwners = &CorrectProjectOwners{eventstore: eventstoreClient} err = projection.Create(ctx, projectionDBClient, eventstoreClient, config.Projections, nil, nil, nil) logging.OnError(err).Fatal("unable to start projections") @@ -227,6 +228,7 @@ func Setup(ctx context.Context, config *Config, steps *Steps, masterKey string) steps.s36FillV2Milestones, steps.s38BackChannelLogoutNotificationStart, steps.s44ReplaceCurrentSequencesIndex, + steps.s45CorrectProjectOwners, } { mustExecuteMigration(ctx, eventstoreClient, step, "migration failed") } diff --git a/docs/docs/support/advisory/a10014.md b/docs/docs/support/advisory/a10014.md new file mode 100644 index 0000000000..be19dd2cbf --- /dev/null +++ b/docs/docs/support/advisory/a10014.md @@ -0,0 +1,26 @@ +--- +title: Technical Advisory 10014 +--- + +## Date + +Versions: >= v2.67.3, v2.66 >= v2.66.6 + +Date: 2025-01-17 + +## Description + +Prior to version [v2.66.0](https://github.com/zitadel/zitadel/releases/tag/v2.66.0), some project grants were incorrectly created under the granted organization instead of the project owner's organization. To find these grants, users had to set the `x-zitadel-orgid` header to the granted organization ID when using the [`ListAllProjectGrants`](/apis/resources/mgmt/management-service-add-project-grant) gRPC method. + +Zitadel [v2.66.0](https://github.com/zitadel/zitadel/releases/tag/v2.66.0) corrected this behavior for new grants. However, existing grants were not automatically updated. Version v2.66.6 corrects the owner of these existing grants. + +## Impact + +After the release of v2.66.6, if your application uses the [`ListAllProjectGrants`](/apis/resources/mgmt/management-service-add-project-grant) method with the `x-zitadel-orgid` header set to the granted organization ID, you will not retrieve any results. + +## Mitigation + +To ensure your application continues to function correctly after the release of v2.66.6, implement the following changes: + +1. **Conditional Header:** Only set the `x-zitadel-orgid` header to the project owner's organization ID if the user executing the [`ListAllProjectGrants`](/apis/resources/mgmt/management-service-add-project-grant) method belongs to a different organization than the project. +2. **Use `grantedOrgIdQuery`:** Utilize the `grantedOrgIdQuery` parameter to filter grants for the specific granted organization. \ No newline at end of file diff --git a/docs/docs/support/technical_advisory.mdx b/docs/docs/support/technical_advisory.mdx index 7562ff3870..8805e2e1d8 100644 --- a/docs/docs/support/technical_advisory.mdx +++ b/docs/docs/support/technical_advisory.mdx @@ -214,6 +214,18 @@ We understand that these advisories may include breaking changes, and we aim to - 2024-12-09 + + + A-10014 + + Correction of project grant owner + Breaking Behavior Change + + Correct project grant owners, ensuring they are correctly associated with the projects organization. + + - + 2025-01-10 + ## Subscribe to our Mailing List diff --git a/internal/eventstore/handler/v2/statement.go b/internal/eventstore/handler/v2/statement.go index 961881d24b..a02e5d3580 100644 --- a/internal/eventstore/handler/v2/statement.go +++ b/internal/eventstore/handler/v2/statement.go @@ -601,6 +601,12 @@ func NewCond(name string, value interface{}) Condition { } } +func NewUnequalCond(name string, value any) Condition { + return func(param string) (string, []any) { + return name + " <> " + param, []any{value} + } +} + func NewNamespacedCondition(name string, value interface{}) NamespacedCondition { return func(namespace string) Condition { return NewCond(namespace+"."+name, value) diff --git a/internal/eventstore/v3/sequence.go b/internal/eventstore/v3/sequence.go index 7d97e1080d..1976af4093 100644 --- a/internal/eventstore/v3/sequence.go +++ b/internal/eventstore/v3/sequence.go @@ -125,7 +125,18 @@ func scanToSequence(rows *sql.Rows, sequences []*latestSequence) error { return nil } sequence.sequence = currentSequence - if sequence.aggregate.ResourceOwner == "" { + if resourceOwner != "" && sequence.aggregate.ResourceOwner != "" && sequence.aggregate.ResourceOwner != resourceOwner { + logging.WithFields( + "current_sequence", sequence.sequence, + "instance_id", sequence.aggregate.InstanceID, + "agg_type", sequence.aggregate.Type, + "agg_id", sequence.aggregate.ID, + "current_owner", resourceOwner, + "provided_owner", sequence.aggregate.ResourceOwner, + ).Info("would have set wrong resource owner") + } + // set resource owner from previous events + if resourceOwner != "" { sequence.aggregate.ResourceOwner = resourceOwner } diff --git a/internal/query/projection/project_grant.go b/internal/query/projection/project_grant.go index d6fbde8556..d5a075c486 100644 --- a/internal/query/projection/project_grant.go +++ b/internal/query/projection/project_grant.go @@ -93,6 +93,10 @@ func (p *projectGrantProjection) Reducers() []handler.AggregateReducer { Event: project.ProjectRemovedType, Reduce: p.reduceProjectRemoved, }, + { + Event: project.ProjectOwnerCorrected, + Reduce: p.reduceOwnerCorrected, + }, }, }, { @@ -269,3 +273,17 @@ func (p *projectGrantProjection) reduceOwnerRemoved(event eventstore.Event) (*ha ), ), nil } + +func (p *projectGrantProjection) reduceOwnerCorrected(event eventstore.Event) (*handler.Statement, error) { + return handler.NewUpdateStatement( + event, + []handler.Column{ + handler.NewCol(ProjectGrantColumnResourceOwner, event.Aggregate().ResourceOwner), + }, + []handler.Condition{ + handler.NewCond(ProjectGrantColumnInstanceID, event.Aggregate().InstanceID), + handler.NewCond(ProjectGrantColumnProjectID, event.Aggregate().ID), + handler.NewUnequalCond(ProjectGrantColumnResourceOwner, event.Aggregate().ResourceOwner), + }, + ), nil +} diff --git a/internal/repository/owner/owner_corrected.go b/internal/repository/owner/owner_corrected.go new file mode 100644 index 0000000000..29bb4842d4 --- /dev/null +++ b/internal/repository/owner/owner_corrected.go @@ -0,0 +1,40 @@ +package owner + +import ( + "context" + + "github.com/zitadel/zitadel/internal/eventstore" +) + +const OwnerCorrectedType = ".owner.corrected" + +type Corrected struct { + eventstore.BaseEvent `json:"-"` + + PreviousOwners map[uint32]string `json:"previousOwners,omitempty"` +} + +var _ eventstore.Command = (*Corrected)(nil) + +func (e *Corrected) Payload() interface{} { + return e +} + +func (e *Corrected) UniqueConstraints() []*eventstore.UniqueConstraint { + return nil +} + +func NewCorrected( + ctx context.Context, + aggregate *eventstore.Aggregate, + previousOwners map[uint32]string, +) *Corrected { + return &Corrected{ + BaseEvent: *eventstore.NewBaseEventForPush( + ctx, + aggregate, + eventstore.EventType(aggregate.Type+OwnerCorrectedType), + ), + PreviousOwners: previousOwners, + } +} diff --git a/internal/repository/project/project.go b/internal/repository/project/project.go index 6147a632eb..44f882b3e1 100644 --- a/internal/repository/project/project.go +++ b/internal/repository/project/project.go @@ -16,6 +16,7 @@ const ( ProjectDeactivatedType = projectEventTypePrefix + "deactivated" ProjectReactivatedType = projectEventTypePrefix + "reactivated" ProjectRemovedType = projectEventTypePrefix + "removed" + ProjectOwnerCorrected = projectEventTypePrefix + "owner.corrected" ProjectSearchType = "project" ProjectObjectRevision = uint8(1)