Prevent SuperAdmin from modifying their own status

Previously it was possible for SuperAdmins to remove their own status.
This could create an application state where there were no super admins.
This is not an acceptable application state.
pull/2639/head
Michael Desa 2017-12-20 15:17:08 -08:00
parent 466dc3d9ca
commit 14af1aa115
3 changed files with 195 additions and 0 deletions

View File

@ -1241,6 +1241,59 @@ func TestServer(t *testing.T) {
}`,
},
},
{
name: "PATCH /users/1",
subName: "SuperAdmin modifying their own status",
fields: fields{
Users: []chronograf.User{
{
ID: 1, // This is artificial, but should be reflective of the users actual ID
Name: "billibob",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: true,
Roles: []chronograf.Role{
{
Name: "admin",
Organization: "default",
},
},
},
},
},
args: args{
server: &server.Server{
GithubClientID: "not empty",
GithubClientSecret: "not empty",
},
method: "PATCH",
path: "/chronograf/v1/users/1",
payload: map[string]interface{}{
"id": "1",
"superAdmin": false,
"roles": []interface{}{
map[string]interface{}{
"name": "admin",
"organization": "default",
},
},
},
principal: oauth2.Principal{
Organization: "default",
Subject: "billibob",
Issuer: "github",
},
},
wants: wants{
statusCode: http.StatusUnauthorized,
body: `
{
"code": 401,
"message": "user cannot modify their own SuperAdmin status"
}
`,
},
},
{
name: "PUT /me",
subName: "Change SuperAdmins current organization to org they dont belong to",

View File

@ -273,6 +273,21 @@ func (s *Service) UpdateUser(w http.ResponseWriter, r *http.Request) {
return
}
// Don't allow SuperAdmins to modify their own SuperAdmin status.
// Allowing them to do so could result in an application where there
// are no super admins.
ctxUser, ok := hasUserContext(ctx)
if !ok {
Error(w, http.StatusInternalServerError, "failed to retrieve user from context", s.Logger)
return
}
// If the user being updated is the user making the request and they are
// changing their SuperAdmin status, return an unauthorized error
if ctxUser.ID == u.ID && req.SuperAdmin != u.SuperAdmin {
Error(w, http.StatusUnauthorized, "user cannot modify their own SuperAdmin status", s.Logger)
return
}
if err := setSuperAdmin(ctx, req, u); err != nil {
Error(w, http.StatusUnauthorized, err.Error(), s.Logger)
return

View File

@ -667,6 +667,13 @@ func TestService_UpdateUser(t *testing.T) {
"http://any.url",
nil,
),
userKeyUser: &chronograf.User{
ID: 0,
Name: "coolUser",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: false,
},
user: &userRequest{
ID: 1336,
Roles: []chronograf.Role{
@ -715,6 +722,13 @@ func TestService_UpdateUser(t *testing.T) {
"http://any.url",
nil,
),
userKeyUser: &chronograf.User{
ID: 0,
Name: "coolUser",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: false,
},
user: &userRequest{
ID: 1336,
Roles: []chronograf.Role{
@ -786,6 +800,119 @@ func TestService_UpdateUser(t *testing.T) {
wantContentType: "application/json",
wantBody: `{"code":422,"message":"duplicate organization \"1\" in roles"}`,
},
{
name: "SuperAdmin modifying their own SuperAdmin Status - user missing from context",
fields: fields{
Logger: log.New(log.DebugLevel),
UsersStore: &mocks.UsersStore{
UpdateF: func(ctx context.Context, user *chronograf.User) error {
return nil
},
GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) {
switch *q.ID {
case 1336:
return &chronograf.User{
ID: 1336,
Name: "bobbetta",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: true,
Roles: []chronograf.Role{
{
Name: roles.EditorRoleName,
Organization: "1",
},
},
}, nil
default:
return nil, fmt.Errorf("User with ID %d not found", *q.ID)
}
},
},
},
args: args{
w: httptest.NewRecorder(),
r: httptest.NewRequest(
"PATCH",
"http://any.url",
nil,
),
user: &userRequest{
ID: 1336,
SuperAdmin: false,
Roles: []chronograf.Role{
{
Name: roles.AdminRoleName,
Organization: "1",
},
},
},
},
id: "1336",
wantStatus: http.StatusInternalServerError,
wantContentType: "application/json",
wantBody: `{"code":500,"message":"failed to retrieve user from context"}`,
},
{
name: "SuperAdmin modifying their own SuperAdmin Status",
fields: fields{
Logger: log.New(log.DebugLevel),
UsersStore: &mocks.UsersStore{
UpdateF: func(ctx context.Context, user *chronograf.User) error {
return nil
},
GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) {
switch *q.ID {
case 1336:
return &chronograf.User{
ID: 1336,
Name: "bobbetta",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: true,
Roles: []chronograf.Role{
{
Name: roles.EditorRoleName,
Organization: "1",
},
},
}, nil
default:
return nil, fmt.Errorf("User with ID %d not found", *q.ID)
}
},
},
},
args: args{
w: httptest.NewRecorder(),
r: httptest.NewRequest(
"PATCH",
"http://any.url",
nil,
),
user: &userRequest{
ID: 1336,
SuperAdmin: false,
Roles: []chronograf.Role{
{
Name: roles.AdminRoleName,
Organization: "1",
},
},
},
userKeyUser: &chronograf.User{
ID: 1336,
Name: "coolUser",
Provider: "github",
Scheme: "oauth2",
SuperAdmin: true,
},
},
id: "1336",
wantStatus: http.StatusUnauthorized,
wantContentType: "application/json",
wantBody: `{"code":401,"message":"user cannot modify their own SuperAdmin status"}`,
},
{
name: "Update a SuperAdmin's Roles - without super admin context",
fields: fields{