From 0e6dd751c74db4cb29a7b03fdd1bb6b1fd86535f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Mar 2026 19:49:43 +0000 Subject: [PATCH 1/5] Initial plan From f547bdb709ec7f10936137303db1607156ee5dbf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Mar 2026 19:55:10 +0000 Subject: [PATCH 2/5] Fix RemoveGroupMember and AddGroupMember change detection Check membership before operations to accurately detect changes. This fixes PruneEntitlements always returning Changed=False when AD cmdlets are idempotent (newer ActiveDirectory module versions). Co-authored-by: ntt-matthias-fleschuetz <55826276+ntt-matthias-fleschuetz@users.noreply.github.com> --- .../Private/New-IdleADAdapter.ps1 | 68 +++++++++++++------ 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 index c938f803..95507b38 100644 --- a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 +++ b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 @@ -678,26 +678,39 @@ function New-IdleADAdapter { [string] $MemberIdentity ) - $params = @{ + # Check if already a member (idempotency + reliable change detection) + $getMembersParams = @{ Identity = $GroupIdentity - Members = $MemberIdentity ErrorAction = 'Stop' } if ($null -ne $this.Credential) { - $params['Credential'] = $this.Credential + $getMembersParams['Credential'] = $this.Credential } try { - Add-ADGroupMember @params - return $true - } - catch { - # Idempotency: already a member is a no-op - if ($_.Exception.Message -match 'already a member') { + $members = Get-ADGroupMember @getMembersParams + $isMember = $null -ne ($members | Where-Object { $_.DistinguishedName -eq $MemberIdentity }) + + if ($isMember) { + # Already a member - no change needed return $false } - throw } + catch { + # If Get-ADGroupMember fails (e.g., group doesn't exist), let Add-ADGroupMember handle it + } + + $params = @{ + Identity = $GroupIdentity + Members = $MemberIdentity + ErrorAction = 'Stop' + } + if ($null -ne $this.Credential) { + $params['Credential'] = $this.Credential + } + + Add-ADGroupMember @params + return $true } -Force $adapter | Add-Member -MemberType ScriptMethod -Name RemoveGroupMember -Value { @@ -711,27 +724,40 @@ function New-IdleADAdapter { [string] $MemberIdentity ) - $params = @{ + # Check if actually a member (idempotency + reliable change detection) + $getMembersParams = @{ Identity = $GroupIdentity - Members = $MemberIdentity - Confirm = $false ErrorAction = 'Stop' } if ($null -ne $this.Credential) { - $params['Credential'] = $this.Credential + $getMembersParams['Credential'] = $this.Credential } try { - Remove-ADGroupMember @params - return $true - } - catch { - # Idempotency: not a member is a no-op - if ($_.Exception.Message -match 'not a member|Member does not exist') { + $members = Get-ADGroupMember @getMembersParams + $isMember = $null -ne ($members | Where-Object { $_.DistinguishedName -eq $MemberIdentity }) + + if (-not $isMember) { + # Not a member - no change needed return $false } - throw } + catch { + # If Get-ADGroupMember fails (e.g., group doesn't exist), let Remove-ADGroupMember handle it + } + + $params = @{ + Identity = $GroupIdentity + Members = $MemberIdentity + Confirm = $false + ErrorAction = 'Stop' + } + if ($null -ne $this.Credential) { + $params['Credential'] = $this.Credential + } + + Remove-ADGroupMember @params + return $true } -Force $adapter | Add-Member -MemberType ScriptMethod -Name GetUserGroups -Value { From acea5eed19711539cfd73f59543b05a0714dbfc8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Mar 2026 20:24:02 +0000 Subject: [PATCH 3/5] Improve error handling comments in membership checks Add clearer explanations for why catch blocks proceed on error, ensuring legitimate operations aren't blocked by idempotency checks. Co-authored-by: ntt-matthias-fleschuetz <55826276+ntt-matthias-fleschuetz@users.noreply.github.com> --- src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 index 95507b38..91a4f0fe 100644 --- a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 +++ b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 @@ -697,7 +697,9 @@ function New-IdleADAdapter { } } catch { - # If Get-ADGroupMember fails (e.g., group doesn't exist), let Add-ADGroupMember handle it + # If Get-ADGroupMember fails (e.g., group doesn't exist, permission denied), proceed + # to Add-ADGroupMember which will provide the authoritative error message. + # This ensures idempotency check doesn't prevent legitimate operations. } $params = @{ @@ -743,7 +745,9 @@ function New-IdleADAdapter { } } catch { - # If Get-ADGroupMember fails (e.g., group doesn't exist), let Remove-ADGroupMember handle it + # If Get-ADGroupMember fails (e.g., group doesn't exist, permission denied), proceed + # to Remove-ADGroupMember which will provide the authoritative error message. + # This ensures idempotency check doesn't prevent legitimate operations. } $params = @{ From 20981b2fa8635c962910e69c6c35d8a501d77198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Flesch=C3=BCtz?= <55826276+ntt-matthias-fleschuetz@users.noreply.github.com> Date: Mon, 2 Mar 2026 13:39:57 +0100 Subject: [PATCH 4/5] reduce work for large groups by filtering on memberOf of Identity Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 index 91a4f0fe..1fa3fbe5 100644 --- a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 +++ b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 @@ -688,8 +688,10 @@ function New-IdleADAdapter { } try { - $members = Get-ADGroupMember @getMembersParams - $isMember = $null -ne ($members | Where-Object { $_.DistinguishedName -eq $MemberIdentity }) + $existingMember = Get-ADGroupMember @getMembersParams | + Where-Object { $_.DistinguishedName -eq $MemberIdentity } | + Select-Object -First 1 + $isMember = $null -ne $existingMember if ($isMember) { # Already a member - no change needed From a3c4f1635551c77460a178793c33f4076c55ec43 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 2 Mar 2026 12:54:50 +0000 Subject: [PATCH 5/5] Refactor membership checks and add unit tests - Extract TestGroupMembership helper to eliminate code duplication - Add Write-Verbose to catch blocks (fixes PSAvoidUsingEmptyCatchBlock) - Optimize RemoveGroupMember with Select-Object -First 1 - Add comprehensive unit tests for change detection behavior - Tests cover no-op, actual change, and fail-forward scenarios Co-authored-by: ntt-matthias-fleschuetz <55826276+ntt-matthias-fleschuetz@users.noreply.github.com> --- .../Private/New-IdleADAdapter.ps1 | 67 +++--- ...w-IdleADAdapter.MembershipChecks.Tests.ps1 | 200 ++++++++++++++++++ 2 files changed, 235 insertions(+), 32 deletions(-) create mode 100644 tests/Providers/New-IdleADAdapter.MembershipChecks.Tests.ps1 diff --git a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 index 1fa3fbe5..3c3a3ec1 100644 --- a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 +++ b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 @@ -667,7 +667,9 @@ function New-IdleADAdapter { } } -Force - $adapter | Add-Member -MemberType ScriptMethod -Name AddGroupMember -Value { + # Helper method to check if a user is a member of a group + # Returns $true if member, $false if not a member, $null if check fails + $adapter | Add-Member -MemberType ScriptMethod -Name TestGroupMembership -Value { param( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] @@ -678,7 +680,6 @@ function New-IdleADAdapter { [string] $MemberIdentity ) - # Check if already a member (idempotency + reliable change detection) $getMembersParams = @{ Identity = $GroupIdentity ErrorAction = 'Stop' @@ -688,21 +689,38 @@ function New-IdleADAdapter { } try { + # For large groups, short-circuit after finding the first match $existingMember = Get-ADGroupMember @getMembersParams | Where-Object { $_.DistinguishedName -eq $MemberIdentity } | Select-Object -First 1 - $isMember = $null -ne $existingMember - - if ($isMember) { - # Already a member - no change needed - return $false - } + return ($null -ne $existingMember) } catch { - # If Get-ADGroupMember fails (e.g., group doesn't exist, permission denied), proceed - # to Add-ADGroupMember which will provide the authoritative error message. - # This ensures idempotency check doesn't prevent legitimate operations. + # Return null to signal check failure (let calling method proceed to authoritative operation) + Write-Verbose "TestGroupMembership: Get-ADGroupMember failed for group '$GroupIdentity' and member '$MemberIdentity': $($_.Exception.Message)" + return $null } + } -Force + + $adapter | Add-Member -MemberType ScriptMethod -Name AddGroupMember -Value { + param( + [Parameter(Mandatory)] + [ValidateNotNullOrEmpty()] + [string] $GroupIdentity, + + [Parameter(Mandatory)] + [ValidateNotNullOrEmpty()] + [string] $MemberIdentity + ) + + # Check if already a member (idempotency + reliable change detection) + $isMember = $this.TestGroupMembership($GroupIdentity, $MemberIdentity) + + if ($isMember -eq $true) { + # Already a member - no change needed + return $false + } + # If $isMember is $null (check failed), proceed to Add-ADGroupMember for authoritative error $params = @{ Identity = $GroupIdentity @@ -729,28 +747,13 @@ function New-IdleADAdapter { ) # Check if actually a member (idempotency + reliable change detection) - $getMembersParams = @{ - Identity = $GroupIdentity - ErrorAction = 'Stop' - } - if ($null -ne $this.Credential) { - $getMembersParams['Credential'] = $this.Credential - } - - try { - $members = Get-ADGroupMember @getMembersParams - $isMember = $null -ne ($members | Where-Object { $_.DistinguishedName -eq $MemberIdentity }) - - if (-not $isMember) { - # Not a member - no change needed - return $false - } - } - catch { - # If Get-ADGroupMember fails (e.g., group doesn't exist, permission denied), proceed - # to Remove-ADGroupMember which will provide the authoritative error message. - # This ensures idempotency check doesn't prevent legitimate operations. + $isMember = $this.TestGroupMembership($GroupIdentity, $MemberIdentity) + + if ($isMember -eq $false) { + # Not a member - no change needed + return $false } + # If $isMember is $null (check failed), proceed to Remove-ADGroupMember for authoritative error $params = @{ Identity = $GroupIdentity diff --git a/tests/Providers/New-IdleADAdapter.MembershipChecks.Tests.ps1 b/tests/Providers/New-IdleADAdapter.MembershipChecks.Tests.ps1 new file mode 100644 index 00000000..c1cd7711 --- /dev/null +++ b/tests/Providers/New-IdleADAdapter.MembershipChecks.Tests.ps1 @@ -0,0 +1,200 @@ +Set-StrictMode -Version Latest + +BeforeAll { + . (Join-Path -Path $PSScriptRoot -ChildPath '..\_testHelpers.ps1') + Import-IdleTestModule + + $repoRoot = Split-Path -Path (Split-Path -Path $PSScriptRoot -Parent) -Parent + $adapterPath = Join-Path -Path $repoRoot -ChildPath 'src\IdLE.Provider.AD\Private\New-IdleADAdapter.ps1' + + if (-not (Test-Path -LiteralPath $adapterPath -PathType Leaf)) { + throw "New-IdleADAdapter.ps1 not found at: $adapterPath" + } + + # Mock the AD cmdlets globally for this test + function Get-ADGroupMember { + param($Identity, $Credential, $ErrorAction) + + # Simulate group membership data + if ($script:MockGroupMembers.ContainsKey($Identity)) { + return $script:MockGroupMembers[$Identity] + } + + throw "Group '$Identity' not found" + } + + function Add-ADGroupMember { + param($Identity, $Members, $Credential, $ErrorAction) + return + } + + function Remove-ADGroupMember { + param($Identity, $Members, $Confirm, $Credential, $ErrorAction) + return + } + + # Now source the adapter (which will use our mocked functions) + . $adapterPath +} + +Describe 'New-IdleADAdapter membership checks' { + + BeforeEach { + $script:MockGroupMembers = @{} + } + + Context 'TestGroupMembership method' { + It 'returns $true when user is a member' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + [pscustomobject]@{ DistinguishedName = 'CN=User2,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.TestGroupMembership('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + + It 'returns $false when user is not a member' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.TestGroupMembership('CN=TestGroup,DC=contoso,DC=com', 'CN=User2,DC=contoso,DC=com') + + $result | Should -Be $false + } + + It 'returns $null when Get-ADGroupMember fails' { + # Don't add the group to MockGroupMembers, so Get-ADGroupMember will throw + + $adapter = New-IdleADAdapter + $result = $adapter.TestGroupMembership('CN=NonExistentGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -BeNullOrEmpty + } + + It 'short-circuits after finding first match (performance)' { + # Create a large group + $members = @() + for ($i = 1; $i -le 1000; $i++) { + $members += [pscustomobject]@{ DistinguishedName = "CN=User$i,DC=contoso,DC=com" } + } + $script:MockGroupMembers['CN=LargeGroup,DC=contoso,DC=com'] = $members + + $adapter = New-IdleADAdapter + # User1 is at the beginning - should find quickly + $result = $adapter.TestGroupMembership('CN=LargeGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + } + + Context 'AddGroupMember change detection' { + It 'returns $false when user is already a member (no-op)' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.AddGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $false + } + + It 'returns $true when user is added (change occurred)' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User2,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.AddGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + + It 'proceeds to Add-ADGroupMember when membership check fails (fail-forward)' { + # Don't add the group to MockGroupMembers, so membership check will fail + + $adapter = New-IdleADAdapter + $result = $adapter.AddGroupMember('CN=NonExistentGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + } + + Context 'RemoveGroupMember change detection' { + It 'returns $false when user is not a member (no-op)' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User2,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.RemoveGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $false + } + + It 'returns $true when user is removed (change occurred)' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + $result = $adapter.RemoveGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + + It 'proceeds to Remove-ADGroupMember when membership check fails (fail-forward)' { + # Don't add the group to MockGroupMembers, so membership check will fail + + $adapter = New-IdleADAdapter + $result = $adapter.RemoveGroupMember('CN=NonExistentGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + + $result | Should -Be $true + } + } + + Context 'Idempotency across multiple calls' { + It 'AddGroupMember is idempotent - second call returns $false' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @() + + $adapter = New-IdleADAdapter + + # First call - should add + $result1 = $adapter.AddGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + $result1 | Should -Be $true + + # Simulate the user now being a member + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + ) + + # Second call - should be no-op + $result2 = $adapter.AddGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + $result2 | Should -Be $false + } + + It 'RemoveGroupMember is idempotent - second call returns $false' { + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @( + [pscustomobject]@{ DistinguishedName = 'CN=User1,DC=contoso,DC=com' } + ) + + $adapter = New-IdleADAdapter + + # First call - should remove + $result1 = $adapter.RemoveGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + $result1 | Should -Be $true + + # Simulate the user no longer being a member + $script:MockGroupMembers['CN=TestGroup,DC=contoso,DC=com'] = @() + + # Second call - should be no-op + $result2 = $adapter.RemoveGroupMember('CN=TestGroup,DC=contoso,DC=com', 'CN=User1,DC=contoso,DC=com') + $result2 | Should -Be $false + } + } +}