Optimize permission lookups for a user #10906

stevenwinship · 2024-10-03T16:59:34Z

What this PR does / why we need it: The need to query "what dataverses does User x have Permission y on"

Which issue(s) this PR closes: #6467

Closes Optimize permission lookups for a user #6467

Special notes for your reviewer: I'm not crazy about the api being called /allowedcollections/. Any suggestions for a better name would be appreciated.

Suggestions on how to test this: See IT test. Also test a group within a group for nested group permissions. I have tested it manually and it was working. For Shib testing I tested the sql manually. This still needs to be tested in a running environment.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: No

Is there a release notes update needed for this change?: Included

Additional documentation:

coveralls · 2024-10-03T17:09:05Z

coverage: 20.856% (-0.01%) from 20.869%
when pulling be2c143 on 6467-optimize-permission-lookups-for-a-user
into a0cb73d on develop.

pdurbin

Some initial feedback. My eyes glazed over at the SQL but I'll request a review from @scolapasta for that part.

pdurbin · 2024-10-04T15:15:08Z

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

@@ -0,0 +1,9 @@
+The following API have been added:


Once we've finalized the name of the API endpoint, please add it to the API Guide.

pdurbin · 2024-10-04T15:21:24Z

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

@@ -0,0 +1,9 @@
+The following API have been added:
+
+/api/users/{identifier}/allowedcollections/{permission}


I can live with this name. I would probably camel case it as allowedCollections.

How would we extend this API to datasets or files, if we needed to? Like below?

/api/users/{identifier}/allowedDatasets/{permission}
/api/users/{identifier}/allowedFiles/{permission}

Just playing around below, maybe instead we could have...

/api/users/{identifier}/permissions/collections/{permission}
/api/users/{identifier}/permissions/datasets/{permission}
/api/users/{identifier}/permissions/files/{permission}

? You might want to get some other opinions.

I like the top APIs better. I think the allowedCollections etc. is more descriptive of what is being returned.

@qqmyers @scolapasta Any comment :)

I'm fine with either set of names.

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

pdurbin · 2024-10-04T15:22:32Z

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

+Valid Permissions are: AddDataverse, AddDataset, ViewUnpublishedDataverse, ViewUnpublishedDataset, DownloadFile, EditDataverse, EditDataset, ManageDataversePermissions,
+ManageDatasetPermissions, ManageFilePermissions, PublishDataverse, PublishDataset, DeleteDataverse, DeleteDatasetDraft, and "any" as a wildcard option.


It's fine to list these here but it might be nice to have an API to list all these permissions.

I wouldn't have bother listing them in the release note but I wanted to point out the "any" permission for a wildcard. But since this wasn't asked for maybe we just leave it out of the release notes?

Maybe, but do you think it's potentially useful to list these permissions via API? Maybe not for this PR but in the future.

The permissions should probably be listed in the guides for now. That way, the release note could link to them.

An API would be nice. There is already a way to list the permissions in a role for the UI so adding a simple list of all permissions should be a quick add.

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

pdurbin · 2024-10-04T15:30:08Z

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

+
+/api/users/{identifier}/allowedcollections/{permission}
+
+This API lists the dataverses/collections that the user has access to via the permission passed.


What about cases where the user is granted access at runtime based on their membership in Shibboleth groups or IP groups?

We have this related comment in the code:

/** * We don't expect this to support Shibboleth groups because even though * a Shibboleth user can have an API token the transient * shibIdentityProvider String on AuthenticatedUser is only set when a * SAML assertion is made at runtime via the browser. */

On a related note, that comment is above the call to this method on ServiceDocumentManagerImpl

public List<Dataverse> getDataversesUserHasPermissionOn(AuthenticatedUser user, Permission permission)

Should this method be replaced by the new one in this PR:

public List<Dataverse> findPermittedCollections(AuthenticatedUser user, int permissionBit)

?

I'll swap it out and test it

@pdurbin I added IP Group support but unfortunately ServiceDocumentManagerImpl doesn't have access to the request and therefore the ip address of the caller. Unless you know of a way to get it?

I don't see an obvious way. However, the SWORD API has never supported IP Groups and to my knowledge nobody has asked for it, so I think it's ok.

The main use case for IP Groups is read-only access, such as walking into a library and having access to data because you are on the library's IP range.

I found a way to pass the IPAddress from the request made to Sword.

src/main/java/edu/harvard/iq/dataverse/api/Users.java

Co-authored-by: Philip Durbin <[email protected]>

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java

…nagerImpl call for new call

src/main/java/edu/harvard/iq/dataverse/api/datadeposit/SWORDv2ServiceDocumentServlet.java

scolapasta

Hi, finally am managing to review - looks good overall, a few questions, easier to ask as a general comment imo:

Does the sql query deal with nested groups (I think so, with the "With" but wanted to confrim?
Does the last union (ip groups) need the exists / permissions bit clause?
Do we have any sense on performace of this query yet?

scolapasta · 2024-11-18T20:00:35Z

doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

@@ -0,0 +1,9 @@
+The following API have been added:
+
+/api/users/{identifier}/allowedcollections/{permission}


I'm fine with either set of names.

stevenwinship · 2024-11-18T21:31:03Z

Hi, finally am managing to review - looks good overall, a few questions, easier to ask as a general comment imo:

Does the sql query deal with nested groups (I think so, with the "With" but wanted to confrim?

Does the last union (ip groups) need the exists / permissions bit clause?

Do we have any sense on performace of this query yet?

Yes it handles groups within groups. I did test that
Sorry it's a bit confusing. The permission bit part is added in the"AND @IPRANGESQL" by calling findPermittedCollections
There hasn't been any formal performance testing but local tests as well as testing on the perf server were pretty good.

scolapasta · 2024-11-21T18:24:43Z

Hi, finally am managing to review - looks good overall, a few questions, easier to ask as a general comment imo:

Does the sql query deal with nested groups (I think so, with the "With" but wanted to confrim?

Does the last union (ip groups) need the exists / permissions bit clause?

Do we have any sense on performace of this query yet?

Yes it handles groups within groups. I did test that

Sorry it's a bit confusing. The permission bit part is added in the"AND @IPRANGESQL" by calling findPermittedCollections

There hasn't been any formal performance testing but local tests as well as testing on the perf server were pretty good.

OK on 1 and 3. For 2, maybe I'm still missing it - for the query part added in lines 143-152 for the ip groups, I don't see any reference to permisison bits).

stevenwinship · 2024-11-21T19:38:17Z

OK on 1 and 3. For 2, maybe I'm still missing it - for the query part added in lines 143-152 for the ip groups, I don't see any reference to permisison bits).

Ok. You are correct. there is no permission bits in IP groups. This is a bit confusing but here goes

in regular groups:
"_roleAlias": "contributor",
where contributor has permissions where each bit denotes a permission (for file download bit 4 is set)
sql: (dataverserole.permissionbits & 16 !=0)

in IP Groups the role is the specific permission so there is no need to check the bit
sql: look for 'fileDownloader' in WHERE roleassignment.assigneeidentifier IN (SELECT CONCAT('&ip/', persistedglobalgroup.persistedgroupalias) as assignee...)
{
"status": "OK",
"data": {
"assignee": "&ip/ipGroupebc7aa00",
"roleId": 2,
"_roleAlias": "fileDownloader",
"definitionPointId": 4
}
}

scolapasta · 2024-11-21T19:57:33Z

in IP Groups the role is the specific permission so there is no need to check the bit

I don't think that's accurate (or at least now how it's designed. Sure, IP groups are mostly meant to allow read access (and usually file download), BUT it could be for the role "viewer".

And technically, if you're logged in and from a certain ip address, then contributor role should work too. (as examples)

stevenwinship · 2024-11-21T21:04:12Z

I added the PERMISSIONBIT to the IpGroup sql and changed the test to test with "CURATOR" role

github-actions · 2024-11-22T19:39:05Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:6467-optimize-permission-lookups-for-a-user

ghcr.io/gdcc/configbaker:6467-optimize-permission-lookups-for-a-user

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

added api for optimized lookup for a user

cfd9da8

stevenwinship self-assigned this Oct 3, 2024

stevenwinship added Feature: Permissions Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) labels Oct 3, 2024

This comment has been minimized.

Sign in to view

add release note

7f8ed30

This comment has been minimized.

Sign in to view

removed unused include

66c112d

This comment has been minimized.

Sign in to view

optimize sql

e51cf6c

This comment has been minimized.

Sign in to view

stevenwinship removed their assignment Oct 4, 2024

pdurbin self-assigned this Oct 4, 2024

pdurbin reviewed Oct 4, 2024

View reviewed changes

pdurbin requested a review from scolapasta October 4, 2024 15:38

pdurbin assigned stevenwinship Oct 4, 2024

Update doc/release-notes/6467-optimize-permission-lookups-for-a-user.md

3b860e5

Co-authored-by: Philip Durbin <[email protected]>

This comment has been minimized.

Sign in to view

reformattting per review comments

83c01d1

github-actions bot reviewed Oct 4, 2024

View reviewed changes

src/main/java/edu/harvard/iq/dataverse/PermissionServiceBean.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

remove tabs from sql

728835e

This comment has been minimized.

Sign in to view

adding :authenticated-users to SQl and swapping out ServiceDocumentMa…

7257e7d

…nagerImpl call for new call

This comment has been minimized.

Sign in to view

fix typo

a194138

This comment has been minimized.

Sign in to view

adding IP Group permission access

423077d

qqmyers reviewed Oct 8, 2024

View reviewed changes

src/main/java/edu/harvard/iq/dataverse/api/datadeposit/SWORDv2ServiceDocumentServlet.java Outdated Show resolved Hide resolved

change get ipaddress code

be2c143

This comment has been minimized.

Sign in to view

cmbz added Size: 10 A percentage of a sprint. 7 hours. FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) and removed Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) labels Oct 9, 2024

pdurbin unassigned pdurbin and stevenwinship Oct 9, 2024

pdurbin added the Type: Bug a defect label Oct 9, 2024

scolapasta self-assigned this Oct 16, 2024

cmbz added the FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) label Oct 23, 2024

cmbz added the FY25 Sprint 10 FY25 Sprint 10 (2024-11-06 - 2024-11-20) label Nov 7, 2024

scolapasta reviewed Nov 18, 2024

View reviewed changes

Merge branch 'develop' into 6467-optimize-permission-lookups-for-a-user

6560326

This comment has been minimized.

Sign in to view

add PERMISSIONBIT to IpGroup sql

5d0d0ec

This comment has been minimized.

Sign in to view

adding shipGroup to sql

65e87ed

This comment has been minimized.

Sign in to view

fix typo

3606328

cmbz added the FY25 Sprint 12 FY25 Sprint 12 (2024-12-04 - 2024-12-18) label Dec 5, 2024

scolapasta approved these changes Dec 16, 2024

View reviewed changes

scolapasta removed their assignment Dec 16, 2024

ofahimIQSS self-assigned this Dec 18, 2024

		@@ -0,0 +1,9 @@
		The following API have been added:

		/api/users/{identifier}/allowedcollections/{permission}

		Valid Permissions are: AddDataverse, AddDataset, ViewUnpublishedDataverse, ViewUnpublishedDataset, DownloadFile, EditDataverse, EditDataset, ManageDataversePermissions,
		ManageDatasetPermissions, ManageFilePermissions, PublishDataverse, PublishDataset, DeleteDataverse, DeleteDatasetDraft, and "any" as a wildcard option.


		/api/users/{identifier}/allowedcollections/{permission}

		This API lists the dataverses/collections that the user has access to via the permission passed.

Optimize permission lookups for a user #10906

Are you sure you want to change the base?

Optimize permission lookups for a user #10906

Conversation

stevenwinship commented Oct 3, 2024 • edited Loading

coveralls commented Oct 3, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pdurbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenwinship Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

scolapasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenwinship commented Nov 18, 2024

This comment has been minimized.

scolapasta commented Nov 21, 2024

stevenwinship commented Nov 21, 2024 • edited Loading

scolapasta commented Nov 21, 2024

stevenwinship commented Nov 21, 2024

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 22, 2024

stevenwinship commented Oct 3, 2024 •

edited

Loading

coveralls commented Oct 3, 2024 •

edited

Loading

stevenwinship Oct 4, 2024 •

edited

Loading

stevenwinship commented Nov 21, 2024 •

edited

Loading