From 7d6c896cdf987c6dbbbde2215dcf62e22d114e67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CJan-Kazlouski-elastic=E2=80=9C?= Date: Wed, 16 Sep 2026 19:30:21 +0300 Subject: [PATCH] [8.19] fix: restrict unpublished SharePoint Online pages to owners/editors (#3645) (#4437) Backported from #4437; adapted to the monolithic sharepoint_online module on 8.19. --- connectors/sources/sharepoint_online.py | 109 ++++++++-- tests/sources/test_sharepoint_online.py | 265 +++++++++++++++++++++++- 2 files changed, 351 insertions(+), 23 deletions(-) diff --git a/connectors/sources/sharepoint_online.py b/connectors/sources/sharepoint_online.py index fffd614f5..73f9b474c 100644 --- a/connectors/sources/sharepoint_online.py +++ b/connectors/sources/sharepoint_online.py @@ -103,6 +103,7 @@ def _is_excluded_sharepoint_url(url: str) -> bool: # See also: https://learn.microsoft.com/en-us/previous-versions/office/sharepoint-csom/ee536458(v=office.15) VIEW_ITEM_MASK = 0x1 # View items in lists, documents in document libraries, and Web discussion comments. VIEW_PAGE_MASK = 0x20000 # View pages in a Site. +EDIT_ITEM_MASK = 0x4 # Edit items in lists, edit documents in document libraries, and customize Web Part Pages in document libraries. # See https://github.com/pnp/pnpcore/blob/dev/src/sdk/PnP.Core/Model/SharePoint/Core/Public/Enums/RoleType.cs # See also: https://learn.microsoft.com/en-us/dotnet/api/microsoft.sharepoint.client.roletype?view=sharepoint-csom @@ -123,6 +124,13 @@ def _is_excluded_sharepoint_url(url: str) -> bool: REVIEWER, SYSTEM, ] +EDIT_ROLE_TYPES = [ + CONTRIBUTOR, + WEB_DESIGNER, + ADMINISTRATOR, + EDITOR, + SYSTEM, +] # $expand param has a max of 20: see: https://developer.microsoft.com/en-us/graph/known-issues/?search=expand SPO_MAX_EXPAND_SIZE = 20 @@ -1296,6 +1304,23 @@ def _parse_created_date_time(created_date_time): return datetime.strptime(created_date_time, TIMESTAMP_FORMAT) +def _is_page_published(version_string): + """True when the page version is a major release (e.g. "3.0").""" + if not version_string: + return True + + _, _, minor = str(version_string).partition(".") + + if not minor: + return True + + try: + return int(minor) == 0 + except ValueError: + # An unparsable minor version is no evidence of a published page + return False + + class SharepointOnlineDataSource(BaseDataSource): """Sharepoint Online""" @@ -1554,17 +1579,23 @@ async def _site_access_control(self, site): [ "user":spo-admin" ] + - list: site members with edit-or-higher access, plus site-admins + [ + "user:spo-admin", + "user:spo-editor" + ] """ self._logger.debug(f"Looking at site: {site['id']} with url {site['webUrl']}") if not self._dls_enabled(): - return [], [] + return [], [], [] def _is_site_admin(user): return user.get("IsSiteAdmin", False) access_control = set() site_admins_access_control = set() + editors_access_control = set() try: async for role_assignment in self.client.site_role_assignments( @@ -1576,6 +1607,12 @@ def _is_site_admin(user): await self._get_access_control_from_role_assignment(role_assignment) ) + editors_access_control.update( + await self._get_access_control_from_role_assignment( + role_assignment, require_edit_access=True + ) + ) + if _is_site_admin(member): # These are likely in the "Owners" group for the site site_admins_access_control |= member_access_control @@ -1600,7 +1637,13 @@ def _is_site_admin(user): ) raise PermissionsMissing(msg) from e - return list(access_control), list(site_admins_access_control) + editors_access_control |= site_admins_access_control + + return ( + list(access_control), + list(site_admins_access_control), + list(editors_access_control), + ) def _dls_enabled(self): if self._features is None: @@ -1845,6 +1888,7 @@ async def get_docs(self, filtering=None): ( site_access_control, site_admin_access_control, + site_editors_access_control, ) = await self._site_access_control(site) yield ( @@ -1916,7 +1960,9 @@ async def get_docs(self, filtering=None): yield list_item, download_func # Sync site pages - async for site_page in self.site_pages(site, site_access_control): + async for site_page in self.site_pages( + site, site_access_control, site_editors_access_control + ): # Always include site admins in site page access controls site_page = self._decorate_with_access_control( site_page, site_admin_access_control @@ -1948,6 +1994,7 @@ async def get_docs_incrementally(self, sync_cursor, filtering=None): ( site_access_control, site_admin_access_control, + site_editors_access_control, ) = await self._site_access_control(site) yield ( @@ -2029,7 +2076,10 @@ async def get_docs_incrementally(self, sync_cursor, filtering=None): # Sync site pages async for site_page in self.site_pages( - site, site_access_control, check_timestamp=True + site, + site_access_control, + site_editors_access_control, + check_timestamp=True, ): # Always include site admins in site page access controls site_page = self._decorate_with_access_control( @@ -2362,11 +2412,14 @@ async def site_lists(self, site, site_access_control, check_timestamp=False): yield site_list - async def _get_access_control_from_role_assignment(self, role_assignment): + async def _get_access_control_from_role_assignment( + self, role_assignment, require_edit_access=False + ): """Extracts access control from a role assignment. Args: role_assignment (dict): dictionary representing a role assignment. + require_edit_access (bool): when True, return only members with edit access. Returns: access_control (list): list of usernames and dynamic group ids, which have the role assigned. @@ -2375,7 +2428,7 @@ async def _get_access_control_from_role_assignment(self, role_assignment): If any role is assigned to a user this means at least "read" access. """ - def _has_limited_access(role_assignment): + def _grants_access(role_assignment): bindings = role_assignment.get("RoleDefinitionBindings", []) # If there is no permission information, default to restrict access @@ -2383,9 +2436,8 @@ def _has_limited_access(role_assignment): self._logger.debug( f"No RoleDefinitionBindings found for '{role_assignment.get('odata.id')}'" ) - return True + return False - # if any binding grants view access, this role assignment's member has view access for binding in bindings: # full explanation of the bit-math: https://stackoverflow.com/questions/51897160/how-to-parse-getusereffectivepermissions-sharepoint-response-in-java # this approach was confirmed as valid by a Microsoft Sr. Support Escalation Engineer @@ -2393,18 +2445,21 @@ def _has_limited_access(role_assignment): nested_get_from_dict(binding, ["BasePermissions", "Low"], "0") # pyright: ignore ) role_type_kind = binding.get("RoleTypeKind", 0) - if ( + if require_edit_access: + if (base_permission_low & EDIT_ITEM_MASK) or ( + role_type_kind in EDIT_ROLE_TYPES + ): + return True + elif ( (base_permission_low & VIEW_ITEM_MASK) or (base_permission_low & VIEW_PAGE_MASK) or (role_type_kind in VIEW_ROLE_TYPES) ): - return False + return True - return ( - True # no evidence of view access was found, so assuming limited access - ) + return False - if _has_limited_access(role_assignment): + if not _grants_access(role_assignment): return [] access_control = [] @@ -2429,7 +2484,13 @@ def _has_limited_access(role_assignment): return access_control - async def site_pages(self, site, site_access_control, check_timestamp=False): + async def site_pages( + self, + site, + site_access_control, + site_editors_access_control, + check_timestamp=False, + ): site_id = site["id"] url = site["webUrl"] async for site_page in self.client.site_pages(url): @@ -2446,6 +2507,13 @@ async def site_pages(self, site, site_access_control, check_timestamp=False): site_page["_id"] = f"{site_id}-site_page-{site_page['Id']}" site_page["object_type"] = "site_page" + published = _is_page_published(site_page.get("OData__UIVersionString")) + site_page["published"] = published + if not published: + self._logger.debug( + f"Unpublished site page '{site_page['_id']}'; restricting ACL to owners/editors." + ) + has_unique_role_assignments = False # ignore parent site permissions and use unique per page permissions ("unique permissions" means breaking the inheritance to the parent site) @@ -2473,7 +2541,8 @@ async def site_pages(self, site, site_access_control, check_timestamp=False): ): page_access_control.extend( await self._get_access_control_from_role_assignment( - role_assignment + role_assignment, + require_edit_access=not published, ) ) @@ -2481,10 +2550,14 @@ async def site_pages(self, site, site_access_control, check_timestamp=False): site_page, page_access_control ) - # set parent site access control if not has_unique_role_assignments: + inherited_access_control = ( + site_access_control + if published + else site_editors_access_control + ) site_page = self._decorate_with_access_control( - site_page, site_access_control + site_page, inherited_access_control ) for html_field in [ diff --git a/tests/sources/test_sharepoint_online.py b/tests/sources/test_sharepoint_online.py index ee23dfc00..1abf99935 100644 --- a/tests/sources/test_sharepoint_online.py +++ b/tests/sources/test_sharepoint_online.py @@ -47,6 +47,7 @@ ThrottledError, TokenFetchFailed, _get_login_name, + _is_page_published, _prefix_email, _prefix_group, _prefix_user, @@ -124,6 +125,18 @@ } ] +EDIT_BINDING = [ + { + "BasePermissions": {"High": "432", "Low": "1011030767"}, + "Description": "Can add, edit and delete lists; can view, add, update and delete list items and documents.", + "Hidden": False, + "Id": 1073741827, + "Name": "Edit", + "Order": 48, + "RoleTypeKind": 6, + } +] + LIMITED_ACCESS_BINDING = [ { "BasePermissions": {"High": "16", "Low": "134283264"}, @@ -2558,7 +2571,7 @@ async def test_get_docs_with_access_control(self, patch_sharepoint_client): async with create_spo_source(use_document_level_security=True) as source: source._site_access_control = AsyncMock( - return_value=(expected_access_control, []) + return_value=(expected_access_control, [], expected_access_control) ) results = [] @@ -2668,7 +2681,7 @@ async def test_get_docs_incrementally_with_empty_cursor( @freeze_time(iso_utc()) async def test_get_docs_incrementally(self, patch_sharepoint_client): async with create_spo_source() as source: - source._site_access_control = AsyncMock(return_value=([], [])) + source._site_access_control = AsyncMock(return_value=([], [], [])) # mock cache lookup source.site_group_users = AsyncMock(return_value=self.site_group_users) @@ -3189,7 +3202,7 @@ async def test_drive_items_permissions_when_fetch_drive_item_permissions_enabled async with create_spo_source(use_document_level_security=True) as source: source._site_access_control = AsyncMock( - return_value=(site_access_controls, []) + return_value=(site_access_controls, [], site_access_controls) ) results = [] @@ -3238,7 +3251,11 @@ async def test_site_page_permissions_when_fetch_drive_item_permissions_enabled( async with create_spo_source(use_document_level_security=True) as source: source._site_access_control = AsyncMock( - return_value=([], admin_site_access_controls) + return_value=( + [], + admin_site_access_controls, + admin_site_access_controls, + ) ) results = [] @@ -3702,7 +3719,7 @@ async def test_site_access_control(self, patch_sharepoint_client): site = {"id": 1, "webUrl": "some url"} - access_control, _ = await source._site_access_control(site) + access_control, _, _ = await source._site_access_control(site) two_users = 2 two_groups = 2 @@ -4243,6 +4260,244 @@ async def test_get_access_control_from_role_assignment( identity in access_control for identity in expected_access_control ) + @pytest.mark.parametrize( + "role_assignment, expected_access_control", + [ + ( + # Reader (view-only) is excluded when edit access is required + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": READ_BINDING, + }, + [], + ), + ( + # "Reader" role type (view-only) is excluded + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": [{"RoleTypeKind": 2}], + }, + [], + ), + ( + # View item mask only is excluded when edit access is required + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": [{"BasePermissions": {"Low": "1"}}], + }, + [], + ), + ( + # Editor binding grants edit access + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": EDIT_BINDING, + }, + [_prefix_user(USER_ONE_EMAIL)], + ), + ( + # "Contributor" role type grants edit access + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": [{"RoleTypeKind": 3}], + }, + [_prefix_user(USER_ONE_EMAIL)], + ), + ( + # Edit item mask only grants edit access + { + "Member": { + "odata.type": "SP.User", + "UserPrincipalName": USER_ONE_EMAIL, + }, + "RoleDefinitionBindings": [{"BasePermissions": {"Low": "4"}}], + }, + [_prefix_user(USER_ONE_EMAIL)], + ), + ], + ) + @pytest.mark.asyncio + async def test_get_access_control_from_role_assignment_require_edit_access( + self, role_assignment, expected_access_control + ): + async with create_spo_source() as source: + access_control = await source._get_access_control_from_role_assignment( + role_assignment, require_edit_access=True + ) + + assert len(access_control) == len(expected_access_control) + assert all( + identity in access_control for identity in expected_access_control + ) + + @pytest.mark.parametrize( + "version_string, expected_published", + [ + ("1.0", True), + ("3.0", True), + ("12.0", True), + ("0.1", False), + ("3.1", False), + ("2.5", False), + # Missing/undeterminable version defaults to published + (None, True), + ("", True), + ("5", True), + ("not-a-version", True), + # An unparsable minor version is not evidence of a published page + ("3.foo", False), + ("1.x", False), + ], + ) + def test_is_page_published(self, version_string, expected_published): + assert _is_page_published(version_string) == expected_published + + @pytest.mark.asyncio + @patch( + "connectors.sources.sharepoint_online.ACCESS_CONTROL", + ALLOW_ACCESS_CONTROL_PATCHED, + ) + async def test_site_pages_unpublished_page_with_unique_permissions_excludes_viewers( + self, patch_sharepoint_client + ): + viewer = { + "Member": {"odata.type": "SP.User", "UserPrincipalName": USER_ONE_EMAIL}, + "RoleDefinitionBindings": READ_BINDING, + } + editor = { + "Member": {"odata.type": "SP.User", "UserPrincipalName": USER_TWO_EMAIL}, + "RoleDefinitionBindings": EDIT_BINDING, + } + + async with create_spo_source(use_document_level_security=True) as source: + source.client.site_pages = AsyncIterator( + [ + { + "Id": "4", + "Modified": self.day_ago, + "OData__UIVersionString": "2.1", + } + ] + ) + source.client.site_page_has_unique_role_assignments = AsyncMock( + return_value=True + ) + source.client.site_page_role_assignments = AsyncIterator([viewer, editor]) + + pages = [ + page + async for page in source.site_pages( + {"id": "1", "webUrl": "http://fakesharepoint.com/site"}, + site_access_control=[], + site_editors_access_control=[], + ) + ] + + assert len(pages) == 1 + page = pages[0] + assert page["published"] is False + access_control = page[ALLOW_ACCESS_CONTROL_PATCHED] + assert _prefix_user(USER_TWO_EMAIL) in access_control + assert _prefix_user(USER_ONE_EMAIL) not in access_control + + @pytest.mark.asyncio + @patch( + "connectors.sources.sharepoint_online.ACCESS_CONTROL", + ALLOW_ACCESS_CONTROL_PATCHED, + ) + async def test_site_pages_published_page_with_unique_permissions_includes_viewers( + self, patch_sharepoint_client + ): + viewer = { + "Member": {"odata.type": "SP.User", "UserPrincipalName": USER_ONE_EMAIL}, + "RoleDefinitionBindings": READ_BINDING, + } + + async with create_spo_source(use_document_level_security=True) as source: + source.client.site_pages = AsyncIterator( + [ + { + "Id": "4", + "Modified": self.day_ago, + "OData__UIVersionString": "2.0", + } + ] + ) + source.client.site_page_has_unique_role_assignments = AsyncMock( + return_value=True + ) + source.client.site_page_role_assignments = AsyncIterator([viewer]) + + pages = [ + page + async for page in source.site_pages( + {"id": "1", "webUrl": "http://fakesharepoint.com/site"}, + site_access_control=[], + site_editors_access_control=[], + ) + ] + + assert len(pages) == 1 + page = pages[0] + assert page["published"] is True + assert _prefix_user(USER_ONE_EMAIL) in page[ALLOW_ACCESS_CONTROL_PATCHED] + + @pytest.mark.asyncio + @patch( + "connectors.sources.sharepoint_online.ACCESS_CONTROL", + ALLOW_ACCESS_CONTROL_PATCHED, + ) + async def test_site_pages_unpublished_inherited_page_uses_editors_access_control( + self, patch_sharepoint_client + ): + site_access_control = [_prefix_user("viewer")] + site_editors_access_control = [_prefix_user("editor")] + + async with create_spo_source(use_document_level_security=True) as source: + source.client.site_pages = AsyncIterator( + [ + { + "Id": "4", + "Modified": self.day_ago, + "OData__UIVersionString": "2.1", + } + ] + ) + source.client.site_page_has_unique_role_assignments = AsyncMock( + return_value=False + ) + + pages = [ + page + async for page in source.site_pages( + {"id": "1", "webUrl": "http://fakesharepoint.com/site"}, + site_access_control=site_access_control, + site_editors_access_control=site_editors_access_control, + ) + ] + + assert len(pages) == 1 + page = pages[0] + assert page["published"] is False + access_control = page[ALLOW_ACCESS_CONTROL_PATCHED] + assert _prefix_user("editor") in access_control + assert _prefix_user("viewer") not in access_control + @pytest.mark.parametrize( "raw_login_name, expected_login_name", [