From f900098cc9f5771c02b6189ba5138547b4f5e6c2 Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Wed, 30 Aug 2023 10:59:35 +0200 Subject: [PATCH] [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 --------- Co-authored-by: Kalle Persson --- pkg/services/authn/authnimpl/service.go | 2 +- pkg/services/authn/clients/ldap.go | 51 ++++++++++-- pkg/services/authn/clients/ldap_test.go | 104 ++++++++++++++++++------ 3 files changed, 124 insertions(+), 33 deletions(-) diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 963212d08af..32fec07dbaf 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -97,7 +97,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/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 }