From d98c09701157fae7fc9735a1ddfcc1af95f1a215 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 03:00:49 +0000 Subject: [PATCH] feat(connectors): OneDrive checks follow the design's rows, copy and states OneDrive follows the design: - Names and order: the 9 checks are the design's 8 rows. The drive count shares the "List users" row, which now validates the credential binding. Dependencies follow the design's run order; "Find a OneDrive" also runs after "Find selected users", so no user found stops it. - Every failure gives a short line and a description that names the grant the app needs. Passing checks give what they found ("Up to {n} OneDrives", "Opened the OneDrive of {user}", "Read members of {group}"...). - Find selected users is a partial find: some missing is INCOMPLETE and the drive checks walk the found users; none found is FAILED. An entry that is not an email and a list of blanks have their rows. - Checks that passed without testing anything are now INCOMPLETE: an empty OneDrive for the permission read, and groups that all hide their members. A group that went away is INCOMPLETE, and a user list that does not end is INCOMPLETE, not an unexpected error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KFMazQbuG8ui3jxBxm47sC --- .../connectors/onedrive/capability_checks.py | 565 +++++++++++++----- .../onedrive/test_onedrive_check_row_text.py | 263 ++++++++ .../onedrive/test_onedrive_connector.py | 26 +- 3 files changed, 684 insertions(+), 170 deletions(-) create mode 100644 backend/tests/unit/onyx/connectors/onedrive/test_onedrive_check_row_text.py diff --git a/backend/onyx/connectors/onedrive/capability_checks.py b/backend/onyx/connectors/onedrive/capability_checks.py index 2484a24ce0..f58a02fe3a 100644 --- a/backend/onyx/connectors/onedrive/capability_checks.py +++ b/backend/onyx/connectors/onedrive/capability_checks.py @@ -1,5 +1,18 @@ +"""Capability checks for the OneDrive connector. + +Each check makes small probes (one user, one drive, one delta page, one +group) with the gateway operations indexing and the syncs call. The drive +checks walk candidate users: the first configured users Graph finds, else +one user from each of the first pages of the tenant's user listing. + +"Find selected users" is a partial find: when some are missing it is +INCOMPLETE, and the drive checks after it walk the users it found. +""" + from collections.abc import Callable, Generator -from typing import TypeVar +from typing import NoReturn, TypeVar + +from pydantic import BaseModel from onyx.connectors.capability_checks.findings import bounded_count from onyx.connectors.capability_checks.form_state import FormState @@ -16,11 +29,15 @@ from onyx.connectors.capability_checks.models import ( ) from onyx.connectors.exceptions import ( ConnectorValidationError, + InconclusiveValidationError, UnexpectedValidationError, ) from onyx.connectors.microsoft_utils.check_text import ( + ENTRA_GROUPS_DENIED_ROW, MicrosoftAccountLabels, + MicrosoftRowText, MicrosoftSignIn, + grant_line, ) from onyx.connectors.microsoft_utils.drive_delta import ( DRIVE_DELTA_SELECT_FIELDS, @@ -51,42 +68,84 @@ _CANDIDATE_PAGES = 20 # A bound on the pages the drive count reads. Pages hold up to 999 users, so # the count reaches its cap long before this unless Graph sends empty pages. _COUNT_PAGES = 20 +# The longest row line the card shows. +_MAX_LINE = 40 +_LOCKED_STATUS = 423 T = TypeVar("T") _HIDDEN_MEMBERSHIP_VISIBILITY = "HiddenMembership" _ACCOUNT_LABELS = MicrosoftAccountLabels.with_prefix( "OneDrive", private_key="OneDrive Certificate" ) +_USERS_GRANT_LINE = "Grant `User.Read.All` and admin-consent it." +_RETRY_LINE = "Re-run the checks. If it repeats, send the error details to support." + _SIGN_IN_CHECK_ID = "onedrive_token_auth" -# Every probe signs in first, so a failed sign-in is one row. -_AFTER_SIGN_IN: tuple[str, ...] = (_SIGN_IN_CHECK_ID,) +_USERS_CHECK_ID = "onedrive_users" +_CONFIGURED_USERS_CHECK_ID = "onedrive_configured_users" +_DRIVE_CHECK_ID = "onedrive_drive" +_GROUPS_CHECK_ID = "onedrive_groups" +# check_id to the checks it runs after, in run order. +_DEPENDS_ON: dict[str, tuple[str, ...]] = { + _USERS_CHECK_ID: (_SIGN_IN_CHECK_ID,), + "onedrive_drive_count": (_SIGN_IN_CHECK_ID,), + _CONFIGURED_USERS_CHECK_ID: (_USERS_CHECK_ID,), + # The selected users too: with none found there is no OneDrive to open. + _DRIVE_CHECK_ID: (_USERS_CHECK_ID, _CONFIGURED_USERS_CHECK_ID), + "onedrive_delta": (_DRIVE_CHECK_ID,), + "onedrive_item_permissions": (_DRIVE_CHECK_ID,), + _GROUPS_CHECK_ID: (_SIGN_IN_CHECK_ID,), + "onedrive_group_members": (_GROUPS_CHECK_ID,), +} -_EMPTY_DRIVE_ADVISORY = CheckAdvisory( - message=( - "The check found no item in the OneDrives it sampled. " - "It did not read item permissions. " - f"The check samples the drives of the first {_CANDIDATE_PAGES} configured " - f"users, or of the users on the first {_CANDIDATE_PAGES} pages of the " - "user list. Add a file to one of those drives and run the check again " - "to verify this access." - ) + +def _line(prefix: str, name: str, suffix: str = "") -> str: + """``prefix``, ``name`` and ``suffix``, with the name cut to fit the row.""" + room: int = _MAX_LINE - len(prefix) - len(suffix) + cut: str = name if len(name) <= room else name[: room - 1] + "…" + return f"{prefix}{cut}{suffix}" + + +def _row(summary: str, description: str) -> MicrosoftRowText: + return MicrosoftRowText(summary=summary, description=description) + + +_USERS_DENIED_ROW = _row( + "No permission to read user directory", + f"Every-user mode and user lookups would not work.\n{_USERS_GRANT_LINE}", ) -_NO_VISIBLE_GROUP_ADVISORY = CheckAdvisory( - message=( - "The check found no Entra group with visible membership " - f"in the first {_CANDIDATE_PAGES} groups. " - "It did not read transitive group members. " - "The check skips groups with hidden membership." - ) +_USER_LIST_UNENDING_ROW = _row( + "User list did not end", + "Microsoft kept returning short pages.\nRe-run the checks later.", +) +_BLANK_USERS_ROW = _row( + "Users holds only blank entries", + "Specific scope is chosen but no user address is filled in.\nAdd at least one " + "address to Users, or choose General.", +) +_DRIVES_DENIED_ROW = _row( + "No permission to read sampled OneDrives", + f"The app is refused on every OneDrive it tried.\n{grant_line('Sites.Read.All')}", +) +_GROUP_MEMBERS_DENIED_ROW = _row( + "No permission to read group members", + "The app cannot read who is in an Entra group.\n" + + grant_line("GroupMember.Read.All"), +) +_NO_DRIVE_ROW = _row( + "No OneDrives found to test", + "None of the sampled users has a OneDrive the app could open.\nSelect users " + "who have a OneDrive in Users, then run the checks again.", ) -class _NoCandidateFound(ConnectorValidationError): +class _NoCandidateFound(InconclusiveValidationError): pass def _gateway(context: CapabilityCheckContext) -> OneDriveSourceOperations: - assert isinstance(context.source_operations, OneDriveSourceOperations) + if not isinstance(context.source_operations, OneDriveSourceOperations): + raise TypeError("OneDrive checks need the OneDrive gateway.") return context.source_operations @@ -96,16 +155,49 @@ def _config(context: CapabilityCheckContext) -> OneDriveConnectorConfig: return form_config(context, OneDriveConnectorConfig) +def _raise_row(message: str, row: MicrosoftRowText) -> NoReturn: + raise ConnectorValidationError( + message, summary=row.summary, description=row.description + ) + + +def _is_invalid_entry(user: str) -> bool: + if not user.strip(): + return False + try: + normalize_configured_users([user]) + except ConnectorValidationError: + return True + return False + + def _configured_users(context: CapabilityCheckContext) -> list[str]: - return normalize_configured_users(_config(context).users) + """The Users entries, normalized as indexing reads them. An entry that is + not an email, or a list of blanks, fails with its row.""" + users: list[str] = _config(context).users or [] + try: + return normalize_configured_users(users) + except ConnectorValidationError as error: + if not any(user.strip() for user in users): + _raise_row(error.message, _BLANK_USERS_ROW) + entry: str = next( + (user.strip() for user in users if _is_invalid_entry(user)), "" + ) + _raise_row( + error.message, + _row( + _line("Not an email address: ", entry), + "Each entry in Users must be a user principal name or primary " + "email address.", + ), + ) def _candidate_users( context: CapabilityCheckContext, ) -> Generator[OneDriveUser, None, None]: gateway = _gateway(context) - config = _config(context) - configured = normalize_configured_users(config.users) + configured = _configured_users(context) if configured: for identifier in configured[:_CANDIDATE_PAGES]: try: @@ -135,12 +227,21 @@ def _candidate_users( return +class _Opened(BaseModel): + """The user whose OneDrive answered a probe.""" + + user: OneDriveUser + drive: OneDriveDrive + + def _first_drive_that( context: CapabilityCheckContext, opens: Callable[[OneDriveDrive], T | None], denied_message: str, - nothing_to_probe: str, -) -> tuple[OneDriveDrive, T]: + nothing_to_probe: MicrosoftRowText, +) -> tuple[_Opened, T]: + """The first candidate's OneDrive on which ``opens`` returns something. + A 403 is remembered: when nothing opens it is the likelier cause.""" gateway = _gateway(context) denied: OneDriveGraphError | None = None for user in _candidate_users(context): @@ -163,10 +264,18 @@ def _first_drive_that( denied = error continue if opened is not None: - return drive, opened + return _Opened(user=user, drive=drive), opened if denied is not None: - raise_for_graph_error(denied, denied_message) - raise _NoCandidateFound(nothing_to_probe) + raise_for_graph_error(denied, denied_message, denied_row=_DRIVES_DENIED_ROW) + raise _NoCandidateFound( + nothing_to_probe.description.split("\n", 1)[0], + summary=nothing_to_probe.summary, + description=nothing_to_probe.description, + ) + + +def _user_name(user: OneDriveUser) -> str: + return user.display_name or user.user_principal_name def _first_delta_item( @@ -176,7 +285,8 @@ def _first_delta_item( ) -> DriveDeltaItem | None: cursor: str | None = start_url for _ in range(_CANDIDATE_PAGES): - assert cursor is not None + if cursor is None: + return None result = gateway.get_delta_page( drive_id=drive.id, page_url=cursor, @@ -190,10 +300,12 @@ def _first_delta_item( if item is not None: return item cursor = result.next_checkpoint_url - if cursor is None: - return None - raise ConnectorValidationError( - f"No readable OneDrive item was found in {_CANDIDATE_PAGES} delta pages." + raise InconclusiveValidationError( + f"No readable OneDrive item was found in {_CANDIDATE_PAGES} delta pages.", + summary="No files found to test", + description=( + "The change feed of a OneDrive held only deleted items in its first pages." + ), ) @@ -243,15 +355,46 @@ def _group_membership_probe_group( return None -class _TokenCheck(CapabilityCheck): +class _OneDriveCheck(CapabilityCheck[OneDriveConnectorConfig]): + def __init__( + self, + *, + check_id: str, + display_name: str, + remediation: str, + capability: CredentialCapability = CredentialCapability.INDEXING, + requires_connector_config: bool = False, + validates_binding: bool = False, + fails_when_unreachable: bool = False, + discovery: bool = False, + row_id: str | None = None, + ) -> None: + super().__init__( + capability=capability, + check_id=check_id, + display_name=display_name, + requires_connector_instance=False, + requires_connector_config=requires_connector_config, + remediation=remediation, + docs_link=_DOCS_LINK, + depends_on=_DEPENDS_ON.get(check_id, ()), + validates_binding=validates_binding, + fails_when_unreachable=fails_when_unreachable, + discovery=discovery, + row_id=row_id, + ) + + +class _ConfigReadingCheck(_OneDriveCheck): + config_class = OneDriveConnectorConfig + + +class _TokenCheck(_OneDriveCheck): def __init__(self) -> None: super().__init__( - capability=CredentialCapability.INDEXING, check_id=_SIGN_IN_CHECK_ID, display_name="Sign in to Microsoft", - requires_connector_instance=False, remediation="Check the app id, tenant id, and client credential.", - docs_link=_DOCS_LINK, validates_binding=True, fails_when_unreachable=True, ) @@ -267,23 +410,26 @@ class _TokenCheck(CapabilityCheck): ) -class _UsersCheck(CapabilityCheck): +class _UsersCheck(_OneDriveCheck): def __init__(self) -> None: super().__init__( - capability=CredentialCapability.INDEXING, - check_id="onedrive_users", - display_name="Tenant users can be listed", - requires_connector_instance=False, + check_id=_USERS_CHECK_ID, + display_name="List users", remediation="Grant and admin-consent `User.Read.All`.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, + validates_binding=True, ) - def run(self, context: CapabilityCheckContext) -> None: + def run(self, context: CapabilityCheckContext) -> CheckOutput: try: _gateway(context).list_users(page_size=_PROBE_PAGE_SIZE) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot list tenant users.") + raise_for_graph_error( + error, + "The app cannot list tenant users.", + denied_row=_USERS_DENIED_ROW, + request="the user listing", + ) + return CheckOutput(summary="Tenant user list is readable") def _tenant_users( @@ -297,30 +443,27 @@ def _tenant_users( next_link = page.next_link if next_link is None: return - raise UnexpectedValidationError( - f"The tenant's user listing did not end within {_COUNT_PAGES} pages." + raise InconclusiveValidationError( + f"The tenant's user listing did not end within {_COUNT_PAGES} pages.", + summary=_USER_LIST_UNENDING_ROW.summary, + description=_USER_LIST_UNENDING_ROW.description, ) -class _DriveCountCheck(CapabilityCheck[OneDriveConnectorConfig]): +class _DriveCountCheck(_ConfigReadingCheck): """Counts the enabled member users: the owners of the OneDrives indexing walks in every-user mode. It does not open each drive, so a user without a - OneDrive still counts. Separate from ``onedrive_users``, which reads one - user to validate. A config that lists users walks only those, so the - tenant count does not apply to it.""" - - config_class = OneDriveConnectorConfig + OneDrive still counts. It shares the "List users" row, whose probe reads + one user. A config that lists users walks only those, so the tenant count + does not apply to it.""" def __init__(self) -> None: super().__init__( - capability=CredentialCapability.INDEXING, check_id="onedrive_drive_count", - display_name="Counts the tenant's OneDrives", - requires_connector_instance=False, + display_name="List users", remediation="Grant and admin-consent `User.Read.All`.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, discovery=True, + row_id=_USERS_CHECK_ID, ) def applies(self, form_state: FormState[OneDriveConnectorConfig]) -> bool: @@ -334,83 +477,169 @@ class _DriveCountCheck(CapabilityCheck[OneDriveConnectorConfig]): cap=MAX_FINDING_OPTIONS, ) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot list tenant users.") - return CheckOutput(findings=[finding]) + raise_for_graph_error( + error, + "The app cannot list tenant users.", + denied_row=_USERS_DENIED_ROW, + request="the user listing", + ) + count: str = f"{finding.count}+" if finding.at_least else str(finding.count) + return CheckOutput( + summary=f"Up to {count} {'OneDrive' if count == '1' else 'OneDrives'}", + findings=[finding], + ) -class _ConfiguredUsersCheck(CapabilityCheck[OneDriveConnectorConfig]): - config_class = OneDriveConnectorConfig +class _UserProblem(BaseModel): + identifier: str + message: str + locked: bool + + +def _user_problem_row(problem: _UserProblem) -> MicrosoftRowText: + if problem.locked: + return _row( + _line("User is locked: ", problem.identifier), + f"Microsoft refused this user's lookup.\n{_RETRY_LINE}", + ) + return _row( + _line("User not found: ", problem.identifier), + "No enabled member user has this address.\nCorrect or remove " + f"{problem.identifier} in Users.", + ) + + +def _users_output(identifiers: list[str], problems: list[_UserProblem]) -> CheckOutput: + if not problems: + return CheckOutput( + summary=( + "1 selected user found" + if len(identifiers) == 1 + else f"All {len(identifiers)} selected users found" + ) + ) + message: str = " ".join(problem.message for problem in problems) + first: MicrosoftRowText = _user_problem_row(problems[0]) + names: str = ", ".join(problem.identifier for problem in problems) + if len(problems) == len(identifiers): + if all(problem.locked for problem in problems): + raise UnexpectedValidationError( + message, summary=first.summary, description=first.description + ) + _raise_row( + message, + first + if len(problems) == 1 + else _row( + "No selected user found", + "None of the selected users is visible to this account.\nNot " + f"found: {names}.", + ), + ) + if len(problems) == 1: + return CheckOutput( + summary=first.summary, description=first.description, partial=True + ) + return CheckOutput( + summary=f"{len(problems)} of {len(identifiers)} users not found", + description=f"Not found: {names}.\nCorrect or remove them in Users.", + partial=True, + ) + + +class _ConfiguredUsersCheck(_ConfigReadingCheck): + """Every Users entry names an enabled member user. Some missing is + INCOMPLETE, and the drive checks walk the found ones; none found + FAILS.""" def __init__(self) -> None: super().__init__( - capability=CredentialCapability.INDEXING, - check_id="onedrive_configured_users", - display_name="Configured users resolve", - requires_connector_instance=False, - requires_connector_config=True, + check_id=_CONFIGURED_USERS_CHECK_ID, + display_name="Find selected users", remediation="Use enabled member user principal names.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, + requires_connector_config=True, ) - def run(self, context: CapabilityCheckContext) -> None: + def applies(self, form_state: FormState[OneDriveConnectorConfig]) -> bool: + return bool(form_state.config.users) + + def run(self, context: CapabilityCheckContext) -> CheckOutput: gateway = _gateway(context) - for identifier in _configured_users(context): + identifiers: list[str] = _configured_users(context) + problems: list[_UserProblem] = [] + for identifier in identifiers: try: user = gateway.get_user(identifier=identifier) except OneDriveGraphError as error: - raise_for_graph_error(error, f"The app cannot resolve `{identifier}`.") + if error.status != _LOCKED_STATUS: + raise_for_graph_error( + error, + f"The app cannot resolve `{identifier}`.", + denied_row=_USERS_DENIED_ROW, + request="the user lookup", + ) + problems.append( + _UserProblem( + identifier=identifier, + message=f"The app cannot resolve `{identifier}` ({error.code}).", + locked=True, + ) + ) + continue if user is None: - raise ConnectorValidationError(f"No user matches `{identifier}`.") + problems.append( + _UserProblem( + identifier=identifier, + message=f"No user matches `{identifier}`.", + locked=False, + ) + ) + return _users_output(identifiers, problems) -class _DriveCheck(CapabilityCheck[OneDriveConnectorConfig]): - config_class = OneDriveConnectorConfig - +class _DriveCheck(_ConfigReadingCheck): def __init__(self) -> None: super().__init__( - capability=CredentialCapability.INDEXING, - check_id="onedrive_drive", - display_name="A OneDrive is readable", - requires_connector_instance=False, - requires_connector_config=True, + check_id=_DRIVE_CHECK_ID, + display_name="Find a OneDrive", remediation="Grant `Sites.Read.All` or a selected personal-site read grant.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, + requires_connector_config=True, ) - def run(self, context: CapabilityCheckContext) -> None: + def run(self, context: CapabilityCheckContext) -> CheckOutput: try: - _first_drive_that( + opened, _ = _first_drive_that( context, lambda drive: drive, "The app cannot read the tenant's first OneDrives.", - "No readable OneDrive was found among the first users.", + _NO_DRIVE_ROW, ) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot read this user's OneDrive.") - - -class _DeltaCheck(CapabilityCheck[OneDriveConnectorConfig]): - config_class = OneDriveConnectorConfig - - def __init__(self) -> None: - super().__init__( - capability=CredentialCapability.INDEXING, - check_id="onedrive_delta", - display_name="OneDrive changes are readable", - requires_connector_instance=False, - requires_connector_config=True, - remediation="Grant `Sites.Read.All` or a selected personal-site read grant.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, + raise_for_graph_error( + error, + "The app cannot read this user's OneDrive.", + denied_row=_DRIVES_DENIED_ROW, + request="the OneDrive read", + ) + return CheckOutput( + summary=_line("Opened the OneDrive of ", _user_name(opened.user)) ) - def run(self, context: CapabilityCheckContext) -> None: + +class _DeltaCheck(_ConfigReadingCheck): + def __init__(self) -> None: + super().__init__( + check_id="onedrive_delta", + display_name="Read OneDrive changes", + remediation="Grant `Sites.Read.All` or a selected personal-site read grant.", + requires_connector_config=True, + ) + + def run(self, context: CapabilityCheckContext) -> CheckOutput: gateway = _gateway(context) try: host = _config(context).graph_api_host.rstrip("/") - _first_drive_that( + opened, _ = _first_drive_that( context, lambda drive: gateway.get_delta_page( drive_id=drive.id, @@ -424,100 +653,126 @@ class _DeltaCheck(CapabilityCheck[OneDriveConnectorConfig]): allow_full_resync=True, ), "The app cannot read changes in the tenant's first OneDrives.", - "No readable OneDrive delta was found among the first users.", + _row( + "OneDrive changes could not be read", + "The OneDrives opened, but none answered the change feed in a " + "usable way.\nRe-run the checks.", + ), ) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot read OneDrive changes.") + raise_for_graph_error( + error, + "The app cannot read OneDrive changes.", + denied_row=_DRIVES_DENIED_ROW, + request="the change feed read", + ) + return CheckOutput( + summary=_line("Read changes in ", _user_name(opened.user), "'s OneDrive") + ) -class _PermissionCheck(CapabilityCheck[OneDriveConnectorConfig]): - config_class = OneDriveConnectorConfig - +class _PermissionCheck(_ConfigReadingCheck): def __init__(self) -> None: super().__init__( capability=CredentialCapability.DOC_PERMISSION_SYNC, check_id="onedrive_item_permissions", - display_name="OneDrive item permissions are readable", - requires_connector_instance=False, - requires_connector_config=True, + display_name="Read file permissions", remediation="Grant `Sites.Read.All` or a selected personal-site read grant.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, + requires_connector_config=True, ) - def run(self, context: CapabilityCheckContext) -> CheckOutput | None: + def run(self, context: CapabilityCheckContext) -> CheckOutput: gateway = _gateway(context) host = _config(context).graph_api_host.rstrip("/") - found_empty_drive = False - - def probe_permissions(drive: OneDriveDrive) -> bool | None: - nonlocal found_empty_drive - result = _probe_item_permissions( - gateway, - drive, - f"{host}/{GRAPH_API_VERSION}", - ) - found_empty_drive = result is None - return result - try: _first_drive_that( context, - probe_permissions, + lambda drive: _probe_item_permissions( + gateway, drive, f"{host}/{GRAPH_API_VERSION}" + ), "The app cannot read permissions in the tenant's first OneDrives.", - "No readable OneDrive was found among the first users.", + _row( + "No files found to test", + "The OneDrives the check sampled hold no file.\nAdd a file to " + "one of them, then run the checks again.", + ), ) - except _NoCandidateFound: - if not found_empty_drive: - raise - return CheckOutput(advisories=[_EMPTY_DRIVE_ADVISORY]) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot read OneDrive permissions.") - return None + raise_for_graph_error( + error, + "The app cannot read OneDrive permissions.", + denied_row=_DRIVES_DENIED_ROW, + request="the permission read", + ) + return CheckOutput(summary="Read permissions of a file in a OneDrive") -class _GroupListCheck(CapabilityCheck): +class _GroupListCheck(_OneDriveCheck): def __init__(self) -> None: super().__init__( capability=CredentialCapability.EXTERNAL_GROUP_SYNC, - check_id="onedrive_groups", - display_name="Entra groups are readable", - requires_connector_instance=False, + check_id=_GROUPS_CHECK_ID, + display_name="List groups", remediation="Grant and admin-consent `GroupMember.ReadBasic.All`.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, ) - def run(self, context: CapabilityCheckContext) -> None: + def run(self, context: CapabilityCheckContext) -> CheckOutput: try: _gateway(context).list_groups(page_size=_PROBE_PAGE_SIZE) except OneDriveGraphError as error: - raise_for_graph_error(error, "The app cannot list Entra groups.") + raise_for_graph_error( + error, + "The app cannot list Entra groups.", + denied_row=ENTRA_GROUPS_DENIED_ROW, + request="the group listing", + ) + return CheckOutput(summary="Entra group list is readable") -class _GroupMembershipCheck(CapabilityCheck): +class _GroupMembershipCheck(_OneDriveCheck): def __init__(self) -> None: super().__init__( capability=CredentialCapability.EXTERNAL_GROUP_SYNC, check_id="onedrive_group_members", - display_name="Entra transitive group members are readable", - requires_connector_instance=False, + display_name="Read group members", remediation="Grant `GroupMember.ReadBasic.All`.", - docs_link=_DOCS_LINK, - depends_on=_AFTER_SIGN_IN, ) - def run(self, context: CapabilityCheckContext) -> CheckOutput | None: + def run(self, context: CapabilityCheckContext) -> CheckOutput: gateway = _gateway(context) + denied = "The app cannot expand transitive Entra group members." try: group = _group_membership_probe_group(gateway) if group is None: - return CheckOutput(advisories=[_NO_VISIBLE_GROUP_ADVISORY]) + raise InconclusiveValidationError( + "The check found no Entra group with visible membership in " + f"the first {_CANDIDATE_PAGES} groups, so it read no members.", + summary="No readable groups found", + description=( + "The first groups all hide their members, or the tenant " + "has none." + ), + ) gateway.list_transitive_group_members(group_id=group.id) except OneDriveGraphError as error: - denied = "The app cannot expand transitive Entra group members." - raise_for_graph_error(error, denied) - return None + if error.status == 404: + raise InconclusiveValidationError( + denied, + summary="Sample group disappeared", + description=( + "The group was deleted between the listing and the member " + "read.\nRe-run the checks." + ), + ) from error + raise_for_graph_error( + error, + denied, + denied_row=_GROUP_MEMBERS_DENIED_ROW, + request="the group member read", + ) + return CheckOutput( + summary=_line("Read members of ", group.display_name or group.id) + ) def build_onedrive_indexing_checks() -> list[CapabilityCheck]: diff --git a/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_check_row_text.py b/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_check_row_text.py new file mode 100644 index 0000000000..307275e5d1 --- /dev/null +++ b/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_check_row_text.py @@ -0,0 +1,263 @@ +"""The OneDrive checks' rows: the design's names, short lines and +descriptions, the run order, the partial find of selected users, and what a +check that tested nothing says.""" + +from typing import Any +from unittest.mock import MagicMock, create_autospec + +import pytest + +from onyx.configs.constants import DocumentSource +from onyx.connectors.capability_checks.models import ( + CapabilityCheck, + CapabilityCheckContext, + CapabilityCheckResult, + CapabilityCheckStatus, + CheckErrorKind, +) +from onyx.connectors.capability_checks.runner import run_capability_checks +from onyx.connectors.microsoft_utils.drive_delta import ( + DriveDeltaFetchResult, + DriveDeltaPage, +) +from onyx.connectors.microsoft_utils.entra import ( + EntraDirectoryObject, + EntraGroup, + EntraPage, +) +from onyx.connectors.microsoft_utils.graph_errors import MicrosoftGraphError +from onyx.connectors.microsoft_utils.models import MicrosoftTokenInfo +from onyx.connectors.onedrive.capability_checks import ( + build_onedrive_doc_permission_sync_checks, + build_onedrive_group_sync_checks, + build_onedrive_indexing_checks, +) +from onyx.connectors.onedrive.models import ( + OneDriveDrive, + OneDriveUser, + OneDriveUserPage, +) +from onyx.connectors.onedrive.source_operations import OneDriveSourceOperations + +_CHECKS: dict[str, CapabilityCheck] = { + check.check_id: check + for check in build_onedrive_indexing_checks() + + build_onedrive_doc_permission_sync_checks() + + build_onedrive_group_sync_checks() +} +_OWNER = OneDriveUser( + id="owner-id", user_principal_name="owner@example.com", display_name="Owner" +) + + +def _gateway() -> MagicMock: + gateway: MagicMock = create_autospec(OneDriveSourceOperations, instance=True) + gateway.check_token.return_value = MicrosoftTokenInfo(app_name="Onyx") + gateway.list_users.return_value = OneDriveUserPage(users=[_OWNER]) + gateway.get_user.return_value = _OWNER + gateway.get_default_drive.return_value = OneDriveDrive(id="d", name="Documents") + gateway.get_delta_page.return_value = DriveDeltaFetchResult( + page=DriveDeltaPage.model_validate( + {"value": [{"id": "item", "name": "a.pdf", "file": {}}]} + ) + ) + gateway.list_groups.return_value = EntraPage[EntraGroup]( + items=[EntraGroup(id="g", displayName="Engineering")] + ) + gateway.list_transitive_group_members.return_value = EntraPage[ + EntraDirectoryObject + ](items=[]) + return gateway + + +def _run_all( + check_ids: list[str], gateway: MagicMock, config: dict[str, Any] | None = None +) -> dict[str, CapabilityCheckResult]: + context = CapabilityCheckContext( + source=DocumentSource.ONEDRIVE, + credential_json={}, + connector_specific_config=config if config is not None else {}, + source_operations=gateway, + ) + return { + result.check_id: result + for result in run_capability_checks( + [_CHECKS[check_id] for check_id in check_ids], context + ) + } + + +def _run( + check_id: str, gateway: MagicMock, config: dict[str, Any] | None = None +) -> CapabilityCheckResult: + return _run_all([check_id], gateway, config)[check_id] + + +def test_the_rows_have_the_design_names() -> None: + assert {check_id: check.display_name for check_id, check in _CHECKS.items()} == { + "onedrive_token_auth": "Sign in to Microsoft", + "onedrive_users": "List users", + "onedrive_drive_count": "List users", + "onedrive_configured_users": "Find selected users", + "onedrive_drive": "Find a OneDrive", + "onedrive_delta": "Read OneDrive changes", + "onedrive_item_permissions": "Read file permissions", + "onedrive_groups": "List groups", + "onedrive_group_members": "Read group members", + } + assert _CHECKS["onedrive_drive_count"].row_id == "onedrive_users" + assert _CHECKS["onedrive_users"].validates_binding + + +@pytest.mark.parametrize( + ("check_id", "summary"), + [ + ("onedrive_token_auth", "Signed in as Onyx"), + ("onedrive_users", "Tenant user list is readable"), + ("onedrive_drive_count", "Up to 1 OneDrive"), + ("onedrive_drive", "Opened the OneDrive of Owner"), + ("onedrive_delta", "Read changes in Owner's OneDrive"), + ("onedrive_item_permissions", "Read permissions of a file in a OneDrive"), + ("onedrive_groups", "Entra group list is readable"), + ("onedrive_group_members", "Read members of Engineering"), + ], +) +def test_a_passing_check_says_what_it_found(check_id: str, summary: str) -> None: + result = _run(check_id, _gateway()) + + assert result.status == CapabilityCheckStatus.PASSED, result.message + assert result.summary == summary + + +def test_a_refused_user_listing_names_the_grant() -> None: + gateway = _gateway() + gateway.list_users.side_effect = MicrosoftGraphError(403, "Authorization", "") + + result = _run("onedrive_users", gateway) + + assert result.status == CapabilityCheckStatus.FAILED + assert result.summary == "No permission to read user directory" + + +# --- Find selected users --- + + +def test_all_selected_users_found() -> None: + result = _run( + "onedrive_configured_users", + _gateway(), + {"users": ["owner@example.com", "b@example.com"]}, + ) + + assert result.summary == "All 2 selected users found" + + +def test_an_entry_that_is_not_an_email_names_it() -> None: + result = _run("onedrive_configured_users", _gateway(), {"users": ["not-an-email"]}) + + assert result.status == CapabilityCheckStatus.FAILED + assert result.summary == "Not an email address: not-an-email" + + +def test_blank_entries_say_so() -> None: + result = _run("onedrive_configured_users", _gateway(), {"users": [" "]}) + + assert result.summary == "Users holds only blank entries" + + +def test_some_missing_users_are_a_partial_find() -> None: + gateway = _gateway() + gateway.get_user.side_effect = lambda identifier: ( + _OWNER if identifier == "owner@example.com" else None + ) + + results = _run_all( + ["onedrive_users", "onedrive_configured_users", "onedrive_drive"], + gateway, + {"users": ["owner@example.com", "gone@example.com"]}, + ) + + users = results["onedrive_configured_users"] + assert users.status == CapabilityCheckStatus.INCOMPLETE + assert users.partial + assert users.summary == "User not found: gone@example.com" + assert results["onedrive_drive"].status == CapabilityCheckStatus.PASSED + + +def test_no_selected_user_found_fails_and_stops_the_drive_check() -> None: + gateway = _gateway() + gateway.get_user.return_value = None + + results = _run_all( + ["onedrive_configured_users", "onedrive_drive"], + gateway, + {"users": ["a@example.com", "b@example.com"]}, + ) + + assert results["onedrive_configured_users"].summary == "No selected user found" + assert results["onedrive_drive"].blocked_by == "onedrive_configured_users" + + +# --- Samples that test nothing --- + + +def test_no_drive_to_open_is_incomplete() -> None: + gateway = _gateway() + gateway.get_default_drive.return_value = None + + result = _run("onedrive_drive", gateway) + + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "No OneDrives found to test" + + +def test_every_drive_refused_names_the_grant() -> None: + gateway = _gateway() + gateway.get_default_drive.side_effect = MicrosoftGraphError(403, "accessDenied", "") + + result = _run("onedrive_drive", gateway) + + assert result.status == CapabilityCheckStatus.FAILED + assert result.summary == "No permission to read sampled OneDrives" + + +def test_an_empty_drive_tests_no_permission_read() -> None: + gateway = _gateway() + gateway.get_delta_page.return_value = DriveDeltaFetchResult(page=DriveDeltaPage()) + + result = _run("onedrive_item_permissions", gateway) + + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "No files found to test" + + +def test_a_group_that_went_away_is_incomplete() -> None: + gateway = _gateway() + gateway.list_transitive_group_members.side_effect = MicrosoftGraphError( + 404, "Request_ResourceNotFound", "" + ) + + result = _run("onedrive_group_members", gateway) + + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "Sample group disappeared" + + +def test_a_refused_group_listing_stops_the_member_check() -> None: + gateway = _gateway() + gateway.list_groups.side_effect = MicrosoftGraphError(403, "Authorization", "") + + results = _run_all(["onedrive_groups", "onedrive_group_members"], gateway) + + assert results["onedrive_groups"].summary == "No permission to read Entra groups" + assert results["onedrive_group_members"].blocked_by == "onedrive_groups" + + +def test_a_throttled_listing_is_rate_limited() -> None: + gateway = _gateway() + gateway.list_users.side_effect = MicrosoftGraphError(429, "TooManyRequests", "") + + result = _run("onedrive_users", gateway) + + assert result.error_kind == CheckErrorKind.RATE_LIMITED + assert result.summary == "Microsoft is throttling requests" diff --git a/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_connector.py b/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_connector.py index 81851fb0e0..15d0ea95d1 100644 --- a/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_connector.py +++ b/backend/tests/unit/onyx/connectors/onedrive/test_onedrive_connector.py @@ -27,6 +27,7 @@ from onyx.connectors.exceptions import ( ConnectorValidationError, CredentialExpiredError, CredentialInvalidError, + InconclusiveValidationError, InsufficientPermissionsError, UnexpectedValidationError, ) @@ -1208,7 +1209,7 @@ def test_onedrive_capability_user_probe_is_bounded_for_mock_pages() -> None: if check.check_id == "onedrive_drive" ) - with pytest.raises(ConnectorValidationError, match="No readable OneDrive"): + with pytest.raises(InconclusiveValidationError, match="has a OneDrive"): check.run(context) assert gateway.list_users.call_count == 20 @@ -1435,11 +1436,8 @@ def test_onedrive_permission_check_accepts_empty_drive_at_end_cursor() -> None: build_onedrive_doc_permission_sync_checks(), context ) - assert result.status == CapabilityCheckStatus.PASSED - (advisory,) = result.advisories - assert "did not read item permissions" in advisory.message - # The admin must know which drives the check samples. - assert "first 20 configured users" in advisory.message + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "No files found to test" gateway.get_delta_page.assert_called_once() gateway.list_permissions.assert_not_called() @@ -1493,7 +1491,7 @@ def test_onedrive_permission_check_bounds_empty_delta_pages() -> None: ) with pytest.raises( - ConnectorValidationError, match="No readable OneDrive item was found" + InconclusiveValidationError, match="No readable OneDrive item was found" ): build_onedrive_doc_permission_sync_checks()[0].run(context) @@ -1517,9 +1515,8 @@ def test_onedrive_hidden_group_check_does_not_require_optional_scope() -> None: ) (result,) = run_capability_checks([_group_members_check()], context) - assert result.status == CapabilityCheckStatus.PASSED - (advisory,) = result.advisories - assert "did not read transitive group members" in advisory.message + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "No readable groups found" gateway.list_transitive_group_members.assert_not_called() @@ -1534,9 +1531,8 @@ def test_onedrive_group_check_reports_tenant_without_groups() -> None: (result,) = run_capability_checks([_group_members_check()], context) - assert result.status == CapabilityCheckStatus.PASSED - (advisory,) = result.advisories - assert "did not read transitive group members" in advisory.message + assert result.status == CapabilityCheckStatus.INCOMPLETE + assert result.summary == "No readable groups found" gateway.list_groups.assert_called_once() gateway.list_transitive_group_members.assert_not_called() @@ -1631,9 +1627,9 @@ def test_onedrive_group_check_bounds_hidden_group_discovery() -> None: if check.check_id == "onedrive_group_members" ) - output = check.run(context) + with pytest.raises(InconclusiveValidationError, match="first 20 groups"): + check.run(context) - assert len(_advisories(output)) == 1 assert gateway.list_groups.call_count == 20 gateway.list_transitive_group_members.assert_not_called()