Add DELETE endpoint for UserChangemakerPermission

[slifty]

This PR adds the ability to DELETE various permissions.

Related to #1351

[codecov]

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.45%. Comparing base (dea02f8) to head (203b00c).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/handlers/userChangemakerPermissionsHandlers.ts	91.66%	2 Missing ⚠️
...emakerPermissions/loadUserChangemakerPermission.ts	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1349      +/-   ##
==========================================
+ Coverage   87.34%   87.45%   +0.11%     
==========================================
  Files         185      187       +2     
  Lines        2387     2432      +45     
  Branches      314      330      +16     
==========================================
+ Hits         2085     2127      +42     
+ Misses        302      279      -23     
- Partials        0       26      +26     

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

[slifty]

I wanted to highlight this for consideration. The internet disagrees on this -- should it be 404 the second time, or 204. What does idempotent mean in this context?

It's 404 if it never existed of course.

We do happen to have a record of if something has been deleted because we aren't removing rows, so in theory we could do either one.

If 204, should it update the not_after to the most recent call, or should not_after not be touched if the permission had already expired?

[slifty]

@bickelj I wanted to highlight the above comment, since you started to explore some of the questions in it below. Specifically this question of: If 204, should it update the not_after to the most recent call, or should not_after not be touched if the permission had already expired?

[slifty]

@jasonaowen @bickelj I do think the above question is something we should answer, even without an audit log.

If someone deletes a permission that has already been deleted we should essentially ignore the second request, correct? Should it return 404, or 204?

[bickelj]

I was able to lint, build, test:ci, migrate, and start this locally. I got the expected responses when attempting to create permissions for non-existent entities. I was able to add a permission as expected and then delete it as expected. For example, an added and then deleted permission:

pdc=> select * from user_changemaker_permissions ;
        user_keycloak_user_id         | permission | changemaker_id |              created_by              |          created_at           |           not_after           
--------------------------------------+------------+----------------+--------------------------------------+-------------------------------+-------------------------------
 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | edit       |              5 | 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | 2024-12-03 11:20:21.078377-06 | 2024-12-03 11:21:15.407213-06
(1 row)

However, after re-deleting that same permission, the not_after was updated in the same row, like this:

pdc=> select * from user_changemaker_permissions ;
        user_keycloak_user_id         | permission | changemaker_id |              created_by              |          created_at           |           not_after           
--------------------------------------+------------+----------------+--------------------------------------+-------------------------------+-------------------------------
 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | edit       |              5 | 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | 2024-12-03 11:20:21.078377-06 | 2024-12-03 11:27:24.499753-06
(1 row)

And then after re-adding the same permission, the not_after was cleared:

pdc=> select * from user_changemaker_permissions ;
        user_keycloak_user_id         | permission | changemaker_id |              created_by              |          created_at           | not_after 
--------------------------------------+------------+----------------+--------------------------------------+-------------------------------+-----------
 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | edit       |              5 | 8f6e6dd9-d4af-45db-af50-712f7e962cd7 | 2024-12-03 11:20:21.078377-06 | 
(1 row)

I think the case of a repeated delete should compare now() to the existing not_after and if now() is later, do not update the not_after because that would be redundant and erase the actual expiration of the permission.

I'm less sure of what I expect for the last case (adding a previously deleted permission) but in the spirit of retaining what actually happened, as complex as it is, I think I want new non-overlapping-in-time rows.

[bickelj]

Perhaps it is too much to try to track the entire permissions history directly in the permissions table. Instead, we can allow simple insert and delete and enable postgres mod logs. We can look at the database logs if a question arises as to the history of permissions changes. Or if we wish to keep a history in-band in the database itself, we could use a history table off to the side: an audit log table.

[slifty]

Perhaps it is too much to try to track the entire permissions history directly in the permissions table.

I agree that if the goal is simply supporting an audit that the above solution would make a simple design much more complex and prone to having errors in future queries.

I think it makes more sense to have an audit log system (which we had an issue fore: #135) than to turn the permissions tables into audit logs themselves. Whether / when an audit log system becomes an engineering priority I think should be driven by use case + client + @kfogel, if that makes sense.

[bickelj]

Given what was discussed in the meeting, I think keep the not_after column until we have a better substitute because it's something rather than nothing.

[slifty]

@bickelj I just created #1355 to capture the audit log discussion.