Add Check for Application Exclusions for Conditional Access Policies

[dagarwal-mitre]

🗣 Description

This PR adds the ability to ensure that conditional access policies do not contain guest user exclusions, and ensures that every policy checking conditional access uses the PolicyConditionsMatch for consistency.

💭 Motivation and context

Closes #1170
Closes #1323

🧪 Testing

To test, within the conditional access policies, inside of users, inside the ExternalGuestorExternalUsers, update the MembershipKind and the GuestOrExternalUserTypes, and update it to look like the image. The default for these is null, feel free to take a look at the AADBaseConfig.rego file to see the default around line 25. Test with the different tenants.

✅ 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

• Demonstrate changes to the team for questions and comments.
  (Note: Only required for issues of size Medium or larger)

✅ 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.

[twneale]

Looks good 🐄

[tkol2022]

Looks like some unit tests are missing for specific non-compliant states that PolicyConditionsMatch is checking for.

Missing unit tests for ExcludeRoles for 1.1, 3.1, 3.7, 3.8
Missing unit tests for ExcludeApplications for 1.1, 2.1, 2.3, 3.2, 3.6, 3.7, 3.8

[tkol2022]

@dagarwal-mitre I see 6 instances of the comment below in AADConfig.Rego and it doesn't make sense to me in any of the contexts where it is placed. I'm wondering if we should remove it? Kindly take a look

# Filter: only include policies that meet all the requirements

[tkol2022]

The unit tests for policy 3.6 use an invalid JSON from AADBaseConfig.rego. In that JSON there is an IncludeUsers = All and then the unit tests add an IncludeRoles element as well. In a real tenant this can never happen because when you select the option to include roles you cannot then select the option to include all users as well. I think we should correct this to ensure that the policy and unit tests are aligned with practical scenarios. Given how finicky Rego is, we want to align the states we are testing with as closely to the real world as possible.

[tkol2022]

Some instances of calling PolicyConditionsMatch look like below whereas other cases the == true is omitted. Change them so they are all consistent to whatever you think is easier to maintain.

PolicyConditionsMatch(CAPolicy, true) == true

[tkol2022]

I think you should delete the line count(PrivRolesSet & ConvertToSet(CAPolicy.Conditions.Users.ExcludeRoles)) == 0 in AADConfig.rego for policy 3.6 because PolicyConditionsMatch verifies that the ExcludeRoles is empty so I think that line becomes OBE? I commented it out for now with my changes but take a look and see if you agree.