Store ephemeral user group associations

[slifty]

This PR expands our auth middleware to store a user's group associations as part of the process of establishing the authentication context.

It does not actually use those associations when populating a user's roles (This will be in a future PR).

Resolves #1483

[codecov]

Codecov Report

Attention: Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.90%. Comparing base (b96f134) to head (fe75765).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/middleware/addUserContext.ts	80.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1491      +/-   ##
==========================================
+ Coverage   86.89%   86.90%   +0.01%     
==========================================
  Files         235      238       +3     
  Lines        2931     2964      +33     
  Branches      402      407       +5     
==========================================
+ Hits         2547     2576      +29     
+ Misses        384      355      -29     
- Partials        0       33      +33     

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

[slifty]

I'm adding a test to ensure that the rows are properly inserted by the middlware

[hminsky2002]

I'm adding a test to ensure that the rows are properly inserted by the middlware

One of my comments was going to be adding some amount of tests so I will wait for your updates to give final review!

[bickelj]

Overall, as awkward as it is, I still strongly support leaving users and groups up to PDC Keycloak while managing PDC service object permissions within the PDC service.

The ephemeral table approach is a good starting point. I wonder, though, if we are going from JSON to SQL rows back to JSON again unnecessarily, and if we can do something more JSONy with postgres in the future. For example, if we ephemerally store (or not store but use) the whole validated JWT (maybe sans header, sans signature), then we have a single thing to pass to postgres queries instead of multiple things. This idea only occurred to me while reviewing here and I haven't tried writing anything that would make use of such a JWT in postgres. So it may not slay the complexity dragon but merely move it into the queries that make use of the JWT. Again, for now I think the approach here is what we should use. If it is slow to run a query or two on every request, I think we could increase the resources of the app and its database before we try calling the auth server instead (which in turn probably queries its own database and has the overhead of an HTTP request).

In any case, there are some plain old errors or typos that need to be fixed before this can be merged. See inline.

[bickelj]

I am kind of surprised that so many tests pass since not a single authenticated endpoint works on the Swagger UI. I guess that means we don't integrate our mandatory middleware into those tests. That cannot be true because we have to use auth headers to do anything in the software. Maybe we are missing a couple of tests that capture that, though I don't know how difficult that would be with supertest and jest.

[slifty]

I am kind of surprised that so many tests pass since not a single authenticated endpoint works on the Swagger UI. I guess that means we don't integrate our mandatory middleware into those tests. That cannot be true because we have to use auth headers to do anything in the software. Maybe we are missing a couple of tests that capture that, though I don't know how difficult that would be with supertest and jest.

I'm betting it's because I didn't update our tests to actually include an ephemeral organization association (and as you pointed out most of the code is broken) WHOOPS!

[bickelj]

It looks like all my concerns were addressed. Note I did not try this locally but reviewed some of the changes and noticed the very good tests added. LGTM if you are comfortable with it.