Changes from Code Review

Author: jrmccannon

src/Core/AdminConsole/AbilitiesCache/ExtendedOrganizationAbilityCacheService.cs

@@ -17,12 +17,10 @@ public class ExtendedOrganizationAbilityCacheService(
     IOrganizationRepository organizationRepository)
     : IOrganizationAbilityCacheService
 {
-
     public async Task<OrganizationAbility?> GetOrganizationAbilityAsync(Guid orgId, CancellationToken cancellationToken = default)
     {
-        var cacheKey = BuildCacheKeyForOrganizationAbility(orgId);
         return await cache.GetOrSetAsync<OrganizationAbility?>(
-            cacheKey,
+            orgId.ToString(),
             async (_, _) => await organizationRepository.GetAbilityAsync(orgId),
             token: cancellationToken);
     }
@@ -38,16 +36,11 @@ public async Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbiliti
 
     public async Task UpsertOrganizationAbilityAsync(Organization organization, CancellationToken cancellationToken = default)
     {
-        var cacheKey = BuildCacheKeyForOrganizationAbility(organization.Id);
-        await cache.SetAsync<OrganizationAbility?>(cacheKey, new OrganizationAbility(organization), token: cancellationToken);
+        await cache.SetAsync<OrganizationAbility?>(organization.Id.ToString(), new OrganizationAbility(organization), token: cancellationToken);
     }
 
     public async Task DeleteOrganizationAbilityAsync(Guid organizationId, CancellationToken cancellationToken = default)
     {
-        var cacheKey = BuildCacheKeyForOrganizationAbility(organizationId);
-        await cache.RemoveAsync(cacheKey, token: cancellationToken);
+        await cache.RemoveAsync(organizationId.ToString(), token: cancellationToken);
     }
-
-    private static string BuildCacheKeyForOrganizationAbility(Guid organizationId)
-        => $"org-ability:{organizationId:N}";
 }

src/Core/AdminConsole/AbilitiesCache/OrganizationAbilityServiceCollectionsExtension.cs

@@ -8,5 +8,5 @@ public static class OrganizationAbilityServiceCollectionsExtension
     public static IServiceCollection AddOrganizationAbilityCache(this IServiceCollection serviceCollection,
         GlobalSettings globalSettings) =>
         serviceCollection.AddExtendedCache(ExtendedOrganizationAbilityCacheConstants.CacheName, globalSettings)
-            .AddScoped<IOrganizationAbilityCacheService, ExtendedOrganizationAbilityCacheService>();
+            .AddSingleton<IOrganizationAbilityCacheService, ExtendedOrganizationAbilityCacheService>();
 }

src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/AcceptOrgUserCommand.cs

@@ -232,9 +232,15 @@ private async Task HandleAutomaticUserConfirmationPolicyAsync(OrganizationUser o
         var policyRequirement = await _policyRequirementQuery.GetAsync<AutomaticUserConfirmationPolicyRequirement>(
             user.Id);
 
+        // Ensure that the user is not already in the list of organization users
+        if (!allOrgUsers.Any(x => x.OrganizationId == orgUser.OrganizationId && x.UserId == user.Id))
+        {
+            allOrgUsers.Add(orgUser);
+        }
+
         var error = (await _automaticUserConfirmationPolicyEnforcementValidator.IsCompliantAsync(
                 new AutomaticUserConfirmationPolicyEnforcementRequest(orgUser.OrganizationId,
-                    allOrgUsers.Append(orgUser),
+                    allOrgUsers,
                     user),
                 policyRequirement))
             .Match(

src/Core/Services/Implementations/FeatureRoutedCacheService.cs

@@ -8,7 +8,7 @@ namespace Bit.Core.Services.Implementations;
 
 public class FeatureRoutedCacheService(
     IVCurrentInMemoryApplicationCacheService inMemoryApplicationCacheService,
-    IOrganizationAbilityCacheService extendedCacheService,
+    IOrganizationAbilityCacheService extendedOrgAbilityCacheService,
     IFeatureService featureService)
     : IApplicationCacheService
 {
@@ -17,7 +17,7 @@ public Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbilitiesAsyn
 
     public Task<OrganizationAbility?> GetOrganizationAbilityAsync(Guid orgId) =>
         featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache)
-            ? extendedCacheService.GetOrganizationAbilityAsync(orgId)
+            ? extendedOrgAbilityCacheService.GetOrganizationAbilityAsync(orgId)
             : inMemoryApplicationCacheService.GetOrganizationAbilityAsync(orgId);
 
     public Task<IDictionary<Guid, ProviderAbility>> GetProviderAbilitiesAsync() =>
@@ -42,7 +42,7 @@ public async Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbiliti
     {
         if (featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache))
         {
-            return await extendedCacheService.GetOrganizationAbilitiesAsync(orgIds);
+            return await extendedOrgAbilityCacheService.GetOrganizationAbilitiesAsync(orgIds);
         }
 
         var allOrganizationAbilities = await inMemoryApplicationCacheService.GetOrganizationAbilitiesAsync();
@@ -54,15 +54,15 @@ public async Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbiliti
 
     public Task UpsertOrganizationAbilityAsync(Organization organization) =>
         featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache)
-            ? extendedCacheService.UpsertOrganizationAbilityAsync(organization)
+            ? extendedOrgAbilityCacheService.UpsertOrganizationAbilityAsync(organization)
             : inMemoryApplicationCacheService.UpsertOrganizationAbilityAsync(organization);
 
     public Task UpsertProviderAbilityAsync(Provider provider) =>
         inMemoryApplicationCacheService.UpsertProviderAbilityAsync(provider);
 
     public Task DeleteOrganizationAbilityAsync(Guid organizationId) =>
         featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache)
-            ? extendedCacheService.DeleteOrganizationAbilityAsync(organizationId)
+            ? extendedOrgAbilityCacheService.DeleteOrganizationAbilityAsync(organizationId)
             : inMemoryApplicationCacheService.DeleteOrganizationAbilityAsync(organizationId);
 
     public Task DeleteProviderAbilityAsync(Guid providerId) =>
@@ -72,7 +72,7 @@ public async Task BaseUpsertOrganizationAbilityAsync(Organization organization)
     {
         if (featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache))
         {
-            await extendedCacheService.UpsertOrganizationAbilityAsync(organization);
+            await extendedOrgAbilityCacheService.UpsertOrganizationAbilityAsync(organization);
             return;
         }
 
@@ -90,7 +90,7 @@ public async Task BaseDeleteOrganizationAbilityAsync(Guid organizationId)
     {
         if (featureService.IsEnabled(FeatureFlagKeys.OrgAbilityExtendedCache))
         {
-            await extendedCacheService.DeleteOrganizationAbilityAsync(organizationId);
+            await extendedOrgAbilityCacheService.DeleteOrganizationAbilityAsync(organizationId);
             return;
         }
 

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

@@ -105,36 +105,27 @@ public async Task Put_UpdatesOrganization_CacheReflectsUpdatedValues()
     [Fact]
     public async Task Delete_RemovesOrganization_CacheReturnsNull()
     {
-        // Arrange - create a separate org for deletion so we don't affect other tests
-        var deleteOwnerEmail = $"delete-test-{Guid.NewGuid()}@example.com";
-        await _factory.LoginWithNewAccount(deleteOwnerEmail);
-
-        var signUpResult = await OrganizationTestHelpers.SignUpAsync(
-            _factory,
-            plan: PlanType.EnterpriseAnnually,
-            ownerEmail: deleteOwnerEmail,
-            passwordManagerSeats: 5,
-            paymentMethod: PaymentMethodType.Card);
-        var orgToDelete = signUpResult.Item1;
+        // Arrange - setup in InitializeAsync()
+        await _loginHelper.LoginAsync(_ownerEmail);
 
         // Verify cache is populated before delete
         var cacheService = _factory.GetService<IApplicationCacheService>();
-        var abilityBeforeDelete = await cacheService.GetOrganizationAbilityAsync(orgToDelete.Id);
+        var abilityBeforeDelete = await cacheService.GetOrganizationAbilityAsync(_organization.Id);
         Assert.NotNull(abilityBeforeDelete);
 
         // Act - delete the organization via the HTTP endpoint
-        await _loginHelper.LoginAsync(deleteOwnerEmail);
+        await _loginHelper.LoginAsync(_ownerEmail);
         var deleteRequest = new SecretVerificationRequestModel
         {
             MasterPasswordHash = "master_password_hash"
         };
         var response = await _client.PostAsJsonAsync(
-            $"/organizations/{orgToDelete.Id}/delete", deleteRequest);
+            $"/organizations/{_organization.Id}/delete", deleteRequest);
 
         // Assert - endpoint succeeded and cache was cleared
         Assert.Equal(HttpStatusCode.OK, response.StatusCode);
 
-        var abilityAfterDelete = await cacheService.GetOrganizationAbilityAsync(orgToDelete.Id);
+        var abilityAfterDelete = await cacheService.GetOrganizationAbilityAsync(_organization.Id);
         Assert.Null(abilityAfterDelete);
     }
 }