Solved

Review my code please

Posted on 2016-08-10
10
36 Views
Last Modified: 2016-08-11
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

0
Comment
Question by:Roccat
  • 5
  • 4
10 Comments
 
LVL 12

Expert Comment

by:Dustin Saunders
ID: 41750467
Is this the whole script?  What is:
$error.clear()
$MaximumErrorCount = 10000

Open in new window

for?
0
 
LVL 12

Expert Comment

by:Dustin Saunders
ID: 41750476
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
 
LVL 22

Assisted Solution

by:Joseph Moody
Joseph Moody earned 250 total points
ID: 41750478
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
 

Author Comment

by:Roccat
ID: 41750531
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
 
LVL 12

Expert Comment

by:Dustin Saunders
ID: 41750546
{
			$LogonName = $_.Login2
		}
		else
       	{
         	$LogonName = $_.Login
       	}

Open in new window


What is $_.Login2 ?
0
 

Author Comment

by:Roccat
ID: 41750551
Login 2 is a field in the csv file that offers a second login name if the first one is in use.
0
 
LVL 12

Expert Comment

by:Dustin Saunders
ID: 41750571
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
 

Author Comment

by:Roccat
ID: 41750586
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
 
LVL 12

Accepted Solution

by:
Dustin Saunders earned 250 total points
ID: 41750624
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
 

Author Closing Comment

by:Roccat
ID: 41752104
Thank you!!
0

Join & Write a Comment

Resolve DNS query failed errors for Exchange
This article explains how to prepare an HTML email signature template file containing dynamic placeholders for users' Azure AD data. Furthermore, it explains how to use this file to remotely set up a department-wide email signature policy in Office …
This tutorial will walk an individual through the process of transferring the five major, necessary Active Directory Roles, commonly referred to as the FSMO roles from a Windows Server 2008 domain controller to a Windows Server 2012 domain controlle…
This tutorial will walk an individual through the process of transferring the five major, necessary Active Directory Roles, commonly referred to as the FSMO roles to another domain controller. Log onto the new domain controller with a user account t…

746 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

13 Experts available now in Live!

Get 1:1 Help Now