Skip to content

feat(datagen): add GroupIdentity with built-in AD groups and membership#149

Open
Dylan-M wants to merge 1 commit into04-06-feat_datagen_add_name_pools_and_useridentity_generationfrom
04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership
Open

feat(datagen): add GroupIdentity with built-in AD groups and membership#149
Dylan-M wants to merge 1 commit into04-06-feat_datagen_add_name_pools_and_useridentity_generationfrom
04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership

Conversation

@Dylan-M
Copy link
Copy Markdown
Contributor

@Dylan-M Dylan-M commented Apr 27, 2026

Proposed Change

Adds GroupIdentity with a built-in AD group catalog and membership semantics.

Part of PIPE-927 common data generation package stack. Foundation for PIPE-785, PIPE-928, PIPE-943, and the rest of the simulator stack.

Checklist
  • Changes are tested
  • CI has passed

Copy link
Copy Markdown
Contributor Author

Dylan-M commented Apr 27, 2026

@Dylan-M Dylan-M marked this pull request as ready for review April 27, 2026 14:41
@Dylan-M Dylan-M requested review from a team as code owners April 27, 2026 14:41
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from e955ee9 to 6c1cf6d Compare May 8, 2026 17:17
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from 67cf1db to 822f438 Compare May 8, 2026 17:17
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from 6c1cf6d to 9a3b684 Compare May 8, 2026 17:23
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from 822f438 to f7b19a4 Compare May 8, 2026 17:23
Comment thread internal/datagen/groups.go Outdated

// GenerateGroups produces groups including built-in AD groups and department-based groups.
// Users are assigned to groups based on department. At least 1-2 admin users go to Domain Admins.
func GenerateGroups(seed int64, count int, domain *DomainIdentity, users []*UserIdentity) []*GroupIdentity {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, GenerateGroups does not honor count when count is smaller than the built-in catalog. It always appends all built-in groups first, so GenerateGroups(seed, 5, ...) still returns 9 groups. The test even codifies this with “at least 10 groups"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the parameter to targetTotal and rewrote the docstring: built-in AD groups (n=9) are always included; department groups append until total reaches targetTotal or the catalog is exhausted (cap at 18). Built-ins shouldn't be truncated since an AD environment without Domain Admins / Domain Users would be incoherent. Test added that exercises targetTotal < 9.

Comment thread internal/datagen/groups.go Outdated
Comment on lines +140 to +148
if domainAdmins != nil && len(users) > 0 {
adminCount := 1
if len(users) > 10 {
adminCount = 2
}
for i := 0; i < adminCount && i < len(users); i++ {
idx := r.Intn(len(users)) // #nosec G404
domainAdmins.MemberSIDs = append(domainAdmins.MemberSIDs, users[idx].SID)
users[idx].GroupSIDs = append(users[idx].GroupSIDs, domainAdmins.SID)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the Domain Admins assignment pick the same user more than once because it samples with replacement? With adminCount = 2, the same SID can be appended twice to domainAdmins.MemberSIDs, and that same group SID can be appended twice to users[idx].GroupSIDs. Correct me if I'm wrong

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adminCount is now a parameter and the picker uses a Fisher-Yates partial shuffle so duplicate user SIDs can't enter MemberSIDs and a single user can't appear twice in GroupSIDs. The previous "1 if users≤10, else 2" heuristic was unrealistic — Microsoft's published guidance is qualitative ("minimize Domain Admins membership") rather than a sizing table; the new step function (2/3/5/8/15/25/35 across user-count buckets up to 10000+) tracks what audited environments commonly observe rather than best-practice. Exposed via EnvironmentOpts.DomainAdminsCount (zero = use heuristic).

@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from 9a3b684 to 411dc81 Compare May 8, 2026 18:25
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from f7b19a4 to 25932c1 Compare May 8, 2026 18:26
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from 411dc81 to c2d0a07 Compare May 8, 2026 19:52
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from 25932c1 to deb45b1 Compare May 8, 2026 19:52
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from c2d0a07 to d4d1bd6 Compare May 8, 2026 19:58
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from deb45b1 to 8de7833 Compare May 8, 2026 19:58
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_name_pools_and_useridentity_generation branch from d4d1bd6 to e969cc4 Compare May 8, 2026 22:20
@Dylan-M Dylan-M force-pushed the 04-06-feat_datagen_add_groupidentity_with_built-in_ad_groups_and_membership branch from 8de7833 to 4868206 Compare May 8, 2026 22:20
@Dylan-M Dylan-M requested a review from eKuG May 8, 2026 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants