From 846b9327a54aa41e40b3a5b0a1c19ccda315e310 Mon Sep 17 00:00:00 2001 From: Samuel Date: Fri, 3 May 2019 22:53:07 +1000 Subject: [PATCH] LDAP: Added reload endpoint for LDAP config (#15470) * 4843 - Added reload endpoint for LDAP config closes #4843 * Refactor to make the reload work after master drifted --- pkg/api/admin_ldap.go | 17 +++++ pkg/api/api.go | 1 + pkg/login/ldap_login.go | 10 ++- pkg/login/ldap_login_test.go | 10 +-- pkg/middleware/auth_proxy/auth_proxy.go | 9 ++- pkg/middleware/auth_proxy/auth_proxy_test.go | 12 ++-- pkg/plugins/plugins.go | 2 +- pkg/services/ldap/settings.go | 66 ++++++++++++++------ 8 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 pkg/api/admin_ldap.go diff --git a/pkg/api/admin_ldap.go b/pkg/api/admin_ldap.go new file mode 100644 index 00000000000..6e1b40bfa3e --- /dev/null +++ b/pkg/api/admin_ldap.go @@ -0,0 +1,17 @@ +package api + +import ( + "github.com/grafana/grafana/pkg/services/ldap" +) + +func (server *HTTPServer) ReloadLdapCfg() Response { + if !ldap.IsEnabled() { + return Error(400, "LDAP is not enabled", nil) + } + + err := ldap.ReloadConfig() + if err != nil { + return Error(500, "Failed to reload ldap config.", err) + } + return Success("Ldap config reloaded") +} diff --git a/pkg/api/api.go b/pkg/api/api.go index e095986a229..9b5aae105cb 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -393,6 +393,7 @@ func (hs *HTTPServer) registerRoutes() { adminRoute.Post("/provisioning/dashboards/reload", Wrap(hs.AdminProvisioningReloadDasboards)) adminRoute.Post("/provisioning/datasources/reload", Wrap(hs.AdminProvisioningReloadDatasources)) adminRoute.Post("/provisioning/notifications/reload", Wrap(hs.AdminProvisioningReloadNotifications)) + adminRoute.Post("/ldap/reload", Wrap(hs.ReloadLdapCfg)) }, reqGrafanaAdmin) // rendering diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index b8811158200..abd861cbe6d 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -3,12 +3,15 @@ package login import ( "github.com/grafana/grafana/pkg/models" LDAP "github.com/grafana/grafana/pkg/services/ldap" + "github.com/grafana/grafana/pkg/util/errutil" ) var newLDAP = LDAP.New -var readLDAPConfig = LDAP.ReadConfig +var getLDAPConfig = LDAP.GetConfig var isLDAPEnabled = LDAP.IsEnabled +// loginUsingLdap logs in user using LDAP. It returns whether LDAP is enabled and optional error and query arg will be +// populated with the logged in user if successful. var loginUsingLdap = func(query *models.LoginUserQuery) (bool, error) { enabled := isLDAPEnabled() @@ -16,7 +19,10 @@ var loginUsingLdap = func(query *models.LoginUserQuery) (bool, error) { return false, nil } - config := readLDAPConfig() + config, err := getLDAPConfig() + if err != nil { + return true, errutil.Wrap("Failed to get LDAP config", err) + } if len(config.Servers) == 0 { return true, ErrNoLDAPServers } diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index c6bd30834a9..3ea82d0a8ed 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -20,12 +20,12 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - readLDAPConfig = func() *LDAP.Config { + getLDAPConfig = func() (*LDAP.Config, error) { config := &LDAP.Config{ Servers: []*LDAP.ServerConfig{}, } - return config + return config, nil } enabled, err := loginUsingLdap(sc.loginUserQuery) @@ -129,7 +129,7 @@ func ldapLoginScenario(desc string, fn ldapLoginScenarioFunc) { ldapAuthenticatorMock: mock, } - readLDAPConfig = func() *LDAP.Config { + getLDAPConfig = func() (*LDAP.Config, error) { config := &LDAP.Config{ Servers: []*LDAP.ServerConfig{ { @@ -138,7 +138,7 @@ func ldapLoginScenario(desc string, fn ldapLoginScenarioFunc) { }, } - return config + return config, nil } newLDAP = func(server *LDAP.ServerConfig) LDAP.IAuth { @@ -147,7 +147,7 @@ func ldapLoginScenario(desc string, fn ldapLoginScenarioFunc) { defer func() { newLDAP = LDAP.New - readLDAPConfig = LDAP.ReadConfig + getLDAPConfig = LDAP.GetConfig }() fn(sc) diff --git a/pkg/middleware/auth_proxy/auth_proxy.go b/pkg/middleware/auth_proxy/auth_proxy.go index 7eee806c082..b9e71d1b480 100644 --- a/pkg/middleware/auth_proxy/auth_proxy.go +++ b/pkg/middleware/auth_proxy/auth_proxy.go @@ -22,8 +22,8 @@ const ( ) var ( - readLDAPConfig = ldap.ReadConfig - isLDAPEnabled = ldap.IsEnabled + getLDAPConfig = ldap.GetConfig + isLDAPEnabled = ldap.IsEnabled ) // AuthProxy struct @@ -219,7 +219,10 @@ func (auth *AuthProxy) GetUserIDViaLDAP() (int64, *Error) { Username: auth.header, } - config := readLDAPConfig() + config, err := getLDAPConfig() + if err != nil { + return 0, newError("Failed to get LDAP config", nil) + } if len(config.Servers) == 0 { return 0, newError("No LDAP servers available", nil) } diff --git a/pkg/middleware/auth_proxy/auth_proxy_test.go b/pkg/middleware/auth_proxy/auth_proxy_test.go index 849c156ac98..fbddca81d40 100644 --- a/pkg/middleware/auth_proxy/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy/auth_proxy_test.go @@ -67,18 +67,18 @@ func TestMiddlewareContext(t *testing.T) { return true } - readLDAPConfig = func() *ldap.Config { + getLDAPConfig = func() (*ldap.Config, error) { config := &ldap.Config{ Servers: []*ldap.ServerConfig{ {}, }, } - return config + return config, nil } defer func() { isLDAPEnabled = ldap.IsEnabled - readLDAPConfig = ldap.ReadConfig + getLDAPConfig = ldap.GetConfig }() store := remotecache.NewFakeStore(t) @@ -109,16 +109,16 @@ func TestMiddlewareContext(t *testing.T) { return true } - readLDAPConfig = func() *ldap.Config { + getLDAPConfig = func() (*ldap.Config, error) { config := &ldap.Config{ Servers: []*ldap.ServerConfig{}, } - return config + return config, nil } defer func() { isLDAPEnabled = ldap.IsEnabled - readLDAPConfig = ldap.ReadConfig + getLDAPConfig = ldap.GetConfig }() store := remotecache.NewFakeStore(t) diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go index b10e4640f4b..0a031e3a504 100644 --- a/pkg/plugins/plugins.go +++ b/pkg/plugins/plugins.go @@ -124,7 +124,7 @@ func (pm *PluginManager) Run(ctx context.Context) error { } } - // kil backend plugins + // kill backend plugins for _, p := range DataSources { p.Kill() } diff --git a/pkg/services/ldap/settings.go b/pkg/services/ldap/settings.go index c726fff3968..633920d0a19 100644 --- a/pkg/services/ldap/settings.go +++ b/pkg/services/ldap/settings.go @@ -2,9 +2,11 @@ package ldap import ( "fmt" - "os" + "sync" "github.com/BurntSushi/toml" + "github.com/grafana/grafana/pkg/util/errutil" + "golang.org/x/xerrors" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" @@ -56,47 +58,72 @@ type GroupToOrgRole struct { var config *Config var logger = log.New("ldap") +// loadingMutex locks the reading of the config so multiple requests for reloading are sequential. +var loadingMutex = &sync.Mutex{} + // IsEnabled checks if ldap is enabled func IsEnabled() bool { return setting.LdapEnabled } -// ReadConfig reads the config if -// ldap is enabled otherwise it will return nil -func ReadConfig() *Config { +// ReloadConfig reads the config from the disc and caches it. +func ReloadConfig() error { if IsEnabled() == false { return nil } + loadingMutex.Lock() + defer loadingMutex.Unlock() + + var err error + config, err = readConfig(setting.LdapConfigFile) + return err +} + +// GetConfig returns the LDAP config if LDAP is enabled otherwise it returns nil. It returns either cached value of +// the config or it reads it and caches it first. +func GetConfig() (*Config, error) { + if IsEnabled() == false { + return nil, nil + } // Make it a singleton if config != nil { - return config + return config, nil } - config = getConfig(setting.LdapConfigFile) + loadingMutex.Lock() + defer loadingMutex.Unlock() + + var err error + config, err = readConfig(setting.LdapConfigFile) - return config + return config, err } -func getConfig(configFile string) *Config { + +func readConfig(configFile string) (*Config, error) { result := &Config{} logger.Info("Ldap enabled, reading config file", "file", configFile) _, err := toml.DecodeFile(configFile, result) if err != nil { - logger.Crit("Failed to load ldap config file", "error", err) - os.Exit(1) + return nil, errutil.Wrap("Failed to load ldap config file", err) } if len(result.Servers) == 0 { - logger.Crit("ldap enabled but no ldap servers defined in config file") - os.Exit(1) + return nil, xerrors.New("ldap enabled but no ldap servers defined in config file") } // set default org id for _, server := range result.Servers { - assertNotEmptyCfg(server.SearchFilter, "search_filter") - assertNotEmptyCfg(server.SearchBaseDNs, "search_base_dns") + err = assertNotEmptyCfg(server.SearchFilter, "search_filter") + if err != nil { + return nil, errutil.Wrap("Failed to validate SearchFilter section", err) + } + err = assertNotEmptyCfg(server.SearchBaseDNs, "search_base_dns") + if err != nil { + return nil, errutil.Wrap("Failed to validate SearchBaseDNs section", err) + } for _, groupMap := range server.Groups { if groupMap.OrgId == 0 { @@ -105,22 +132,21 @@ func getConfig(configFile string) *Config { } } - return result + return result, nil } -func assertNotEmptyCfg(val interface{}, propName string) { +func assertNotEmptyCfg(val interface{}, propName string) error { switch v := val.(type) { case string: if v == "" { - logger.Crit("LDAP config file is missing option", "option", propName) - os.Exit(1) + return xerrors.Errorf("LDAP config file is missing option: %v", propName) } case []string: if len(v) == 0 { - logger.Crit("LDAP config file is missing option", "option", propName) - os.Exit(1) + return xerrors.Errorf("LDAP config file is missing option: %v", propName) } default: fmt.Println("unknown") } + return nil }