Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EC-508] SCIM CQRS Refactor - Users/Delete #2261

Merged
merged 16 commits into from
Oct 17, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
148e080
[EC-390] Added Scim.Test unit tests project
r-tome Sep 9, 2022
b925cc7
[EC-390] Added ConflictException type. Updated BadRequestException to…
r-tome Sep 9, 2022
976ac23
[EC-539] Implemented CQRS for Users Delete and added unit tests
r-tome Sep 9, 2022
3c7a42c
[EC-508] Created ScimServiceCollectionExtensions
r-tome Oct 3, 2022
dee798c
[EC-508] Created ExceptionHandlerFilterAttribute on SCIM project
r-tome Oct 3, 2022
a67ca60
Merge branch 'master' into EC-539-scim-cqrs-refactor-users-delete
r-tome Oct 4, 2022
92d32d4
[EC-508] Removed unneeded model from DeleteUserCommand. Removed unnee…
r-tome Oct 4, 2022
1bb485a
[EC-508] Removed Bit.Scim.Models dependency from DeleteUserCommandTests
r-tome Oct 4, 2022
1a07044
[EC-508] Deleted 'DeleteUserCommand' from SCIM; Created commands on C…
r-tome Oct 7, 2022
7144a25
[EC-508] Changed DeleteOrganizationUserCommand back to using IOrganiz…
r-tome Oct 10, 2022
d677877
Merge branch 'master' into EC-539-scim-cqrs-refactor-users-delete
r-tome Oct 10, 2022
defc466
[EC-508] Fixed DeleteOrganizationUserCommand unit tests
r-tome Oct 10, 2022
9f0b671
Merge branch 'EC-539-scim-cqrs-refactor-users-delete' of https://gith…
r-tome Oct 10, 2022
78ef4f2
[EC-508] Remove unneeded obsolete comments. Update DeleteUserAsync Ob…
r-tome Oct 13, 2022
e69cb62
[EC-508] Move DeleteOrganizationUserCommand to OrganizationFeatures f…
r-tome Oct 13, 2022
de90c6b
Merge branch 'feature/scim-cqrs' into EC-539-scim-cqrs-refactor-users…
r-tome Oct 17, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions bitwarden-server.sln
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Api.IntegrationTest", "test
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Scim.IntegrationTest", "bitwarden_license\test\Scim.IntegrationTest\Scim.IntegrationTest.csproj", "{FE998849-5FC8-41A2-B7C9-9227901471A0}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Scim.Test", "bitwarden_license\test\Scim.Test\Scim.Test.csproj", "{B1595DA3-4C60-41AA-8BF0-499A5F75A885}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -238,6 +240,10 @@ Global
{FE998849-5FC8-41A2-B7C9-9227901471A0}.Debug|Any CPU.Build.0 = Debug|Any CPU
{FE998849-5FC8-41A2-B7C9-9227901471A0}.Release|Any CPU.ActiveCfg = Release|Any CPU
{FE998849-5FC8-41A2-B7C9-9227901471A0}.Release|Any CPU.Build.0 = Release|Any CPU
{B1595DA3-4C60-41AA-8BF0-499A5F75A885}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{B1595DA3-4C60-41AA-8BF0-499A5F75A885}.Debug|Any CPU.Build.0 = Debug|Any CPU
{B1595DA3-4C60-41AA-8BF0-499A5F75A885}.Release|Any CPU.ActiveCfg = Release|Any CPU
{B1595DA3-4C60-41AA-8BF0-499A5F75A885}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -276,6 +282,7 @@ Global
{7EFB1124-F40A-40EB-9EDA-94FD540AA8FD} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84F}
{CBE96C6D-A4D6-46E1-94C5-42D6CAD8531C} = {DD5BD056-4AAE-43EF-BBD2-0B569B8DA84F}
{FE998849-5FC8-41A2-B7C9-9227901471A0} = {287CFF34-BBDB-4BC4-AF88-1E19A5A4679B}
{B1595DA3-4C60-41AA-8BF0-499A5F75A885} = {287CFF34-BBDB-4BC4-AF88-1E19A5A4679B}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {E01CBF68-2E20-425F-9EDB-E0A6510CA92F}
Expand Down
32 changes: 32 additions & 0 deletions bitwarden_license/src/Scim/Commands/Users/DeleteUserCommand.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Scim.Commands.Users.Interfaces;
using Bit.Scim.Models;

namespace Bit.Scim.Commands.Users;

public class DeleteUserCommand : IDeleteUserCommand
{
private readonly IOrganizationService _organizationService;
private readonly IOrganizationUserRepository _organizationUserRepository;

public DeleteUserCommand(
IOrganizationService organizationService,
IOrganizationUserRepository organizationUserRepository)
{
_organizationService = organizationService;
_organizationUserRepository = organizationUserRepository;
}

public async Task DeleteUserAsync(Guid organizationId, Guid id, ScimUserRequestModel model)
{
var orgUser = await _organizationUserRepository.GetByIdAsync(id);
if (orgUser == null || orgUser.OrganizationId != organizationId)
{
throw new NotFoundException("User not found.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic already exists within the organizationService.DeleteUserAsync which makes this a bit repetitive. Should we check what error we get back from the OrganizationService and handle it that way instead?

However by removing that logic we've essentially made this command just a direct call to the service. At which point the commands benefit is slightly unclear. (Other than acting as a potential basis for in the future extracting the DeleteUserAsync completely into a command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MGibson1 curious what your thoughts are, in my opinion we can either.

  1. Not create a command for this
  2. Create a command for deleting a organization user, but put it in Core, and deprecate the method on the service. The thinking being that we will eventually want to migrate over to use the command instead of the service.

Copy link
Member

@eliykat eliykat Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote 2, personally. DeleteUserAsync can be a command in Core which can be called from wherever, SCIM might just have some error handling code around it, but wouldn't itself implement a separate query. That makes this refactor more useful. Thoughts @r-tome?

Although @Hinton my question would then be how to handle the call to OrganizationService.HasConfirmedOwnersExceptAsync in OrganizationService.DeleteUserAsync. Is that considered a query by itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 sounds good to me although it can turn out to be a bigger refactor than anticipated.
@eliykat I second that question on OrganizationService.HasConfirmedOwnersExceptAsync and also OrganizationService.DeleteAndPushUserRegistrationAsync. Should I create a new query and command and mark the old methods as obsolete?

Copy link
Member

@Hinton Hinton Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, just having the command call the service would be enough. Once everything uses the command we can extract it. That saves us having two identical implementations.

The alternative is to refactor the organization service to call the command, and ensure we only have a single implementation. If code is generic enough it's fine to continue having it in the service. We should try and avoid scope creep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep this simple and make the command just call the service for now, we can refactor this later when we switch everything to use commands.

There is a slight difference between the implementations. When the user is not found on OrganizationService it returns a BadRequestException whereas SCIM throws a NotFoundException. How do I handle this? Change the thrown exception type to NotFoundExceptionfor both or handle that inside ExceptionHandlerFilterAttribute on SCIM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've up the check back on the command even though it seems redundant just because it's easier to throw a different exception type.

Copy link
Member

@eliykat eliykat Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, it's a bit redundant but it was also already here, and it ensures that we comply with the SCIM protocol without causing regressions in other clients. From Oscar's comments above I take it that we don't want to expand the scope of this refactor too much.


await _organizationService.DeleteUserAsync(organizationId, id, null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using Bit.Scim.Models;

namespace Bit.Scim.Commands.Users.Interfaces;

public interface IDeleteUserCommand
{
Task DeleteUserAsync(Guid organizationId, Guid id, ScimUserRequestModel model);
}
21 changes: 14 additions & 7 deletions bitwarden_license/src/Scim/Controllers/v2/UsersController.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Models.Data;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Utilities;
using Bit.Scim.Commands.Users.Interfaces;
using Bit.Scim.Context;
using Bit.Scim.Models;
using Microsoft.AspNetCore.Authorization;
Expand All @@ -21,6 +23,7 @@ public class UsersController : Controller
private readonly IOrganizationService _organizationService;
private readonly IScimContext _scimContext;
private readonly ScimSettings _scimSettings;
private readonly IDeleteUserCommand _deleteUserCommand;
private readonly ILogger<UsersController> _logger;

public UsersController(
Expand All @@ -30,6 +33,7 @@ public UsersController(
IOrganizationService organizationService,
IScimContext scimContext,
IOptions<ScimSettings> scimSettings,
IDeleteUserCommand deleteUserCommand,
ILogger<UsersController> logger)
{
_userService = userService;
Expand All @@ -38,6 +42,7 @@ public UsersController(
_organizationService = organizationService;
_scimContext = scimContext;
_scimSettings = scimSettings?.Value;
_deleteUserCommand = deleteUserCommand;
_logger = logger;
}

Expand Down Expand Up @@ -264,17 +269,19 @@ public async Task<IActionResult> Patch(Guid organizationId, Guid id, [FromBody]
[HttpDelete("{id}")]
public async Task<IActionResult> Delete(Guid organizationId, Guid id, [FromBody] ScimUserRequestModel model)
{
var orgUser = await _organizationUserRepository.GetByIdAsync(id);
if (orgUser == null || orgUser.OrganizationId != organizationId)
try
{
return new NotFoundObjectResult(new ScimErrorResponseModel
await _deleteUserCommand.DeleteUserAsync(organizationId, id, model);
return new NoContentResult();
}
catch (NotFoundException ex)
{
return NotFound(new ScimErrorResponseModel
{
Status = 404,
Detail = "User not found."
Status = StatusCodes.Status404NotFound,
Detail = ex.Message
});
}
await _organizationService.DeleteUserAsync(organizationId, id, null);
return new NoContentResult();
}

private async Task<bool> HandleActiveOperationAsync(Core.Entities.OrganizationUser orgUser, bool active)
Expand Down
4 changes: 4 additions & 0 deletions bitwarden_license/src/Scim/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
using Bit.Core.Context;
using Bit.Core.Settings;
using Bit.Core.Utilities;
using Bit.Scim.Commands.Users;
using Bit.Scim.Commands.Users.Interfaces;
using Bit.Scim.Context;
using Bit.Scim.Utilities;
using Bit.SharedWeb.Utilities;
Expand Down Expand Up @@ -75,6 +77,8 @@ public void ConfigureServices(IServiceCollection services)
config.Filters.Add(new LoggingExceptionHandlerFilterAttribute());
});
services.Configure<RouteOptions>(options => options.LowercaseUrls = true);

services.AddScoped<IDeleteUserCommand, DeleteUserCommand>();
}

public void Configure(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using Bit.Core.Entities;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Scim.Commands.Users;
using Bit.Scim.Models;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using NSubstitute;
using Xunit;

namespace Bit.Scim.Test.Commands.Users;

[SutProviderCustomize]
public class DeleteUserCommandTests
{
[Theory]
[BitAutoData]
public async Task DeleteUser_Success(SutProvider<DeleteUserCommand> sutProvider, Guid organizationId, Guid organizationUserId, ScimUserRequestModel model)
{
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUserId)
.Returns(new OrganizationUser
{
Id = organizationUserId,
OrganizationId = organizationId
});

await sutProvider.Sut.DeleteUserAsync(organizationId, organizationUserId, model);

await sutProvider.GetDependency<IOrganizationUserRepository>().Received(1).GetByIdAsync(organizationUserId);
await sutProvider.GetDependency<IOrganizationService>().Received(1).DeleteUserAsync(organizationId, organizationUserId, null);
}

[Theory]
[BitAutoData]
public async Task DeleteUser_NotFound_Throws(SutProvider<DeleteUserCommand> sutProvider, Guid organizationId, Guid organizationUserId, ScimUserRequestModel model)
{
await Assert.ThrowsAsync<NotFoundException>(async () => await sutProvider.Sut.DeleteUserAsync(organizationId, organizationUserId, model));
}

[Theory]
[BitAutoData]
public async Task DeleteUser_MismatchingOrganizationId_Throws(SutProvider<DeleteUserCommand> sutProvider, Guid organizationId, Guid organizationUserId, ScimUserRequestModel model)
{
sutProvider.GetDependency<IOrganizationUserRepository>()
.GetByIdAsync(organizationUserId)
.Returns(new OrganizationUser
{
Id = organizationUserId,
OrganizationId = Guid.NewGuid()
});

await Assert.ThrowsAsync<NotFoundException>(async () => await sutProvider.Sut.DeleteUserAsync(organizationId, organizationUserId, model));
}
}
25 changes: 25 additions & 0 deletions bitwarden_license/test/Scim.Test/Scim.Test.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<IsPackable>false</IsPackable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNetTestSdkVersion)" />
<PackageReference Include="xunit" Version="$(XUnitVersion)" />
<PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioVersion)">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
<PackageReference Include="coverlet.collector" Version="$(CoverletCollectorVersion)">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
<PackageReference Include="NSubstitute" Version="$(NSubstitueVersion)" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\Scim\Scim.csproj" />
<ProjectReference Include="..\..\..\test\Common\Common.csproj" />
</ItemGroup>
</Project>
Loading