@johnclary, thanks for the issue to look into this morning regarding deleting temp records. You are correct, the issue does stem from the recent clean-up we've done in terms of user permissions. I figured that it was going to be easier to explain my thoughts that came out of the triage for this if I had a PR to reference with a possible solution in it, particularly because the changes had to be made to even test my findings.
Findings
When you first click the delete button on a temp crash record, the application sends back the mutation with the crash_id filled in correctly. That mutation is denied due to permissions, and the application then behaves in an unexpected way, and continues to offer you the delete modal, but now with no crash_id filled in. The missing ID is visible in the model as the lack of a crash number, and as you observed, missing in the subsequent mutations that are sent back.
The editor and vz-admin roles need to have permission to delete from the four main tables, crashes, units, persons & primarypersons, however, in an ideal solution, they would only have permissions to delete temporary crash records and other records associated to them. The ability to protect non-temp records was a decision we made intentionally, and this PR walks that back, technically.
Good news though; we have a temp_record field in the crashes table, that we can use to define a nuanced delete permission like this:
delete_permissions:
- role: editor
permission:
filter:
temp_record:
_eq: true
comment: "Allow for the deletion of temp records only"
- role: vz-admin
permission:
filter:
temp_record:
_eq: true
comment: "Allow for the deletion of temp records only"
So, that's great, but we don't have a field in the units, persons, primarypersons table that indicates if they are associated with a temp crash, nor should we, as that would be denormalized under our current model. We have a couple of options, at least, because we have to figure out how to open up these tables to be deleted by these user roles:
Just open the permission up, and rely on the first part of the mutation to halt the whole transaction if someone passes in a non-temp record. This isn't great because a nefarious user has permission to go on a delete frenzy using the graphql endpoint of users, persons & primarypersons, but is passable if we trust our privileged users.
:cry: Implement a custom web hook to validate if these child records belong to a temp record using this beta feature of the graphql-engine middle-ware. I'm not super stoked about this, but I think it's how we would really do it right.
This PR implements the first of these two possible options. I hope this is helpful, and please let me know what you'd like to do as next steps. Thanks!
Testing
Local
Spin up the DB, optionally grabbing a replication from production.
Apply the metadata
Create and delete a crash record
With child records
and without.
Ship list
[ ] Check migrations for any conflicts with latest migrations in master branch
[ ] Confirm Hasura role permissions for necessary access
Associated issues
This PR proposes a OK, but not fantastic, solution to https://github.com/cityofaustin/atd-data-tech/issues/16420. Details below.
Triage results
@johnclary, thanks for the issue to look into this morning regarding deleting temp records. You are correct, the issue does stem from the recent clean-up we've done in terms of user permissions. I figured that it was going to be easier to explain my thoughts that came out of the triage for this if I had a PR to reference with a possible solution in it, particularly because the changes had to be made to even test my findings.
Findings
When you first click the delete button on a temp crash record, the application sends back the mutation with the crash_id filled in correctly. That mutation is denied due to permissions, and the application then behaves in an unexpected way, and continues to offer you the delete modal, but now with no crash_id filled in. The missing ID is visible in the model as the lack of a crash number, and as you observed, missing in the subsequent mutations that are sent back.
The
editor
andvz-admin
roles need to have permission to delete from the four main tables,crashes
,units
,persons
&primarypersons
, however, in an ideal solution, they would only have permissions to delete temporary crash records and other records associated to them. The ability to protect non-temp records was a decision we made intentionally, and this PR walks that back, technically.Good news though; we have a
temp_record
field in the crashes table, that we can use to define a nuanced delete permission like this:So, that's great, but we don't have a field in the
units
,persons
,primarypersons
table that indicates if they are associated with a temp crash, nor should we, as that would be denormalized under our current model. We have a couple of options, at least, because we have to figure out how to open up these tables to be deleted by these user roles:users
,persons
&primarypersons
, but is passable if we trust our privileged users.graphql-engine
middle-ware. I'm not super stoked about this, but I think it's how we would really do it right.This PR implements the first of these two possible options. I hope this is helpful, and please let me know what you'd like to do as next steps. Thanks!
Testing
Local
Ship list