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

[v10.1.x] LDAP: Fix user disabling (#74109)

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

* [LDAP] Disable removed users on login

* Fix tests

* Add test for user disabling

* Add tests for disabling user behind auth proxy

* Linting.

* Rename setup func

* Account for reviews comments

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>

---------

Co-authored-by: Kalle Persson <kalle.persson@grafana.com>
(cherry picked from commit f900098cc9)

* manual backport of #74016

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

* Fix middleware test

---------

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Gabriel MABILLE <gabriel.mabille@grafana.com>
This commit is contained in:
grafana-delivery-bot[bot]
2023-08-30 16:51:25 +02:00
committed by GitHub
co-authored by Gabriel MABILLE Gabriel MABILLE
parent f99cb27617
commit 473f1808b4
11 changed files with 173 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/login/auth.go
+4 -2
View File
@@ -37,16 +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, cfg *setting.Cfg) *AuthenticatorService {
userService user.Service, authInfoService login.AuthInfoService, cfg *setting.Cfg) *AuthenticatorService {
a := &AuthenticatorService{
loginService: loginService,
loginAttemptService: loginAttemptService,
userService: userService,
authInfoService: authInfoService,
cfg: cfg,
}
return a
@@ -78,7 +80,7 @@ func (a *AuthenticatorService) AuthenticateUser(ctx context.Context, query *logi
return err
}
ldapEnabled, ldapErr := loginUsingLDAP(ctx, query, a.loginService, a.cfg)
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
+1 -1
View File
@@ -209,7 +209,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, _ *setting.Cfg) (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
+30 -5
View File
@@ -9,6 +9,7 @@ import (
"github.com/grafana/grafana/pkg/services/ldap"
"github.com/grafana/grafana/pkg/services/ldap/multildap"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
@@ -24,7 +25,7 @@ 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, cfg *setting.Cfg) (bool, error) {
loginService login.Service, userService user.Service, authInfoService login.AuthInfoService, cfg *setting.Cfg) (bool, error) {
if !cfg.LDAPAuthEnabled {
return false, nil
}
@@ -37,13 +38,37 @@ var loginUsingLDAP = func(ctx context.Context, query *login.LoginUserQuery,
externalUser, err := newLDAP(config.Servers, cfg).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}
authinfo, 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", authinfo.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
+7 -2
View File
@@ -12,6 +12,7 @@ import (
"github.com/grafana/grafana/pkg/services/ldap/multildap"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
)
@@ -32,7 +33,9 @@ func TestLoginUsingLDAP(t *testing.T) {
}
loginService := &logintest.LoginServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService, cfg)
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)
@@ -45,7 +48,9 @@ func TestLoginUsingLDAP(t *testing.T) {
sc.withLoginResult(false)
loginService := &logintest.LoginServiceFake{}
enabled, err := loginUsingLDAP(context.Background(), sc.loginUserQuery, loginService, cfg)
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, sc.cfg)
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
@@ -94,7 +94,7 @@ func ProvideService(
var proxyClients []authn.ProxyClient
var passwordClients []authn.PasswordClient
if s.cfg.LDAPAuthEnabled {
ldap := clients.ProvideLDAP(cfg, ldapService)
ldap := clients.ProvideLDAP(cfg, ldapService, userService, authInfoService)
proxyClients = append(proxyClients, ldap)
passwordClients = append(passwordClients, ldap)
}
pkg/services/authn/authnimpl/sync/user_sync.go
+2 -1
View File
@@ -3,6 +3,7 @@ package sync
import (
"context"
"errors"
"fmt"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/authn"
@@ -234,7 +235,7 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
}
if needsUpdate {
s.log.FromContext(ctx).Debug("Syncing user info", "id", id.ID, "update", updateCmd)
s.log.FromContext(ctx).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,9 +4,11 @@ import (
"context"
"errors"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/ldap/multildap"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
@@ -18,13 +20,16 @@ type ldapService interface {
User(username string) (*login.ExternalUserInfo, error)
}
func ProvideLDAP(cfg *setting.Cfg, ldapService ldapService) *LDAP {
return &LDAP{cfg, ldapService}
func ProvideLDAP(cfg *setting.Cfg, ldapService ldapService, userService user.Service, authInfoService login.AuthInfoService) *LDAP {
return &LDAP{cfg, log.New("authn.ldap"), ldapService, 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) String() string {
@@ -34,7 +39,7 @@ func (c *LDAP) String() string {
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 {
@@ -51,8 +56,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
@@ -69,6 +73,39 @@ func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, usern
return c.identityFromLDAPInfo(r.OrgID, info), 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}
authinfo, 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", authinfo.AuthId)
if errDisable := c.userService.Disable(ctx, &user.DisableUserCommand{UserID: dbUser.ID, IsDisabled: true}); errDisable != nil {
return nil, errDisable
}
return nil, retErr
}
func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) *authn.Identity {
return &authn.Identity{
OrgID: orgID,
pkg/services/authn/clients/ldap_test.go
+79 -25
View File
@@ -6,26 +6,39 @@ 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/ldap/multildap"
"github.com/grafana/grafana/pkg/services/ldap/service"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/logintest"
"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",
@@ -65,32 +78,35 @@ 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: &service.LDAPFakeService{ExpectedUser: tt.expectedLDAPInfo, ExpectedError: 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",
@@ -140,20 +156,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: &service.LDAPFakeService{ExpectedUser: tt.expectedLDAPInfo, ExpectedError: 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: &service.LDAPFakeService{ExpectedUser: tt.expectedLDAPInfo, ExpectedError: tt.expectedLDAPErr},
userService: userService,
authInfoService: authInfoService,
}
return c
}
func strPtr(s string) *string {
return &s
}
pkg/services/ldap/ldap.go
+2 -2
View File
@@ -288,8 +288,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 {