diff --git a/organizations/users.go b/organizations/users.go index 0db6e2cd62..f4e35fcff7 100644 --- a/organizations/users.go +++ b/organizations/users.go @@ -142,6 +142,22 @@ func (s *UsersStore) Add(ctx context.Context, u *chronograf.User) (*chronograf.U // and the user that was found in the underlying store usr.Roles = append(roles, u.Roles...) + // u.SuperAdmin == true is logically equivalent to u.SuperAdmin, however + // it is more clear on a conceptual level to check equality + // + // TODO(desa): this should go away with https://github.com/influxdata/chronograf/issues/2207 + // I do not like checking super admin here. The organization users store should only be + // concerned about organizations. + // + // This allows users to be promoted to SuperAdmin on Add if they had not previously had the status. + // The reason for this is that we reuse Users and it is possible that one may try to "create" a user, + // who has super admin status, without super admin status on the request. In this case, a user should + // still retain their previous status and not be demoted. If we had done usr.SupderAdmin = u.SuperAdmin + // we could possibly demote a users super admin status. + if u.SuperAdmin == true { + usr.SuperAdmin = true + } + // Update the user in the underlying store if err := s.store.Update(ctx, usr); err != nil { return nil, err diff --git a/organizations/users_test.go b/organizations/users_test.go index 0baa40a08f..4f350adea8 100644 --- a/organizations/users_test.go +++ b/organizations/users_test.go @@ -315,9 +315,62 @@ func TestUsersStore_Add(t *testing.T) { Scheme: "oauth2", Roles: []chronograf.Role{ { - Organization: "1337", - Name: "editor", + Organization: "1336", + Name: "admin", }, + }, + }, + }, + { + name: "Add non-new user with Role. Stored user is not super admin. Provided user is super admin", + 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", + SuperAdmin: false, + Roles: []chronograf.Role{ + { + Organization: "1337", + Name: "editor", + }, + }, + }, nil + }, + }, + }, + args: args{ + ctx: context.Background(), + u: &chronograf.User{ + ID: 1234, + Name: "docbrown", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + Roles: []chronograf.Role{ + { + Organization: "1336", + Name: "admin", + }, + }, + }, + orgID: "1336", + }, + want: &chronograf.User{ + Name: "docbrown", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + Roles: []chronograf.Role{ { Organization: "1336", Name: "admin", @@ -503,6 +556,9 @@ func TestUsersStore_Add(t *testing.T) { if got == nil && tt.want == nil { continue } + if diff := cmp.Diff(got, tt.want, userCmpOptions...); diff != "" { + t.Errorf("%q. UsersStore.Add():\n-got/+want\ndiff %s", tt.name, diff) + } } } diff --git a/server/users.go b/server/users.go index ab2c13bc87..02d2d65739 100644 --- a/server/users.go +++ b/server/users.go @@ -157,6 +157,13 @@ func (s *Service) NewUser(w http.ResponseWriter, r *http.Request) { } ctx := r.Context() + + cfg, err := s.Store.Config(ctx).Get(ctx) + if err != nil { + Error(w, http.StatusInternalServerError, err.Error(), s.Logger) + return + } + user := &chronograf.User{ Name: req.Name, Provider: req.Provider, @@ -164,6 +171,10 @@ func (s *Service) NewUser(w http.ResponseWriter, r *http.Request) { Roles: req.Roles, } + if cfg.Auth.SuperAdminNewUsers { + req.SuperAdmin = true + } + if err := setSuperAdmin(ctx, req, user); err != nil { Error(w, http.StatusUnauthorized, err.Error(), s.Logger) return diff --git a/server/users_test.go b/server/users_test.go index 17155ecbec..8f9a26970e 100644 --- a/server/users_test.go +++ b/server/users_test.go @@ -112,8 +112,9 @@ func TestService_UserID(t *testing.T) { func TestService_NewUser(t *testing.T) { type fields struct { - UsersStore chronograf.UsersStore - Logger chronograf.Logger + UsersStore chronograf.UsersStore + ConfigStore chronograf.ConfigStore + Logger chronograf.Logger } type args struct { w *httptest.ResponseRecorder @@ -146,6 +147,13 @@ func TestService_NewUser(t *testing.T) { }, fields: fields{ Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: false, + }, + }, + }, UsersStore: &mocks.UsersStore{ AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { return &chronograf.User{ @@ -189,6 +197,13 @@ func TestService_NewUser(t *testing.T) { }, fields: fields{ Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: false, + }, + }, + }, UsersStore: &mocks.UsersStore{ AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { return &chronograf.User{ @@ -241,6 +256,13 @@ func TestService_NewUser(t *testing.T) { }, fields: fields{ Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: false, + }, + }, + }, UsersStore: &mocks.UsersStore{ AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { return &chronograf.User{ @@ -291,6 +313,13 @@ func TestService_NewUser(t *testing.T) { }, fields: fields{ Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: false, + }, + }, + }, UsersStore: &mocks.UsersStore{ AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { return &chronograf.User{ @@ -332,6 +361,13 @@ func TestService_NewUser(t *testing.T) { }, fields: fields{ Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: false, + }, + }, + }, UsersStore: &mocks.UsersStore{ AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { return &chronograf.User{ @@ -349,13 +385,56 @@ func TestService_NewUser(t *testing.T) { wantContentType: "application/json", wantBody: `{"id":"1338","superAdmin":true,"name":"bob","provider":"github","scheme":"oauth2","roles":[],"links":{"self":"/chronograf/v1/users/1338"}}`, }, + { + name: "Create a new User with SuperAdminNewUsers: true in ConfigStore", + args: args{ + w: httptest.NewRecorder(), + r: httptest.NewRequest( + "POST", + "http://any.url", + nil, + ), + user: &userRequest{ + Name: "bob", + Provider: "github", + Scheme: "oauth2", + }, + userKeyUser: &chronograf.User{ + ID: 0, + Name: "coolUser", + Provider: "github", + Scheme: "oauth2", + SuperAdmin: true, + }, + }, + fields: fields{ + Logger: log.New(log.DebugLevel), + ConfigStore: &mocks.ConfigStore{ + Config: &chronograf.Config{ + Auth: chronograf.AuthConfig{ + SuperAdminNewUsers: true, + }, + }, + }, + UsersStore: &mocks.UsersStore{ + AddF: func(ctx context.Context, user *chronograf.User) (*chronograf.User, error) { + user.ID = 1338 + return user, nil + }, + }, + }, + wantStatus: http.StatusCreated, + wantContentType: "application/json", + wantBody: `{"id":"1338","superAdmin":true,"name":"bob","provider":"github","scheme":"oauth2","roles":[],"links":{"self":"/chronograf/v1/users/1338"}}`, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := &Service{ Store: &mocks.Store{ - UsersStore: tt.fields.UsersStore, + UsersStore: tt.fields.UsersStore, + ConfigStore: tt.fields.ConfigStore, }, Logger: tt.fields.Logger, } diff --git a/ui/src/admin/components/chronograf/UsersTableRowNew.js b/ui/src/admin/components/chronograf/UsersTableRowNew.js index d084dd2865..7b63c3e46c 100644 --- a/ui/src/admin/components/chronograf/UsersTableRowNew.js +++ b/ui/src/admin/components/chronograf/UsersTableRowNew.js @@ -3,7 +3,6 @@ import React, {Component, PropTypes} from 'react' import Authorized, {SUPERADMIN_ROLE} from 'src/auth/Authorized' import Dropdown from 'shared/components/Dropdown' -import SlideToggle from 'shared/components/SlideToggle' import {USERS_TABLE} from 'src/admin/constants/chronografTableSizing' import {USER_ROLES} from 'src/admin/constants/chronografAdmin' @@ -17,7 +16,6 @@ class UsersTableRowNew extends Component { provider: '', scheme: 'oauth2', role: this.props.organization.defaultRole, - superAdmin: false, } } @@ -54,10 +52,6 @@ class UsersTableRowNew extends Component { this.setState({role: newRole.text}) } - handleSelectSuperAdmin = superAdmin => { - this.setState({superAdmin}) - } - handleKeyDown = e => { const {name, provider} = this.state const preventCreate = !name || !provider @@ -86,7 +80,7 @@ class UsersTableRowNew extends Component { colActions, } = USERS_TABLE const {onBlur} = this.props - const {name, provider, scheme, role, superAdmin} = this.state + const {name, provider, scheme, role} = this.state const dropdownRolesItems = USER_ROLES.map(r => ({...r, text: r.name})) const preventCreate = !name || !provider @@ -116,11 +110,7 @@ class UsersTableRowNew extends Component { - + — diff --git a/ui/src/style/pages/users.scss b/ui/src/style/pages/users.scss index 96b4800180..9878d4b7ea 100644 --- a/ui/src/style/pages/users.scss +++ b/ui/src/style/pages/users.scss @@ -104,11 +104,6 @@ table.table.chronograf-admin-table tbody tr.chronograf-admin-table--user td div. /* Styles for new user row */ table.table.chronograf-admin-table tbody tr.chronograf-admin-table--new-user { background-color: $g4-onyx; - - > td { - padding-top: 8px; - padding-bottom: 8px; - } } /* Highlight "Me" in the users table */