diff --git a/pkg/api/ldap_debug.go b/pkg/api/ldap_debug.go index b9064e9bb20..44e194e8d6b 100644 --- a/pkg/api/ldap_debug.go +++ b/pkg/api/ldap_debug.go @@ -1,7 +1,6 @@ package api import ( - "context" "fmt" "net/http" @@ -18,7 +17,6 @@ import ( var ( getLDAPConfig = multildap.GetConfig newLDAP = multildap.New - tokenService = AuthToken{}.TokenService logger = log.New("LDAP.debug") @@ -61,14 +59,6 @@ type LDAPServerDTO struct { Error string `json:"error"` } -type AuthToken struct { - TokenService TokenRevoker `inject:""` -} - -type TokenRevoker interface { - RevokeAllUserTokens(context.Context, int64) error -} - // FetchOrgs fetches the organization(s) information by executing a single query to the database. Then, populating the DTO with the information retrieved. func (user *LDAPUserDTO) FetchOrgs() error { orgIds := []int64{} @@ -199,8 +189,7 @@ func (server *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) Response { user, _, err := ldapServer.User(query.Result.Login) if err != nil { - if err == ldap.ErrCouldNotFindUser { // User was not in the LDAP server - we need to take action: - + if err == multildap.ErrDidNotFindUser { // User was not in the LDAP server - we need to take action: if setting.AdminUser == query.Result.Login { // User is *the* Grafana Admin. We cannot disable it. errMsg := fmt.Sprintf(`Refusing to sync grafana super admin "%s" - it would be disabled`, query.Result.Login) logger.Error(errMsg) @@ -214,13 +203,16 @@ func (server *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) Response { return Error(http.StatusInternalServerError, "Failed to disable the user", err) } - err = tokenService.RevokeAllUserTokens(context.TODO(), userId) + err = server.AuthTokenService.RevokeAllUserTokens(c.Req.Context(), userId) if err != nil { return Error(http.StatusInternalServerError, "Failed to remove session tokens for the user", err) } - return Success("User disabled without any updates in the information") // should this be a success? + return Error(http.StatusBadRequest, "User not found in LDAP. Disabled the user without updating information", nil) // should this be a success? } + + logger.Debug("Failed to sync the user with LDAP", "err", err) + return Error(http.StatusBadRequest, "Something went wrong while finding the user in LDAP", err) } upsertCmd := &models.UpsertUserCommand{ diff --git a/pkg/api/ldap_debug_test.go b/pkg/api/ldap_debug_test.go index ce930b455e7..e065a8f6091 100644 --- a/pkg/api/ldap_debug_test.go +++ b/pkg/api/ldap_debug_test.go @@ -1,7 +1,6 @@ package api import ( - "context" "errors" "net/http" "net/http/httptest" @@ -9,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/multildap" "github.com/grafana/grafana/pkg/setting" @@ -20,9 +20,6 @@ type LDAPMock struct { Results []*models.ExternalUserInfo } -type TokenServiceMock struct { -} - var userSearchResult *models.ExternalUserInfo var userSearchConfig ldap.ServerConfig var userSearchError error @@ -46,10 +43,6 @@ func (m *LDAPMock) User(login string) (*models.ExternalUserInfo, ldap.ServerConf return userSearchResult, userSearchConfig, userSearchError } -func (ts *TokenServiceMock) RevokeAllUserTokens(ctx context.Context, userId int64) error { - return nil -} - //*** // GetUserFromLDAP tests //*** @@ -391,7 +384,7 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string) *scenarioConte setting.LDAPEnabled = true defer func() { setting.LDAPEnabled = ldap }() - hs := &HTTPServer{Cfg: setting.NewCfg()} + hs := &HTTPServer{Cfg: setting.NewCfg(), AuthTokenService: auth.NewFakeUserAuthTokenService()} sc.defaultHandler = Wrap(func(c *models.ReqContext) Response { sc.context = c @@ -490,7 +483,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenGrafanaAdmin(t *testing.T) { return &LDAPMock{} } - userSearchError = ldap.ErrCouldNotFindUser + userSearchError = multildap.ErrDidNotFindUser admin := setting.AdminUser setting.AdminUser = "ldap-daniel" @@ -516,7 +509,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenGrafanaAdmin(t *testing.T) { expected := ` { - "error": "Can't find user in LDAP", + "error": "Did not find a user", "message": "Refusing to sync grafana super admin \"ldap-daniel\" - it would be disabled" } ` @@ -529,8 +522,6 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) { return &ldap.Config{}, nil } - tokenService = &TokenServiceMock{} - newLDAP = func(_ []*ldap.ServerConfig) multildap.IMultiLDAP { return &LDAPMock{} } @@ -563,11 +554,11 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) { sc := postSyncUserWithLDAPContext(t, "/api/admin/ldap/sync/34") - assert.Equal(t, http.StatusOK, sc.resp.Code) + assert.Equal(t, http.StatusBadRequest, sc.resp.Code) expected := ` { - "message": "User disabled without any updates in the information" + "message": "User not found in LDAP. Disabled the user without updating information" } ` diff --git a/public/app/features/admin/UserSyncInfo.tsx b/public/app/features/admin/UserSyncInfo.tsx index 577014d0392..33be5683751 100644 --- a/public/app/features/admin/UserSyncInfo.tsx +++ b/public/app/features/admin/UserSyncInfo.tsx @@ -3,6 +3,7 @@ import { dateTime } from '@grafana/data'; import { LdapUserSyncInfo } from 'app/types'; interface Props { + disableSync: boolean; syncInfo: LdapUserSyncInfo; onSync?: () => void; } @@ -31,21 +32,23 @@ export class UserSyncInfo extends PureComponent { }; render() { - const { syncInfo } = this.props; + const { syncInfo, disableSync } = this.props; const { isSyncing } = this.state; const nextSyncTime = syncInfo.nextSync ? dateTime(syncInfo.nextSync).format(syncTimeFormat) : ''; const prevSyncSuccessful = syncInfo && syncInfo.prevSync; const prevSyncTime = prevSyncSuccessful ? dateTime(syncInfo.prevSync).format(syncTimeFormat) : ''; + const isDisabled = isSyncing || disableSync; return ( <> -

- LDAP - -

+ + +
+ +

LDAP

diff --git a/public/app/features/admin/ldap/LdapUserPage.tsx b/public/app/features/admin/ldap/LdapUserPage.tsx index 582518158e6..c0fcff5f45b 100644 --- a/public/app/features/admin/ldap/LdapUserPage.tsx +++ b/public/app/features/admin/ldap/LdapUserPage.tsx @@ -86,6 +86,10 @@ export class LdapUserPage extends PureComponent { revokeAllSessions(userId); }; + isUserError = (): boolean => { + return !!(this.props.userError && this.props.userError.title); + }; + render() { const { user, ldapUser, userError, navModel, sessions, ldapSyncInfo } = this.props; const { isLoading } = this.state; @@ -102,7 +106,7 @@ export class LdapUserPage extends PureComponent {
- This user is synced via LDAP – all changes must be done in LDAP or mappings. + This user is synced via LDAP – All changes must be done in LDAP or mappings.
{userError && userError.title && (
@@ -115,9 +119,12 @@ export class LdapUserPage extends PureComponent {
)} + {userSyncInfo && ( + + )} + {ldapUser && } {!ldapUser && user && } - {userSyncInfo && } {sessions && (