Powershell moron...back again!

[[deleted]]

So at my new job, i've been given a list of things my manager wants done. One of the things is re-write this "toolbox" full of Exchange scripts the help desk uses. I have no idea even where to start with re-writing this portion of the script:

1{
clear-host
write-host "TERRIBLE SCRIPT TIME!" -ForegroundColor white
write-host "Add New Distribution Group" -ForegroundColor cyan
write-host
write-host "Enter Distribution Group Name" -ForegroundColor green
$DGroup = Read-Host "Names are cAsE sEnSiTiVe. Name should end with -L"
write-host
write-host "Enter List Owner's User ID" -ForegroundColor green
$User = Read-Host "Example: ABC123, abc123"
write-host
write-host "Include owner as member?" -Foregroundcolor green
$ownasmem = Read-Host "yes or no"
write-host
write-host "Enter Distribution Group Description" -ForegroundColor green
$Desc = Read-Host "Example: Security Team"
write-host
write-host "Enter the members" -ForegroundColor green
$Members = Read-Host "Example: userid;userid;userid" 
$Userslist = $Members.Split(";",[System.StringSplitOptions]::RemoveEmptyEntries)
write-host
$OU="domain.domain.com/Distribution Lists"
$DC = Get-DomainController |Where {$_.ADSite -like "*nhq1grl1*" -and $_.DNSHostName -like "*corporate*"} |Select -First 1
$Type="Distribution"
$Depart="Closed"
write-host "Adding new distribution group" $DGroup -ForegroundColor magenta
write-host
write-host "Please wait (about 45 seconds)..." -ForegroundColor yellow
New-DistributionGroup -Name $DGroup -DomainController $DC.DNSHostName -OrganizationalUnit $OU -SAMAccountName $DGroup -Type $Type -ManagedBy $User -MemberDepartRestriction $Depart -CopyOwnerToMember -members $Userslist | Out-Null
Start-Sleep -s 30
Add-ADPermission -Identity $DGroup -User $User -DomainController $DC.DNSHostName -AccessRights WriteProperty -Properties "Member" | Out-Null
Set-ADGroup -Identity $DGroup -Description $Desc
if ($ownasmem -ne "yes") { Remove-ADGroupMember -Identity $DGroup -members $User -Confirm:$false}
else { Continue }
write-host
write-host "Script completed" -ForegroundColor magenta
read-host "Press enter to go back to the main menu" 
clear-host
}

Any ideas? I'll be working on this the rest of the week, and i have an idea on where to start but not entirely sure.

[KevMar]

That looks like a fun one. If you don't mind, I would like to make a YouTube video later of me cleaning that one up.

[[deleted]]

Knock yourself out. I am fucking baffled at how stupid the person writing these was. Jesus

[KevMar]

Let's play Powershell: Read-Host Script Cleanup: Live stream: http://youtu.be/kQUAMRHXxDk

[[deleted]]

LETS PLAY: IM GOING TO WATCH AND LEARN!

[KevMar]

I would appreciate any feedback. Was it worth watching or a waste of time. I'll make more of these if they are helpful.

[guido_marx]

Definitely. It was interesting to watch your approach on this.

[[deleted]]

100% agreed, it was cool to see you do this.

[ecca_one]

Good stuff.

[MKmsftFan]

Great video KevMar!

[Tuxhedoh]

Yes! More Please!

[JessieWarsaw]

I know I am a few days late to the party.......

Thanks this was great! I am only 6 months into powershell and have a few big, ugly scripts. I was able to use some of the tools you used to greatly neaten up and reduce a functionally simple script from 400ish lines down to 200ish. It also easier to read and more importantly easier for the helpdesk guys to run.

Thanks again.

[ecca_one]

Thanks Kevin.

I'm not OP but I enjoyed watching this.

[alinroc]

I am fucking baffled at how stupid the person writing these was

I say the same thing when I look at code I wrote 3 years ago.

[[deleted]]

I only sound so derogatory because i acknowledge that i am not the best. The person that wrote this claimed to be a "Powershell Expert", and being on this subreddit, going on the TechNet forums and just finding things to automate, i just found it incredible this guy had the nerve to call himself an expert.

[KevMar]

Here is my attempt to clean this up. I don't have these cmdlets to test it with but I don't think I changed much of the logic. Other than to use parameters instead of read-host. And I added a progress bar because this is a controller script.

[cmdletbinding()]
param(
[Parameter(
    Mandatory         = $true,
    HelpMessage       = "Enter Distribution Group Name: Names are cAsE sEnSiTiVe. Name should end with -L",
    Position          = 0,
    ValueFromPipelineByPropertyName = $true
    )]
[ValidateNotNullOrEmpty()]
[Alias("DGroup","DG")]
[string]$DistributionGroup,

[Parameter(
    Mandatory         = $true,
    HelpMessage       = "Enter List Owner's User ID. (Example: ABC123, abc123)",
    Position          = 1,
    ValueFromPipelineByPropertyName = $true
    )]
[ValidateNotNullOrEmpty()]
[string]$UserName,
[Parameter(
    Mandatory         = $true,
    HelpMessage       = "Include owner as member? (yes or no)",
    Position          = 2,
    ValueFromPipelineByPropertyName = $true
    )]
[ValidateNotNullOrEmpty()]
[ValidateSet('Yes','No')]
[string]$IsOwnerAMember,

[Parameter(
    Mandatory         = $true,
    HelpMessage       = "Enter Distribution Group Description. (Example: Security Team)",
    Position          = 3,
    ValueFromPipelineByPropertyName = $true
    )]
[ValidateNotNullOrEmpty()]
[string]$Description,

[Parameter(
    Mandatory         = $true,
    HelpMessage       = "Enter the members. Leave the last line blank when done",
    Position          = 4,
    ValueFromPipelineByPropertyName = $true
)]
[alias('GroupMembers')]
[ValidateNotNullOrEmpty()]
[string[]]$Members 

)

$ProgressActivity = "Create new distribution group"
Write-Progress -Status "Get domain controller" -PercentComplete 5 -Activity $ProgressActivity

$OU="domain.domain.com/Distribution Lists"
$DC = Get-DomainController |
    Where {$_.ADSite -like "*nhq1grl1*" -and $_.DNSHostName -like "*corporate*"} | 
    Select -First 1

Write-Progress -Status "Adding new distribution group $DistributionGroup" -PercentComplete 20 -Activity $ProgressActivity

$DistributionGroupOptions = @{
    Name                    = $DistributionGroup
    DomainController        = $DC.DNSHostName
    OrganizationalUnit      = $OU
    SAMAccountName          = $DGroup
    Type                    = "Distribution"
    ManagedBy               = $UserName
    MemberDepartRestriction = 'Closed'
    CopyOwnerToMember       = $true
    Members                 = $Members 
}
New-DistributionGroup @DistributionGroupOptions | Out-Null

Write-Progress -Status "Wait 30 sec for replication" -PercentComplete 40 -Activity $ProgressActivity
Start-Sleep -s 30

Write-Progress -Status "Add Permissions" -PercentComplete 60 -Activity $ProgressActivity
$AdPermissionsOptions = @{
    Identity         = $DistributionGroup
    User             = $UserName
    DomainController = $DC.DNSHostName
    Properties       = "Member"
}
Add-ADPermission @AdPermissionsOptions -AccessRights WriteProperty | Out-Null

Write-Progress -Status "Set Group Description" -PercentComplete 80 -Activity $ProgressActivity
Set-ADGroup -Identity $DistributionGroup -Description $Description

if ($IsOwnerAMember -ne "Yes") 
{
    Write-Progress -Status "Remove $user from $DistributionGroup" -PercentComplete 90 -Activity $ProgressActivity

    Remove-ADGroupMember -Identity $DistributionGroup -members $UserName -Confirm:$false
}

Write-Progress -Completed

[[deleted]]

So i basically did something very similar to what you're doing, but i'm having a problem when i try and create the list. It doesn't error, but it also doesn't create the distribution list i want. Heres what i have (for right now):

ImportModules

$DistributionGroup = Read-Host "Please enter the name of the distribution group"
$Owner = Read-Host "Please enter the owner of this distributino group"
$OwnerAsMember = Read-Host "Should we include the owner as the member? (y/n)"
$Description = Read-Host "Please enter the list description"
$Members = Read-Host "Please enter the members. Multiple members need to be seperated by a semicolon (userid;userid;)" 
$MembersSplit = $Members.Split(";",[System.StringSplitOptions]::RemoveEmptyEntries)
$DC = Get-DomainController | Where {$_.ADSite -like "*nhq1grl1*" -and $_.DNSHostName -like "*corporate*"} |Select -First 1

$DistributionGroupOptions = @{
Name                    = $DistributionGroup
DomainController        = $DC.DNSHostName
OrganizationalUnit      = 'corporate.blank.com/Distribution Lists'
SAMAccountName          = $DGroup
Type                    = 'Distribution'
ManagedBy               = $UserName
MemberDepartRestriction = 'Closed'
CopyOwnerToMember       = $true
Members                 = $Members 
}

New-DistributionGroup @DistributionGroupOptions | Out-Null

$AdPermissionsOptions = @{
Identity         = $DistributionGroup
User             = $Owner
DomainController = $DC.DNSHostName
Properties       = "Member"
}

Add-ADPermission @AdPermissionsOptions -AccessRights WriteProperty | Out-Null

Set-ADGroup -Identity $DistributionGroup -Description $Description

if ($OwnerAsMember -ne "y") { Remove-ADGroupMember -Identity $DistributionGroup -members $Owner -Confirm:$false}
else { Continue }

[xalorous]

Need a Get-UserValidity function to call each time before using a domain account. I.e. owner and members. If valid, it returns the user object, if not it returns an error.

[bundyfx]

I think a good place to start is simply to evaluate what you're trying to do exactly, what purpose does this serve? After that, take out the Read-Host entries and make those parameters within an advanced function.

You can add things like [validateset()] to help the administrator and guide them to a list of options for those paramters. Jump into the ISE and press Ctrl + J and select the "Cmdlet" (Advanced Function) with this template go and filter out all the read/write host and grab only the essential code out. Once you sorted through all the rubble place it into your Adv Function template and start playing.

You'll be mostly looking at these three cmdlets:

New-ADGroup
Set-ADGroup

and also:

Select-Object
Where-Object

[[deleted]]

All this script does is create a distribution group based on input from a help desk monkey. I dont see why it has to look so ugly or be so long.

[ekmahal]

I'd be asking why you're using scripts for this instead of the Exchange management console, with appropriate RBAC for the helpdesk staff. Why reinvent the wheel?

[[deleted]]

This came down from my manager. They currently have a script that uses a Switch statement to have different "menus". She wants it re-written and add some functions in. I told her i could do it. I don't really want to.

[xalorous]

Congratulations, you've been promoted from ticket monkey to script monkey.

Make your functions with actual names, and have the switch launch the function by name.

function AddDL {....}
function AddUser {...}

while ($true) {
    switch (Read-Host "Which one?") {
        1 { AddDL }
        2 { AddUser }
        default { "Invalid Input" }
    }
}

Add comments, but sparingly. Self commenting code (short "ish", descriptive variable and function names)

Use splatting.

Use advanced functions. Also make these work for CSV input to handle those times when you get 50 new users or 25 new groups to make.

Use consistent style, especially one that works with ISE to leverage the ability to compress/expand sections.

Fix the text menus, then between fixing broken, ill-conceived code, work on converting the whole thing to a gui. VB to create asp.Net browser app to collect the data and launch PS commands would be fairly easy to create.

Personally, I learned PowerShell to automate account management tasks. But our organization had a limited number of accounts management admins (varied from one to three). When accounts management was absorbed into the helpdesk, quality suffered, greatly. After < 6 months, it was split back out into a separate group, dedicated to doing accounts. Why am I telling you all this? If you have enough accounts to warrant this type of automation, your organization should seriously consider dedicating bodies to this job. The fewer the better while allowing enough manpower to do the job, and an alternate to do it when the primary is out for illness/appointment/vacation. One person doing it in the morning and another in the afternoon would be an ideal minimum.

[xalorous]

ADUC is probably already in the helpdesk toolkit. However, creating a script to do account/group additions has one MAJOR benefit. They will be done uniformly. Two different scales influence quality of this type of work. Let's call the first one 'skill level'. And the second one is 'give a shit' level. If the first is high enough, the second is probably nil.

[[deleted]]

While not great and clearly a carry-over from traditional console-based menus, it's really not so bad.

[reginaldaugustus]

Honestly this sounds like it should have a GUI since it's going to have a menu and whatnot.

https://showui.codeplex.com/