diff --git a/pkg/login/auth.go b/pkg/login/auth.go index ad26e68b41e..5de065893c5 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -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) { diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index 33557e03abd..23b994dccb4 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -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 } diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index e4f646e16cb..beb970ed8d0 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -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 diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index 9b6509f7796..cbcacbbbd37 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -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) diff --git a/pkg/middleware/middleware_basic_auth_test.go b/pkg/middleware/middleware_basic_auth_test.go index d6d01c5eb8f..f4cc7ffc15d 100644 --- a/pkg/middleware/middleware_basic_auth_test.go +++ b/pkg/middleware/middleware_basic_auth_test.go @@ -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() diff --git a/pkg/middleware/testing.go b/pkg/middleware/testing.go index 9c928d624c0..c0ef01d5210 100644 --- a/pkg/middleware/testing.go +++ b/pkg/middleware/testing.go @@ -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 } diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 652d919306c..b7cf9448a91 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -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) } diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 648fbfb5223..687fda1ac7a 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -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 } diff --git a/pkg/services/authn/clients/ldap.go b/pkg/services/authn/clients/ldap.go index e4da257d0b8..3948572205b 100644 --- a/pkg/services/authn/clients/ldap.go +++ b/pkg/services/authn/clients/ldap.go @@ -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, diff --git a/pkg/services/authn/clients/ldap_test.go b/pkg/services/authn/clients/ldap_test.go index 2bc5086e232..2477656accc 100644 --- a/pkg/services/authn/clients/ldap_test.go +++ b/pkg/services/authn/clients/ldap_test.go @@ -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 } diff --git a/pkg/services/ldap/ldap.go b/pkg/services/ldap/ldap.go index e7cb7962347..5b952079ee1 100644 --- a/pkg/services/ldap/ldap.go +++ b/pkg/services/ldap/ldap.go @@ -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 {