diff --git a/bolt/organizations.go b/bolt/organizations.go index 631bbc7c4d..81dfce09cd 100644 --- a/bolt/organizations.go +++ b/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 { diff --git a/bolt/organizations_test.go b/bolt/organizations_test.go index 1f8180e4e8..9ef54c3b23 100644 --- a/bolt/organizations_test.go +++ b/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{ { diff --git a/chronograf.go b/chronograf.go index 47d95ab3b9..658ea9cb7c 100644 --- a/chronograf.go +++ b/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") ) diff --git a/organizations/users.go b/organizations/users.go index e15d9a8035..0db6e2cd62 100644 --- a/organizations/users.go +++ b/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...) diff --git a/organizations/users_test.go b/organizations/users_test.go index b6d352dc95..0baa40a08f 100644 --- a/organizations/users_test.go +++ b/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{ diff --git a/ui/src/admin/actions/chronograf.js b/ui/src/admin/actions/chronograf.js index 789e206472..d7a64731ef 100644 --- a/ui/src/admin/actions/chronograf.js +++ b/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 + ) } } diff --git a/ui/src/admin/reducers/chronograf.js b/ui/src/admin/reducers/chronograf.js index cd48710e26..34d46dcceb 100644 --- a/ui/src/admin/reducers/chronograf.js +++ b/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 ), } }