diff --git a/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 b/src/IdLE.Provider.AD/Private/New-IdleADAdapter.ps1 index c938f803..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,29 +680,29 @@ function New-IdleADAdapter { [string] $MemberIdentity ) - $params = @{ + $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 + # For large groups, short-circuit after finding the first match + $existingMember = Get-ADGroupMember @getMembersParams | + Where-Object { $_.DistinguishedName -eq $MemberIdentity } | + Select-Object -First 1 + return ($null -ne $existingMember) } catch { - # Idempotency: already a member is a no-op - if ($_.Exception.Message -match 'already a member') { - return $false - } - throw + # 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 RemoveGroupMember -Value { + $adapter | Add-Member -MemberType ScriptMethod -Name AddGroupMember -Value { param( [Parameter(Mandatory)] [ValidateNotNullOrEmpty()] @@ -711,27 +713,60 @@ function New-IdleADAdapter { [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 Members = $MemberIdentity - Confirm = $false ErrorAction = 'Stop' } if ($null -ne $this.Credential) { $params['Credential'] = $this.Credential } - try { - Remove-ADGroupMember @params - return $true + Add-ADGroupMember @params + return $true + } -Force + + $adapter | Add-Member -MemberType ScriptMethod -Name RemoveGroupMember -Value { + param( + [Parameter(Mandatory)] + [ValidateNotNullOrEmpty()] + [string] $GroupIdentity, + + [Parameter(Mandatory)] + [ValidateNotNullOrEmpty()] + [string] $MemberIdentity + ) + + # Check if actually a member (idempotency + reliable change detection) + $isMember = $this.TestGroupMembership($GroupIdentity, $MemberIdentity) + + if ($isMember -eq $false) { + # Not a member - no change needed + return $false } - catch { - # Idempotency: not a member is a no-op - if ($_.Exception.Message -match 'not a member|Member does not exist') { - return $false - } - throw + # If $isMember is $null (check failed), proceed to Remove-ADGroupMember for authoritative error + + $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 { 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 + } + } +}