IQSS / dataverse

Open source research data repository software
http://dataverse.org
Other
885 stars 495 forks source link

Optimize permission lookups for a user #10906

Open stevenwinship opened 1 month ago

stevenwinship commented 1 month ago

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: https://github.com/IQSS/dataverse/issues/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 commented 1 month ago

Coverage Status

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

github-actions[bot] commented 1 month ago

:package: 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

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

stevenwinship commented 1 week ago

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

  1. Does the sql query deal with nested groups (I think so, with the "With" but wanted to confrim?
  2. Does the last union (ip groups) need the exists / permissions bit clause?
  3. Do we have any sense on performace of this query yet?
  1. Yes it handles groups within groups. I did test that
  2. Sorry it's a bit confusing. The permission bit part is added in the"AND @IPRANGESQL" by calling findPermittedCollections
  3. There hasn't been any formal performance testing but local tests as well as testing on the perf server were pretty good.
github-actions[bot] commented 1 week ago

:package: 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

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

scolapasta commented 1 week ago

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

  1. Does the sql query deal with nested groups (I think so, with the "With" but wanted to confrim?
  2. Does the last union (ip groups) need the exists / permissions bit clause?
  3. Do we have any sense on performace of this query yet?
  1. Yes it handles groups within groups. I did test that
  2. Sorry it's a bit confusing. The permission bit part is added in the"AND @IPRANGESQL" by calling findPermittedCollections
  3. 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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

github-actions[bot] commented 1 week ago

:package: 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

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

github-actions[bot] commented 1 week ago

:package: 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

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

github-actions[bot] commented 6 days ago

:package: 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

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