[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6324: Disallow affecting the "admin" Role.

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324#discussion_r742465425



##########
File path: traffic_ops/traffic_ops_golang/role/roles.go
##########
@@ -239,12 +256,19 @@ func (role *TORole) Update(h http.Header) (error, error, int) {
 
 func (role *TORole) Delete() (error, error, int) {
 	assignedUsers := 0
-	if err := role.ReqInfo.Tx.Get(&assignedUsers, "SELECT COUNT(id) FROM tm_user WHERE role=$1", role.ID); err != nil {
+	if err := role.ReqInfo.Tx.Get(&assignedUsers, "SELECT COUNT(id) FROM public.tm_user WHERE role=$1", role.ID); err != nil {
 		return nil, errors.New("role delete counting assigned users: " + err.Error()), http.StatusInternalServerError
 	} else if assignedUsers != 0 {
 		return fmt.Errorf("can not delete a role with %d assigned users", assignedUsers), nil, http.StatusBadRequest
 	}
 
+	var isAdmin bool
+	if err := role.ReqInfo.Tx.Get(&isAdmin, isAdminQuery, role.ID); err != nil {
+		return nil, fmt.Errorf("checking if Role to be deleted is '%s': %w", tc.AdminRoleName, err), http.StatusInternalServerError
+	} else if isAdmin {

Review comment:
       no, I don't think so. That's just habit, because `isAdmin` is assigned a value within the `if` statement I thought of it as only accessible within that scope. But that's not true.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org