You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/11/02 21:46:52 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #6324: Disallow affecting the "admin" Role.
ocket8888 opened a new pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324
This PR adds validation to the TO API that forbids modifying or deleting the special "admin" Role. It also adds a database migration that changes the "admin" Role's Permission set to the one specified by the original blueprint: `["ALL"]`.
Changes were also made to Traffic Portal to make it clear that the "admin" Role cannot be deleted or modified.
<hr/>
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
Make sure the provided test passes
## PR submission checklist
- [x] This PR has tests
- [x] This PR has documentation
- [x] This PR has a CHANGELOG.md entry
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
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
[GitHub] [trafficcontrol] shamrickus merged pull request #6324: Disallow affecting the "admin" Role.
Posted by GitBox <gi...@apache.org>.
shamrickus merged pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6324: Disallow affecting the "admin" Role.
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324#discussion_r742404562
##########
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:
Is this `else` needed?
##########
File path: traffic_ops/traffic_ops_golang/role/roles.go
##########
@@ -213,6 +223,13 @@ func (role *TORole) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
}
func (role *TORole) Update(h http.Header) (error, error, int) {
+ var isAdmin bool
+ if err := role.ReqInfo.Tx.Get(&isAdmin, isAdminQuery, role.ID); err != nil {
+ return nil, fmt.Errorf("checking if Role to be modified is '%s': %w", tc.AdminRoleName, err), http.StatusInternalServerError
+ } else if isAdmin {
Review comment:
Is this `else` needed?
##########
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:
Is this `else` needed?
##########
File path: traffic_ops/traffic_ops_golang/role/roles.go
##########
@@ -213,6 +223,13 @@ func (role *TORole) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
}
func (role *TORole) Update(h http.Header) (error, error, int) {
+ var isAdmin bool
+ if err := role.ReqInfo.Tx.Get(&isAdmin, isAdminQuery, role.ID); err != nil {
+ return nil, fmt.Errorf("checking if Role to be modified is '%s': %w", tc.AdminRoleName, err), http.StatusInternalServerError
+ } else if isAdmin {
Review comment:
Is this `else` needed?
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6324: Disallow affecting the "admin" Role.
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324#discussion_r742404562
##########
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:
Is this `else` needed?
##########
File path: traffic_ops/traffic_ops_golang/role/roles.go
##########
@@ -213,6 +223,13 @@ func (role *TORole) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
}
func (role *TORole) Update(h http.Header) (error, error, int) {
+ var isAdmin bool
+ if err := role.ReqInfo.Tx.Get(&isAdmin, isAdminQuery, role.ID); err != nil {
+ return nil, fmt.Errorf("checking if Role to be modified is '%s': %w", tc.AdminRoleName, err), http.StatusInternalServerError
+ } else if isAdmin {
Review comment:
Is this `else` needed?
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6324: Disallow affecting the "admin" Role.
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324#discussion_r742404562
##########
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:
Is this `else` needed?
##########
File path: traffic_ops/traffic_ops_golang/role/roles.go
##########
@@ -213,6 +223,13 @@ func (role *TORole) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
}
func (role *TORole) Update(h http.Header) (error, error, int) {
+ var isAdmin bool
+ if err := role.ReqInfo.Tx.Get(&isAdmin, isAdminQuery, role.ID); err != nil {
+ return nil, fmt.Errorf("checking if Role to be modified is '%s': %w", tc.AdminRoleName, err), http.StatusInternalServerError
+ } else if isAdmin {
Review comment:
Is this `else` needed?
--
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
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6324: Disallow affecting the "admin" Role.
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6324:
URL: https://github.com/apache/trafficcontrol/pull/6324#discussion_r747712909
##########
File path: traffic_ops/app/db/migrations/2021110207242921_admin_all_permission.up.sql
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
Review comment:
This file and the corresponding `down` migration file should be renamed so that the timestamp is after the latest refetch migration.
--
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
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6324: Disallow affecting the "admin" Role.
Posted by 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