5
\$\begingroup\$

I think I'm finally done with my code and was hoping for some pointers. My goal for this script was to:

  • Create new folder
  • Create AD group FS-TESTSHARE-R
  • Create AD group FS-TESTSHARE-RW
  • Apply both groups to the new share folder
  • Set full read permissions to FS-TESTSHARE-R
  • Set full read/Rights permissions to FS-TESTSHARE-RW
  • Set full access permissions for local machine admins and domain admins

Everything seems to be working ok, but I was wondering if there was a cleaner way to do this or if I'm on the right track. Right before posting this I was doing the "FS-$($NAME.replace(' ',''))-RW".toupper() in all of the individual spots where I now have $ADNameRW (and $ADNameR), but I realized that it would work better to create the string first.

Any other tips or suggestions on my creation?

$FileShare = Read-Host -prompt "Verify Fileshare Server Name - (ie ECCOFS01)" 
if ( ($FileShare -eq $null) -or ($FileShare -eq "") -or ($FileShare -eq " ") -or ($FileShare -eq "c:") -or ($FileShare -eq "d:"))
  {
    Write-Host "You entered $FileShare which is an incorrect value: This Script is now Exiting." -Foreground "White" -Background "Red"
    Exit
  }
else 
  {
    Write-Host "You Entered $FileShare"
  }

$Parent = Read-Host -prompt "Enter full parent path that will contain the new folder (ie. \\eccofs01\Groups\ECCO IT\ or C:\Test Share)" 
if ( ($Parent -eq $null) -or ($Parent -eq "") -or ($Parent -eq " ") -or ($Parent -eq "c:") -or ($Parent -eq "d:"))
  {
    Write-Host "You entered $Parent which is an incorrect value: This Script is now Exiting." -Foreground "White" -Background "Red"
    Exit
  }
else 
  {
    Write-Host "You Entered $Parent"
  }

$Name = Read-Host -prompt "Enter New Folder Name." 
if ( ($Name -eq $null) -or ($Name -eq "") -or ($Name -eq " "))
  {
    Write-Host "You entered $Name which is an incorrect value: This Script is now Exiting." -Foreground "White" -Background "Red"
    Exit
  }
else 
  {
    Write-Host "You Entered $Name"
  }

$Path = "$($parent)\$($Name)" 
    Write-Host = "New Folder Path = $Path"

$Country = Read-Host -prompt "Enter the Country OU that the Security Group will reside in (i.e. Global, Americas, Europe, Asia Pacific)." 
if ( ($Country -eq $null) -or ($Country -eq "") -or ($Country -eq " "))
  {
    Write-Host "You entered $Country which is an incorrect value: This Script is now Exiting." -Foreground "White" -Background "Red"
    Exit
  }
else 
  {
    Write-Host "You Entered $Country"
  } 


# These do not check for empty values - that can be figured out later
#$Parent = read-host -prompt "Enter full parent path that will contain the new folder (ie. \\eccofs01\Groups\ECCO IT\ or C:\Test Share)"
#$Name = read-host -prompt "Enter New Folder Name."
#$Country = read-host -prompt "Enter the AD Security Group Country (i.e. Global, Americas, Europe, Asia Pacific)"
#$City = read-host -prompt "Enter City name: spelling must be correct. You can also leave this blank to create Security Group in the country-specific OU"


Import-Module ActiveDirectory


$FileShareFull = $($FileShare.replace(' ',''))
$NameFull = ($NAME.replace(' ',''))

$ADNameRW = "FS-$FileShareFull-$Name-RW".toupper()
$ADNameR = "FS-$FileShareFull-$Name-R".toupper()

# Create Security Groups =
# Creates the parameters for the new groups
# Capitalizes and removes spaces from the answer to $Name
# Creates the Security Groups in the appropriate Active Directory OU based off of the answser to $Country
$GroupParams1= @{
    'Name' = $ADNameRW
    'SamAccountName' = $ADNameRW
    'GroupCategory' = "Security"
    'GroupScope' = "Global"
    'DisplayName' = "$NAME Read-Write Access"
    'Path' = "OU=$Country,OU=FILE SHARE GROUPS,OU=Security Groups,DC=esg,DC=intl"
    'Description' = "Members of this group have read-write access to $Path."
}

New-ADGroup @GroupParams1

$GroupParams2= @{
    'Name' = $ADNameR
    'SamAccountName' = $ADNameR
    'GroupCategory' = "Security"
    'GroupScope' = "Global"
    'DisplayName' = "$NAME Read-Write Access"
    'Path' = "OU=$Country,OU=FILE SHARE GROUPS,OU=Security Groups,DC=esg,DC=intl"
    'Description' = "Members of this group have read access to $Path"
}

New-ADGroup @GroupParams2

# Create New Share Folder
New-Item -Path $Path -ItemType Directory

# Create initial ACE
# Create the initial Object
# Set domain - This could also be changed to prompt for domain if we decide it is needed
# Define local Administrators group by Well Known SID
# Set additional ACEs for the new AD File Share Groups
# Set ACLs on the new folder
function New-Ace {
  [CmdletBinding()]
  Param(
    [Parameter(Mandatory=$true, Position=0)]
    [Security.Principal.NTAccount]$Account,
    [Parameter(Mandatory=$false, Position=1)]
    [Security.AccessControl.FileSystemRights]$Permissions = 'ReadAndExecute',
    [Parameter(Mandatory=$false, Position=2)]
    [Security.AccessControl.InheritanceFlags]$InheritanceFlags = 'ContainerInherit,ObjectInherit',
    [Parameter(Mandatory=$false, Position=3)]
    [Security.AccessControl.PropagationFlags]$PropagationFlags = 'None',
    [Parameter(Mandatory=$false, Position=4)]
    [Security.AccessControl.AccessControlType]$Type = 'Allow'
  )

  New-Object Security.AccessControl.FileSystemAccessRule(
    $Account, $Permissions, $InheritanceFlags, $PropagationFlags, $Type
  )
}

$domain = 'ESG.INTL'

$administrators = ([wmi]"Win32_Sid.Sid='S-1-5-32-544'").AccountName

$acl = Get-Acl $path

$administrators, "$domain\Domain Admins" | ForEach-Object {
  $acl.AddAccessRule((New-Ace $_ 'FullControl'))
}
$acl.AddAccessRule((New-Ace $ADNameRW 'Modify'))
$acl.AddAccessRule((New-Ace $ADNameR 'ReadAndExecute'))

Set-Acl $path $acl
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$
    – Dan
    Apr 14 '16 at 12:25
  • \$\begingroup\$ Sorry, for that. I was looking for the rules last night but was so tired I gave up. Thanks! \$\endgroup\$ Apr 14 '16 at 14:11
2
\$\begingroup\$

Advanced Functions

I would have a serious look at the about_advanced_function_parameters. In the beginning you are doing some parameter validation on read-host Input. I would consider making this into an advanced script capable of accepting parameters so that you can make a series of calls to it and you don't need to rely on Read-Host which can get tedious if you are going to be running this several times at once.

Another set back in this is that if a user gets the first 2 prompts correct and makes a mistake on the third they have to start over again. You could go a couple of different routes but in its simplest form perhaps you should give users a couple of chances so they have a margin for error. If you used parameters like I mentioned earlier then the could just call the command again and fix the mistake without going through all the prompts.

Input Validation

I see logic this several times which is you trying to verify that a string is not null or empty space....

($Name -eq $null) -or ($Name -eq "") -or ($Name -eq " ")

The string static method [string]::IsNullOrWhiteSpace() would cover all those bases nicely.

If([string]::IsNullOrWhiteSpace($name)){"Do Stuff"}else{"Do Less Stuff"}

String Concatenation for Creating File Paths

You gather both parent folder and folder name separately and put them together later. Consider using the [io.path] method Combine() which will make it so you don't have to worry about the presence of a slash in the input from the user.

$Path = [IO.Path]::Combine($parent, $Name)

Continuing on this path I also see that you ask for the server name and then the full share path anyway (which contains the server path). Instead of asking for them separately you can cast the string as a URI to get just the host name without much hassle. Saves asking the user for something twice.

([uri]"\\eccofs01\Groups\ECCO IT\").Host

Split-Path would also help when taking a single sting and separating a path from its parent. Just something to keep in the back pocket.

split-path "\\eccofs01\Groups\ECCO IT\" -Parent
\\eccofs01\Groups

split-path "\\eccofs01\Groups\ECCO IT\" -Leaf
ECCO IT

-Parent can be omitted and is used by default.

Maybe a Little Regex Would Help

You are removing spaces using the string method `.Replace()` which is fine. Depending on your local other white-space characters are possible to run into. You can cover them with a simple `-replace '\s'` which will replace all white-space characters with nothing. 

$FileShareFull = $FileShare -replace '\s'

Avoid duplicate code

I read your previous post and we helped to remove some redundancy I would like to point out a couple other of areas to consider improvement.

$ADNameRW = "FS-$FileShareFull-$Name-RW".toupper()
$ADNameR = "FS-$FileShareFull-$Name-R".toupper()

You are making almost the same string accept that one has a W at the end. Why no make one a basic copy of the other. This way, in the unlikely event you change your naming convention you only really have to edit the one line. THINK OF THE SAVINGS!

$ADNameR = "FS-$FileShareFull-$Name-R".toupper()
$ADNameRW = $ADNameR + "W"

I love splatting an applaud your use of it. In the cases of $GroupParams1 and $GroupParams2 both share similar params. Why not more that shared group out and add it to the unique param groups.

$sharedParams = @{
    'GroupCategory' = "Security"
    'GroupScope' = "Global"
    'Path' = "OU=$Country,OU=FILE SHARE GROUPS,OU=Security Groups,DC=esg,DC=intl"
}

$GroupParams1 = $sharedParams + @{
    'Name' = $ADNameRW
    'SamAccountName' = $ADNameRW
    'DisplayName' = "$NAME Read-Write Access"
    'Description' = "Members of this group have read-write access to $Path."
}

$GroupParams2= $sharedParams + @{
    'Name' = $ADNameR
    'SamAccountName' = $ADNameR
    'DisplayName' = "$NAME Read-Write Access"
    'Description' = "Members of this group have read access to $Path"
}

There are others that are so close to being able to consolidate but making it work would hamper readability which ultimately is important.

Potentially unwanted output

The New-Item cmdlet would return the directory object that was created to the console as output. That can sometimes be seen as a nuisance. If that is the case an easy approach is to cast the whole command to [void] or pipe into Out-Null. Both have the same result.

[void](New-Item -Path $Path -ItemType Directory)
New-Item -Path $Path -ItemType Directory | Out-Null

These suggestions don't all have to be implemented but would be good to at least consider. I do like the overall feel of the code and I have always been procrastinating about doing something very similar. Nice work.

\$\endgroup\$
3
  • \$\begingroup\$ Holy cow! Thank you so much! I have learned WAY more than I expected in the last couple of days thanks to this forum. I appreciate all of this!!! \$\endgroup\$ Apr 13 '16 at 16:51
  • \$\begingroup\$ I've spent some time and cleaned up the script a bit. I'll post the updated script probably tomorrow (after a bit more cleaning). Thanks again; I have been looking for something like this for way too long and I finally got frustrated with the lack of structure and the things that have been missing when my team has been building out shares so I decided to spend the time to make this happen. haha \$\endgroup\$ Apr 13 '16 at 23:04
  • \$\begingroup\$ I uploaded my new script (a couple of times... haha). I made a bunch of changes as you recommended and also added a few extra checks and format changes as well as making sure that the path was capitalized when adding the description to the AD Groups. \$\endgroup\$ Apr 14 '16 at 0:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.