search

dsccommunity / ActiveDirectoryDsc

This module contains DSC resources for deployment and configuration of Active Directory Domain Services.
MIT License
345 stars 142 forks source link

ADFineGrainedPasswordPolicy: New resource #585

Closed fl0p3k closed 4 years ago

fl0p3k commented 4 years ago

Pull Request (PR) description

This PR adds the ADFineGrainedPasswordPolicy resource to the module.

This Pull Request (PR) fixes the following issues

Task list

This change is Reviewable

johlju commented 4 years ago

@fl0p3k can you please look at the failing tests?

johlju commented 4 years ago

Please see @X-Guardian's https://github.com/dsccommunity/ActiveDirectoryDsc/issues/584#issuecomment-610797852

fl0p3k commented 4 years ago

I am going to push the unit tests today once completed coding for it

X-Guardian commented 4 years ago

Ian, please can you complete the new resource proposal issue as requested before continuing with this PR, so that we can discuss the properties and implementation of this resource.

fl0p3k commented 4 years ago

Ian, please can you complete the new resource proposal issue as requested before continuing with this PR, so that we can discuss the properties and implementation of this resource.

I have just updated the proposal #584

fl0p3k commented 4 years ago

@fl0p3k can you please look at the failing tests?

I corrected the failing tests

fl0p3k commented 4 years ago

I will be looking over the comments and will provide some updates.

fl0p3k commented 4 years ago

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…
Please resolve the comments in the issue, or discuss them if you have other thoughts.

I have made all changes from the review feedback. The only remaining pending action would be to understand the changes needed to support the PsDscRunAsCredential modifications. Right now all is working and not sure I need to change anything regarding that.

X-Guardian commented 4 years ago

Ok, as I said in the resource proposal issue, the ADDomainDefaultPasswordPolicy resource was written four years ago, and does not use a modern logic pattern. It also not a resource that uses an 'Ensure' property, so the logic flow through the functions is very different. A better example to follow is the ADManagedServiceAccount which has a modern logic flow of an 'Ensure' property resource both in the module and in its unit tests.

With regard to @johlju comments about the Credential parameter, I'm happy to leave this in, as it along with the DomainController parameter are needed to execute the resource on a non-domain joined machine, where the PsDscRunAsCredential parameter with domain credentials can't be used.

Key features that I would like the new resource to use:

If you are not sure about anything, please ask before coding!

stale[bot] commented 4 years ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

fl0p3k commented 4 years ago

@X-Guardian I've been away from this project for a bit, but I have implemented some changes based on your feedback. I am hoping that I am meeting the bar at this point to move forward. My only doubt was how to determine the code coverage the unit tests were evaluating. I did my best to ensure that all scenarios would get tested. Thanks.

codecov[bot] commented 4 years ago

Codecov Report

Merging #585 into master will increase coverage by 0%. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #585    +/-   ##
======================================
  Coverage      98%    98%            
======================================
  Files          24     25     +1     
  Lines        3110   3335   +225     
======================================
+ Hits         3049   3275   +226     
+ Misses         61     60     -1
X-Guardian commented 4 years ago

Hi @fl0p3k, If you go to the 'Checks' tab on this PR and then select the top check (dsccommunity.ActiveDirectoryDsc) then "View more details on Azure Pipelines" then select "Code Coverage", you will see the Code Coverage report for the module. If you then drill down into the MSFT_ADFineGrainedPasswordPolicy module you will get a line-by-line report of coverage.

Taking a quick look at your updated code, we are getting closer, but there are still some of the features that I listed that haven't been implemented:

fl0p3k commented 4 years ago
  • Get-TargetResource function only includes mandatory parameters and parameters explicitly required for the function (DomainController and Credential).
  • Get-TargetResource function only returns properties defined in the schema.

@X-Guardian Ok, the two items above I am not sure I am on the same page. I updated the Get-TargetResource function parameters that no longer use the DomainController or Credential parameters. Also, I updated the Schema to include the SubjectsDifferent read only boolean output.

X-Guardian commented 4 years ago

Hi @fl0p3k , Get-TargetResource needs the DomainController and Credential parameters to pass to the Get-ADFineGrainedPasswordPolicy cmdlet, but it doesn't need the Subjects, SubjectsToInclude and SubjectsToExclude parameters. There is also no need for the SubjectsDifferent read only property, which we didn't agree to in the resource proposal. Please stick to the proposal, or discuss first if you think you need to change it.

stale[bot] commented 4 years ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

fl0p3k commented 4 years ago

Hi @fl0p3k , Get-TargetResource needs the DomainController and Credential parameters to pass to the Get-ADFineGrainedPasswordPolicy cmdlet, but it doesn't need the Subjects, SubjectsToInclude and SubjectsToExclude parameters. There is also no need for the SubjectsDifferent read only property, which we didn't agree to in the resource proposal. Please stick to the proposal, or discuss first if you think you need to change it.

@X-Guardian, is there a forum we can chat in realtime (best time as well for you, I'm in PST)? I've been limited being able to focus on this but I have a few days ahead to address updates, but I would like to have a discussion about specific implements before making any more changes.

X-Guardian commented 4 years ago

Hi @fl0p3k , I'm available on the Slack PowerShell channel at http://slack.poshcode.org/

fl0p3k commented 4 years ago

Hmm, I think I need an invitation, I am getting a message that the link is no longer active.

X-Guardian commented 4 years ago

Try the link on the home page: https://poshcode.org/

johlju commented 4 years ago

@fl0p3k here is more information and links (let me know if they don't work ) https://dsccommunity.org/community/contact/

fl0p3k commented 4 years ago

I believe it is now ready with 100% code coverage and the changes to support the subjects. Please review at your convenience. Thanks!

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 64 at r13 (raw file):

Previously, X-Guardian (Simon Heather) wrote…
Please remove the `Precedence` parameter from $parameters here, then there is no need to remove it from both `$getADFineGrainedPasswordPolicyParams` and `$getADFineGrainedPasswordPolicySubjectParams` further down.

I get an error if I remove it since it is called out as mandatory in the schema. This is a required parameter for the 'New-ADFineGrainedPasswordPolicy' cmdlet and I must make it mandatory in the schema I think.

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 92 at r13 (raw file):

Previously, X-Guardian (Simon Heather) wrote…
I only get an `ADIdentityNotFoundException` when the password policy doesn't exist, not a lack of subjects. I don't think you need this specific catch as it would have been caught in the previous catch.

I added this incase something else goes wrong (like auth or server error from the parameters).

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 318 at r13 (raw file):

Previously, X-Guardian (Simon Heather) wrote…
What is this for?

I mentioned this in the chat, but when the subjects are null from the Get-TargetResource function, the Compare-ResoucePropertyState does not detect anything mismatched.

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 318 at r13 (raw file):

Previously, fl0p3k (Ian Florek) wrote…
I have always returned an empty array when the subjects are null. I made some specific changes to ensure only an empty array object is returned, but I get this: VERBOSE: [IFLABDC01]: [[ADFineGrainedPasswordPolicy]UserAccounts] Evaluating the state of the property 'Subjects'. (ADCOMMON0003) Cannot bind argument to parameter 'ReferenceObject' because it is null. + CategoryInfo : InvalidData: (:) [], CimException + FullyQualifiedErrorId : ParameterArgumentValidationErrorNullNotAllowed,Microsoft.PowerShell.Commands.CompareObjectCommand + PSComputerName : IFLABDC01 VERBOSE: [IFLABDC01]: [[ADFineGrainedPasswordPolicy]UserAccounts] The parameter 'Subjects' is in desired state. (ADCOMMON0004)

I figured this one out by using [string[]] instead of object array.

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 318 at r13 (raw file):

Previously, fl0p3k (Ian Florek) wrote…
I figured this one out by using [string[]] instead of object array.

Done.

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 92 at r13 (raw file):

Previously, X-Guardian (Simon Heather) wrote…
Yes that would be better

Done.

fl0p3k commented 4 years ago

source/DSCResources/MSFT_ADFineGrainedPasswordPolicy/MSFT_ADFineGrainedPasswordPolicy.psm1, line 92 at r13 (raw file):

Previously, fl0p3k (Ian Florek) wrote…
Done.

Any update on closing this out and merging?

johlju commented 4 years ago

@x-guardian have probably been busy with the day job, I think he come around to this as soon as possible.

X-Guardian commented 4 years ago

Hi @johlju, I've been talking to @fl0p3k about this PR on Slack.

X-Guardian commented 4 years ago

I just noticed your Unit and Integration tests are in a directory Tests with a capital 'T', where they should be in a directory tests with a lower case 't'. Windows will merge this into one, so you won't see it on your local PC, but you can see they are different if you look here. https://github.com/fl0p3k/ActiveDirectoryDsc/tree/newADFineGrainedPasswordPolicy. To fix them to lower case, I think you need to delete the files, commit the change, then create them again in the test directory with a lowercase 't' then commit them again.

johlju commented 4 years ago

It is possible to fix by using git mv. Use git ls-files to find the relative paths that should be changed then use for example:

git mv Tests/Unit/test.ps1 tests/Unit/test2.ps1
git mv tests/Unit/test2.ps1 tests/Unit/test.ps1

Need to rename the file to a temporary filename then back. After all files have been “moved” then rename the local folder in Windows file system.

X-Guardian commented 4 years ago

Well done @fl0p3k, we finally got there. Thanks for your patience and perseverance! We have ended up with a good quality resource.

johlju commented 4 years ago

@fl0p3k really impressive work on this one! Thank you! 😃

fl0p3k commented 4 years ago

Thanks for the review feedback and patience. We made a lot of changes and learned a lot. I am grateful to have been a part of the process to get this done.