Refactor Slack Callback Mechanism (#6388)

* Refactor Slack Callback

* Add more safety to state param, clarify if logic, update tests

* Added an additional 2 possible cases to test: integration is not a slack integration, and the integration has already been claimed

* Implement SonarQube suggestion

* Adjusted org hash to include timestamp; addressed PR feedback

src/Api/AdminConsole/Controllers/SlackIntegrationController.cs

@@ -1,7 +1,4 @@
-// FIXME: Update this file to be null safe and then delete the line below
-#nullable disable
-
-using System.Text.Json;
+using System.Text.Json;
 using Bit.Api.AdminConsole.Models.Response.Organizations;
 using Bit.Core;
 using Bit.Core.AdminConsole.Entities;
@@ -18,25 +15,58 @@ using Microsoft.AspNetCore.Mvc;
 namespace Bit.Api.AdminConsole.Controllers;
 
 [RequireFeature(FeatureFlagKeys.EventBasedOrganizationIntegrations)]
-[Route("organizations/{organizationId:guid}/integrations/slack")]
+[Route("organizations")]
 [Authorize("Application")]
 public class SlackIntegrationController(
     ICurrentContext currentContext,
     IOrganizationIntegrationRepository integrationRepository,
-    ISlackService slackService) : Controller
+    ISlackService slackService,
+    TimeProvider timeProvider) : Controller
 {
-    [HttpGet("redirect")]
+    [HttpGet("{organizationId:guid}/integrations/slack/redirect")]
     public async Task<IActionResult> RedirectAsync(Guid organizationId)
     {
         if (!await currentContext.OrganizationOwner(organizationId))
         {
             throw new NotFoundException();
         }
-        string callbackUrl = Url.RouteUrl(
-            nameof(CreateAsync),
-            new { organizationId },
-            currentContext.HttpContext.Request.Scheme);
-        var redirectUrl = slackService.GetRedirectUrl(callbackUrl);
+
+        string? callbackUrl = Url.RouteUrl(
+            routeName: nameof(CreateAsync),
+            values: null,
+            protocol: currentContext.HttpContext.Request.Scheme,
+            host: currentContext.HttpContext.Request.Host.ToUriComponent()
+        );
+        if (string.IsNullOrEmpty(callbackUrl))
+        {
+            throw new BadRequestException("Unable to build callback Url");
+        }
+
+        var integrations = await integrationRepository.GetManyByOrganizationAsync(organizationId);
+        var integration = integrations.FirstOrDefault(i => i.Type == IntegrationType.Slack);
+
+        if (integration is null)
+        {
+            // No slack integration exists, create Initiated version
+            integration = await integrationRepository.CreateAsync(new OrganizationIntegration
+            {
+                OrganizationId = organizationId,
+                Type = IntegrationType.Slack,
+                Configuration = null,
+            });
+        }
+        else if (integration.Configuration is not null)
+        {
+            // A Completed (fully configured) Slack integration already exists, throw to prevent overriding
+            throw new BadRequestException("There already exists a Slack integration for this organization");
+
+        } // An Initiated slack integration exits, re-use it and kick off a new OAuth flow
+
+        var state = IntegrationOAuthState.FromIntegration(integration, timeProvider);
+        var redirectUrl = slackService.GetRedirectUrl(
+            callbackUrl: callbackUrl,
+            state: state.ToString()
+        );
 
         if (string.IsNullOrEmpty(redirectUrl))
         {
@@ -46,23 +76,42 @@ public class SlackIntegrationController(
         return Redirect(redirectUrl);
     }
 
-    [HttpGet("create", Name = nameof(CreateAsync))]
-    public async Task<IActionResult> CreateAsync(Guid organizationId, [FromQuery] string code)
+    [HttpGet("integrations/slack/create", Name = nameof(CreateAsync))]
+    [AllowAnonymous]
+    public async Task<IActionResult> CreateAsync([FromQuery] string code, [FromQuery] string state)
     {
-        if (!await currentContext.OrganizationOwner(organizationId))
+        var oAuthState = IntegrationOAuthState.FromString(state: state, timeProvider: timeProvider);
+        if (oAuthState is null)
         {
             throw new NotFoundException();
         }
 
-        if (string.IsNullOrEmpty(code))
+        // Fetch existing Initiated record
+        var integration = await integrationRepository.GetByIdAsync(oAuthState.IntegrationId);
+        if (integration is null ||
+            integration.Type != IntegrationType.Slack ||
+            integration.Configuration is not null)
         {
-            throw new BadRequestException("Missing code from Slack.");
+            throw new NotFoundException();
         }
 
-        string callbackUrl = Url.RouteUrl(
-            nameof(CreateAsync),
-            new { organizationId },
-            currentContext.HttpContext.Request.Scheme);
+        // Verify Organization matches hash
+        if (!oAuthState.ValidateOrg(integration.OrganizationId))
+        {
+            throw new NotFoundException();
+        }
+
+        // Fetch token from Slack and store to DB
+        string? callbackUrl = Url.RouteUrl(
+            routeName: nameof(CreateAsync),
+            values: null,
+            protocol: currentContext.HttpContext.Request.Scheme,
+            host: currentContext.HttpContext.Request.Host.ToUriComponent()
+        );
+        if (string.IsNullOrEmpty(callbackUrl))
+        {
+            throw new BadRequestException("Unable to build callback Url");
+        }
         var token = await slackService.ObtainTokenViaOAuth(code, callbackUrl);
 
         if (string.IsNullOrEmpty(token))
@@ -70,14 +119,10 @@ public class SlackIntegrationController(
             throw new BadRequestException("Invalid response from Slack.");
         }
 
-        var integration = await integrationRepository.CreateAsync(new OrganizationIntegration
-        {
-            OrganizationId = organizationId,
-            Type = IntegrationType.Slack,
-            Configuration = JsonSerializer.Serialize(new SlackIntegration(token)),
-        });
-        var location = $"/organizations/{organizationId}/integrations/{integration.Id}";
+        integration.Configuration = JsonSerializer.Serialize(new SlackIntegration(token));
+        await integrationRepository.UpsertAsync(integration);
 
+        var location = $"/organizations/{integration.OrganizationId}/integrations/{integration.Id}";
         return Created(location, new OrganizationIntegrationResponseModel(integration));
     }
 }

src/Api/AdminConsole/Models/Response/Organizations/OrganizationIntegrationResponseModel.cs

@@ -2,8 +2,6 @@
 using Bit.Core.Enums;
 using Bit.Core.Models.Api;
 
-#nullable enable
-
 namespace Bit.Api.AdminConsole.Models.Response.Organizations;
 
 public class OrganizationIntegrationResponseModel : ResponseModel
@@ -21,4 +19,29 @@ public class OrganizationIntegrationResponseModel : ResponseModel
     public Guid Id { get; set; }
     public IntegrationType Type { get; set; }
     public string? Configuration { get; set; }
+
+    public OrganizationIntegrationStatus Status => Type switch
+    {
+        // Not yet implemented, shouldn't be present, NotApplicable
+        IntegrationType.CloudBillingSync => OrganizationIntegrationStatus.NotApplicable,
+        IntegrationType.Scim => OrganizationIntegrationStatus.NotApplicable,
+
+        // Webhook is allowed to be null. If it's present, it's Completed
+        IntegrationType.Webhook => OrganizationIntegrationStatus.Completed,
+
+        // If present and the configuration is null, OAuth has been initiated, and we are
+        // waiting on the return call
+        IntegrationType.Slack => string.IsNullOrWhiteSpace(Configuration)
+            ? OrganizationIntegrationStatus.Initiated
+            : OrganizationIntegrationStatus.Completed,
+
+        // HEC and Datadog should only be allowed to be created non-null.
+        // If they are null, they are Invalid
+        IntegrationType.Hec => string.IsNullOrWhiteSpace(Configuration)
+            ? OrganizationIntegrationStatus.Invalid
+            : OrganizationIntegrationStatus.Completed,
+        IntegrationType.Datadog => string.IsNullOrWhiteSpace(Configuration)
+            ? OrganizationIntegrationStatus.Invalid
+            : OrganizationIntegrationStatus.Completed,
+    };
 }

src/Core/AdminConsole/Enums/OrganizationIntegrationStatus.cs

@@ -0,0 +1,10 @@
+namespace Bit.Api.AdminConsole.Models.Response.Organizations;
+
+public enum OrganizationIntegrationStatus : int
+{
+    NotApplicable,
+    Invalid,
+    Initiated,
+    InProgress,
+    Completed
+}

src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationOAuthState.cs

@@ -0,0 +1,71 @@
+using System.Security.Cryptography;
+using System.Text;
+using Bit.Core.AdminConsole.Entities;
+
+namespace Bit.Core.AdminConsole.Models.Data.EventIntegrations;
+
+public class IntegrationOAuthState
+{
+    private const int _orgHashLength = 12;
+    private static readonly TimeSpan _maxAge = TimeSpan.FromMinutes(20);
+
+    public Guid IntegrationId { get; }
+    private DateTimeOffset Issued { get; }
+    private string OrganizationIdHash { get; }
+
+    private IntegrationOAuthState(Guid integrationId, string organizationIdHash, DateTimeOffset issued)
+    {
+        IntegrationId = integrationId;
+        OrganizationIdHash = organizationIdHash;
+        Issued = issued;
+    }
+
+    public static IntegrationOAuthState FromIntegration(OrganizationIntegration integration, TimeProvider timeProvider)
+    {
+        var integrationId = integration.Id;
+        var issuedUtc = timeProvider.GetUtcNow();
+        var organizationIdHash = ComputeOrgHash(integration.OrganizationId, issuedUtc.ToUnixTimeSeconds());
+
+        return new IntegrationOAuthState(integrationId, organizationIdHash, issuedUtc);
+    }
+
+    public static IntegrationOAuthState? FromString(string state, TimeProvider timeProvider)
+    {
+        if (string.IsNullOrWhiteSpace(state)) return null;
+
+        var parts = state.Split('.');
+        if (parts.Length != 3) return null;
+
+        // Verify timestamp
+        if (!long.TryParse(parts[2], out var unixSeconds)) return null;
+
+        var issuedUtc = DateTimeOffset.FromUnixTimeSeconds(unixSeconds);
+        var now = timeProvider.GetUtcNow();
+        var age = now - issuedUtc;
+
+        if (age > _maxAge) return null;
+
+        // Parse integration id and store org
+        if (!Guid.TryParse(parts[0], out var integrationId)) return null;
+        var organizationIdHash = parts[1];
+
+        return new IntegrationOAuthState(integrationId, organizationIdHash, issuedUtc);
+    }
+
+    public bool ValidateOrg(Guid orgId)
+    {
+        var expected = ComputeOrgHash(orgId, Issued.ToUnixTimeSeconds());
+        return expected == OrganizationIdHash;
+    }
+
+    public override string ToString()
+    {
+        return $"{IntegrationId}.{OrganizationIdHash}.{Issued.ToUnixTimeSeconds()}";
+    }
+
+    private static string ComputeOrgHash(Guid orgId, long timestamp)
+    {
+        var bytes = SHA256.HashData(Encoding.UTF8.GetBytes($"{orgId:N}:{timestamp}"));
+        return Convert.ToHexString(bytes)[.._orgHashLength];
+    }
+}

src/Core/AdminConsole/Services/ISlackService.cs

@@ -5,7 +5,7 @@ public interface ISlackService
     Task<string> GetChannelIdAsync(string token, string channelName);
     Task<List<string>> GetChannelIdsAsync(string token, List<string> channelNames);
     Task<string> GetDmChannelByEmailAsync(string token, string email);
-    string GetRedirectUrl(string redirectUrl);
+    string GetRedirectUrl(string callbackUrl, string state);
     Task<string> ObtainTokenViaOAuth(string code, string redirectUrl);
     Task SendSlackMessageByChannelIdAsync(string token, string message, string channelId);
 }

src/Core/AdminConsole/Services/Implementations/EventIntegrations/SlackService.cs

@@ -19,6 +19,7 @@ public class SlackService(
     private readonly string _slackApiBaseUrl = globalSettings.Slack.ApiBaseUrl;
 
     public const string HttpClientName = "SlackServiceHttpClient";
+    private const string _slackOAuthBaseUri = "https://slack.com/oauth/v2/authorize";
 
     public async Task<string> GetChannelIdAsync(string token, string channelName)
     {
@@ -73,9 +74,18 @@ public class SlackService(
         return await OpenDmChannel(token, userId);
     }
 
-    public string GetRedirectUrl(string redirectUrl)
+    public string GetRedirectUrl(string callbackUrl, string state)
     {
-        return $"https://slack.com/oauth/v2/authorize?client_id={_clientId}&scope={_scopes}&redirect_uri={redirectUrl}";
+        var builder = new UriBuilder(_slackOAuthBaseUri);
+        var query = HttpUtility.ParseQueryString(builder.Query);
+
+        query["client_id"] = _clientId;
+        query["scope"] = _scopes;
+        query["redirect_uri"] = callbackUrl;
+        query["state"] = state;
+
+        builder.Query = query.ToString();
+        return builder.ToString();
     }
 
     public async Task<string> ObtainTokenViaOAuth(string code, string redirectUrl)

src/Core/AdminConsole/Services/NoopImplementations/NoopSlackService.cs

@@ -19,7 +19,7 @@ public class NoopSlackService : ISlackService
         return Task.FromResult(string.Empty);
     }
 
-    public string GetRedirectUrl(string redirectUrl)
+    public string GetRedirectUrl(string callbackUrl, string state)
     {
         return string.Empty;
     }