New resource FileSystemAccessRule #1

johlju · 2020-02-22T14:44:34Z

FileSystemDsc
- Added resource FileSystemAccessRule

This PR moves the resource xFileSystemAccessRule from the module xSystemSecurity to this module. This PR will make it possible to deprecate the module xSystemSecurity.

This change is

johlju · 2020-02-22T14:45:38Z

@PlagueHO I trade you reviews. 😉 This is a big one so no hurry.

gaelcolas

Did a quick review and spotted some quick changes.
Maybe worth for Daniel -eagle eye- Scott-Raynsford to double check though, as I went fast ;)

Reviewed 29 of 29 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johlju and @PlagueHO)

CHANGELOG.md, line 13 at r1 (raw file):

  - Added resource FileSystemAccessRule

### Changed

You probably want to clean up those not in use here?

RequiredModules.psd1, line 22 at r1 (raw file):

    xDscResourceDesigner        = 'latest'
    'DscResource.DocGenerator'  = 'latest'
    'DscResource.Common'        = '0.2.0'

Are you sure you want to pin DscResource.Common?
I'd prefer leaving to latest, and the day a new version of DscResource.Common breaks a build, pin it to previous working version until it's fixed.

tests/Integration/DSC_FileSystemAccessRule.Integration.Tests.ps1, line 11 at r1 (raw file):

catch [System.IO.FileNotFoundException]
{
    throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.'

should this be -ResolveDependency as well, or you just rely on the autoResolve?

tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 12 at r1 (raw file):

    catch [System.IO.FileNotFoundException]
    {
        throw 'DscResource.Test module dependency not found. Please run ".\build.ps1 -Tasks build" first.'

same

johlju

Reviewable status: 27 of 29 files reviewed, 4 unresolved discussions (waiting on @gaelcolas and @PlagueHO)

CHANGELOG.md, line 13 at r1 (raw file):

Previously, gaelcolas (Gael) wrote…

You probably want to clean up those not in use here?

Done.

RequiredModules.psd1, line 22 at r1 (raw file):

Previously, gaelcolas (Gael) wrote…

Are you sure you want to pin DscResource.Common?
I'd prefer leaving to latest, and the day a new version of DscResource.Common breaks a build, pin it to previous working version until it's fixed.

Done.

tests/Integration/DSC_FileSystemAccessRule.Integration.Tests.ps1, line 11 at r1 (raw file):

Previously, gaelcolas (Gael) wrote…

should this be -ResolveDependency as well, or you just rely on the autoResolve?

I think something else (help doc) should mention the prereq for running build? 🤔

tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 12 at r1 (raw file):

Previously, gaelcolas (Gael) wrote…

same

I think something else (help doc) should mention the prereq for running build? 🤔

PlagueHO

Reviewed 5 of 29 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @gaelcolas and @johlju)

azure-pipelines.yml, line 49 at r2 (raw file):

              publishLocation: 'Container'

  - stage: Test

Can you also run unit/integration tests in WS2016 as well?

azure-pipelines.yml, line 117 at r2 (raw file):

        displayName: 'Integration'
        pool:
          vmImage: 'win1803'

Can you use windows-2019 instead?

tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 2 at r2 (raw file):

$configFile = [System.IO.Path]::ChangeExtension($MyInvocation.MyCommand.Path, 'json')
if (Test-Path -Path $configFile)

Add blank line before?

tests/Integration/DSC_FileSystemAccessRule.config.ps1, line 181 at r2 (raw file):

    .SYNOPSIS
        Removes only the specified access rules for the identity when the
        identity in the current state have more rights than desired state.

have -> has

PlagueHO

Reviewed 1 of 29 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gaelcolas and @johlju)

build.ps1, line 11 at r2 (raw file):

(
    [Parameter(Position = 0)]
    [string[]]$Tasks = '.',

Nit pick:
$Tasks on the next line...

PlagueHO

Reviewed 22 of 29 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @gaelcolas and @johlju)

source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.psm1, line 509 at r2 (raw file):

                    # If any rights that we want to deny are individually a full subset of existing rights.
                    $currentRightResult = -not ($rightNotAllowed -eq ($rightNotAllowed -band ([System.Security.AccessControl.FileSystemRights] $currentState.Rights)))

Is there any way to reduce the length of this line?

source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 4 at r2 (raw file):

class DSC_FileSystemAccessRule : OMI_BaseResource
{
    [Key, Description("The path to the item that should have permissions set")] string Path;

Nitpick- end description with full stop.

source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.schema.mof, line 5 at r2 (raw file):

{
    [Key, Description("The path to the item that should have permissions set")] string Path;
    [Key, Description("The identity to set permissions for")] string Identity;

Nitpick- end description with full stop.

source/DSCResources/DSC_FileSystemAccessRule/en-US/DSC_FileSystemAccessRule.strings.psd1, line 12 at r2 (raw file):

    NotPossibleClusterResourceOwner = The node '{0}' is not a possible owner for the path '{1}'. (FSAR0007)
    NoClusterDiskPartitionFound = No cluster disk partition was found that the path '{0}' could belong to. (FSAR0008)
    NodeIsNotClusterMember = 'Node does not belong to a Windows Server Failover Cluster. (FSAR0009)

There is a ' at the beginning that shouldn't be there.

source/Examples/README.md, line 4 at r2 (raw file):

This will help to understand how to setup certain scenarios with the
FIleSystemDsx resource module.

FIleSystemDsx->FileSystemDsc

Also, capitalization problems.

tests/Unit/DSC_FileSystemAccessRule.Tests.ps1, line 251 at r2 (raw file):

                Context 'When the configuration is present' {
                    Context 'When the path exist' {

exists

johlju

Thank you for these comments! Great catches! 😃

Reviewable status: 21 of 29 files reviewed, 14 unresolved discussions (waiting on @gaelcolas and @PlagueHO)

azure-pipelines.yml, line 49 at r2 (raw file):