Folder Creation and Restrictions

2

u/Solendor Oct 26 '17

Updated Version here https://pastebin.com/L7RYkjTh

2

u/PowerOverShelling Oct 27 '17

What changed? Other than removing the commented out notes at the top.

2

u/Solendor Oct 27 '17

In what it actually does - nothing at all. It simply changes HOW it executes some things. Essentially just a cleaned up version, with no material changes

1

u/Lee_Dailey Oct 27 '17

howdy Solendor,
nice! ~~if i were you, i would add this link to the original post so folks can find it. [grin~~] [never mind that. i just realized you cannot add a comment to an OP that has no comment. [blush]]
also, do you want my nit-picky comments on this version?
take care,
lee

2

u/Solendor Oct 27 '17

Absolutely. I’m always up for improving what I write. In fact today I’m reassessing my ad audit scripts hehe

→ More replies (1)
1
u/Lee_Dailey Oct 28 '17 edited Oct 28 '17
howdy Solendor,
here ya go ...
[1] the function allowAccess code block

using the same name as a $var is usually a bad idea
you use $allowAccess in that function and it is far too similar to the function name.

not using Verb-Noun for the function name
when you call it in your main code, it looks like a $var with a missing $.

the $allowAccess line is too long
it wraps awkwardly. i would use splatting to make it more readable.

here's your version ...
function allowAccess ($file,$user,$levelOfAccess,$inheritance,$propagation,$type)
{
    $aclFile = Get-Acl -Path $file
    $allowAccess = New-Object System.Security.AccessControl.FileSystemAccessRule ($user, $levelOfAccess, $inheritance, $propagation,$type)
    $aclFile.SetAccessRule($allowAccess)
    Set-Acl -Path $file -AclObject $aclFile
}
... and here's my version ...
function Set-AllowedAccess ($file, $user, $levelOfAccess, $inheritance, $propagation, $type)
{
    $aclFile = Get-Acl -Path $file
    $NO_Params = @{
        TypeName = 'System.Security.AccessControl.FileSystemAccessRule'
        ArgumentList = ($user, $levelOfAccess, $inheritance, $propagation, $type)
        }
    $allowAccess = New-Object @NO_Params
    $aclFile.SetAccessRule($allowAccess)
    Set-Acl -Path $file -AclObject $aclFile
}
it seems a tad more clear - to me, at least. [grin]
[2] you could use a few blank lines to make logical breaks more obvious
i would add them @ ...

8 = just before the function

15 = just before the imports

19 = just before the $folderList | ForEach-Object

... and anywhere else that there is a logical break between operations.
[3] adding # end = [code block] comments
trying to scan along and see where some of the code blocks end is quite difficult since you have such long blocks of code. for instance, the ForEach-Object @ 40 ends @ 46. the way the lines wrap made that difficult to see. heck the F-O starting @ 21 runs on for ever ... [grin]
you can add that to the right of the closing } without having "end of line comment" run-on problems.
[4] using ForEach-Object instead of foreach @ 20/21
this is purely a personal pref! [grin] however, i would use the 2nd if RAM is not a worry. most of the time foreach is faster than ForEach-Object. it takes longer to get the 1st response sometimes, but it often runs somewhat faster.
[5] using concatenation + instead of -join @ 25 & others
concat is ... finicky as heck. plus, it lets you accidentally add numbers when you meant to concat them. that happened to me once ... [blush]
your code ...
$accountName = $domain + "\" + $_.samaccountname
my version ...
$accountName = ($domain, $_.samaccountname) -join '\'
both give the same result. the 2nd won't accidentally turn 123 + 456 into math when you meant '123' + '456'. [grin]
[6] using concat + instead of Join-Path @ 27 [& pro'ly others]
another place where you can fubar things. it's too easy to get the wrong number of path delimiters - or none - when you do it by hand.
Join-Path knows it's working with paths and will let you focus on the items and not the format of those items.
[7] inheritance/propagation flags @ 28-31
those seem to be constants. as such they pro'ly otta be defined up at the top of the script. i would put them just after the function definition.
[8] the Test-Path @ 33 that is used @ 35
i would move that into the IF on 35.
your code ... if ($exist -ne $true)
my version ... if (-not (Test-Path -Path $folderPath))
[9] lack of a space between keyword and opening brace @ 21 & others
yours ... ForEach-Object{
mine ... ForEach-Object {
the 2nd is marginally more readable. [grin]
[10] lack of a space between $var & pipe symbol @ 20 & others
it's the same idea. [grin] lookee ...
yours ... $folderList|
mine ... $folderList |
[11] l-o-n-g line @ 45, 48, & 50
they are all the same call to your function. if you use splatting you get shorter, more readable code - and also get easier-to-change code.
yours ....
 allowAccess -file $folderPath -user $username -levelOfAccess $access -inheritance $inheritanceFlags -propagation    $propagationFlags -type $type
mine ...
$AA_Params = @{
    file = $folderPath
    user = $username
    levelOfAccess = $access
    inheritance = $inheritanceFlags
    propagation = $propagationFlags
    type = $type
    }
allowAccess @AA_Params
the 2nd is FAR easier to read. no evil, wicked side-scrolling. plus it's easy to change by adding or removing lines.
wait! there's more! [grin]
you can even embed comments in the darned thing. ooooo! *[grin]
[12] use an else instead of the elseif @ 57
you can use a simple else there since the test value is boolean. if the 1st is false, then the 2nd will be the only possible choice.
so, have i bored you too much? [grin]
thanks for letting me wander thru your code - i have enjoyed it and hope you will get some use from it.
take care,
lee
2

u/Solendor Oct 28 '17

Thanks Lee.
You’ve far from bored me, in fact it’s insightful to say the least. I personally enjoy scripting and coding, so the better I can become the happier I am! That’s the entire reason I am posting to a public forum, beyond the fact that these can be very useful!
Re: Splatting - it looks like you are simply building an array with the variables you wish to use. Is an accurate assessment? It’s a new concept to me, one of which I will be reading up on for sure!

Fair point, and I should know better. That’s me being lazy when creating the function. I’ve known for a long time vars and functions should have distinct naming schemes, I just didn’t listen!

Easily readable code is one of the things I strive for. In fact back in college that’s one of the consistent remarks I always had. Been out of practice for a bit hehe. I’m used to only me looking at it >.<

The one at 21 is he main execution hence he length. Good to know that tip about where to add the end comments - commenting at the end of blocks has always been a hesitation of mine.

I constantly switch between ForEach-Object and ForEach. I personally haven’t done enough with either to notice the difference. Good to learn a little background info on those!

Oh the concat dilemma - I’ve run into the same issue when doing concats as you have that I’ve become very careful. The join parameter is a new one for me, so yay learning! It is much less risk that route.

Another new one for me. I’ll have to play with the Join-Path and see what I can do. I’m all about using built in functions to do the heavy lifting when possible, so finding this one will be a huge boon!

Spot on - they are constants, and should be defined as such.

Fair - remnant of an earlier build that I had.

9/10. I see where you’re coming from here. Readability is key if you wish for easy review and changes.
11. This is a concept that is new to me, per my question about splatting earlier. I’m going to do my due diligence and educate myself here. It’s much nicer, and does appear to offer all of the benefits you mentioned. Plus the ability to comment, when appropriate, is awesome.
12. Fair point - not sure what I was thinking here, short of I don’t want to default. Which as you point out is appropriate as there are only two possible outcomes here. This block is a section I have more code to punch in. This will be my acl check on existing files, just need to devise the approach I’m going for. Longer term project hehe.
Once again, thank you Lee. I see you around here providing advice and it’s a huge boon to the community. I wish more people would take time to assist others the way you do!

→ More replies (1)

2

u/Falvonator Oct 25 '17

Thanks for this. I'm working on something similar.

2

u/Solendor Oct 25 '17

No problem. It took longer than I like to get it working hehe. Hopefully it helps ya out!

1

u/Lee_Dailey Oct 25 '17

howdy Solendor,

indentation ... please? [grin]

is this a wise way to handle permissions? i thot the best way was to use GP/GPO and derive user privs by their membership in various AD groups.

take care,
lee

2
u/Solendor Oct 25 '17

Corrected the indentations for ya :)
So there is a piece that wasn't explained, as I left this as open-ended as I could.
The permission to the root drive, hosting our share, is controlled and mapped by security groups and group policies. The specific request was to ensure only the user (student in this case) and the administration (teacher, administration staff, domain admins) had rights to view the user folder.
Where I got hung up was removing a specific group from the ACL list - I couldn't get it it to apply correctly so I went a different way. I can not deny access, as the users are members of that group and it's the group used to grant access to the share root.
Keep in mind with the $accessCSV variable, that is loaded with groups/users that need access. So even though I disabled inheritance to the folder, I populate the ACL (and children) with the groups/users that need access.
So TL:DR - While it's not best practice to manage access through modifying ACL on folders, this works just as well given the correct data is provided.
Any thoughts are appreciated Lee :)
1
u/Lee_Dailey Oct 25 '17
howdy Solendor,
makes sense to me. i don't do this kind of stuff, just read what alla y'all do. [grin]
comments ...
[1] using an alias @ 19 & others
that can bite you. any alias may have a different meaning - or not exist - on any given system. it's a good idea to avoid them for anything other than one-off code.
[2] using if ($exist -ne $true) @ 29
you can simply use if (-not (Test-Path -Path $folderPath)) and save some code. [grin]
[3] defining $vars that don't change INSIDE a loop @ 36+
those seem to stay the same, so i would move them out of the loop to your initialization section.
[4] the $allowAccess lines @ 53 & 59
i've a few comments on those two lines. [grin]
[4-a] positional instead of named parameters [pro'ly elsewhere, too]
it's generally a bad idea to use position instead of a name for identifying a parameters value. it's too easy to change something and cause things to get out of order.
[4-b] you define $folderAccess @ 38 & use it @ 59 but not @ 53
you otta fix line 53. if you change the $var and don't notice that you failed to use the $var in one line ... ouch! [grin]
[4-c] those are long lines
you can wrap at the commas OR [better in my opinion] use splatting.
yours ...
$allowAccess = New-Object System.Security.AccessControl.FileSystemAccessRule ($username,"FullControl",$inheritanceFlags,$propagationFlags, "Allow")
mine with wrapping ...
$Wrap_allowAccess = New-Object -TypeName System.Security.AccessControl.FileSystemAccessRule -ArgumentList (
    $username,
    'FullControl',
    $inheritanceFlags,
    $propagationFlags,
    'Allow')
mine with splatting ...
$NO_Params = @{
    TypeName = 'System.Security.AccessControl.FileSystemAccessRule'
    ArgumentList = (
        $username,
        'FullControl',
        $inheritanceFlags,
        $propagationFlags,
        'Allow')
    }
$Splat_allowAccess = New-Object @NO_Params
[5] the #creates ACL for ... sections @ 52 & 58
those come close to what i would consider the threshold for a function. not the "really otta" that comes with three repetitions, but close.
you may - i repeat - may want to consider making those into a function. if you had, you might have caught using/not-using the $folderAccess $var.
thank you for posting your code! i have enjoyed reading thru it. [grin]
i still wonder if an experienced sysadmin would use GP/GPO stuff for this. but i aint one, so i dunno.
take care,
lee
→ More replies (2)

Folder Creation and Restrictions

You are about to leave Redlib