[PM-16811] - SCIM Invite Users Optimizations #5398
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5398 +/- ##
==========================================
+ Coverage 44.50% 47.97% +3.47%
==========================================
Files 1542 1571 +29
Lines 70658 71399 +741
Branches 6322 6392 +70
==========================================
+ Hits 31448 34257 +2809
+ Misses 37864 35708 -2156
- Partials 1346 1434 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (4)Checkmarx found the following issues in this Pull Request
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
|
LaunchDarkly flag references🔍 1 flag added or modified
|
… up a few things.
…viting. Added test to validate Ef and Sproc
# Conflicts: # src/Core/Constants.cs
…erviceTests. Fixed some tests in org service in relation to moving out SendOrgInviteCommand code. Added side effects to InviteOrganizationUsersCommand
… scim. Did a bit of refactoring on the SM validation. Fixed couple bugs found.
There was a problem hiding this comment.
Here are some initial thoughts, broadly limited to the controller, commands, and various models. TODO: review the validation, tests, and generic Result/Validation classes. For that reason some of my questions/thoughts may be answered by further review, but I wanted to get something checked in.
| var organization = await _organizationRepository.GetByIdAsync(organizationId); | ||
| var hasStandaloneSecretsManager = await _paymentService.HasSecretsManagerStandalone(organization); | ||
| var organization = await organizationRepository.GetByIdAsync(organizationId); | ||
| var hasStandaloneSecretsManager = await paymentService.HasSecretsManagerStandalone(organization); |
There was a problem hiding this comment.
paymentService hides a call to the Stripe API, which we want to defer for as long as possible. I think you can move this into the command. Today it's an argument that's passed in, but that just means that it's repeated in every single calling location, which is unnecessary code duplication.
There was a problem hiding this comment.
If a company has secrets manager, every invite will be for both PM and SM?
There was a problem hiding this comment.
Not quite - it depends how they've signed up. I've sent you a DM with the background here because it's lengthy and confusing.
| _factory.SubstituteService((IFeatureService featureService) | ||
| => featureService.IsEnabled(FeatureFlagKeys.ScimInviteUserOptimization) | ||
| .Returns(isScimInviteUserOptimizationEnabled)); |
There was a problem hiding this comment.
Nice idea, but this is too late to affect the creation of the services. Try using the debugger - your new code is not being executed in either test run.
I found that I had to put it in the constructor of the class, e.g. https://github.com/bitwarden/server/blob/main/bitwarden_license/test/Scim.IntegrationTest/Controllers/v2/GroupsControllerPatchTestsvNext.cs#L24-L29. Unfortunately that does require duplication at the class level. There may be a better way to do it (ask Justin if you want to dig into this further).
There was a problem hiding this comment.
I found I could do it at the individual test by creating a new factory and reinitializing the db.
| new(organization.Id, | ||
| organization.UseCustomPermissions, | ||
| organization.Seats, | ||
| organization.MaxAutoscaleSeats, | ||
| organization.SmSeats, | ||
| organization.MaxAutoscaleSmSeats, | ||
| StaticStore.GetPlan(organization.PlanType), | ||
| organization.GatewayCustomerId, | ||
| organization.GatewaySubscriptionId, | ||
| organization.UseSecretsManager); |
There was a problem hiding this comment.
It's more boilerplate, but I strongly prefer named arguments here when we have so many weakly typed parameters (e.g. ints). Sorry about your fancy record syntax ;)
There was a problem hiding this comment.
Its fine. It can still be a record though :D
There was a problem hiding this comment.
This seems like a specific selection of Organization properties, scoped to what's specifically required by this command. Maybe the name should reflect this - e.g. InviteOrganization?
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Outdated
Show resolved
Hide resolved
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Outdated
Show resolved
Hide resolved
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Outdated
Show resolved
Hide resolved
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Outdated
Show resolved
Hide resolved
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/SendOrganizationInvitesCommand.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Great work moving this into a separate class. It's definitely its own weird bespoke logic.
| public static OrganizationDto FromOrganization(Organization organization) => | ||
| new(organization.Id, | ||
| organization.UseCustomPermissions, | ||
| organization.Seats, | ||
| organization.MaxAutoscaleSeats, | ||
| organization.SmSeats, | ||
| organization.MaxAutoscaleSmSeats, | ||
| StaticStore.GetPlan(organization.PlanType), | ||
| organization.GatewayCustomerId, | ||
| organization.GatewaySubscriptionId, | ||
| organization.UseSecretsManager); |
There was a problem hiding this comment.
Constructors are the standard C# way for creating objects so I think using a new constructor is more intuitive
| public static OrganizationDto FromOrganization(Organization organization) => | |
| new(organization.Id, | |
| organization.UseCustomPermissions, | |
| organization.Seats, | |
| organization.MaxAutoscaleSeats, | |
| organization.SmSeats, | |
| organization.MaxAutoscaleSmSeats, | |
| StaticStore.GetPlan(organization.PlanType), | |
| organization.GatewayCustomerId, | |
| organization.GatewaySubscriptionId, | |
| organization.UseSecretsManager); | |
| public OrganizationDto(Organization organization) | |
| : this( | |
| OrganizationId: organization.Id, | |
| UseCustomPermissions: organization.UseCustomPermissions, | |
| Seats: organization.Seats, | |
| MaxAutoScaleSeats: organization.MaxAutoscaleSeats, | |
| SmSeats: organization.SmSeats, | |
| SmMaxAutoScaleSeats: organization.MaxAutoscaleSmSeats, | |
| Plan: StaticStore.GetPlan(organization.PlanType), | |
| GatewayCustomerId: organization.GatewayCustomerId, | |
| GatewaySubscriptionId: organization.GatewaySubscriptionId, | |
| UseSecretsManager: organization.UseSecretsManager) | |
| { | |
| } |
...Console/OrganizationFeatures/OrganizationUsers/InviteUsers/InviteOrganizationUsersCommand.cs
Outdated
Show resolved
Hide resolved
| public static InviteOrganizationUsersRequest Create(InviteScimOrganizationUserRequest request) => | ||
| new([OrganizationUserInvite.Create(request.Invite, request.ExternalId)], | ||
| request.Organization, | ||
| Guid.Empty, | ||
| request.PerformedAt); |
There was a problem hiding this comment.
I may not be understanding why these static methods are better than constructors
| public static InviteOrganizationUsersRequest Create(InviteScimOrganizationUserRequest request) => | |
| new([OrganizationUserInvite.Create(request.Invite, request.ExternalId)], | |
| request.Organization, | |
| Guid.Empty, | |
| request.PerformedAt); | |
| public InviteOrganizationUsersRequest(InviteScimOrganizationUserRequest request) | |
| : this( | |
| Invites: [OrganizationUserInvite.Create(request.Invite, request.ExternalId)], | |
| Organization: request.Organization, | |
| PerformedBy: Guid.Empty, | |
| PerformedAt: request.PerformedAt) | |
| { | |
| } |
There was a problem hiding this comment.
I prefer the way the Create static method reads in code. Especially when working with Func. It also allows you to put some validation logic into the method, which I prefer over putting in the constructor.
I can switch over to constructors since that seems to be others' preference.
There was a problem hiding this comment.
That is fair, I can see why you don't want logic inside the constructor. But it's not very intuitive that to create an object you have to use the Create method over the constructor.
…se model. Moved create methods to constructors. A few more CR changes included.
# Conflicts: # src/Core/AdminConsole/Services/Implementations/OrganizationService.cs # test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationUserRepositoryTests.cs
# Conflicts: # src/Core/Models/Commands/CommandResult.cs
# Conflicts: # src/Core/Models/Commands/CommandResult.cs
… overload for lighter weight model and moved common code to private method.
|
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers; | ||
|
|
||
| public interface ISendOrganizationInvitesCommand |
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers; | ||
|
|
||
| public interface IInviteOrganizationUsersCommand |
| return new Success<IEnumerable<OrganizationUser>>(organizationUserCollection.Select(x => x.OrganizationUser)); | ||
| } | ||
|
|
||
| private async Task RevertPasswordManagerChangesAsync(Valid<InviteUserOrganizationValidationRequest> valid, Organization organization) |
There was a problem hiding this comment.
Keeping consistency on the name helps readability. Other methods also
| private async Task RevertPasswordManagerChangesAsync(Valid<InviteUserOrganizationValidationRequest> valid, Organization organization) | |
| private async Task RevertPasswordManagerChangesAsync(Valid<InviteUserOrganizationValidationRequest> validatedRequest, Organization organization) |
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers.Models; | ||
|
|
||
| public class CreateOrganizationUser |
There was a problem hiding this comment.
Add some xmldoc here or maybe rename it to something that would indicate what this is used for.
🎟️ Tracking
PM-16811
📔 Objective
The goal of this PR is to improve the Invite Organization User flow from the SCIM POST endpoint. It will be later used to optimize the InviteOrganizationUser flow.
This was an attempt to surface all validation of inviting a user to the front of the call and to reduce duplicate database calls and validations.
This adds two a few new wrapper classes to allow for better flow control for when a request fails. By adding
ValidationResultand a typedCommandResult, we can "fail" a request for different reasons and allow the caller to decide what to do with the failure.Please refer to the diagram below of the new call stack.
📸 Diagram
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes