[EC-507] SCIM CQRS Refactor - Groups/Put

[r-tome]

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Refactor the SCIM API to use the CQRS pattern which implies that each action must be isolated in a specific command to do only one specific task.
Added unit tests to check that each command completes its task.

Code changes

• bitwarden-server.sln: Added new unit tests project Scim.Test
• bitwarden_license/src/Scim/Commands/Groups/Interfaces/*: SCIM group endpoints command interfaces
• bitwarden_license/src/Scim/Commands/Groups/*: SCIM group endpoints command implementations
• bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: Updated to use commands and handle exceptions
• bitwarden_license/src/Scim/Startup.cs: Registered commands for injection
• bitwarden_license/test/Scim.Test/Commands/Groups/*: SCIM group command unit tests
• bitwarden_license/test/Scim.Test/Scim.Test.csproj: New unit test project for SCIM
• bitwarden_license/test/Scim.Test/packages.lock.json: Scim.Test project package dependencies
• src/Core/Exceptions/BadRequestException.cs: Added parameterless constructor
• src/Core/Exceptions/ConflictException.cs: Added a new Exception type for Conflict
• src/Core/Exceptions/NotFoundException.cs: Added parameterless constructor

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• If making database changes - make sure you also update Entity Framework queries and/or migrations
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[eliykat]

Should we check that it's actually updated group.Name?

[r-tome]

I've added an extra check for group.Name on both the SaveAsync group parameter and the returned command result.

[eliykat]

I was thinking of checking that the group object has the name updated at the time it's passed to GroupService.SaveAsync. But because we're mutating the object, this is actually fairly difficult to test in this level of detail. So that's okay, we have integration tests for db state anyway.

[r-tome]

That is already being tested here:

Assert.Equal(displayName, group.Name);

await sutProvider.GetDependency<IGroupService>().Received(1).SaveAsync(group);

[eliykat]

Received only checks the object reference, it doesn't do a deep comparison on the object. So we know that the group object was passed to SaveAsync, but we don't know that it had the correct Name at the time. We only know that group.Name is correct after the whole command has finished executing.

If you don't follow me, try reordering the code in the command so that the group name is changed after the GroupService.SaveAsync call. The unit test will still pass.

I thought that maybe we could do Arg.Is(g => g.Name == displayName), but that suffers from the same problem. It compares g.Name at the time the matcher is evaluated, not at the time the service was called. See "Modifying values being matched" here: https://nsubstitute.github.io/help/argument-matchers/

But, as I said, not that concerned about it, unless you feel like diving into it further. 🤷

[r-tome]

That's a really good explanation, now I've learned something new. Thanks for that! I'll keep the direct assertion for now.

[eliykat]

Just noticed this