From 255992f5ecd76b83827d4caf8d2561846d153cc3 Mon Sep 17 00:00:00 2001 From: Edd Robinson Date: Fri, 17 Mar 2017 18:30:24 +0000 Subject: [PATCH] Reduce cost of admin user check This commits adds a caching mechanism to the Data object, such that when large numbers of users exist in the system, the cost of determining if there is at least one admin user will be low. To ensure that previously marshalled Data objects contain the correct cached admin user value, we exhaustively determine if there is an admin user present whenever we unmarshal a Data object. --- CHANGELOG.md | 1 + services/httpd/handler.go | 16 +------ services/httpd/handler_test.go | 36 ++++++---------- services/meta/client.go | 8 +--- services/meta/data.go | 39 +++++++++++++++++ services/meta/data_test.go | 78 ++++++++++++++++++++++++++++++++++ 6 files changed, 135 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57034a30df..7489d50921 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ - [#7995](https://github.com/influxdata/influxdb/issues/7995): Update liner dependency to handle docker exec. - [#7811](https://github.com/influxdata/influxdb/issues/7811): Kill query not killing query - [#7457](https://github.com/influxdata/influxdb/issues/7457): KILL QUERY should work during all phases of a query +- [#8155](https://github.com/influxdata/influxdb/pull/8155): Simplify admin user check. ## v1.2.2 [2017-03-14] diff --git a/services/httpd/handler.go b/services/httpd/handler.go index 4a4a3fc0e2..1dfaf6cd10 100644 --- a/services/httpd/handler.go +++ b/services/httpd/handler.go @@ -72,8 +72,8 @@ type Handler struct { MetaClient interface { Database(name string) *meta.DatabaseInfo Authenticate(username, password string) (ui *meta.UserInfo, err error) - Users() []meta.UserInfo User(username string) (*meta.UserInfo, error) + AdminUserExists() bool } QueryAuthorizer interface { @@ -964,20 +964,8 @@ func authenticate(inner func(http.ResponseWriter, *http.Request, *meta.UserInfo) } var user *meta.UserInfo - // Retrieve user list. - uis := h.MetaClient.Users() - - // See if admin user exists. - adminExists := false - for i := range uis { - if uis[i].Admin { - adminExists = true - break - } - } - // TODO corylanou: never allow this in the future without users - if requireAuthentication && adminExists { + if requireAuthentication && h.MetaClient.AdminUserExists() { creds, err := parseCredentials(r) if err != nil { atomic.AddInt64(&h.stats.AuthenticationFailures, 1) diff --git a/services/httpd/handler_test.go b/services/httpd/handler_test.go index a0f411b5c2..3b4820240e 100644 --- a/services/httpd/handler_test.go +++ b/services/httpd/handler_test.go @@ -88,16 +88,7 @@ func TestHandler_Query_Auth(t *testing.T) { h := NewHandler(true) // Set mock meta client functions for the handler to use. - h.MetaClient.UsersFn = func() []meta.UserInfo { - return []meta.UserInfo{ - { - Name: "user1", - Hash: "abcd", - Admin: true, - Privileges: make(map[string]influxql.Privilege), - }, - } - } + h.MetaClient.AdminUserExistsFn = func() bool { return true } h.MetaClient.UserFn = func(username string) (*meta.UserInfo, error) { if username != "user1" { @@ -355,8 +346,10 @@ func TestHandler_Query_ErrAuthorize(t *testing.T) { h.QueryAuthorizer.AuthorizeQueryFn = func(u *meta.UserInfo, q *influxql.Query, db string) error { return errors.New("marker") } - h.MetaClient.UsersFn = func() []meta.UserInfo { - return []meta.UserInfo{ + h.MetaClient.AdminUserExistsFn = func() bool { return true } + h.MetaClient.AuthenticateFn = func(u, p string) (*meta.UserInfo, error) { + + users := []meta.UserInfo{ { Name: "admin", Hash: "admin", @@ -370,9 +363,8 @@ func TestHandler_Query_ErrAuthorize(t *testing.T) { }, }, } - } - h.MetaClient.AuthenticateFn = func(u, p string) (*meta.UserInfo, error) { - for _, user := range h.MetaClient.Users() { + + for _, user := range users { if u == user.Name { if p == user.Hash { return &user, nil @@ -637,11 +629,11 @@ func NewHandler(requireAuthentication bool) *Handler { // HandlerMetaStore is a mock implementation of Handler.MetaClient. type HandlerMetaStore struct { - PingFn func(d time.Duration) error - DatabaseFn func(name string) *meta.DatabaseInfo - AuthenticateFn func(username, password string) (ui *meta.UserInfo, err error) - UsersFn func() []meta.UserInfo - UserFn func(username string) (*meta.UserInfo, error) + PingFn func(d time.Duration) error + DatabaseFn func(name string) *meta.DatabaseInfo + AuthenticateFn func(username, password string) (ui *meta.UserInfo, err error) + UserFn func(username string) (*meta.UserInfo, error) + AdminUserExistsFn func() bool } func (s *HandlerMetaStore) Ping(b bool) error { @@ -660,8 +652,8 @@ func (s *HandlerMetaStore) Authenticate(username, password string) (ui *meta.Use return s.AuthenticateFn(username, password) } -func (s *HandlerMetaStore) Users() []meta.UserInfo { - return s.UsersFn() +func (s *HandlerMetaStore) AdminUserExists() bool { + return s.AdminUserExistsFn() } func (s *HandlerMetaStore) User(username string) (*meta.UserInfo, error) { diff --git a/services/meta/client.go b/services/meta/client.go index 93ee6125a2..e8e660fffc 100644 --- a/services/meta/client.go +++ b/services/meta/client.go @@ -547,13 +547,7 @@ func (c *Client) UserPrivilege(username, database string) (*influxql.Privilege, func (c *Client) AdminUserExists() bool { c.mu.RLock() defer c.mu.RUnlock() - - for _, u := range c.cacheData.Users { - if u.Admin { - return true - } - } - return false + return c.cacheData.AdminUserExists() } // Authenticate returns a UserInfo if the username and password match an existing entry. diff --git a/services/meta/data.go b/services/meta/data.go index 4b8ce89cf5..32bc72c196 100644 --- a/services/meta/data.go +++ b/services/meta/data.go @@ -42,6 +42,10 @@ type Data struct { Databases []DatabaseInfo Users []UserInfo + // adminUserExists provides a constant time mechanism for determining + // if there is at least one admin user. + adminUserExists bool + MaxShardGroupID uint64 MaxShardID uint64 } @@ -568,6 +572,11 @@ func (data *Data) CreateUser(name, hash string, admin bool) error { Admin: admin, }) + // We know there is now at least one admin user. + if admin { + data.adminUserExists = true + } + return nil } @@ -575,10 +584,17 @@ func (data *Data) CreateUser(name, hash string, admin bool) error { func (data *Data) DropUser(name string) error { for i := range data.Users { if data.Users[i].Name == name { + wasAdmin := data.Users[i].Admin data.Users = append(data.Users[:i], data.Users[i+1:]...) + + // Maybe we dropped the only admin user? + if wasAdmin { + data.adminUserExists = data.hasAdminUser() + } return nil } } + return ErrUserNotFound } @@ -630,9 +646,17 @@ func (data *Data) SetAdminPrivilege(name string, admin bool) error { ui.Admin = admin + // We could have promoted or revoked the only admin. Check if an admin + // user exists. + data.adminUserExists = data.hasAdminUser() return nil } +// AdminUserExists returns true if an admin user exists. +func (data Data) AdminUserExists() bool { + return data.adminUserExists +} + // UserPrivileges gets the privileges for a user. func (data *Data) UserPrivileges(name string) (map[string]influxql.Privilege, error) { ui := data.User(name) @@ -714,6 +738,10 @@ func (data *Data) unmarshal(pb *internal.Data) { for i, x := range pb.GetUsers() { data.Users[i].unmarshal(x) } + + // Exhaustively determine if there is an admin user. The marshalled cache + // value may not be correct. + data.adminUserExists = data.hasAdminUser() } // MarshalBinary encodes the metadata to a binary format. @@ -731,6 +759,17 @@ func (data *Data) UnmarshalBinary(buf []byte) error { return nil } +// hasAdminUser exhaustively checks for the presence of at least one admin +// user. +func (data *Data) hasAdminUser() bool { + for _, u := range data.Users { + if u.Admin { + return true + } + } + return false +} + // NodeInfo represents information about a single node in the cluster. type NodeInfo struct { ID uint64 diff --git a/services/meta/data_test.go b/services/meta/data_test.go index 044970849f..ca23bde216 100644 --- a/services/meta/data_test.go +++ b/services/meta/data_test.go @@ -110,6 +110,84 @@ func Test_Data_CreateRetentionPolicy(t *testing.T) { } } +func TestData_AdminUserExists(t *testing.T) { + data := meta.Data{} + + // No users means no admin. + if data.AdminUserExists() { + t.Fatal("no admin user should exist") + } + + // Add a non-admin user. + if err := data.CreateUser("user1", "a", false); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), false; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Add an admin user. + if err := data.CreateUser("admin1", "a", true); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), true; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Remove the original user + if err := data.DropUser("user1"); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), true; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Add another admin + if err := data.CreateUser("admin2", "a", true); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), true; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Revoke privileges of the first admin + if err := data.SetAdminPrivilege("admin1", false); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), true; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Add user1 back. + if err := data.CreateUser("user1", "a", false); err != nil { + t.Fatal(err) + } + // Revoke remaining admin. + if err := data.SetAdminPrivilege("admin2", false); err != nil { + t.Fatal(err) + } + // No longer any admins + if got, exp := data.AdminUserExists(), false; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Make user1 an admin + if err := data.SetAdminPrivilege("user1", true); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), true; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } + + // Drop user1... + if err := data.DropUser("user1"); err != nil { + t.Fatal(err) + } + if got, exp := data.AdminUserExists(), false; got != exp { + t.Fatalf("got %v, expected %v", got, exp) + } +} + func TestUserInfo_AuthorizeDatabase(t *testing.T) { emptyUser := &meta.UserInfo{} if !emptyUser.AuthorizeDatabase(influxql.NoPrivileges, "anydb") {