Merge pull request #2435 from influxdata/multitenancy_prevent_bad_add_org_user

Prevent unintended update of existing user on Add; fix duplicate User & Org creation client UX

bolt/organizations.go

@@ -87,7 +87,7 @@ func (s *OrganizationsStore) DefaultOrganization(ctx context.Context) (*chronogr
 // Add creates a new Organization in the OrganizationsStore
 func (s *OrganizationsStore) Add(ctx context.Context, o *chronograf.Organization) (*chronograf.Organization, error) {
 	if !s.nameIsUnique(ctx, o.Name) {
-		return nil, chronograf.ErrOrganizationNameTaken
+		return nil, chronograf.ErrOrganizationAlreadyExists
 	}
 	if err := s.client.db.Update(func(tx *bolt.Tx) error {
 		b := tx.Bucket(OrganizationsBucket)
@@ -276,7 +276,7 @@ func (s *OrganizationsStore) Update(ctx context.Context, o *chronograf.Organizat
 		return err
 	}
 	if o.Name != org.Name && !s.nameIsUnique(ctx, o.Name) {
-		return chronograf.ErrOrganizationNameTaken
+		return chronograf.ErrOrganizationAlreadyExists
 	}
 	return s.client.db.Update(func(tx *bolt.Tx) error {
 		if v, err := internal.MarshalOrganization(o); err != nil {

bolt/organizations_test.go

@@ -373,7 +373,7 @@ func TestOrganizationsStore_Update(t *testing.T) {
 			addFirst: true,
 		},
 		{
-			name: "Update organization name - name already taken",
+			name: "Update organization name - organization already exists",
 			fields: fields{
 				orgs: []chronograf.Organization{
 					{
@@ -552,7 +552,7 @@ func TestOrganizationsStore_Add(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "Add organization - name already taken",
+			name: "Add organization - organization already exists",
 			fields: fields{
 				orgs: []chronograf.Organization{
 					{

chronograf.go

@@ -33,7 +33,7 @@ const (
 	ErrAuthentication                  = Error("user not authenticated")
 	ErrUninitialized                   = Error("client uninitialized. Call Open() method")
 	ErrInvalidAxis                     = Error("Unexpected axis in cell. Valid axes are 'x', 'y', and 'y2'")
-	ErrOrganizationNameTaken           = Error("organization name is taken")
+	ErrOrganizationAlreadyExists       = Error("organization already exists")
 	ErrCannotDeleteDefaultOrganization = Error("cannot delete default organization")
 )
 

organizations/users.go

@@ -128,6 +128,16 @@ func (s *UsersStore) Add(ctx context.Context, u *chronograf.User) (*chronograf.U
 		}
 	}
 
+	// If the user already has a role in the organization then the user
+	// cannot be "created".
+	// This can be thought of as:
+	// (total # of roles a user has) - (# of roles not in the organization) = (# of roles in organization)
+	// if this value is greater than 1 the user cannot be "added".
+	numRolesInOrganization := len(usr.Roles) - len(roles)
+	if numRolesInOrganization > 0 {
+		return nil, chronograf.ErrUserAlreadyExists
+	}
+
 	// 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
 	usr.Roles = append(roles, u.Roles...)

organizations/users_test.go

@@ -325,6 +325,50 @@ func TestUsersStore_Add(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Add user that already exists",
+			fields: fields{
+				UsersStore: &mocks.UsersStore{
+					AddF: func(ctx context.Context, u *chronograf.User) (*chronograf.User, error) {
+						return u, nil
+					},
+					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{
+							ID:       1234,
+							Name:     "docbrown",
+							Provider: "github",
+							Scheme:   "oauth2",
+							Roles: []chronograf.Role{
+								{
+									Organization: "1337",
+									Name:         "editor",
+								},
+							},
+						}, nil
+					},
+				},
+			},
+			args: args{
+				ctx: context.Background(),
+				u: &chronograf.User{
+					ID:       1234,
+					Name:     "docbrown",
+					Provider: "github",
+					Scheme:   "oauth2",
+					Roles: []chronograf.Role{
+						{
+							Organization: "1337",
+							Name:         "admin",
+						},
+					},
+				},
+				orgID: "1337",
+			},
+			wantErr: true,
+		},
 		{
 			name: "Has invalid Role: missing Organization",
 			fields: fields{

ui/src/admin/actions/chronograf.js

@@ -1,3 +1,6 @@
+import _ from 'lodash'
+import uuid from 'node-uuid'
+
 import {
   getUsers as getUsersAJAX,
   getOrganizations as getOrganizationsAJAX,
@@ -12,6 +15,8 @@ import {
 import {publishAutoDismissingNotification} from 'shared/dispatchers'
 import {errorThrown} from 'shared/actions/errors'
 
+import {REVERT_STATE_DELAY} from 'shared/constants'
+
 // action creators
 
 // response contains `users` and `links`
@@ -109,13 +114,20 @@ export const loadOrganizationsAsync = url => async dispatch => {
 }
 
 export const createUserAsync = (url, user) => async dispatch => {
-  dispatch(addUser(user))
+  // temp uuid is added to be able to disambiguate a created user that has the
+  // same scheme, provider, and name as an existing user
+  const userWithTempID = {...user, _tempID: uuid.v4()}
+  dispatch(addUser(userWithTempID))
   try {
     const {data} = await createUserAJAX(url, user)
-    dispatch(syncUser(user, data))
+    dispatch(syncUser(userWithTempID, data))
   } catch (error) {
-    dispatch(errorThrown(error))
-    dispatch(removeUser(user))
+    const message = `${_.upperFirst(
+      _.toLower(error.data.message)
+    )}: ${user.scheme}::${user.provider}::${user.name}`
+    dispatch(errorThrown(error, message))
+    // undo optimistic update
+    setTimeout(() => dispatch(removeUser(userWithTempID)), REVERT_STATE_DELAY)
   }
 }
 
@@ -168,13 +180,23 @@ export const createOrganizationAsync = (
   url,
   organization
 ) => async dispatch => {
-  dispatch(addOrganization(organization))
+  // temp uuid is added to be able to disambiguate a created organization with
+  // the same name as an existing organization
+  const organizationWithTempID = {...organization, _tempID: uuid.v4()}
+  dispatch(addOrganization(organizationWithTempID))
   try {
     const {data} = await createOrganizationAJAX(url, organization)
     dispatch(syncOrganization(organization, data))
   } catch (error) {
-    dispatch(errorThrown(error))
-    dispatch(removeOrganization(organization))
+    const message = `${_.upperFirst(
+      _.toLower(error.data.message)
+    )}: ${organization.name}`
+    dispatch(errorThrown(error, message))
+    // undo optimistic update
+    setTimeout(
+      () => dispatch(removeOrganization(organizationWithTempID)),
+      REVERT_STATE_DELAY
+    )
   }
 }
 

ui/src/admin/reducers/chronograf.js

@@ -44,8 +44,12 @@ const adminChronograf = (state = initialState, action) => {
       const {user} = action.payload
       return {
         ...state,
-        // stale user does not have links, so uniqueness is on name, provider, & scheme
-        users: state.users.filter(u => !isSameUser(u, user)),
+        // stale user does not necessarily have links, so uniqueness is on name,
+        // provider, & scheme, except for a created users that is a duplicate
+        // of an existing user, in which case a temp uuid is used to match
+        users: state.users.filter(
+          u => (user._tempID ? u._tempID !== user._tempID : u.id !== user.id)
+        ),
       }
     }
 
@@ -80,7 +84,10 @@ const adminChronograf = (state = initialState, action) => {
       return {
         ...state,
         organizations: state.organizations.filter(
-          o => o.name !== organization.name
+          o =>
+            organization._tempID
+              ? o._tempID !== organization._tempID
+              : o.id !== organization.id
         ),
       }
     }