public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[v9.4.x] LDAP: Fix user disabling (#74318)

Browse Source 
* [LDAP] Disable removed users on login (#74016)

* manual backport of #74016 on legacy code

* LDAP: Fix active sync with large quantities of users (#73834)

* Fix authenticate user test
This commit is contained in:
Gabriel MABILLE
2023-09-04 16:14:20 +02:00
committed by GitHub
parent 1e00f2e0ab
commit 49d3cf745f
12 changed files with 187 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/login/auth.go
+8 -2
View File
@@ -10,6 +10,7 @@ import (
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/loginattempt"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
var (
@@ -36,13 +37,18 @@ type AuthenticatorService struct {
loginService login.Service
loginAttemptService loginattempt.Service
userService user.Service
authInfoService login.AuthInfoService
cfg *setting.Cfg
}
func ProvideService(store db.DB, loginService login.Service, loginAttemptService loginattempt.Service, userService user.Service) *AuthenticatorService {
func ProvideService(store db.DB, loginService login.Service, loginAttemptService loginattempt.Service,
userService user.Service, authInfoService login.AuthInfoService, cfg *setting.Cfg) *AuthenticatorService {
a := &AuthenticatorService{
loginService: loginService,
loginAttemptService: loginAttemptService,
userService: userService,
authInfoService: authInfoService,
cfg: cfg,
}
return a
}
@@ -73,7 +79,7 @@ func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *logi
return err
}
ldapEnabled, ldapErr := loginUsingLDAP(ctx, query, a.loginService)
ldapEnabled, ldapErr := loginUsingLDAP(ctx, query, a.loginService, a.userService, a.authInfoService, a.cfg)
if ldapEnabled {
query.AuthModule = login.LDAPAuthModule
if ldapErr == nil || !errors.Is(ldapErr, ldap.ErrInvalidCredentials) {
pkg/login/auth_test.go
+2 -2
View File
@@ -38,7 +38,7 @@ func TestAuthenticateUser(t *testing.T) {
sc.loginUserQuery.Cfg.DisableLogin = true
loginAttemptService := &loginattempttest.MockLoginAttemptService{ExpectedValid: true}
a := AuthenticatorService{loginAttemptService: loginAttemptService, loginService: &logintest.LoginServiceFake{}}
a := AuthenticatorService{loginAttemptService: loginAttemptService, loginService: &logintest.LoginServiceFake{}, cfg: setting.NewCfg()}
err := a.AuthenticateUser(context.Background(), sc.loginUserQuery)
require.EqualError(t, err, ErrNoAuthProvider.Error())
@@ -195,7 +195,7 @@ func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) {
}
func mockLoginUsingLDAP(enabled bool, err error, sc *authScenarioContext) {
loginUsingLDAP = func(ctx context.Context, query *login.LoginUserQuery, _ login.Service) (bool, error) {
loginUsingLDAP = func(ctx context.Context, query *login.LoginUserQuery, _ login.Service, _ user.Service, _ login.AuthInfoService, _ *setting.Cfg) (bool, error) {
sc.ldapLoginWasCalled = true
return enabled, err
}
pkg/login/ldap_login.go
+32 -11
View File
@@ -9,15 +9,13 @@ import (
"github.com/grafana/grafana/pkg/services/ldap"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/multildap"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
// getLDAPConfig gets LDAP config
var getLDAPConfig = multildap.GetConfig
// isLDAPEnabled checks if LDAP is enabled
var isLDAPEnabled = multildap.IsEnabled
// newLDAP creates multiple LDAP instance
var newLDAP = multildap.New
@@ -26,10 +24,9 @@ var ldapLogger = log.New("login.ldap")
// 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(ctx context.Context, query *login.LoginUserQuery, loginService login.Service) (bool, error) {
enabled := isLDAPEnabled()
if !enabled {
var loginUsingLDAP = func(ctx context.Context, query *login.LoginUserQuery,
loginService login.Service, userService user.Service, authInfoService login.AuthInfoService, cfg *setting.Cfg) (bool, error) {
if !cfg.LDAPAuthEnabled {
return false, nil
}
@@ -41,13 +38,37 @@ var loginUsingLDAP = func(ctx context.Context, query *login.LoginUserQuery, logi
externalUser, err := newLDAP(config.Servers).Login(query)
if err != nil {
if errors.Is(err, ldap.ErrCouldNotFindUser) {
// Ignore the error since user might not be present anyway
if err := loginService.DisableExternalUser(ctx, query.Username); err != nil {
ldapLogger.Debug("Failed to disable external user", "err", err)
ldapLogger.Debug("user was not found in the LDAP directory tree", "username", query.Username)
retErr := ldap.ErrInvalidCredentials
// Retrieve the user from store based on the login
dbUser, errGet := userService.GetByLogin(ctx, &user.GetUserByLoginQuery{
LoginOrEmail: query.Username,
})
if errors.Is(errGet, user.ErrUserNotFound) {
return true, retErr
} else if errGet != nil {
return true, errGet
}
// Check if the user logged in via LDAP
authModuleQuery := &login.GetAuthInfoQuery{UserId: dbUser.ID, AuthModule: login.LDAPAuthModule}
errGetAuthInfo := authInfoService.GetAuthInfo(ctx, authModuleQuery)
if errors.Is(errGetAuthInfo, user.ErrUserNotFound) {
return true, retErr
} else if errGetAuthInfo != nil {
return true, errGetAuthInfo
}
// Disable the user
ldapLogger.Debug("user was removed from the LDAP directory tree, disabling it", "username", query.Username, "authID", authModuleQuery.Result.AuthId)
if errDisable := loginService.DisableExternalUser(ctx, query.Username); errDisable != nil {
ldapLogger.Debug("Failed to disable external user", "err", errDisable)
return true, errDisable
}
// Return invalid credentials if we couldn't find the user anywhere
return true, ldap.ErrInvalidCredentials
return true, retErr
}
return true, err
pkg/login/ldap_login_test.go
+11 -4
View File
@@ -12,6 +12,7 @@ import (
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/multildap"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
)
@@ -19,7 +20,8 @@ var errTest = errors.New("test error")
func TestLoginUsingLDAP(t *testing.T) {
LDAPLoginScenario(t, "When LDAP enabled and no server configured", func(sc *LDAPLoginScenarioContext) {
setting.LDAPAuthEnabled = true
cfg := setting.NewCfg()
cfg.LDAPAuthEnabled = true
sc.withLoginResult(false)
getLDAPConfig = func(*setting.Cfg) (*ldap.Config, error) {
@@ -31,7 +33,9 @@ func TestLoginUsingLDAP(t *testing.T) {
}
loginService := &logintest.LoginServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService)
userService := &usertest.FakeUserService{}
authInfoService := &logintest.AuthInfoServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService, userService, authInfoService, cfg)
require.EqualError(t, err, errTest.Error())
assert.True(t, enabled)
@@ -39,11 +43,14 @@ func TestLoginUsingLDAP(t *testing.T) {
})
LDAPLoginScenario(t, "When LDAP disabled", func(sc *LDAPLoginScenarioContext) {
setting.LDAPAuthEnabled = false
cfg := setting.NewCfg()
cfg.LDAPAuthEnabled = false
sc.withLoginResult(false)
loginService := &logintest.LoginServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService)
userService := &usertest.FakeUserService{}
authInfoService := &logintest.AuthInfoServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService, userService, authInfoService, cfg)
require.NoError(t, err)
assert.False(t, enabled)
pkg/middleware/middleware_basic_auth_test.go
+1 -1
View File
@@ -67,7 +67,7 @@ func TestMiddlewareBasicAuth(t *testing.T) {
sc.userService.ExpectedUser = &user.User{Password: encoded, ID: id, Salt: salt}
sc.userService.ExpectedSignedInUser = &user.SignedInUser{UserID: id}
login.ProvideService(sc.mockSQLStore, &logintest.LoginServiceFake{}, nil, sc.userService)
login.ProvideService(sc.mockSQLStore, &logintest.LoginServiceFake{}, nil, sc.userService, sc.authInfoService, sc.cfg)
authHeader := util.GetBasicAuthHeader("myUser", password)
sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).exec()
pkg/middleware/testing.go
+2
View File
@@ -18,6 +18,7 @@ import (
"github.com/grafana/grafana/pkg/services/contexthandler/ctxkey"
contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
"github.com/grafana/grafana/pkg/services/login/loginservice"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/org/orgtest"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
@@ -49,6 +50,7 @@ type scenarioContext struct {
userService *usertest.FakeUserService
oauthTokenService *authtest.FakeOAuthTokenService
orgService *orgtest.FakeOrgService
authInfoService *logintest.AuthInfoServiceFake
req *http.Request
}
pkg/services/authn/authnimpl/service.go
+1 -1
View File
@@ -86,7 +86,7 @@ func ProvideService(
var proxyClients []authn.ProxyClient
var passwordClients []authn.PasswordClient
if s.cfg.LDAPAuthEnabled {
ldap := clients.ProvideLDAP(cfg)
ldap := clients.ProvideLDAP(cfg, userService, authInfoService)
proxyClients = append(proxyClients, ldap)
passwordClients = append(passwordClients, ldap)
}
pkg/services/authn/authnimpl/sync/user_sync.go
+1 -1
View File
@@ -176,7 +176,7 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
}
if needsUpdate {
s.log.Debug("Syncing user info", "id", usr.ID, "update", updateCmd)
s.log.Debug("Syncing user info", "id", id.ID, "update", fmt.Sprintf("%v", updateCmd))
if err := s.userService.Update(ctx, updateCmd); err != nil {
return err
}
pkg/services/authn/clients/ldap.go
+44 -7
View File
@@ -4,28 +4,33 @@ import (
"context"
"errors"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/multildap"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
var _ authn.ProxyClient = new(LDAP)
var _ authn.PasswordClient = new(LDAP)
func ProvideLDAP(cfg *setting.Cfg) *LDAP {
return &LDAP{cfg, &ldapServiceImpl{cfg}}
func ProvideLDAP(cfg *setting.Cfg, userService user.Service, authInfoService login.AuthInfoService) *LDAP {
return &LDAP{cfg, log.New("authn.ldap"), &ldapServiceImpl{cfg}, userService, authInfoService}
}
type LDAP struct {
cfg *setting.Cfg
service ldapService
cfg *setting.Cfg
logger log.Logger
service ldapService
userService user.Service
authInfoService login.AuthInfoService
}
func (c *LDAP) AuthenticateProxy(ctx context.Context, r *authn.Request, username string, _ map[string]string) (*authn.Identity, error) {
info, err := c.service.User(username)
if errors.Is(err, multildap.ErrDidNotFindUser) {
return nil, errIdentityNotFound.Errorf("no user found: %w", err)
return c.disableUser(ctx, username)
}
if err != nil {
@@ -42,8 +47,7 @@ func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, usern
})
if errors.Is(err, multildap.ErrCouldNotFindUser) {
// FIXME: disable user in grafana if not found
return nil, errIdentityNotFound.Errorf("no user found: %w", err)
return c.disableUser(ctx, username)
}
// user was found so set auth module in req metadata
@@ -60,6 +64,39 @@ func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, usern
return identityFromLDAPInfo(r.OrgID, info, c.cfg.LDAPAllowSignup), nil
}
// disableUser will disable users if they logged in via LDAP previously
func (c *LDAP) disableUser(ctx context.Context, username string) (*authn.Identity, error) {
c.logger.Debug("user was not found in the LDAP directory tree", "username", username)
retErr := errIdentityNotFound.Errorf("no user found: %w", multildap.ErrDidNotFindUser)
// Retrieve the user from store based on the login
dbUser, errGet := c.userService.GetByLogin(ctx, &user.GetUserByLoginQuery{
LoginOrEmail: username,
})
if errors.Is(errGet, user.ErrUserNotFound) {
return nil, retErr
} else if errGet != nil {
return nil, errGet
}
// Check if the user logged in via LDAP
query := &login.GetAuthInfoQuery{UserId: dbUser.ID, AuthModule: login.LDAPAuthModule}
errGetAuthInfo := c.authInfoService.GetAuthInfo(ctx, query)
if errors.Is(errGetAuthInfo, user.ErrUserNotFound) {
return nil, retErr
} else if errGetAuthInfo != nil {
return nil, errGetAuthInfo
}
// Disable the user
c.logger.Debug("user was removed from the LDAP directory tree, disabling it", "username", username, "authID", query.Result.AuthId)
if errDisable := c.userService.Disable(ctx, &user.DisableUserCommand{UserID: dbUser.ID, IsDisabled: true}); errDisable != nil {
return nil, errDisable
}
return nil, retErr
}
type ldapService interface {
Login(query *login.LoginUserQuery) (*login.ExternalUserInfo, error)
User(username string) (*login.ExternalUserInfo, error)
pkg/services/authn/clients/ldap_test.go
+78 -25
View File
@@ -6,25 +6,38 @@ import (
"github.com/stretchr/testify/assert"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/ldap"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/multildap"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
)
func TestLDAP_AuthenticateProxy(t *testing.T) {
type testCase struct {
desc string
username string
expectedLDAPErr error
expectedLDAPInfo *login.ExternalUserInfo
expectedErr error
expectedIdentity *authn.Identity
}
type ldapTestCase struct {
desc string
username string
password string
expectedErr error
expectedLDAPErr error
expectedLDAPInfo *login.ExternalUserInfo
expectedIdentity *authn.Identity
tests := []testCase{
// Disabling User
expectedUser user.User
expectedUserErr error
expectedAuthInfo login.UserAuth
expectedAuthInfoErr error
disableCalled bool
expectDisable bool
}
func TestLDAP_AuthenticateProxy(t *testing.T) {
tests := []ldapTestCase{
{
desc: "should return valid identity when found by ldap service",
username: "test",
@@ -62,32 +75,34 @@ func TestLDAP_AuthenticateProxy(t *testing.T) {
desc: "should return error when user is not found",
username: "test",
expectedLDAPErr: multildap.ErrDidNotFindUser,
expectedUserErr: user.ErrUserNotFound,
expectedErr: errIdentityNotFound,
},
{
desc: "should disable user when user is not found",
username: "test",
expectedLDAPErr: multildap.ErrDidNotFindUser,
expectedUser: user.User{ID: 11, Login: "test"},
expectedAuthInfo: login.UserAuth{UserId: 11, AuthId: "cn=test,ou=users,dc=example,dc=org", AuthModule: login.LDAPAuthModule},
expectDisable: true,
expectedErr: errIdentityNotFound,
},
}
for _, tt := range tests {
for i := range tests {
tt := tests[i]
t.Run(tt.desc, func(t *testing.T) {
c := &LDAP{cfg: setting.NewCfg(), service: fakeLDAPService{ExpectedInfo: tt.expectedLDAPInfo, ExpectedErr: tt.expectedLDAPErr}}
c := setupLDAPTestCase(&tt)
identity, err := c.AuthenticateProxy(context.Background(), &authn.Request{OrgID: 1}, tt.username, nil)
assert.ErrorIs(t, err, tt.expectedErr)
assert.EqualValues(t, tt.expectedIdentity, identity)
assert.Equal(t, tt.expectDisable, tt.disableCalled)
})
}
}
func TestLDAP_AuthenticatePassword(t *testing.T) {
type testCase struct {
desc string
username string
password string
expectedErr error
expectedLDAPErr error
expectedLDAPInfo *login.ExternalUserInfo
expectedIdentity *authn.Identity
}
tests := []testCase{
tests := []ldapTestCase{
{
desc: "should successfully authenticate with correct username and password",
username: "test",
@@ -135,20 +150,58 @@ func TestLDAP_AuthenticatePassword(t *testing.T) {
password: "wrong",
expectedErr: errIdentityNotFound,
expectedLDAPErr: ldap.ErrCouldNotFindUser,
expectedUserErr: user.ErrUserNotFound,
},
{
desc: "should disable user if not found",
username: "test",
password: "wrong",
expectedErr: errIdentityNotFound,
expectedLDAPErr: ldap.ErrCouldNotFindUser,
expectedUser: user.User{ID: 11, Login: "test"},
expectedAuthInfo: login.UserAuth{UserId: 11, AuthId: "cn=test,ou=users,dc=example,dc=org", AuthModule: login.LDAPAuthModule},
expectDisable: true,
},
}
for _, tt := range tests {
for i := range tests {
tt := tests[i]
t.Run(tt.desc, func(t *testing.T) {
c := &LDAP{cfg: setting.NewCfg(), service: fakeLDAPService{ExpectedInfo: tt.expectedLDAPInfo, ExpectedErr: tt.expectedLDAPErr}}
c := setupLDAPTestCase(&tt)
identity, err := c.AuthenticatePassword(context.Background(), &authn.Request{OrgID: 1}, tt.username, tt.password)
assert.ErrorIs(t, err, tt.expectedErr)
assert.EqualValues(t, tt.expectedIdentity, identity)
assert.Equal(t, tt.expectDisable, tt.disableCalled)
})
}
}
func setupLDAPTestCase(tt *ldapTestCase) *LDAP {
userService := &usertest.FakeUserService{
ExpectedError: tt.expectedUserErr,
ExpectedUser: &tt.expectedUser,
DisableFn: func(ctx context.Context, cmd *user.DisableUserCommand) error {
tt.disableCalled = true
return nil
},
}
authInfoService := &logintest.AuthInfoServiceFake{
ExpectedUserAuth: &tt.expectedAuthInfo,
ExpectedError: tt.expectedAuthInfoErr,
}
c := &LDAP{
cfg: setting.NewCfg(),
logger: log.New("authn.ldap.test"),
service: &fakeLDAPService{ExpectedInfo: tt.expectedLDAPInfo, ExpectedErr: tt.expectedLDAPErr},
userService: userService,
authInfoService: authInfoService,
}
return c
}
func strPtr(s string) *string {
return &s
}
pkg/services/ldap/ldap.go
+2 -2
View File
@@ -283,8 +283,8 @@ func (server *Server) Users(logins []string) (
) {
var users [][]*ldap.Entry
err := getUsersIteration(logins, func(previous, current int) error {
var err error
users, err = server.users(logins[previous:current])
iterationUsers, err := server.users(logins[previous:current])
users = append(users, iterationUsers...)
return err
})
if err != nil {
pkg/services/user/usertest/fake.go
+5
View File
@@ -17,6 +17,7 @@ type FakeUserService struct {
GetSignedInUserFn func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error)
CreateFn func(ctx context.Context, cmd *user.CreateUserCommand) (*user.User, error)
DisableFn func(ctx context.Context, cmd *user.DisableUserCommand) error
counter int
}
@@ -92,6 +93,10 @@ func (f *FakeUserService) Search(ctx context.Context, query *user.SearchUsersQue
}
func (f *FakeUserService) Disable(ctx context.Context, cmd *user.DisableUserCommand) error {
if f.DisableFn != nil {
return f.DisableFn(ctx, cmd)
}
return f.ExpectedError
}