Fix session cleanup test (#126966) (#127087)

(cherry picked from commit 2f83c65)

# Conflicts:
#	x-pack/plugins/security/server/session_management/session_index.ts

x-pack/plugins/security/server/session_management/session_index.test.ts

@@ -270,6 +270,7 @@ describe('Session index', () => {
       expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
       expect(mockElasticsearchClient.bulk).not.toHaveBeenCalled();
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).not.toHaveBeenCalled(); // since the search failed, we don't refresh the index
     });
 
     it('throws if bulk delete call to Elasticsearch fails', async () => {
@@ -282,7 +283,20 @@ describe('Session index', () => {
       expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1);
       expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
       expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1);
-      expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); // since we attempted to delete sessions, we still refresh the index
+    });
+
+    it('does not throw if index refresh call to Elasticsearch fails', async () => {
+      const failureReason = new errors.ResponseError(
+        securityMock.createApiResponse(securityMock.createApiResponse({ body: { type: 'Uh oh.' } }))
+      );
+      mockElasticsearchClient.indices.refresh.mockRejectedValue(failureReason);
+
+      await sessionIndex.cleanUp();
+      expect(mockElasticsearchClient.openPointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); // since we attempted to delete sessions, we still refresh the index
     });
 
     it('when neither `lifespan` nor `idleTimeout` is configured', async () => {
@@ -443,6 +457,7 @@ describe('Session index', () => {
         }
       );
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('when only `idleTimeout` is configured', async () => {
@@ -529,6 +544,7 @@ describe('Session index', () => {
         }
       );
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('when both `lifespan` and `idleTimeout` are configured', async () => {
@@ -625,6 +641,7 @@ describe('Session index', () => {
         }
       );
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('when both `lifespan` and `idleTimeout` are configured and multiple providers are enabled', async () => {
@@ -769,6 +786,7 @@ describe('Session index', () => {
         }
       );
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('should clean up sessions in batches of 10,000', async () => {
@@ -788,6 +806,7 @@ describe('Session index', () => {
       expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(2);
       expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(2);
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('should limit number of batches to 10', async () => {
@@ -805,6 +824,7 @@ describe('Session index', () => {
       expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(10);
       expect(mockElasticsearchClient.bulk).toHaveBeenCalledTimes(10);
       expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1);
+      expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1);
     });
 
     it('should log audit event', async () => {

x-pack/plugins/security/server/session_management/session_index.ts

@@ -447,20 +447,21 @@ export class SessionIndex {
    * Trigger a removal of any outdated session values.
    */
   async cleanUp() {
-    this.options.logger.debug(`Running cleanup routine.`);
+    const { auditLogger, elasticsearchClient, logger } = this.options;
+    logger.debug(`Running cleanup routine.`);
 
+    let error: Error | undefined;
+    let indexNeedsRefresh = false;
     try {
       for await (const sessionValues of this.getSessionValuesInBatches()) {
         const operations: Array<Required<Pick<BulkOperationContainer, 'delete'>>> = [];
         sessionValues.forEach(({ _id, _source }) => {
           const { usernameHash, provider } = _source!;
-          this.options.auditLogger.log(
-            sessionCleanupEvent({ sessionId: _id, usernameHash, provider })
-          );
+          auditLogger.log(sessionCleanupEvent({ sessionId: _id, usernameHash, provider }));
           operations.push({ delete: { _id } });
         });
         if (operations.length > 0) {
-          const { body: bulkResponse } = await this.options.elasticsearchClient.bulk(
+          const { body: bulkResponse } = await elasticsearchClient.bulk(
             {
               index: this.indexName,
               operations,
@@ -474,24 +475,40 @@ export class SessionIndex {
               0
             );
             if (errorCount < bulkResponse.items.length) {
-              this.options.logger.warn(
+              logger.warn(
                 `Failed to clean up ${errorCount} of ${bulkResponse.items.length} invalid or expired sessions. The remaining sessions were cleaned up successfully.`
               );
+              indexNeedsRefresh = true;
             } else {
-              this.options.logger.error(
+              logger.error(
                 `Failed to clean up ${bulkResponse.items.length} invalid or expired sessions.`
               );
             }
           } else {
-            this.options.logger.debug(
-              `Cleaned up ${bulkResponse.items.length} invalid or expired sessions.`
-            );
+            logger.debug(`Cleaned up ${bulkResponse.items.length} invalid or expired sessions.`);
+            indexNeedsRefresh = true;
           }
         }
       }
     } catch (err) {
-      this.options.logger.error(`Failed to clean up sessions: ${err.message}`);
-      throw err;
+      logger.error(`Failed to clean up sessions: ${err.message}`);
+      error = err;
+    }
+
+    if (indexNeedsRefresh) {
+      // Only refresh the index if we have actually deleted one or more sessions. The index will auto-refresh eventually anyway, this just
+      // ensures that searches after the cleanup process are accurate, and this only impacts integration tests.
+      try {
+        await elasticsearchClient.indices.refresh({ index: this.indexName });
+        logger.debug(`Refreshed session index.`);
+      } catch (err) {
+        logger.error(`Failed to refresh session index: ${err.message}`);
+      }
+    }
+
+    if (error) {
+      // If we couldn't fetch or delete sessions, throw an error so the task will be retried.
+      throw error;
     }
   }