Skip to content

[PM-39455] Organization user v2 upgrade token repository tests - #8168

Merged
mzieniukbw merged 4 commits into
mainfrom
km/pm-39455-organization-user-v2-upgrade-token-repository-tests
Aug 24, 2026
Merged

mzieniukbw merged 4 commits into
mainfrom
km/pm-39455-organization-user-v2-upgrade-token-repository-tests

Conversation

@mzieniukbw

@mzieniukbw mzieniukbw commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Adds database integration tests for organization user v2 upgrade token added in #8158, that would otherwise be not possible, without #8165 first.

This will be used as a baseline for regression testing of key rotation, which includes both changes from both PRs.

Also adds emergency access key rotation integration tests.

📸 Screenshots

@mzieniukbw mzieniukbw added the t:tech-debt Change Type - Tech debt label Aug 7, 2026
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.71%. Comparing base (6f0ff3d) to head (45a6aa2).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8168      +/-   ##
==========================================
+ Coverage   63.29%   68.71%   +5.41%     
==========================================
  Files        2383     2383              
  Lines      103921   103921              
  Branches     9403     9403              
==========================================
+ Hits        65779    71411    +5632     
+ Misses      35898    30168    -5730     
- Partials     2244     2342      +98     

☔ View full report in Codecov by Harness.
📢 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.

Base automatically changed from km/pm-39455-set-upgrade-tokens-for-organizations to main August 14, 2026 15:32
Cover OrganizationUserRepository.UpdateForKeyRotation across all four providers: it
persists the reset password key and the V2 upgrade token together, clears a token left
over from an earlier upgrade, and leaves another member's membership untouched.

These need both the V2UpgradeToken column and the shared rotation transaction, so they
are kept out of the branches that add each.
@mzieniukbw
mzieniukbw force-pushed the km/pm-39455-organization-user-v2-upgrade-token-repository-tests branch from 0967486 to 45a6aa2 Compare August 18, 2026 16:42
@mzieniukbw
mzieniukbw marked this pull request as ready for review August 18, 2026 16:51
@mzieniukbw
mzieniukbw requested review from a team as code owners August 18, 2026 16:51
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This is a test-only change adding database integration tests for OrganizationUserRepository.UpdateForKeyRotation (V2 upgrade token persistence, stale-token clearing, and the UserId ownership filter) and EmergencyAccessRepository.UpdateForKeyRotation (key persistence, non-key field immutability, and omitted-grant isolation). It also refactors UserRepositoryTests.RunUpdateUserDataAsync to delegate to the existing DatabaseTransactionActionTestHelper, removing the hand-rolled SqlConnection/transaction block.

I traced each assertion against both the Dapper (OrganizationUser_UpdateDataForKeyRotation stored procedure, SqlBulkCopy temp-table path for emergency access) and EF Core implementations and the expectations hold for all four supported providers. The ownership-filter test is a genuinely valuable regression guard: it verifies a caller cannot write a V2 upgrade token onto a membership they do not own. No security, correctness, or breaking-change concerns found.

Code Review Details

No findings.

Notes considered and intentionally not raised as findings:

  • The refactored RunUpdateUserDataAsync now supplies a real connection and transaction on EF providers where it previously passed null. This matches how UpdateUserDataAsync and SetV2AccountCryptographicStateAsync invoke UpdateUserData in production, so it is a coverage improvement rather than a regression.
  • The private CreateUserAsync helper duplicates the CreateTestUserAsync extension in AdminConsole/OrganizationTestHelpers.cs, but file-local user factories are an established pattern in this project (e.g. Auth/Repositories/DeviceRepositoryTests.cs).
  • _camelCase constant naming matches the private_fields rule in .editorconfig.

@mzieniukbw
mzieniukbw enabled auto-merge (squash) August 18, 2026 16:59

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auth changes are only a test file - approving.

@mzieniukbw
mzieniukbw merged commit aebb9c4 into main Aug 24, 2026
55 of 56 checks passed
@mzieniukbw
mzieniukbw deleted the km/pm-39455-organization-user-v2-upgrade-token-repository-tests branch August 24, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants