Skip to content

Auth/PM-37621 - Fix Device.LastActivityDate surfacing legacy NULL rows as DateTime.UtcNow#7649

Merged
JaredSnider-Bitwarden merged 2 commits into
mainfrom
auth/pm-37621/device-null-last-activity-date-defaulting-to-value
May 15, 2026
Merged

Auth/PM-37621 - Fix Device.LastActivityDate surfacing legacy NULL rows as DateTime.UtcNow#7649
JaredSnider-Bitwarden merged 2 commits into
mainfrom
auth/pm-37621/device-null-last-activity-date-defaulting-to-value

Conversation

@JaredSnider-Bitwarden
Copy link
Copy Markdown
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden commented May 15, 2026 •
edited
Loading

🎟️ Tracking

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

📔 Objective

To fix an issue with where null Device.LastActivityDate values were coming out of the server as new DateTime.UtcNow

📸 Screenshots

PM-37621.mov

@JaredSnider-Bitwarden
PM-37621 - Fix Device.LastActivityDate surfacing legacy NULL rows as …
5a7f309 
…DateTime.UtcNow

Dapper's deserializer skips the property setter when a nullable column is
DBNull, leaving the property at its CLR default. The field initializer
`public DateTime? LastActivityDate { get; internal set; } = DateTime.UtcNow`
poisoned that default, so rows whose LastActivityDate column was NULL (e.g.
devices created before the column existed) read back as the current time.

Drop the initializer, relax `internal set` to `set`, and stamp
LastActivityDate explicitly at the two creation call sites
(DeviceValidator.GetDeviceFromRequest and DeviceRequestModel.ToDevice). Adds
an integration regression test that creates a device with an explicit null
LastActivityDate and asserts the read path surfaces null. Augments
DeviceValidatorTests.GetDeviceFromRequest_RawDeviceInfoValid_ReturnsDevice
to lock in the creation-time stamp.
@JaredSnider-Bitwarden JaredSnider-Bitwarden marked this pull request as ready for review May 15, 2026 19:57
@JaredSnider-Bitwarden JaredSnider-Bitwarden requested a review from a team as a code owner May 15, 2026 19:57
@JaredSnider-Bitwarden JaredSnider-Bitwarden added the ai-review Request a Claude code review label May 15, 2026
JaredSnider-Bitwarden
JaredSnider-Bitwarden commented May 15, 2026
View reviewed changes
Comment thread src/Api/Models/Request/DeviceRequestModels.cs
Copy link
Copy Markdown
Contributor Author

@JaredSnider-Bitwarden JaredSnider-Bitwarden May 15, 2026

Choose a reason for hiding this comment

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

ℹ️ this is used in the device creation post seen here: https://github.com/bitwarden/server/blob/main/src/Api/Controllers/DevicesController.cs#L91-L99

JaredSnider-Bitwarden
JaredSnider-Bitwarden commented May 15, 2026
View reviewed changes
Comment thread src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs
Copy link
Copy Markdown
Contributor Author

@JaredSnider-Bitwarden JaredSnider-Bitwarden May 15, 2026

Choose a reason for hiding this comment

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

This is when we create a device on first success login.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.26%. Comparing base (419edd5) to head (e3bbf9f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Models/Request/DeviceRequestModels.cs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7649      +/-   ##
==========================================
+ Coverage   59.84%   64.26%   +4.41%     
==========================================
  Files        2121     2121              
  Lines       93460    93464       +4     
  Branches     8291     8291              
==========================================
+ Hits        55932    60063    +4131     
+ Misses      35547    31333    -4214     
- Partials     1981     2068      +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.

@JaredSnider-Bitwarden JaredSnider-Bitwarden added ai-review-vnext Request a Claude code review using the vNext workflow and removed ai-review Request a Claude code review labels May 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026 •
edited
Loading

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR fixes a bug where legacy NULL Device.LastActivityDate rows were surfacing as DateTime.UtcNow to clients. The root cause — a property initializer = DateTime.UtcNow on Device.LastActivityDate — has been removed, and the responsibility for stamping creation-time activity is moved to the two production construction sites (DeviceValidator.GetDeviceFromRequest for the auth grant flow and DeviceRequestModel.ToDevice(Guid?) for the POST /devices flow). The seeder factory and integration test helper were updated to mirror production behavior, and a new integration test asserts that a NULL LastActivityDate in the database round-trips as NULL rather than the legacy default.

Code Review Details

No findings. The change is well-scoped: both production new Device { ... } sites in src/ have been updated, the seeder mirrors production, and the new GetManyByUserIdWithDeviceAuth_NullLastActivityDateInDb_ReturnsNullNotDefaultAsync integration test locks in the legacy-NULL passthrough behavior. The internal set → set visibility change on LastActivityDate is required because Bit.Api is not in InternalsVisibleTo for Bit.Core, and is consistent with how the property is already mutated by EF/Dapper repositories.

Patrick-Pimentel-Bitwarden
Patrick-Pimentel-Bitwarden approved these changes May 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden left a comment

Choose a reason for hiding this comment

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

Changes look good!

@JaredSnider-Bitwarden JaredSnider-Bitwarden removed the request for review from rr-bw May 15, 2026 20:13
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 15, 2026

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

@JaredSnider-Bitwarden JaredSnider-Bitwarden merged commit eb9c8c9 into main May 15, 2026
46 checks passed
@JaredSnider-Bitwarden JaredSnider-Bitwarden deleted the auth/pm-37621/device-null-last-activity-date-defaulting-to-value branch May 15, 2026 20:34
@djsmith85 djsmith85 added the t:bugfix Change Type - Bugfix label May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden approved these changes

Assignees

No one assigned

Labels

ai-review-vnext Request a Claude code review using the vNext workflow t:bugfix Change Type - Bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JaredSnider-Bitwarden @Patrick-Pimentel-Bitwarden @djsmith85