Merge pull request #2426 from influxdata/fix/enterprise-role-user-updates

Update enterprise users and roles to remove diffs instead of all

CHANGELOG.md

@@ -16,6 +16,7 @@
 1. [#2408](https://github.com/influxdata/chronograf/pull/2408): Fix updated Dashboard names not updating dashboard list
 1. [#2416](https://github.com/influxdata/chronograf/pull/2416): Fix default y-axis labels not displaying properly
 1. [#2423](https://github.com/influxdata/chronograf/pull/2423): Gracefully scale Template Variables Manager overlay on smaller displays
+1. [#2426](https://github.com/influxdata/chronograf/pull/2426): Fix Influx Enterprise users from deletion in race condition
 
 
 ### Features

enterprise/meta.go

@@ -23,8 +23,6 @@ type MetaClient struct {
 	client client
 }
 
-type ClientBuilder func() client
-
 // NewMetaClient represents a meta node in an Influx Enterprise cluster
 func NewMetaClient(url *url.URL) *MetaClient {
 	return &MetaClient{
@@ -118,39 +116,10 @@ func (m *MetaClient) DeleteUser(ctx context.Context, name string) error {
 	return m.Post(ctx, "/user", a, nil)
 }
 
-// RemoveAllUserPerms revokes all permissions for a user in Influx Enterprise
+// RemoveUserPerms revokes permissions for a user in Influx Enterprise
-func (m *MetaClient) RemoveAllUserPerms(ctx context.Context, name string) error {
+func (m *MetaClient) RemoveUserPerms(ctx context.Context, name string, perms Permissions) error {
-	user, err := m.User(ctx, name)
-	if err != nil {
-		return err
-	}
-
-	// No permissions to remove
-	if len(user.Permissions) == 0 {
-		return nil
-	}
-
 	a := &UserAction{
 		Action: "remove-permissions",
-		User:   user,
-	}
-	return m.Post(ctx, "/user", a, nil)
-}
-
-// SetUserPerms removes all permissions and then adds the requested perms
-func (m *MetaClient) SetUserPerms(ctx context.Context, name string, perms Permissions) error {
-	err := m.RemoveAllUserPerms(ctx, name)
-	if err != nil {
-		return err
-	}
-
-	// No permissions to add, so, user is in the right state
-	if len(perms) == 0 {
-		return nil
-	}
-
-	a := &UserAction{
-		Action: "add-permissions",
 		User: &User{
 			Name:        name,
 			Permissions: perms,
@@ -159,6 +128,38 @@ func (m *MetaClient) SetUserPerms(ctx context.Context, name string, perms Permis
 	return m.Post(ctx, "/user", a, nil)
 }
 
+// SetUserPerms removes permissions not in set and then adds the requested perms
+func (m *MetaClient) SetUserPerms(ctx context.Context, name string, perms Permissions) error {
+	user, err := m.User(ctx, name)
+	if err != nil {
+		return err
+	}
+
+	revoke, add := permissionsDifference(perms, user.Permissions)
+
+	// first, revoke all the permissions the user currently has, but,
+	// shouldn't...
+	if len(revoke) > 0 {
+		err := m.RemoveUserPerms(ctx, name, revoke)
+		if err != nil {
+			return err
+		}
+	}
+
+	// ... next, add any permissions the user should have
+	if len(add) > 0 {
+		a := &UserAction{
+			Action: "add-permissions",
+			User: &User{
+				Name:        name,
+				Permissions: add,
+			},
+		}
+		return m.Post(ctx, "/user", a, nil)
+	}
+	return nil
+}
+
 // UserRoles returns a map of users to all of their current roles
 func (m *MetaClient) UserRoles(ctx context.Context) (map[string]Roles, error) {
 	res, err := m.Roles(ctx, nil)
@@ -235,39 +236,10 @@ func (m *MetaClient) DeleteRole(ctx context.Context, name string) error {
 	return m.Post(ctx, "/role", a, nil)
 }
 
-// RemoveAllRolePerms removes all permissions from a role
+// RemoveRolePerms revokes permissions from a role
-func (m *MetaClient) RemoveAllRolePerms(ctx context.Context, name string) error {
+func (m *MetaClient) RemoveRolePerms(ctx context.Context, name string, perms Permissions) error {
-	role, err := m.Role(ctx, name)
-	if err != nil {
-		return err
-	}
-
-	// No permissions to remove
-	if len(role.Permissions) == 0 {
-		return nil
-	}
-
 	a := &RoleAction{
 		Action: "remove-permissions",
-		Role:   role,
-	}
-	return m.Post(ctx, "/role", a, nil)
-}
-
-// SetRolePerms removes all permissions and then adds the requested perms to role
-func (m *MetaClient) SetRolePerms(ctx context.Context, name string, perms Permissions) error {
-	err := m.RemoveAllRolePerms(ctx, name)
-	if err != nil {
-		return err
-	}
-
-	// No permissions to add, so, role is in the right state
-	if len(perms) == 0 {
-		return nil
-	}
-
-	a := &RoleAction{
-		Action: "add-permissions",
 		Role: &Role{
 			Name:        name,
 			Permissions: perms,
@@ -276,7 +248,39 @@ func (m *MetaClient) SetRolePerms(ctx context.Context, name string, perms Permis
 	return m.Post(ctx, "/role", a, nil)
 }
 
-// SetRoleUsers removes all users and then adds the requested users to role
+// SetRolePerms removes permissions not in set and then adds the requested perms to role
+func (m *MetaClient) SetRolePerms(ctx context.Context, name string, perms Permissions) error {
+	role, err := m.Role(ctx, name)
+	if err != nil {
+		return err
+	}
+
+	revoke, add := permissionsDifference(perms, role.Permissions)
+
+	// first, revoke all the permissions the role currently has, but,
+	// shouldn't...
+	if len(revoke) > 0 {
+		err := m.RemoveRolePerms(ctx, name, revoke)
+		if err != nil {
+			return err
+		}
+	}
+
+	// ... next, add any permissions the role should have
+	if len(add) > 0 {
+		a := &RoleAction{
+			Action: "add-permissions",
+			Role: &Role{
+				Name:        name,
+				Permissions: add,
+			},
+		}
+		return m.Post(ctx, "/role", a, nil)
+	}
+	return nil
+}
+
+// SetRoleUsers removes users not in role and then adds the requested users to role
 func (m *MetaClient) SetRoleUsers(ctx context.Context, name string, users []string) error {
 	role, err := m.Role(ctx, name)
 	if err != nil {
@@ -320,6 +324,29 @@ func Difference(wants []string, haves []string) (revoke []string, add []string)
 	return
 }
 
+func permissionsDifference(wants Permissions, haves Permissions) (revoke Permissions, add Permissions) {
+	revoke = make(Permissions)
+	add = make(Permissions)
+	for scope, want := range wants {
+		have, ok := haves[scope]
+		if ok {
+			r, a := Difference(want, have)
+			revoke[scope] = r
+			add[scope] = a
+		} else {
+			add[scope] = want
+		}
+	}
+
+	for scope, have := range haves {
+		_, ok := wants[scope]
+		if !ok {
+			revoke[scope] = have
+		}
+	}
+	return
+}
+
 // AddRoleUsers updates a role to have additional users.
 func (m *MetaClient) AddRoleUsers(ctx context.Context, name string, users []string) error {
 	// No permissions to add, so, role is in the right state

enterprise/meta_test.go

@@ -595,7 +595,7 @@ func TestMetaClient_SetUserPerms(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "Successful set permissions User",
+			name: "Remove all permissions for a user",
 			fields: fields{
 				URL: &url.URL{
 					Host:   "twinpinesmall.net:8091",
@@ -615,7 +615,7 @@ func TestMetaClient_SetUserPerms(t *testing.T) {
 			wantRm: `{"action":"remove-permissions","user":{"name":"admin","permissions":{"":["ViewAdmin","ViewChronograf"]}}}`,
 		},
 		{
-			name: "Successful set permissions User",
+			name: "Remove some permissions and add others",
 			fields: fields{
 				URL: &url.URL{
 					Host:   "twinpinesmall.net:8091",
@@ -1137,7 +1137,7 @@ func TestMetaClient_SetRolePerms(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "Successful set permissions role",
+			name: "Remove all roles from user",
 			fields: fields{
 				URL: &url.URL{
 					Host:   "twinpinesmall.net:8091",
@@ -1154,10 +1154,10 @@ func TestMetaClient_SetRolePerms(t *testing.T) {
 				ctx:  context.Background(),
 				name: "admin",
 			},
-			wantRm: `{"action":"remove-permissions","role":{"name":"admin","permissions":{"":["ViewAdmin","ViewChronograf"]},"users":["marty"]}}`,
+			wantRm: `{"action":"remove-permissions","role":{"name":"admin","permissions":{"":["ViewAdmin","ViewChronograf"]}}}`,
 		},
 		{
-			name: "Successful set single permissions role",
+			name: "Remove some users and add permissions to other",
 			fields: fields{
 				URL: &url.URL{
 					Host:   "twinpinesmall.net:8091",
@@ -1179,7 +1179,7 @@ func TestMetaClient_SetRolePerms(t *testing.T) {
 					},
 				},
 			},
-			wantRm:  `{"action":"remove-permissions","role":{"name":"admin","permissions":{"":["ViewAdmin","ViewChronograf"]},"users":["marty"]}}`,
+			wantRm:  `{"action":"remove-permissions","role":{"name":"admin","permissions":{"":["ViewAdmin","ViewChronograf"]}}}`,
 			wantAdd: `{"action":"add-permissions","role":{"name":"admin","permissions":{"telegraf":["ReadData"]}}}`,
 		},
 	}
@@ -1218,7 +1218,7 @@ func TestMetaClient_SetRolePerms(t *testing.T) {
 
 		got, _ := ioutil.ReadAll(prm.Body)
 		if string(got) != tt.wantRm {
-			t.Errorf("%q. MetaClient.SetRolePerms() = %v, want %v", tt.name, string(got), tt.wantRm)
+			t.Errorf("%q. MetaClient.SetRolePerms() removal = \n%v\n, want \n%v\n", tt.name, string(got), tt.wantRm)
 		}
 		if tt.wantAdd != "" {
 			prm := reqs[2]
@@ -1231,7 +1231,7 @@ func TestMetaClient_SetRolePerms(t *testing.T) {
 
 			got, _ := ioutil.ReadAll(prm.Body)
 			if string(got) != tt.wantAdd {
-				t.Errorf("%q. MetaClient.SetRolePerms() = %v, want %v", tt.name, string(got), tt.wantAdd)
+				t.Errorf("%q. MetaClient.SetRolePerms() addition = \n%v\n, want \n%v\n", tt.name, string(got), tt.wantAdd)
 			}
 		}
 	}

enterprise/users.go

@@ -70,44 +70,49 @@ func (c *UserStore) Update(ctx context.Context, u *chronograf.User) error {
 		return c.Ctrl.ChangePassword(ctx, u.Name, u.Passwd)
 	}
 
-	// Make a list of the roles we want this user to have:
+	if u.Roles != nil {
-	want := make([]string, len(u.Roles))
+		// Make a list of the roles we want this user to have:
-	for i, r := range u.Roles {
+		want := make([]string, len(u.Roles))
-		want[i] = r.Name
+		for i, r := range u.Roles {
-	}
+			want[i] = r.Name
+		}
 
-	// Find the list of all roles this user is currently in
+		// Find the list of all roles this user is currently in
-	userRoles, err := c.UserRoles(ctx)
+		userRoles, err := c.UserRoles(ctx)
-	if err != nil {
+		if err != nil {
-		return nil
+			return nil
-	}
+		}
-	// Make a list of the roles the user currently has
+		// Make a list of the roles the user currently has
-	roles := userRoles[u.Name]
+		roles := userRoles[u.Name]
-	have := make([]string, len(roles.Roles))
+		have := make([]string, len(roles.Roles))
-	for i, r := range roles.Roles {
+		for i, r := range roles.Roles {
-		have[i] = r.Name
+			have[i] = r.Name
-	}
+		}
 
-	// Calculate the roles the user will be removed from and the roles the user
+		// Calculate the roles the user will be removed from and the roles the user
-	// will be added to.
+		// will be added to.
-	revoke, add := Difference(want, have)
+		revoke, add := Difference(want, have)
 
-	// First, add the user to the new roles
+		// First, add the user to the new roles
-	for _, role := range add {
+		for _, role := range add {
-		if err := c.Ctrl.AddRoleUsers(ctx, role, []string{u.Name}); err != nil {
+			if err := c.Ctrl.AddRoleUsers(ctx, role, []string{u.Name}); err != nil {
-			return err
+				return err
+			}
+		}
+
+		// ... and now remove the user from an extra roles
+		for _, role := range revoke {
+			if err := c.Ctrl.RemoveRoleUsers(ctx, role, []string{u.Name}); err != nil {
+				return err
+			}
 		}
 	}
 
-	// ... and now remove the user from an extra roles
+	if u.Permissions != nil {
-	for _, role := range revoke {
+		perms := ToEnterprise(u.Permissions)
-		if err := c.Ctrl.RemoveRoleUsers(ctx, role, []string{u.Name}); err != nil {
+		return c.Ctrl.SetUserPerms(ctx, u.Name, perms)
-			return err
-		}
 	}
-
+	return nil
-	perms := ToEnterprise(u.Permissions)
-	return c.Ctrl.SetUserPerms(ctx, u.Name, perms)
 }
 
 // All is all users in influx

server/swagger.json

@@ -550,6 +550,7 @@
       "patch": {
         "tags": ["sources", "users"],
         "summary": "Update user configuration",
+        "description": "Update one parameter at a time (one of password, permissions or roles)",
         "parameters": [
           {
             "name": "id",