Want to win a PS4? Go Premium and enter to win our High-Tech Treats giveaway. Enter to Win

x
?
Solved

Review my code please

Posted on 2016-08-10
10
Medium Priority
?
48 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 1000 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
Has Powershell sent you back into the Stone Age?

If managing Active Directory using Windows Powershell® is making you feel like you stepped back in time, you are not alone.  For nearly 20 years, AD admins around the world have used one tool for day-to-day AD management: Hyena. Discover why.

 

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 1000 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

Are your AD admin tools letting you down?

Managing Active Directory can get complicated.  Often, the native tools for managing AD are just not up to the task.  The largest Active Directory installations in the world have relied on one tool to manage their day-to-day administration tasks: Hyena. Start your trial today.

Question has a verified solution.

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

This process allows computer passwords to be managed and secured without using LAPS. This is an improvement on an existing process, enhanced to store password encrypted, instead of clear-text files within SQL
How to deal with a specific error when using the Enable-RemoteMailbox cmdlet to create a mailbox in the cloud-based service, for an existing user in an on-premises Active Directory.
Attackers love to prey on accounts that have privileges. Reducing privileged accounts and protecting privileged accounts therefore is paramount. Users, groups, and service accounts need to be protected to help protect the entire Active Directory …
Screencast - Getting to Know the Pipeline

597 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