[PM-34601] Bump Group.RevisionDate on edits and access changes

[r-tome]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-34601

📔 Objective

Ensure Group.RevisionDate is updated whenever group membership or collection-group access is modified

[codecov]

Codecov Report

❌ Patch coverage is 95.37037% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.76%. Comparing base (53dc0c4) to head (0bef3ce).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
.../OrganizationFeatures/Groups/CreateGroupCommand.cs	71.42%	2 Missing ⚠️
.../OrganizationFeatures/Groups/UpdateGroupCommand.cs	71.42%	2 Missing ⚠️
...dminConsole/Public/Controllers/GroupsController.cs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7467      +/-   ##
==========================================
+ Coverage   59.25%   63.76%   +4.51%     
==========================================
  Files        2081     2081              
  Lines       92036    92113      +77     
  Branches     8179     8185       +6     
==========================================
+ Hits        54534    58736    +4202     
+ Misses      35563    31351    -4212     
- Partials     1939     2026      +87     

☔ 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.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR ensures Group.RevisionDate is bumped whenever group membership or collection-group access is modified, covering all relevant code paths: direct group member updates, user-group assignment changes, SCIM operations, organization imports, and collection-group access changes. The implementation maintains dual-ORM parity between Dapper/SQL stored procedures and Entity Framework, uses TimeProvider for testability, and includes comprehensive unit and integration tests. The SQL migration uses a backward-compatible @RevisionDate = NULL default parameter, ensuring safe rollout.

Code Review Details

No findings. The change is well-structured with consistent patterns across all layers (API controllers, business logic commands/services, repository interfaces, Dapper repositories, EF repositories, and SQL stored procedures). Test coverage is thorough, including both unit tests with FakeTimeProvider and integration tests that verify the revision date is actually persisted.

[github-actions]

Checkmarx One – Scan Summary & Details – 5dce4173-9e16-4bc6-9b10-70f5b6de12ba

---

New Issues (9)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 151	details Method at line 151 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from model. This parameter valu... Attack Vector
2		CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 127	details Method at line 127 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from model. This parameter valu... Attack Vector
3		CSRF	src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 392	details Method at line 392 of /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs gets a parameter from a user request from model. This par... Attack Vector
4		CSRF	src/Api/AdminConsole/Public/Controllers/MembersController.cs: 232	details Method at line 232 of /src/Api/AdminConsole/Public/Controllers/MembersController.cs gets a parameter from a user request from model. This parame... Attack Vector
5		CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 275	details Method at line 275 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from orgUserId. This parameter ... Attack Vector
6		CSRF	src/Api/AdminConsole/Public/Controllers/MembersController.cs: 269	details Method at line 269 of /src/Api/AdminConsole/Public/Controllers/MembersController.cs gets a parameter from a user request from model. This parame... Attack Vector
7		CSRF	src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 167	details Method at line 167 of /src/Api/AdminConsole/Public/Controllers/GroupsController.cs gets a parameter from a user request from model. This paramet... Attack Vector
8		CSRF	src/Api/Vault/Controllers/CiphersController.cs: 1558	details Method at line 1558 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
9		CSRF	src/Api/Vault/Controllers/CiphersController.cs: 1385	details Method at line 1385 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector

---

Fixed Issues (7)

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

Severity	Issue	Source File / Package
	CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 275
	CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 127
	CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 151
	CSRF	src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 392
	CSRF	src/Api/AdminConsole/Public/Controllers/MembersController.cs: 232
	CSRF	src/Api/AdminConsole/Public/Controllers/MembersController.cs: 269
	CSRF	src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 164

[shane-melton]

Vault change looks good!

[mkincaid-bw]

LGTM

[JaredScar]

@claude Can you please review this?

[claude]

Claude finished @JaredScar's task in 7m 46s —— View job

---

Claude Code Review

Overall Assessment: APPROVE

Reviewed a 39-file change that threads a revisionDate parameter through GroupUser_* and Collection_UpdateWith* stored procedures and their Dapper/EF counterparts, injects TimeProvider into the relevant commands, and adds integration tests that verify the new behavior. Dual-ORM parity, SQL backward-compat via @RevisionDate = NULL defaults, old-plus-new group union semantics, and call-site updates all check out. Public API (GroupsController.PutMemberIds) response shape is unchanged.

Code Review Details

What was checked

• Dual-ORM parity between Dapper sprocs and EF repositories for GroupUser_UpdateGroups, GroupUser_AddUsers, GroupUser_Delete, GroupUser_UpdateUsers, Collection_UpdateWithGroups, Collection_UpdateWithGroupsAndUsers, Collection_CreateOrUpdateAccessForMany — all agree on which groups get bumped (union of old + new for replace-style ops; just @Groups for add/update-only).
• @RevisionDate DATETIME2(7) = NULL default + IF @RevisionDate IS NOT NULL guards — backward compatible with existing callers.
• TimeProvider is injected into commands and the same group.RevisionDate is threaded through repository calls (not re-resolved at each layer). DI is satisfied transitively via AddDefaultServices -> AddPush -> TryAddSingleton(TimeProvider.System).
• Event logging in CreateGroupCommand/UpdateGroupCommand now uses group.RevisionDate as eventDate — deliberate alignment with the mutation timestamp.
• Public API response shape on GroupsController.PutMemberIds — unchanged.

Open question

❓ User account deletion (UserRepository.DeleteAsync / DeleteManyAsync) and organization-user deletion (OrganizationUser_DeleteById.sql, OrganizationUser_DeleteByIds.sql, and their EF equivalents at src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs:130 and :235) remove GroupUser rows directly without bumping Group.RevisionDate. These paths also modify group membership, so a client caching groups by revision date could miss membership drops caused by user/org-user removal. Was this intentionally scoped out of PM-34601, or should a follow-up address it?

• src/Infrastructure.EntityFramework/Repositories/UserRepository.cs:438
• src/Infrastructure.EntityFramework/AdminConsole/Repositories/OrganizationUserRepository.cs:130
• src/Sql/dbo/Stored Procedures/OrganizationUser_DeleteById.sql:38

• Gather PR context and changed files
• Review source code changes
• Review SQL migration and stored procedures
• Review tests
• Post final review
  • Branch: ac/pm-34601/update-groups-revision-date

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
59.8% Duplication on New Code

See analysis details on SonarQube Cloud