xFileSystemAccessRule: Make Cluster Aware

[randomnote1]

Pull Request (PR) description
Made the resource cluster aware. When ProcessOnlyOnActiveNode is specified, the resource will only determine if a change is needed if the target node is the active host of the file system object.

This Pull Request (PR) fixes the following issues:
Fixes #16

---

This change is 

[johlju]

@randomnote1 Can you work on this if I review?

[randomnote1]

@johlju Yes, I am ready to go. Thanks for reviewing!

[johlju]

@randomnote1 Great! I come back to this tomorrow. I have the e-mail notification as reminder.

[johlju]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

README.md, line 37 at r1 (raw file):

can be empty if ensure = absent

Maybe ..rule. Optional if Ensure is set to the value 'Absent'. Default value is an empty string array.

---

README.md, line 38 at r1 (raw file):

* **`[String]` Identity** _(Key)_: The identity to set permissions for
* **`[String[]]` Rights** _(Write)_: The permissions to include in this rule, can be empty if ensure = absent. Should be a combination of the following:
  * "ListDirectory"

Could we add these like this instead?

{ ListDirectory | ReadData | WriteData | ...etc... }`

---

README.md, line 61 at r1 (raw file):

  * "Synchronize"
  * "FullControl"
* **`[String]` Ensure** _(Write)_: Present to create the rule, Absent to remove an existing rule

Can we add this? ...existing rule. Default value is 'Present'. { *Present* | Absent }

---

README.md, line 62 at r1 (raw file):

  * "FullControl"
* **`[String]` Ensure** _(Write)_: Present to create the rule, Absent to remove an existing rule
* **`[Boolean]` ProcessOnlyOnActiveNode** _(Write)_: Specifies that the resource will only determine if a change is needed if the target node is the active host of the filesystem object.

Should we add that to use this the user the configuration is run as must have permission in the cluster?

---

README.md, line 63 at r1 (raw file):

* **`[String]` Ensure** _(Write)_: Present to create the rule, Absent to remove an existing rule
* **`[Boolean]` ProcessOnlyOnActiveNode** _(Write)_: Specifies that the resource will only determine if a change is needed if the target node is the active host of the filesystem object.
* **`[Boolean]` IsActiveNode** _(Read)_: Determines if the current node is actively hosting the filesystem object.

Maybe add something like this (please proof read): "This will always return $true if ProcessOnlyOnActiveNode is left out or set to the value $false."

---

README.md, line 69 at r1 (raw file):

## Versions

### Unreleased

Please add an entry or entries for the changes to the Unreleased section.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 38 at r1 (raw file):

        # Is the node a member of a WSFC?
        $msCluster = Get-CimInstance -Namespace root/MSCluster -ClassName MSCluster_Cluster -ErrorAction SilentlyContinue

If the resource is run with a user that does not have permission to evaluate the cluster, this would just continue without throwing an error. Can we catch that, or will the note in the README.md be sufficient? If we should catch that, then maybe we should only do the cluster parts if ProcessOnlyOnActiveNode -eq $true?

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 82 at r1 (raw file):

        $accessRules = $acl.Access

        $result.Rights = @(

This will return an object array of type FileSystemRights. Not a string array?

PS > $rights.GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     Object[]                                 System.Array


PS > $rights[0].GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     FileSystemRights                         System.Enum

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 91 at r1 (raw file):

}

<#

Update the comment-based help with the added/changed descriptions in the README.md.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 151 at r1 (raw file):

        )]
        [String[]]
        $Rights = @(),

The default value here, does that mean that when used with 'Present' that will replace all existing permissions with no permissions for that identity, or will it not do anything?

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 200 at r1 (raw file):

-Verbose

We should not have -Verbose here?

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 319 at r1 (raw file):

    if ( $comparisonResult.Count -gt 0 )
    {
        Write-Verbose -Message ( 'The identity "{0}" has the rights "{1}".' -f $Identity,( $currentValues.Rights -join ', ' ) ) -Verbose

Maybe write out what they should be too, the expected rights?

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.schema.mof, line 7 at r1 (raw file):

    [Key, Description("The identity to set permissions for")] string Identity;
    [Write, Description("The permissions to include in this rule, can be empty if ensure = absent"), ValueMap{"ListDirectory","ReadData","WriteData","CreateFiles","CreateDirectories","AppendData","ReadExtendedAttributes","WriteExtendedAttributes","Traverse","ExecuteFile","DeleteSubdirectoriesAndFiles","ReadAttributes","WriteAttributes","Write","Delete","ReadPermissions","Read","ReadAndExecute","Modify","ChangePermissions","TakeOwnership","Synchronize","FullControl"}, Values{"ListDirectory","ReadData","WriteData","CreateFiles","CreateDirectories","AppendData","ReadExtendedAttributes","WriteExtendedAttributes","Traverse","ExecuteFile","DeleteSubdirectoriesAndFiles","ReadAttributes","WriteAttributes","Write","Delete","ReadPermissions","Read","ReadAndExecute","Modify","ChangePermissions","TakeOwnership","Synchronize","FullControl"}] string Rights[];
    [Write, Description("Present to create the rule, Absent to remove an existing rule"), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] string Ensure;

Please match the changes in parameter descriptions to the descriptions here (those you have changed, or will change) 🙂

---

Tests/Unit/MSFT_xFileSystemAccessRule.tests.ps1, line 13 at r1 (raw file):

Import-Module -Name (Join-Path -Path $script:moduleRoot -ChildPath (Join-Path -Path 'DSCResource.Tests' -ChildPath 'TestHelper.psm1')) -Force

# TODO: Insert the correct <ModuleName> and <ResourceName> for your resource

Let's remove the TODO comment.

---

Comments from Reviewable

[johlju]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 42 at r1 (raw file):

-Verbose

We should not need this here.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 68 at r1 (raw file):

-Verbose

We should not need this here.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 173 at r1 (raw file):

-Verbose

We should not need this here.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 294 at r1 (raw file):

-Verbose

We should not need this here.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 319 at r1 (raw file):

-Verbose

We should not need this here.

---

Comments from Reviewable

[randomnote1]

Review status: 3 of 4 files reviewed at latest revision, 19 unresolved discussions.

---

README.md, line 37 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

can be empty if ensure = absent

Maybe ..rule. Optional if Ensure is set to the value 'Absent'. Default value is an empty string array.

Done.

---

README.md, line 38 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add these like this instead?

{ ListDirectory | ReadData | WriteData | ...etc... }`

Done.

---

README.md, line 61 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we add this? ...existing rule. Default value is 'Present'. { *Present* | Absent }

Done.

---

README.md, line 62 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should we add that to use this the user the configuration is run as must have permission in the cluster?

Done.

---

README.md, line 63 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe add something like this (please proof read): "This will always return $true if ProcessOnlyOnActiveNode is left out or set to the value $false."

Done.

---

README.md, line 69 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please add an entry or entries for the changes to the Unreleased section.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 38 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If the resource is run with a user that does not have permission to evaluate the cluster, this would just continue without throwing an error. Can we catch that, or will the note in the README.md be sufficient? If we should catch that, then maybe we should only do the cluster parts if ProcessOnlyOnActiveNode -eq $true?

I can think of a couple options here:

1. Trap the permissions failure message and silently continue if the cluster doesn't exist
2. Leave it as-is because the failure will ultimately be caught in the throw "Unable to get ACL for '$Path' because it does not exist" line.

I'm leaning toward the second option because I don't think adding the additional error handling adds much value.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 42 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not need this here.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 68 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not need this here.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 82 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This will return an object array of type FileSystemRights. Not a string array?

PS > $rights.GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     Object[]                                 System.Array


PS > $rights[0].GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     FileSystemRights                         System.Enum

No, it returns a string array. I added a type to the variable to make readability easier.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 91 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Update the comment-based help with the added/changed descriptions in the README.md.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 151 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

The default value here, does that mean that when used with 'Present' that will replace all existing permissions with no permissions for that identity, or will it not do anything?

Yes, good point. Since $Rights is optional when $Ensure = 'Absent', I added a test to determine if the $Rights parameter was passed when $Ensure = 'Present'.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 173 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not need this here.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 200 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not have -Verbose here?

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 294 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not need this here.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 319 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

-Verbose

We should not need this here.

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 319 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe write out what they should be too, the expected rights?

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.schema.mof, line 7 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please match the changes in parameter descriptions to the descriptions here (those you have changed, or will change) 🙂

Done.

---

Tests/Unit/MSFT_xFileSystemAccessRule.tests.ps1, line 13 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Let's remove the TODO comment.

Done.

---

Comments from Reviewable

[randomnote1]

@johlju The first round of updates are complete.

[johlju]

Awesome work again! Just a few tiny comments left! 🙂

---

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

README.md, line 39 at r3 (raw file):

the user

The (upper 'T')

---

README.md, line 39 at r3 (raw file):

haver

have

---

README.md, line 49 at r3 (raw file):

[Dan Reist (@randomnote1)](https://github.com/randomnote1)

In other resource modules we switched to have the contributor name at the end of the change log entry instead, to make reading of the change log easier. Can we do that here to?

  * Fixed issue when cluster shared disk is not present on the server ([issue #16](https://github.com/PowerShell/xSystemSecurity/issues/16)). [Dan Reist (@randomnote1)](https://github.com/randomnote1)

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 38 at r1 (raw file):

Previously, randomnote1 (Dan Reist) wrote…

I can think of a couple options here:

1. Trap the permissions failure message and silently continue if the cluster doesn't exist
2. Leave it as-is because the failure will ultimately be caught in the throw "Unable to get ACL for '$Path' because it does not exist" line.

I'm leaning toward the second option because I don't think adding the additional error handling adds much value.

Let's go with as-is, and if there is a problem in the future that can be fixed at that time.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 82 at r1 (raw file):

[System.String[]]@(

We should add a space between the type and @(); [System.String[]] @()

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 29 at r3 (raw file):

[System.String[]]@()

We should add a space between the type and @(); [System.String[]] @()

---

Comments from Reviewable

[randomnote1]

Review status: 1 of 4 files reviewed at latest revision, 6 unresolved discussions.

---

README.md, line 39 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

the user

The (upper 'T')

Done.

---

README.md, line 39 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

haver

have

Done.

---

README.md, line 49 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Dan Reist (@randomnote1)](https://github.com/randomnote1)

In other resource modules we switched to have the contributor name at the end of the change log entry instead, to make reading of the change log easier. Can we do that here to?

  * Fixed issue when cluster shared disk is not present on the server ([issue #16](https://github.com/PowerShell/xSystemSecurity/issues/16)). [Dan Reist (@randomnote1)](https://github.com/randomnote1)

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 82 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[System.String[]]@(

We should add a space between the type and @(); [System.String[]] @()

Done.

---

DSCResources/MSFT_xFileSystemAccessRule/MSFT_xFileSystemAccessRule.psm1, line 29 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[System.String[]]@()

We should add a space between the type and @(); [System.String[]] @()

Done.

---

Comments from Reviewable

[johlju]

---

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[johlju]

@randomnote1 Awesome work here! Thanks a lot for this!