LDAP: Fixing sync issues (#19446)

The arching goal of this commit is to enable single user
synchronisation with LDAP. Also, it included minor fixes of style,
error messages and minor bug fixing.

The changes are:

- bug: The `multildap` package has its own errors when the user is
  not found. We fixed the conditional branch on this error by asserting
on the `multildap` errors as opposed to the `ldap` one

- bug: The previous interface usage of `RevokeAllUserTokens` did not
  work as expected. This replaces the manual injection of the service by
leveraging the service injected as part of the `server` struct.

- chore: Better error messages around not finding the user in LDAP.

- fix: Enable the single sync button and disable it when we receive an
  error from LDAP. Please note, that you can enable it by dispatching
the error. This allows you to try again without having to reload the
page.

- fix: Move the sync info to the top, then move the sync button above
  that information and clearfix to have more harmony with the UI.

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{

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"
 	}
 	`
 

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<Props, State> {
   };
 
   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 (
       <>
-        <h3 className="page-heading">
-          LDAP
-          <button className={`btn btn-secondary pull-right`} onClick={this.handleSyncClick} hidden={true}>
-            <span className="btn-title">Sync user</span>
-            {isSyncing && <i className="fa fa-spinner fa-fw fa-spin run-icon" />}
-          </button>
-        </h3>
+        <button className={`btn btn-secondary pull-right`} onClick={this.handleSyncClick} disabled={isDisabled}>
+          <span className="btn-title">Sync user</span>
+          {isSyncing && <i className="fa fa-spinner fa-fw fa-spin run-icon" />}
+        </button>
+
+        <div className="clearfix" />
+
+        <h3 className="page-heading">LDAP</h3>
         <div className="gf-form-group">
           <div className="gf-form">
             <table className="filter-table form-inline">

public/app/features/admin/ldap/LdapUserPage.tsx

@@ -86,6 +86,10 @@ export class LdapUserPage extends PureComponent<Props, State> {
     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<Props, State> {
       <Page navModel={navModel}>
         <Page.Contents isLoading={isLoading}>
           <div className="grafana-info-box">
-            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.
           </div>
           {userError && userError.title && (
             <div className="gf-form-group">
@@ -115,9 +119,12 @@ export class LdapUserPage extends PureComponent<Props, State> {
             </div>
           )}
 
+          {userSyncInfo && (
+            <UserSyncInfo syncInfo={userSyncInfo} onSync={this.onSyncUser} disableSync={this.isUserError()} />
+          )}
+
           {ldapUser && <LdapUserInfo ldapUser={ldapUser} />}
           {!ldapUser && user && <UserInfo user={user} />}
-          {userSyncInfo && <UserSyncInfo syncInfo={userSyncInfo} onSync={this.onSyncUser} />}
 
           {sessions && (
             <UserSessions