3
\$\begingroup\$

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
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ First welcome to CodeReview. I have a couple of questions here. Are the permissions on each folder not inherited? It might be faster to ignore none inherited permissions on such a large structure. What about the users that need full control like domain admins and such? \$\endgroup\$
    – Matt
    Jan 24, 2016 at 2:10
  • \$\begingroup\$ That is a bit random, some folders have the permissions inherited and other no... that's why I make the list of folders first of all, to make sure the script runs over all the CIFS. About the users that need full control like domain admins and such? That's a nice question... I'm thinking about make something like: 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\$ Feb 8, 2016 at 16:50

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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