0
\$\begingroup\$

I created a function for creating a new user in AD, assigning an o360 license and enabling mailbox, however, my code is very chaotic and I do not have a much experience with PowerShell. Could you please advise on any improvements, code optimization or generally what could be done better?

Function CreateUser{
#$GroupsAvalible = @(Get-ADGroup -Filter 'GroupCategory -eq "Security" -and GroupScope -ne "DomainLocal"') #| Select-Object Name)
#Show-Menu $GroupsAvalible.name -MultiSelect 
Clear-Host
$searchbase = 'OU=Company Users,OU=Company,DC=company,DC=local'
$OuList = @(Get-ADOrganizationalUnit -SearchBase $searchbase -SearchScope Subtree -Filter * | Select-Object Name)
#$GroupsAvalible = @(Get-ADGroup -Filter 'GroupCategory -eq "Security" -and GroupScope -ne "DomainLocal"' | Select-Object Name)
 Add-Type -AssemblyName 'System.Web'
$password = [System.Web.Security.Membership]::GeneratePassword(14,8)
$fname = Read-Host -Prompt 'First Name'
$lname = Read-Host -Prompt 'Last Name'
$flname = $fname.Substring(0,1) + $lname
#$cn = $fname + " " + $lname
$ext = Read-Host -Prompt ‘Ext Number’
if ([string]::IsNullOrWhiteSpace($ext)){$ext = ‘0000’}
$email = $flname + '@company.com'
if(($email = Read-Host "Email is set to: $email Press Enter to accept or type new") -eq ''){$email = $flname + '@company.com'}#else{$email}
Clear-Host
'Select OU to put user in'
$OU = Show-Menu($OuList.Name)
$test = "OU=$OU,$searchbase"
Clear-Host
New-ADUser -Name "$fname $lname" -OtherAttributes @{'homeDirectory'="\\company-storage\Users\$flname";'homeDrive'='L';'ipPhone'=$ext} -Path $test -SamAccountName $flname -UserPrincipalName ($fname.Substring(0,1) + $lname + "@company.com") -AccountPassword (ConvertTo-SecureString -String $password -AsPlainText -Force) -ChangePasswordAtLogon $true -Enabled $true
$confirmation = Read-Host "Would you like to copy groups from other user in same OU? Y/N"
if ($confirmation -eq 'Y') {
$UserToCopy = Get-ADUser -Filter * -SearchBase $test
$UserToCopy2 = Show-Menu($UserToCopy.SamAccountName)

Get-ADUser -Identity $UserToCopy2 -Properties memberof | Select-Object -ExpandProperty memberof |  Add-ADGroupMember -Members $flname

}
#Assignin Licensces!!!!!!!!<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  $Usrnm = $env:UserName
  $usrnmat = $Usrnm + "@company.com"
  $password=Read-Host "Enter administrator password for $usrnmat" -AsSecureString 
  $cred=New-Object System.Management.Automation.PSCredential($Usrnm,$password)
  $cred2=New-Object System.Management.Automation.PSCredential($usrnmat,$password)
  $upn = $email
  
  
  "connecting to Azure...:";Connect-MsolService -Credential $Cred2;"Ok"
  "Connecting to DC04...";$s = New-PSSession -ComputerName DC-DC04 -credential $cred;"Ok"
  Invoke-Command -Session $s -Scriptblock {
  "Importing ADSync Module...";Import-Module -Name "c:\Program Files\Microsoft Azure AD Sync\Bin\ADSync\ADSync.psd1";"Ok"
  "Forcing Azure Sync...";Start-ADSyncSyncCycle -PolicyType Delta;"Ok"}
  Do{$User = Get-MsolUser -UserPrincipalName $upn -ErrorAction SilentlyContinue
    Start-Sleep 1
    Clear-Host
    "Waiting for $upn to sync to Azure"
  } Until($Null -ne $user)
  write-host "$user Synced"
  "Closing Connection with DC04...";Remove-PSSession $s; "Ok"
  "Setting user UsageLocation to GB..."; Set-MsolUser -UserPrincipalName $upn -UsageLocation GB ; "Ok"
    $chs = Show-Menu -MenuItems ("Bussines","E3")
  if ($chs -eq "Bussines"){$License = "company:O365_BUSINESS_PREMIUM"}
  if ($chs -eq "E3"){$License = "company:ENTERPRISEPACK"}
  #Get-MsolUser -All -UnlicensedUsersOnly
  $EnabledPlans = @(
      'TEAMS1'
      'O365_BUSINESS'
      'OFFICESUBSCRIPTION'
  )
  $AllPlans = (Get-MsolAccountSku | Where-Object { $_.AccountSkuId -eq $License } | Select-Object -ExpandProperty ServiceStatus).ServicePlan.ServiceName
  $DisabledPlans = $AllPlans | Where-Object { $EnabledPlans -notcontains $_ }
  $E1CustomizedLicense = New-MsolLicenseOptions -AccountSkuId $License -DisabledPlans $DisabledPlans
  Set-MsolUserLicense -UserPrincipalName $upn -AddLicenses $License -LicenseOptions $E1CustomizedLicense
 
  #Setting Up MailBox!!!!!<<<<<<<<<<<<<<<<<<<<<<<<<
  
Get-PSSession | Remove-PSSession
$Session = New-PSSession -ConfigurationName Microsoft.Exchange -ConnectionUri http://company-exchange1/powershell/ -Authentication Kerberos -AllowRedirection
Import-PSSession $Session –DisableNameChecking -AllowClobber
Clear-Host
Write-Host "This fucker thinks MB is bigger than GB so automation is impossible, select DB with biggest free space..." -ForegroundColor Red
Get-MailboxDatabase -Status | Where-Object {$_.Identity -notlike "company-Archive*"} | Select-Object name,AvailableNewMailboxSpace

$db1 = Get-MailboxDatabase | Where-Object {$_.Identity -notlike "company-Archive*"}# | Select AvailableNewMailboxSpace | Sort AvailableNewMailboxSpace -Descending
$db1 = Show-Menu -MenuItems ($db1)
$db = Get-MailboxDatabase | Where-Object {$_.Identity -eq $db1} | select-object name
$ach1 = $db1 -replace "Database", "Archive"
$ach = Get-MailboxDatabase | Where-Object {$_.Identity -eq $ach1} | select-object name
$ach.name
$db.name
Enable-Mailbox -database $db.name -Identity $flname -Alias $flname
pause
Get-PSSession | Remove-PSSession



")}
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Overall it looks good. There's definitely room for improvement, especially if you intend to share the script and/or having someone else work on it, but the business logic looks good to me and there's nothing outright bad.

Reliability

Mind the replication times between DCs

You're starting a sync cycle from a DC named DC04, so I'm assuming you have others, and as you don't specify a server in your New-ADUser call you may end up creating the user in another DC and starting a cycle while the user isn't on DC-DC04 yet. Either add -Server DC-DC04 to your New-ADUser call, or force a replication with repadmin.exe /syncall DC-DC04 dc=your,dc=domain /d /e after creating the user, if your account has the permissions needed.

Import your modules as soon as possible and stop if you can't

Not having a module installed will break your script entirely, so try to import everything as soon as possible (meaning at the beginning of the function), and wrap the imports into a try catch block to prevent creating a half-baked user that you will need to fix manually later in case you're missing a module.

 try {
     Import-Module xxx
     Import-Module yyy
 } catch [FileNotFoundException] {
     # Display a message and exit, send an alert e-mail, etc...
 }

I quote, "This fucker thinks MB is bigger than GB"

I don't have an Exchange at hand so I can only assume that it tries to sort alphabetically instead of numerically ? Just in case, here's this snippet I use to convert strings like "2 GB", "1.235PB" or "4M" into actual, sortable numbers. Be careful, it assumes you're working on bytes and not bits, and 1K = 1024. It returns a double you can sort.

function Convert-NumStringToInt([String]$NumAsString) {
    $x,$number,$suffix = ($NumAsString | select-string -Pattern "(.*?)([KMGTPB]{1,2})$").Matches.Groups.Value
    switch ($suffix[0]) {
        P { $NumUnit = [double]$number * 1PB }
        T { $NumUnit = [double]$number * 1TB }
        G { $NumUnit = [double]$number * 1GB }
        M { $NumUnit = [double]$number * 1MB }
        K { $NumUnit = [double]$number * 1KB }
        Default {$NumUnit = [double]$number}
    }
    return $NumUnit
}

You can use it as a calculated property then sort according to the results like so:

 $MailboxDatabase | Select *,@{l="SizeAsNumber";e={Convert-NumStringToInt $_.AvailableNewMailboxSpace}} | Sort SizeAsNumber

Readability

Use longer variable names

You don't pay extra if you use longer variable names, and we aren't counting kilobytes like it's the eighties anymore, so consider giving your variable explicit names, like $FirstName, $ExtNumber or $UsernameAtDomain. Whoever will have to maintain that script after you will thank you.

Use Write-Progress to, well, indicate your progress

You use bare strings at some points like when connecting to MSOL and when creating the mailbox($ach.name), consider using Write-Progress to make it into a progress bar-style status message or Write-Output to log info to the standard output. It's more verbose, but you're not being paid by line saved, are you ?

   Write-Progress "connecting to Azure...:"
   Connect-MsolService -Credential $Cred2
   Write-Progress "Connecting to DC04..."
   $s = New-PSSession -ComputerName DC-DC04 -credential $cred;
   [...]

Use Get-Credential instead of Read-Host + New-Object

  • You can use Get-Credential -Username $usrnmat -Message "Enter administrator password for $usrnmat" instead of asking for a SecureString and creating the PSCredential by hand.

Break up long lines

You can break some of the longer lines (like the New-ADUser one) with a backtick, like so:

  New-ADUser -samaccountname $samname `
             -Name $username `
             -userprincipalname $upn

Best practices

Consider doing your input as parameters instead of reading the terminal

You're querying the user for input a lot throughout your script. If what you're asking for doesn't depend on other data (like asking if the email generated from fname and lname is fine), consider using parameters instead. You have two ways of making mandatory parameters, as in, your script will ask for them if you don't specify them when calling the function:

  • Using the built-in Mandatory attribute for parameters. It's clean and concise, but will only display a prompt with the name of the variable to the user, nothing else.
  • You can use an expression as a default value, and you can use Read-Host in this expression to query the user. This is more verbose, makes it harder to have an actual default value (see $ext in the second example) but it's way more user-friendly.

So instead of doing this:

 $fname = Read-Host -Prompt 'First Name'
 $lname = Read-Host -Prompt 'Last Name'
 [...]
 $ext = Read-Host -Prompt ‘Ext Number’
 if ([string]::IsNullOrWhiteSpace($ext)){$ext = ‘0000’}

You can use this:

 Function CreateUser {
 param(
     [Parameter(Mandatory)]
     [string] $fname,

     [Parameter(Mandatory)]
     [string] $lname,
     
     [string] $ext = '0000'

 )

Or this:

 Function CreateUser {
 param(
     [string] $fname = Read-Host -Prompt "User's first name",

     [string] $lname = Read-Host -Prompt "User's last name",
     
     [string] $ext = $(if ($value=Read-Host "Ext number") {$value} else {'0000'})
 )

Prefer making your output clear instead of manipulating the terminal

At several points you're using Clear-Host to "separate" steps of your process, this won't work if your logging to a file and is generally not considered best-practice. Why not either use Write-Progress to display the current step, or simply output a few newlines and/or a separator like a line of dashes ?

Also, prefer Write-Output to Write-Host, it'll work better with logging to a file or running into a PSSession (powershell's "SSH") as it logs to the standard input which you can redirect, instead of straight to the terminal. Though, a valid use case for Write-Host is if you're sure the script will be run locally, interactively, and if you need color (check out -BackgroundColor and -ForegroundColor).

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.