fix(m365): surface AuditLog.Read.All permission errors instead of false positives (#10907)

Co-authored-by: Hugo P.Brito <hugopbrit@gmail.com>
This commit is contained in:
abdou
2026-05-12 19:22:19 +02:00
committed by GitHub
parent 132e79df89
commit 7f3dcdf02f
7 changed files with 346 additions and 14 deletions
+8
View File
@@ -16,6 +16,14 @@ All notable changes to the **Prowler SDK** are documented in this file.
---
## [5.26.2] (Prowler UNRELEASED)
### 🐞 Fixed
- `entra_users_mfa_capable` and `entra_break_glass_account_fido2_security_key_registered` report a preventive FAIL per affected user (with the missing permission named) when the M365 service principal lacks `AuditLog.Read.All`, instead of mass false positives [(#10907)](https://github.com/prowler-cloud/prowler/pull/10907)
---
## [5.26.1] (Prowler v5.26.1)
### 🐞 Fixed
@@ -85,6 +85,15 @@ class entra_break_glass_account_fido2_security_key_registered(Check):
resource_id=user.id,
)
if entra_client.user_registration_details_error:
report.status = "FAIL"
report.status_extended = (
f"Cannot verify FIDO2 security key registration for break glass account {user.name}: "
f"{entra_client.user_registration_details_error}."
)
findings.append(report)
continue
auth_methods = set(user.authentication_methods)
has_fido2 = "fido2SecurityKey" in auth_methods
has_passkey_device_bound = "passKeyDeviceBound" in auth_methods
@@ -3,7 +3,7 @@ import json
from asyncio import gather
from datetime import datetime, timezone
from enum import Enum
from typing import Dict, List, Optional
from typing import Any, Dict, List, Optional, Tuple
from uuid import UUID
from kiota_abstractions.base_request_configuration import RequestConfiguration
@@ -76,6 +76,7 @@ class Entra(M365Service):
self.tenant_domain = provider.identity.tenant_domain
self.tenant_id = getattr(provider.identity, "tenant_id", None)
self.user_registration_details_error: Optional[str] = None
attributes = loop.run_until_complete(
gather(
self._get_authorization_policy(),
@@ -854,7 +855,9 @@ class Entra(M365Service):
for member in members:
user_roles_map.setdefault(member.id, []).append(role_template_id)
registration_details = await self._get_user_registration_details()
registration_details, self.user_registration_details_error = (
await self._get_user_registration_details()
)
while users_response:
for user in getattr(users_response, "value", []) or []:
@@ -897,18 +900,24 @@ class Entra(M365Service):
)
return users
async def _get_user_registration_details(self):
async def _get_user_registration_details(
self,
) -> Tuple[Dict[str, Dict[str, Any]], Optional[str]]:
"""Retrieve user authentication method registration details.
Fetches registration details from the Microsoft Graph API, including
MFA capability and the specific authentication methods each user has registered.
Returns:
dict: A dictionary mapping user IDs to their registration details,
where each value is a dict with 'is_mfa_capable' (bool) and
'authentication_methods' (list of str).
A tuple containing:
- A dictionary mapping user IDs to their registration details,
where each value is a dict with 'is_mfa_capable' (bool) and
'authentication_methods' (list of str), or an empty dict if
retrieval fails.
- An error message string if there was an access error, None otherwise.
"""
registration_details = {}
error_message = None
try:
registration_builder = (
self.client.reports.authentication_methods.user_registration_details
@@ -933,16 +942,25 @@ class Entra(M365Service):
next_link
).get()
except Exception as error:
if (
error.__class__.__name__ == "ODataError"
and error.__dict__.get("response_status_code", None) == 403
):
except ODataError as error:
error_code = getattr(error.error, "code", None) if error.error else None
if error_code == "Authorization_RequestDenied":
error_message = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All"
logger.error(
f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error_message}"
)
else:
logger.error(
f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
)
error_message = str(error)
except Exception as error:
logger.error(
f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
)
error_message = f"Failed to retrieve user registration details: {error}"
return registration_details
return registration_details, error_message
async def _get_oauth_apps(self) -> Optional[Dict[str, "OAuthApp"]]:
"""
@@ -13,6 +13,10 @@ class entra_users_mfa_capable(Check):
("Ensure all member users are 'MFA capable'").
Guest users and disabled accounts are excluded from the evaluation.
- PASS: The member user is MFA capable.
- FAIL: The member user is not MFA capable, or MFA capability cannot be
verified due to insufficient permissions to read user registration details.
"""
def execute(self) -> List[CheckReportM365]:
@@ -42,7 +46,13 @@ class entra_users_mfa_capable(Check):
resource_id=user.id,
)
if not user.is_mfa_capable:
if entra_client.user_registration_details_error:
report.status = "FAIL"
report.status_extended = (
f"Cannot verify MFA capability for user {user.name}: "
f"{entra_client.user_registration_details_error}."
)
elif not user.is_mfa_capable:
report.status = "FAIL"
report.status_extended = f"User {user.name} is not MFA capable."
else:
@@ -67,6 +67,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -104,6 +105,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -142,6 +144,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -178,6 +181,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -228,6 +232,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -275,6 +280,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -321,6 +327,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -368,6 +375,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -422,6 +430,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -457,6 +466,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -500,3 +510,117 @@ class Test_entra_break_glass_account_fido2_security_key_registered:
assert len(result) == 1
assert result[0].status == "PASS"
assert result[0].resource_name == "BreakGlass1"
def test_user_registration_details_permission_error(self):
"""Test FAIL when there's a permission error reading user registration details."""
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All"
with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_m365_provider(),
),
mock.patch(
f"{CHECK_MODULE_PATH}.entra_client",
new=entra_client,
),
):
from prowler.providers.m365.services.entra.entra_break_glass_account_fido2_security_key_registered.entra_break_glass_account_fido2_security_key_registered import (
entra_break_glass_account_fido2_security_key_registered,
)
policy_id = str(uuid4())
bg_user_id = str(uuid4())
entra_client.conditional_access_policies = {
policy_id: _make_policy(policy_id, excluded_users=[bg_user_id]),
}
entra_client.users = {
bg_user_id: User(
id=bg_user_id,
name="BreakGlass1",
on_premises_sync_enabled=False,
authentication_methods=[],
),
}
check = entra_break_glass_account_fido2_security_key_registered()
result = check.execute()
assert len(result) == 1
assert result[0].status == "FAIL"
assert (
"Cannot verify FIDO2 security key registration for break glass account BreakGlass1"
in result[0].status_extended
)
assert "AuditLog.Read.All" in result[0].status_extended
assert result[0].resource_name == "BreakGlass1"
assert result[0].resource_id == bg_user_id
def test_user_registration_details_permission_error_with_missing_user(self):
"""Per-user emission and missing-user short-circuit on the error path.
Two break-glass user IDs are excluded from all CAPs, but only one is
present in ``entra_client.users``. With ``user_registration_details_error``
set, the present user must produce one preventive FAIL anchored to the
real user; the missing user must be skipped by the existing
``if not user: continue`` guard rather than crash or yield a synthetic
finding.
"""
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All"
with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_m365_provider(),
),
mock.patch(
f"{CHECK_MODULE_PATH}.entra_client",
new=entra_client,
),
):
from prowler.providers.m365.services.entra.entra_break_glass_account_fido2_security_key_registered.entra_break_glass_account_fido2_security_key_registered import (
entra_break_glass_account_fido2_security_key_registered,
)
policy_id = str(uuid4())
present_user_id = str(uuid4())
missing_user_id = str(uuid4())
entra_client.conditional_access_policies = {
policy_id: _make_policy(
policy_id,
excluded_users=[present_user_id, missing_user_id],
),
}
entra_client.users = {
present_user_id: User(
id=present_user_id,
name="BreakGlass1",
on_premises_sync_enabled=False,
authentication_methods=[],
),
# missing_user_id intentionally absent — exercises the
# `if not user: continue` short-circuit inside the loop.
}
check = entra_break_glass_account_fido2_security_key_registered()
result = check.execute()
# One finding for the present user; the missing one is skipped.
assert len(result) == 1
assert result[0].status == "FAIL"
assert (
"Cannot verify FIDO2 security key registration for break glass account BreakGlass1"
in result[0].status_extended
)
assert "AuditLog.Read.All" in result[0].status_extended
assert result[0].resource == entra_client.users[present_user_id]
assert result[0].resource_name == "BreakGlass1"
assert result[0].resource_id == present_user_id
@@ -11,6 +11,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -53,6 +54,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -95,6 +97,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -153,6 +156,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -191,6 +195,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -248,6 +253,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -286,6 +292,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -324,6 +331,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -383,6 +391,7 @@ class Test_entra_users_mfa_capable:
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = None
with (
mock.patch(
@@ -420,3 +429,125 @@ class Test_entra_users_mfa_capable:
assert result[0].resource == entra_client.users[user_id]
assert result[0].resource_name == "Test User"
assert result[0].resource_id == user_id
def test_user_registration_details_permission_error(self):
"""Test FAIL when there's a permission error reading user registration details."""
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All"
with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_m365_provider(),
),
mock.patch(
"prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable.entra_client",
new=entra_client,
),
):
from prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable import (
entra_users_mfa_capable,
)
user_id = str(uuid4())
entra_client.users = {
user_id: User(
id=user_id,
name="Test User",
on_premises_sync_enabled=False,
directory_roles_ids=[],
is_mfa_capable=False,
account_enabled=True,
)
}
check = entra_users_mfa_capable()
result = check.execute()
assert len(result) == 1
assert result[0].status == "FAIL"
assert (
"Cannot verify MFA capability for user Test User"
in result[0].status_extended
)
assert "AuditLog.Read.All" in result[0].status_extended
assert result[0].resource == entra_client.users[user_id]
assert result[0].resource_name == "Test User"
assert result[0].resource_id == user_id
def test_user_registration_details_permission_error_skips_guest_and_disabled(self):
"""CIS-scope skip (Guest, disabled) still applies on the permission-error path.
With ``user_registration_details_error`` set, only enabled member users
should receive a per-user "Cannot verify MFA capability" FAIL — guests
and disabled members are filtered out before the error branch runs.
"""
entra_client = mock.MagicMock
entra_client.audited_tenant = "audited_tenant"
entra_client.audited_domain = DOMAIN
entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All"
with (
mock.patch(
"prowler.providers.common.provider.Provider.get_global_provider",
return_value=set_mocked_m365_provider(),
),
mock.patch(
"prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable.entra_client",
new=entra_client,
),
):
from prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable import (
entra_users_mfa_capable,
)
member_id = str(uuid4())
guest_id = str(uuid4())
disabled_member_id = str(uuid4())
entra_client.users = {
member_id: User(
id=member_id,
name="Enabled Member",
on_premises_sync_enabled=False,
directory_roles_ids=[],
is_mfa_capable=False,
account_enabled=True,
user_type="Member",
),
guest_id: User(
id=guest_id,
name="Guest User",
on_premises_sync_enabled=False,
directory_roles_ids=[],
is_mfa_capable=False,
account_enabled=True,
user_type="Guest",
),
disabled_member_id: User(
id=disabled_member_id,
name="Disabled Member",
on_premises_sync_enabled=False,
directory_roles_ids=[],
is_mfa_capable=False,
account_enabled=False,
user_type="Member",
),
}
check = entra_users_mfa_capable()
result = check.execute()
# Only the enabled member should be reported — Guest and
# disabled member are skipped before the error branch.
assert len(result) == 1
assert result[0].status == "FAIL"
assert (
"Cannot verify MFA capability for user Enabled Member"
in result[0].status_extended
)
assert "AuditLog.Read.All" in result[0].status_extended
assert result[0].resource == entra_client.users[member_id]
assert result[0].resource_name == "Enabled Member"
assert result[0].resource_id == member_id
@@ -665,10 +665,11 @@ class Test_Entra_Service:
)
)
registration_details = asyncio.run(
registration_details, error_message = asyncio.run(
entra_service._get_user_registration_details()
)
assert error_message is None
assert registration_details == {
"user-1": {
"is_mfa_capable": True,
@@ -686,6 +687,37 @@ class Test_Entra_Service:
registration_builder.with_url.assert_called_once_with("next-link")
registration_builder_next.get.assert_awaited()
def test__get_user_registration_details_returns_error_on_permission_denied(self):
"""Test that 403 Authorization_RequestDenied returns an empty dict and
a descriptive error message naming the missing AuditLog.Read.All permission.
"""
from msgraph.generated.models.o_data_errors.main_error import MainError
from msgraph.generated.models.o_data_errors.o_data_error import ODataError
odata_error = ODataError()
odata_error.error = MainError()
odata_error.error.code = "Authorization_RequestDenied"
registration_builder = SimpleNamespace(get=AsyncMock(side_effect=odata_error))
entra_service = Entra.__new__(Entra)
entra_service.client = SimpleNamespace(
reports=SimpleNamespace(
authentication_methods=SimpleNamespace(
user_registration_details=registration_builder
)
)
)
registration_details, error_message = asyncio.run(
entra_service._get_user_registration_details()
)
assert registration_details == {}
assert error_message is not None
assert "AuditLog.Read.All" in error_message
assert "user registration details" in error_message
def test__get_service_principals_filters_third_party_owners(self):
"""Service principals owned by another tenant must not be returned."""
# Mixed-case input to verify the service normalizes both sides before