[PM-26636] - Auto Confirm Org User Command #6488

jrmccannon · 2025-10-23T19:28:56Z

🎟️ Tracking

PM-26636

📔 Objective

TODO

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

github-actions · 2025-10-23T19:42:12Z

Checkmarx One – Scan Summary & Details – c84838ae-1410-44f3-b616-ed8d0c0bd9f4

Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~CSRF~~	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 173
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 72

r-tome · 2025-10-24T13:04:09Z

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs

+
+        if (userId is null || userId.Value == Guid.Empty)
+        {
+            throw new UnauthorizedAccessException();


Could possibly be an attribute? Just something to think about

r-tome · 2025-10-24T13:05:57Z

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs


+
+    [Authorize<ManageUsersRequirement>]
+    [HttpPost("{id:guid}/auto-confirm")]


I remember a discussion about this https://bitwarden.slack.com/archives/C02LPDKPML6/p1641389870010400?thread_ts=1641386236.008400&cid=C02LPDKPML6

r-tome · 2025-10-24T13:12:25Z

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs

+            switch (result.AsError)
+            {
+                case OrganizationNotFound organizationNotFound:
+                    throw new NotFoundException(organizationNotFound.Message);


How does the return message look like?

server/src/Api/Utilities/ExceptionHandlerFilterAttribute.cs

Line 121 in ff4b3eb

errorMessage = "Resource not found.";

Should we throw an exception on the controller or return NotFound()

Ah. I see now. Well that's not great.

Should we allow for the overriding of that not found message? Or is that something that needs to be run by architecture?

I think its wise to run by architecture

...izationFeatures/OrganizationUsers/Interfaces/IAutomaticallyConfirmOrganizationUserCommand.cs

…ests.

…m-user-command

codecov · 2025-10-28T19:59:52Z

Codecov Report

❌ Patch coverage is 0% with 249 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.24%. Comparing base (5f0e038) to head (031c080).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...ser/AutomaticallyConfirmOrganizationUserCommand.cs	0.00%	124 Missing ⚠️
.../AutomaticallyConfirmOrganizationUsersValidator.cs	0.00%	68 Missing ⚠️
...ser/AutomaticallyConfirmOrganizationUserRequest.cs	0.00%	15 Missing ⚠️
...AdminConsole/Utilities/v2/Results/CommandResult.cs	0.00%	13 Missing ⚠️
...atures/OrganizationUsers/AutoConfirmUser/Errors.cs	0.00%	12 Missing ⚠️
...equirements/SingleOrganizationPolicyRequirement.cs	0.00%	5 Missing ⚠️
...onsole/Utilities/v2/Validation/ValidationResult.cs	0.00%	5 Missing ⚠️
src/Core/AdminConsole/Utilities/v2/Errors.cs	0.00%	4 Missing ⚠️
...eatures/OrganizationServiceCollectionExtensions.cs	0.00%	2 Missing ⚠️
...ures/Policies/PolicyServiceCollectionExtensions.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6488       +/-   ##
===========================================
- Coverage   51.86%   13.24%   -38.62%     
===========================================
  Files        1901     1141      -760     
  Lines       84051    49886    -34165     
  Branches     7501     3888     -3613     
===========================================
- Hits        43594     6608    -36986     
- Misses      38763    43158     +4395     
+ Partials     1694      120     -1574

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

r-tome · 2025-10-31T12:26:16Z

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs

+        [FromRoute] Guid id,
+        [FromBody] OrganizationUserConfirmRequestModel model)
+    {
+        if (!_featureService.IsEnabled(FeatureFlagKeys.AutomaticConfirmUsers))


This should be replaced with [RequireFeature(FeatureFlagKeys.AutomaticConfirmUsers)]

r-tome · 2025-10-31T12:34:59Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+
+        var successfulConfirmation = await organizationUserRepository.ConfirmOrganizationUserAsync(validatedRequest.OrganizationUser);
+
+        if (!successfulConfirmation) return new None();


I would add a comment here to explain that the only reason this would not be successful is if the user is already confirmed. Otherwise it seems like silent failure.

I think it's also worth explaining here that this is idempotent and designed to handle multiple simultaneous requests from logged in clients.

r-tome · 2025-10-31T12:35:41Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+    {
+        try
+        {
+            if (!featureService.IsEnabled(FeatureFlagKeys.CreateDefaultLocation)


Extract all these conditions into a private method

r-tome · 2025-10-31T12:37:49Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserRequest.cs

Filename is misspelled

r-tome · 2025-10-31T12:41:00Z

src/Api/AdminConsole/Controllers/OrganizationUsersController.cs

+
+        if (result is { IsError: true })
+        {
+            return result.AsError switch


The Billing team has a helper method in BaseBillingController.Handle, we should do something similar

r-tome · 2025-10-31T12:44:18Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

We should add unit tests for this command

…ctory.

eliykat

Nice work!

eliykat · 2025-10-31T05:42:29Z

...izationFeatures/OrganizationUsers/Interfaces/IAutomaticallyConfirmOrganizationUserCommand.cs

+/// <summary>
+/// Command to automatically confirm an organization user.
+/// </summary>


More context around what autoconfirm is would be helpful - <remarks> is good for this. e.g.

The autoconfirm feature sends push notifications to eligible logged in client apps to automatically confirm OrganizationUsers without manual intervention by administrators. When a client app receives the push notification, it performs the key exchange and sends an autoconfirm request to the server. This command is used to process those requests. It should not be used to confirm users in any other context.

I think a README in the AutoConfirmUser folder would also be useful to document the feature in more detail. I'll raise it in the dev channel.

eliykat · 2025-10-31T05:49:18Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+    {
+        var requestData = await RetrieveDataAsync(request);
+
+        if (requestData.IsError) return requestData.AsError;


Here, I got you these: { and }

(Please avoid single line if statements.)

eliykat · 2025-10-31T06:29:17Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+            await collectionRepository.CreateAsync(
+                new Collection
+                {
+                    OrganizationId = request.Organization.Id,
+                    Name = request.DefaultUserCollectionName,
+                    Type = CollectionType.DefaultUserCollection
+                },
+                groups: null,
+                [new CollectionAccessSelection
+                {
+                    Id = request.OrganizationUser.Id,
+                    Manage = true
+                }]);


💭 (non-blocking) Thoughts on making this an idempotent repository method as well? e.g. CreateDefaultCollectionAsync. Given that we are creating it in multiple places and creating default collections properly is quite important. (not this PR though)

eliykat · 2025-10-31T06:32:48Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+
+        if (organization.IsError) return organization.AsError;
+
+        return new AutomaticallyConfirmOrganizationUserRequestData


I suggest that it's clearer to call this the ValidationRequest; it's an enriched DTO required by the validator. The overall flow of data then looks like: API request -> Command Request -> Validation Request -> Validation Result -> Command Result -> API response... which is nice and symmetrical. The Data suffix is not very clear by comparison. (it's not a data model)

eliykat · 2025-11-01T00:46:02Z

...ionFeatures/OrganizationUsers/AutoConfirmUser/AutomaticallyConfirmOrganizationUserCommand.cs

+
+        var successfulConfirmation = await organizationUserRepository.ConfirmOrganizationUserAsync(validatedRequest.OrganizationUser);
+
+        if (!successfulConfirmation) return new None();


I think it's also worth explaining here that this is idempotent and designed to handle multiple simultaneous requests from logged in clients.

eliykat · 2025-11-01T02:56:41Z

src/Core/AdminConsole/Utilities/v2/Results/CommandResult.cs

👏 thank you for moving this. I should've used the v2 convention to begin with.

eliykat · 2025-11-01T03:00:18Z

src/Core/AdminConsole/Utilities/v2/Results/CommandResult.cs

+        var input = await inputTask;
+        if (input.IsError) return input.AsError;
+        return await next(input.AsSuccess);


Could this use Match to take advantage of the OneOf library utils? (same for the other util methods)

eliykat · 2025-11-01T03:02:45Z

test/Api.IntegrationTest/Helpers/OrganizationTestHelpers.cs

+    /// <summary>
+    /// Enables the Organization Auto Confirm policy for the specified organization.
+    /// </summary>
+    public static async Task EnableOrganizationAutoConfirmPolicyAsync<T>(


I think a generic EnablePolicyAsync(PolicyType policyType) method would be helpful, otherwise we're going to end up with a helper method for every policy.

eliykat · 2025-11-01T03:05:13Z

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

Controller tests are going to get very long; thoughts on making separate files per set of functionality? e.g. autoconfirm endpoints would have their own test file. I did this with unit tests for OrganizationUserControllerPutTests.

eliykat · 2025-11-01T03:07:25Z

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

+    }
+
+    [Fact]
+    public async Task AutoConfirm_WhenOwnerInvitesValidUser_ThenShouldReturnNoContent()


Test name: we're testing acceptance, not invitation.

This test should also check the db to verify the user has moved into the correct state (that is, they've actually been confirmed).

eliykat · 2025-11-01T03:09:29Z

Also... enjoy your merge errors 😎

jrmccannon added 2 commits October 23, 2025 14:27

Adding auto confirm endpoint and initial command work.

14f3e88

Adding validator

4645222

Finished command implementation.

6a29377

r-tome reviewed Oct 24, 2025

View reviewed changes

jrmccannon added 5 commits October 24, 2025 14:57

Enabled the feature renomved used method. Enabled the policy in the t…

89b8d59

…ests.

Added extension functions to allow for railroad programming.

f60f0f5

Removed guid from route template. Added xml docs

0ba770d

Merge branch 'refs/heads/main' into jmccannon/ac/pm-26636-auto-confir…

e5f07de

…m-user-command

Added validation for command.

5527ca6

jrmccannon added 2 commits October 28, 2025 15:12

Added default collection creation to command.

27ce4b8

formatting.

2069c9f

eliykat self-requested a review October 29, 2025 05:23

jrmccannon added 4 commits October 29, 2025 09:34

Added additional error types and mapped to appropriate results.

cb81d29

Merge branch 'main' into jmccannon/ac/pm-26636-auto-confirm-user-command

24a39d8

Added tests for auto confirm validator

a1b2bac

Adding tests

7307454

r-tome reviewed Oct 31, 2025

View reviewed changes

jrmccannon added 5 commits October 31, 2025 08:06

fixing file name

1303b22

Cleaned up OrgUserController. Added integration tests.

4bb8734

Consolidated CommandResult and validation result stuff into a v2 dire…

68afb17

…ctory.

changing result to match handle method.

aee2734

Moves validation thenasync method.

031c080

eliykat requested changes Nov 1, 2025

View reviewed changes



		[Authorize<ManageUsersRequirement>]
		[HttpPost("{id:guid}/auto-confirm")]


		var successfulConfirmation = await organizationUserRepository.ConfirmOrganizationUserAsync(validatedRequest.OrganizationUser);

		if (!successfulConfirmation) return new None();


		if (organization.IsError) return organization.AsError;

		return new AutomaticallyConfirmOrganizationUserRequestData

[PM-26636] - Auto Confirm Org User Command #6488

Are you sure you want to change the base?

[PM-26636] - Auto Confirm Org User Command #6488

Conversation

jrmccannon commented Oct 23, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliykat commented Nov 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jrmccannon commented Oct 23, 2025 •

edited by atlassian bot

Loading

github-actions bot commented Oct 23, 2025 •

edited

Loading

codecov bot commented Oct 28, 2025 •

edited

Loading