diff --git a/x-pack/plugins/security/server/session_management/session_index.test.ts b/x-pack/plugins/security/server/session_management/session_index.test.ts index 45ce865de56357..919f3dd4c7f021 100644 --- a/x-pack/plugins/security/server/session_management/session_index.test.ts +++ b/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 () => { diff --git a/x-pack/plugins/security/server/session_management/session_index.ts b/x-pack/plugins/security/server/session_management/session_index.ts index d05a11c417678a..d34c6de4747dbe 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/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>> = []; 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; } }