[PM-3779] idor allow the attacker to delete the victim domain (#3308)

* [PM-3779] Added IOrganizationDomainRepository.GetDomainByIdAndOrganizationIdAsync and SQL stored procedure

* [PM-3779] Changed GetOrganizationDomainByIdQuery to also take OrgId as a parameter. Updated existing unit tests and added new. Updated controller to match command changes

* [PM-3779] Removed type from url routes

* [PM-3779] Renamed IGetOrganizationDomainByIdAndOrganizationIdQuery to IGetOrganizationDomainByIdOrganizationIdQuery

* [PM-3779] Renamed GetOrganizationDomainByIdOrganizationIdQueryTests file and added more tests

src/Api/Controllers/OrganizationDomainController.cs

@@ -19,7 +19,7 @@ public class OrganizationDomainController : Controller
     private readonly ICreateOrganizationDomainCommand _createOrganizationDomainCommand;
     private readonly IVerifyOrganizationDomainCommand _verifyOrganizationDomainCommand;
     private readonly IDeleteOrganizationDomainCommand _deleteOrganizationDomainCommand;
-    private readonly IGetOrganizationDomainByIdQuery _getOrganizationDomainByIdQuery;
+    private readonly IGetOrganizationDomainByIdOrganizationIdQuery _getOrganizationDomainByIdAndOrganizationIdQuery;
     private readonly IGetOrganizationDomainByOrganizationIdQuery _getOrganizationDomainByOrganizationIdQuery;
     private readonly ICurrentContext _currentContext;
     private readonly IOrganizationRepository _organizationRepository;
@@ -29,7 +29,7 @@ public class OrganizationDomainController : Controller
         ICreateOrganizationDomainCommand createOrganizationDomainCommand,
         IVerifyOrganizationDomainCommand verifyOrganizationDomainCommand,
         IDeleteOrganizationDomainCommand deleteOrganizationDomainCommand,
-        IGetOrganizationDomainByIdQuery getOrganizationDomainByIdQuery,
+        IGetOrganizationDomainByIdOrganizationIdQuery getOrganizationDomainByIdAndOrganizationIdQuery,
         IGetOrganizationDomainByOrganizationIdQuery getOrganizationDomainByOrganizationIdQuery,
         ICurrentContext currentContext,
         IOrganizationRepository organizationRepository,
@@ -38,7 +38,7 @@ public class OrganizationDomainController : Controller
         _createOrganizationDomainCommand = createOrganizationDomainCommand;
         _verifyOrganizationDomainCommand = verifyOrganizationDomainCommand;
         _deleteOrganizationDomainCommand = deleteOrganizationDomainCommand;
-        _getOrganizationDomainByIdQuery = getOrganizationDomainByIdQuery;
+        _getOrganizationDomainByIdAndOrganizationIdQuery = getOrganizationDomainByIdAndOrganizationIdQuery;
         _getOrganizationDomainByOrganizationIdQuery = getOrganizationDomainByOrganizationIdQuery;
         _currentContext = currentContext;
         _organizationRepository = organizationRepository;
@@ -46,71 +46,78 @@ public class OrganizationDomainController : Controller
     }
 
     [HttpGet("{orgId}/domain")]
-    public async Task<ListResponseModel<OrganizationDomainResponseModel>> Get(string orgId)
+    public async Task<ListResponseModel<OrganizationDomainResponseModel>> Get(Guid orgId)
     {
-        var orgIdGuid = new Guid(orgId);
-        await ValidateOrganizationAccessAsync(orgIdGuid);
+        await ValidateOrganizationAccessAsync(orgId);
 
         var domains = await _getOrganizationDomainByOrganizationIdQuery
-            .GetDomainsByOrganizationId(orgIdGuid);
+            .GetDomainsByOrganizationIdAsync(orgId);
         var response = domains.Select(x => new OrganizationDomainResponseModel(x)).ToList();
         return new ListResponseModel<OrganizationDomainResponseModel>(response);
     }
 
     [HttpGet("{orgId}/domain/{id}")]
-    public async Task<OrganizationDomainResponseModel> Get(string orgId, string id)
+    public async Task<OrganizationDomainResponseModel> Get(Guid orgId, Guid id)
     {
-        var orgIdGuid = new Guid(orgId);
-        var IdGuid = new Guid(id);
-        await ValidateOrganizationAccessAsync(orgIdGuid);
+        await ValidateOrganizationAccessAsync(orgId);
 
-        var domain = await _getOrganizationDomainByIdQuery.GetOrganizationDomainById(IdGuid);
+        var organizationDomain = await _getOrganizationDomainByIdAndOrganizationIdQuery
+            .GetOrganizationDomainByIdOrganizationIdAsync(id, orgId);
+        if (organizationDomain is null)
+        {
+            throw new NotFoundException();
+        }
+
+        return new OrganizationDomainResponseModel(organizationDomain);
+    }
+
+    [HttpPost("{orgId}/domain")]
+    public async Task<OrganizationDomainResponseModel> Post(Guid orgId,
+        [FromBody] OrganizationDomainRequestModel model)
+    {
+        await ValidateOrganizationAccessAsync(orgId);
+
+        var organizationDomain = new OrganizationDomain
+        {
+            OrganizationId = orgId,
+            Txt = model.Txt,
+            DomainName = model.DomainName.ToLower()
+        };
+
+        organizationDomain = await _createOrganizationDomainCommand.CreateAsync(organizationDomain);
+
+        return new OrganizationDomainResponseModel(organizationDomain);
+    }
+
+    [HttpPost("{orgId}/domain/{id}/verify")]
+    public async Task<OrganizationDomainResponseModel> Verify(Guid orgId, Guid id)
+    {
+        await ValidateOrganizationAccessAsync(orgId);
+
+        var organizationDomain = await _organizationDomainRepository.GetDomainByIdOrganizationIdAsync(id, orgId);
+        if (organizationDomain is null)
+        {
+            throw new NotFoundException();
+        }
+
+        organizationDomain = await _verifyOrganizationDomainCommand.VerifyOrganizationDomainAsync(organizationDomain);
+
+        return new OrganizationDomainResponseModel(organizationDomain);
+    }
+
+    [HttpDelete("{orgId}/domain/{id}")]
+    [HttpPost("{orgId}/domain/{id}/remove")]
+    public async Task RemoveDomain(Guid orgId, Guid id)
+    {
+        await ValidateOrganizationAccessAsync(orgId);
+
+        var domain = await _organizationDomainRepository.GetDomainByIdOrganizationIdAsync(id, orgId);
         if (domain is null)
         {
             throw new NotFoundException();
         }
 
-        return new OrganizationDomainResponseModel(domain);
-    }
-
-    [HttpPost("{orgId}/domain")]
-    public async Task<OrganizationDomainResponseModel> Post(string orgId,
-        [FromBody] OrganizationDomainRequestModel model)
-    {
-        var orgIdGuid = new Guid(orgId);
-        await ValidateOrganizationAccessAsync(orgIdGuid);
-
-        var organizationDomain = new OrganizationDomain
-        {
-            OrganizationId = orgIdGuid,
-            Txt = model.Txt,
-            DomainName = model.DomainName.ToLower()
-        };
-
-        var domain = await _createOrganizationDomainCommand.CreateAsync(organizationDomain);
-        return new OrganizationDomainResponseModel(domain);
-    }
-
-    [HttpPost("{orgId}/domain/{id}/verify")]
-    public async Task<OrganizationDomainResponseModel> Verify(string orgId, string id)
-    {
-        var orgIdGuid = new Guid(orgId);
-        var idGuid = new Guid(id);
-        await ValidateOrganizationAccessAsync(orgIdGuid);
-
-        var domain = await _verifyOrganizationDomainCommand.VerifyOrganizationDomain(idGuid);
-        return new OrganizationDomainResponseModel(domain);
-    }
-
-    [HttpDelete("{orgId}/domain/{id}")]
-    [HttpPost("{orgId}/domain/{id}/remove")]
-    public async Task RemoveDomain(string orgId, string id)
-    {
-        var orgIdGuid = new Guid(orgId);
-        var idGuid = new Guid(id);
-        await ValidateOrganizationAccessAsync(orgIdGuid);
-
-        await _deleteOrganizationDomainCommand.DeleteAsync(idGuid);
+        await _deleteOrganizationDomainCommand.DeleteAsync(domain);
     }
 
     [AllowAnonymous]

src/Core/OrganizationFeatures/OrganizationDomains/DeleteOrganizationDomainCommand.cs

@@ -1,5 +1,5 @@
-using Bit.Core.Enums;
-using Bit.Core.Exceptions;
+using Bit.Core.Entities;
+using Bit.Core.Enums;
 using Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
@@ -18,15 +18,9 @@ public class DeleteOrganizationDomainCommand : IDeleteOrganizationDomainCommand
         _eventService = eventService;
     }
 
-    public async Task DeleteAsync(Guid id)
+    public async Task DeleteAsync(OrganizationDomain organizationDomain)
     {
-        var domain = await _organizationDomainRepository.GetByIdAsync(id);
-        if (domain is null)
-        {
-            throw new NotFoundException();
-        }
-
-        await _organizationDomainRepository.DeleteAsync(domain);
-        await _eventService.LogOrganizationDomainEventAsync(domain, EventType.OrganizationDomain_Removed);
+        await _organizationDomainRepository.DeleteAsync(organizationDomain);
+        await _eventService.LogOrganizationDomainEventAsync(organizationDomain, EventType.OrganizationDomain_Removed);
     }
 }

src/Core/OrganizationFeatures/OrganizationDomains/GetOrganizationDomainByIdOrganizationIdQuery.cs

@@ -4,15 +4,15 @@ using Bit.Core.Repositories;
 
 namespace Bit.Core.OrganizationFeatures.OrganizationDomains;
 
-public class GetOrganizationDomainByIdQuery : IGetOrganizationDomainByIdQuery
+public class GetOrganizationDomainByIdOrganizationIdQuery : IGetOrganizationDomainByIdOrganizationIdQuery
 {
     private readonly IOrganizationDomainRepository _organizationDomainRepository;
 
-    public GetOrganizationDomainByIdQuery(IOrganizationDomainRepository organizationDomainRepository)
+    public GetOrganizationDomainByIdOrganizationIdQuery(IOrganizationDomainRepository organizationDomainRepository)
     {
         _organizationDomainRepository = organizationDomainRepository;
     }
 
-    public async Task<OrganizationDomain> GetOrganizationDomainById(Guid id)
-        => await _organizationDomainRepository.GetByIdAsync(id);
+    public async Task<OrganizationDomain> GetOrganizationDomainByIdOrganizationIdAsync(Guid id, Guid organizationId)
+        => await _organizationDomainRepository.GetDomainByIdOrganizationIdAsync(id, organizationId);
 }

src/Core/OrganizationFeatures/OrganizationDomains/GetOrganizationDomainByOrganizationIdQuery.cs

@@ -13,6 +13,6 @@ public class GetOrganizationDomainByOrganizationIdQuery : IGetOrganizationDomain
         _organizationDomainRepository = organizationDomainRepository;
     }
 
-    public async Task<ICollection<OrganizationDomain>> GetDomainsByOrganizationId(Guid orgId)
+    public async Task<ICollection<OrganizationDomain>> GetDomainsByOrganizationIdAsync(Guid orgId)
         => await _organizationDomainRepository.GetDomainsByOrganizationIdAsync(orgId);
 }

src/Core/OrganizationFeatures/OrganizationDomains/Interfaces/IDeleteOrganizationDomainCommand.cs

@@ -1,6 +1,8 @@
-namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
+using Bit.Core.Entities;
+
+namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
 
 public interface IDeleteOrganizationDomainCommand
 {
-    Task DeleteAsync(Guid id);
+    Task DeleteAsync(OrganizationDomain organizationDomain);
 }

src/Core/OrganizationFeatures/OrganizationDomains/Interfaces/IGetOrganizationDomainByIdOrganizationIdQuery.cs

@@ -0,0 +1,8 @@
+using Bit.Core.Entities;
+
+namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
+
+public interface IGetOrganizationDomainByIdOrganizationIdQuery
+{
+    Task<OrganizationDomain> GetOrganizationDomainByIdOrganizationIdAsync(Guid id, Guid organizationId);
+}

src/Core/OrganizationFeatures/OrganizationDomains/Interfaces/IGetOrganizationDomainByIdQuery.cs

@@ -1,8 +0,0 @@
-using Bit.Core.Entities;
-
-namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
-
-public interface IGetOrganizationDomainByIdQuery
-{
-    Task<OrganizationDomain> GetOrganizationDomainById(Guid id);
-}

src/Core/OrganizationFeatures/OrganizationDomains/Interfaces/IGetOrganizationDomainByOrganizationIdQuery.cs

@@ -4,5 +4,5 @@ namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
 
 public interface IGetOrganizationDomainByOrganizationIdQuery
 {
-    Task<ICollection<OrganizationDomain>> GetDomainsByOrganizationId(Guid orgId);
+    Task<ICollection<OrganizationDomain>> GetDomainsByOrganizationIdAsync(Guid orgId);
 }

src/Core/OrganizationFeatures/OrganizationDomains/Interfaces/IVerifyOrganizationDomainCommand.cs

@@ -4,5 +4,5 @@ namespace Bit.Core.OrganizationFeatures.OrganizationDomains.Interfaces;
 
 public interface IVerifyOrganizationDomainCommand
 {
-    Task<OrganizationDomain> VerifyOrganizationDomain(Guid id);
+    Task<OrganizationDomain> VerifyOrganizationDomainAsync(OrganizationDomain organizationDomain);
 }

src/Core/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs

@@ -27,14 +27,8 @@ public class VerifyOrganizationDomainCommand : IVerifyOrganizationDomainCommand
         _logger = logger;
     }
 
-    public async Task<OrganizationDomain> VerifyOrganizationDomain(Guid id)
+    public async Task<OrganizationDomain> VerifyOrganizationDomainAsync(OrganizationDomain domain)
     {
-        var domain = await _organizationDomainRepository.GetByIdAsync(id);
-        if (domain is null)
-        {
-            throw new NotFoundException();
-        }
-
         if (domain.VerifiedDate is not null)
         {
             domain.SetLastCheckedDate();

src/Core/OrganizationFeatures/OrganizationServiceCollectionExtensions.cs

@@ -118,7 +118,7 @@ public static class OrganizationServiceCollectionExtensions
     {
         services.AddScoped<ICreateOrganizationDomainCommand, CreateOrganizationDomainCommand>();
         services.AddScoped<IVerifyOrganizationDomainCommand, VerifyOrganizationDomainCommand>();
-        services.AddScoped<IGetOrganizationDomainByIdQuery, GetOrganizationDomainByIdQuery>();
+        services.AddScoped<IGetOrganizationDomainByIdOrganizationIdQuery, GetOrganizationDomainByIdOrganizationIdQuery>();
         services.AddScoped<IGetOrganizationDomainByOrganizationIdQuery, GetOrganizationDomainByOrganizationIdQuery>();
         services.AddScoped<IDeleteOrganizationDomainCommand, DeleteOrganizationDomainCommand>();
     }

src/Core/Repositories/IOrganizationDomainRepository.cs

@@ -9,6 +9,7 @@ public interface IOrganizationDomainRepository : IRepository<OrganizationDomain,
     Task<ICollection<OrganizationDomain>> GetDomainsByOrganizationIdAsync(Guid orgId);
     Task<ICollection<OrganizationDomain>> GetManyByNextRunDateAsync(DateTime date);
     Task<OrganizationDomainSsoDetailsData> GetOrganizationDomainSsoDetailsAsync(string email);
+    Task<OrganizationDomain> GetDomainByIdOrganizationIdAsync(Guid id, Guid organizationId);
     Task<OrganizationDomain> GetDomainByOrgIdAndDomainNameAsync(Guid orgId, string domainName);
     Task<ICollection<OrganizationDomain>> GetExpiredOrganizationDomainsAsync();
     Task<bool> DeleteExpiredAsync(int expirationPeriod);

src/Infrastructure.Dapper/Repositories/OrganizationDomainRepository.cs

@@ -69,6 +69,20 @@ public class OrganizationDomainRepository : Repository<OrganizationDomain, Guid>
         }
     }
 
+    public async Task<OrganizationDomain> GetDomainByIdOrganizationIdAsync(Guid id, Guid orgId)
+    {
+        using (var connection = new SqlConnection(ConnectionString))
+        {
+            var results = await connection
+                .QueryAsync<OrganizationDomain>(
+                    $"[{Schema}].[OrganizationDomain_ReadByIdOrganizationId]",
+                    new { Id = id, OrganizationId = orgId },
+                    commandType: CommandType.StoredProcedure);
+
+            return results.SingleOrDefault();
+        }
+    }
+
     public async Task<OrganizationDomain> GetDomainByOrgIdAndDomainNameAsync(Guid orgId, string domainName)
     {
         using (var connection = new SqlConnection(ConnectionString))

src/Infrastructure.EntityFramework/Repositories/OrganizationDomainRepository.cs

@@ -93,6 +93,18 @@ public class OrganizationDomainRepository : Repository<Core.Entities.Organizatio
         return ssoDetails;
     }
 
+    public async Task<Core.Entities.OrganizationDomain> GetDomainByIdOrganizationIdAsync(Guid id, Guid orgId)
+    {
+        using var scope = ServiceScopeFactory.CreateScope();
+        var dbContext = GetDatabaseContext(scope);
+        var domain = await dbContext.OrganizationDomains
+            .Where(x => x.Id == id && x.OrganizationId == orgId)
+            .AsNoTracking()
+            .FirstOrDefaultAsync();
+
+        return Mapper.Map<Core.Entities.OrganizationDomain>(domain);
+    }
+
     public async Task<Core.Entities.OrganizationDomain> GetDomainByOrgIdAndDomainNameAsync(Guid orgId, string domainName)
     {
         using var scope = ServiceScopeFactory.CreateScope();

src/Sql/dbo/Stored Procedures/OrganizationDomain_ReadByIdOrganizationId.sql

@@ -0,0 +1,16 @@
+CREATE PROCEDURE [dbo].[OrganizationDomain_ReadByIdOrganizationId]
+    @Id UNIQUEIDENTIFIER,
+    @OrganizationId UNIQUEIDENTIFIER
+AS
+BEGIN
+    SET NOCOUNT ON
+
+SELECT
+    *
+FROM
+    [dbo].[OrganizationDomain]
+WHERE
+    [Id] = @Id
+  AND
+    [OrganizationId] = @OrganizationId
+END