From c5d5de0aed34bb92fab4a01e81ee24ffe9fc0115 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Tue, 26 Mar 2024 00:26:12 +1000 Subject: [PATCH] [AC-2334] Fix unable to load members when permissions is "null" (#3922) * Also add xmldoc comment to CoreHelpers.LoadClassFromJsonData to warn about this --- .../OrganizationUsersController.cs | 16 ++- .../ProfileOrganizationResponseModel.cs | 9 +- src/Core/Utilities/CoreHelpers.cs | 8 ++ .../OrganizationUsersControllerTests.cs | 103 ++++++++++++++++++ 4 files changed, 128 insertions(+), 8 deletions(-) diff --git a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs index a478f6e78..9cc62490e 100644 --- a/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs +++ b/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs @@ -94,8 +94,11 @@ public class OrganizationUsersController : Controller response.Type = GetFlexibleCollectionsUserType(response.Type, response.Permissions); // Set 'Edit/Delete Assigned Collections' custom permissions to false - response.Permissions.EditAssignedCollections = false; - response.Permissions.DeleteAssignedCollections = false; + if (response.Permissions is not null) + { + response.Permissions.EditAssignedCollections = false; + response.Permissions.DeleteAssignedCollections = false; + } } if (includeGroups) @@ -552,8 +555,11 @@ public class OrganizationUsersController : Controller orgUser.Type = GetFlexibleCollectionsUserType(orgUser.Type, orgUser.Permissions); // Set 'Edit/Delete Assigned Collections' custom permissions to false - orgUser.Permissions.EditAssignedCollections = false; - orgUser.Permissions.DeleteAssignedCollections = false; + if (orgUser.Permissions is not null) + { + orgUser.Permissions.EditAssignedCollections = false; + orgUser.Permissions.DeleteAssignedCollections = false; + } return orgUser; }); @@ -565,7 +571,7 @@ public class OrganizationUsersController : Controller private OrganizationUserType GetFlexibleCollectionsUserType(OrganizationUserType type, Permissions permissions) { // Downgrade Custom users with no other permissions than 'Edit/Delete Assigned Collections' to User - if (type == OrganizationUserType.Custom) + if (type == OrganizationUserType.Custom && permissions is not null) { if ((permissions.EditAssignedCollections || permissions.DeleteAssignedCollections) && permissions is diff --git a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs index 8fa41a2f5..55c1d9cb1 100644 --- a/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs +++ b/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs @@ -74,7 +74,7 @@ public class ProfileOrganizationResponseModel : ResponseModel if (FlexibleCollections) { // Downgrade Custom users with no other permissions than 'Edit/Delete Assigned Collections' to User - if (Type == OrganizationUserType.Custom) + if (Type == OrganizationUserType.Custom && Permissions is not null) { if ((Permissions.EditAssignedCollections || Permissions.DeleteAssignedCollections) && Permissions is @@ -98,8 +98,11 @@ public class ProfileOrganizationResponseModel : ResponseModel } // Set 'Edit/Delete Assigned Collections' custom permissions to false - Permissions.EditAssignedCollections = false; - Permissions.DeleteAssignedCollections = false; + if (Permissions is not null) + { + Permissions.EditAssignedCollections = false; + Permissions.DeleteAssignedCollections = false; + } } } diff --git a/src/Core/Utilities/CoreHelpers.cs b/src/Core/Utilities/CoreHelpers.cs index b54cbc3f5..5d0becf7b 100644 --- a/src/Core/Utilities/CoreHelpers.cs +++ b/src/Core/Utilities/CoreHelpers.cs @@ -763,6 +763,14 @@ public static class CoreHelpers return claims; } + /// + /// Deserializes JSON data into the specified type. + /// If the JSON data is a null reference, it will still return an instantiated class. + /// However, if the JSON data is a string "null", it will return null. + /// + /// The JSON data + /// The type to deserialize into + /// public static T LoadClassFromJsonData(string jsonData) where T : new() { if (string.IsNullOrWhiteSpace(jsonData)) diff --git a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs index 5af1a8619..363d6db35 100644 --- a/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs +++ b/test/Api.Test/AdminConsole/Controllers/OrganizationUsersControllerTests.cs @@ -8,14 +8,17 @@ using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Context; using Bit.Core.Entities; +using Bit.Core.Enums; using Bit.Core.Models.Data; using Bit.Core.Models.Data.Organizations; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces; using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Authorization; using NSubstitute; using Xunit; @@ -201,4 +204,104 @@ public class OrganizationUsersControllerTests cas.All(c => model.Collections.Any(m => m.Id == c.Id))), model.Groups); } + + [Theory] + [BitAutoData] + public async Task Get_WithFlexibleCollections_ReturnsUsers( + ICollection organizationUsers, OrganizationAbility organizationAbility, + SutProvider sutProvider) + { + Get_Setup(organizationAbility, organizationUsers, sutProvider); + var response = await sutProvider.Sut.Get(organizationAbility.Id); + + Assert.True(response.Data.All(r => organizationUsers.Any(ou => ou.Id == r.Id))); + } + + [Theory] + [BitAutoData] + public async Task Get_WithFlexibleCollections_HandlesNullPermissionsObject( + ICollection organizationUsers, OrganizationAbility organizationAbility, + SutProvider sutProvider) + { + Get_Setup(organizationAbility, organizationUsers, sutProvider); + organizationUsers.First().Permissions = "null"; + var response = await sutProvider.Sut.Get(organizationAbility.Id); + + Assert.True(response.Data.All(r => organizationUsers.Any(ou => ou.Id == r.Id))); + } + + [Theory] + [BitAutoData] + public async Task Get_WithFlexibleCollections_SetsDeprecatedCustomPermissionstoFalse( + ICollection organizationUsers, OrganizationAbility organizationAbility, + SutProvider sutProvider) + { + Get_Setup(organizationAbility, organizationUsers, sutProvider); + + var customUser = organizationUsers.First(); + customUser.Type = OrganizationUserType.Custom; + customUser.Permissions = CoreHelpers.ClassToJsonData(new Permissions + { + AccessReports = true, + EditAssignedCollections = true, + DeleteAssignedCollections = true, + AccessEventLogs = true + }); + + var response = await sutProvider.Sut.Get(organizationAbility.Id); + + var customUserResponse = response.Data.First(r => r.Id == organizationUsers.First().Id); + Assert.Equal(OrganizationUserType.Custom, customUserResponse.Type); + Assert.True(customUserResponse.Permissions.AccessReports); + Assert.True(customUserResponse.Permissions.AccessEventLogs); + Assert.False(customUserResponse.Permissions.EditAssignedCollections); + Assert.False(customUserResponse.Permissions.DeleteAssignedCollections); + } + + [Theory] + [BitAutoData] + public async Task Get_WithFlexibleCollections_DowngradesCustomUsersWithDeprecatedPermissions( + ICollection organizationUsers, OrganizationAbility organizationAbility, + SutProvider sutProvider) + { + Get_Setup(organizationAbility, organizationUsers, sutProvider); + + var customUser = organizationUsers.First(); + customUser.Type = OrganizationUserType.Custom; + customUser.Permissions = CoreHelpers.ClassToJsonData(new Permissions + { + EditAssignedCollections = true, + DeleteAssignedCollections = true, + }); + + var response = await sutProvider.Sut.Get(organizationAbility.Id); + + var customUserResponse = response.Data.First(r => r.Id == organizationUsers.First().Id); + Assert.Equal(OrganizationUserType.User, customUserResponse.Type); + Assert.False(customUserResponse.Permissions.EditAssignedCollections); + Assert.False(customUserResponse.Permissions.DeleteAssignedCollections); + } + + private void Get_Setup(OrganizationAbility organizationAbility, + ICollection organizationUsers, + SutProvider sutProvider) + { + organizationAbility.FlexibleCollections = true; + foreach (var orgUser in organizationUsers) + { + orgUser.Permissions = null; + } + sutProvider.GetDependency().GetOrganizationAbilityAsync(organizationAbility.Id) + .Returns(organizationAbility); + + sutProvider.GetDependency().AuthorizeAsync( + user: Arg.Any(), + resource: Arg.Any(), + requirements: Arg.Any>()) + .Returns(AuthorizationResult.Success()); + + sutProvider.GetDependency() + .GetManyDetailsByOrganizationAsync(organizationAbility.Id, Arg.Any(), Arg.Any()) + .Returns(organizationUsers); + } }