I am trying to write s “Simple” script to add users to a specific security groups in AD based upon the organization attribute. If they belong to one org, they will be added to org1 security group. If they belongs to org2 they will be added to the org2 attribute. Simple, right.
I gathering all users with organization=org1 into a variable. Rather than rebuilding the group every time this is ran, I want to only add the delta of users that are not already a member of org1 group.
-
I gather all users that are already in the group into a variable.
-
I gather all users that have org1 value for AD Attribute “organization” (Actually, ‘O’)
-
I then try to run a compare-object against each variable expecting to find the difference and then just add those to the group. - VIOLA.
When I run the compare-object, it doe not give me the difference. It gives me one name that is already in the group. I cant understand why. I am pasting my script below in case there are glaring errors.
$currentGroupMembers = Get-ADGroupMember -Identity vumcaccounts | select samaccountname
$vumcUsers = get-aduser -LDAPFilter “(&(objectclass=user)(Objectcategory=person)(name=*)(O=org1))” -SearchBase “cn=users,dc=testlab,dc=com” -SearchScope Subtree -Properties samaccountname,o | select samaccountname
if ($currentGroupMembers) {
compare-object -ReferenceObject $currentGroupMembers -DifferenceObject $vumcusers -PassThru
} Else {
if ($currentGroupMembers -eq $null) {
foreach ($vumcUser in $vumcUsers) {
Add-ADGroupMember -Identity vumcaccounts -members $vumcUser.samaccountname
}
}
}
Clear-Variable currentgroupmembers, vumcusers
I did only few test runs on this script, but seems to work.
$org = 'Org1'
$usersToAdd = @()
$sourceGroup = 'vumcaccounts'
$currentGroupMembers = Get-ADGroupMember -Identity $sourceGroup | select -ExpandProperty samaccountname
$vumcUsers = get-aduser -fi {Organization -eq $org} -SearchBase "cn=users,dc=testlab,dc=com" -SearchScope Subtree | select -ExpandProperty sAMAccountName
ForEach ($vumcUser in $vumcUsers) {
if ($vumcUser -notin $currentGroupMembers) {$usersToAdd += $vumcUser}
}
Add-ADGroupMember -Identity $sourceGroup -members $usersToAdd
That version worked. however, I have another question.
is there any added benefit or performance enhancement to appending non-members of the group to the empty array and then processing at the end?
My script has evolved a little so bear with me. I am testing the accounts for the value of organization. if vumcOrg then add to VUMCACCOUNTS if vuOrg add to VUACCOUNTS. If $NULL log to file to remediate.
My questions is… is it better or worse to use the following and process the results accordingly after testing the affiliation in the script block or is it better to put applicable accounts into the empty array and then process at the end.
$vumcOrg = ‘vumc’
$vuOrg = ‘VU’
$vumcUsersToAdd = @()
$vuUsersToAdd = @()
$vumcSourceGroup = ‘vumcaccounts’
$vuSourceGroup = ‘vuaccounts’
$vumcCurrentGroupMembers = Get-ADGroupMember -Identity $vumcSourceGroup | select -ExpandProperty samaccountname
$vuCurrentGroupMembers = Get-ADGroupMember -Identity $vuSourceGroup | select -ExpandProperty samaccountname
$orgaccountsUnaffiliated = get-aduser -filter {Organization -eq “$null”} -SearchBase “cn=users,dc=testlab,dc=com” -SearchScope Subtree -Properties * | select ‘sAMAccountName’,‘givenname’,‘sn’,‘mail’
$orgAccounts = get-aduser -filter {Organization -ne “$null”} -SearchBase “cn=users,dc=testlab,dc=com” -SearchScope Subtree -Properties * | select ‘sAMAccountName’,‘Organization’
Performs a test on whether the user is a VUMC or VU employee and processes accordingly.
ForEach ($oAccount in $orgAccounts) {
if ($oAccount.organization -eq “VUMC”) {
if ($oAccount -notin $vumcSourceGroup) {
Add-ADGroupMember -Identity $vumcSourceGroup -Members $oAccount.sAMAccountName
}
} Else {
if ($oAccount.organization -eq "VU") {
if ($oAccount -notin $vuSourceGroup) {
Add-ADGroupMember -Identity $vuSourceGroup -Members $oAccount.sAMAccountName
}
}
}
}
This one I did not test, but I would do a function and call it with two different sets of parameters.
function Add-OrganizationUsersToGroups {
[CmdletBinding()]
Param
(
[Parameter(Mandatory=$true,
Position=0)]
$Org,
[Parameter(Mandatory=$true,
Position=0)]
$SourceGroup
)
$usersToAdd = @()
$currentGroupMembers = Get-ADGroupMember -Identity $sourceGroup | select -ExpandProperty samaccountname
$vumcUsers = get-aduser -fi {Organization -eq $org} -SearchBase "cn=users,dc=testlab,dc=com" -SearchScope Subtree | select -ExpandProperty sAMAccountName
ForEach ($vumcUser in $vumcUsers) {
if ($vumcUser -notin $currentGroupMembers) {$usersToAdd += $vumcUser}
}
Add-ADGroupMember -Identity $sourceGroup -members $usersToAdd
}
Add-OrganizationUsersToGroups -Org vumc -SourceGroup vumcaccounts
Add-OrganizationUsersToGroups -Org vu -SourceGroup vuaccounts
PS. Sorry Don, there is no help. I know you don’t like that.
“My questions is… is it better or worse to use the following and process the results accordingly after testing the affiliation in the script block or is it better to put applicable accounts into the empty array and then process at the end.”
I often run it inside the for-each loop, but that generates more load to domain controller. One thing that you should consider is not to use -properties *. Define the properties you really need and you can speed up your scripts quite a lot.
I was doing some tests with my colleague Jarkko on that matter and the result was massive. by defining the three needed properties against * made our script to run. We went through all our AD users (~10k objects) and running time was 14 seconds against 5 minutes or so.
Thanks Aapeli,
The ‘-properties *’ is just to be used in my test environment. I really don’t need it to return all properties. I was just running through some tests and did not change it back. I likely would have forgot all about it if you had not caught it. - THX.
As for determining the best place to process the add members, I understand you to be saying that by running the ADD in the foreach loop, it will generate more load on the DC’s. So, by running it after collecting the elements into an array, it will take the load off of the DC and place it locally on the workstation. Is that correct?
you are correct. If we would do add-adgroupmember inside the loop, then we would open a connection to dc, ask for it to to a change and then close the connection for X times. now we are moving the stress to the client machine where you run the script. In a small scale that doesn’t even really matter but I just came a cross to a system that hits our DC with LDAP queries and that actually was able to DDOS one of our DCs. I gave few more CPUs for it which solved the problem.
PS is much more lighter for DC than LDAP, but anyways that’s one thing to keep in mind.