From d854d6f8a48c8aadc6172f128a427a0c9666625a Mon Sep 17 00:00:00 2001
From: Michael Desa <mjdesa@gmail.com>
Date: Fri, 10 Nov 2017 19:06:38 -0500
Subject: [PATCH] Fix Update method in organization.UsersStore

Previously, when users were updated, we did not use the fields on the
user that was being persisted. As a result, the API responded with the
attribute being set, but the updated values were not actually stored in
bolt. This was not noticed previously because SuperAdmins had raw access
to the UsersStore
---
 organizations/users.go      |  8 +++--
 organizations/users_test.go | 62 ++++++++++++++++++++++++++++++++++---
 2 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/organizations/users.go b/organizations/users.go
index b43dbbf79..8bf8732a7 100644
--- a/organizations/users.go
+++ b/organizations/users.go
@@ -195,11 +195,15 @@ func (s *UsersStore) Update(ctx context.Context, usr *chronograf.User) error {
 		}
 	}
 
+	// Make a copy of the usr so that we dont modify the underlying add roles on to
+	// the user that was passed in
+	user := *usr
+
 	// Set the users roles to be the union of the roles set on the provided user
 	// and the user that was found in the underlying store
-	u.Roles = append(roles, usr.Roles...)
+	user.Roles = append(roles, usr.Roles...)
 
-	return s.store.Update(ctx, u)
+	return s.store.Update(ctx, &user)
 }
 
 // All returns all users where roles have been filters to be exclusively for
diff --git a/organizations/users_test.go b/organizations/users_test.go
index 4953110c7..94edf62b6 100644
--- a/organizations/users_test.go
+++ b/organizations/users_test.go
@@ -561,10 +561,11 @@ func TestUsersStore_Update(t *testing.T) {
 		UsersStore chronograf.UsersStore
 	}
 	type args struct {
-		ctx   context.Context
-		usr   *chronograf.User
-		roles []chronograf.Role
-		orgID string
+		ctx        context.Context
+		usr        *chronograf.User
+		roles      []chronograf.Role
+		superAdmin bool
+		orgID      string
 	}
 	tests := []struct {
 		name    string
@@ -646,6 +647,51 @@ func TestUsersStore_Update(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Update user super admin",
+			fields: fields{
+				UsersStore: &mocks.UsersStore{
+					UpdateF: func(ctx context.Context, u *chronograf.User) error {
+						return nil
+					},
+					GetF: func(ctx context.Context, q chronograf.UserQuery) (*chronograf.User, error) {
+						return &chronograf.User{
+							Name:       "bobetta",
+							Provider:   "github",
+							Scheme:     "oauth2",
+							SuperAdmin: false,
+							Roles: []chronograf.Role{
+								{
+									Organization: "1337",
+									Name:         "viewer",
+								},
+								{
+									Organization: "1338",
+									Name:         "editor",
+								},
+							},
+						}, nil
+					},
+				},
+			},
+			args: args{
+				ctx: context.Background(),
+				usr: &chronograf.User{
+					Name:     "bobetta",
+					Provider: "github",
+					Scheme:   "oauth2",
+					Roles:    []chronograf.Role{},
+				},
+				superAdmin: true,
+				orgID:      "1338",
+			},
+			want: &chronograf.User{
+				Name:       "bobetta",
+				Provider:   "github",
+				Scheme:     "oauth2",
+				SuperAdmin: true,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt.args.ctx = context.WithValue(tt.args.ctx, organizations.ContextKey, tt.args.orgID)
@@ -655,6 +701,10 @@ func TestUsersStore_Update(t *testing.T) {
 			tt.args.usr.Roles = tt.args.roles
 		}
 
+		if tt.args.superAdmin {
+			tt.args.usr.SuperAdmin = tt.args.superAdmin
+		}
+
 		if err := s.Update(tt.args.ctx, tt.args.usr); (err != nil) != tt.wantErr {
 			t.Errorf("%q. UsersStore.Update() error = %v, wantErr %v", tt.name, err, tt.wantErr)
 		}
@@ -664,6 +714,10 @@ func TestUsersStore_Update(t *testing.T) {
 			continue
 		}
 
+		if diff := cmp.Diff(tt.args.usr, tt.want, userCmpOptions...); diff != "" {
+			t.Errorf("%q. UsersStore.Update():\n-got/+want\ndiff %s", tt.name, diff)
+		}
+
 	}
 }