From 15a1c45cf1d6e1351476233e1ce789ab6a0cbfaa Mon Sep 17 00:00:00 2001
From: Michael Desa <mjdesa@gmail.com>
Date: Mon, 4 Dec 2017 16:53:30 -0500
Subject: [PATCH] Allow admins to update a superadmins roles

---
 server/users.go      | 18 ++++++++-----
 server/users_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/server/users.go b/server/users.go
index 69e62196d..ab2c13bc8 100644
--- a/server/users.go
+++ b/server/users.go
@@ -295,18 +295,24 @@ func (s *Service) Users(w http.ResponseWriter, r *http.Request) {
 }
 
 func setSuperAdmin(ctx context.Context, req userRequest, user *chronograf.User) error {
+	// At a high level, this function checks the following
+	//   1. Is the user making the request a SuperAdmin.
+	//      If they are, allow them to make whatever changes they please.
+	//
+	//   2. Is the user making the request trying to change the SuperAdmin
+	//      status. If so, return an error.
+	//
+	//   3. If none of the above are the case, let the user make whichever
+	//      changes were requested.
+
 	// Only allow users to set SuperAdmin if they have the superadmin context
 	// TODO(desa): Refactor this https://github.com/influxdata/chronograf/issues/2207
 	if isSuperAdmin := hasSuperAdminContext(ctx); isSuperAdmin {
 		user.SuperAdmin = req.SuperAdmin
-	} else if !isSuperAdmin && (req.SuperAdmin == true) {
+	} else if !isSuperAdmin && (user.SuperAdmin != req.SuperAdmin) {
 		// If req.SuperAdmin has been set, and the request was not made with the SuperAdmin
 		// context, return error
-		//
-		// Even though req.SuperAdmin == true is logically equivalent to req.SuperAdmin it is
-		// more clear that this code should only be ran in the case that a user is trying to
-		// set the SuperAdmin field.
-		return fmt.Errorf("Cannot set SuperAdmin")
+		return fmt.Errorf("User does not have authorization required to set SuperAdmin status")
 	}
 
 	return nil
diff --git a/server/users_test.go b/server/users_test.go
index ba7045e7e..17155ecbe 100644
--- a/server/users_test.go
+++ b/server/users_test.go
@@ -305,7 +305,7 @@ func TestService_NewUser(t *testing.T) {
 			},
 			wantStatus:      http.StatusUnauthorized,
 			wantContentType: "application/json",
-			wantBody:        `{"code":401,"message":"Cannot set SuperAdmin"}`,
+			wantBody:        `{"code":401,"message":"User does not have authorization required to set SuperAdmin status"}`,
 		},
 		{
 			name: "Create a new SuperAdmin User - as superadmin",
@@ -707,6 +707,66 @@ func TestService_UpdateUser(t *testing.T) {
 			wantContentType: "application/json",
 			wantBody:        `{"code":422,"message":"duplicate organization \"1\" in roles"}`,
 		},
+		{
+			name: "Update a SuperAdmin's Roles - without super admin 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: true,
+					Roles: []chronograf.Role{
+						{
+							Name:         roles.AdminRoleName,
+							Organization: "1",
+						},
+					},
+				},
+				userKeyUser: &chronograf.User{
+					ID:         0,
+					Name:       "coolUser",
+					Provider:   "github",
+					Scheme:     "oauth2",
+					SuperAdmin: false,
+				},
+			},
+			id:              "1336",
+			wantStatus:      http.StatusOK,
+			wantContentType: "application/json",
+			wantBody:        `{"links":{"self":"/chronograf/v1/users/1336"},"id":"1336","name":"bobbetta","provider":"github","scheme":"oauth2","superAdmin":true,"roles":[{"name":"admin","organization":"1"}]}`,
+		},
 		{
 			name: "Update a Chronograf user to super admin - without super admin context",
 			fields: fields{
@@ -761,7 +821,7 @@ func TestService_UpdateUser(t *testing.T) {
 			id:              "1336",
 			wantStatus:      http.StatusUnauthorized,
 			wantContentType: "application/json",
-			wantBody:        `{"code":401,"message":"Cannot set SuperAdmin"}`,
+			wantBody:        `{"code":401,"message":"User does not have authorization required to set SuperAdmin status"}`,
 		},
 		{
 			name: "Update a Chronograf user to super admin - with super admin context",