Skip to content

Commit

Permalink
[LDAP] Disable removed users on login (#74016)
Browse files Browse the repository at this point in the history 
* [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>
  • Loading branch information
@gamab @kalleep
gamab and kalleep committed Aug 30, 2023
1 parent a6ff503 commit f900098
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 33 deletions.
2 changes: 1 addition & 1 deletion pkg/services/authn/authnimpl/service.go
View file
Expand Up @@ -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)
}
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
104 changes: 79 additions & 25 deletions pkg/services/authn/clients/ldap_test.go
View file
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
}

0 comments on commit f900098

Please sign in to comment.