Optimize permission lookups for a user

[stevenwinship]

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.

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]

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]

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

[pdurbin]

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.

[stevenwinship]

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

@qqmyers @scolapasta Any comment :)

[scolapasta]

I'm fine with either set of names.

[pdurbin]

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

[stevenwinship]

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?

[pdurbin]

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.

[stevenwinship]

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.

[pdurbin]

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)

?

[stevenwinship]

I'll swap it out and test it

[stevenwinship]

@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?

[pdurbin]

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.

[stevenwinship]

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

[github-actions]

📦 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.

[scolapasta]

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?

[scolapasta]

I'm fine with either set of names.

[stevenwinship]

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.