Skip to content

Commit

Permalink
[v10.0.x] LDAP: Fix user disabling (#74107)
Browse files Browse the repository at this point in the history 
* [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 f900098)

* 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>
  • Loading branch information
@grafana-delivery-bot @gamab
3 people committed Aug 30, 2023
1 parent 3698822 commit 31b1a7b
Show file tree
Hide file tree
Showing 11 changed files with 173 additions and 47 deletions.
6 changes: 4 additions & 2 deletions pkg/login/auth.go
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/login/auth_test.go
View file
Expand Up @@ -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
}
Expand Down
35 changes: 30 additions & 5 deletions pkg/login/ldap_login.go
View file
Expand Up @@ -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"
)

Expand All @@ -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
}
Expand All @@ -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
Expand Down
9 changes: 7 additions & 2 deletions pkg/login/ldap_login_test.go
View file
Expand Up @@ -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"
)

Expand All @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/middleware/middleware_basic_auth_test.go
View file
Expand Up @@ -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()
Expand Down
2 changes: 2 additions & 0 deletions pkg/middleware/testing.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -49,6 +50,7 @@ type scenarioContext struct {
userService *usertest.FakeUserService
oauthTokenService *authtest.FakeOAuthTokenService
orgService *orgtest.FakeOrgService
authInfoService *logintest.AuthInfoServiceFake

req *http.Request
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/authn/authnimpl/service.go
View file
Expand Up @@ -92,7 +92,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)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/services/authn/authnimpl/sync/user_sync.go
View file
Expand Up @@ -3,6 +3,7 @@ package sync
import (
"context"
"errors"
"fmt"
"time"

"github.com/grafana/grafana/pkg/infra/log"
Expand Down Expand Up @@ -228,7 +229,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
}
Expand Down
51 changes: 44 additions & 7 deletions pkg/services/authn/clients/ldap.go
View file
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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,
Expand Down

0 comments on commit 31b1a7b

Please sign in to comment.