search

X-Guardian / AdfsDsc

DSC resources for deployment and configuration of Active Directory Federation Services
MIT License
9 stars 5 forks source link

AdfsFarm: Add parameters to set certificates based on their names #48

Closed Yvand closed 1 year ago

Yvand commented 3 years ago

Pull Request (PR) description

I use DSC to completely provision Active Directory, including the creation of AD CS and AD FS. The certificates used by AD FS are issued by AD CS. In this scenario, the thumbprint of those certificates are know only at run-time, so I cannot set the thumbprint at design-time. For this reason, I used the certificates names instead, and I updated AdfsFarm to support it

This Pull Request (PR) fixes the following issues

Task list

This change is Reviewable

X-Guardian commented 3 years ago

Hi @Yvand, thanks for raising the PR. Good to know that the module is being used. Can you take a look at the PR validation checks that are failing? This will also need the unit tests extending to cover the new code.

Yvand commented 3 years ago

@X-Guardian, sure it is, you did very good work ! I'm happy to validate all the checks but I wanted to validate that you would approve those changes before I spend time on this. Specifically, are you fine with the (necessary) removal of the required flag for the parameter CertificateThumbprint?

Yvand commented 3 years ago

@X-Guardian ok, this is more complicated than I thought. How can I find exactly what tests fail? I see it's in Tests\Unit\MSFT_AdfsFarm.tests.ps1 but it contains many tests, and the CI output is not easy to read.

X-Guardian commented 3 years ago

If you look at the HQRM / Run HQRM Test and Unit / Run Unit Test output in Azure Pipelines and search for [-] you will see where the tests are faiilng.

heinejeppesen commented 1 year ago

Hi,

I'm trying to do the same (automate AD, ADCS, ADFS) and have stranded in the same way, where providing a thumbprint simply isn't possible, as the certificate is installed in the step before installing ADFS.

So I'm pretty curious if there any update on this PR? ;-)

Yvand commented 1 year ago

I never really gave up to work on this PR to complete the tasks, but writing the Pester tests is not my cup of tea. With that said, I'm using the changes in this PR intensively it and it works like a charm.

heinejeppesen commented 1 year ago

I never really gave up to work on this PR to complete the tasks, but writing the Pester tests is not my cup of tea. With that said, I'm using the changes in this PR intensively it and it works like a charm.

I'm also using the changes you put into this PR now and they do work great, so thanks for that.

Now I just have to make something similar for AdfsApplicationPermission where it only takes an GUID for client identifier. Need to implement a lookup for name, just like this. ;-)

X-Guardian commented 1 year ago

Hi @Yvand, I have made some small changes to your PR branch to fix the HQRM and unit tests that were failing. If you want to now look at the comments I made previously, we can get this PR ready for merging.

Regarding the Unit test coverage, I can add a couple of tests to cover your new code if you are not comfortable doing that.

codecov[bot] commented 1 year ago

Codecov Report

Merging #48 (504787c) into master (fa1cc05) will increase coverage by 0%. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #48   +/-   ##
=====================================
  Coverage      98%    98%           
=====================================
  Files          16     16           
  Lines        2080   2133   +53     
=====================================
+ Hits         2044   2097   +53     
  Misses         36     36
Yvand commented 1 year ago

@X-Guardian thank you, I'm checking now

Yvand commented 1 year ago

@X-Guardian I tested it in 2 deployments, it works like a charm

X-Guardian commented 1 year ago

@Yvand, can you look at the review comments I made on April 11th 2021. You can click on the Reviewable button to see these annotated on the code.

Yvand commented 1 year ago

@X-Guardian now I see those comments, I'm not familiar with Reviewable and completely missed them. I'll clone the PR branch to my machine and review them asap, probably but maybe not today

Yvand commented 1 year ago

@X-Guardian I think I covered all the changes you requested, but the ui of reviewable is so complicated to understand... Anyway, waiting on your feedback

X-Guardian commented 1 year ago

Thanks @Yvand, I will review and get back to you.

Yvand commented 1 year ago

@X-Guardian do you have an update on this PR?

X-Guardian commented 1 year ago

Hi @Yvand, I've reviewed your code and added some unit tests for the new functionality and an example. I also made a few small changes to your code. The only significant change was adding a new check so that if either SigningCertificateDnsName or DecryptionCertificateDnsName were specified, the other parameter also needed to be specified. This is a requirement of the Install-AdfsFarm cmdlet.

Can you take a look at my changes and check you are happy with them. We can then get the PR merged and create a preview release for testing through the PowerShell Gallery.

Yvand commented 1 year ago

Thank you @X-Guardian I'll review/test your changes and come back to you

Yvand commented 1 year ago

@X-Guardian I tested with your changes and it works perfectly fine

Yvand commented 1 year ago

@X-Guardian is this PR still in your radar?

X-Guardian commented 1 year ago

Hi @Yvand, I have released AdfsDsc v1.3.0 to the PowerShell Gallery which includes this PR. Thanks very much for your contribution.