From 32d21356b991c47a307c7f62a2237f2ed3302e37 Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Mon, 17 Jun 2024 15:34:06 +0300 Subject: [PATCH] Auth: Keep config in a separate struct in LDAP (#89149) keep config in a separate struct in LDAP --- pkg/services/ldap/api/service.go | 20 ++++---- pkg/services/ldap/api/service_test.go | 24 ++++----- pkg/services/ldap/api/support_bundle.go | 12 ++--- pkg/services/ldap/ldap.go | 9 ++-- pkg/services/ldap/ldap_login_test.go | 26 +++++----- pkg/services/ldap/ldap_private_test.go | 31 +++++++----- pkg/services/ldap/ldap_test.go | 44 +++++++++-------- pkg/services/ldap/multildap/multildap.go | 5 +- pkg/services/ldap/multildap/multildap_test.go | 49 +++++++++---------- pkg/services/ldap/service/fake.go | 4 +- pkg/services/ldap/service/helpers.go | 4 +- pkg/services/ldap/service/ldap.go | 18 +++---- pkg/services/ldap/settings.go | 37 +++++++++++--- 13 files changed, 157 insertions(+), 126 deletions(-) diff --git a/pkg/services/ldap/api/service.go b/pkg/services/ldap/api/service.go index 368e3acff6b..20080b10cb1 100644 --- a/pkg/services/ldap/api/service.go +++ b/pkg/services/ldap/api/service.go @@ -29,7 +29,8 @@ import ( ) type Service struct { - cfg *setting.Cfg + cfg *ldap.Config + adminUser string userService user.Service authInfoService login.AuthInfoService ldapGroupsService ldap.Groups @@ -47,7 +48,8 @@ func ProvideService( sessionService auth.UserTokenService, bundleRegistry supportbundles.Service, ) *Service { s := &Service{ - cfg: cfg, + cfg: ldap.GetLDAPConfig(cfg), + adminUser: cfg.AdminUser, userService: userService, authInfoService: authInfoService, ldapGroupsService: ldapGroupsService, @@ -96,7 +98,7 @@ func ProvideService( // 403: forbiddenError // 500: internalServerError func (s *Service) ReloadLDAPCfg(c *contextmodel.ReqContext) response.Response { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return response.Error(http.StatusBadRequest, "LDAP is not enabled", nil) } @@ -122,7 +124,7 @@ func (s *Service) ReloadLDAPCfg(c *contextmodel.ReqContext) response.Response { // 403: forbiddenError // 500: internalServerError func (s *Service) GetLDAPStatus(c *contextmodel.ReqContext) response.Response { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return response.Error(http.StatusBadRequest, "LDAP is not enabled", nil) } @@ -169,7 +171,7 @@ func (s *Service) GetLDAPStatus(c *contextmodel.ReqContext) response.Response { // 403: forbiddenError // 500: internalServerError func (s *Service) PostSyncUserWithLDAP(c *contextmodel.ReqContext) response.Response { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return response.Error(http.StatusBadRequest, "LDAP is not enabled", nil) } @@ -206,7 +208,7 @@ func (s *Service) PostSyncUserWithLDAP(c *contextmodel.ReqContext) response.Resp userInfo, _, err := ldapClient.User(usr.Login) if err != nil { if errors.Is(err, multildap.ErrDidNotFindUser) { // User was not in the LDAP server - we need to take action: - if s.cfg.AdminUser == usr.Login { // User is *the* Grafana Admin. We cannot disable it. + if s.adminUser == usr.Login { // User is *the* Grafana Admin. We cannot disable it. errMsg := fmt.Sprintf(`Refusing to sync grafana super admin "%s" - it would be disabled`, usr.Login) s.log.Error(errMsg) return response.Error(http.StatusBadRequest, errMsg, err) @@ -250,7 +252,7 @@ func (s *Service) PostSyncUserWithLDAP(c *contextmodel.ReqContext) response.Resp // 403: forbiddenError // 500: internalServerError func (s *Service) GetUserFromLDAP(c *contextmodel.ReqContext) response.Response { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return response.Error(http.StatusBadRequest, "LDAP is not enabled", nil) } @@ -330,8 +332,8 @@ func (s *Service) identityFromLDAPUser(user *login.ExternalUserInfo) *authn.Iden SyncUser: true, SyncTeams: true, EnableUser: true, - SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync, - AllowSignUp: s.cfg.LDAPAllowSignup, + SyncOrgRoles: !s.cfg.SkipOrgRoleSync, + AllowSignUp: s.cfg.AllowSignUp, }, } } diff --git a/pkg/services/ldap/api/service_test.go b/pkg/services/ldap/api/service_test.go index 1e239a06e19..95fd54a0234 100644 --- a/pkg/services/ldap/api/service_test.go +++ b/pkg/services/ldap/api/service_test.go @@ -95,7 +95,7 @@ func TestGetUserFromLDAPAPIEndpoint_UserNotFound(t *testing.T) { ExpectedClient: &LDAPMock{ UserSearchResult: nil, }, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -160,7 +160,7 @@ func TestGetUserFromLDAPAPIEndpoint_OrgNotfound(t *testing.T) { UserSearchResult: userSearchResult, UserSearchConfig: userSearchConfig, }, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -229,7 +229,7 @@ func TestGetUserFromLDAPAPIEndpoint(t *testing.T) { UserSearchResult: userSearchResult, UserSearchConfig: userSearchConfig, }, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -314,7 +314,7 @@ func TestGetUserFromLDAPAPIEndpoint_WithTeamHandler(t *testing.T) { UserSearchResult: userSearchResult, UserSearchConfig: userSearchConfig, }, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -368,7 +368,7 @@ func TestGetLDAPStatusAPIEndpoint(t *testing.T) { _, server := setupAPITest(t, func(a *Service) { a.ldapService = &service.LDAPFakeService{ ExpectedClient: &LDAPMock{}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -407,7 +407,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_Success(t *testing.T) { ExpectedClient: &LDAPMock{UserSearchResult: &login.ExternalUserInfo{ Login: "ldap-daniel", }}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -442,7 +442,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotFound(t *testing.T) { a.userService = userServiceMock a.ldapService = &service.LDAPFakeService{ ExpectedClient: &LDAPMock{}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -475,10 +475,10 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenGrafanaAdmin(t *testing.T) { _, server := setupAPITest(t, func(a *Service) { a.userService = userServiceMock - a.cfg.AdminUser = "ldap-daniel" + a.adminUser = "ldap-daniel" a.ldapService = &service.LDAPFakeService{ ExpectedClient: &LDAPMock{UserSearchError: multildap.ErrDidNotFindUser}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -511,7 +511,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) { a.authInfoService = &authinfotest.FakeService{ExpectedExternalUser: &login.ExternalUserInfo{IsDisabled: true, UserId: 34}} a.ldapService = &service.LDAPFakeService{ ExpectedClient: &LDAPMock{UserSearchError: multildap.ErrDidNotFindUser}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) @@ -641,12 +641,12 @@ search_base_dns = ["dc=grafana,dc=org"]`) t.Run(tt.desc, func(t *testing.T) { _, server := setupAPITest(t, func(a *Service) { a.userService = &usertest.FakeUserService{ExpectedUser: &user.User{Login: "ldap-daniel", ID: 1}} - a.cfg.LDAPConfigFilePath = ldapConfigFile + a.cfg.ConfigFilePath = ldapConfigFile a.ldapService = &service.LDAPFakeService{ ExpectedClient: &LDAPMock{UserSearchResult: &login.ExternalUserInfo{ Login: "ldap-daniel", }}, - ExpectedConfig: &ldap.Config{}, + ExpectedConfig: &ldap.ServersConfig{}, } }) // Add minimal setup to pass handler diff --git a/pkg/services/ldap/api/support_bundle.go b/pkg/services/ldap/api/support_bundle.go index f85784d6f43..88d71c53f13 100644 --- a/pkg/services/ldap/api/support_bundle.go +++ b/pkg/services/ldap/api/support_bundle.go @@ -73,12 +73,12 @@ func (s *Service) supportBundleCollector(context.Context) (*supportbundles.Suppo bWriter.WriteString("```ini\n") - bWriter.WriteString(fmt.Sprintf("enabled = %v\n", s.cfg.LDAPAuthEnabled)) - bWriter.WriteString(fmt.Sprintf("config_file = %s\n", s.cfg.LDAPConfigFilePath)) - bWriter.WriteString(fmt.Sprintf("allow_sign_up = %v\n", s.cfg.LDAPAllowSignup)) - bWriter.WriteString(fmt.Sprintf("sync_cron = %s\n", s.cfg.LDAPSyncCron)) - bWriter.WriteString(fmt.Sprintf("active_sync_enabled = %v\n", s.cfg.LDAPActiveSyncEnabled)) - bWriter.WriteString(fmt.Sprintf("skip_org_role_sync = %v\n", s.cfg.LDAPSkipOrgRoleSync)) + bWriter.WriteString(fmt.Sprintf("enabled = %v\n", s.cfg.Enabled)) + bWriter.WriteString(fmt.Sprintf("config_file = %s\n", s.cfg.ConfigFilePath)) + bWriter.WriteString(fmt.Sprintf("allow_sign_up = %v\n", s.cfg.AllowSignUp)) + bWriter.WriteString(fmt.Sprintf("sync_cron = %s\n", s.cfg.SyncCron)) + bWriter.WriteString(fmt.Sprintf("active_sync_enabled = %v\n", s.cfg.ActiveSyncEnabled)) + bWriter.WriteString(fmt.Sprintf("skip_org_role_sync = %v\n", s.cfg.SkipOrgRoleSync)) bWriter.WriteString("```\n\n") diff --git a/pkg/services/ldap/ldap.go b/pkg/services/ldap/ldap.go index e4d75119ebf..add301da595 100644 --- a/pkg/services/ldap/ldap.go +++ b/pkg/services/ldap/ldap.go @@ -18,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -45,7 +44,7 @@ type IServer interface { // Server is basic struct of LDAP authorization type Server struct { - cfg *setting.Cfg + cfg *Config Config *ServerConfig Connection IConnection log log.Logger @@ -86,7 +85,7 @@ var ( ) // New creates the new LDAP connection -func New(config *ServerConfig, cfg *setting.Cfg) IServer { +func New(config *ServerConfig, cfg *Config) IServer { return &Server{ Config: config, cfg: cfg, @@ -414,7 +413,7 @@ func (server *Server) users(logins []string) ( // If there are no ldap group mappings access is true // otherwise a single group must match func (server *Server) validateGrafanaUser(user *login.ExternalUserInfo) error { - if !server.cfg.LDAPSkipOrgRoleSync && len(server.Config.Groups) > 0 && + if !server.cfg.SkipOrgRoleSync && len(server.Config.Groups) > 0 && (len(user.OrgRoles) == 0 && (user.IsGrafanaAdmin == nil || !*user.IsGrafanaAdmin)) { server.log.Warn( "User does not belong in any of the specified LDAP groups", @@ -499,7 +498,7 @@ func (server *Server) buildGrafanaUser(user *ldap.Entry) (*login.ExternalUserInf } // Skipping org role sync - if server.cfg.LDAPSkipOrgRoleSync { + if server.cfg.SkipOrgRoleSync { server.log.Debug("Skipping organization role mapping.") return extUser, nil } diff --git a/pkg/services/ldap/ldap_login_test.go b/pkg/services/ldap/ldap_login_test.go index a3feb6d9734..11d1d8a6cfc 100644 --- a/pkg/services/ldap/ldap_login_test.go +++ b/pkg/services/ldap/ldap_login_test.go @@ -10,7 +10,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/login" - "github.com/grafana/grafana/pkg/setting" ) var defaultLogin = &login.LoginUserQuery{ @@ -31,8 +30,9 @@ func TestServer_Login_UserBind_Fail(t *testing.T) { } } - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, Config: &ServerConfig{ @@ -105,8 +105,9 @@ func TestServer_Login_ValidCredentials(t *testing.T) { return nil } - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -142,8 +143,9 @@ func TestServer_Login_UnauthenticatedBind(t *testing.T) { return nil } - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -189,8 +191,9 @@ func TestServer_Login_AuthenticatedBind(t *testing.T) { return nil } - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -232,8 +235,9 @@ func TestServer_Login_UserWildcardBind(t *testing.T) { return nil } - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, diff --git a/pkg/services/ldap/ldap_private_test.go b/pkg/services/ldap/ldap_private_test.go index 24e1f3efe4c..6dc52c19ba7 100644 --- a/pkg/services/ldap/ldap_private_test.go +++ b/pkg/services/ldap/ldap_private_test.go @@ -10,7 +10,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/setting" ) func TestServer_getSearchRequest(t *testing.T) { @@ -53,8 +52,9 @@ func TestServer_getSearchRequest(t *testing.T) { func TestSerializeUsers(t *testing.T) { t.Run("simple case", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -92,8 +92,9 @@ func TestSerializeUsers(t *testing.T) { }) t.Run("without lastname", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -129,8 +130,9 @@ func TestSerializeUsers(t *testing.T) { }) t.Run("mark user without matching group as disabled", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -163,8 +165,9 @@ func TestSerializeUsers(t *testing.T) { func TestServer_validateGrafanaUser(t *testing.T) { t.Run("no group config", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -183,8 +186,9 @@ func TestServer_validateGrafanaUser(t *testing.T) { }) t.Run("user in group", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -210,8 +214,9 @@ func TestServer_validateGrafanaUser(t *testing.T) { }) t.Run("user not in group", func(t *testing.T) { - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, diff --git a/pkg/services/ldap/ldap_test.go b/pkg/services/ldap/ldap_test.go index 11a38172608..b6768665418 100644 --- a/pkg/services/ldap/ldap_test.go +++ b/pkg/services/ldap/ldap_test.go @@ -11,7 +11,6 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/setting" ) const ( @@ -55,7 +54,7 @@ func TestNew(t *testing.T) { result := New(&ServerConfig{ Attr: AttributeMap{}, SearchBaseDNs: []string{"BaseDNHere"}, - }, &setting.Cfg{}) + }, &Config{}) assert.Implements(t, (*IServer)(nil), result) } @@ -68,7 +67,7 @@ func TestServer_Dial(t *testing.T) { ClientCert: "./testdata/parsable.cert", ClientKey: "./testdata/parsable.pem", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -79,7 +78,7 @@ func TestServer_Dial(t *testing.T) { serverConfig := &ServerConfig{ RootCACert: "./testdata/invalid.cert", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -90,7 +89,7 @@ func TestServer_Dial(t *testing.T) { serverConfig := &ServerConfig{ RootCACert: "./testdata/nofile.cert", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -102,7 +101,7 @@ func TestServer_Dial(t *testing.T) { ClientCert: "./testdata/invalid.cert", ClientKey: "./testdata/invalid.pem", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -114,7 +113,7 @@ func TestServer_Dial(t *testing.T) { ClientCert: "./testdata/nofile.cert", ClientKey: "./testdata/parsable.pem", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -128,7 +127,7 @@ func TestServer_Dial(t *testing.T) { ClientCertValue: validCert, ClientKeyValue: validKey, } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -139,7 +138,7 @@ func TestServer_Dial(t *testing.T) { serverConfig := &ServerConfig{ RootCACertValue: []string{"invalid-certificate"}, } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -150,7 +149,7 @@ func TestServer_Dial(t *testing.T) { serverConfig := &ServerConfig{ RootCACertValue: []string{"aW52YWxpZC1jZXJ0aWZpY2F0ZQ=="}, } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -162,7 +161,7 @@ func TestServer_Dial(t *testing.T) { ClientCertValue: "invalid-certificate", ClientKeyValue: validKey, } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -174,7 +173,7 @@ func TestServer_Dial(t *testing.T) { ClientCertValue: validCert, ClientKeyValue: "aW52YWxpZC1rZXk=", } - server := New(serverConfig, &setting.Cfg{}) + server := New(serverConfig, &Config{}) err := server.Dial() require.Error(t, err) @@ -226,8 +225,9 @@ func TestServer_Users(t *testing.T) { conn.setSearchResult(&result) // Set up attribute map without surname and email - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -323,7 +323,7 @@ func TestServer_Users(t *testing.T) { }) server := &Server{ - cfg: setting.NewCfg(), + cfg: &Config{}, Config: &ServerConfig{ Attr: AttributeMap{ Username: "username", @@ -370,8 +370,9 @@ func TestServer_Users(t *testing.T) { } }) - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -464,8 +465,9 @@ func TestServer_Users(t *testing.T) { }) isGrafanaAdmin := true - cfg := setting.NewCfg() - cfg.LDAPAuthEnabled = true + cfg := &Config{ + Enabled: true, + } server := &Server{ cfg: cfg, @@ -506,7 +508,7 @@ func TestServer_Users(t *testing.T) { require.True(t, res[0].IsDisabled) }) t.Run("skip org role sync", func(t *testing.T) { - server.cfg.LDAPSkipOrgRoleSync = true + server.cfg.SkipOrgRoleSync = true res, err := server.Users([]string{"groot"}) require.NoError(t, err) @@ -517,7 +519,7 @@ func TestServer_Users(t *testing.T) { require.False(t, res[0].IsDisabled) }) t.Run("sync org role", func(t *testing.T) { - server.cfg.LDAPSkipOrgRoleSync = false + server.cfg.SkipOrgRoleSync = false res, err := server.Users([]string{"groot"}) require.NoError(t, err) require.Len(t, res, 1) diff --git a/pkg/services/ldap/multildap/multildap.go b/pkg/services/ldap/multildap/multildap.go index 2e70a8d5978..601a058da42 100644 --- a/pkg/services/ldap/multildap/multildap.go +++ b/pkg/services/ldap/multildap/multildap.go @@ -7,7 +7,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/login" - "github.com/grafana/grafana/pkg/setting" ) // GetConfig gets LDAP config @@ -54,12 +53,12 @@ type IMultiLDAP interface { // MultiLDAP is basic struct of LDAP authorization type MultiLDAP struct { configs []*ldap.ServerConfig - cfg *setting.Cfg + cfg *ldap.Config log log.Logger } // New creates the new LDAP auth -func New(configs []*ldap.ServerConfig, cfg *setting.Cfg) IMultiLDAP { +func New(configs []*ldap.ServerConfig, cfg *ldap.Config) IMultiLDAP { return &MultiLDAP{ configs: configs, cfg: cfg, diff --git a/pkg/services/ldap/multildap/multildap_test.go b/pkg/services/ldap/multildap/multildap_test.go index f76bdb9bab1..a04d0b1d037 100644 --- a/pkg/services/ldap/multildap/multildap_test.go +++ b/pkg/services/ldap/multildap/multildap_test.go @@ -8,7 +8,6 @@ import ( "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/login" - "github.com/grafana/grafana/pkg/setting" //TODO(sh0rez): remove once import cycle resolved _ "github.com/grafana/grafana/pkg/api/response" @@ -19,7 +18,7 @@ func TestMultiLDAP(t *testing.T) { t.Run("Should return error for absent config list", func(t *testing.T) { setup() - multi := New([]*ldap.ServerConfig{}, setting.NewCfg()) + multi := New([]*ldap.ServerConfig{}, &ldap.Config{}) _, err := multi.Ping() require.Error(t, err) @@ -35,7 +34,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {Host: "10.0.0.1", Port: 361}, - }, setting.NewCfg()) + }, &ldap.Config{}) statuses, err := multi.Ping() @@ -53,7 +52,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {Host: "10.0.0.1", Port: 361}, - }, setting.NewCfg()) + }, &ldap.Config{}) statuses, err := multi.Ping() @@ -71,7 +70,7 @@ func TestMultiLDAP(t *testing.T) { t.Run("Should return error for absent config list", func(t *testing.T) { setup() - multi := New([]*ldap.ServerConfig{}, setting.NewCfg()) + multi := New([]*ldap.ServerConfig{}, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Error(t, err) @@ -88,7 +87,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) @@ -104,7 +103,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 2, mock.dialCalledTimes) @@ -125,7 +124,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) result, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 1, mock.dialCalledTimes) @@ -145,7 +144,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 2, mock.dialCalledTimes) @@ -164,7 +163,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 2, mock.dialCalledTimes) @@ -184,7 +183,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 2, mock.dialCalledTimes) @@ -202,7 +201,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Login(&login.LoginUserQuery{}) require.Equal(t, 1, mock.dialCalledTimes) @@ -219,7 +218,7 @@ func TestMultiLDAP(t *testing.T) { t.Run("Should return error for absent config list", func(t *testing.T) { setup() - multi := New([]*ldap.ServerConfig{}, setting.NewCfg()) + multi := New([]*ldap.ServerConfig{}, &ldap.Config{}) _, _, err := multi.User("test") require.Error(t, err) @@ -236,7 +235,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, _, err := multi.User("test") @@ -251,7 +250,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, _, err := multi.User("test") require.Equal(t, 2, mock.dialCalledTimes) @@ -271,7 +270,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, _, err := multi.User("test") require.Equal(t, 1, mock.dialCalledTimes) @@ -298,7 +297,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) user, _, err := multi.User("test") require.Equal(t, 1, mock.dialCalledTimes) @@ -319,7 +318,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, _, err := multi.User("test") require.Equal(t, 2, mock.dialCalledTimes) @@ -338,7 +337,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Users([]string{"test"}) require.Equal(t, 2, mock.dialCalledTimes) @@ -349,7 +348,7 @@ func TestMultiLDAP(t *testing.T) { t.Run("Should return error for absent config list", func(t *testing.T) { setup() - multi := New([]*ldap.ServerConfig{}, setting.NewCfg()) + multi := New([]*ldap.ServerConfig{}, &ldap.Config{}) _, err := multi.Users([]string{"test"}) require.Error(t, err) @@ -366,7 +365,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Users([]string{"test"}) @@ -381,7 +380,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Users([]string{"test"}) require.Equal(t, 2, mock.dialCalledTimes) @@ -401,7 +400,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) _, err := multi.Users([]string{"test"}) require.Equal(t, 1, mock.dialCalledTimes) @@ -434,7 +433,7 @@ func TestMultiLDAP(t *testing.T) { multi := New([]*ldap.ServerConfig{ {}, {}, - }, setting.NewCfg()) + }, &ldap.Config{}) users, err := multi.Users([]string{"test"}) require.Equal(t, 2, mock.dialCalledTimes) @@ -512,7 +511,7 @@ func (mock *mockLDAP) Bind() error { func setup() *mockLDAP { mock := &mockLDAP{} - newLDAP = func(config *ldap.ServerConfig, cfg *setting.Cfg) ldap.IServer { + newLDAP = func(config *ldap.ServerConfig, cfg *ldap.Config) ldap.IServer { return mock } diff --git a/pkg/services/ldap/service/fake.go b/pkg/services/ldap/service/fake.go index 05940531038..c6aa168ae37 100644 --- a/pkg/services/ldap/service/fake.go +++ b/pkg/services/ldap/service/fake.go @@ -7,7 +7,7 @@ import ( ) type LDAPFakeService struct { - ExpectedConfig *ldap.Config + ExpectedConfig *ldap.ServersConfig ExpectedClient multildap.IMultiLDAP ExpectedError error ExpectedUser *login.ExternalUserInfo @@ -22,7 +22,7 @@ func (s *LDAPFakeService) ReloadConfig() error { return s.ExpectedError } -func (s *LDAPFakeService) Config() *ldap.Config { +func (s *LDAPFakeService) Config() *ldap.ServersConfig { return s.ExpectedConfig } diff --git a/pkg/services/ldap/service/helpers.go b/pkg/services/ldap/service/helpers.go index 245bddb9aa5..6e2c10d5eb6 100644 --- a/pkg/services/ldap/service/helpers.go +++ b/pkg/services/ldap/service/helpers.go @@ -13,8 +13,8 @@ import ( const defaultTimeout = 10 -func readConfig(configFile string) (*ldap.Config, error) { - result := &ldap.Config{} +func readConfig(configFile string) (*ldap.ServersConfig, error) { + result := &ldap.ServersConfig{} logger.Info("LDAP enabled, reading config file", "file", configFile) diff --git a/pkg/services/ldap/service/ldap.go b/pkg/services/ldap/service/ldap.go index 8378a9d5570..1380b65a9b3 100644 --- a/pkg/services/ldap/service/ldap.go +++ b/pkg/services/ldap/service/ldap.go @@ -19,7 +19,7 @@ var ( // LDAP is the interface for the LDAP service. type LDAP interface { ReloadConfig() error - Config() *ldap.Config + Config() *ldap.ServersConfig Client() multildap.IMultiLDAP // Login authenticates the user against the LDAP server. @@ -30,8 +30,8 @@ type LDAP interface { type LDAPImpl struct { client multildap.IMultiLDAP - cfg *setting.Cfg - ldapCfg *ldap.Config + cfg *ldap.Config + ldapCfg *ldap.ServersConfig log log.Logger // loadingMutex locks the reading of the config so multiple requests for reloading are sequential. @@ -42,7 +42,7 @@ func ProvideService(cfg *setting.Cfg) *LDAPImpl { s := &LDAPImpl{ client: nil, ldapCfg: nil, - cfg: cfg, + cfg: ldap.GetLDAPConfig(cfg), log: log.New("ldap.service"), loadingMutex: &sync.Mutex{}, } @@ -63,14 +63,14 @@ func ProvideService(cfg *setting.Cfg) *LDAPImpl { } func (s *LDAPImpl) ReloadConfig() error { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return nil } s.loadingMutex.Lock() defer s.loadingMutex.Unlock() - config, err := readConfig(s.cfg.LDAPConfigFilePath) + config, err := readConfig(s.cfg.ConfigFilePath) if err != nil { return err } @@ -90,12 +90,12 @@ func (s *LDAPImpl) Client() multildap.IMultiLDAP { return s.client } -func (s *LDAPImpl) Config() *ldap.Config { +func (s *LDAPImpl) Config() *ldap.ServersConfig { return s.ldapCfg } func (s *LDAPImpl) Login(query *login.LoginUserQuery) (*login.ExternalUserInfo, error) { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return nil, ErrLDAPNotEnabled } @@ -108,7 +108,7 @@ func (s *LDAPImpl) Login(query *login.LoginUserQuery) (*login.ExternalUserInfo, } func (s *LDAPImpl) User(username string) (*login.ExternalUserInfo, error) { - if !s.cfg.LDAPAuthEnabled { + if !s.cfg.Enabled { return nil, ErrLDAPNotEnabled } diff --git a/pkg/services/ldap/settings.go b/pkg/services/ldap/settings.go index 459f6dadcf2..a7d7b837cb0 100644 --- a/pkg/services/ldap/settings.go +++ b/pkg/services/ldap/settings.go @@ -15,8 +15,18 @@ import ( const defaultTimeout = 10 -// Config holds list of connections to LDAP +// Config holds parameters from the .ini config file type Config struct { + Enabled bool + ConfigFilePath string + AllowSignUp bool + SkipOrgRoleSync bool + SyncCron string + ActiveSyncEnabled bool +} + +// ServersConfig holds list of connections to LDAP +type ServersConfig struct { Servers []*ServerConfig `toml:"servers" json:"servers"` } @@ -83,16 +93,27 @@ var loadingMutex = &sync.Mutex{} // We need to define in this space so `GetConfig` fn // could be defined as singleton -var config *Config +var config *ServersConfig + +func GetLDAPConfig(cfg *setting.Cfg) *Config { + return &Config{ + Enabled: cfg.LDAPAuthEnabled, + ConfigFilePath: cfg.LDAPConfigFilePath, + AllowSignUp: cfg.LDAPAllowSignup, + SkipOrgRoleSync: cfg.LDAPSkipOrgRoleSync, + SyncCron: cfg.LDAPSyncCron, + ActiveSyncEnabled: cfg.LDAPActiveSyncEnabled, + } +} // 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(cfg *setting.Cfg) (*Config, error) { +func GetConfig(cfg *Config) (*ServersConfig, error) { if cfg != nil { - if !cfg.LDAPAuthEnabled { + if !cfg.Enabled { return nil, nil } - } else if !cfg.LDAPAuthEnabled { + } else if !cfg.Enabled { return nil, nil } @@ -104,11 +125,11 @@ func GetConfig(cfg *setting.Cfg) (*Config, error) { loadingMutex.Lock() defer loadingMutex.Unlock() - return readConfig(cfg.LDAPConfigFilePath) + return readConfig(cfg.ConfigFilePath) } -func readConfig(configFile string) (*Config, error) { - result := &Config{} +func readConfig(configFile string) (*ServersConfig, error) { + result := &ServersConfig{} logger.Info("LDAP enabled, reading config file", "file", configFile)