From 886ba9ae6d4da5d8796658cf7c7226c2cd7ec65e Mon Sep 17 00:00:00 2001 From: Brant DeBow <125889545+brant-livefront@users.noreply.github.com> Date: Wed, 17 Dec 2025 11:43:53 -0500 Subject: [PATCH] Refactor IntegrationHandlerResult to provide more detail around failures (#6736) * Refactor IntegrationHandlerResult to provide more detail around failures * ServiceUnavailable now retryable, more explicit http status handling, more consistency with different handlers, additional xmldocs * Address PR feedback --- .../IntegrationFailureCategory.cs | 37 +++++ .../IntegrationHandlerResult.cs | 82 ++++++++++- .../Services/IIntegrationHandler.cs | 117 ++++++++++------ ...ureServiceBusIntegrationListenerService.cs | 11 ++ .../RabbitMqIntegrationListenerService.cs | 22 ++- .../SlackIntegrationHandler.cs | 68 +++++++--- .../TeamsIntegrationHandler.cs | 51 +++++-- .../IntegrationHandlerResultTests.cs | 128 ++++++++++++++++++ ...rviceBusIntegrationListenerServiceTests.cs | 35 +++-- .../DatadogIntegrationHandlerTests.cs | 2 +- .../Services/IntegrationHandlerTests.cs | 108 ++++++++++++++- ...RabbitMqIntegrationListenerServiceTests.cs | 25 ++-- .../Services/SlackIntegrationHandlerTests.cs | 4 +- .../Services/TeamsIntegrationHandlerTests.cs | 82 ++++++++++- .../WebhookIntegrationHandlerTests.cs | 4 +- 15 files changed, 663 insertions(+), 113 deletions(-) create mode 100644 src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationFailureCategory.cs create mode 100644 test/Core.Test/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResultTests.cs diff --git a/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationFailureCategory.cs b/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationFailureCategory.cs new file mode 100644 index 0000000000..544e671d51 --- /dev/null +++ b/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationFailureCategory.cs @@ -0,0 +1,37 @@ +namespace Bit.Core.AdminConsole.Models.Data.EventIntegrations; + +/// +/// Categories of event integration failures used for classification and retry logic. +/// +public enum IntegrationFailureCategory +{ + /// + /// Service is temporarily unavailable (503, upstream outage, maintenance). + /// + ServiceUnavailable, + + /// + /// Authentication failed (401, 403, invalid_auth, token issues). + /// + AuthenticationFailed, + + /// + /// Configuration error (invalid config, channel_not_found, etc.). + /// + ConfigurationError, + + /// + /// Rate limited (429, rate_limited). + /// + RateLimited, + + /// + /// Transient error (timeouts, 500, network errors). + /// + TransientError, + + /// + /// Permanent failure unrelated to authentication/config (e.g., unrecoverable payload/format issue). + /// + PermanentFailure +} diff --git a/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResult.cs b/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResult.cs index 8db054561b..375f2489cb 100644 --- a/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResult.cs +++ b/src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResult.cs @@ -1,16 +1,84 @@ namespace Bit.Core.AdminConsole.Models.Data.EventIntegrations; +/// +/// Represents the result of an integration handler operation, including success status, +/// failure categorization, and retry metadata. Use the factory method +/// for successful operations or for failures with automatic retry-ability +/// determination based on the failure category. +/// public class IntegrationHandlerResult { - public IntegrationHandlerResult(bool success, IIntegrationMessage message) + /// + /// True if the integration send succeeded, false otherwise. + /// + public bool Success { get; } + + /// + /// The integration message that was processed. + /// + public IIntegrationMessage Message { get; } + + /// + /// Optional UTC date/time indicating when a failed operation should be retried. + /// Will be used by the retry queue to delay re-sending the message. + /// Usually set based on the Retry-After header from rate-limited responses. + /// + public DateTime? DelayUntilDate { get; private init; } + + /// + /// Category of the failure. Null for successful results. + /// + public IntegrationFailureCategory? Category { get; private init; } + + /// + /// Detailed failure reason or error message. Empty for successful results. + /// + public string? FailureReason { get; private init; } + + /// + /// Indicates whether the operation is retryable. + /// Computed from the failure category. + /// + public bool Retryable => Category switch + { + IntegrationFailureCategory.RateLimited => true, + IntegrationFailureCategory.TransientError => true, + IntegrationFailureCategory.ServiceUnavailable => true, + IntegrationFailureCategory.AuthenticationFailed => false, + IntegrationFailureCategory.ConfigurationError => false, + IntegrationFailureCategory.PermanentFailure => false, + null => false, + _ => false + }; + + /// + /// Creates a successful result. + /// + public static IntegrationHandlerResult Succeed(IIntegrationMessage message) + { + return new IntegrationHandlerResult(success: true, message: message); + } + + /// + /// Creates a failed result with a failure category and reason. + /// + public static IntegrationHandlerResult Fail( + IIntegrationMessage message, + IntegrationFailureCategory category, + string failureReason, + DateTime? delayUntil = null) + { + return new IntegrationHandlerResult(success: false, message: message) + { + Category = category, + FailureReason = failureReason, + DelayUntilDate = delayUntil + }; + } + + private IntegrationHandlerResult(bool success, IIntegrationMessage message) { Success = success; Message = message; } - - public bool Success { get; set; } = false; - public bool Retryable { get; set; } = false; - public IIntegrationMessage Message { get; set; } - public DateTime? DelayUntilDate { get; set; } - public string FailureReason { get; set; } = string.Empty; } diff --git a/src/Core/AdminConsole/Services/IIntegrationHandler.cs b/src/Core/AdminConsole/Services/IIntegrationHandler.cs index bb10dc01b9..c36081cb52 100644 --- a/src/Core/AdminConsole/Services/IIntegrationHandler.cs +++ b/src/Core/AdminConsole/Services/IIntegrationHandler.cs @@ -29,46 +29,87 @@ public abstract class IntegrationHandlerBase : IIntegrationHandler IntegrationMessage message, TimeProvider timeProvider) { - var result = new IntegrationHandlerResult(success: response.IsSuccessStatusCode, message); - - if (response.IsSuccessStatusCode) return result; - - switch (response.StatusCode) + if (response.IsSuccessStatusCode) { - case HttpStatusCode.TooManyRequests: - case HttpStatusCode.RequestTimeout: - case HttpStatusCode.InternalServerError: - case HttpStatusCode.BadGateway: - case HttpStatusCode.ServiceUnavailable: - case HttpStatusCode.GatewayTimeout: - result.Retryable = true; - result.FailureReason = response.ReasonPhrase ?? $"Failure with status code: {(int)response.StatusCode}"; - - if (response.Headers.TryGetValues("Retry-After", out var values)) - { - var value = values.FirstOrDefault(); - if (int.TryParse(value, out var seconds)) - { - // Retry-after was specified in seconds. Adjust DelayUntilDate by the requested number of seconds. - result.DelayUntilDate = timeProvider.GetUtcNow().AddSeconds(seconds).UtcDateTime; - } - else if (DateTimeOffset.TryParseExact(value, - "r", // "r" is the round-trip format: RFC1123 - CultureInfo.InvariantCulture, - DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, - out var retryDate)) - { - // Retry-after was specified as a date. Adjust DelayUntilDate to the specified date. - result.DelayUntilDate = retryDate.UtcDateTime; - } - } - break; - default: - result.Retryable = false; - result.FailureReason = response.ReasonPhrase ?? $"Failure with status code {(int)response.StatusCode}"; - break; + return IntegrationHandlerResult.Succeed(message); } - return result; + var category = ClassifyHttpStatusCode(response.StatusCode); + var failureReason = response.ReasonPhrase ?? $"Failure with status code {(int)response.StatusCode}"; + + if (category is not (IntegrationFailureCategory.RateLimited + or IntegrationFailureCategory.TransientError + or IntegrationFailureCategory.ServiceUnavailable) || + !response.Headers.TryGetValues("Retry-After", out var values) + ) + { + return IntegrationHandlerResult.Fail(message: message, category: category, failureReason: failureReason); + } + + // Handle Retry-After header for rate-limited and retryable errors + DateTime? delayUntil = null; + var value = values.FirstOrDefault(); + if (int.TryParse(value, out var seconds)) + { + // Retry-after was specified in seconds + delayUntil = timeProvider.GetUtcNow().AddSeconds(seconds).UtcDateTime; + } + else if (DateTimeOffset.TryParseExact(value, + "r", // "r" is the round-trip format: RFC1123 + CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal, + out var retryDate)) + { + // Retry-after was specified as a date + delayUntil = retryDate.UtcDateTime; + } + + return IntegrationHandlerResult.Fail( + message, + category, + failureReason, + delayUntil + ); + } + + /// + /// Classifies an as an to drive + /// retry behavior and operator-facing failure reporting. + /// + /// The HTTP status code. + /// The corresponding . + protected static IntegrationFailureCategory ClassifyHttpStatusCode(HttpStatusCode statusCode) + { + var explicitCategory = statusCode switch + { + HttpStatusCode.Unauthorized => IntegrationFailureCategory.AuthenticationFailed, + HttpStatusCode.Forbidden => IntegrationFailureCategory.AuthenticationFailed, + HttpStatusCode.NotFound => IntegrationFailureCategory.ConfigurationError, + HttpStatusCode.Gone => IntegrationFailureCategory.ConfigurationError, + HttpStatusCode.MovedPermanently => IntegrationFailureCategory.ConfigurationError, + HttpStatusCode.TemporaryRedirect => IntegrationFailureCategory.ConfigurationError, + HttpStatusCode.PermanentRedirect => IntegrationFailureCategory.ConfigurationError, + HttpStatusCode.TooManyRequests => IntegrationFailureCategory.RateLimited, + HttpStatusCode.RequestTimeout => IntegrationFailureCategory.TransientError, + HttpStatusCode.InternalServerError => IntegrationFailureCategory.TransientError, + HttpStatusCode.BadGateway => IntegrationFailureCategory.TransientError, + HttpStatusCode.GatewayTimeout => IntegrationFailureCategory.TransientError, + HttpStatusCode.ServiceUnavailable => IntegrationFailureCategory.ServiceUnavailable, + HttpStatusCode.NotImplemented => IntegrationFailureCategory.PermanentFailure, + _ => (IntegrationFailureCategory?)null + }; + + if (explicitCategory is not null) + { + return explicitCategory.Value; + } + + return (int)statusCode switch + { + >= 300 and <= 399 => IntegrationFailureCategory.ConfigurationError, + >= 400 and <= 499 => IntegrationFailureCategory.ConfigurationError, + >= 500 and <= 599 => IntegrationFailureCategory.ServiceUnavailable, + _ => IntegrationFailureCategory.ServiceUnavailable + }; } } diff --git a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/AzureServiceBusIntegrationListenerService.cs b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/AzureServiceBusIntegrationListenerService.cs index 633a53296b..c97c5f7efe 100644 --- a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/AzureServiceBusIntegrationListenerService.cs +++ b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/AzureServiceBusIntegrationListenerService.cs @@ -85,6 +85,17 @@ public class AzureServiceBusIntegrationListenerService : Backgro { // Non-recoverable failure or exceeded the max number of retries // Return false to indicate this message should be dead-lettered + _logger.LogWarning( + "Integration failure - non-recoverable error or max retries exceeded. " + + "MessageId: {MessageId}, IntegrationType: {IntegrationType}, OrganizationId: {OrgId}, " + + "FailureCategory: {Category}, Reason: {Reason}, RetryCount: {RetryCount}, MaxRetries: {MaxRetries}", + message.MessageId, + message.IntegrationType, + message.OrganizationId, + result.Category, + result.FailureReason, + message.RetryCount, + _maxRetries); return false; } } diff --git a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/RabbitMqIntegrationListenerService.cs b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/RabbitMqIntegrationListenerService.cs index b426032c92..0762edc040 100644 --- a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/RabbitMqIntegrationListenerService.cs +++ b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/RabbitMqIntegrationListenerService.cs @@ -106,14 +106,32 @@ public class RabbitMqIntegrationListenerService : BackgroundServ { // Exceeded the max number of retries; fail and send to dead letter queue await _rabbitMqService.PublishToDeadLetterAsync(channel, message, cancellationToken); - _logger.LogWarning("Max retry attempts reached. Sent to DLQ."); + _logger.LogWarning( + "Integration failure - max retries exceeded. " + + "MessageId: {MessageId}, IntegrationType: {IntegrationType}, OrganizationId: {OrgId}, " + + "FailureCategory: {Category}, Reason: {Reason}, RetryCount: {RetryCount}, MaxRetries: {MaxRetries}", + message.MessageId, + message.IntegrationType, + message.OrganizationId, + result.Category, + result.FailureReason, + message.RetryCount, + _maxRetries); } } else { // Fatal error (i.e. not retryable) occurred. Send message to dead letter queue without any retries await _rabbitMqService.PublishToDeadLetterAsync(channel, message, cancellationToken); - _logger.LogWarning("Non-retryable failure. Sent to DLQ."); + _logger.LogWarning( + "Integration failure - non-retryable. " + + "MessageId: {MessageId}, IntegrationType: {IntegrationType}, OrganizationId: {OrgId}, " + + "FailureCategory: {Category}, Reason: {Reason}", + message.MessageId, + message.IntegrationType, + message.OrganizationId, + result.Category, + result.FailureReason); } // Message has been sent to retry or dead letter queues. diff --git a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/SlackIntegrationHandler.cs b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/SlackIntegrationHandler.cs index 16c756c8c4..e681140afe 100644 --- a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/SlackIntegrationHandler.cs +++ b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/SlackIntegrationHandler.cs @@ -6,15 +6,6 @@ public class SlackIntegrationHandler( ISlackService slackService) : IntegrationHandlerBase { - private static readonly HashSet _retryableErrors = new(StringComparer.Ordinal) - { - "internal_error", - "message_limit_exceeded", - "rate_limited", - "ratelimited", - "service_unavailable" - }; - public override async Task HandleAsync(IntegrationMessage message) { var slackResponse = await slackService.SendSlackMessageByChannelIdAsync( @@ -25,24 +16,61 @@ public class SlackIntegrationHandler( if (slackResponse is null) { - return new IntegrationHandlerResult(success: false, message: message) - { - FailureReason = "Slack response was null" - }; + return IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.TransientError, + "Slack response was null" + ); } if (slackResponse.Ok) { - return new IntegrationHandlerResult(success: true, message: message); + return IntegrationHandlerResult.Succeed(message); } - var result = new IntegrationHandlerResult(success: false, message: message) { FailureReason = slackResponse.Error }; + var category = ClassifySlackError(slackResponse.Error); + return IntegrationHandlerResult.Fail( + message, + category, + slackResponse.Error + ); + } - if (_retryableErrors.Contains(slackResponse.Error)) + /// + /// Classifies a Slack API error code string as an to drive + /// retry behavior and operator-facing failure reporting. + /// + /// + /// + /// Slack responses commonly return an error string when ok is false. This method maps + /// known Slack error codes to failure categories. + /// + /// + /// Any unrecognized error codes default to to avoid + /// incorrectly marking new/unknown Slack failures as non-retryable. + /// + /// + /// The Slack error code string (e.g. invalid_auth, rate_limited). + /// The corresponding . + private static IntegrationFailureCategory ClassifySlackError(string error) + { + return error switch { - result.Retryable = true; - } - - return result; + "invalid_auth" => IntegrationFailureCategory.AuthenticationFailed, + "access_denied" => IntegrationFailureCategory.AuthenticationFailed, + "token_expired" => IntegrationFailureCategory.AuthenticationFailed, + "token_revoked" => IntegrationFailureCategory.AuthenticationFailed, + "account_inactive" => IntegrationFailureCategory.AuthenticationFailed, + "not_authed" => IntegrationFailureCategory.AuthenticationFailed, + "channel_not_found" => IntegrationFailureCategory.ConfigurationError, + "is_archived" => IntegrationFailureCategory.ConfigurationError, + "rate_limited" => IntegrationFailureCategory.RateLimited, + "ratelimited" => IntegrationFailureCategory.RateLimited, + "message_limit_exceeded" => IntegrationFailureCategory.RateLimited, + "internal_error" => IntegrationFailureCategory.TransientError, + "service_unavailable" => IntegrationFailureCategory.ServiceUnavailable, + "fatal_error" => IntegrationFailureCategory.ServiceUnavailable, + _ => IntegrationFailureCategory.TransientError + }; } } diff --git a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/TeamsIntegrationHandler.cs b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/TeamsIntegrationHandler.cs index 41d60bd69c..9e3645a99f 100644 --- a/src/Core/AdminConsole/Services/Implementations/EventIntegrations/TeamsIntegrationHandler.cs +++ b/src/Core/AdminConsole/Services/Implementations/EventIntegrations/TeamsIntegrationHandler.cs @@ -1,4 +1,5 @@ -using Bit.Core.AdminConsole.Models.Data.EventIntegrations; +using System.Text.Json; +using Bit.Core.AdminConsole.Models.Data.EventIntegrations; using Microsoft.Rest; namespace Bit.Core.Services; @@ -18,24 +19,48 @@ public class TeamsIntegrationHandler( channelId: message.Configuration.ChannelId ); - return new IntegrationHandlerResult(success: true, message: message); + return IntegrationHandlerResult.Succeed(message); } catch (HttpOperationException ex) { - var result = new IntegrationHandlerResult(success: false, message: message); - var statusCode = (int)ex.Response.StatusCode; - result.Retryable = statusCode is 429 or >= 500 and < 600; - result.FailureReason = ex.Message; - - return result; + var category = ClassifyHttpStatusCode(ex.Response.StatusCode); + return IntegrationHandlerResult.Fail( + message, + category, + ex.Message + ); + } + catch (ArgumentException ex) + { + return IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.ConfigurationError, + ex.Message + ); + } + catch (UriFormatException ex) + { + return IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.ConfigurationError, + ex.Message + ); + } + catch (JsonException ex) + { + return IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.PermanentFailure, + ex.Message + ); } catch (Exception ex) { - var result = new IntegrationHandlerResult(success: false, message: message); - result.Retryable = false; - result.FailureReason = ex.Message; - - return result; + return IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.TransientError, + ex.Message + ); } } } diff --git a/test/Core.Test/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResultTests.cs b/test/Core.Test/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResultTests.cs new file mode 100644 index 0000000000..6925a978eb --- /dev/null +++ b/test/Core.Test/AdminConsole/Models/Data/EventIntegrations/IntegrationHandlerResultTests.cs @@ -0,0 +1,128 @@ +using Bit.Core.AdminConsole.Models.Data.EventIntegrations; +using Bit.Test.Common.AutoFixture.Attributes; +using Xunit; + +namespace Bit.Core.Test.AdminConsole.Models.Data.EventIntegrations; + +public class IntegrationHandlerResultTests +{ + [Theory, BitAutoData] + public void Succeed_SetsSuccessTrue_CategoryNull(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Succeed(message); + + Assert.True(result.Success); + Assert.Null(result.Category); + Assert.Equal(message, result.Message); + Assert.Null(result.FailureReason); + } + + [Theory, BitAutoData] + public void Fail_WithCategory_SetsSuccessFalse_CategorySet(IntegrationMessage message) + { + var category = IntegrationFailureCategory.AuthenticationFailed; + var failureReason = "Invalid credentials"; + + var result = IntegrationHandlerResult.Fail(message, category, failureReason); + + Assert.False(result.Success); + Assert.Equal(category, result.Category); + Assert.Equal(failureReason, result.FailureReason); + Assert.Equal(message, result.Message); + } + + [Theory, BitAutoData] + public void Fail_WithDelayUntil_SetsDelayUntilDate(IntegrationMessage message) + { + var delayUntil = DateTime.UtcNow.AddMinutes(5); + + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.RateLimited, + "Rate limited", + delayUntil + ); + + Assert.Equal(delayUntil, result.DelayUntilDate); + } + + [Theory, BitAutoData] + public void Retryable_RateLimited_ReturnsTrue(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.RateLimited, + "Rate limited" + ); + + Assert.True(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_TransientError_ReturnsTrue(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.TransientError, + "Temporary network issue" + ); + + Assert.True(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_AuthenticationFailed_ReturnsFalse(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.AuthenticationFailed, + "Invalid token" + ); + + Assert.False(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_ConfigurationError_ReturnsFalse(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.ConfigurationError, + "Channel not found" + ); + + Assert.False(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_ServiceUnavailable_ReturnsTrue(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.ServiceUnavailable, + "Service is down" + ); + + Assert.True(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_PermanentFailure_ReturnsFalse(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Fail( + message, + IntegrationFailureCategory.PermanentFailure, + "Permanent failure" + ); + + Assert.False(result.Retryable); + } + + [Theory, BitAutoData] + public void Retryable_SuccessCase_ReturnsFalse(IntegrationMessage message) + { + var result = IntegrationHandlerResult.Succeed(message); + + Assert.False(result.Retryable); + } +} diff --git a/test/Core.Test/AdminConsole/Services/AzureServiceBusIntegrationListenerServiceTests.cs b/test/Core.Test/AdminConsole/Services/AzureServiceBusIntegrationListenerServiceTests.cs index 23627f3962..9e46a3a99a 100644 --- a/test/Core.Test/AdminConsole/Services/AzureServiceBusIntegrationListenerServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/AzureServiceBusIntegrationListenerServiceTests.cs @@ -78,8 +78,10 @@ public class AzureServiceBusIntegrationListenerServiceTests var sutProvider = GetSutProvider(); message.RetryCount = 0; - var result = new IntegrationHandlerResult(false, message); - result.Retryable = false; + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.AuthenticationFailed, // NOT retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -89,6 +91,12 @@ public class AzureServiceBusIntegrationListenerServiceTests await _handler.Received(1).HandleAsync(Arg.Is(expected.ToJson())); await _serviceBusService.DidNotReceiveWithAnyArgs().PublishToRetryAsync(Arg.Any()); + _logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? "").Contains("Integration failure - non-recoverable error or max retries exceeded.")), + Arg.Any(), + Arg.Any>()); } [Theory, BitAutoData] @@ -96,9 +104,10 @@ public class AzureServiceBusIntegrationListenerServiceTests { var sutProvider = GetSutProvider(); message.RetryCount = _config.MaxRetries; - var result = new IntegrationHandlerResult(false, message); - result.Retryable = true; - + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.TransientError, // Retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -108,6 +117,12 @@ public class AzureServiceBusIntegrationListenerServiceTests await _handler.Received(1).HandleAsync(Arg.Is(expected.ToJson())); await _serviceBusService.DidNotReceiveWithAnyArgs().PublishToRetryAsync(Arg.Any()); + _logger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? "").Contains("Integration failure - non-recoverable error or max retries exceeded.")), + Arg.Any(), + Arg.Any>()); } [Theory, BitAutoData] @@ -116,8 +131,10 @@ public class AzureServiceBusIntegrationListenerServiceTests var sutProvider = GetSutProvider(); message.RetryCount = 0; - var result = new IntegrationHandlerResult(false, message); - result.Retryable = true; + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.TransientError, // Retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -133,7 +150,7 @@ public class AzureServiceBusIntegrationListenerServiceTests public async Task HandleMessageAsync_SuccessfulResult_Succeeds(IntegrationMessage message) { var sutProvider = GetSutProvider(); - var result = new IntegrationHandlerResult(true, message); + var result = IntegrationHandlerResult.Succeed(message); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -156,7 +173,7 @@ public class AzureServiceBusIntegrationListenerServiceTests _logger.Received(1).Log( LogLevel.Error, Arg.Any(), - Arg.Any(), + Arg.Is(o => (o.ToString() ?? "").Contains("Unhandled error processing ASB message")), Arg.Any(), Arg.Any>()); diff --git a/test/Core.Test/AdminConsole/Services/DatadogIntegrationHandlerTests.cs b/test/Core.Test/AdminConsole/Services/DatadogIntegrationHandlerTests.cs index 5f0a9915bf..9cb21f012a 100644 --- a/test/Core.Test/AdminConsole/Services/DatadogIntegrationHandlerTests.cs +++ b/test/Core.Test/AdminConsole/Services/DatadogIntegrationHandlerTests.cs @@ -51,7 +51,7 @@ public class DatadogIntegrationHandlerTests Assert.True(result.Success); Assert.Equal(result.Message, message); - Assert.Empty(result.FailureReason); + Assert.Null(result.FailureReason); sutProvider.GetDependency().Received(1).CreateClient( Arg.Is(AssertHelper.AssertPropertyEqual(DatadogIntegrationHandler.HttpClientName)) diff --git a/test/Core.Test/AdminConsole/Services/IntegrationHandlerTests.cs b/test/Core.Test/AdminConsole/Services/IntegrationHandlerTests.cs index f6f587cfd7..b3bbcb7ef2 100644 --- a/test/Core.Test/AdminConsole/Services/IntegrationHandlerTests.cs +++ b/test/Core.Test/AdminConsole/Services/IntegrationHandlerTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.AdminConsole.Models.Data.EventIntegrations; +using System.Net; +using Bit.Core.AdminConsole.Models.Data.EventIntegrations; using Bit.Core.Enums; using Bit.Core.Services; using Xunit; @@ -7,7 +8,6 @@ namespace Bit.Core.Test.Services; public class IntegrationHandlerTests { - [Fact] public async Task HandleAsync_ConvertsJsonToTypedIntegrationMessage() { @@ -33,13 +33,113 @@ public class IntegrationHandlerTests Assert.Equal(expected.IntegrationType, typedResult.IntegrationType); } + [Theory] + [InlineData(HttpStatusCode.Unauthorized)] + [InlineData(HttpStatusCode.Forbidden)] + public void ClassifyHttpStatusCode_AuthenticationFailed(HttpStatusCode code) + { + Assert.Equal( + IntegrationFailureCategory.AuthenticationFailed, + TestIntegrationHandler.Classify(code)); + } + + [Theory] + [InlineData(HttpStatusCode.NotFound)] + [InlineData(HttpStatusCode.Gone)] + [InlineData(HttpStatusCode.MovedPermanently)] + [InlineData(HttpStatusCode.TemporaryRedirect)] + [InlineData(HttpStatusCode.PermanentRedirect)] + public void ClassifyHttpStatusCode_ConfigurationError(HttpStatusCode code) + { + Assert.Equal( + IntegrationFailureCategory.ConfigurationError, + TestIntegrationHandler.Classify(code)); + } + + [Fact] + public void ClassifyHttpStatusCode_TooManyRequests_IsRateLimited() + { + Assert.Equal( + IntegrationFailureCategory.RateLimited, + TestIntegrationHandler.Classify(HttpStatusCode.TooManyRequests)); + } + + [Fact] + public void ClassifyHttpStatusCode_RequestTimeout_IsTransient() + { + Assert.Equal( + IntegrationFailureCategory.TransientError, + TestIntegrationHandler.Classify(HttpStatusCode.RequestTimeout)); + } + + [Theory] + [InlineData(HttpStatusCode.InternalServerError)] + [InlineData(HttpStatusCode.BadGateway)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public void ClassifyHttpStatusCode_Common5xx_AreTransient(HttpStatusCode code) + { + Assert.Equal( + IntegrationFailureCategory.TransientError, + TestIntegrationHandler.Classify(code)); + } + + [Fact] + public void ClassifyHttpStatusCode_ServiceUnavailable_IsServiceUnavailable() + { + Assert.Equal( + IntegrationFailureCategory.ServiceUnavailable, + TestIntegrationHandler.Classify(HttpStatusCode.ServiceUnavailable)); + } + + [Fact] + public void ClassifyHttpStatusCode_NotImplemented_IsPermanentFailure() + { + Assert.Equal( + IntegrationFailureCategory.PermanentFailure, + TestIntegrationHandler.Classify(HttpStatusCode.NotImplemented)); + } + + [Fact] + public void FClassifyHttpStatusCode_Unhandled3xx_IsConfigurationError() + { + Assert.Equal( + IntegrationFailureCategory.ConfigurationError, + TestIntegrationHandler.Classify(HttpStatusCode.Found)); + } + + [Fact] + public void ClassifyHttpStatusCode_Unhandled4xx_IsConfigurationError() + { + Assert.Equal( + IntegrationFailureCategory.ConfigurationError, + TestIntegrationHandler.Classify(HttpStatusCode.BadRequest)); + } + + [Fact] + public void ClassifyHttpStatusCode_Unhandled5xx_IsServiceUnavailable() + { + Assert.Equal( + IntegrationFailureCategory.ServiceUnavailable, + TestIntegrationHandler.Classify(HttpStatusCode.HttpVersionNotSupported)); + } + + [Fact] + public void ClassifyHttpStatusCode_UnknownCode_DefaultsToServiceUnavailable() + { + // cast an out-of-range value to ensure default path is stable + Assert.Equal( + IntegrationFailureCategory.ServiceUnavailable, + TestIntegrationHandler.Classify((HttpStatusCode)799)); + } + private class TestIntegrationHandler : IntegrationHandlerBase { public override Task HandleAsync( IntegrationMessage message) { - var result = new IntegrationHandlerResult(success: true, message: message); - return Task.FromResult(result); + return Task.FromResult(IntegrationHandlerResult.Succeed(message: message)); } + + public static IntegrationFailureCategory Classify(HttpStatusCode code) => ClassifyHttpStatusCode(code); } } diff --git a/test/Core.Test/AdminConsole/Services/RabbitMqIntegrationListenerServiceTests.cs b/test/Core.Test/AdminConsole/Services/RabbitMqIntegrationListenerServiceTests.cs index 5fcd121252..71985889f8 100644 --- a/test/Core.Test/AdminConsole/Services/RabbitMqIntegrationListenerServiceTests.cs +++ b/test/Core.Test/AdminConsole/Services/RabbitMqIntegrationListenerServiceTests.cs @@ -86,8 +86,10 @@ public class RabbitMqIntegrationListenerServiceTests new BasicProperties(), body: Encoding.UTF8.GetBytes(message.ToJson()) ); - var result = new IntegrationHandlerResult(false, message); - result.Retryable = false; + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.AuthenticationFailed, // NOT retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -105,7 +107,7 @@ public class RabbitMqIntegrationListenerServiceTests _logger.Received().Log( LogLevel.Warning, Arg.Any(), - Arg.Is(o => (o.ToString() ?? "").Contains("Non-retryable failure")), + Arg.Is(o => (o.ToString() ?? "").Contains("Integration failure - non-retryable.")), Arg.Any(), Arg.Any>()); @@ -133,8 +135,10 @@ public class RabbitMqIntegrationListenerServiceTests new BasicProperties(), body: Encoding.UTF8.GetBytes(message.ToJson()) ); - var result = new IntegrationHandlerResult(false, message); - result.Retryable = true; + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.TransientError, // Retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -151,7 +155,7 @@ public class RabbitMqIntegrationListenerServiceTests _logger.Received().Log( LogLevel.Warning, Arg.Any(), - Arg.Is(o => (o.ToString() ?? "").Contains("Max retry attempts reached")), + Arg.Is(o => (o.ToString() ?? "").Contains("Integration failure - max retries exceeded.")), Arg.Any(), Arg.Any>()); @@ -179,9 +183,10 @@ public class RabbitMqIntegrationListenerServiceTests new BasicProperties(), body: Encoding.UTF8.GetBytes(message.ToJson()) ); - var result = new IntegrationHandlerResult(false, message); - result.Retryable = true; - result.DelayUntilDate = _now.AddMinutes(1); + var result = IntegrationHandlerResult.Fail( + message: message, + category: IntegrationFailureCategory.TransientError, // Retryable + failureReason: "403"); _handler.HandleAsync(Arg.Any()).Returns(result); var expected = IntegrationMessage.FromJson(message.ToJson()); @@ -220,7 +225,7 @@ public class RabbitMqIntegrationListenerServiceTests new BasicProperties(), body: Encoding.UTF8.GetBytes(message.ToJson()) ); - var result = new IntegrationHandlerResult(true, message); + var result = IntegrationHandlerResult.Succeed(message); _handler.HandleAsync(Arg.Any()).Returns(result); await sutProvider.Sut.ProcessReceivedMessageAsync(eventArgs, cancellationToken); diff --git a/test/Core.Test/AdminConsole/Services/SlackIntegrationHandlerTests.cs b/test/Core.Test/AdminConsole/Services/SlackIntegrationHandlerTests.cs index e2e459ceb3..e455100995 100644 --- a/test/Core.Test/AdminConsole/Services/SlackIntegrationHandlerTests.cs +++ b/test/Core.Test/AdminConsole/Services/SlackIntegrationHandlerTests.cs @@ -110,7 +110,7 @@ public class SlackIntegrationHandlerTests } [Fact] - public async Task HandleAsync_NullResponse_ReturnsNonRetryableFailure() + public async Task HandleAsync_NullResponse_ReturnsRetryableFailure() { var sutProvider = GetSutProvider(); var message = new IntegrationMessage() @@ -126,7 +126,7 @@ public class SlackIntegrationHandlerTests var result = await sutProvider.Sut.HandleAsync(message); Assert.False(result.Success); - Assert.False(result.Retryable); + Assert.True(result.Retryable); // Null response is classified as TransientError (retryable) Assert.Equal("Slack response was null", result.FailureReason); Assert.Equal(result.Message, message); diff --git a/test/Core.Test/AdminConsole/Services/TeamsIntegrationHandlerTests.cs b/test/Core.Test/AdminConsole/Services/TeamsIntegrationHandlerTests.cs index b744a6aa69..11056ec2cc 100644 --- a/test/Core.Test/AdminConsole/Services/TeamsIntegrationHandlerTests.cs +++ b/test/Core.Test/AdminConsole/Services/TeamsIntegrationHandlerTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.AdminConsole.Models.Data.EventIntegrations; +using System.Text.Json; +using Bit.Core.AdminConsole.Models.Data.EventIntegrations; using Bit.Core.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; @@ -42,9 +43,77 @@ public class TeamsIntegrationHandlerTests ); } + [Theory, BitAutoData] + public async Task HandleAsync_ArgumentException_ReturnsConfigurationError(IntegrationMessage message) + { + var sutProvider = GetSutProvider(); + message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); + + sutProvider.GetDependency() + .SendMessageToChannelAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .ThrowsAsync(new ArgumentException("argument error")); + var result = await sutProvider.Sut.HandleAsync(message); + + Assert.False(result.Success); + Assert.Equal(IntegrationFailureCategory.ConfigurationError, result.Category); + Assert.False(result.Retryable); + Assert.Equal(result.Message, message); + + await sutProvider.GetDependency().Received(1).SendMessageToChannelAsync( + Arg.Is(AssertHelper.AssertPropertyEqual(_serviceUrl)), + Arg.Is(AssertHelper.AssertPropertyEqual(_channelId)), + Arg.Is(AssertHelper.AssertPropertyEqual(message.RenderedTemplate)) + ); + } [Theory, BitAutoData] - public async Task HandleAsync_HttpExceptionNonRetryable_ReturnsFalseAndNotRetryable(IntegrationMessage message) + public async Task HandleAsync_JsonException_ReturnsPermanentFailure(IntegrationMessage message) + { + var sutProvider = GetSutProvider(); + message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); + + sutProvider.GetDependency() + .SendMessageToChannelAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .ThrowsAsync(new JsonException("JSON error")); + var result = await sutProvider.Sut.HandleAsync(message); + + Assert.False(result.Success); + Assert.Equal(IntegrationFailureCategory.PermanentFailure, result.Category); + Assert.False(result.Retryable); + Assert.Equal(result.Message, message); + + await sutProvider.GetDependency().Received(1).SendMessageToChannelAsync( + Arg.Is(AssertHelper.AssertPropertyEqual(_serviceUrl)), + Arg.Is(AssertHelper.AssertPropertyEqual(_channelId)), + Arg.Is(AssertHelper.AssertPropertyEqual(message.RenderedTemplate)) + ); + } + + [Theory, BitAutoData] + public async Task HandleAsync_UriFormatException_ReturnsConfigurationError(IntegrationMessage message) + { + var sutProvider = GetSutProvider(); + message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); + + sutProvider.GetDependency() + .SendMessageToChannelAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .ThrowsAsync(new UriFormatException("Bad URI")); + var result = await sutProvider.Sut.HandleAsync(message); + + Assert.False(result.Success); + Assert.Equal(IntegrationFailureCategory.ConfigurationError, result.Category); + Assert.False(result.Retryable); + Assert.Equal(result.Message, message); + + await sutProvider.GetDependency().Received(1).SendMessageToChannelAsync( + Arg.Is(AssertHelper.AssertPropertyEqual(_serviceUrl)), + Arg.Is(AssertHelper.AssertPropertyEqual(_channelId)), + Arg.Is(AssertHelper.AssertPropertyEqual(message.RenderedTemplate)) + ); + } + + [Theory, BitAutoData] + public async Task HandleAsync_HttpExceptionForbidden_ReturnsAuthenticationFailed(IntegrationMessage message) { var sutProvider = GetSutProvider(); message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); @@ -62,6 +131,7 @@ public class TeamsIntegrationHandlerTests var result = await sutProvider.Sut.HandleAsync(message); Assert.False(result.Success); + Assert.Equal(IntegrationFailureCategory.AuthenticationFailed, result.Category); Assert.False(result.Retryable); Assert.Equal(result.Message, message); @@ -73,7 +143,7 @@ public class TeamsIntegrationHandlerTests } [Theory, BitAutoData] - public async Task HandleAsync_HttpExceptionRetryable_ReturnsFalseAndRetryable(IntegrationMessage message) + public async Task HandleAsync_HttpExceptionTooManyRequests_ReturnsRateLimited(IntegrationMessage message) { var sutProvider = GetSutProvider(); message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); @@ -92,6 +162,7 @@ public class TeamsIntegrationHandlerTests var result = await sutProvider.Sut.HandleAsync(message); Assert.False(result.Success); + Assert.Equal(IntegrationFailureCategory.RateLimited, result.Category); Assert.True(result.Retryable); Assert.Equal(result.Message, message); @@ -103,7 +174,7 @@ public class TeamsIntegrationHandlerTests } [Theory, BitAutoData] - public async Task HandleAsync_UnknownException_ReturnsFalseAndNotRetryable(IntegrationMessage message) + public async Task HandleAsync_UnknownException_ReturnsTransientError(IntegrationMessage message) { var sutProvider = GetSutProvider(); message.Configuration = new TeamsIntegrationConfigurationDetails(_channelId, _serviceUrl); @@ -114,7 +185,8 @@ public class TeamsIntegrationHandlerTests var result = await sutProvider.Sut.HandleAsync(message); Assert.False(result.Success); - Assert.False(result.Retryable); + Assert.Equal(IntegrationFailureCategory.TransientError, result.Category); + Assert.True(result.Retryable); Assert.Equal(result.Message, message); await sutProvider.GetDependency().Received(1).SendMessageToChannelAsync( diff --git a/test/Core.Test/AdminConsole/Services/WebhookIntegrationHandlerTests.cs b/test/Core.Test/AdminConsole/Services/WebhookIntegrationHandlerTests.cs index 53a3598d47..05aa46681a 100644 --- a/test/Core.Test/AdminConsole/Services/WebhookIntegrationHandlerTests.cs +++ b/test/Core.Test/AdminConsole/Services/WebhookIntegrationHandlerTests.cs @@ -51,7 +51,7 @@ public class WebhookIntegrationHandlerTests Assert.True(result.Success); Assert.Equal(result.Message, message); - Assert.Empty(result.FailureReason); + Assert.Null(result.FailureReason); sutProvider.GetDependency().Received(1).CreateClient( Arg.Is(AssertHelper.AssertPropertyEqual(WebhookIntegrationHandler.HttpClientName)) @@ -79,7 +79,7 @@ public class WebhookIntegrationHandlerTests Assert.True(result.Success); Assert.Equal(result.Message, message); - Assert.Empty(result.FailureReason); + Assert.Null(result.FailureReason); sutProvider.GetDependency().Received(1).CreateClient( Arg.Is(AssertHelper.AssertPropertyEqual(WebhookIntegrationHandler.HttpClientName))