Scheduled Powershell task

PowerShell Help
1

Hi everyone,

I am kind of new to PowerShell and trying to create some advanced script to run on DC each day as scheduled task. The aim of this script is, it will check a specific user OU and sub OUs then if users are not member of the Security group it will add users to the group and if they are already member of the group it will skip it. I kind of developed below script but I am not sure if it’s the right one. I haven’t tested it yet on DC but would be nice to get some feed back from experienced members before I test it on a DC.

Check if Active Directory module is already imported

if (-not (Get-Module -Name ActiveDirectory)) {
Import-Module ActiveDirectory
}

Define the base OU and security group

$BaseOU = “OU=W11_USERS,DC=MYCOMPANY,DC=NET”
$SecurityGroup = “HR”

Get all users from W11_USERS and its sub-OUs

$Users = Get-ADUser -SearchBase $BaseOU -SearchScope Subtree -Filter *

Loop through each user and check group membership before adding

foreach ($User in $Users) {
$UserDN = $User.DistinguishedName

# Check if user is already a member of HR
$IsMember = Get-ADGroupMember -Identity $SecurityGroup | Where-Object { $_.DistinguishedName -eq $UserDN }

if (-not $IsMember) {
    Try {
        Add-ADGroupMember -Identity $SecurityGroup -Members $User -ErrorAction Stop
        Write-Host "Added $($User.SamAccountName) to $SecurityGroup" -ForegroundColor Green
    } Catch {
        Write-Host "Failed to add $($User.SamAccountName): $_" -ForegroundColor Red
    }
} else {
    Write-Host "$($User.SamAccountName) is already a member of $SecurityGroup" -ForegroundColor Yellow
}

}

Write-Host “User addition process completed.”
Exit

Thanks

2

Welcome @Comp_tech.

First, a housekeeping item. It really helps the readability of code if you can format it appropriately. Behind the gear icon is the option for preformatted text. If you use that, the code is much easier to read and thus much easier for everyone to help. You can go back and edit your post to make that change.

Testing stuff in AD can be a challenge. I know our org has never had a test domain that we could use to test this sort of code. If you also don’t have a test AD, one alternative, if you can, is to set up a test OU, create some test users, and a test group. Then try iterations against that group first. That should give you some confidence in your code or point out any obvious issues.

3

For sure what @psdarwin said, readability of the code using proper formatting is huge. Whether you’re here, Reddit, Stackoverflow, anywhere, there is a formatting button for sharing code.

I’m going to comment as I’m reading here:

The ‘-SearchScope Subtree’ is unnecessary as that is the default behavior of Get-ADUser with -SearchBase.

You might consider running your Get-ADGroupMember outside of your loop once, and storing the members in a variable. That way you can check against that variable instead of calling out to AD potentially hundreds of times to do the same work. E.g.

$Members = Get-ADGroupMember -Identity $SecurityGroup  
Foreach ($User in $Users) {
    $IsMember = $Members | Where-Object { $_.DistinguishedName -eq $User.DistinguishedName}
...

The rest of the loop makes sense to me, but you should for sure test it on a couple of accounts manually (maybe test accounts).
The Write-Host statements only help you when running the script manually in a console. If this is going to be a scheduled task those Write-Host statements will go nowhere. Consider replacing them with some form of logging. Maybe write a “Write-Log” function or use Out-File to add text to a file. Logging is really nice to have on something that runs unattended.

this isn’t necessary. The script is over it’s going to exit anyway. If you remove it, you’ll save yourself from accidentally closing your IDE’s terminal if selectively executing all of the code in your ps1.

Now beyond the PowerShell code itself I would recommend not running this directly on a Domain Controller itself. I’m a big fan of “single responsibility” on servers. A Domain Controller should have one job (ok maybe ADDS, DNS are two jobs). Scheduled tasks should live somewhere else like a utility server or something. That way when your DC is getting replaced/upgraded/deprecated, you don’t accidentally lose this critical scheduled task that no one knew was on the DC.

I would definitely put logging in the script, maybe Start-Transcript/Stop-Transcript for the first few runs, and consider adding -WhatIf to the end of your Add-ADGroupMember on the first run to just see what it’s going to do.

2 Likes
4

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.