Solved

Review my code please

Posted on 2016-08-10
10
45 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 5
  • 4
10 Comments
 
LVL 13

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 13

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
Office 365 Training for Admins - 7 Day Trial

Learn how to provision tenants, synchronize on-premise Active Directory, implement Single Sign-On, customize Office deployment, and protect your organization with eDiscovery and DLP policies.  Only from Platform Scholar.

 

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 13

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 13

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 13

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

Featured Post

Edgartown IT Case Study

Learn about Edgartown's quest to ensure the safety and security of the entire town's employee and citizen data. Read the case study!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

A hard and fast method for reducing Active Directory Administrators members.
Group policies can be applied selectively to specific devices with the help of groups. Utilising this, it is possible to phase-in group policies, over a period of time, by randomly adding non-members user or computers at a set interval, to a group f…
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 Micro Tutorial hows how you can integrate  Mac OSX to a Windows Active Directory Domain. Apple has made it easy to allow users to bind their macs to a windows domain with relative ease. The following video show how to bind OSX Mavericks to …

717 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