Auth/PM-37165 - Add Last API Key Rotated Date to User#7634
Merged
JaredSnider-Bitwarden merged 6 commits intoMay 14, 2026
Conversation
Adds a nullable DATETIME2(7) LastApiKeyRotationDate column on the User table alongside the other Last*Date audit columns. Covers the MSSQL table, view, User_Create / User_Update stored procedures (new optional parameter, EDD-safe with default NULL), the SSDT source-of-truth, and EF migrations for MySql, Postgres, and Sqlite. Repository round-trip integration tests verify that CreateAsync defaults the column to NULL and ReplaceAsync persists it across all four providers.
Extracts user API key rotation out of UserService into a new CQS command at src/Core/Auth/UserFeatures/UserApiKey/, mirroring the existing decomposition pattern for other Auth user features. The command generates a new 30-char ApiKey, bumps RevisionDate, sets LastApiKeyRotationDate, and persists via IUserRepository.ReplaceAsync. Adds the PM37165_RotateUserApiKeyCommand feature flag so the new path can be rolled out behind a flag in a follow-up commit. Registers the command via AddUserApiKeyCommands inside AddUserServices. Unit tests verify the command assigns a fresh key, updates both RevisionDate and LastApiKeyRotationDate to the same recent UTC value, and calls ReplaceAsync exactly once.
Wires AccountsController.RotateApiKey to dispatch between IRotateUserApiKeyCommand (flag on) and the legacy UserService.RotateApiKeyAsync (flag off) based on PM37165_RotateUserApiKeyCommand. Both paths preserve the existing auth and secret-verification guards, which run before the flag branch. Marks IUserService.RotateApiKeyAsync and its implementation [Obsolete] pointing callers at IRotateUserApiKeyCommand, with TODOs tying their removal to the flag cleanup. The body of the legacy method is deliberately unchanged so it does NOT write LastApiKeyRotationDate while the flag is off; that genuinely gates the new behavior so the ramp is observable and reversible. The single remaining call site (the controller fallback) is wrapped in #pragma warning disable CS0618 so the attribute continues to flag any new callers. Tests: - AccountsControllerTests: dispatch tests for both flag states; the auth and bad-secret guard tests are parameterized over flag state. Pre-existing typo in two tests that called _sut.ApiKey() instead of _sut.RotateApiKey() is fixed. - UserServiceTests: regression test locks in the legacy non-write behavior so it cannot drift before the flag is removed. - AccountsControllerTest (integration): three endpoint tests cover flag-off (LastApiKeyRotationDate stays NULL), flag-on (column is populated), and bad-secret over both flag states (no rotation occurs). Each flag-state-specific test carries a TODO breadcrumb describing the exact rename or deletion when the flag is cleaned up.
JaredSnider-Bitwarden
added
the
ai-review-vnext
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the addition of a Code Review DetailsNo issues to report. Notable strengths observed during review:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7634 +/- ##
==========================================
+ Coverage 59.83% 64.24% +4.41%
==========================================
Files 2120 2121 +1
Lines 93377 93401 +24
Branches 8284 8285 +1
==========================================
+ Hits 55868 60003 +4135
+ Misses 35527 31331 -4196
- Partials 1982 2067 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JaredSnider-Bitwarden
changed the title
JaredSnider-Bitwarden
requested review from
Patrick-Pimentel-Bitwarden,
enmande and
rr-bw
and removed request for
enmande
withinfocus
reviewed
May 14, 2026
| b.ToTable("OrganizationMemberBaseDetails"); | ||
| }); | ||
|
|
||
| modelBuilder.Entity("Bit.Infrastructure.EntityFramework.AdminConsole.Models.Collection", b => |
Contributor
There was a problem hiding this comment.
I had claude do a little writeup for why we are seeing this in this pr since it has nothing to do with the scope of your pr.
Why it shows up in this PR
DatabaseContextModelSnapshot.cs is a derived artifact — EF only regenerates it when you run dotnet ef migrations add .... PR #7523 changed the namespaces but did not add a new migration, so the snapshot file was left stale (still listing the entities under their old Models.* namespace at their old alphabetical
position).This PR is the first migration generated after that namespace refactor landed. When Jared ran dotnet ef migrations add AddLastApiKeyRotationDateToUserTable, EF re-emitted the entire snapshot from the live model, which:
- Wrote the three Collection entities under their new AdminConsole.Models.* names → at lines ~71-82 (earlier alphabetical position).
- Removed the old blocks from their previous spot lower in the file (see the matching deletions at the old Models.Collection location: - lines 1272-1311 in the diff).
Contributor
Author
Contributor
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
Ready to approve once the new field is being added to the back of the table 👍
Append the new column to the end of User.sql, UserView.sql, the matching CREATE OR ALTER VIEW in the migrator script, and the User entity so SSDT mirrors what ALTER TABLE ADD produces in production.
|
Patrick-Pimentel-Bitwarden
approved these changes
May 14, 2026
Contributor
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👍 Looks solid
JaredSnider-Bitwarden
deleted the
auth/pm-37165/user-last-api-key-rotated-date
branch
JaredSnider-Bitwarden
added a commit
that referenced
this pull request
May 14, 2026
…baseline PR #7634 merged AddLastApiKeyRotationDateToUserTable into main while this branch was open. The prior AddDeviceClientVersion migration's frozen model snapshot (its .Designer.cs) was generated before that PR landed, so it did not include User.LastApiKeyRotationDate. Applying migrations incrementally against that stale snapshot would produce an inconsistent model graph. Regenerated AddDeviceClientVersion on top of the merged-from-main baseline so the new .Designer.cs files include both columns. The migration body itself still only adds Device.ClientVersion; the top-level DatabaseContextModelSnapshot.cs files were already correct from git's three-way merge. New timestamps (20260514192xxx) come after the User migration (20260514011xxx), preserving migration order.
JaredSnider-Bitwarden
added a commit
that referenced
this pull request
May 15, 2026
* PM-37166 - Add ClientVersion to Device entity and repository contract * PM-37166 - Add ClientVersion SQL schema and refactor bump stored procedures * PM-37166 - Implement combined bump in repositories and add EF migrations EF snapshot regeneration also absorbs Collection / CollectionGroup / CollectionUser namespace moves (Bit.Infrastructure.EntityFramework.Models -> Bit.Infrastructure.EntityFramework.AdminConsole.Models) that were left un-regenerated by PR #7523 (PM-35489). Namespace-only, no SQL impact; flagged with the AC team for awareness. * PM-37166 - Replace DeviceLastActivityCacheService with DeviceDataCacheService * PM-37166 - Replace BumpDeviceLastActivityDateCommand with BumpDeviceDataCommand * PM-37166 - Pass ClientVersion through identity request validators * PM-37166 - Align migration script with SQL style guide Refresh Device_ReadBy* sprocs after DeviceView change so their cached schema picks up ClientVersion, swap retired-sproc drops to DROP PROCEDURE IF EXISTS, and tighten the ALTER TABLE indent in step 1. * PM-37166 - Rename BumpData to UpdateLastActivity across device write pathway The "BumpData" naming was vague — "data" named a category, not the thing being written. Rename to "UpdateLastActivity" everywhere: SP, repositories, command, cache, validators, tests. "Last activity" names the event of the device's most recent appearance; LastActivityDate (when) and ClientVersion (what was running) are facts we observed about that event. ClientVersion is treated as a property of the activity event rather than an independent value, so future last-observed properties (last IP, OS, etc.) slot in without renaming. The SQL layer uses Update* per architect guidance on #7302; the Bump* SPs in this codebase are legacy and not being extended. The extensibility note lives on IUpdateDeviceLastActivityCommand with short pointers from the SP, repo, and cache. Cache key prefix changes from device:data: to device:last-activity: — safe because the cache is only a write-suppression optimization (SP guards ensure correctness) and entries TTL out within 24h. Migration renamed to 2026-05-14 to reflect the rewrite. * PM-37166 - Add UTF-8 BOM to device last activity cache files Aligns file encoding with the repo's .editorconfig (charset = utf-8-bom for .cs) so dotnet format --verify-no-changes passes. * PM-37166 - Compare LastActivityDate at second precision in device creation test GetManyByUserIdWithDeviceAuth_ReturnsLastActivityDate_ForNewDeviceAsync was flaking on SqlServer: Dapper binds DateTime params as legacy `datetime` (~3.33ms granularity), so the entity initializer's UtcNow can be rounded a few ms earlier than the in-memory `beforeCreation` capture, making a strict >= comparison occasionally false. Truncate both sides to the second to absorb that drift while still rejecting stale or defaulted values. * PM-37166 - Rename Device.ClientVersion EF migration Renames migration class/files from AddDeviceClientVersionRefactorDeviceDataBump to AddDeviceClientVersion to drop stale "Bump" terminology and the misleading "RefactorDeviceData" prefix. The EF migration only adds the ClientVersion column; the BumpData -> UpdateLastActivity SP refactor lives in MSSQL .sql files and has no EF representation. * PM-37166 - Document null-is-no-op semantics for ClientVersion on IUpdateDeviceLastActivityCommand Tighten the interface-level summary and add a <param> note clarifying that a null clientVersion is treated as "no opinion" and will not clear an existing stored value. * PM-37166 - Regenerate Device.ClientVersion EF migration on post-#7634 baseline PR #7634 merged AddLastApiKeyRotationDateToUserTable into main while this branch was open. The prior AddDeviceClientVersion migration's frozen model snapshot (its .Designer.cs) was generated before that PR landed, so it did not include User.LastApiKeyRotationDate. Applying migrations incrementally against that stale snapshot would produce an inconsistent model graph. Regenerated AddDeviceClientVersion on top of the merged-from-main baseline so the new .Designer.cs files include both columns. The migration body itself still only adds Device.ClientVersion; the top-level DatabaseContextModelSnapshot.cs files were already correct from git's three-way merge. New timestamps (20260514192xxx) come after the User migration (20260514011xxx), preserving migration order. * PM-37166 - util/Migrator/DbScripts/2026-05-14_00_AddDeviceClientVersionAndUpdateLastActivitySp.sql - fix wrong comment * PM-37166 - Bump Device.ClientVersion column width from 20 to 43 43 is the upper bound of Version.ToString() for any input parseable by Version.TryParse — four Int32 components (Int32.MaxValue = 10 digits) joined by 3 dots. Sizing to the type's mathematical max prevents SQL Server error 8152 on malformed/hostile Bitwarden-Client-Version headers without paying the cost of normalization at the call sites. Real Bitwarden CalVer (YYYY.M.B) remains well within bounds at ~9 chars. - Device.cs [MaxLength] + entity doc comment - SSDT table + 4 stored procedures - Cloud migration ALTER TABLE + SP parameters - EF migrations regenerated for MySQL / Postgres / SQLite * PM-37166 - Defer dropping old single-column UpdateLastActivityDate SPs to follow-up Server and DB deploys are decoupled, so dropping the old SPs in the same migration that introduces the new combined ones would break server rollback. Per discussion on PR #7632: - Remove DROP PROCEDURE statements from the migration; replace with a note explaining the deferral. - Restore the old Device_UpdateLastActivityDate{ById,ByIdentifierUserId}.sql files in src/Sql/dbo so the SSDT source-of-truth stays aligned with deployed schema (EDD). A follow-up ticket will drop the old SPs and delete the .sql files together once we're confident no deployed server version still calls them. * PM-37166 - Pass @LastActivityDate into Device_UpdateLastActivity SPs Bitwarden convention is to compute timestamps in the application layer and pass them as DATETIME2(7) params, not call GETUTCDATE() inside SPs. Dapper repo now computes DateTime.UtcNow locally (matching the EF repo and UserRepository.cs precedent) and passes LastActivityDate through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-37165
📔 Objective
We want to add a new column on the user table which tracks last API key rotated dates.
📸 Screenshots
PM-37165.-.User.API.Key.Rotation.-.last.rotation.date.updates.on.rotation.with.flag.on.mov