feat(m365): add Entra Conditional Access group management restriction#11342
Open
SAMurai-16 wants to merge 2 commits into
Open
feat(m365): add Entra Conditional Access group management restriction#11342SAMurai-16 wants to merge 2 commits into
SAMurai-16 wants to merge 2 commits into
Conversation
github-actions
Bot
added
provider/m365
Contributor
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new M365 Entra check to ensure groups referenced by enabled/report-only Conditional Access policies are protected (management-restricted or role-assignable), and extends group collection to fetch the required Graph properties.
Changes:
- Introduces
entra_conditional_access_policy_groups_management_restrictedcheck implementation + metadata. - Updates Entra group retrieval to
$selectrole-assignable and management-restricted properties and handle pagination. - Adds unit tests covering pass/fail scenarios for protected, unprotected, and unresolved group references.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
prowler/providers/m365/services/entra/entra_service.py |
Fetches additional group fields (isAssignableToRole, isManagementRestricted) and paginates groups results. |
prowler/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/entra_conditional_access_policy_groups_management_restricted.py |
New check logic to evaluate Conditional Access group references. |
prowler/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/entra_conditional_access_policy_groups_management_restricted.metadata.json |
Metadata for the new check (description, risk, remediation, URLs). |
tests/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/entra_conditional_access_policy_groups_management_restricted_test.py |
New tests for the check behavior. |
tests/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/__init__.py |
Adds package marker for tests directory. |
prowler/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/__init__.py |
Adds package marker for check directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+724
to
+735
| query_parameters = ( | ||
| GroupsRequestBuilder.GroupsRequestBuilderGetQueryParameters( | ||
| select=[ | ||
| "id", | ||
| "displayName", | ||
| "groupTypes", | ||
| "membershipRule", | ||
| "isAssignableToRole", | ||
| "isManagementRestricted", | ||
| ], | ||
| ) | ||
| ) |
Author
There was a problem hiding this comment.
I verified against the installed msgraph-sdk==1.55.0:
module has GroupsRequestBuilderGetQueryParameters: False class has GroupsRequestBuilderGetQueryParameters: True
Comment on lines
+106
to
+113
| def test_no_enabled_or_report_only_policy_references_groups(self): | ||
| entra_client = mock.MagicMock | ||
| entra_client.audited_tenant = "audited_tenant" | ||
| entra_client.audited_domain = DOMAIN | ||
| entra_client.groups = [] | ||
| entra_client.conditional_access_policies = { | ||
| "policy-1": _make_policy(state=ConditionalAccessPolicyState.DISABLED) | ||
| } |
Comment on lines
+107
to
+113
| entra_client = mock.MagicMock | ||
| entra_client.audited_tenant = "audited_tenant" | ||
| entra_client.audited_domain = DOMAIN | ||
| entra_client.groups = [] | ||
| entra_client.conditional_access_policies = { | ||
| "policy-1": _make_policy(state=ConditionalAccessPolicyState.DISABLED) | ||
| } |
| @@ -0,0 +1 @@ | |||
|
|
|||
Comment on lines
1639
to
+1643
| name: str | ||
| groupTypes: List[str] | ||
| membershipRule: Optional[str] | ||
| is_assignable_to_role: bool = False | ||
| is_management_restricted: bool = False |
Comment on lines
+753
to
+758
| ) | ||
| or False, | ||
| is_management_restricted=getattr( | ||
| group, "is_management_restricted", False | ||
| ) | ||
| or False, |
Author
|
@HugoPBrito Please review and suggest changes if needed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Adds a Microsoft 365 Entra check for Conditional Access policies that reference security groups in
includeGroupsorexcludeGroups.Groups used by Conditional Access policies are security-sensitive because changing group membership can affect who is included in, or excluded from, access controls. This check helps detect groups that are not protected by Restricted Management Administrative Units or role-assignable group controls.
Fix #11060
Description
This PR adds
entra_conditional_access_policy_groups_management_restricted.The check evaluates enabled and report-only Conditional Access policies, collects referenced group IDs from
conditions.users.includeGroupsandconditions.users.excludeGroups, and verifies each resolved group has at least one of:isManagementRestricted = trueisAssignableToRole = trueThe check fails when a referenced group has both values set to false, and reports unresolved group IDs separately as stale Conditional Access references.
Changes included:
isAssignableToRoleandisManagementRestricted.No new provider permissions are required.
Steps to review
prowler/providers/m365/services/entra/entra_service.py.prowler/providers/m365/services/entra/entra_conditional_access_policy_groups_management_restricted/.SDK/CLI
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.