Add policy check for SharePoint 3.2 when using service principal and update SharePoint 4.2 rego for deprecation

[mitchelbaker-cisa]

🗣 Description

• Removed input.OneDrive_PnP_Flag == false/true statements and Not-Implemented test in SharePointConfig.rego. The input.OneDrive_PnP_Flag is still used in the SharePoint unit tests to indicate spo/pnp variants

• Removed unnecessary fields from sharepoint.spo.testplan.yaml to get rid of warnings; removed deprecated parameter ("Detailed"=$true) from Get-SPOSite and Get-PnPTenantSite in SharePoint provider

• Expanded unit test coverage for SharePoint 3.2 to cover service principal cases in SharepointConfig_03_test.rego; expanded functional test coverage for SharePoint 3.2 in the sharepoint.pnp.testplan.yaml

• Logic for SharePoint 3.1 fixed to account for negative integer value, -1, when the Anyone links checkbox is not set. The policy now displays a fail for this case

• Set SharePoint 4.2 to not-implemented in SharepoingConfig.rego, updated its unit/functional tests

💭 Motivation and context

Closes #1221
Closes #1268
Closes #1220
Closes #1324

🧪 Testing

Checkout the branch locally:

git fetch
git checkout 1221-sharepoint-3.2-incorrect-NA

Check if all unit tests pass:

.\Testing\RunUnitTests.ps1 -p sharepoint

Check if all functional tests pass against SharePoint, use both spo/pnp variants.

✅ Pre-approval checklist

• This PR has an informative and human-readable title.
• PR targets the correct parent branch (e.g., main or release-name) for merge.
• Changes are limited to a single goal - eschew scope creep!
• Changes are sized such that they do not touch excessive number of files.
• All future TODOs are captured in issues, which are referenced in code comments.
• These code changes follow the ScubaGear content style guide.
• Related issues these changes resolve are linked preferably via closing keywords.
• All relevant type-of-change labels added.
• All relevant project fields are set.
• All relevant repo and/or project documentation updated to reflect these changes.
• Unit tests added/updated to cover PowerShell and Rego changes.
• Functional tests added/updated to cover PowerShell and Rego changes.
• All relevant functional tests passed.
• All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ Pre-merge checklist

• PR passed smoke test check.

• Feature branch has been rebased against changes from parent branch, as needed

  Use Rebase branch button below or use this reference to rebase from the command line.

• Resolved all merge conflicts on branch

• Notified merge coordinator that PR is ready for merge via comment mention

✅ Post-merge checklist

• Feature branch deleted after merge to clean up repository.
• Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

[tkol2022]

It is not clear why the test_File_Folder_AnonymousLinkType_Correct should produce a Pass? The SharingCapability is explicitly set to 2 (Anyone) within the unit test, but the other fields that are checked by the policy FileAnonymousLinkType & FolderAnonymousLinkType are not set in the unit test. Since they are already setup to the compliant value of 1 in the SharepointBaseConfig.rego file, the policy passes, but as a developer when I read the unit test this is not clear and I think this will make it hard to maintain these tests and know what they doing. I am wondering if we should explicitly set the values related to the policy and scenario we are testing within each unit test?

[tkol2022]

I don't see the difference between these two tests?
Also, what does the "UsingServicePrincipal" in the test name mean? I don't see anything in the test JSON that indicates that a service principal was used.

I don't see the difference between the two tests below either?

[tkol2022]

Reviewed the code updates, unit tests and functional tests. Will leverage the support of another team member to perform hands-on testing.

I had a couple of minor questions related to the consistency and maintainability of the unit tests. Other than that, looks good. Nice work.

[mitchelbaker-cisa]

Reviewed the code updates, unit tests and functional tests. Will leverage the support of another team member to perform hands-on testing.

I had a couple of minor questions related to the consistency and maintainability of the unit tests. Other than that, looks good. Nice work.

@tkol2022 Thanks for the review. I addressed your feedback in 7bc0975

[Sloane4]

I thought we were removing this?

[mitchelbaker-cisa]

It's only removed in the SharePoint.rego file. When running Invoke-SCuBA with the AppId parameter this sets $PnPFlag = $true in Orchestrator.ps1.

It's then passed into the SharePoint provider to conditionally run either Get-PnPTenant/Get-PnpTenantSite or Get-SPOTenant/Get-SPOSite commandlets.

The purpose of keeping input.OneDrive_PnP_Flag as true in the unit tests is to mock running Invoke-SCuBA via service principal, otherwise the tests will default to the interactive authentication variant.

[tkol2022]

@mitchelbaker-cisa Seems there is a logic flaw in the Rego implementation for policy 3.1 that I found when testing the branch. It is easy to fix so I recommend that you address it now especially given the importance of this release.

To recreate the problem

Set the external sharing slider to Anyone

Uncheck the expiration days option

Run ScubaGear and observe that it produces a Pass which is incorrect because the expiration option was not selected and configured

The fix

I believe the fix is relatively straightforward. When the expiration checkbox is unselected in the portal, you will notice that the JSON produces a value of -1 for RequireAnonymousLinksExpireInDays, so I am thinking that we need to ensure that the value of the field is between 1 and 30 inclusive. Currently the Rego code only checks for <= 30 and hence why it is flawed. Take a look and see if you agree.

[tkol2022]

N/A messages for section 3 are inconsistent with the baseline document

While testing I noticed that when policies 3.1, 3.2 or 3.3 produce an N/A, the messages provided in the report are inaccurate and/or do not align with the messages in the baseline document.

Policies 3.1 and 3.2

Should say "This policy is only applicable if the external sharing slider on the admin center sharing page is set to Anyone." but in the report they say "This policy is only applicable if External Sharing is set to any value other than Anyone."

Policy 3.3

Should say "This policy is only applicable if the external sharing slider on the admin center sharing page is set to Anyone or New and existing guests." but in the report it says "This policy is only applicable if External Sharing is set to any value other than Only People In Your Organization or Existing Guests." In this case, technically the message in the report is correct but it does not align with the baseline so we might as well synch them up for consistency.

[tkol2022]

I tested interactive and non-interactive against E5 tenant and policy 3.2 seems to behave as expected.

[mitchelbaker-cisa]

@tkol2022

I addressed the logic for policy 3.1 to account for the negative integer value, -1, when the Anyone links checkbox is not set. The policy now displays a fail for this case.

The report details for policies 3.1-3.3 are also fixed.

[tkol2022]

@tkol2022

I addressed the logic for policy 3.1 to account for the negative integer value, -1, when the Anyone links checkbox is not set. The policy now displays a fail for this case.

The report details for policies 3.1-3.3 are also fixed.

@Sloane4 Can you test the negative integer value logic and make sure it works for you? would be nice to have some independent validation.