From 14b4f55bfe7c71be398194050dd5398089fd763a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Mon, 31 Aug 2026 19:23:46 +0200 Subject: [PATCH 1/4] test: pin legacy device-token register compatibility Every shipped flutter-time build sends "" for app_id and installation_id (proto3 decodes an absent string as empty), so the newly mandatory fields lock the whole fleet out of registering until the app reaches stores. These tests state the contract that fixes it: empty app_id defaults to "time", empty installation_id falls back to match-by-token with a deterministic synthetic id, and an upgraded client adopts the row the legacy registers created. Red until the service implements it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk --- .../DeviceTokenServiceTests.cs | 245 +++++++++++++++++- 1 file changed, 239 insertions(+), 6 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs index 6c1adb49..2450bd42 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs @@ -1,3 +1,4 @@ +using System; using System.Linq; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore; @@ -10,6 +11,7 @@ using NSubstitute; using NUnit.Framework; using TimePlanning.Pn.Services.DeviceTokenService; +using RegisterDeviceTokenModel = TimePlanning.Pn.Infrastructure.Models.DeviceToken.RegisterDeviceTokenModel; namespace TimePlanning.Pn.Test; @@ -313,21 +315,252 @@ public async Task RegisterAsync_Adoption_DoesNotCrossAppId() }); } + // --------------------------------------------------------------------- + // Backward compatibility with clients shipped before the identity model. + // + // proto3 decodes an absent string as "", so every already-installed build + // sends "" for app_id AND installation_id. Rejecting those would leave the + // whole fleet unable to register (new installs get no push at all; a + // rotated FCM token can never re-register) until the app reaches stores - + // and flutter-time reports each rejection to Sentry, so it would also be a + // continuous fleet-wide warning flood. + // --------------------------------------------------------------------- + + // sha256("legacy-tok-golden"), lowercase hex, under the reserved prefix. + // Hard-coded on purpose: it pins the algorithm, the casing and the prefix, + // so a random or row-id-derived scheme cannot pass. + private const string LegacyGoldenToken = "legacy-tok-golden"; + + private const string LegacyGoldenInstallationId = + "legacy-token:04095630657a3b3ffbc147418b20e2c539cd9d69bde95c081049bad010b7a71e"; + [Test] - public async Task RegisterAsync_EmptyInstallationId_IsRejectedWithoutStoring() + public async Task RegisterAsync_EmptyAppId_DefaultsToTimeAndStores() { - var result = await _service.RegisterAsync(350, "tok-x", "android", 0, "time", ""); + var result = await _service.RegisterAsync( + 351, "tok-noapp", "android", 7, "", "inst-noapp"); - Assert.That(result.Success, Is.False); - Assert.That(await TimePlanningPnDbContext!.DeviceTokens.CountAsync(), Is.EqualTo(0)); + Assert.That(result.Success, Is.True); + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.Multiple(() => + { + Assert.That(stored.AppId, Is.EqualTo(DeviceTokenService.TimePlanningAppId), + "this service backs exactly one app, so an empty app_id is not ambiguous"); + Assert.That(stored.InstallationId, Is.EqualTo("inst-noapp")); + Assert.That(stored.SdkSiteId, Is.EqualTo(351)); + Assert.That(stored.AppBuildNumber, Is.EqualTo(7)); + }); + } + + [Test] + public async Task RegisterAsync_EmptyInstallationId_NoExistingRow_CreatesRowWithDeterministicId() + { + var result = await _service.RegisterAsync( + 360, LegacyGoldenToken, "android", 12, "time", ""); + + Assert.That(result.Success, Is.True); + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.Multiple(() => + { + Assert.That(stored.InstallationId, Is.EqualTo(LegacyGoldenInstallationId), + "the synthetic id must be a pure function of the token, or the same device " + + "would insert a fresh row on every legacy register"); + Assert.That(stored.InstallationId, Has.Length.LessThanOrEqualTo(128), + "InstallationId is varchar(128)"); + Assert.That(stored.SdkSiteId, Is.EqualTo(360)); + Assert.That(stored.FcmToken, Is.EqualTo(LegacyGoldenToken)); + Assert.That(stored.AppId, Is.EqualTo("time")); + Assert.That(stored.Platform, Is.EqualTo("android")); + Assert.That(stored.AppBuildNumber, Is.EqualTo(12)); + Assert.That(stored.WorkflowState, Is.EqualTo(Constants.WorkflowStates.Created)); + }); } [Test] - public async Task RegisterAsync_EmptyAppId_IsRejectedWithoutStoring() + public async Task RegisterAsync_SyntheticInstallationId_CannotCollideWithAClientUuid() { - var result = await _service.RegisterAsync(351, "tok-y", "android", 0, "", "inst-noapp"); + await _service.RegisterAsync(361, "tok-shape", "android", 0, "", ""); + + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.Multiple(() => + { + Assert.That(stored.InstallationId, Does.StartWith("legacy-token:")); + Assert.That(Guid.TryParse(stored.InstallationId, out _), Is.False, + "real clients send a v4 UUID; the reserved prefix is outside that alphabet"); + }); + } + + [Test] + public async Task RegisterAsync_EmptyInstallationIdTwice_UpdatesTheSameRow() + { + await _service.RegisterAsync(370, "tok-legacy-twice", "android", 1, "", ""); + var first = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + + var result = await _service.RegisterAsync(371, "tok-legacy-twice", "ios", 2, "", ""); + + Assert.That(result.Success, Is.True); + var rows = await TimePlanningPnDbContext.DeviceTokens.AsNoTracking().ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(1), + "a repeated legacy register from one device must not insert a duplicate"); + Assert.Multiple(() => + { + Assert.That(rows[0].Id, Is.EqualTo(first.Id)); + Assert.That(rows[0].SdkSiteId, Is.EqualTo(371)); + Assert.That(rows[0].Platform, Is.EqualTo("ios")); + Assert.That(rows[0].AppBuildNumber, Is.EqualTo(2)); + }); + } + + [Test] + public async Task RegisterAsync_EmptyInstallationId_ExistingRowForToken_UpdatesInPlace() + { + await _service.RegisterAsync(380, "tok-existing", "android", 5, "time", "inst-real-existing"); + var before = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + + var result = await _service.RegisterAsync(381, "tok-existing", "ios", 6, "", ""); + + Assert.That(result.Success, Is.True); + var rows = await TimePlanningPnDbContext.DeviceTokens.AsNoTracking().ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(1)); + Assert.Multiple(() => + { + Assert.That(rows[0].Id, Is.EqualTo(before.Id), + "match-by-token is the pre-change behaviour and must reuse the row"); + Assert.That(rows[0].InstallationId, Is.EqualTo("inst-real-existing"), + "a legacy register must not downgrade a real install id to the synthetic one"); + Assert.That(rows[0].SdkSiteId, Is.EqualTo(381)); + Assert.That(rows[0].Platform, Is.EqualTo("ios")); + Assert.That(rows[0].AppBuildNumber, Is.EqualTo(6)); + }); + } + + // The whole point of a deterministic synthetic id: when the fleet finally + // upgrades, the real installation_id must CLAIM the row the legacy + // registers created. A second row would be live too, carry the same token + // and site, and the sender would select both - doubled pushes forever. + [Test] + public async Task RegisterAsync_LegacyRow_IsAdoptedWhenTheClientUpgrades() + { + await _service.RegisterAsync(390, "tok-upgrade", "android", 0, "", ""); + var legacyRow = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.That(legacyRow.InstallationId, Does.StartWith("legacy-token:")); + + var result = await _service.RegisterAsync( + 390, "tok-upgrade", "android", 43000, "time", "11111111-2222-4333-8444-555555555555"); + + Assert.That(result.Success, Is.True); + var rows = await TimePlanningPnDbContext.DeviceTokens.AsNoTracking().ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(1), + "the upgraded client must claim the legacy row, not add a second one"); + Assert.Multiple(() => + { + Assert.That(rows[0].Id, Is.EqualTo(legacyRow.Id)); + Assert.That(rows[0].InstallationId, Is.EqualTo("11111111-2222-4333-8444-555555555555")); + Assert.That(rows[0].AppBuildNumber, Is.EqualTo(43000)); + }); + } + + [Test] + public async Task RegisterAsync_SoftDeletedLegacyRow_EmptyInstallationId_IsRevived() + { + await _service.RegisterAsync(400, "tok-legacy-pruned", "android", 0, "", ""); + var row = await TimePlanningPnDbContext!.DeviceTokens.SingleAsync(); + await row.Delete(TimePlanningPnDbContext); + + var result = await _service.RegisterAsync(400, "tok-legacy-pruned", "android", 0, "", ""); + + Assert.That(result.Success, Is.True); + var rows = await TimePlanningPnDbContext.DeviceTokens.AsNoTracking().ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(1), + "a pruned row must be revived, never inserted alongside"); + Assert.That(rows[0].WorkflowState, Is.EqualTo(Constants.WorkflowStates.Created)); + } + + [Test] + public async Task RegisterAsync_LegacyRegister_DoesNotTakeAnotherAppsRow() + { + var foreign = new DeviceToken + { + AppId = "adhoc", + InstallationId = "inst-adhoc", + FcmToken = "tok-cross", + SdkSiteId = 410, + Platform = "android", + }; + await foreign.Create(TimePlanningPnDbContext!); + + await _service.RegisterAsync(410, "tok-cross", "android", 0, "", ""); + + var rows = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking() + .OrderBy(r => r.Id).ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(2)); + Assert.Multiple(() => + { + Assert.That(rows[0].AppId, Is.EqualTo("adhoc")); + Assert.That(rows[0].InstallationId, Is.EqualTo("inst-adhoc")); + Assert.That(rows[1].AppId, Is.EqualTo("time")); + Assert.That(rows[1].InstallationId, Does.StartWith("legacy-token:")); + }); + } + + // fcm_token stays hard-required: there is no sensible fallback for an + // absent token, and it is not one of the fields old clients omit. + [Test] + public async Task RegisterAsync_EmptyToken_IsRejectedWithoutStoring() + { + var result = await _service.RegisterAsync(420, "", "android", 0, "time", "inst-notoken"); Assert.That(result.Success, Is.False); Assert.That(await TimePlanningPnDbContext!.DeviceTokens.CountAsync(), Is.EqualTo(0)); } + + [Test] + public async Task RegisterAsync_BothFieldsSupplied_StoresExactlyWhatTheClientSent() + { + var result = await _service.RegisterAsync( + 430, "tok-modern", "ios", 44000, "time", "aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee"); + + Assert.That(result.Success, Is.True); + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.Multiple(() => + { + Assert.That(stored.AppId, Is.EqualTo("time")); + Assert.That(stored.InstallationId, Is.EqualTo("aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee"), + "the modern path must store the client's id verbatim"); + Assert.That(stored.SdkSiteId, Is.EqualTo(430)); + Assert.That(stored.Platform, Is.EqualTo("ios")); + Assert.That(stored.AppBuildNumber, Is.EqualTo(44000)); + }); + } + + // The REST endpoint is reachable too. DeviceTokenController.Register hands + // the bound model straight to RegisterAsync, and a body from a pre-identity + // client omits both fields - which the model materialises as "". + [Test] + public async Task RegisterAsync_RestModelDefaults_TakeTheLegacyPath() + { + var model = new RegisterDeviceTokenModel + { + Token = "tok-rest", + Platform = "android", + }; + Assert.Multiple(() => + { + Assert.That(model.AppId, Is.Empty); + Assert.That(model.InstallationId, Is.Empty); + }); + + var result = await _service.RegisterAsync( + 440, model.Token, model.Platform, model.BuildNumber, + model.AppId, model.InstallationId); + + Assert.That(result.Success, Is.True); + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.Multiple(() => + { + Assert.That(stored.AppId, Is.EqualTo("time")); + Assert.That(stored.InstallationId, Does.StartWith("legacy-token:")); + Assert.That(stored.SdkSiteId, Is.EqualTo(440)); + }); + } } From 10b7cd9467632e3aaf305934c73abe61f0e6c7cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Mon, 31 Aug 2026 19:30:14 +0200 Subject: [PATCH 2/4] test: pin that a legacy-registered row still reaches the send path Defaulting app_id only helps if the resulting row is visible to PushNotificationService.ResolveTargetTokensAsync, whose query is keyed on AppId. A row stored with a null or empty AppId would be invisible to every push and the compatibility fix would be worthless. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk --- .../DeviceTokenServiceTests.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs index 2450bd42..ef5fda8b 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs @@ -11,6 +11,7 @@ using NSubstitute; using NUnit.Framework; using TimePlanning.Pn.Services.DeviceTokenService; +using TimePlanning.Pn.Services.PushNotificationService; using RegisterDeviceTokenModel = TimePlanning.Pn.Infrastructure.Models.DeviceToken.RegisterDeviceTokenModel; namespace TimePlanning.Pn.Test; @@ -533,6 +534,30 @@ public async Task RegisterAsync_BothFieldsSupplied_StoresExactlyWhatTheClientSen }); } + // The point of DEFAULTING app_id rather than rejecting it: the row a legacy + // register writes must still be picked up by the send path, whose query is + // keyed on AppId (IX_DeviceTokens_AppId_SdkSiteId_WorkflowState). A row + // stored with a null or empty AppId would be invisible to every push - + // which would defeat the entire change. + [Test] + public async Task RegisterAsync_LegacyRegisteredRow_IsSelectedByTheSendPath() + { + await _service.RegisterAsync(450, "tok-sendpath", "android", 0, "", ""); + + // Firebase is not configured in tests, so this constructor never + // touches the process-wide FirebaseApp registry; only the token- + // selection seam is exercised. + var push = new PushNotificationService( + TimePlanningPnDbContext!, + Substitute.For>()); + + var targeted = await push.ResolveTargetTokensAsync(450, minBuild: 0); + + Assert.That(targeted.Select(t => t.FcmToken), + Is.EquivalentTo(new[] { "tok-sendpath" }), + "a device that registered through the legacy path must still receive push"); + } + // The REST endpoint is reachable too. DeviceTokenController.Register hands // the bound model straight to RegisterAsync, and a body from a pre-identity // client omits both fields - which the model materialises as "". From a78a5d7432d5ea527ca2259540aa8cbeee34dd72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Mon, 31 Aug 2026 19:47:36 +0200 Subject: [PATCH 3/4] fix: accept device-token registers from clients shipped before the identity model app_id and installation_id were made mandatory, but proto3 decodes an absent string as "", so every already-installed flutter-time build is now rejected. Until the app reaches both stores that means new installs never register at all, a device whose FCM token rotates can never re-register and goes dark permanently, and - since flutter-time reports each rejection to Sentry from both personal_view_widget.dart and main.dart, on every personal-view mount and every app start - the whole fleet on old builds floods Sentry continuously. Empty app_id now means "time": this service backs exactly one app, so there is nothing to disambiguate. (The same relaxation would NOT be sound in BackendConfiguration, whose table is shared by two apps.) Empty installation_id falls back to matching on the FCM token, the pre-change identity. A matched row is updated in place and keeps its own InstallationId, so a legacy register can never downgrade a real install id. An insert needs one - the column is NOT NULL - so it gets a deterministic 'legacy-token:', which makes repeated legacy registers from one device resolve to the SAME row instead of inserting duplicates, and which the existing adoption path rewrites to the real id once the client upgrades. The reserved prefix cannot collide with a client-generated v4 UUID; see the comment on SyntheticInstallationIdFor for why hashing the token is sound in THIS table specifically. fcm_token stays hard-required and is now explicitly validated: without that guard the legacy path would derive an installation id from the empty string and store a junk row. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk --- .../Controllers/DeviceTokenController.cs | 3 + .../DeviceToken/RegisterDeviceTokenModel.cs | 9 +- .../DeviceTokenService/DeviceTokenService.cs | 105 ++++++++++++++---- .../DeviceTokenService/IDeviceTokenService.cs | 12 +- .../TimePlanningDeviceTokenGrpcService.cs | 8 +- 5 files changed, 105 insertions(+), 32 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs index 407a6ca3..8e3b0c4b 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs @@ -33,6 +33,9 @@ public DeviceTokenController( _baseDbContext = baseDbContext; } + // Reachable public endpoint even though the shipped client uses gRPC. It + // hands the bound model straight to the service, which reads an omitted + // app_id / installation_id the same way the gRPC path does. [HttpPost] public async Task Register([FromBody] RegisterDeviceTokenModel model) { diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Models/DeviceToken/RegisterDeviceTokenModel.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Models/DeviceToken/RegisterDeviceTokenModel.cs index 58c9b0cc..34d7fc4a 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Models/DeviceToken/RegisterDeviceTokenModel.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Models/DeviceToken/RegisterDeviceTokenModel.cs @@ -5,11 +5,16 @@ public class RegisterDeviceTokenModel public string Token { get; set; } = string.Empty; public string Platform { get; set; } = string.Empty; - /// Which app minted the token. Always "time" here. + /// + /// Which app minted the token. Always "time" here, so a body that omits it + /// (binding to "") is read as "time" rather than rejected. + /// public string AppId { get; set; } = string.Empty; /// - /// Stable per-install UUID; the identity of the stored row. Required. + /// Stable per-install UUID; the identity of the stored row. A body that + /// omits it binds to "" and falls back to matching on the FCM token - see + /// IDeviceTokenService.RegisterAsync. /// public string InstallationId { get; set; } = string.Empty; diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs index f9f0b1f9..26cf4262 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs @@ -2,6 +2,8 @@ namespace TimePlanning.Pn.Services.DeviceTokenService; using System; using System.Linq; +using System.Security.Cryptography; +using System.Text; using System.Threading.Tasks; using Infrastructure.Helpers; using Microsoft.EntityFrameworkCore; @@ -95,27 +97,46 @@ public async Task RegisterAsync( { try { - if (string.IsNullOrWhiteSpace(appId)) + // The token is the one field with no sensible fallback, and it is + // not what old clients omit - keep it hard-required. Without this + // guard the legacy path below would derive an installation id from + // the empty string and store a junk row. + if (string.IsNullOrWhiteSpace(token)) { _logger.LogWarning( - "Rejecting device-token registration: app_id is required"); - return new OperationResult(false, "app_id is required"); + "Rejecting device-token registration: fcm_token is required"); + return new OperationResult(false, "fcm_token is required"); } - if (string.IsNullOrWhiteSpace(installationId)) + // Clients shipped before the identity model send neither app_id nor + // installation_id, and proto3 decodes an absent string as "". They + // must keep registering until the app reaches stores, so both get a + // backward-compatible reading here rather than a rejection. + // + // app_id: this service backs exactly one app, so "" is unambiguous. + // (The same relaxation would NOT be sound in BackendConfiguration, + // whose table is shared by two apps.) + if (string.IsNullOrWhiteSpace(appId)) { - _logger.LogWarning( - "Rejecting device-token registration: installation_id is required"); - return new OperationResult(false, "installation_id is required"); + appId = TimePlanningAppId; } + var isLegacyRegister = string.IsNullOrWhiteSpace(installationId); + // Upsert on the install identity, including soft-deleted rows: the // unique index has no WorkflowState filter, so Create() over a // Removed row throws instead of inserting. - var existing = await _dbContext.DeviceTokens - .FirstOrDefaultAsync(dt => dt.AppId == appId - && dt.InstallationId == installationId) - ?? await AdoptRowWithSameTokenAsync(appId, token, installationId); + // + // installation_id: a legacy register has none, so fall back to the + // pre-change identity - the token alone. The row found that way is + // updated in place and KEEPS its InstallationId, so a legacy + // register can never downgrade a real install id to a synthetic one. + var existing = isLegacyRegister + ? await FindRowWithSameTokenAsync(appId, token) + : await _dbContext.DeviceTokens + .FirstOrDefaultAsync(dt => dt.AppId == appId + && dt.InstallationId == installationId) + ?? await AdoptRowWithSameTokenAsync(appId, token, installationId); if (existing != null) { @@ -134,7 +155,9 @@ public async Task RegisterAsync( var deviceToken = new DeviceToken { AppId = appId, - InstallationId = installationId, + InstallationId = isLegacyRegister + ? SyntheticInstallationIdFor(token) + : installationId, FcmToken = token, SdkSiteId = sdkSiteId, Platform = platform, @@ -151,6 +174,46 @@ public async Task RegisterAsync( } } + /// Reserved prefix for synthesised installation ids. + private const string LegacyInstallationIdPrefix = "legacy-token:"; + + /// + /// InstallationId stood in for a client that sent none. Derived from the + /// FCM token so that repeated legacy registers from one device resolve to + /// the SAME row instead of inserting a duplicate on every call - the column + /// is NOT NULL, so an insert has to put something there. + /// + /// sha256(token) as lowercase hex, under a reserved prefix. That prefix is + /// what makes a collision with a real client id impossible: clients send a + /// canonical v4 UUID - 36 characters over [0-9a-f-] - and ':' is outside + /// that alphabet, so no client-generated value can ever equal one of these. + /// It is equally distinct from the migration's 'legacy:<Id>' backfill. + /// The result is 77 characters, well inside InstallationId's varchar(128). + /// + /// Hashing the TOKEN is sound here specifically because this table's old + /// unique index was on Token alone: tokens are unique in it, so two + /// distinct devices cannot hash to the same id. The identical formula was + /// rejected for the BackendConfiguration migration, whose old key was + /// (WorkerId, FcmToken) and therefore permits duplicate tokens - that + /// rejection does not carry over to this table. + /// + private static string SyntheticInstallationIdFor(string token) + { + var digest = SHA256.HashData(Encoding.UTF8.GetBytes(token)); + return LegacyInstallationIdPrefix + Convert.ToHexString(digest).ToLowerInvariant(); + } + + /// + /// The pre-identity lookup: one row per FCM token within this app. Ordered + /// by Id so the outcome is deterministic if two rows ever share a token; + /// the oldest wins. + /// + private Task FindRowWithSameTokenAsync(string appId, string token) => + _dbContext.DeviceTokens + .Where(dt => dt.AppId == appId && dt.FcmToken == token) + .OrderBy(dt => dt.Id) + .FirstOrDefaultAsync(); + /// /// Claims a pre-existing row that already carries this FCM token under a /// different InstallationId, rewriting its InstallationId to the real one. @@ -164,22 +227,16 @@ public async Task RegisterAsync( /// pre-existing user gets doubled pushes indefinitely. FCM never clears it /// either, because the legacy row's token is perfectly valid. /// - /// Ordered by Id so the outcome is deterministic if two rows ever share a - /// token; the oldest wins. Pre-migration that could not happen at all - - /// this table's old unique index was on Token alone. + /// It also claims a row a LEGACY register created under a synthetic + /// installation id: when the fleet upgrades and starts sending real ids, + /// that row must be rewritten rather than joined by a second live row + /// carrying the same token and site - the sender would select both and the + /// user would get every push twice, forever. /// private async Task AdoptRowWithSameTokenAsync( string appId, string token, string installationId) { - if (string.IsNullOrWhiteSpace(token)) - { - return null; - } - - var adopted = await _dbContext.DeviceTokens - .Where(dt => dt.AppId == appId && dt.FcmToken == token) - .OrderBy(dt => dt.Id) - .FirstOrDefaultAsync(); + var adopted = await FindRowWithSameTokenAsync(appId, token); if (adopted == null) { diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/IDeviceTokenService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/IDeviceTokenService.cs index 28a11f59..02829084 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/IDeviceTokenService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/IDeviceTokenService.cs @@ -12,7 +12,7 @@ public interface IDeviceTokenService /// is the client's app build number /// (0 = old/unknown), stored for push version-gating. /// and are the - /// stored row's identity; see . + /// stored row's identity; either may be empty, see . /// Task RegisterForCallerAsync( string token, string platform, int buildNumber = 0, @@ -23,8 +23,14 @@ Task RegisterForCallerAsync( /// (, ) - the app /// install, not the FCM token. A rotated token updates that install's row /// in place, and a different user on the same install reassigns - /// . Both arguments are required; an empty one - /// is rejected without storing anything. + /// . + /// + /// Both carry a backward-compatible reading for clients shipped before the + /// identity model, which send neither (proto3 decodes an absent string as + /// ""): an empty means "time", the only app this + /// service serves, and an empty falls + /// back to matching on the FCM token, the pre-change identity. Only + /// is hard-required. /// Task RegisterAsync( int sdkSiteId, string token, string platform, int buildNumber = 0, diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningDeviceTokenGrpcService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningDeviceTokenGrpcService.cs index 1f6f3616..cbe70878 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningDeviceTokenGrpcService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningDeviceTokenGrpcService.cs @@ -25,9 +25,11 @@ public override async Task RegisterDeviceToken( // DeviceTokenService); the client no longer sends one. The client's // reported app build number is persisted for push version-gating, // and (AppId, InstallationId) is the identity of the stored row. - // An empty app_id or installation_id comes back as an unsuccessful - // OperationResponse - this transport reports failures in the - // response body rather than as a gRPC status. + // Clients shipped before that identity model send neither field and + // proto3 decodes both as "", so the service reads them + // backward-compatibly rather than rejecting; only an empty token + // comes back as an unsuccessful OperationResponse - this transport + // reports failures in the response body, not as a gRPC status. var result = await _deviceTokenService.RegisterForCallerAsync( request.Token, request.Platform, request.BuildNumber, request.AppId, request.InstallationId); From 2a9210e7cb44c7a0d9bf6f142288e84024f37ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Mon, 31 Aug 2026 19:59:26 +0200 Subject: [PATCH 4/4] review: survive the legacy insert race, guard a live token, tighten comments From the dual gate (code-review + code-simplifier) on this branch. Recover from a lost insert race instead of reporting it as a failure. Two legacy registers are concurrent on a client's FIRST launch - flutter-time's personal-view init and its onTokenRefresh handler both register the same token - so both miss the token lookup and both derive the same synthetic id. The loser violated the unique index and returned an error, which the client turns into exactly the Sentry warning this change exists to stop. It now detaches the failed insert, re-reads the row the winner stored and updates that. This also converges when a row already holds the installation id under a stale token. Say out loud what the synthetic id cannot do: a legacy client's token ROTATION still leaves a second row behind, because nothing ties the new token to the old one without a real install id. Pre-identity-model behaviour, not a regression, and it ends when the client upgrades - but the comment previously implied otherwise, and no test said what happens. Also: pin that an empty token cannot blank a LIVE stored token (the unguarded code reached the assignment and took that device dark, which is worse than the empty-DB case already covered); replace the ternary selecting `existing` with if/else so the adoption fallback's scoping is syntactic rather than precedence-dependent; extract ApplyRegistration, now shared by the update and race-recovery paths; and soften "collision is impossible" to what is actually true - no client we ship can produce one, since nothing server-side validates the shape. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HknsjYQGfBzXpCtSXgU8Nk --- .../DeviceTokenServiceTests.cs | 98 +++++++++++- .../Controllers/DeviceTokenController.cs | 6 +- .../DeviceTokenService/DeviceTokenService.cs | 150 ++++++++++++------ 3 files changed, 198 insertions(+), 56 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs index ef5fda8b..31659491 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DeviceTokenServiceTests.cs @@ -327,11 +327,12 @@ public async Task RegisterAsync_Adoption_DoesNotCrossAppId() // continuous fleet-wide warning flood. // --------------------------------------------------------------------- - // sha256("legacy-tok-golden"), lowercase hex, under the reserved prefix. - // Hard-coded on purpose: it pins the algorithm, the casing and the prefix, - // so a random or row-id-derived scheme cannot pass. private const string LegacyGoldenToken = "legacy-tok-golden"; + // sha256(LegacyGoldenToken), lowercase hex, under the reserved prefix. + // Hard-coded on purpose - never computed from the production helper: the + // literal pins the algorithm, the casing and the prefix, so a random or + // row-id-derived scheme cannot pass. private const string LegacyGoldenInstallationId = "legacy-token:04095630657a3b3ffbc147418b20e2c539cd9d69bde95c081049bad010b7a71e"; @@ -515,6 +516,10 @@ public async Task RegisterAsync_EmptyToken_IsRejectedWithoutStoring() Assert.That(await TimePlanningPnDbContext!.DeviceTokens.CountAsync(), Is.EqualTo(0)); } + // Deliberately passes both before and after this change: it is the + // requirement that the modern path stays untouched, not a change detector. + // The modern path's adoption leg is covered separately by + // RegisterAsync_LegacyBackfilledRow_IsAdoptedNotDuplicated. [Test] public async Task RegisterAsync_BothFieldsSupplied_StoresExactlyWhatTheClientSent() { @@ -534,6 +539,80 @@ public async Task RegisterAsync_BothFieldsSupplied_StoresExactlyWhatTheClientSen }); } + // Two legacy registers can be in flight at once on a client's FIRST launch: + // flutter-time's personal-view init and its onTokenRefresh handler both + // register the same token, so both miss the token lookup and both derive + // the SAME synthetic id - the loser hits the unique index. That must not + // surface as a failed register, because the client turns a failure into the + // Sentry warning this whole change exists to stop. + // + // Seeding a row that already holds the synthetic id under a stale token + // reproduces the losing insert deterministically, without threads. + [Test] + public async Task RegisterAsync_LegacyInsertHitsTheUniqueIndex_AdoptsTheExistingRow() + { + var occupier = new DeviceToken + { + AppId = "time", + InstallationId = LegacyGoldenInstallationId, + FcmToken = "stale-token", + SdkSiteId = 500, + Platform = "android", + }; + await occupier.Create(TimePlanningPnDbContext!); + + var result = await _service.RegisterAsync(501, LegacyGoldenToken, "ios", 9, "", ""); + + Assert.That(result.Success, Is.True, + "losing the insert race must not be reported to the client as a failure"); + var rows = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().ToListAsync(); + Assert.That(rows, Has.Count.EqualTo(1)); + Assert.Multiple(() => + { + Assert.That(rows[0].Id, Is.EqualTo(occupier.Id)); + Assert.That(rows[0].FcmToken, Is.EqualTo(LegacyGoldenToken)); + Assert.That(rows[0].SdkSiteId, Is.EqualTo(501)); + Assert.That(rows[0].Platform, Is.EqualTo("ios")); + Assert.That(rows[0].AppBuildNumber, Is.EqualTo(9)); + }); + } + + // A legacy client's token ROTATION cannot land on the old row - nothing + // ties the new token to the old one without a real install id - so it + // leaves a second row behind and the device is pushed to twice until FCM + // reports the dead token. This pins that honestly: it is the + // pre-identity-model behaviour, not something this change introduced, and + // it ends the moment the client upgrades and sends a real installation id. + [Test] + public async Task RegisterAsync_LegacyRegister_AfterTokenRotation_LeavesTheOldRowBehind() + { + await _service.RegisterAsync(460, "tok-rot-1", "android", 0, "", ""); + await _service.RegisterAsync(460, "tok-rot-2", "android", 0, "", ""); + + var rows = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking() + .OrderBy(r => r.Id).ToListAsync(); + Assert.That(rows.Select(r => r.FcmToken), + Is.EqualTo(new[] { "tok-rot-1", "tok-rot-2" }), + "documented limitation: without a real install id the rotated token is a new row"); + } + + // Rejecting an empty token matters most when a row already exists: the + // unguarded code reached the field assignment and blanked a WORKING token, + // taking that device dark. If anyone moves the guard below the identity + // lookup, this fails. + [Test] + public async Task RegisterAsync_EmptyToken_ExistingRow_DoesNotWipeTheStoredToken() + { + await _service.RegisterAsync(470, "tok-keep", "android", 3, "time", "inst-keep"); + + var result = await _service.RegisterAsync(470, "", "android", 3, "time", "inst-keep"); + + Assert.That(result.Success, Is.False); + var stored = await TimePlanningPnDbContext!.DeviceTokens.AsNoTracking().SingleAsync(); + Assert.That(stored.FcmToken, Is.EqualTo("tok-keep"), + "an empty token must never overwrite a live one"); + } + // The point of DEFAULTING app_id rather than rejecting it: the row a legacy // register writes must still be picked up by the send path, whose query is // keyed on AppId (IX_DeviceTokens_AppId_SdkSiteId_WorkflowState). A row @@ -555,12 +634,17 @@ public async Task RegisterAsync_LegacyRegisteredRow_IsSelectedByTheSendPath() Assert.That(targeted.Select(t => t.FcmToken), Is.EquivalentTo(new[] { "tok-sendpath" }), - "a device that registered through the legacy path must still receive push"); + "a legacy-registered device must still be targeted by an ungated send. " + + "Version-gated sends (minBuild > 0) still skip it, because a client " + + "old enough to omit installation_id also reports AppBuildNumber 0"); } - // The REST endpoint is reachable too. DeviceTokenController.Register hands - // the bound model straight to RegisterAsync, and a body from a pre-identity - // client omits both fields - which the model materialises as "". + // The REST endpoint is reachable too, and DeviceTokenController.Register + // hands the bound model straight to RegisterAsync. This pins the seam that + // can silently drift - RegisterDeviceTokenModel's defaults, which are what + // a pre-identity-model JSON body binds to - by feeding them through the + // service. It does not exercise the controller itself (that needs the JWT + // site resolution); the controller's pass-through is a two-line method. [Test] public async Task RegisterAsync_RestModelDefaults_TakeTheLegacyPath() { diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs index 8e3b0c4b..5680d7aa 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Controllers/DeviceTokenController.cs @@ -33,9 +33,9 @@ public DeviceTokenController( _baseDbContext = baseDbContext; } - // Reachable public endpoint even though the shipped client uses gRPC. It - // hands the bound model straight to the service, which reads an omitted - // app_id / installation_id the same way the gRPC path does. + // Reachable public endpoint even though the shipped client uses gRPC: a + // posted body that omits app_id / installation_id binds them to "", which + // the service reads exactly the way it reads the gRPC path's "". [HttpPost] public async Task Register([FromBody] RegisterDeviceTokenModel model) { diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs index 26cf4262..41f04b80 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/DeviceTokenService/DeviceTokenService.cs @@ -126,45 +126,76 @@ public async Task RegisterAsync( // Upsert on the install identity, including soft-deleted rows: the // unique index has no WorkflowState filter, so Create() over a // Removed row throws instead of inserting. - // - // installation_id: a legacy register has none, so fall back to the - // pre-change identity - the token alone. The row found that way is - // updated in place and KEEPS its InstallationId, so a legacy - // register can never downgrade a real install id to a synthetic one. - var existing = isLegacyRegister - ? await FindRowWithSameTokenAsync(appId, token) - : await _dbContext.DeviceTokens - .FirstOrDefaultAsync(dt => dt.AppId == appId - && dt.InstallationId == installationId) - ?? await AdoptRowWithSameTokenAsync(appId, token, installationId); + DeviceToken existing; + if (isLegacyRegister) + { + // installation_id: a legacy register has none, so fall back to + // the pre-change identity - the token alone. The row found this + // way is updated in place and KEEPS its InstallationId, so a + // legacy register can never downgrade a real install id to a + // synthetic one. + existing = await FindRowWithSameTokenAsync(appId, token); + } + else + { + existing = await _dbContext.DeviceTokens + .FirstOrDefaultAsync(dt => dt.AppId == appId + && dt.InstallationId == installationId); + existing ??= await AdoptRowWithSameTokenAsync(appId, token, installationId); + } if (existing != null) { - existing.FcmToken = token; - existing.SdkSiteId = sdkSiteId; - existing.Platform = platform; - existing.AppBuildNumber = buildNumber; - // Explicit revive: PnBase.Update() leaves WorkflowState alone, - // so a row pruned after an FCM permanent failure would stay - // invisible to the send path and the device would go dark. - existing.WorkflowState = Constants.WorkflowStates.Created; + ApplyRegistration(existing, token, sdkSiteId, platform, buildNumber); await existing.Update(_dbContext); + return new OperationResult(true); } - else + + var newInstallationId = isLegacyRegister + ? SyntheticInstallationIdFor(token) + : installationId; + + var deviceToken = new DeviceToken + { + AppId = appId, + InstallationId = newInstallationId, + FcmToken = token, + SdkSiteId = sdkSiteId, + Platform = platform, + AppBuildNumber = buildNumber, + }; + + try { - var deviceToken = new DeviceToken - { - AppId = appId, - InstallationId = isLegacyRegister - ? SyntheticInstallationIdFor(token) - : installationId, - FcmToken = token, - SdkSiteId = sdkSiteId, - Platform = platform, - AppBuildNumber = buildNumber, - }; await deviceToken.Create(_dbContext); } + catch (DbUpdateException) + { + // Lost an insert race on IX_DeviceTokens_AppId_InstallationId. + // Reachable on a legacy client's FIRST launch, which is the + // "new installs never register" case this change exists to fix: + // flutter-time's personal-view init and its onTokenRefresh + // handler both register the same token concurrently, so both + // miss the lookup above and both derive the same synthetic id. + // Letting that reach the catch below would return a failure the + // client turns into a Sentry warning - the exact noise being + // removed here. The winner stored the row we wanted, so take it. + // + // Also covers the rarer case of a row already holding this + // installation id under a stale token; adopting it converges on + // one row per install either way. + _dbContext.Entry(deviceToken).State = EntityState.Detached; + + var winner = await _dbContext.DeviceTokens.FirstOrDefaultAsync( + dt => dt.AppId == appId && dt.InstallationId == newInstallationId); + if (winner == null) + { + throw; + } + + ApplyRegistration(winner, token, sdkSiteId, platform, buildNumber); + await winner.Update(_dbContext); + } return new OperationResult(true); } catch (Exception ex) @@ -174,28 +205,53 @@ public async Task RegisterAsync( } } + /// + /// Writes the mutable half of a register onto an existing row: the token + /// rotates, and a different user on the same install reassigns the owner. + /// + /// The WorkflowState revive is explicit because PnBase.Update() leaves it + /// alone - a row pruned after an FCM permanent failure would otherwise stay + /// invisible to the send path and the device would go dark. + /// + private static void ApplyRegistration( + DeviceToken row, string token, int sdkSiteId, string platform, int buildNumber) + { + row.FcmToken = token; + row.SdkSiteId = sdkSiteId; + row.Platform = platform; + row.AppBuildNumber = buildNumber; + row.WorkflowState = Constants.WorkflowStates.Created; + } + /// Reserved prefix for synthesised installation ids. private const string LegacyInstallationIdPrefix = "legacy-token:"; /// /// InstallationId stood in for a client that sent none. Derived from the - /// FCM token so that repeated legacy registers from one device resolve to - /// the SAME row instead of inserting a duplicate on every call - the column - /// is NOT NULL, so an insert has to put something there. + /// FCM token so that repeated legacy registers carrying the SAME token + /// resolve to the same row instead of inserting a duplicate on every call - + /// the column is NOT NULL, so an insert has to put something there. + /// + /// It cannot make a legacy client's token ROTATION land on the old row: + /// nothing ties the new token to the old one without a real install id, so + /// that inserts a second row and the device is pushed to twice until FCM + /// reports the dead token. That is exactly the pre-identity-model + /// behaviour, not a regression, and it ends the moment the client upgrades. /// - /// sha256(token) as lowercase hex, under a reserved prefix. That prefix is - /// what makes a collision with a real client id impossible: clients send a - /// canonical v4 UUID - 36 characters over [0-9a-f-] - and ':' is outside - /// that alphabet, so no client-generated value can ever equal one of these. - /// It is equally distinct from the migration's 'legacy:<Id>' backfill. - /// The result is 77 characters, well inside InstallationId's varchar(128). + /// The reserved prefix is what rules out a collision with a real client id: + /// clients send a canonical v4 UUID - 36 characters over [0-9a-f-] - and + /// ':' is outside that alphabet, so no client we ship can produce one. + /// (Nothing server-side VALIDATES the shape; both transports accept an + /// arbitrary string, so this is a claim about our clients, not an + /// invariant.) It is equally distinct from the migration's + /// 'legacy:<Id>' backfill. The result is 77 characters, well inside + /// InstallationId's varchar(128). /// - /// Hashing the TOKEN is sound here specifically because this table's old - /// unique index was on Token alone: tokens are unique in it, so two - /// distinct devices cannot hash to the same id. The identical formula was - /// rejected for the BackendConfiguration migration, whose old key was - /// (WorkerId, FcmToken) and therefore permits duplicate tokens - that - /// rejection does not carry over to this table. + /// Hashing the TOKEN is sound in THIS table because its old unique index + /// was on Token alone: tokens are unique here, so two distinct devices + /// cannot hash to the same id. The identical formula was rejected for the + /// BackendConfiguration migration, whose old key (WorkerId, FcmToken) + /// permits duplicate tokens - that rejection does not carry over. /// private static string SyntheticInstallationIdFor(string token) { @@ -217,6 +273,8 @@ private Task FindRowWithSameTokenAsync(string appId, string token) /// /// Claims a pre-existing row that already carries this FCM token under a /// different InstallationId, rewriting its InstallationId to the real one. + /// Callers must have rejected a blank token first (RegisterAsync does); + /// this does not re-check. /// /// This exists for the DeviceTokenIdentityModel migration in /// eform-timeplanning-base, which backfills every pre-existing row with