Merge pull request #164 from croessner/features

Features
croessner · Nov 15, 2024 · e2ae37f · e2ae37f
2 parents 97cd5cc + d1ed22b
commit e2ae37f
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 54 deletions.
diff --git a/server/core/auth.go b/server/core/auth.go
@@ -956,7 +956,7 @@ func (a *AuthState) verifyPassword(passDBs []*PassDBMap) (*PassDBResult, error)
 				break
 			}
 		} else {
-			passDBResult, err = authenticateUser(passDBResult, a, passDB)
+			err = processPassDBResult(passDBResult, a, passDB)
 			if err != nil || a.UserFound {
 				break
 			}
@@ -1037,16 +1037,16 @@ func checkAllBackends(configErrors map[global.Backend]error, a *AuthState) (err
 	return err
 }
 
-// authenticateUser updates the passDBResult based on the provided passDB
+// processPassDBResult updates the passDBResult based on the provided passDB
 // and the AuthState object a.
 // If passDBResult is nil, it returns an error of type errors.ErrNoPassDBResult.
 // It then calls the util.DebugModule function to log debug information.
 // Next, it calls the updateAuthentication function to update the fields of a based on the values in passDBResult.
 // If the UserFound field of passDBResult is true, it sets the UserFound field of a to true.
 // Finally, it returns the updated passDBResult and nil error.
-func authenticateUser(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMap) (*PassDBResult, error) {
+func processPassDBResult(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMap) error {
 	if passDBResult == nil {
-		return passDBResult, errors.ErrNoPassDBResult
+		return errors.ErrNoPassDBResult
 	}
 
 	util.DebugModule(
@@ -1057,20 +1057,20 @@ func authenticateUser(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMa
 		"passdb_result", fmt.Sprintf("%+v", *passDBResult),
 	)
 
-	passDBResult = updateAuthentication(a, passDBResult, passDB)
+	updateAuthentication(a, passDBResult, passDB)
 
-	if passDBResult.UserFound {
-		a.UserFound = true
-	}
-
-	return passDBResult, nil
+	return nil
 }
 
 // updateAuthentication updates the fields of the AuthState struct with the values from the PassDBResult struct.
 // It checks if each field in passDBResult is not nil and if it is not nil, it updates the corresponding field in the AuthState struct.
 // It also updates the SourcePassDBBackend and UsedPassDBBackend fields of the AuthState struct with the values from passDBResult.Backend and passDB.backend respectively.
 // It returns the updated PassDBResult struct.
-func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *PassDBMap) *PassDBResult {
+func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *PassDBMap) {
+	if passDBResult.UserFound {
+		a.UserFound = true
+	}
+
 	if passDBResult.AccountField != nil {
 		a.AccountField = passDBResult.AccountField
 	}
@@ -1093,8 +1093,6 @@ func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *Pass
 
 	a.SourcePassDBBackend = passDBResult.Backend
 	a.UsedPassDBBackend = passDB.backend
-
-	return passDBResult
 }
 
 // setStatusCodes sets different status codes for various services.
@@ -1584,10 +1582,6 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b
 	 */
 
 	if a.UserFound {
-		if passDBResult.AccountField != nil {
-			a.AccountField = passDBResult.AccountField
-		}
-
 		accountName, err = a.updateUserAccountInRedis()
 		if err != nil {
 			level.Error(log.Logger).Log(global.LogKeyGUID, a.GUID, global.LogKeyMsg, err.Error())
@@ -1600,9 +1594,9 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b
 		}
 	}
 
-	if useCache {
+	// Note: User-DB queries never contain a password!
+	if !a.NoAuth && useCache {
 		// Make sure the cache backend is in front of the used backend.
-		// If this is a userdb-request, the authentication state is forced to "true" (see verifyPassword()-moethod)
 		if passDBResult.Authenticated {
 			if accountName != "" {
 				if backendPos[global.BackendCache] < backendPos[a.UsedPassDBBackend] {
@@ -1646,7 +1640,10 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b
 						Attributes: a.Attributes,
 					}
 
-					go backend.SaveUserDataToRedis(a.HTTPClientContext, *a.GUID, redisUserKey, config.LoadableConfig.Server.Redis.PosCacheTTL, ppc)
+					// Safety net. Never store empty passwords into ppc.
+					if ppc.Password != "" {
+						go backend.SaveUserDataToRedis(a.HTTPClientContext, *a.GUID, redisUserKey, config.LoadableConfig.Server.Redis.PosCacheTTL, ppc)
+					}
 				}
 			}
 		} else {
@@ -1661,10 +1658,7 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b
 			a.saveFailedPasswordCounterInRedis()
 		}
 
-		// Only passdb requests need reloading
-		if !a.NoAuth {
-			a.getAllPasswordHistories()
-		}
+		a.getAllPasswordHistories()
 	}
 
 	/*

diff --git a/server/core/cache.go b/server/core/cache.go
@@ -38,49 +38,42 @@ func cachePassDB(auth *AuthState) (passDBResult *PassDBResult, err error) {
 
 	passDBResult = &PassDBResult{}
 
-	cacheNames := backend.GetCacheNames(auth.Protocol.Get(), global.CacheAll)
+	accountName, err = auth.updateUserAccountInRedis()
+	if err != nil {
+		return
+	}
 
-	for _, cacheName := range cacheNames.GetStringSlice() {
-		accountName, err = auth.updateUserAccountInRedis()
-		if err != nil {
-			return
-		}
+	if accountName != "" {
+		cacheNames := backend.GetCacheNames(auth.Protocol.Get(), global.CacheAll)
 
-		if accountName != "" {
+		for _, cacheName := range cacheNames.GetStringSlice() {
 			redisPosUserKey := config.LoadableConfig.Server.Redis.Prefix + "ucp:" + cacheName + ":" + accountName
 
 			ppc = &backend.PositivePasswordCache{}
 
-			if _, err = backend.LoadCacheFromRedis(auth.HTTPClientContext, redisPosUserKey, ppc); err != nil {
+			isRedisErr := false
+			if isRedisErr, err = backend.LoadCacheFromRedis(auth.HTTPClientContext, redisPosUserKey, ppc); err != nil {
 				return
 			}
-		}
 
-		if ppc != nil {
-			if auth.NoAuth || ppc.Password == util.GetHash(util.PreparePassword(auth.Password)) {
-				passDBResult.UserFound = true
-				passDBResult.AccountField = ppc.AccountField
-				passDBResult.TOTPSecretField = ppc.TOTPSecretField
-				passDBResult.UniqueUserIDField = ppc.UniqueUserIDField
-				passDBResult.DisplayNameField = ppc.DisplayNameField
-				passDBResult.Authenticated = true
-				passDBResult.Backend = ppc.Backend
-				passDBResult.Attributes = ppc.Attributes
+			// The user was not found for the current cache name
+			if isRedisErr {
+				continue
 			}
-		}
-	}
 
-	if !passDBResult.Authenticated {
-		if key := auth.getPasswordHistoryRedisHashKey(true); key != "" {
-			auth.loadPasswordHistoryFromRedis(key)
-		}
+			passDBResult.UserFound = true
+			passDBResult.AccountField = ppc.AccountField
+			passDBResult.TOTPSecretField = ppc.TOTPSecretField
+			passDBResult.UniqueUserIDField = ppc.UniqueUserIDField
+			passDBResult.DisplayNameField = ppc.DisplayNameField
+			passDBResult.Backend = ppc.Backend
+			passDBResult.Attributes = ppc.Attributes
 
-		// Prevent password lookups for already known wrong passwords.
-		if auth.PasswordHistory != nil {
-			passwordHash := util.GetHash(util.PreparePassword(auth.Password))
-			if _, foundPassword := (*auth.PasswordHistory)[passwordHash]; foundPassword {
-				passDBResult.UserFound = true
+			if auth.NoAuth || ppc.Password == util.GetHash(util.PreparePassword(auth.Password)) {
+				passDBResult.Authenticated = true
 			}
+
+			break
 		}
 	}