Update SPWebApplication with Claims Authentication

[mrpullen]

#512

Add support for Claims providers to SharePointDSC. I haven't gotten to the SPWebAppExtension and there is a lot more to do to get this to meet submission guidelines. I am trying to find a good claims provider that I can install easily without a lot of additional work for testing.. if you have any ideas on that, greatly appreciated.

• Change details added to Unreleased section of changelog.md?
• Added/updated documentation and descriptions in .schema.mof files where appropriate?
• Examples updated for both the single server and small farm templates in the examples folder?
• New/changed code adheres to Style Guidelines?
• Unit and Integration tests created/updated where possible?

---

This change is 

[msftclas]

Hi @mrpullen, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[mrpullen]

Need to figure out how to support unit testing this.. Cause I would need ADFS or some other STS to issue the claims... And how is that going to work with appveyor. Also need to pull down the SPExtendedWebApplication resource and add changes to that.

[BrianFarnhill]

I'll have a look through this code when I get a minute, but we should be able to mock out the claims stuff to the point of validating our logic at least. I'll update here when I get a minute to look through it with some ideas for you.

[BrianFarnhill]

OK so a few little things (mostly style guidelines). The unit testing though you won't need ADFS or anything at all to test this. You can simply mock the output from the Get-SPAuthenticationProvider method (in much the same way as we already do in the tests for this resource. That will let you fake it to look like it has claims configured without need any infrastructure at all to actually make it work.

The other thing I would like on this one is an extra example file (under "modules\SharePointDsc\Examples\resources\SPWebApplication") that shows a claims based example added in here as well (this gets used by our documentation generator, so would love to get that included there)

Let me know if you need more help getting the tests sorted out and I'll do what I can, it shouldn't be too difficult though.

---

Reviewed 3 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 91 at r3 (raw file):

        $authProvider = Get-SPAuthenticationProvider -WebApplication $wa.Url -Zone "Default" 
        if($authProvider.DisplayName -eq "Windows Authentication") {

Need to drop the open brace to a new line here (style guidelines, not my call :P )

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 100 at r3 (raw file):

                $localAuthMode = "Kerberos" 
            }
            $AuthenticationProvider = $null

Lower case "a" at the start of this one as it's a local variable, not a parameter

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 102 at r3 (raw file):

            $AuthenticationProvider = $null
        }
        else {

Drop the brace to a new line

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 104 at r3 (raw file):

        else {
            $localAuthMode = "Claims"
            $AuthenticationProvider = "$($authProvider.DisplayName)"

No need to wrap this as a string, DisplayName is a string property already so just $authProvider.DisplayName should do it

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 255 at r3 (raw file):

                                                           -DisableKerberos:$true
                    } 
                    elseif ($params.AuthenticationMethod -eq "Claims" -and $params.AuthenticationProvider -ne "") {

Rather than elseif'ing this, can we switch it out for a "switch" statement? Makes more sense now it's a three choice thing

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 255 at r3 (raw file):

                                                           -DisableKerberos:$true
                    } 
                    elseif ($params.AuthenticationMethod -eq "Claims" -and $params.AuthenticationProvider -ne "") {

Can we put a test at the start of the set method that checks to make sure that if you specify "claims" that you have to provide an AuthenticationProvider name? Something like this

if ($AuthenticationMethod -eq "claims" and [string]::IsNullOrEmpty($AuthenticationProvider))
{
    throw "some meaningful error here"
}

And put this near the start of the set method to make sure we catch it early in the piece (outside of the Invoke-SPDscCommand bit)

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 256 at r3 (raw file):

                    } 
                    elseif ($params.AuthenticationMethod -eq "Claims" -and $params.AuthenticationProvider -ne "") {
                        $ap = Get-SPTrustedIdentityTokenIsser -Identity "$($params.AuthenticationProvider)"

Again we don't need to write the Identity value in a string here, just $params.AuthenticationProvider will work as it's a string to begin with

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.schema.mof, line 10 at r3 (raw file):

    [Write, Description("Should anonymous access be enabled for this web app")] boolean AllowAnonymous;
    [Write, Description("What authentication mode should be used for the web app"), ValueMap{"NTLM","Kerberos","Claims"}, Values{"NTLM","Kerberos","Claims"}] string AuthenticationMethod;
    [Write, Description("What authentication provider should be used for the web app Windows Authenication for NTLM, Kerberos, or the TrustedIssuer name for Claims")] string AuthenticationProvider;

Can I get a full stop after "used for the app" before you list what the options are?

---

Modules/SharePointDsc/Examples/Single Server/SharePoint.ps1, line 132 at r3 ([raw file](https://github.com/powershell/sharepointdsc/blob/cb0ff669e79d7a273e8d69ddecf576ea435d6d18/Modules/SharePointDsc/Examples/Single Server/SharePoint.ps1#L132)):

            AllowAnonymous         = $false
            AuthenticationMethod   = "NTLM"
            AuthenticationProvider = "Windows Authentication"

We shouldn't need to put this here by default if I'm looking at this right? So can we take this out from this example

---

Modules/SharePointDsc/Examples/Small Farm/SharePoint.ps1, line 132 at r3 ([raw file](https://github.com/powershell/sharepointdsc/blob/cb0ff669e79d7a273e8d69ddecf576ea435d6d18/Modules/SharePointDsc/Examples/Small Farm/SharePoint.ps1#L132)):

            AllowAnonymous         = $false
            AuthenticationMethod   = "NTLM"
            AuthenticationProvider = "Windows Authentication"

We shouldn't need to put this here by default if I'm looking at this right? So can we take this out from this example

---

Comments from Reviewable

[BrianFarnhill]

Also can I get you to put this in the changelog.md file as well :)

---

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

---

Comments from Reviewable

[mrpullen]

I should have these updates back to you today or tomorrow. Thanks for taking making the time to review.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (dev@4be40ad). Click here to learn what that means.
The diff coverage is 52.63%.

@@          Coverage Diff           @@
##             dev     #513   +/-   ##
======================================
  Coverage       ?   88.19%           
======================================
  Files          ?       79           
  Lines          ?     7409           
  Branches       ?        3           
======================================
  Hits           ?     6534           
  Misses         ?      872           
  Partials       ?        3

Impacted Files	Coverage Δ
...s/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1	83.49% <52.63%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4be40ad...f07d067. Read the comment docs.

[mrpullen]

I think I got everything.. Let me know how it looks.

[mrpullen]

Ok.. I have some more work to do on the unit tests. I will get back to this tonight.

[BrianFarnhill]

Reviewed 4 of 7 files at r4, 2 of 3 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 101 at r6 (raw file):

                $localAuthMode = "Kerberos" 
            }
            ##COMMENT: We need to be clever here. If the Resource specifies Windows Authentication we should return it. 

We don't need to do this sort of thing here - if it's NTLM or Kerberos we should be returning the authentication provider regardless of what the input parameters were. Remember the "get" methods are there to tell us about the current state, so the parameters we pass to it shouldn't have an impact on the get methods ability to describe the current state to us. Does that make sense? So we should just be able to say $authenticationProvider = "Windows Authentication" here and remove the comment.

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 113 at r6 (raw file):

        else 
        {
            ##COMMENT: Are there other things it could be?? For now if it's not Windows Auth we are assuming claims...

We can remove the comment here - I think the discussion is whether or not "claims" is the right terminology here and I think that we might wanna change it since technically speaking both NTLM and Kerberos are still using claims authentication, just against the internal STS. So perhaps the better thing here is to call it "ExternalClaim" or something like that? @ykuijs can I get your two cents worth on this?

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 213 at r6 (raw file):

    {

         if (($PSBoundParameters.ContainsKey("ServerRole") -eq $true) `

Why is this Server role stuff here? A copy/paste mistake I think?

---

Modules/SharePointDsc/DSCResources/MSFT_SPWebApplication/MSFT_SPWebApplication.psm1, line 280 at r6 (raw file):

                    {
                        $disableKerberos = ($params.AuthenticationMethod -eq "NTLM")
                         $ap = New-SPAuthenticationProvider -UseWindowsIntegratedAuthentication `

There's an extra space at the start of this line so it looks out of alignment with the code above it (again, style guidelines :P )

---

Modules/SharePointDsc/Examples/Resources/SPWebApplication/2-ClaimsExample.ps1, line 3 at r6 (raw file):

<#
.EXAMPLE
    This example shows how to create a new web application in the local farm

Can we update the comment here to explain the example a bit more - we use the comment in the documentation

---

Modules/SharePointDsc/Examples/Resources/SPWebApplication/2-ClaimsExample.ps1, line 18 at r6 (raw file):

            ##Ensure the Custom Claims Provider
            SPFarmSolution ClaimsProviderWsp

Do we really need the WSP in this example? I want to keep things nice and generic in these examples to focus on a specific resource. I'm inclined to want to keep this one as just the SPTrustedIdentityTokenIssuer and SPWebApplication.

---

Modules/SharePointDsc/Examples/Resources/SPWebApplication/2-ClaimsExample.ps1, line 26 at r6 (raw file):

                PsDscRunAsCredential = $SetupAccount
            }
            #Ensure the SPTrustedIdentityTokenIssuer

We can take the comments out of these as well

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 38 at r6 (raw file):

                AuthenticationMethod = "NTLM"
                Ensure = "Present"
                

Can we remove this empty line that got added?

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 63 at r6 (raw file):

                AuthenticationMethod = "NTLM"
                Ensure = "Present"
                 DatabaseServer = "sql.domain.local"

These parameters are all extra ones that shouldn't be needed so we can take them back out

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 139 at r6 (raw file):

                AuthenticationMethod = "NTLM"
                Ensure = "Present"
                DatabaseServer = "sql.domain.local"

These parameters are all extra ones that shouldn't be needed so we can take them back out

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 297 at r6 (raw file):

        }
        
        #region SPWebApplication with Claims Tests

Can we remove the region comments

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 431 at r6 (raw file):

            }
             It "Should call the new cmdlet from the set method" {
                Set-TargetResource @testParams

Can I get you to have a look at the logic here - the code coverage report is showing that line 275 ($ap = Get-SPTrustedIdentityTokenIsser -Identity $params.AuthenticationProvider) isn't getting hit and from the looks of it this scenario should have that covered. There should also be a mock for Get-SPTrustedIdentityTokenIsser somewhere in here as well for this scenario as it should be getting called from the set method here.

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 473 at r6 (raw file):

        }

        ##These tests are for the AuthenticationMethod and AuthenticationProvider updated logic.                

Remove the comment

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 487 at r6 (raw file):

             Mock -CommandName Get-SPAuthenticationProvider -MockWith { 
                return @{ 
                    DisableKerberos = $true 

This return value needs to include a "DisplayName" property to make sure that this will check the logic in the get method at line 91

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 518 at r6 (raw file):

             Mock -CommandName Get-SPAuthenticationProvider -MockWith { 
                return @{ 
                    DisableKerberos = $false 

This return value needs to include a "DisplayName" property to make sure that this will check the logic in the get method at line 91

---

Comments from Reviewable

[BrianFarnhill]

Reviewed 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 638 at r7 (raw file):

            It "Should call New-SPWebApplication" {
                Set-TargetResource @testParams | 

This shouldn't be piping out to anything, it's causing an error in the tests. It should also have the Assert-MockCalled cmdlets for the key set commands in there as well

---

Comments from Reviewable

[BrianFarnhill]

I did some looking at your test fails - the fixes are all commented below. Hope that helps

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 397 at r8 (raw file):

            }
            
            Mock -CommandName Get-SPTrustedIdentityTokenIsser -MockWith {

There is a typo here causing the test to fail. "Issuer" instead of "Isser"

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 595 at r8 (raw file):

            }    
            It "Should return exception from the set method" {
                (Set-TargetResource @testParams) | Should Throw "When configuring SPWebApplication to use Claims the AuthenticationProvider value must be specified."

The call to the set function here needs to be inside curly braces {} not normal brackets () - this is causing one of your test failures

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 628 at r8 (raw file):

            
            Mock -CommandName Get-SPWebapplication -MockWith { 
                return $null

The context here says the web application DOES exist, but the mock is returning nothing. I suspect this is why your test method is returning false and causing you a fail. Also the get method here shouldn't be looking for absent, it should be looking for present. So this covers another test failure.

---

Comments from Reviewable

[mrpullen]

Found the same mistake Get-SPTrustedTokenIsser in the Resource file as well. Hoping everything passes muster. I have a lot of other resources I want to build. :)

[BrianFarnhill]

1 new discussion and 1 existing one left to look at here, almost done :)

---

Reviewed 1 of 2 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

Tests/Unit/SharePointDsc/SharePointDsc.SPWebApplication.Tests.ps1, line 53 at r10 (raw file):

        }

        Context -Name "The specified Managed Account does not exist and fails to resolve for unknown reason" -Fixture {

Your comments on these tests talk about managed accounts - can you update these so i can look at this context again?

---

Comments from Reviewable

[mrpullen]

So I was trying to 100% code coverage.. in SPWebApplication resource there is the following code in set-TargetResource

try 
                   {
                       Get-SPManagedAccount $params.ApplicationPoolAccount -ErrorAction Stop
                       $newWebAppParams.Add("ApplicationPoolAccount", $params.ApplicationPoolAccount)
                   }
                   catch 
                   {
                       if ($_.Exception.Message -like "*No matching accounts were found*") 
                       {
                           throw ("The specified managed account was not found. Please make " + `
                                  "sure the managed account exists before continuing.")
                           return
                       } 
                       else 
                       {
                           throw ("Error occurred. Web application was not created. Error " + `
                                  "details: $($_.Exception.Message)")
                           return
                       }
                   }

so just throwing an "alternate" error from the mock to get to the else statement above. The other test case for Managed account exception here was throwing an Exception.Message that matched the if conditional..

[BrianFarnhill]

@mrpullen That bit of code you have there is to make sure we showed a "friendlier" message to a user in the event of the managed account exception. So it's something I want to see left there, even if we don't test for it specifically. Realistically we are targeting 80% code coverage right now (which is 10% higher than the minimum requirement of the PowerShell team to hit at least 70%) and looking at the latest codecov report on this PR you've got 100% coverage of the changes you made to SPWebApplication so I think that's enough. I'll get through the review in full from here and we can merge in what you've done so far! :)

[BrianFarnhill]

Never mind, total mind blank on my behalf (this is why I shouldn't code review tired!) This one

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

---

Comments from Reviewable