Review my code please

This runs great from what I can tell. Its a script to create active directory accounts, add the users to groups and then create the home folders.  Can you check my script and let me know of any issues you might see that I do not. Or any thing bad that might happen if something goes wrong.

Import-Module ActiveDirectory
$error.clear()
$MaximumErrorCount = 10000
Import-Csv "C:\Scripts\accounts\activestaff.csv" | ForEach-Object {

        switch ($_.Location)
	{
		Amgs {
			$OU = "OU=Staff,OU=As,dc=homelab,dc=com"
			$Description = "Staff - AS"
		}
		Bldwn {
			$OU = "OU=Staff,OU=Bn,dc=homelab,dc=com"
			$Description = "Staff - BN"
		}
		"CAM PPR" {
			$OU = "OU=Staff,OU=Cg,dc=homelab,dc=com"
			$Description = "Staff - CA"
		}
		CMBPRT {
			$OU = "OU=Staff,OU=Cmbrdgprt,dc=homelab,dc=com"
			$Description = "Staff - Ct"
		}
		"SCH DMN" {
			$OU = "OU=Staff,OU=Cc,dc=homelab,dc=com"
			$Description = "Staff - Ce"
		}
		CS {
			$OU = "OU=Staff,OU=CR,dc=homelab,dc=com"
			$Description = "Staff - CR"
		}
		"FLT/MN" {
			$OU = "OU=Staff,OU=FMA,dc=homelab,dc=com"
			$Description = "Staff - FM"
		}
		"J & P" {
			$OU = "OU=Staff,OU=Gs,dc=homelab,dc=com"
			$Description = "Staff - Gs"
		}
		Hggrt {
			$OU = "OU=Staff,OU=Hy,dc=homelab,dc=com"
			$Description = "Staff - HY"
		}
		Kng {
			$OU = "OU=Staff,OU=Kg,dc=homelab,dc=com"
			$Description = "Staff - KG"
		}
		"Kng pn" {
			$OU = "OU=Staff,OU=Kn,dc=homelab,dc=com"
			$Description = "Staff - KN"
		}
		"KNN/LNG" {
			$OU = "OU=Staff,OU=KL,dc=homelab,dc=com"
			$Description = "Staff - KLO"
		}
		MRS {
			$OU = "OU=Staff,OU=Me,dc=homelab,dc=com"
			$Description = "Staff - ME"
		}
		Pbdy {
			$OU = "OU=Staff,OU=Py,dc=homelab,dc=com"
			$Description = "Staff - PY"
		}
		"PTNM V"{
			$OU = "OU=Staff,OU=Pm,dc=homelab,dc=com"
			$Description = "Staff - Pm"
		}
		"RNDG V" {
			$OU = "OU=Staff,OU=Re,dc=homelab,dc=com"
			$Description = "Staff - Re"
		}
		Tbn {
			$OU = "OU=Staff,OU=Tn,dc=homelab,dc=com"
			$Description = "Staff - TN"
		}
		'VSSL LN' {
			$OU = "OU=Staff,OU=Vl,dc=homelab,dc=com"
			$Description = "Staff - Vl"
		}
		"SLMN" {
			$OU = "OU=Staff,dc=homelab,dc=com"
			$Description = "Staff - "
		}
		default
		{
			$OU = "OU=Staff,dc=homelab,dc=com"
			$Description = "Staff - "
		}
	}
if (dsquery user -samid $_.Login)
		{
			$LogonName = $_.Login2
		}
		else
       		 {
         		   $LogonName = $_.Login
       		 }
      
    $ADUser = [ordered]@{ }
	$ADUser['Name'] = $_.FirstName + " " + $_.LastName
	$ADUser['SamAccountName'] = $LogonName
	$ADUser['GivenName'] = $_.FirstName
	$ADUser['Surname'] = $_.LastName
    $ADUser['Description'] = $Description
	$ADUser['DisplayName'] = $_.FirstName + " " + $_.LastName
	$ADUser['UserPrincipalName'] = $LogonName + "@homelab.com"
	$ADUser['AccountPassword'] = ConvertTo-SecureString -AsPlainText 'P@ssw0rd' -Force
	$ADUser['Title'] = $_."Job Title"
	$ADUser['EmailAddress'] = $LogonName + "@homelab.com"
	$ADUser['Office'] = $_.EID
    $ADUser['Path'] = $OU
	$ADUser['Enabled'] = $True
	$ADUser['HomeDirectory'] = "\\dc-pc\share\$LogonName"
	$ADUser['HomeDrive'] = 'H:'
	New-ADUser @ADUser 


	Add-ADGroupMember "googleapps" –Members $LogonName
	Add-ADGroupMember "yard-staff" –Members $LogonName

    
	New-Item -type directory -path "\\dc-pc\share\$LogonName"
	$Acl = Get-Acl "\\dc-pc\share\$LogonName"
	$Ar = New-Object system.security.accesscontrol.filesystemaccessrule ("$LogonName", "Modify", "ContainerInherit, ObjectInherit", "None", "Allow")
	$Acl.SetAccessRule($Ar)
	Set-Acl "\\dc-pc\share\$LogonName" $Acl

}

Open in new window

RoccatAsked:
Who is Participating?
 
Dustin SaundersConnect With a Mentor Director of OperationsCommented:
How is that Login2 generated?  You can add checks to that in your code too, for say if they accidentally copy and pasted the same name for both.
if (dsquery user -samid $_.Login)
		{
            if ($_.Login -ne $_.Login2) {$LogonName = $_.Login2}
            else {
            $LogonName = $_.Login + "2" #this is just an example, but you can do logic here for fixing this.
            }
		}
		else
       	{
         	$LogonName = $_.Login
       	}

Open in new window


The biggest problem imo is that the code as it is has no validation of the fields being input and no logging of errors, so you could be tracking down problems for awhile.  
#region Validation
if (!$_.Login) {
WriteLog "Missing Login."
$error++
}
if (!$_.EID) {
WriteLog "Missing EID."
$error++
}
#endregion

Open in new window

If it creates the user, home directory, but something goes wrong setting ACL all you should need to do is go to the log file and look.  You can even add in at the end
if ($error -gt 0) {Invoke-Item $log}

Open in new window


To echo what Joseph said, anywhere you have the same item twice, you should make it a variable so it only needs to be changed in one place.

	    $ADUser['UserPrincipalName'] = $LogonName + "@homelab.com"
	    $ADUser['AccountPassword'] = ConvertTo-SecureString -AsPlainText 'P@ssw0rd' -Force
	    $ADUser['Title'] = $_."Job Title"
	    $ADUser['EmailAddress'] = $LogonName + "@homelab.com"

Open in new window


You can make this a variable earlier on and use that.
$uPN = $LogonName + "@homelab.com"

Open in new window

	    $ADUser['UserPrincipalName'] = $uPN
	    $ADUser['AccountPassword'] = ConvertTo-SecureString -AsPlainText 'P@ssw0rd' -Force
	    $ADUser['Title'] = $_."Job Title"
	    $ADUser['EmailAddress'] = $uPN

Open in new window


I'd also do a try{}catch{} on membership and the directory creation/acl.
0
 
Dustin SaundersDirector of OperationsCommented:
Is this the whole script?  What is:
$error.clear()
$MaximumErrorCount = 10000

Open in new window

for?
0
 
Dustin SaundersDirector of OperationsCommented:
A few suggestions:

If you're using this to create AD users, I'd add in validation to make sure essential fields aren't blank or invalid. Otherwise, this can cause your user creation to fail.

You should log create successes and fails somewhere, so if one user fails you can fix that.  I prefer to create an log writer function then I can be as verbose as I'd like as to where a problem occurred.

You can additionally add in a verification run at the end of the script, to double check that all directories were created and that users exist in AD, and add to your log any verification issues.
0
Simplify Active Directory Administration

Administration of Active Directory does not have to be hard.  Too often what should be a simple task is made more difficult than it needs to be.The solution?  Hyena from SystemTools Software.  With ease-of-use as well as powerful importing and bulk updating capabilities.

 
Joseph MoodyConnect With a Mentor Blogger and wearer of all hats.Commented:
I would do three things:

1. Add some comment blocks to separate out where variables are populated, ad user is created, ad user is added to groups, etc.

2. change the dsquery line to use get-aduser

3. replace the domain value (@homelab.com) and the file server value with variables. Populate those variables higher up in the script.
0
 
RoccatAuthor Commented:
Dustin, This is the whole script.  I was using those "$error.clear()
$MaximumErrorCount = 10000 " in my testing to clear the error count and set the error count maximum to 10,000.  I was going to use the $error variable to count and log errors.
0
 
Dustin SaundersDirector of OperationsCommented:
{
			$LogonName = $_.Login2
		}
		else
       	{
         	$LogonName = $_.Login
       	}

Open in new window


What is $_.Login2 ?
0
 
RoccatAuthor Commented:
Login 2 is a field in the csv file that offers a second login name if the first one is in use.
0
 
Dustin SaundersDirector of OperationsCommented:
Is that something you can add in to the code rather than CSV?  The less that people need to input the better (because human error is the biggest cause of issues in automated user account creation from my experience.)

For logging, something similar to this:

$log = "C:\log\userCreate.log"

function WriteLog($message)
{
    $d = Get-Date
    $out = $d + " -- " + $message
    Add-Content -Path $log -Value $out
}

Open in new window


try 
    {    
        $ADUser = [ordered]@{ }
	    $ADUser['Name'] = $_.FirstName + " " + $_.LastName
	    $ADUser['SamAccountName'] = $LogonName
	    $ADUser['GivenName'] = $_.FirstName
	    $ADUser['Surname'] = $_.LastName
        $ADUser['Description'] = $Description
	    $ADUser['DisplayName'] = $_.FirstName + " " + $_.LastName
	    $ADUser['UserPrincipalName'] = $LogonName + "@homelab.com"
	    $ADUser['AccountPassword'] = ConvertTo-SecureString -AsPlainText 'P@ssw0rd' -Force
	    $ADUser['Title'] = $_."Job Title"
	    $ADUser['EmailAddress'] = $LogonName + "@homelab.com"
	    $ADUser['Office'] = $_.EID
        $ADUser['Path'] = $OU
	    $ADUser['Enabled'] = $True
	    $ADUser['HomeDirectory'] = "\\dc-pc\share\$LogonName"
	    $ADUser['HomeDrive'] = 'H:'
	    New-ADUser ADUser
        WriteLog("Successfully created user $LogonName.")
    }
    catch
    {
        WriteLog("Failed creating user $LogonName.")
        $error++
    }

Open in new window

0
 
RoccatAuthor Commented:
I was going to create the second login option in the script but the other admin offered to generate the second login column when he generates the reports that populates the csv file.
0
 
RoccatAuthor Commented:
Thank you!!
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.