diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index 7641ec51856..755f2a5a05c 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -48,8 +48,8 @@ type ClientParams struct { SyncUser bool // AllowSignUp Adds identity to DB if it doesn't exist when, only work if SyncUser is enabled AllowSignUp bool - // EnableDisabledUsers will enable disabled user, only work if SyncUser is enabled - EnableDisabledUsers bool + // EnableUser will ensure the user is enabled, only work if SyncUser is enabled + EnableUser bool // FetchSyncedUser ensure that all required information is added to the identity FetchSyncedUser bool // SyncTeams will sync the groups from identity to teams in grafana, enterprise only feature diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 432b0e53648..ea9e57c96f5 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -154,7 +154,7 @@ func ProvideService( userSyncService := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService) orgUserSyncService := sync.ProvideOrgSync(userService, orgService, accessControlService) s.RegisterPostAuthHook(userSyncService.SyncUserHook, 10) - s.RegisterPostAuthHook(userSyncService.EnableDisabledUserHook, 20) + s.RegisterPostAuthHook(userSyncService.EnableUserHook, 20) s.RegisterPostAuthHook(orgUserSyncService.SyncOrgRolesHook, 30) s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 120) diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index bdd7f4bd4b8..97bc58482a8 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -163,12 +163,8 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit return nil } -func (s *UserSync) EnableDisabledUserHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { - if !identity.ClientParams.EnableDisabledUsers { - return nil - } - - if !identity.IsDisabled { +func (s *UserSync) EnableUserHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { + if !identity.ClientParams.EnableUser { return nil } diff --git a/pkg/services/authn/authnimpl/sync/user_sync_test.go b/pkg/services/authn/authnimpl/sync/user_sync_test.go index bbaf662e74b..eb851917956 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync_test.go +++ b/pkg/services/authn/authnimpl/sync/user_sync_test.go @@ -349,9 +349,9 @@ func TestUserSync_SyncUserHook(t *testing.T) { AuthenticatedBy: "oauth", AuthID: "2032", ClientParams: authn.ClientParams{ - SyncUser: true, - AllowSignUp: true, - EnableDisabledUsers: true, + SyncUser: true, + AllowSignUp: true, + EnableUser: true, LookUpParams: login.UserLookupParams{ UserID: nil, Email: ptrString("test_create"), @@ -370,9 +370,9 @@ func TestUserSync_SyncUserHook(t *testing.T) { AuthID: "2032", IsGrafanaAdmin: ptrBool(true), ClientParams: authn.ClientParams{ - SyncUser: true, - AllowSignUp: true, - EnableDisabledUsers: true, + SyncUser: true, + AllowSignUp: true, + EnableUser: true, LookUpParams: login.UserLookupParams{ UserID: nil, Email: ptrString("test_create"), @@ -398,8 +398,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { IsDisabled: false, IsGrafanaAdmin: ptrBool(true), ClientParams: authn.ClientParams{ - SyncUser: true, - EnableDisabledUsers: true, + SyncUser: true, + EnableUser: true, LookUpParams: login.UserLookupParams{ UserID: ptrInt64(3), Email: nil, @@ -417,8 +417,8 @@ func TestUserSync_SyncUserHook(t *testing.T) { IsDisabled: false, IsGrafanaAdmin: ptrBool(true), ClientParams: authn.ClientParams{ - SyncUser: true, - EnableDisabledUsers: true, + SyncUser: true, + EnableUser: true, LookUpParams: login.UserLookupParams{ UserID: ptrInt64(3), Email: nil, @@ -486,16 +486,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) { identity: &authn.Identity{ ID: authn.NamespacedID(authn.NamespaceUser, 1), IsDisabled: true, - ClientParams: authn.ClientParams{EnableDisabledUsers: false}, - }, - enableUser: false, - }, - { - desc: "should skip if identity is not disabled", - identity: &authn.Identity{ - ID: authn.NamespacedID(authn.NamespaceUser, 1), - IsDisabled: false, - ClientParams: authn.ClientParams{EnableDisabledUsers: true}, + ClientParams: authn.ClientParams{EnableUser: false}, }, enableUser: false, }, @@ -504,7 +495,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) { identity: &authn.Identity{ ID: authn.NamespacedID(authn.NamespaceAPIKey, 1), IsDisabled: true, - ClientParams: authn.ClientParams{EnableDisabledUsers: true}, + ClientParams: authn.ClientParams{EnableUser: true}, }, enableUser: false, }, @@ -513,7 +504,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) { identity: &authn.Identity{ ID: authn.NamespacedID(authn.NamespaceUser, 1), IsDisabled: true, - ClientParams: authn.ClientParams{EnableDisabledUsers: true}, + ClientParams: authn.ClientParams{EnableUser: true}, }, enableUser: true, }, @@ -529,7 +520,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) { } s := UserSync{userService: userSvc} - err := s.EnableDisabledUserHook(context.Background(), tt.identity, nil) + err := s.EnableUserHook(context.Background(), tt.identity, nil) require.NoError(t, err) assert.Equal(t, tt.enableUser, called) }) diff --git a/pkg/services/authn/clients/ext_jwt_test.go b/pkg/services/authn/clients/ext_jwt_test.go index a2f917a8b88..33361cb58b6 100644 --- a/pkg/services/authn/clients/ext_jwt_test.go +++ b/pkg/services/authn/clients/ext_jwt_test.go @@ -175,13 +175,13 @@ func TestExtendedJWT_Authenticate(t *testing.T) { }, }, ClientParams: authn.ClientParams{ - SyncUser: false, - AllowSignUp: false, - FetchSyncedUser: false, - EnableDisabledUsers: false, - SyncOrgRoles: false, - SyncTeams: false, - SyncPermissions: false, + SyncUser: false, + AllowSignUp: false, + FetchSyncedUser: false, + EnableUser: false, + SyncOrgRoles: false, + SyncTeams: false, + SyncPermissions: false, LookUpParams: login.UserLookupParams{ UserID: nil, Email: nil, diff --git a/pkg/services/authn/clients/grafana_test.go b/pkg/services/authn/clients/grafana_test.go index 5fc79b3c478..53dab4486e4 100644 --- a/pkg/services/authn/clients/grafana_test.go +++ b/pkg/services/authn/clients/grafana_test.go @@ -112,7 +112,7 @@ func TestGrafana_AuthenticateProxy(t *testing.T) { assert.Equal(t, tt.expectedIdentity.ClientParams.SyncUser, identity.ClientParams.SyncUser) assert.Equal(t, tt.expectedIdentity.ClientParams.AllowSignUp, identity.ClientParams.AllowSignUp) assert.Equal(t, tt.expectedIdentity.ClientParams.SyncTeams, identity.ClientParams.SyncTeams) - assert.Equal(t, tt.expectedIdentity.ClientParams.EnableDisabledUsers, identity.ClientParams.EnableDisabledUsers) + assert.Equal(t, tt.expectedIdentity.ClientParams.EnableUser, identity.ClientParams.EnableUser) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Email, identity.ClientParams.LookUpParams.Email) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) diff --git a/pkg/services/authn/clients/ldap.go b/pkg/services/authn/clients/ldap.go index 2745ab64fd1..d505979cb9c 100644 --- a/pkg/services/authn/clients/ldap.go +++ b/pkg/services/authn/clients/ldap.go @@ -107,7 +107,7 @@ func (c *LDAP) disableUser(ctx context.Context, username string) (*authn.Identit } func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) *authn.Identity { - id := &authn.Identity{ + return &authn.Identity{ OrgID: orgID, OrgRoles: info.OrgRoles, Login: info.Login, @@ -118,25 +118,17 @@ func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) * AuthID: info.AuthId, Groups: info.Groups, ClientParams: authn.ClientParams{ - SyncUser: true, - SyncTeams: true, - EnableDisabledUsers: true, - FetchSyncedUser: true, - SyncPermissions: true, - SyncOrgRoles: !c.cfg.LDAPSkipOrgRoleSync, - AllowSignUp: c.cfg.LDAPAllowSignup, + SyncUser: true, + SyncTeams: true, + EnableUser: true, + FetchSyncedUser: true, + SyncPermissions: true, + SyncOrgRoles: !c.cfg.LDAPSkipOrgRoleSync, + AllowSignUp: c.cfg.LDAPAllowSignup, LookUpParams: login.UserLookupParams{ Login: &info.Login, Email: &info.Email, }, }, } - - // The ldap service is not aware of the internal state of the user. Fetching the user - // from the store to know if that user is disabled or not, is almost as costly as - // running an update systematically. We are setting IsDisabled to true so that the - // EnableDisabledUserHook force-enable that user. - id.IsDisabled = true - - return id } diff --git a/pkg/services/authn/clients/ldap_test.go b/pkg/services/authn/clients/ldap_test.go index cd45ad8987e..30d9a79f4a7 100644 --- a/pkg/services/authn/clients/ldap_test.go +++ b/pkg/services/authn/clients/ldap_test.go @@ -60,14 +60,13 @@ func TestLDAP_AuthenticateProxy(t *testing.T) { AuthenticatedBy: login.LDAPAuthModule, AuthID: "123", Groups: []string{"1", "2"}, - IsDisabled: true, // Users are marked as disabled to force enablement on successful login ClientParams: authn.ClientParams{ - SyncUser: true, - SyncTeams: true, - EnableDisabledUsers: true, - FetchSyncedUser: true, - SyncOrgRoles: true, - SyncPermissions: true, + SyncUser: true, + SyncTeams: true, + EnableUser: true, + FetchSyncedUser: true, + SyncOrgRoles: true, + SyncPermissions: true, LookUpParams: login.UserLookupParams{ Email: strPtr("test@test.com"), Login: strPtr("test"), @@ -130,14 +129,13 @@ func TestLDAP_AuthenticatePassword(t *testing.T) { AuthenticatedBy: login.LDAPAuthModule, AuthID: "123", Groups: []string{"1", "2"}, - IsDisabled: true, // Users are marked as disabled to force enablement on successful login ClientParams: authn.ClientParams{ - SyncUser: true, - SyncTeams: true, - EnableDisabledUsers: true, - FetchSyncedUser: true, - SyncOrgRoles: true, - SyncPermissions: true, + SyncUser: true, + SyncTeams: true, + EnableUser: true, + FetchSyncedUser: true, + SyncOrgRoles: true, + SyncPermissions: true, LookUpParams: login.UserLookupParams{ Email: strPtr("test@test.com"), Login: strPtr("test"), diff --git a/pkg/services/authn/clients/oauth_test.go b/pkg/services/authn/clients/oauth_test.go index d812bdcffdb..30da79459c0 100644 --- a/pkg/services/authn/clients/oauth_test.go +++ b/pkg/services/authn/clients/oauth_test.go @@ -227,7 +227,7 @@ func TestOAuth_Authenticate(t *testing.T) { assert.Equal(t, tt.expectedIdentity.ClientParams.SyncUser, identity.ClientParams.SyncUser) assert.Equal(t, tt.expectedIdentity.ClientParams.AllowSignUp, identity.ClientParams.AllowSignUp) assert.Equal(t, tt.expectedIdentity.ClientParams.SyncTeams, identity.ClientParams.SyncTeams) - assert.Equal(t, tt.expectedIdentity.ClientParams.EnableDisabledUsers, identity.ClientParams.EnableDisabledUsers) + assert.Equal(t, tt.expectedIdentity.ClientParams.EnableUser, identity.ClientParams.EnableUser) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Email, identity.ClientParams.LookUpParams.Email) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) diff --git a/pkg/services/ldap/api/service.go b/pkg/services/ldap/api/service.go index bf7497317d8..2c288c60fb1 100644 --- a/pkg/services/ldap/api/service.go +++ b/pkg/services/ldap/api/service.go @@ -326,11 +326,11 @@ func (s *Service) identityFromLDAPUser(user *login.ExternalUserInfo) *authn.Iden AuthID: user.AuthId, Groups: user.Groups, ClientParams: authn.ClientParams{ - SyncUser: true, - SyncTeams: true, - EnableDisabledUsers: true, - SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync, - AllowSignUp: s.cfg.LDAPAllowSignup, + SyncUser: true, + SyncTeams: true, + EnableUser: true, + SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync, + AllowSignUp: s.cfg.LDAPAllowSignup, LookUpParams: login.UserLookupParams{ UserID: &user.UserId, },