Auth/PM-37166 - Devices - add client version#7632
Conversation
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.
JaredSnider-Bitwarden
added
the
ai-review-vnext
Bitwarden Claude Code ReviewOverall Assessment: APPROVE Adds a Code Review DetailsNo findings. Defense-in-depth design (cache → SP/EF composite WHERE guards → CASE expressions) keeps redundant writes from causing data corruption even if the cache layer fails. Per-column semantics are documented consistently across the entity XML doc, the Previously raised reviewer feedback on the column width bound (resolved in 81d9584), |
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7632 +/- ##
==========================================
+ Coverage 59.85% 64.25% +4.40%
==========================================
Files 2121 2121
Lines 93401 93441 +40
Branches 8285 8290 +5
==========================================
+ Hits 55904 60042 +4138
+ Misses 35517 31331 -4186
- Partials 1980 2068 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR is blocked an issue introduced in #7489 which #7633 will fix. The CI DB failures are due to (claude summary):
UPDATE: the PR is unblocked. |
…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.
Aligns file encoding with the repo's .editorconfig (charset = utf-8-bom for .cs) so dotnet format --verify-no-changes passes.
…ation 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.
|
|
||
| public class DeviceLastActivityCacheService : IDeviceLastActivityCacheService | ||
| { | ||
| // TTL is 48h rather than 24h to ensure the entry outlives the full following calendar day |
There was a problem hiding this comment.
The OG 48 hr timeframe was a mistake - 24 hrs is enough for our purposes.
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.
JaredSnider-Bitwarden
requested review from
Patrick-Pimentel-Bitwarden,
djsmith85 and
rr-bw
…ateDeviceLastActivityCommand 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.
…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.
…onAndUpdateLastActivitySp.sql - fix wrong comment
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| [LastActivityDate] = | ||
| CASE | ||
| WHEN [LastActivityDate] IS NULL OR CAST([LastActivityDate] AS DATE) < CAST(GETUTCDATE() AS DATE) | ||
| THEN GETUTCDATE() |
There was a problem hiding this comment.
Per discussion with @mkincaid-bw , using GETUTCDATE() in stored procs is non-standard at BW so we should just pass in @LastActivityDate DATETIME2(7), and replace GETUTCDATE() with that variable.
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
…s 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.
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.
|
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Just one question about Device_ReadActiveWithPendingAuthRequestsByUserId. If the omission is intentional, then it LGTM.
| GO | ||
|
|
||
| -- 2. DeviceView mirrors all columns — refresh so it picks up ClientVersion. | ||
| CREATE OR ALTER VIEW [dbo].[DeviceView] |
There was a problem hiding this comment.
This is fine but you could also just use EXECUTE sp_refreshview N'[dbo].[DeviceView]'
There was a problem hiding this comment.
Oh, ok, will do that next time! Thank you!
| EXECUTE sp_refreshsqlmodule N'[dbo].[Device_ReadByIdentifier]' | ||
| END | ||
| GO | ||
|
|
There was a problem hiding this comment.
❓ I noticed that you did not refresh Device_ReadActiveWithPendingAuthRequestsByUserId, which is fine since that proc explicitly references columns from the Device table (rather than SELECT *). However, it also includes LastActivityDate, so I wasn't sure if it should also return ClientVersion?
There was a problem hiding this comment.
Great question! It is intentional. We don't plan on exposing client version for display on clients today.
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👏 Round of applause on this work and turnaround time. Thank you.
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-37166
📔 Objective
To add a client version to the devices table which can be bumped on login and token refresh.
To avoid hitting the DB n times where n is the # of device columns we want to bump, I chose to refactor the
Device.LastActivityDateimplementation (#7302 ) to be more generic and only hit the DB on either last activity date bump (only once per day) or client version change - the bump is still protected with a Cosmos backed cache to prevent too much load on the DB.📸 Screenshots
PM-37166.-.Device.-.add.client.version.to.last.activity.update.works.mov