Using ForEach with If to Check Computer Accounts for Group Memberships

Question

I'm working on a script that checks the members of an OU to see if they are members of a corresponding group. The script currently just adds devices from the OU to the related group.

What I want is for the script to check each device in the OU to see if it's already a member, and if it isn't to remove all OU groups then add the one for the current OU. If it is a member, the script will just make a note in the transcript and move on to the next device.

The new version runs without errors. But it always runs the else condition. Even when the test device meets the if condition by being in the required group.

I've included the new code that I have below. Can anyone tell what I've got wrong here?

$group = get-adgroup 'OU - Group'
$computers = Get-ADComputer -Filter * -SearchBase "ou=Devices,ou=test,ou=Root,dc=domain,dc=local"
Start-Transcript -Path c:\temp\Transcript.txt
foreach ($computer in $computers){
if(($_.memberOf -like 'OU - Group') -eq $true)
{
Write-Host "$computer is already in $group."
}else{
Get-ADGroup -Filter {name -like 'location - *'} | Remove-ADGroupMember -Members $computer -WhatIf
Add-ADGroupMember -Identity $group -Members $computer -WhatIf
}}
Stop-Transcript

Answer 1

Have a look at this code. I've annotated the important changes with comments. If you're going to use PowerShell, you should also get used to using the pipeline. There's no benefit to creating an array of objects (e.g., computers) and then use a "foreach" to process that array. With a pipeline you can start processing the items as they are returned from the Get-ADComputer. With only a small number of array elements the difference wouldn't be noticeable, but if you're working with thousands of items it takes time to return the results and build the array before you begin processing the information.

Start-Transcript -Path c:\temp\Transcript.txt
$group = get-adgroup 'OU - Group'
Get-ADComputer -Filter * -SearchBase "ou=Devices,ou=test,ou=Root,dc=domain,dc=local" -Properties memberof | # Add -Properties parameter
    ForEach-Object {
        $computer = $_
        if($computer.memberOf -contains $group.distinguishedname){ # memberof is a list of distinguishednames,so look for group's DN in that list
            Write-Host "$($computer.Name) is already in $($group.name)"
        }else{
            Get-ADGroup -Filter {name -like 'location - *'} |   # if this filter doesn't work, replace the "{" and "}" characters with double-quotes
                Remove-ADGroupMember -Members $computer.distinguishedname -WhatIf  # I'd work with DNs to avoid ambiguity
            Add-ADGroupMember -Identity $group.distinguishedname -Members $computer.distinguishedname -WhatIf  # I'd work with DNs to avoid ambiguity
        }
    }
Stop-Transcript

Answer 2

I see a couple of issues. I personally find using Visual Studio Code a great way of testing scripts. It has Poweshell support and can allow you to step through your scripts, check values, etc.

The first problem I see is that you're relying on the property memberOf but that property isn't returned by default (at least in my AD). Therefore you will never find a match. When requesting the computers, include the property.

Get-ADComputer -Filter * -SearchBase "..." -Properties memberOf

In VS Code, once you've executed this line in the debugger you should be able to view the objects to ensure the property is coming back.

The second issue I see is that memberOf is an array and you're calling -like on it. That would return back the value that matches the value. You then try to take that string value and compare it to $true which is always false. Since memberOf is an array then use contains instead.

if ($computer.memberOf -contains 'groupname') { } else { }

The third problem is that within the if you're using $_ which is not defined (since this isn't a script block). The correct variable is $computer, given your code.

Answer 3

Hi,

There are many error in your script and I suggest you to start working on the script below , you have to put the right group and OU name in the variable $groupename and $OU_Name before execute it:

$groupname = "GroupName"

$Ou_Name = "TestOU"

$OU_DN = Get-ADOrganizationalUnit -Filter 'Name -like $Ou_Name' | select -ExpandProperty distinguishedName
$computer_list =get-adcomputer -Filter * -SearchBase $OU_DN | select -ExpandProperty distinguishedName

#Add missing computer under the OU to group

foreach($computer in $computer_list)
{
$Group_Members =Get-ADGroupMember -Identity $groupname | select -ExpandProperty distinguishedName


if ($Group_Members -contains $computer)

{

Write-Host "$computer nothing to do"
}

else
{

#Add-ADGroupMember -Identity $groupname -Members $computer
Write-Host "$computer has been added to $groupname"
}

}

#clean member of the group

$Group_Members =Get-ADGroupMember -Identity $groupname | select -ExpandProperty distinguishedName

foreach($member in $Group_Members)
{
if($member -like "*$OU_DN")

{

Write-Host "$computer nothing to do, the computer is alreday under the $OU_DN"
}
else
{

Remove-ADGroupMember -Identity $groupname -Members $computer
Write-Host "$computer has been removed from $groupname"
}
}

Please don't forget to mark helpful answer as accepted

Answer 4

Hello there,

The simplest way would be to start, export the memberships to a CSV file and then load that to compare the previous run with the current run. To do this, use two additional foreach loops: the first will look for users that were added and the second will look for users that were removed.

foreach ($group in $groups) {
    $previousRunMemberships = $null
    if (Test-Path "C:\tmp\$group.csv") {
        $previousRunMemberships = Import-Csv -Path "C:\tmp\$group.csv"
    }
    $groupMembers = Get-LocalGroupMember $group
    $added = foreach ($member in $groupMembers) {
        if ($previousRunMemberships.SID -notcontains $member.SID) {
            $member
        }
    }
    $removed = foreach ($member in $previousRunMemberships) {
        if ($groupMembers.SID -notcontains $member.SID) {
            $member
        }
    }
   $groupMembers | Export-Csv -Path "C:\tmp\$group.csv" -NoTypeInformation
}

Hope this resolves your Query !!

--If the reply is helpful, please Upvote and Accept it as an answer--