[PM-38769] Fix OrganizationConnectionsController logic

src/Api/AdminConsole/Controllers/OrganizationConnectionsController.cs

@@ -1,7 +1,4 @@
-// FIXME: Update this file to be null safe and then delete the line below
-#nullable disable
-
-using Bit.Api.AdminConsole.Models.Request.Organizations;
+using Bit.Api.AdminConsole.Models.Request.Organizations;
 using Bit.Api.AdminConsole.Models.Response.Organizations;
 using Bit.Core.AdminConsole.Models.OrganizationConnectionConfigs;
 using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationConnections.Interfaces;
@@ -57,7 +54,7 @@ public bool ConnectionsEnabled()
     [HttpPost]
     public async Task<OrganizationConnectionResponseModel> CreateConnection([FromBody] OrganizationConnectionRequestModel model)
     {
-        if (!await HasPermissionAsync(model?.OrganizationId, model?.Type))
+        if (!await HasPermissionAsync(model.OrganizationId, model.Type))
         {
             throw new BadRequestException($"You do not have permission to create a connection of type {model.Type}.");
         }
@@ -92,11 +89,16 @@ public async Task<OrganizationConnectionResponseModel> UpdateConnection(Guid org
             throw new NotFoundException();
         }
 
-        if (!await HasPermissionAsync(model?.OrganizationId, model?.Type))
+        if (!await HasPermissionAsync(existingOrganizationConnection.OrganizationId, existingOrganizationConnection.Type))
         {
             throw new BadRequestException("You do not have permission to update this connection.");
         }
 
+        if (model.Type != existingOrganizationConnection.Type)
+        {
+            throw new BadRequestException("The connection type cannot be changed.");
+        }
+
         if (await HasConnectionTypeAsync(model, organizationConnectionId, model.Type))
         {
             throw new BadRequestException($"The requested organization already has a connection of type {model.Type}. Only one of each connection type may exist per organization.");
@@ -157,13 +159,6 @@ public async Task DeleteConnection(Guid organizationConnectionId)
         await _deleteOrganizationConnectionCommand.DeleteAsync(connection);
     }
 
-    [HttpPost("{organizationConnectionId}/delete")]
-    [Obsolete("This endpoint is deprecated. Use DELETE method instead")]
-    public async Task PostDeleteConnection(Guid organizationConnectionId)
-    {
-        await DeleteConnection(organizationConnectionId);
-    }
-
     private async Task<ICollection<OrganizationConnection>> GetConnectionsAsync(Guid organizationId, OrganizationConnectionType type) =>
         await _organizationConnectionRepository.GetByOrganizationIdTypeAsync(organizationId, type);
 
@@ -175,6 +170,15 @@ private async Task<bool> HasConnectionTypeAsync(OrganizationConnectionRequestMod
         return existingConnections.Any(c => c.Type == model.Type && (!connectionId.HasValue || c.Id != connectionId.Value));
     }
 
+    /// <summary>
+    /// Returns whether the current user has permission to manage a connection of the given <paramref name="type"/>
+    /// in the given organization. The required permission varies by connection type (e.g. Scim requires Manage SCIM,
+    /// while CloudBillingSync requires Organization Owner).
+    /// </summary>
+    /// <remarks>
+    /// When authorizing an update or delete against an existing connection, <paramref name="type"/> MUST be sourced
+    /// from the persisted connection — never from the request body.
+    /// </remarks>
     private async Task<bool> HasPermissionAsync(Guid? organizationId, OrganizationConnectionType? type = null)
     {
         if (!organizationId.HasValue)
@@ -195,7 +199,8 @@ private async Task ValidateBillingSyncConfig(OrganizationConnectionRequestModel<
             throw new BadRequestException($"Cannot create a {typedModel.Type} connection outside of a self-hosted instance.");
         }
         var license = await _licensingService.ReadOrganizationLicenseAsync(typedModel.OrganizationId);
-        if (!_licensingService.VerifyLicense(license))
+
+        if (license == null || !_licensingService.VerifyLicense(license))
         {
             throw new BadRequestException("Cannot verify license file.");
         }
@@ -205,7 +210,7 @@ private async Task ValidateBillingSyncConfig(OrganizationConnectionRequestModel<
     private async Task<OrganizationConnectionResponseModel> CreateOrUpdateOrganizationConnectionAsync<T>(
         Guid? organizationConnectionId,
         OrganizationConnectionRequestModel model,
-        Func<OrganizationConnectionRequestModel<T>, Task> validateAction = null)
+        Func<OrganizationConnectionRequestModel<T>, Task>? validateAction = null)
         where T : IConnectionConfig
     {
         var typedModel = new OrganizationConnectionRequestModel<T>(model);

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

@@ -2,6 +2,7 @@
 #nullable disable
 
 using System.Text.Json;
+using System.Text.Json.Serialization;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
 
@@ -15,6 +16,9 @@ public class OrganizationConnectionResponseModel
     public bool Enabled { get; set; }
     public JsonDocument Config { get; set; }
 
+    [JsonConstructor]
+    public OrganizationConnectionResponseModel() { }
+
     public OrganizationConnectionResponseModel(OrganizationConnection connection, Type configType)
     {
         if (connection == null)

test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationConnectionsControllerTests.cs

@@ -0,0 +1,176 @@
+using System.Net;
+using Bit.Api.AdminConsole.Models.Response.Organizations;
+using Bit.Api.IntegrationTest.Factories;
+using Bit.Api.IntegrationTest.Helpers;
+using Bit.Core.AdminConsole.Entities;
+using Bit.Core.AdminConsole.Models.OrganizationConnectionConfigs;
+using Bit.Core.Billing.Enums;
+using Bit.Core.Entities;
+using Bit.Core.Enums;
+using Bit.Core.Repositories;
+using Xunit;
+
+namespace Bit.Api.IntegrationTest.AdminConsole.Controllers;
+
+public class OrganizationConnectionsControllerTests : IClassFixture<ApiApplicationFactory>, IAsyncLifetime
+{
+    private readonly HttpClient _client;
+    private readonly ApiApplicationFactory _factory;
+    private readonly LoginHelper _loginHelper;
+
+    private Organization _organization = null!;
+    private string _ownerEmail = null!;
+
+    public OrganizationConnectionsControllerTests(ApiApplicationFactory apiFactory)
+    {
+        _factory = apiFactory;
+        _client = _factory.CreateClient();
+        _loginHelper = new LoginHelper(_factory, _client);
+    }
+
+    public async Task InitializeAsync()
+    {
+        _ownerEmail = $"org-connections-test-{Guid.NewGuid()}@example.com";
+        await _factory.LoginWithNewAccount(_ownerEmail);
+
+        (_organization, _) = await OrganizationTestHelpers.SignUpAsync(_factory, plan: PlanType.EnterpriseAnnually,
+            ownerEmail: _ownerEmail, passwordManagerSeats: 5, paymentMethod: PaymentMethodType.Card);
+    }
+
+    public Task DisposeAsync()
+    {
+        _client.Dispose();
+        return Task.CompletedTask;
+    }
+
+    [Fact]
+    public async Task CreateConnection_AsOwner_Succeeds()
+    {
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var response = await _client.PostAsJsonAsync(
+            "organizations/connections",
+            BuildScimRequest(_organization.Id, enabled: true));
+
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        var responseBody = await response.Content.ReadFromJsonAsync<OrganizationConnectionResponseModel>();
+        Assert.NotNull(responseBody);
+        Assert.Equal(OrganizationConnectionType.Scim, responseBody!.Type);
+        Assert.Equal(_organization.Id, responseBody.OrganizationId);
+
+        var connectionRepository = _factory.GetService<IOrganizationConnectionRepository>();
+        var persisted = await connectionRepository.GetByOrganizationIdTypeAsync(
+            _organization.Id, OrganizationConnectionType.Scim);
+        var single = Assert.Single(persisted);
+        Assert.True(single.GetConfig<ScimConfig>()!.Enabled);
+    }
+
+    [Fact]
+    public async Task CreateConnection_AsRegularUser_Forbidden()
+    {
+        var (regularUserEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(
+            _factory, _organization.Id, OrganizationUserType.User);
+        await _loginHelper.LoginAsync(regularUserEmail);
+
+        var response = await _client.PostAsJsonAsync(
+            "organizations/connections",
+            BuildScimRequest(_organization.Id, enabled: true));
+
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+        var body = await response.Content.ReadAsStringAsync();
+        Assert.Contains("You do not have permission to create a connection of type", body);
+    }
+
+    [Fact]
+    public async Task UpdateConnection_AsOwner_Succeeds()
+    {
+        var existing = await SeedScimConnectionAsync(enabled: false);
+
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var response = await _client.PutAsJsonAsync(
+            $"organizations/connections/{existing.Id}",
+            BuildScimRequest(_organization.Id, enabled: true));
+
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+
+        var connectionRepository = _factory.GetService<IOrganizationConnectionRepository>();
+        var persisted = await connectionRepository.GetByIdAsync(existing.Id);
+        Assert.NotNull(persisted);
+        Assert.True(persisted!.Enabled);
+        Assert.True(persisted.GetConfig<ScimConfig>()!.Enabled);
+    }
+
+    [Fact]
+    public async Task UpdateConnection_AsRegularUser_Forbidden()
+    {
+        var existing = await SeedScimConnectionAsync();
+
+        var (regularUserEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(
+            _factory, _organization.Id, OrganizationUserType.User);
+        await _loginHelper.LoginAsync(regularUserEmail);
+
+        var response = await _client.PutAsJsonAsync(
+            $"organizations/connections/{existing.Id}",
+            BuildScimRequest(_organization.Id, enabled: false));
+
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+        var body = await response.Content.ReadAsStringAsync();
+        Assert.Contains("You do not have permission to update this connection.", body);
+    }
+
+    [Fact]
+    public async Task GetConnection_AsOwner_Succeeds()
+    {
+        var existing = await SeedScimConnectionAsync();
+
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var response = await _client.GetAsync(
+            $"organizations/connections/{_organization.Id}/{(int)OrganizationConnectionType.Scim}");
+
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        var responseBody = await response.Content.ReadFromJsonAsync<OrganizationConnectionResponseModel>();
+        Assert.NotNull(responseBody);
+        Assert.Equal(existing.Id, responseBody!.Id);
+        Assert.Equal(OrganizationConnectionType.Scim, responseBody.Type);
+    }
+
+    [Fact]
+    public async Task DeleteConnection_AsOwner_Succeeds()
+    {
+        var existing = await SeedScimConnectionAsync();
+
+        await _loginHelper.LoginAsync(_ownerEmail);
+
+        var response = await _client.DeleteAsync($"organizations/connections/{existing.Id}");
+
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+
+        var connectionRepository = _factory.GetService<IOrganizationConnectionRepository>();
+        var persisted = await connectionRepository.GetByIdAsync(existing.Id);
+        Assert.Null(persisted);
+    }
+
+    private async Task<OrganizationConnection> SeedScimConnectionAsync(bool enabled = true)
+    {
+        var connectionRepository = _factory.GetService<IOrganizationConnectionRepository>();
+        var connection = new OrganizationConnection
+        {
+            OrganizationId = _organization.Id,
+            Type = OrganizationConnectionType.Scim,
+            Enabled = enabled,
+        };
+        connection.SetConfig(new ScimConfig { Enabled = enabled });
+        return await connectionRepository.CreateAsync(connection);
+    }
+
+    private static object BuildScimRequest(Guid organizationId, bool enabled) =>
+        new
+        {
+            type = OrganizationConnectionType.Scim,
+            organizationId,
+            enabled,
+            config = new ScimConfig { Enabled = enabled },
+        };
+}

test/Api.Test/AdminConsole/Controllers/OrganizationConnectionsControllerTests.cs

@@ -408,6 +408,88 @@ public async Task DeleteConnection_Success(OrganizationConnection connection,
         await sutProvider.GetDependency<IDeleteOrganizationConnectionCommand>().DeleteAsync(connection);
     }
 
+    [Theory]
+    [BitAutoData]
+    public async Task UpdateConnection_PermissionCheckUsesPersistedType_NotRequestType(
+        Guid connectionId, Guid organizationId, BillingSyncConfig config,
+        SutProvider<OrganizationConnectionsController> sutProvider)
+    {
+        var existing = new OrganizationConnection
+        {
+            Id = connectionId,
+            Type = OrganizationConnectionType.CloudBillingSync,
+            OrganizationId = organizationId,
+            Config = JsonSerializer.Serialize(config),
+        };
+
+        var request = new OrganizationConnectionRequestModel
+        {
+            Type = OrganizationConnectionType.Scim,
+            OrganizationId = organizationId,
+            Enabled = true,
+            Config = JsonDocumentFromObject(new ScimConfig()),
+        };
+
+        sutProvider.GetDependency<IOrganizationConnectionRepository>()
+            .GetByIdOrganizationIdAsync(connectionId, organizationId)
+            .Returns(existing);
+
+        // The user can update a Scim connection but not a Cloud Billing Sync connection
+        sutProvider.GetDependency<ICurrentContext>().ManageScim(organizationId).Returns(true);
+        sutProvider.GetDependency<ICurrentContext>().OrganizationOwner(organizationId).Returns(false);
+
+        var exception = await Assert.ThrowsAsync<BadRequestException>(
+            () => sutProvider.Sut.UpdateConnection(connectionId, request));
+
+        Assert.Contains("You do not have permission to update this connection.", exception.Message);
+        await sutProvider.GetDependency<IUpdateOrganizationConnectionCommand>()
+            .DidNotReceiveWithAnyArgs()
+            .UpdateAsync<BillingSyncConfig>(default);
+        await sutProvider.GetDependency<IUpdateOrganizationConnectionCommand>()
+            .DidNotReceiveWithAnyArgs()
+            .UpdateAsync<ScimConfig>(default);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task UpdateConnection_TypeCannotBeChanged(
+        Guid connectionId, Guid organizationId, BillingSyncConfig config,
+        SutProvider<OrganizationConnectionsController> sutProvider)
+    {
+        var existing = new OrganizationConnection
+        {
+            Id = connectionId,
+            Type = OrganizationConnectionType.CloudBillingSync,
+            OrganizationId = organizationId,
+            Config = JsonSerializer.Serialize(config),
+        };
+
+        var request = new OrganizationConnectionRequestModel
+        {
+            Type = OrganizationConnectionType.Scim,
+            OrganizationId = organizationId,
+            Enabled = true,
+            Config = JsonDocumentFromObject(new ScimConfig()),
+        };
+
+        sutProvider.GetDependency<IOrganizationConnectionRepository>()
+            .GetByIdOrganizationIdAsync(connectionId, organizationId)
+            .Returns(existing);
+        sutProvider.GetDependency<ICurrentContext>().OrganizationOwner(organizationId).Returns(true);
+        sutProvider.GetDependency<ICurrentContext>().ManageScim(organizationId).Returns(true);
+
+        var exception = await Assert.ThrowsAsync<BadRequestException>(
+            () => sutProvider.Sut.UpdateConnection(connectionId, request));
+
+        Assert.Contains("The connection type cannot be changed.", exception.Message);
+        await sutProvider.GetDependency<IUpdateOrganizationConnectionCommand>()
+            .DidNotReceiveWithAnyArgs()
+            .UpdateAsync<BillingSyncConfig>(default);
+        await sutProvider.GetDependency<IUpdateOrganizationConnectionCommand>()
+            .DidNotReceiveWithAnyArgs()
+            .UpdateAsync<ScimConfig>(default);
+    }
+
     private static OrganizationConnectionRequestModel<T> RequestModelFromEntity<T>(OrganizationConnection entity)
         where T : IConnectionConfig
     {