[SPO] Add configurable drive item permissions support by timgrein · Pull Request #1259 · elastic/connectors

timgrein · 2023-07-12T12:00:20Z

Closes https://github.com/elastic/enterprise-search-team/issues/5164

This PR adds a RFC fetch_drive_item_permissions, which specifies whether drive item specific permissions should be fetched or not. Fetching drive item permissions is now also done via scrolling and not via fetching everything at once.

Using the $batch API to batch 20 permission requests for drive items into 1 reducing the number of requests drastically.

Note: We add siteUser and siteGroup for drive item permissions, but we don't fetch them during an access control sync yet. This will be part of another PR and will also be made configurable + the dependency checks will be added on the RFC side.

A separate PR adding the fetch_drive_item_permissions RFC will follow for Kibana.

Checklists

Pre-Review Checklist

this PR has a meaningful title
this PR links to all relevant github issues that it fixes or partially addresses
if there is no GH issue, please create it. Each PR should have a link to an issue
this PR has a thorough description
Covered the changes with automated tests
Tested the changes locally
Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
Considered corresponding documentation changes

This reverts commit 6c86b16.

seanstory

LGTM. Have you shared the results of this branch yet with Gustavo? May be good to get confirmation of output before declaring victory.

seanstory · 2023-07-12T14:41:50Z

                "type": "bool",
                "value": False,
            },
+            "fetch_drive_item_permissions": {


will need a corresponding kibana PR, and a corresponding docs PR. Also, CC @artem-shelkovnikov who is working on SPO ftest

Added RFC to functional test connector.json with 0719134

Just merged the PR, you can update tests/sources/fixtures/sharepoint_online/connector.json to include the field now

Also added, but I've to decrement the order also here :)

seanstory · 2023-07-12T14:43:01Z

+                "display": "toggle",
+                "label": "Fetch drive item permissions",
+                "order": 9,
+                "tooltip": "Enable this option to fetch drive item specific permissions. Note that this setting can potentially increase sync time.",


@leemthompo for review of copy (easier to do this now then have to come back and reword later)

I think the order needs to be decremented after #1255 (review) - it removes item no. 8

seanstory · 2023-07-12T14:43:43Z

            "identity": {
                "email": prefixed_mail,
                "username": prefixed_username,
+                "user_id": prefixed_user_id,


This is a good idea. 👍

wangch079 · 2023-07-13T03:54:41Z

        prefixed_mail = _prefix_email(email)
        prefixed_username = _prefix_user(username)
+        prefixed_user_id = _prefix_user_id(user.get("id"))
        id_ = email if email else username


Wondering can we use the user.get("id") here, instead of email/username?

As we may want to have cross-system identities in the near future, it's probably better to use the email as the id as this is searchable.

++. The sharepointID isn't typically easy for a customer to look up or reason about. But everyone knows their email address. This is why we decided to use email as the key (ES _id) for SPO.

wangch079 · 2023-07-13T03:55:46Z

+                "order": 9,
+                "tooltip": "Enable this option to fetch drive item specific permissions. Note that this setting can potentially increase sync time.",
+                "type": "bool",
+                "value": True,


This is expected to be True by default, and I don't see 8.9 label in the PR, is migration needed if customers upgrade from 8.9 to 8.10?

Yes, I would expect that we need one.

Good call, Chenhui. Tim, can you create and link an issue so we don't forget?

Created it last week: https://github.com/elastic/enterprise-search-team/issues/5181

Co-authored-by: Liam Thompson <32779855+leemthompo@users.noreply.github.com>

…rmissions".

…be removed

artem-shelkovnikov · 2023-07-13T13:37:46Z

@@ -606,9 +606,14 @@ async def drive_items(self, drive_id, url=None):
            yield page

    async def drive_item_permissions(self, drive_id, item_id):


So there's no way to fetch permissions in batch?

Do you think doing request batching could help here?

Just for my understanding: We would pile up to 20 drive items in memory and then fetch the permissions for these 20 at once?

Yup. Code will get ugly, but that's potentially 20x performance bump?

Let's do it, if it improves performance 🚀

Co-authored-by: Artem Shelkovnikov <lavatroublebubble@gmail.com>

Co-authored-by: Chenhui Wang <54903978+wangch079@users.noreply.github.com>

This reverts commit 67abae4.

seanstory · 2023-07-17T16:21:27Z

                async for site_drive in self.site_drives(site):
                    yield self._decorate_with_access_control(
-                        site_drive, access_control
+                        site_drive, site_access_control


Can drives not be given more granular access than their parent site?

They're treated as lists AFAIU and they can have unique permissions. Will be addressed in a separate PR 👍 Issue for tracking: https://github.com/elastic/enterprise-search-team/issues/5363

Co-authored-by: Sean Story <sean.j.story@gmail.com>

…issions' into tim/configurable-drive-item-permissions

github-actions · 2023-07-18T07:14:43Z

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 1259 --autoMerge --autoMergeMethod squash

(cherry picked from commit 4060efe)

timgrein · 2023-07-18T08:08:45Z

💚 All backports created successfully

Status	Branch	Result
✅	pre-8.10-stable

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit 4060efe)

timgrein added 7 commits June 30, 2023 09:53

Add "get_last_update_time" to MySQLClient

6c86b16

Merge remote-tracking branch 'origin/main'

0288b37

Merge remote-tracking branch 'origin/main'

ec5f52b

Merge remote-tracking branch 'origin/main'

13c604e

Merge remote-tracking branch 'origin/main'

981d5cd

Allow to fetch permissions for drive items

9dbbf37

Revert "Add "get_last_update_time" to MySQLClient"

25fae58

This reverts commit 6c86b16.

timgrein requested review from a team and seanstory July 12, 2023 12:00

github-actions Bot added auto-backport v8.10.0.0 labels Jul 12, 2023

Adjust tooltip

5f33612

timgrein changed the title ~~tim/configurable-drive-item-permissions~~ [SPO] Add configurable drive item permission support Jul 12, 2023

timgrein changed the title ~~[SPO] Add configurable drive item permission support~~ [SPO] Add configurable drive item permissions support Jul 12, 2023

Merge branch 'main' into tim/configurable-drive-item-permissions

958d652

seanstory previously approved these changes Jul 12, 2023

View reviewed changes

leemthompo reviewed Jul 12, 2023

View reviewed changes

Comment thread connectors/sources/sharepoint_online.py Outdated

wangch079 reviewed Jul 13, 2023

View reviewed changes

Update connectors/sources/sharepoint_online.py

2474a4c

Co-authored-by: Liam Thompson <32779855+leemthompo@users.noreply.github.com>

timgrein dismissed seanstory’s stale review via 2474a4c July 13, 2023 06:58

timgrein added 2 commits July 13, 2023 09:04

Move check for "fetch_drive_item_permissions" to "_with_drive_item_pe…

e3820ba

…rmissions".

Merge branch 'main' into tim/configurable-drive-item-permissions

e271089

timgrein mentioned this pull request Jul 13, 2023

[SPO] Add configurable unique page permissions support #1264

Merged

6 tasks done

timgrein added 3 commits July 13, 2023 14:06

Merge branch 'main' into tim/configurable-drive-item-permissions

035ee34

Add fetch_drive_item_permissions to functional test connector.json

0719134

Decrease "fetch_drive_item_permissions as "fetch_users_by_site" will …

876deb3

…be removed

artem-shelkovnikov reviewed Jul 13, 2023

View reviewed changes

Comment thread connectors/sources/sharepoint_online.py Outdated

artem-shelkovnikov reviewed Jul 13, 2023

View reviewed changes

timgrein and others added 2 commits July 13, 2023 15:39

Update connectors/sources/sharepoint_online.py

47fce4a

Co-authored-by: Artem Shelkovnikov <lavatroublebubble@gmail.com>

Add iterable_batches_generator utility

37a5818

timgrein and others added 5 commits July 17, 2023 14:29

Update connectors/sources/sharepoint_online.py

5a959d4

Co-authored-by: Chenhui Wang <54903978+wangch079@users.noreply.github.com>

Remove todo

c958364

Adjust log message

aed8e77

Remove return

76271e0

Adjust doc string

ca3db1c

timgrein requested review from artem-shelkovnikov, seanstory and wangch079 July 17, 2023 12:50

timgrein added 3 commits July 17, 2023 14:50

Add "get_last_update_time" to MySQLClient

67abae4

merge

19b1fd4

Revert "Add "get_last_update_time" to MySQLClient"

949733c

This reverts commit 67abae4.

seanstory previously approved these changes Jul 17, 2023

View reviewed changes

Update connectors/sources/sharepoint_online.py

e99170e

Co-authored-by: Sean Story <sean.j.story@gmail.com>

timgrein dismissed seanstory’s stale review via e99170e July 17, 2023 16:32

timgrein added 2 commits July 17, 2023 18:39

Remove payload from log message

375f354

Merge remote-tracking branch 'origin/tim/configurable-drive-item-perm…

e09bd7b

…issions' into tim/configurable-drive-item-permissions

timgrein requested a review from seanstory July 17, 2023 16:40

timgrein added 2 commits July 17, 2023 18:52

Autoformat

fb92351

Merge branch 'main' into tim/configurable-drive-item-permissions

90933d5

seanstory added the vpre-8.10-stable label Jul 17, 2023

seanstory approved these changes Jul 17, 2023

View reviewed changes

timgrein enabled auto-merge (squash) July 18, 2023 07:09

Merge branch 'main' into tim/configurable-drive-item-permissions

531658e

timgrein merged commit 4060efe into main Jul 18, 2023

timgrein deleted the tim/configurable-drive-item-permissions branch July 18, 2023 07:14

timgrein added a commit that referenced this pull request Jul 18, 2023

[SPO] Add configurable drive item permissions support (#1259)

f20674b

(cherry picked from commit 4060efe)

timgrein mentioned this pull request Jul 18, 2023

[pre-8.10-stable] [SPO] Add configurable drive item permissions support (#1259) #1287

Merged

timgrein added a commit that referenced this pull request Jul 18, 2023

[SPO] Add configurable drive item permissions support (#1259) (#1287)

0864576

(cherry picked from commit 4060efe)

		@@ -606,9 +606,14 @@ async def drive_items(self, drive_id, url=None):
		yield page

		async def drive_item_permissions(self, drive_id, item_id):

Conversation

timgrein commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes https://github.com/elastic/enterprise-search-team/issues/5164

Checklists

Pre-Review Checklist

Uh oh!

seanstory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-shelkovnikov Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgrein Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 18, 2023

💔 Failed to create backport PR(s)

Uh oh!

timgrein commented Jul 18, 2023

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timgrein commented Jul 12, 2023 •

edited

Loading

artem-shelkovnikov Jul 13, 2023 •

edited

Loading

timgrein Jul 13, 2023 •

edited

Loading