From ffecf9f01e16d0e6d6048a7115edfa6134a0529d Mon Sep 17 00:00:00 2001 From: Javier David Carnelli Date: Thu, 23 Jul 2026 01:13:35 -0300 Subject: [PATCH 1/5] CXP-768 Gate account re-parenting on org/OU sync opt-in accountResourceType.List() re-parented every account onto its Organizations Root/OU whenever organizations:ListParents succeeded, regardless of whether the organization/organizational_unit resource types (OptInRequired, off by default) were actually being synced. In the default orgs+sso configuration this left every account with a parent pointing at a resource that was never synced, and c1 silently drops the reference as a dangling MISSING RESOURCE. Thread cli.ConnectorOpts through New() (previously discarded) to learn whether this sync run will actually emit organization/ organizational_unit, and gate both the ListParents call and the resulting parent attachment on it -- including the resolved parent's specific type, so partial opt-in can't produce a dangling parent either. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/account.go | 58 ++++++++++----- pkg/connector/connector.go | 29 ++++++-- pkg/connector/organization_test.go | 71 ++++++++++++++++++- ...permission_set_assignment_behavior_test.go | 5 +- 4 files changed, 139 insertions(+), 24 deletions(-) diff --git a/pkg/connector/account.go b/pkg/connector/account.go index 2fc6ff5a..89019478 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -129,6 +129,14 @@ type accountResourceType struct { identityInstance *awsSsoAdminTypes.InstanceMetadata identityClient client.IdentityStoreClient region string + + // willSyncOrganization/willSyncOrganizationalUnit report whether this sync run will + // actually sync the corresponding (OptInRequired) hierarchy resource type. Account + // re-parenting (see List) is gated on these so accounts never point at a Root/OU + // resource that this run never syncs, which would otherwise leave a dangling + // "MISSING RESOURCE" parent. See CXP-768. + willSyncOrganization bool + willSyncOrganizationalUnit bool } func (o *accountResourceType) ResourceType(_ context.Context) *v2.ResourceType { @@ -190,15 +198,29 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r // Sparse ACLs hierarchy (Phase 2): re-parent the account under its Root/OU so c1's // by-inheritance review can walk Account → OU → Root with the role pinned. Fail-soft: // without organizations:ListParents the account stays flat (parentless) and we WARN once. - parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) - if err != nil { - return nil, nil, err - } - if accessDenied { - orgReadDenied = true - } - if parentID != nil { - resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) + // + // Gated on willSyncOrganization/willSyncOrganizationalUnit (CXP-768): organization and + // organizational_unit are OptInRequired, so a sync run may never emit them. Pointing an + // account's parent at a resource type this run isn't syncing produces a dangling + // "MISSING RESOURCE" parent that c1 silently drops (see permissionSetRoleID's comment on + // dangling references). Skip the ListParents call entirely when neither hierarchy type + // will be synced; when only one is, still resolve the parent but only attach it if its + // resolved type is one this run will actually sync. + if o.willSyncOrganization || o.willSyncOrganizationalUnit { + parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) + if err != nil { + return nil, nil, err + } + if accessDenied { + orgReadDenied = true + } + if parentID != nil { + willSyncParentType := (parentID.ResourceType == resourceTypeOrganization.Id && o.willSyncOrganization) || + (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit) + if willSyncParentType { + resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) + } + } } resourceOpts = append(resourceOpts, resourceSdk.WithResourceProfile(profile)) @@ -952,15 +974,19 @@ func accountBuilder( identityInstance *awsSsoAdminTypes.InstanceMetadata, region string, identityClient client.IdentityStoreClient, + willSyncOrganization bool, + willSyncOrganizationalUnit bool, ) *accountResourceType { return &accountResourceType{ - resourceType: resourceTypeAccount, - orgClient: orgClient, - roleArn: roleArn, - ssoAdminClient: ssoAdminClient, - identityClient: identityClient, - identityInstance: identityInstance, - region: region, + resourceType: resourceTypeAccount, + orgClient: orgClient, + roleArn: roleArn, + ssoAdminClient: ssoAdminClient, + identityClient: identityClient, + identityInstance: identityInstance, + region: region, + willSyncOrganization: willSyncOrganization, + willSyncOrganizationalUnit: willSyncOrganizationalUnit, } } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 8599d415..2547b16f 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -96,6 +96,12 @@ type AWS struct { cloudTrailClient *cloudtrail.Client assumeRoleWithWebIdentity func(context.Context, *sts.AssumeRoleWithWebIdentityInput) (*sts.AssumeRoleWithWebIdentityOutput, error) + // willSyncOrganization/willSyncOrganizationalUnit report whether this sync run's + // resource-type filter (if any) includes the OptInRequired org/OU hierarchy types. + // See accountResourceType.List (CXP-768). + willSyncOrganization bool + willSyncOrganizationalUnit bool + syncSecrets bool syncSSOUserLastLogin bool syncOnlyAttachedPolicies bool @@ -242,15 +248,24 @@ func validateConfig(awsc *cfg.Aws) error { return nil } -func New(ctx context.Context, awsc *cfg.Aws, opts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { +func New(ctx context.Context, awsc *cfg.Aws, connectorOpts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { l := ctxzap.Extract(ctx) + // Default to "will sync" when the caller gave no explicit resource-type filter (or no + // ConnectorOpts at all), matching ConnectorOpts.WillSyncResourceType's own default. + willSyncOrganization := true + willSyncOrganizationalUnit := true + if connectorOpts != nil { + willSyncOrganization = connectorOpts.WillSyncResourceType(resourceTypeOrganization.Id) + willSyncOrganizationalUnit = connectorOpts.WillSyncResourceType(resourceTypeOrganizationalUnit.Id) + } + // syncIAMPolicyGrants gates the cross-type iam_policy grants that iamUser, role, // iamGroup, and permissionSet emit as a sync optimization. When the caller has // explicitly filtered resource types and iam_policy isn't among them, emitting // those grants is invalid/wasteful. A nil opts (e.g. capabilities generation or // callers that don't pass one) is treated the same as "no filter" -> sync everything. - syncIAMPolicyGrants := opts == nil || opts.WillSyncResourceType("iam_policy") + syncIAMPolicyGrants := connectorOpts == nil || connectorOpts.WillSyncResourceType("iam_policy") err := field.Validate(cfg.Config, awsc) if err != nil { @@ -320,6 +335,9 @@ func New(ctx context.Context, awsc *cfg.Aws, opts *cli.ConnectorOpts) (connector syncIAMPolicyGrants: syncIAMPolicyGrants, accountProvisioningTarget: config.AccountProvisioningTarget, + + willSyncOrganization: willSyncOrganization, + willSyncOrganizationalUnit: willSyncOrganizationalUnit, } rv.awsClientFactory = NewAWSClientFactory(config, rv, httpClient) @@ -482,7 +500,8 @@ func (c *AWS) ResourceSyncers(ctx context.Context) []connectorbuilder.ResourceSy if c.orgsEnabled && c.ssoEnabled { l.Debug("orgsEnabled. creating accountBuilder") - acct := accountBuilder(c.orgClient, c.roleARN, c.ssoAdminClient, c.identityInstance, c.ssoRegion, c.identityStoreClient) + acct := accountBuilder(c.orgClient, c.roleARN, c.ssoAdminClient, c.identityInstance, c.ssoRegion, c.identityStoreClient, + c.willSyncOrganization, c.willSyncOrganizationalUnit) rs = append(rs, acct, // Sparse ACLs (Cloud Infrastructure Access): permission set as role, and the @@ -528,9 +547,9 @@ func (d *defaultCapabilitiesBuilder) ResourceSyncers(_ context.Context) []connec inlinePolicyBuilder(nil, nil, nil, nil), ssoUserBuilder("", nil, nil, nil, nil), ssoGroupBuilder("", nil, nil, nil), - accountBuilder(nil, "", nil, nil, "", nil), + accountBuilder(nil, "", nil, nil, "", nil, true, true), permissionSetBuilder(nil, nil, true), - permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil)), + permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil, true, true)), organizationBuilder(nil), organizationalUnitBuilder(nil), accountIAMBuilder(nil, nil, nil), diff --git a/pkg/connector/organization_test.go b/pkg/connector/organization_test.go index 59e721be..df97fd02 100644 --- a/pkg/connector/organization_test.go +++ b/pkg/connector/organization_test.go @@ -264,12 +264,79 @@ func TestAccountList_FlatWhenOrgReadDenied(t *testing.T) { assert.Nil(t, resources[0].ParentResourceId, "account stays flat when re-parenting is denied") } +// account.List must not call ListParents at all, and must leave the account flat, when neither +// hierarchy type is opted into this sync run — otherwise the account gets a parent pointing at +// a resource type that will never be synced ("MISSING RESOURCE"). Regression test for CXP-768. +func TestAccountList_SkipsReparentWhenHierarchyNotSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return &awsOrgs.ListParentsOutput{Parents: []awsOrgsTypes.Parent{{ + Id: awsSdk.String(testOUID), + Type: awsOrgsTypes.ParentTypeOrganizationalUnit, + }}}, nil + }, + } + acct := newOrgAccountWithSyncFilter(orgs, false, false) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when hierarchy isn't synced") + assert.Equal(t, 0, orgs.listParentsCalls, "must not call ListParents when neither hierarchy type is synced") +} + +// account.List resolves the parent but only attaches it when the resolved parent's specific +// type (organization vs organizational_unit) is one this run will actually sync — partial +// opt-in must not produce a dangling parent either. +func TestAccountList_SkipsReparentWhenResolvedParentTypeNotSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return &awsOrgs.ListParentsOutput{Parents: []awsOrgsTypes.Parent{{ + Id: awsSdk.String(testOUID), + Type: awsOrgsTypes.ParentTypeOrganizationalUnit, + }}}, nil + }, + } + // organization is opted in but organizational_unit is not; the account's actual parent + // resolves to an OU, so it must still be left flat. + acct := newOrgAccountWithSyncFilter(orgs, true, false) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when its resolved parent type isn't synced") + assert.Equal(t, 1, orgs.listParentsCalls, "must still resolve the parent when at least one hierarchy type is synced") +} + // newOrgAccount builds an accountResourceType backed by the given fakeOrgs (SSO client unused -// by the List/re-parent path under test). +// by the List/re-parent path under test), with the org/OU hierarchy types opted into this sync. func newOrgAccount(orgs *fakeOrgs) *accountResourceType { + return newOrgAccountWithSyncFilter(orgs, true, true) +} + +// newOrgAccountWithSyncFilter is like newOrgAccount but lets the test control whether the +// organization/organizational_unit hierarchy types are opted into this sync run, to exercise +// the CXP-768 re-parenting gate. +func newOrgAccountWithSyncFilter(orgs *fakeOrgs, willSyncOrganization, willSyncOrganizationalUnit bool) *accountResourceType { identityInstance := &awsSsoAdminTypes.InstanceMetadata{ InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil) + return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil, willSyncOrganization, willSyncOrganizationalUnit) } diff --git a/pkg/connector/permission_set_assignment_behavior_test.go b/pkg/connector/permission_set_assignment_behavior_test.go index dc860e62..7ab4ed09 100644 --- a/pkg/connector/permission_set_assignment_behavior_test.go +++ b/pkg/connector/permission_set_assignment_behavior_test.go @@ -162,6 +162,8 @@ type fakeOrgs struct { listOUsFn func(*awsOrgs.ListOrganizationalUnitsForParentInput) (*awsOrgs.ListOrganizationalUnitsForParentOutput, error) listParentsFn func(*awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) describeAccountFn func(*awsOrgs.DescribeAccountInput) (*awsOrgs.DescribeAccountOutput, error) + + listParentsCalls int } func (f *fakeOrgs) DescribeAccount(_ context.Context, in *awsOrgs.DescribeAccountInput, _ ...func(*awsOrgs.Options)) (*awsOrgs.DescribeAccountOutput, error) { @@ -197,6 +199,7 @@ func (f *fakeOrgs) ListOrganizationalUnitsForParent( } func (f *fakeOrgs) ListParents(_ context.Context, in *awsOrgs.ListParentsInput, _ ...func(*awsOrgs.Options)) (*awsOrgs.ListParentsOutput, error) { + f.listParentsCalls++ if f.listParentsFn != nil { return f.listParentsFn(in) } @@ -215,7 +218,7 @@ func newBehaviorAccount(sso *fakeSSOAdmin) *accountResourceType { InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}) + return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}, true, true) } func behaviorBinding(t *testing.T) (*v2.Resource, *v2.Entitlement) { From 833196ad9ae84969c613f7cefaed26dff572b06d Mon Sep 17 00:00:00 2001 From: Javier David Carnelli Date: Mon, 27 Jul 2026 12:10:38 -0300 Subject: [PATCH 2/5] fix: also validates if the root orgs are being synced before ataching parentIDs to accounts --- pkg/connector/account.go | 10 +++++++--- pkg/connector/connector.go | 2 +- pkg/connector/organization_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/connector/account.go b/pkg/connector/account.go index 89019478..f75be008 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -134,7 +134,7 @@ type accountResourceType struct { // actually sync the corresponding (OptInRequired) hierarchy resource type. Account // re-parenting (see List) is gated on these so accounts never point at a Root/OU // resource that this run never syncs, which would otherwise leave a dangling - // "MISSING RESOURCE" parent. See CXP-768. + // "MISSING RESOURCE" parent. willSyncOrganization bool willSyncOrganizationalUnit bool } @@ -199,7 +199,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r // by-inheritance review can walk Account → OU → Root with the role pinned. Fail-soft: // without organizations:ListParents the account stays flat (parentless) and we WARN once. // - // Gated on willSyncOrganization/willSyncOrganizationalUnit (CXP-768): organization and + // Gated on willSyncOrganization/willSyncOrganizationalUnit: organization and // organizational_unit are OptInRequired, so a sync run may never emit them. Pointing an // account's parent at a resource type this run isn't syncing produces a dangling // "MISSING RESOURCE" parent that c1 silently drops (see permissionSetRoleID's comment on @@ -215,8 +215,12 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r orgReadDenied = true } if parentID != nil { + // OU sync is itself contingent on organization sync: the OU crawl is seeded + // exclusively from Root (see organization.go), so an OU can never actually be + // synced when organization is not. Require willSyncOrganization here too, or a + // partial opt-in (OU in, org out) would attach a parent that never gets emitted. willSyncParentType := (parentID.ResourceType == resourceTypeOrganization.Id && o.willSyncOrganization) || - (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit) + (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit && o.willSyncOrganization) if willSyncParentType { resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 2547b16f..923a2dd9 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -98,7 +98,7 @@ type AWS struct { // willSyncOrganization/willSyncOrganizationalUnit report whether this sync run's // resource-type filter (if any) includes the OptInRequired org/OU hierarchy types. - // See accountResourceType.List (CXP-768). + // See accountResourceType.List. willSyncOrganization bool willSyncOrganizationalUnit bool diff --git a/pkg/connector/organization_test.go b/pkg/connector/organization_test.go index df97fd02..8e2c7c3b 100644 --- a/pkg/connector/organization_test.go +++ b/pkg/connector/organization_test.go @@ -46,7 +46,7 @@ func TestOrganizationList_EmitsRoots(t *testing.T) { // organizationResource must attach a ChildResourceType annotation to the emitted resource // instance — this is what the SDK's syncer actually reads to schedule the organizational_unit // child crawl (a ResourceType-level annotation alone does not drive dispatch). Regression test -// for CXP-756, where this was missing and the OU tier silently never synced. +// for the case where this was missing and the OU tier silently never synced. func TestOrganizationResource_EmitsChildResourceTypeAnnotation(t *testing.T) { r, err := organizationResource(awsOrgsTypes.Root{Id: awsSdk.String(testRootID)}) require.NoError(t, err) @@ -62,7 +62,7 @@ func TestOrganizationResource_EmitsChildResourceTypeAnnotation(t *testing.T) { } // organizationalUnitResource must attach a ChildResourceType annotation pointing at itself so -// the SDK recurses into nested OUs. Regression test for CXP-756. +// the SDK recurses into nested OUs. Regression test for the same issue as above. func TestOrganizationalUnitResource_EmitsChildResourceTypeAnnotation(t *testing.T) { parent := &v2.ResourceId{ResourceType: resourceTypeOrganization.Id, Resource: testRootID} r, err := organizationalUnitResource(awsOrgsTypes.OrganizationalUnit{Id: awsSdk.String(testOUID)}, parent) @@ -266,7 +266,7 @@ func TestAccountList_FlatWhenOrgReadDenied(t *testing.T) { // account.List must not call ListParents at all, and must leave the account flat, when neither // hierarchy type is opted into this sync run — otherwise the account gets a parent pointing at -// a resource type that will never be synced ("MISSING RESOURCE"). Regression test for CXP-768. +// a resource type that will never be synced ("MISSING RESOURCE"). func TestAccountList_SkipsReparentWhenHierarchyNotSynced(t *testing.T) { ctx := context.Background() orgs := &fakeOrgs{ @@ -332,7 +332,7 @@ func newOrgAccount(orgs *fakeOrgs) *accountResourceType { // newOrgAccountWithSyncFilter is like newOrgAccount but lets the test control whether the // organization/organizational_unit hierarchy types are opted into this sync run, to exercise -// the CXP-768 re-parenting gate. +// the re-parenting gate. func newOrgAccountWithSyncFilter(orgs *fakeOrgs, willSyncOrganization, willSyncOrganizationalUnit bool) *accountResourceType { identityInstance := &awsSsoAdminTypes.InstanceMetadata{ InstanceArn: awsSdk.String(behaviorInstanceArn), From fbb7a735faac20c84d7e6fb6e1859e10bc2cb37a Mon Sep 17 00:00:00 2001 From: Javier David Carnelli Date: Wed, 29 Jul 2026 01:08:48 -0300 Subject: [PATCH 3/5] chore: simplify guard for syncing or not parent ids --- pkg/connector/account.go | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/pkg/connector/account.go b/pkg/connector/account.go index f75be008..90192232 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -199,14 +199,14 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r // by-inheritance review can walk Account → OU → Root with the role pinned. Fail-soft: // without organizations:ListParents the account stays flat (parentless) and we WARN once. // - // Gated on willSyncOrganization/willSyncOrganizationalUnit: organization and - // organizational_unit are OptInRequired, so a sync run may never emit them. Pointing an - // account's parent at a resource type this run isn't syncing produces a dangling - // "MISSING RESOURCE" parent that c1 silently drops (see permissionSetRoleID's comment on - // dangling references). Skip the ListParents call entirely when neither hierarchy type - // will be synced; when only one is, still resolve the parent but only attach it if its - // resolved type is one this run will actually sync. - if o.willSyncOrganization || o.willSyncOrganizationalUnit { + // Gated on willSyncOrganization: organization and organizational_unit are OptInRequired, + // so a sync run may never emit them. Pointing an account's parent at a resource type this + // run isn't syncing produces a dangling "MISSING RESOURCE" parent that c1 silently drops + // (see permissionSetRoleID's comment on dangling references). The OU crawl is itself + // seeded exclusively from Root (see organization.go), so an OU can never actually be + // synced when organization is not - skip the ListParents call entirely in that case + // rather than resolving a parent that can never be attached. + if o.willSyncOrganization { parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) if err != nil { return nil, nil, err @@ -215,12 +215,8 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r orgReadDenied = true } if parentID != nil { - // OU sync is itself contingent on organization sync: the OU crawl is seeded - // exclusively from Root (see organization.go), so an OU can never actually be - // synced when organization is not. Require willSyncOrganization here too, or a - // partial opt-in (OU in, org out) would attach a parent that never gets emitted. - willSyncParentType := (parentID.ResourceType == resourceTypeOrganization.Id && o.willSyncOrganization) || - (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit && o.willSyncOrganization) + willSyncParentType := parentID.ResourceType == resourceTypeOrganization.Id || + (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit) if willSyncParentType { resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) } From 990f51819bebff5653eda1507d7ad115e0d73a79 Mon Sep 17 00:00:00 2001 From: Javier David Carnelli Date: Wed, 29 Jul 2026 01:54:22 -0300 Subject: [PATCH 4/5] chore: positional boolean values replaced by a struct --- pkg/connector/account.go | 46 +++++++++++-------- pkg/connector/connector.go | 6 +-- pkg/connector/organization_test.go | 3 +- ...permission_set_assignment_behavior_test.go | 3 +- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/pkg/connector/account.go b/pkg/connector/account.go index 90192232..e0d1c259 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -121,6 +121,16 @@ type entitlementsPageState struct { PermissionSetIndex int `json:"psi"` } +// HierarchySyncFlags reports whether a sync run will actually emit the (OptInRequired) +// organization/organizational_unit resource types. Grouped into a named struct rather than +// two positional bools so a call site can't silently swap Organization and +// OrganizationalUnit - the compiler catches a missing/misnamed field, and a transposition +// requires actually swapping the field names, not just their order. +type HierarchySyncFlags struct { + Organization bool + OrganizationalUnit bool +} + type accountResourceType struct { resourceType *v2.ResourceType orgClient orgsAPI @@ -130,13 +140,11 @@ type accountResourceType struct { identityClient client.IdentityStoreClient region string - // willSyncOrganization/willSyncOrganizationalUnit report whether this sync run will - // actually sync the corresponding (OptInRequired) hierarchy resource type. Account - // re-parenting (see List) is gated on these so accounts never point at a Root/OU - // resource that this run never syncs, which would otherwise leave a dangling - // "MISSING RESOURCE" parent. - willSyncOrganization bool - willSyncOrganizationalUnit bool + // hierarchySync reports whether this sync run will actually sync the corresponding + // (OptInRequired) hierarchy resource type. Account re-parenting (see List) is gated on + // this so accounts never point at a Root/OU resource that this run never syncs, which + // would otherwise leave a dangling "MISSING RESOURCE" parent. + hierarchySync HierarchySyncFlags } func (o *accountResourceType) ResourceType(_ context.Context) *v2.ResourceType { @@ -206,7 +214,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r // seeded exclusively from Root (see organization.go), so an OU can never actually be // synced when organization is not - skip the ListParents call entirely in that case // rather than resolving a parent that can never be attached. - if o.willSyncOrganization { + if o.hierarchySync.Organization { parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) if err != nil { return nil, nil, err @@ -216,7 +224,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r } if parentID != nil { willSyncParentType := parentID.ResourceType == resourceTypeOrganization.Id || - (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.willSyncOrganizationalUnit) + (parentID.ResourceType == resourceTypeOrganizationalUnit.Id && o.hierarchySync.OrganizationalUnit) if willSyncParentType { resourceOpts = append(resourceOpts, resourceSdk.WithParentResourceID(parentID)) } @@ -974,19 +982,17 @@ func accountBuilder( identityInstance *awsSsoAdminTypes.InstanceMetadata, region string, identityClient client.IdentityStoreClient, - willSyncOrganization bool, - willSyncOrganizationalUnit bool, + hierarchySync HierarchySyncFlags, ) *accountResourceType { return &accountResourceType{ - resourceType: resourceTypeAccount, - orgClient: orgClient, - roleArn: roleArn, - ssoAdminClient: ssoAdminClient, - identityClient: identityClient, - identityInstance: identityInstance, - region: region, - willSyncOrganization: willSyncOrganization, - willSyncOrganizationalUnit: willSyncOrganizationalUnit, + resourceType: resourceTypeAccount, + orgClient: orgClient, + roleArn: roleArn, + ssoAdminClient: ssoAdminClient, + identityClient: identityClient, + identityInstance: identityInstance, + region: region, + hierarchySync: hierarchySync, } } diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 923a2dd9..3a758d82 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -501,7 +501,7 @@ func (c *AWS) ResourceSyncers(ctx context.Context) []connectorbuilder.ResourceSy if c.orgsEnabled && c.ssoEnabled { l.Debug("orgsEnabled. creating accountBuilder") acct := accountBuilder(c.orgClient, c.roleARN, c.ssoAdminClient, c.identityInstance, c.ssoRegion, c.identityStoreClient, - c.willSyncOrganization, c.willSyncOrganizationalUnit) + HierarchySyncFlags{Organization: c.willSyncOrganization, OrganizationalUnit: c.willSyncOrganizationalUnit}) rs = append(rs, acct, // Sparse ACLs (Cloud Infrastructure Access): permission set as role, and the @@ -547,9 +547,9 @@ func (d *defaultCapabilitiesBuilder) ResourceSyncers(_ context.Context) []connec inlinePolicyBuilder(nil, nil, nil, nil), ssoUserBuilder("", nil, nil, nil, nil), ssoGroupBuilder("", nil, nil, nil), - accountBuilder(nil, "", nil, nil, "", nil, true, true), + accountBuilder(nil, "", nil, nil, "", nil, HierarchySyncFlags{Organization: true, OrganizationalUnit: true}), permissionSetBuilder(nil, nil, true), - permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil, true, true)), + permissionSetAssignmentBuilder(accountBuilder(nil, "", nil, nil, "", nil, HierarchySyncFlags{Organization: true, OrganizationalUnit: true})), organizationBuilder(nil), organizationalUnitBuilder(nil), accountIAMBuilder(nil, nil, nil), diff --git a/pkg/connector/organization_test.go b/pkg/connector/organization_test.go index 8e2c7c3b..d0377028 100644 --- a/pkg/connector/organization_test.go +++ b/pkg/connector/organization_test.go @@ -338,5 +338,6 @@ func newOrgAccountWithSyncFilter(orgs *fakeOrgs, willSyncOrganization, willSyncO InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil, willSyncOrganization, willSyncOrganizationalUnit) + return accountBuilder(orgs, "", &fakeSSOAdmin{}, identityInstance, behaviorRegion, nil, + HierarchySyncFlags{Organization: willSyncOrganization, OrganizationalUnit: willSyncOrganizationalUnit}) } diff --git a/pkg/connector/permission_set_assignment_behavior_test.go b/pkg/connector/permission_set_assignment_behavior_test.go index 7ab4ed09..19915b17 100644 --- a/pkg/connector/permission_set_assignment_behavior_test.go +++ b/pkg/connector/permission_set_assignment_behavior_test.go @@ -218,7 +218,8 @@ func newBehaviorAccount(sso *fakeSSOAdmin) *accountResourceType { InstanceArn: awsSdk.String(behaviorInstanceArn), IdentityStoreId: awsSdk.String(behaviorIdentityStoreID), } - return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}, true, true) + return accountBuilder(&fakeOrgs{}, "", sso, identityInstance, behaviorRegion, &test.MockedIdentityStoreClient{}, + HierarchySyncFlags{Organization: true, OrganizationalUnit: true}) } func behaviorBinding(t *testing.T) (*v2.Resource, *v2.Entitlement) { From 6dba26292f54b539523f5afdcd28404bd90e13b2 Mon Sep 17 00:00:00 2001 From: Javier David Carnelli Date: Mon, 3 Aug 2026 10:08:48 -0300 Subject: [PATCH 5/5] chore: test added for particular case. Warn log degraded to Debug log --- pkg/connector/account.go | 14 ++------------ pkg/connector/organization_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/pkg/connector/account.go b/pkg/connector/account.go index e0d1c259..dfc5b32c 100644 --- a/pkg/connector/account.go +++ b/pkg/connector/account.go @@ -203,17 +203,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r }), } - // Sparse ACLs hierarchy (Phase 2): re-parent the account under its Root/OU so c1's - // by-inheritance review can walk Account → OU → Root with the role pinned. Fail-soft: - // without organizations:ListParents the account stays flat (parentless) and we WARN once. - // - // Gated on willSyncOrganization: organization and organizational_unit are OptInRequired, - // so a sync run may never emit them. Pointing an account's parent at a resource type this - // run isn't syncing produces a dangling "MISSING RESOURCE" parent that c1 silently drops - // (see permissionSetRoleID's comment on dangling references). The OU crawl is itself - // seeded exclusively from Root (see organization.go), so an OU can never actually be - // synced when organization is not - skip the ListParents call entirely in that case - // rather than resolving a parent that can never be attached. + // Gated on hierarchySync.Organization: organization and organizational_unit are OptInRequired, if o.hierarchySync.Organization { parentID, accessDenied, err := accountParentResourceID(ctx, o.orgClient, accountId) if err != nil { @@ -245,7 +235,7 @@ func (o *accountResourceType) List(ctx context.Context, _ *v2.ResourceId, opts r rv = append(rv, userResource) } if orgReadDenied { - l.Warn("baton-aws: missing organizations:ListParents permission; accounts synced flat (no Root/OU hierarchy). " + + l.Debug("baton-aws: missing organizations:ListParents permission; accounts synced flat (no Root/OU hierarchy). " + "Add organizations:ListParents to enable by-inheritance review across the org tree.") } diff --git a/pkg/connector/organization_test.go b/pkg/connector/organization_test.go index d0377028..af3332be 100644 --- a/pkg/connector/organization_test.go +++ b/pkg/connector/organization_test.go @@ -293,6 +293,36 @@ func TestAccountList_SkipsReparentWhenHierarchyNotSynced(t *testing.T) { assert.Equal(t, 0, orgs.listParentsCalls, "must not call ListParents when neither hierarchy type is synced") } +// account.List must not call ListParents when organization is opted out even if +// organizational_unit is opted in — OU sync is itself seeded from Root (see organization.go), +// so OU can never actually be synced when organization is not, and resolving the parent would +// be wasted work. This also means a ListParents access-denied error can never surface the +// "missing organizations:ListParents permission" WARN in this specific combo; the account still +// ends up flat either way, so this is a lost diagnostic, not a correctness bug (see the comment +// on the ListParents gate in account.go). +func TestAccountList_SkipsReparentWhenOnlyOrganizationalUnitSynced(t *testing.T) { + ctx := context.Background() + orgs := &fakeOrgs{ + listAccountsFn: func(_ *awsOrgs.ListAccountsInput) (*awsOrgs.ListAccountsOutput, error) { + return &awsOrgs.ListAccountsOutput{Accounts: []awsOrgsTypes.Account{{ + Id: awsSdk.String(testAccountID), + Name: awsSdk.String("prod"), + Status: awsOrgsTypes.AccountStatusActive, + }}}, nil + }, + listParentsFn: func(_ *awsOrgs.ListParentsInput) (*awsOrgs.ListParentsOutput, error) { + return nil, &awsOrgsTypes.AccessDeniedException{Message: awsSdk.String("no perms")} + }, + } + acct := newOrgAccountWithSyncFilter(orgs, false, true) + + resources, _, err := acct.List(ctx, nil, resourceSdk.SyncOpAttrs{}) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Nil(t, resources[0].ParentResourceId, "account must stay flat when organization isn't synced") + assert.Equal(t, 0, orgs.listParentsCalls, "must not call ListParents when organization is opted out, regardless of organizational_unit") +} + // account.List resolves the parent but only attaches it when the resolved parent's specific // type (organization vs organizational_unit) is one this run will actually sync — partial // opt-in must not produce a dangling parent either.