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 94e92a2689fc38..b19f580f2d8dd1 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 @@ -215,6 +215,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 () => { @@ -227,7 +228,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 () => { @@ -388,6 +402,7 @@ describe('Session index', () => { } ); expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1); }); it('when only `idleTimeout` is configured', async () => { @@ -474,6 +489,7 @@ describe('Session index', () => { } ); expect(mockElasticsearchClient.closePointInTime).toHaveBeenCalledTimes(1); + expect(mockElasticsearchClient.indices.refresh).toHaveBeenCalledTimes(1); }); it('when both `lifespan` and `idleTimeout` are configured', async () => { @@ -570,6 +586,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 () => { @@ -714,6 +731,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 () => { @@ -729,6 +747,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 () => { @@ -742,6 +761,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 8a69b9b7f0043c..2a677f6ecf5616 100644 --- a/x-pack/plugins/security/server/session_management/session_index.ts +++ b/x-pack/plugins/security/server/session_management/session_index.ts @@ -444,20 +444,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 bulkResponse = await this.options.elasticsearchClient.bulk( + const bulkResponse = await elasticsearchClient.bulk( { index: this.indexName, operations, @@ -471,24 +472,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; } }