From 14af1aa1151ebe5ee8455f22b6c9ec6cd6e8597c Mon Sep 17 00:00:00 2001 From: Michael Desa Date: Wed, 20 Dec 2017 15:17:08 -0800 Subject: [PATCH] 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. --- integrations/server_test.go | 53 +++++++++++++++ server/users.go | 15 +++++ server/users_test.go | 127 ++++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+) diff --git a/integrations/server_test.go b/integrations/server_test.go index ec7ce4279..c28943324 100644 --- a/integrations/server_test.go +++ b/integrations/server_test.go @@ -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", diff --git a/server/users.go b/server/users.go index eb425d0a5..1f0b71d31 100644 --- a/server/users.go +++ b/server/users.go @@ -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 diff --git a/server/users_test.go b/server/users_test.go index fe4711bef..7641d4922 100644 --- a/server/users_test.go +++ b/server/users_test.go @@ -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{