[AC-2084] Include Collection permissions for admin endpoints

[shane-melton]

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We’ve run into multiple places where we need the current user’s collection permissions after fetching them from the admin endpoints. Those responses do not include the manage, readonly, hidePasswords flags so we’ve been forced to merge the permissions from the collection sync data with the admin endpoint response multiple times.

This PR adds those permission details to those admin endpoint responses. Doing so required creating new SQL queries to fetch collections and permission information even if there is no relationship (all existing queries that included permission info automatically filter out collections that have no existing User/Group relationship).

Code changes

New CollectionAdminDetails Model

Add a new Domain model that represents collection details that are intended to be used for collection management/administrative purposes. The model includes permission details for a given user, a flag for if that user is explicitly assigned to the collection, and optional lists for User/Group access relationships.

ICollectionRepository.cs

Add additional documentation to existing methods and add the two new *WithPermissions queries. By their inherent nature, these queries do not perform our normal database level authorization checks, so care should be taken to ensure the requesting user has access to the organization. (Our existing queries quietly remove any collection resources the user does not have access to.)

• GetManyByOrganizationIdWithPermissionsAsync()
  • Fetches all of collections that belong to an organization, with permission details for a provided userId. Can also optionally include User/Group access relationships.
• GetByIdWithPermissionsAsync()
  • Fetches a single collection by Id with permission details for the provided userId. Can also optionally include User/Group access relationships.

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• If making database changes - make sure you also update Entity Framework queries and/or migrations
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[codecov]

Codecov Report

Attention: Patch coverage is 7.05521% with 303 lines in your changes are missing coverage. Please review.

Project coverage is 38.79%. Comparing base (c045739) to head (05e070b).

Files	Patch %	Lines
...tityFramework/Repositories/CollectionRepository.cs	0.00%	175 Missing ⚠️
...epositories/Queries/CollectionAdminDetailsQuery.cs	0.00%	62 Missing ⚠️
...ucture.Dapper/Repositories/CollectionRepository.cs	0.00%	53 Missing ⚠️
src/Api/Controllers/CollectionsController.cs	42.10%	9 Missing and 2 partials ⚠️
src/Api/Models/Response/CollectionResponseModel.cs	85.71%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3793      +/-   ##
==========================================
- Coverage   38.95%   38.79%   -0.17%     
==========================================
  Files        1214     1216       +2     
  Lines       58874    59169     +295     
  Branches     5632     5646      +14     
==========================================
+ Hits        22937    22955      +18     
- Misses      34882    35157     +275     
- Partials     1055     1057       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[eliykat]

Started looking at this today just to get my head around it. Will continue tomorrow.

[eliykat]

I've reviewed the SQL sprocs, models, and controller methods to make sure they're in line with our discussions and AC Team's requirements, but I'll leave the rest (mostly EF queries and test logic I think) to the Vault reviewer as codeowner.

Thank you again for tackling this!

[eliykat]

Love the tests, a couple of comments below but nothing blocking.

[eliykat]

The conversions are unnecessary:

• collectionGroup.All(c => c.ReadOnly) (equivalent of MIN, it will return false if there is any ReadOnly = false)
• collectionGroup.Any(c => c.Manage) (equivalent of MAX, it will return true if there is any Manage = true)

However I can see this is how we do it in existing queries, and that does more closely track the SQL equivalent, so I'll leave it up to you as to which you prefer.

[shane-melton]

That's a fair point. I had gone with what already existed in the other queries, as I figured there was some underlying implementation detail forcing the extra conversions (e.g. maybe one provider doesn't behave properly with Any or All).

I think I'll opt to leave it alone for now since we know it works as expected and it keeps consistent with the other queries.

[eliykat]

Boo :( If this is the case, should we just have the single implementation? I guess the Sqlite version here is arguably less performant because it doesn't use delayed execution. But in practice I'm not sure it would make a significant difference, and it avoids having divergent EF implementations for different databases. Would be interested in what @justindbaur thinks.

[shane-melton]

I'm not too attached either way (a single implementation or split). We do have a few other repository calls here that have similar behavior, so its not entirely new, if still pretty ugly.

I'm also not too familiar with the magnitude of the performance difference, but I do expect this to be called fairly frequently in the AC until we have better caching there, so its something to keep in mind.