ocket8888 commented on code in PR #6802:
URL: https://github.com/apache/trafficcontrol/pull/6802#discussion_r887168008


##########
traffic_ops/app/db/create_tables.sql:
##########
@@ -143,6 +143,42 @@ LANGUAGE plpgsql;
 

Review Comment:
   It appears that the issue is that resetting the database runs the table 
creation script, followed by all migrations. Data seeding is never done, and 
the migration that adds the Permissions in question has already run so a 
following `upgrade` command won't run it.
   
   However, the state of the database at that point is perfectly valid. The 
`admin` Role exists and has the special `ALL` Permission (which actually does 
nothing, and anyway an `admin`-Role user's Permissions are never checked). Any 
existing users with equivalent Permissions to `admin` will have the Permissions 
added to keep their ability to access the API up-to-date with `admin`.
   
   If anything, I'd say the migration should specifically *not* add any 
Permissions to the special `admin` Role; it's meant to be immutable, and in 
fact the API itself will not allow adding Permissions to that Role. But 
existing migrations shouldn't be edited. Best I can think of doing is adding a 
statement to the patches script that strips `admin` of all Permissions besides 
`ALL` (and perhaps removes `ALL` from any and all other Roles that may have 
it). That seems somewhat unnecessary to me, but if you think it's a good idea 
then I'll do it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to