[PM-33951] automatically confirm pending users on admin login#7527
[PM-33951] automatically confirm pending users on admin login#7527JaredScar wants to merge 73 commits into
Conversation
- Introduced IBulkAutomaticallyConfirmOrganizationUsersCommand and its implementation. - Added new endpoints in OrganizationUsersController for bulk auto-confirm and fetching pending auto-confirm users. - Updated IOrganizationUserRepository to include a method for retrieving pending auto-confirm users. - Enhanced feature flags with BulkAutoConfirmOnLogin. - Added response models for pending auto-confirm users. - Implemented tests for the bulk auto-confirm command to ensure correct behavior.
…-33919-automatically-confirm-pending-users-on-admin-login
Overall Assessment: APPROVEAdds bulk auto-confirmation of pending organization users on admin login, gated behind the This PR has been through extensive review iterations with Admin Console, Auth, and DBA reviewers; all 100 prior review threads are resolved, and previously raised substantive concerns (SP signature parity, dual-ORM divergence on Code Review DetailsNo new blocking or important findings beyond what prior reviewers have already addressed and resolved. Observations worth tracking (not blocking):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7527 +/- ##
===========================================
- Coverage 64.86% 14.76% -50.10%
===========================================
Files 2140 1325 -815
Lines 94629 57575 -37054
Branches 8445 4512 -3933
===========================================
- Hits 61378 8500 -52878
- Misses 31155 48930 +17775
+ Partials 2096 145 -1951 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (28)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
BTreston
left a comment
BTreston
left a comment
There was a problem hiding this comment.
I've added a server dev for a second set of eyes, but here are some changes in the meantime.
- Updated OrganizationUsersController to utilize feature flags for bulk auto-confirm endpoints. - Modified IOrganizationUserRepository to retrieve organization users by status. - Enhanced stored procedure to accept a status parameter for filtering users. - Adjusted response handling in bulk auto-confirm method to return appropriate results. This refactor improves the clarity and functionality of the bulk auto-confirm feature, ensuring it adheres to the new status-based retrieval logic.
…ers-on-admin-login
…rsController - Added IPolicyQuery dependency to OrganizationUsersController. - Enhanced GetPendingAutoConfirmUsersAsync method to check organization ability and policy for automatic user confirmation. - Improved response handling to return an empty list if automatic confirmation is not enabled. This update ensures that pending users are only confirmed automatically when the organization settings allow it.
- Introduced BulkAutomaticallyConfirmOrganizationUsersCommand to handle batch confirmations of organization users. - Added BulkAutomaticallyConfirmOrganizationUsersValidator for validating multiple confirmation requests efficiently. - Enhanced IOrganizationUserRepository with ConfirmManyOrganizationUsersAsync method for bulk confirmation in a single database operation. - Created stored procedure OrganizationUser_ConfirmByIds to process bulk confirmations in SQL. - Added user-defined table type OrganizationUserToConfirmArray for passing multiple user confirmations to the stored procedure. This implementation improves the efficiency and scalability of user confirmation processes within organizations.
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
A few changes requested.
…-33919-automatically-confirm-pending-users-on-admin-login
- Removed the user-defined table type `OrganizationUserToConfirmArray` and replaced it with a JSON parameter in the stored procedure `OrganizationUser_UpdateManyConfirmByIds`. - Updated the `OrganizationUserRepository` to utilize the new JSON approach for confirming multiple users. - Renamed the stored procedure `OrganizationUser_ReadByOrganizationId` to `OrganizationUser_ReadManyByOrganizationIdStatus` and adjusted its parameters to include only `@OrganizationId` and `@Status`. - Created a new stored procedure `OrganizationUser_UpdateManyConfirmByIds` to handle bulk confirmations using JSON input. These changes streamline the confirmation process and improve database interaction efficiency.
…-33919-automatically-confirm-pending-users-on-admin-login
…-33919-automatically-confirm-pending-users-on-admin-login
…ModelBinder for automatic organization parameter binding
|
…ion methods for organization users - Implemented ConfirmManyOrganizationUsersAsync to confirm multiple users in a single operation. - Added GetManyPendingAutoConfirmAsync to retrieve users pending automatic confirmation. - Created stored procedures for bulk confirmation and fetching pending users. - Updated relevant repository interfaces and implementations across Dapper and Entity Framework.
…ationUsersAsync to IReadOnlyCollection - Updated the ConfirmManyOrganizationUsersAsync method signature in the IOrganizationUserRepository and its implementations to use IReadOnlyCollection instead of IEnumerable for better performance and clarity. - Adjusted related repository methods in both Dapper and Entity Framework implementations to reflect this change. - Added unit tests to ensure the new implementation behaves as expected, including scenarios for mixed batches and idempotency.
…tionModelBinder for organization parameter binding with unit tests
…zation for organization parameter
…ation parameter binding This commit introduces the BindOrganizationAttribute and its associated OrganizationModelBinder to facilitate the binding of Organization parameters in controller actions. The binder retrieves the organization from the database using route parameters and handles exceptions for missing or invalid IDs. Additionally, tests for the new attribute and binder have been added to ensure correct functionality and error handling.
… binding This commit updates the GetResetPasswordDetails method in the OrganizationUsersController to utilize the organization object passed as a parameter, ensuring correct validation of the organization user against the provided organization. This change enhances the method's reliability and aligns it with the recent introduction of the BindOrganizationAttribute for parameter binding.
…Details method to use bound organization
…s part of database cleanup.
…dingAutoConfirm methods - Introduced ConfirmManyOrganizationUsersTests to validate the confirmation of multiple organization users, ensuring only accepted users are confirmed and idempotency is maintained. - Added GetManyPendingAutoConfirmTests to verify retrieval of pending auto-confirm users, ensuring only eligible users are returned based on specific criteria. - Removed duplicate test implementations from OrganizationUserRepositoryTests to maintain clarity and organization in the test suite.
…n GetResetPasswordDetails method - Updated test cases to pass the organization directly instead of relying on repository calls. - Ensured that the tests correctly assert NotFoundException when the organization user does not match the bound organization. - Improved clarity in test setup by explicitly binding the organization to the method calls.
…Collection This commit updates the ConfirmManyOrganizationUsersAsync method in the IOrganizationUserRepository and its implementations to accept an IReadOnlyCollection instead of an IEnumerable for the usersToConfirm parameter. This change enhances type safety and performance by ensuring that the collection is not modified during processing. Additionally, new integration tests have been added to verify the functionality of confirming multiple organization users, ensuring only accepted users are processed and confirming idempotency in subsequent calls.
…ers-on-admin-login
| WHERE | ||
| [OrganizationId] = @OrganizationId | ||
| AND [Status] = 1 -- Accepted | ||
| AND [Type] = 0 -- User |
There was a problem hiding this comment.
CRITICAL — Dual-ORM divergence: filter selects Owners instead of Users
OrganizationUserType.User = 2, not 0. Type = 0 is Owner. The comment says -- User but the predicate filters for Owner.
The EF implementation in src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs:1128 correctly filters ou.Type == OrganizationUserType.User (=2), so this stored procedure diverges from the EF implementation. The MSSQL backend will return organization owners while the EF-backed databases return regular users.
Impact:
- The new
GET pending-auto-confirmendpoint returns owners on MSSQL but pending User-type members on Postgres/MySQL/SQLite. - The new integration test
GetManyPendingAutoConfirmAsync_ReturnsOnlyAcceptedUsersWithUserTypewill fail on MSSQL:CreateAcceptedTestOrganizationUserAsynccreates aType = Owneruser that the test expects to be excluded, but the buggy SP will return it. Meanwhile the eligibleType = Useruser will be excluded.
The same fix is needed in util/Migrator/DbScripts/2026-05-14_01_UpdateOrganizationUser_ReadByOrganizationId_AddStatusParam.sql:14.
| AND [Type] = 0 -- User | |
| AND [Type] = 2 -- User |
| WHERE | ||
| [OrganizationId] = @OrganizationId | ||
| AND [Status] = 1 -- Accepted | ||
| AND [Type] = 0 -- User |
There was a problem hiding this comment.
CRITICAL — Dual-ORM divergence: filter selects Owners instead of Users
Same bug as in src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByPendingAutoConfirm.sql:14. Type = 0 is Owner; User = 2. The migration must apply the corrected predicate so existing MSSQL installations match the EF behaviour and the source-of-truth stored procedure.
| AND [Type] = 0 -- User | |
| AND [Type] = 2 -- User |
Side note: the migration filename (UpdateOrganizationUser_ReadByOrganizationId_AddStatusParam.sql) describes a different change than the actual content (creates OrganizationUser_ReadByPendingAutoConfirm). Consider renaming for accurate change history, e.g. 2026-05-14_01_AddOrganizationUser_ReadByPendingAutoConfirm.sql.
…e related repository method - Added OrganizationUser_UpdateStatusKey stored procedure to handle updating the status and key of organization users based on a JSON input. - Updated OrganizationUserRepository to call the new stored procedure instead of the previous confirmation procedure. - Modified OrganizationUser_ReadByPendingAutoConfirm stored procedure to filter users by a new type value. - Enhanced integration tests to verify the correct behavior of the updated confirmation logic, ensuring revision dates are accurately tracked.
…ithub.com/bitwarden/server into ac/pm-33919-automatically-confirm-pending-users-on-admin-login
…/server into ac/pm-33919-automatically-confirm-pending-users-on-admin-login
…anyStatusKey - Renamed the stored procedure to OrganizationUser_UpdateManyStatusKey to better reflect its functionality of updating multiple organization users' statuses. - Updated the OrganizationUserRepository to call the new stored procedure. - Adjusted the migration script to create or alter the procedure accordingly.
…/server into ac/pm-33919-automatically-confirm-pending-users-on-admin-login
6 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33951
📔 Objective
When an admin logins to the browser extension for an organization, we want users who accepted an invite to be confirmed as users automatically. When the extension is locked, unlocking should also proceed to accept the users pending confirmation.