IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

AuthN: Change EnableDisabledUserHook to EnableUserHook (#75248)

Browse Source 
* Replace the enable disable user hook by a hook that systematically enable users

* Fix tests

* Remove the skip test
This commit is contained in:
Gabriel MABILLE
2023-09-27 11:16:53 +02:00
committed by GitHub
parent a3d0dfcbcc
commit 0ed649b108
10 changed files with 53 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
pkg/services/authn/authn.go
View File

@@ -48,8 +48,8 @@ type ClientParams struct {
SyncUser bool SyncUser bool
// AllowSignUp Adds identity to DB if it doesn't exist when, only work if SyncUser is enabled // AllowSignUp Adds identity to DB if it doesn't exist when, only work if SyncUser is enabled
AllowSignUp bool AllowSignUp bool
// EnableDisabledUsers will enable disabled user, only work if SyncUser is enabled // EnableUser will ensure the user is enabled, only work if SyncUser is enabled
EnableDisabledUsers bool EnableUser bool
// FetchSyncedUser ensure that all required information is added to the identity // FetchSyncedUser ensure that all required information is added to the identity
FetchSyncedUser bool FetchSyncedUser bool
// SyncTeams will sync the groups from identity to teams in grafana, enterprise only feature // SyncTeams will sync the groups from identity to teams in grafana, enterprise only feature

2
pkg/services/authn/authnimpl/service.go
View File

@@ -154,7 +154,7 @@ func ProvideService(
userSyncService := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService) userSyncService := sync.ProvideUserSync(userService, userProtectionService, authInfoService, quotaService)
orgUserSyncService := sync.ProvideOrgSync(userService, orgService, accessControlService) orgUserSyncService := sync.ProvideOrgSync(userService, orgService, accessControlService)
s.RegisterPostAuthHook(userSyncService.SyncUserHook, 10) s.RegisterPostAuthHook(userSyncService.SyncUserHook, 10)
s.RegisterPostAuthHook(userSyncService.EnableDisabledUserHook, 20) s.RegisterPostAuthHook(userSyncService.EnableUserHook, 20)
s.RegisterPostAuthHook(orgUserSyncService.SyncOrgRolesHook, 30) s.RegisterPostAuthHook(orgUserSyncService.SyncOrgRolesHook, 30)
s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 120) s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 120)

8
pkg/services/authn/authnimpl/sync/user_sync.go
View File

@@ -163,12 +163,8 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit
return nil return nil
} }
func (s *UserSync) EnableDisabledUserHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { func (s *UserSync) EnableUserHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
if !identity.ClientParams.EnableDisabledUsers { if !identity.ClientParams.EnableUser {
return nil
}
if !identity.IsDisabled {
return nil return nil
} }

37
pkg/services/authn/authnimpl/sync/user_sync_test.go
View File

@@ -349,9 +349,9 @@ func TestUserSync_SyncUserHook(t *testing.T) {
AuthenticatedBy: "oauth", AuthenticatedBy: "oauth",
AuthID: "2032", AuthID: "2032",
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
AllowSignUp: true, AllowSignUp: true,
EnableDisabledUsers: true, EnableUser: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: nil, UserID: nil,
Email: ptrString("test_create"), Email: ptrString("test_create"),
@@ -370,9 +370,9 @@ func TestUserSync_SyncUserHook(t *testing.T) {
AuthID: "2032", AuthID: "2032",
IsGrafanaAdmin: ptrBool(true), IsGrafanaAdmin: ptrBool(true),
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
AllowSignUp: true, AllowSignUp: true,
EnableDisabledUsers: true, EnableUser: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: nil, UserID: nil,
Email: ptrString("test_create"), Email: ptrString("test_create"),
@@ -398,8 +398,8 @@ func TestUserSync_SyncUserHook(t *testing.T) {
IsDisabled: false, IsDisabled: false,
IsGrafanaAdmin: ptrBool(true), IsGrafanaAdmin: ptrBool(true),
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
EnableDisabledUsers: true, EnableUser: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: ptrInt64(3), UserID: ptrInt64(3),
Email: nil, Email: nil,
@@ -417,8 +417,8 @@ func TestUserSync_SyncUserHook(t *testing.T) {
IsDisabled: false, IsDisabled: false,
IsGrafanaAdmin: ptrBool(true), IsGrafanaAdmin: ptrBool(true),
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
EnableDisabledUsers: true, EnableUser: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: ptrInt64(3), UserID: ptrInt64(3),
Email: nil, Email: nil,
@@ -486,16 +486,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
identity: &authn.Identity{ identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1), ID: authn.NamespacedID(authn.NamespaceUser, 1),
IsDisabled: true, IsDisabled: true,
ClientParams: authn.ClientParams{EnableDisabledUsers: false}, ClientParams: authn.ClientParams{EnableUser: 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},
}, },
enableUser: false, enableUser: false,
}, },
@@ -504,7 +495,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
identity: &authn.Identity{ identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceAPIKey, 1), ID: authn.NamespacedID(authn.NamespaceAPIKey, 1),
IsDisabled: true, IsDisabled: true,
ClientParams: authn.ClientParams{EnableDisabledUsers: true}, ClientParams: authn.ClientParams{EnableUser: true},
}, },
enableUser: false, enableUser: false,
}, },
@@ -513,7 +504,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
identity: &authn.Identity{ identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1), ID: authn.NamespacedID(authn.NamespaceUser, 1),
IsDisabled: true, IsDisabled: true,
ClientParams: authn.ClientParams{EnableDisabledUsers: true}, ClientParams: authn.ClientParams{EnableUser: true},
}, },
enableUser: true, enableUser: true,
}, },
@@ -529,7 +520,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
} }
s := UserSync{userService: userSvc} s := UserSync{userService: userSvc}
err := s.EnableDisabledUserHook(context.Background(), tt.identity, nil) err := s.EnableUserHook(context.Background(), tt.identity, nil)
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, tt.enableUser, called) assert.Equal(t, tt.enableUser, called)
}) })

14
pkg/services/authn/clients/ext_jwt_test.go
View File

@@ -175,13 +175,13 @@ func TestExtendedJWT_Authenticate(t *testing.T) {
}, },
}, },
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: false, SyncUser: false,
AllowSignUp: false, AllowSignUp: false,
FetchSyncedUser: false, FetchSyncedUser: false,
EnableDisabledUsers: false, EnableUser: false,
SyncOrgRoles: false, SyncOrgRoles: false,
SyncTeams: false, SyncTeams: false,
SyncPermissions: false, SyncPermissions: false,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: nil, UserID: nil,
Email: nil, Email: nil,

2
pkg/services/authn/clients/grafana_test.go
View File

@@ -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.SyncUser, identity.ClientParams.SyncUser)
assert.Equal(t, tt.expectedIdentity.ClientParams.AllowSignUp, identity.ClientParams.AllowSignUp) 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.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.Email, identity.ClientParams.LookUpParams.Email)
assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login)

24
pkg/services/authn/clients/ldap.go
View File

@@ -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 { func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) *authn.Identity {
id := &authn.Identity{ return &authn.Identity{
OrgID: orgID, OrgID: orgID,
OrgRoles: info.OrgRoles, OrgRoles: info.OrgRoles,
Login: info.Login, Login: info.Login,
@@ -118,25 +118,17 @@ func (c *LDAP) identityFromLDAPInfo(orgID int64, info *login.ExternalUserInfo) *
AuthID: info.AuthId, AuthID: info.AuthId,
Groups: info.Groups, Groups: info.Groups,
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
SyncTeams: true, SyncTeams: true,
EnableDisabledUsers: true, EnableUser: true,
FetchSyncedUser: true, FetchSyncedUser: true,
SyncPermissions: true, SyncPermissions: true,
SyncOrgRoles: !c.cfg.LDAPSkipOrgRoleSync, SyncOrgRoles: !c.cfg.LDAPSkipOrgRoleSync,
AllowSignUp: c.cfg.LDAPAllowSignup, AllowSignUp: c.cfg.LDAPAllowSignup,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
Login: &info.Login, Login: &info.Login,
Email: &info.Email, 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
} }

26
pkg/services/authn/clients/ldap_test.go
View File

@@ -60,14 +60,13 @@ func TestLDAP_AuthenticateProxy(t *testing.T) {
AuthenticatedBy: login.LDAPAuthModule, AuthenticatedBy: login.LDAPAuthModule,
AuthID: "123", AuthID: "123",
Groups: []string{"1", "2"}, Groups: []string{"1", "2"},
IsDisabled: true, // Users are marked as disabled to force enablement on successful login
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
SyncTeams: true, SyncTeams: true,
EnableDisabledUsers: true, EnableUser: true,
FetchSyncedUser: true, FetchSyncedUser: true,
SyncOrgRoles: true, SyncOrgRoles: true,
SyncPermissions: true, SyncPermissions: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
Email: strPtr("test@test.com"), Email: strPtr("test@test.com"),
Login: strPtr("test"), Login: strPtr("test"),
@@ -130,14 +129,13 @@ func TestLDAP_AuthenticatePassword(t *testing.T) {
AuthenticatedBy: login.LDAPAuthModule, AuthenticatedBy: login.LDAPAuthModule,
AuthID: "123", AuthID: "123",
Groups: []string{"1", "2"}, Groups: []string{"1", "2"},
IsDisabled: true, // Users are marked as disabled to force enablement on successful login
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
SyncTeams: true, SyncTeams: true,
EnableDisabledUsers: true, EnableUser: true,
FetchSyncedUser: true, FetchSyncedUser: true,
SyncOrgRoles: true, SyncOrgRoles: true,
SyncPermissions: true, SyncPermissions: true,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
Email: strPtr("test@test.com"), Email: strPtr("test@test.com"),
Login: strPtr("test"), Login: strPtr("test"),

2
pkg/services/authn/clients/oauth_test.go
View File

@@ -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.SyncUser, identity.ClientParams.SyncUser)
assert.Equal(t, tt.expectedIdentity.ClientParams.AllowSignUp, identity.ClientParams.AllowSignUp) 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.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.Email, identity.ClientParams.LookUpParams.Email)
assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login) assert.EqualValues(t, tt.expectedIdentity.ClientParams.LookUpParams.Login, identity.ClientParams.LookUpParams.Login)

10
pkg/services/ldap/api/service.go
View File

@@ -326,11 +326,11 @@ func (s *Service) identityFromLDAPUser(user *login.ExternalUserInfo) *authn.Iden
AuthID: user.AuthId, AuthID: user.AuthId,
Groups: user.Groups, Groups: user.Groups,
ClientParams: authn.ClientParams{ ClientParams: authn.ClientParams{
SyncUser: true, SyncUser: true,
SyncTeams: true, SyncTeams: true,
EnableDisabledUsers: true, EnableUser: true,
SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync, SyncOrgRoles: !s.cfg.LDAPSkipOrgRoleSync,
AllowSignUp: s.cfg.LDAPAllowSignup, AllowSignUp: s.cfg.LDAPAllowSignup,
LookUpParams: login.UserLookupParams{ LookUpParams: login.UserLookupParams{
UserID: &user.UserId, UserID: &user.UserId,
}, },