I need to delete all the orphaned SIDs in the ACLs of about 20 shares (between 100GB/6TB) and change full control of users groups to other permissions (modify or read/execute). I have done this script but I'm pretty sure it's easy to improve. Any advice?
$root = "\192円.168.1.1\folder"
$log = "C:\log.txt"
#log creation
"----------SIDS----------" | Out-File $log
#folders list
Get-ChildItem $root -Recurse -Directory | ForEach-Object -Begin {
$folders = @((Get-Item $root).FullName)
} -Process {
$folders += $_.FullName
}
##############################DELETE ORPHANED SIDS########################
foreach ($folder in $folders) {
try {
$acl = Get-Acl -Path $folder
foreach ($acc in $acl.access ) {
$acc_ir = $acc.IdentityReference.Value
if ($acc_ir -match "S-1-5*") {
$ACL.RemoveAccessRule($acc) | Out-Null
Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
$hour = Get-Date
Write-Host -ForegroundColor Green "$hour - $acc_ir deleted in $folder"
Write-Output "$hour - $acc_ir deleted in $folder" | Out-File -Append $log
}
}
} catch {
$hour_e = Get-Date
Write-Host -ForegroundColor Red "$hour_e - error with $acc_ir in $folder"
Write-Output "$hour_e - error with $acc_ir in $folder" | Out-File -Append $log
}
}
##############################CHANGE FULLCONTROL########################
"----------FULLCONTROL----------" | Out-File -Append $log
foreach ($folder in $folders) {
try {
$acl = Get-Acl -Path $folder
foreach ($acc in $acl.access ) {
$acc_ir = $acc.IdentityReference
$acc_fsr = $acc.FileSystemRights
if ($acc_ir -match "GROUP" -and $acc_fsr -match "FullControl") {
$newacc = New-Object System.Security.AccessControl.FileSystemAccessRule($acc_ir, 'Modify', $acc.InheritanceFlags, $acc.PropagationFlags, $acc.AccessControlType)
$ACL.RemoveAccessRule($acc) | Out-Null
$ACL.AddAccessRule($newacc) | Out-Null
Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
$new_perm = $newacc.FileSystemRights
$hour = Get-Date
Write-Host -ForegroundColor Green "$hour - $folder : the object $acc_ir now have $new_perm"
Write-Output "$hour - $folder : the object $acc_ir now have $new_perm" | Out-File -Append $log
}
}
} catch {
$hour_e = Get-Date
Write-Host -ForegroundColor Red "$hour_e - error with the object $acc_ir in $folder"
Write-Output "$hour_e - error with the object $acc_ir in $folder" | Out-File -Append $log
}
}
1 Answer 1
Should be able to add a little more if you answers my questions in a comment I made on your question. In the mean time there definitely are some points worth bringing to light. I am going to assume you have at least PowerShell v3.0.
Over complicated folder name query
Property Expansion done right
You don't need to build an array for the fullname. Nor do you really need to get the fullname via a directory object for the $root as you already have it. If dot notation does not work Select-Object Fullname would work just as well.
$folders = @($root) + (Get-ChildItem $root -Recurse -Directory).FullName
Since $root is not an array we make it one, just as you did, with @() then simply concatenate the rest of the folder names to make the list of folders.
Avoid array concatenation
This is not a bid deal if you read into my first couple of points but I still want to cover this. It is a memory intensive waste as it destroys and recreates the array adding the new element. Use the pipeline if possible to avoid this.
Regular Expressions
I wanted to draw attention to -match. That operator and others like -replace support regular expressions as well.
In regex "S-1-5*" would match anything that had those characters anywhere in the string and not just the beginning. More importantly it would match strings like "S-1-5555" since 5* matches 0 or as many 5's as it can find. If you wanted to keep using match I would recommend the start of line anchor. In the spirit of what you were trying to do "^S-1-5" would accomplish the same thing. Look for a string that starts with "S-1-5". However the easier solution is just to use -like which is the operator you were, in essence, using
if ($acc_ir -like "S-1-5*")
Double processing
While you have two actions to perform (remove dead sids and change permissions) all the work is still being done on the same folder set. I would not bother looping twice. You could just loop the once and merge your if statements inside the single loop.
foreach ($folder in $folders) {
try {
$acl = Get-Acl -Path $folder
foreach ($acc in $acl.access ) {
$acc_ir = $acc.IdentityReference
$acc_fsr = $acc.FileSystemRights
if ($acc_ir -match "GROUP" -and $acc_fsr -match "FullControl") {
# Logic for when the group is matched and has fullcontrol
} elseif ($acc_ir -match "S-1-5*") {
# Logic for when their is a dead sid.
}
}
} catch {
# Errors captured here.
}
}
This would have implications in your log as the entries would no longer be grouped by action but I will cover that later.
Perhaps you prefer the separation
If you didn't want to go the route of the single loop (which you should for performance) I would suggest you use Where-Object in place of your if's as that is in effect what you are doing. Consider you first loop group where you remove the dead sids.
$acl.access | Where-Object{$_.IdentityReference.Value -match "^S-1-5"} | ForEach-Object{
$acc_ir = $_.IdentityReference.Value
$acl.RemoveAccessRule($_) | Out-Null
Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
$hour = Get-Date
Write-Host -ForegroundColor Green "$hour - $acc_ir deleted in $folder"
Write-Output "$hour - $acc_ir deleted in $folder" | Out-File -Append $log
}
Logging
About Out-File
It is considered one of the weaker means to output text to a file. Add-Content is a good upgrade as it is just as easy to use and performs better. It also is a cmdlet that appends by default and adds a newline to the end of the string automatically. You could also use Set-Content for the first line if you intend to clean the log every time the script is run.
Functions
You should be using at least one function here. You are repeating your blocks of code several times throughout your script. A function would make it easier to read the code and more importantly simpler to make changes to your logging.
function Log-Action($reference,$location,[switch]$LogError){
$now = (get-Date).ToString()
If($LogError.IsPresent){
Write-Host -ForegroundColor Red "$now - error with $reference in $location"
Write-Output "$now - error with $reference in $location" | Add-Content $log
} else {
Write-Host -ForegroundColor Green "$now - $reference deleted in $location"
Write-Output "$now - $reference deleted in $location" | Add-Content $log
}
}
That would work but some would say that it has a terrible name as it performs two duties.... Logging both errors and information. Also removes the need to save the current time in different variables.
Either way this would be how you would call it in the script...
try {
$acl = Get-Acl -Path $folder
foreach ($acc in $acl.access ) {
$acc_ir = $acc.IdentityReference.Value
if ($acc_ir -match "S-1-5*") {
$ACL.RemoveAccessRule($acc) | Out-Null
Set-Acl -Path $folder -AclObject $acl -ErrorAction Stop
Log-Action $acc_ir $folder
}
}
} catch {
Log-Action $acc_ir $folder -LogError
}
Objects and CSV output
Something else to consider would be to try and export CSV data so that it would not matter what order your output was as your could organize it as you saw fit with a simple column sort in Excel or similar program.
This one might be harder to show without major changes but here is how it could go.
# Need to use this for each contruct in order to use the pipeline.
$folders | ForEach-Object{
$folder = $_
# Build the row
$result = [pscustomobject][ordered]@{
Time = (Get-Date).ToString()
Folder = $folder
IdentityReference = $null
Action = "Remove dead SID"
ActionSuccedded = $false # Assume False
}
# reset in case of early failure where this might not get populated
$acc_ir = $null
try {
$acl = Get-Acl -Path $folder
$acl.access | ForEach-Object{
$acc = $_
$acc_ir = $acc.IdentityReference.Value
if ($acc_ir -match "S-1-5*") {
# Stuff Happens
# Log the changes
$result.Time = (Get-Date).ToString()
$result.IdentityReference = $acc_ir
$result.ActionSuccedded = $true
# send the result through the pipeline
$result
}
}
} catch {
# Error has occured.
$result.Time = (Get-Date).ToString()
$resulr.IdentityReference = $acc_ir
# send the result through the pipeline
$result
}
} | Export-CSV -NoTypeInformation $log
This way you have highly functional output since we populated a result object with the status of each action and get output that looks something like this
Time Folder IdentityReference Action ActionSuccedded
---- ------ ----------------- ------ ---------------
1/23/2016 11:28:17 PM C:\temp S-1-5-21-1220945662-1202665555-839525555-5555 Remove dead SID True
1/23/2016 11:28:17 PM C:\temp\test S-1-5-21-1220945662-1204465555-839525555-5555 Remove dead SID False
Most of my suggestions don't build together but I wanted to cover each one separately in an effort to maintain focus. You will have to forgive me if I lost you on any part of this.
if($acc_ir -notmatch "ADMIN_GROUP_1" -and $acc_ir -notmatch "ADMIN_GROUP_2" -and $acc_ir -notmatch "ADMIN_GROUP_N" -and $acc_fsr -match "FullControl")\$\endgroup\$