Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/core/auth/login/provider/login_provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ class Login extends _$Login {
log('handle user loaded: ${_tbClient.getAuthUser()?.userId}');

if (!_tbClient.isAuthenticated()) {
if (getIt<IFirebaseService>().apps.isNotEmpty) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This guard reads IFirebaseService.apps, which is only populated after the unawaited initializeApp(...) in main.dart:37 — and on this particular path there is exactly one chance to get it right.

What's verifiable in the repo:

  • main.dart:37 starts initializeApp() without await, and FirebaseService.initializeApp only does _apps.add(name) after await Firebase.initializeApp(...) (firebase_service.dart:34-35).
  • RefreshListenable registers a permanent _ref.listen(loginProvider, ...) (refresh_listenable.dart:9) from inside Router.build(), which ThingsboardApp.build watches on the first frame. So the autoDispose notifier is created once and kept alive — Login.build(), and therefore the Future(() => handleUserLoaded()) at :38, runs exactly once per launch.
  • handleUserLoaded has only two live call sites: that one-shot Future and the UserLoadedEvent listener.
  • CommunicationService wraps a plain EventBus with no replay, so the UserLoadedEvent the client fires while clearing the expired token — inside setUpRootDependencies(), before runApp and before that listener exists — is dropped.

Put together: if apps is still empty when that single Future runs, the cleanup is skipped and nothing re-triggers it for the rest of the session — the fix silently no-ops on exactly the reported scenario.

What I can't tell from reading is whether Firebase actually loses that race. It's [native init started just before runApp] versus [one await AppLinks().getInitialLink() + runApp + first frame + one event-loop turn], which is device-dependent and probably fine most of the time — the isCustomEndpoint() check inside initializeApp is cheap since _cachedEndpoint is already warm from TbClientService.init(). So I'm not claiming this is broken.

But awaiting initializeApp() in main() costs nothing (it returns null rather than throwing on failure), removes the question entirely, and makes the three other apps.isNotEmpty guards deterministic too. Worth doing before merge — and worth one manual check on a genuinely cold start (app killed, token expired days ago) rather than with the app already running, since that's the only configuration where this single-shot path is exercised.

Separately, and much more minor: this is now the fourth copy of the getIt<IFirebaseService>().apps.isNotEmpty guard wrapped around a NotificationService call (Login.logout at :43, Login._onFullyLoggedIn at :119, TbContext.logout at :306, and here). It reads like an invariant of the service rather than something each call site should have to remember — which is another argument for pushing the check down into NotificationService.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 04f3f07 on both fronts. main() now awaits initializeApp() (it swallows its own errors and returns null, so the surrounding try/catch behavior is unchanged), which removes the race on the single-shot path. And the apps.isNotEmpty guard moved into NotificationService itself — init(), logout() and the renamed cleanUpStalePushRegistration() early-return when Firebase isn't configured — so all four call-site copies are gone (Login.logout keeps only its isFullyAuthenticated() check). Verified manually on a genuinely cold start with an expired refresh token: the cleanup runs and pushes stop.

await getIt<NotificationService>().handleSessionExpired();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awaiting here puts the push cleanup in front of state = const LoginState(isUserLoaded: false), which is what AuthRedirect keys off (auth_redirect.dart:36) to route to the login screen.

Having traced what the cold-start path actually does, most of it is cheap: _fcmToken is null on the freshly-constructed singleton so removeMobileSession is skipped entirely, the three subscription cancels are no-ops, and the TbStorage read hits an already-open in-memory Hive box (_tb_secure_storage.dart:36). The one that isn't cheap is _messaging.deleteToken(), which has to reach FCM to drop the registration — on a bad network that's what delays the login screen appearing. (On the "expired while the app was running" path removeMobileSession is in play too; whether that short-circuits locally instead of hitting the wire depends on client internals I couldn't check, since thingsboard_ce_client 4.4.0 isn't vendored here.)

Not dramatic, but nothing in handleUserLoaded needs the result and handleSessionExpired() already swallows its own errors, so setting the state first and then kicking off the cleanup would keep the login screen's latency independent of the network. Because the flag is only deleted at the very end of logout(), an interrupted cleanup already retries on the next launch — which is what makes fire-and-forget safe here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reordered in 04f3f07: the state is set first and the cleanup is now unawaited(...), so the login screen no longer waits on deleteToken(). The flag-deleted-last ordering that makes the retry safe is asserted by a test ("keeps the registration flag when the cleanup is interrupted"), and the airplane-mode cold start was checked manually — login screen appears immediately, cleanup completes on the next launch.

}
state = const LoginState(isUserLoaded: false);
return;
}
Expand Down
45 changes: 42 additions & 3 deletions lib/utils/services/notification_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import 'package:thingsboard_app/utils/services/tb_client_service/i_tb_client_ser
import 'package:thingsboard_app/utils/utils.dart';

class NotificationService {
static const _pushRegisteredKey = 'push_notifications_registered';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The rest of the app funnels persisted keys through DatabaseKeys (lib/constants/database_keys.dart) and reads/writes them behind ILocalDatabaseService, whose implementation is the only place the raw key strings and the as String? casts live (local_database_service.dart:15-46). This adds a fourth persisted key that bypasses both. NotificationsLocalService does own its own key, so there is precedent — but that one at least concentrates its storage access in a dedicated service.

Adding pushNotificationsRegistered to DatabaseKeys plus a small isPushRegistered/setPushRegistered/clearPushRegistered trio on ILocalDatabaseService would keep the convention and, as a bonus, give the test a trivially fakeable seam instead of needing a mocked TbStorage in the global locator.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved in 04f3f07: the key is DatabaseKeys.pushNotificationsRegistered and the storage access sits behind isPushRegistered()/setPushRegistered()/clearPushRegistered() on ILocalDatabaseService. NotificationService resolves it as a final ILocalDatabaseService _localDatabase = getIt(); field, and the tests mock that seam instead of TbStorage.


static FirebaseMessaging _messaging = FirebaseMessaging.instance;
late NotificationDetails _notificationDetails;
final TbLogger _log = getIt();
Expand Down Expand Up @@ -98,9 +100,17 @@ class NotificationService {
getIt<TbLogger>().debug(
'NotificationService::logout() removeMobileSession',
);
_tbClient.getUserControllerApi().removeMobileSession(
xMobileToken: _fcmToken!,
);
try {
await _tbClient.getUserControllerApi().removeMobileSession(
xMobileToken: _fcmToken!,
);
} catch (e) {
// Best effort: the session may already be invalid (e.g. expired JWT).
// Deleting the local FCM token below still stops the notifications.
getIt<TbLogger>().debug(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two small things here. The class already has a _log field (:22), so this could use it rather than adding another inline locator lookup — the surrounding getIt<TbLogger>() calls in this method are pre-existing, but new lines don't have to copy them.

More substantively, debug feels too low for a swallowed remote-call failure. Since the endpoint is @PreAuthorize-guarded server-side, this catch will fire on every expired-session cleanup, so it's the normal path rather than an anomaly — but a removeMobileSession failing for some other reason (endpoint change, API regression) is exactly what you'd want visible in field logs. _log.warn would distinguish it from routine debug noise without crying wolf.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed in 04f3f07: the catch logs via _log.warn, and since the method was being restructured anyway, the pre-existing getIt<TbLogger>() lookups in logout() were switched to the _log field as well. The same warn level is used for the other previously fire-and-forget removeMobileSession failures (token-refresh listener, _resetToken).

'NotificationService::logout() removeMobileSession failed: $e',
);
}
}

await _foregroundMessageSubscription?.cancel();
Expand All @@ -110,6 +120,28 @@ class NotificationService {
await _messaging.setAutoInitEnabled(false);
await flutterLocalNotificationsPlugin.cancelAll();
await _localService.clearNotificationBadgeCount();
await getIt<TbStorage>().deleteItem(_pushRegisteredKey);
}

/// Cleans up the push registration after a session that ended without an
/// explicit logout (e.g. the refresh token expired while the app was
/// closed, #304). The JWT is already invalid at this point, so the
/// server-side mobile session usually can't be removed here; deleting the
/// local FCM token makes further pushes bounce, and the platform purges
/// the session on the next delivery attempt.
Future<void> handleSessionExpired() async {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The name promises more than the call site can actually establish: handleUserLoaded invokes this for any unauthenticated client — a fresh install that never logged in, the instant after an explicit logout, and (given Login.build() runs once per launch) every launch that starts logged out. It's harmless because the flag gates it, but someone reading the call site can't tell whether a session actually expired, and a future caller may reasonably assume the opposite — that it's only safe to call on genuine expiry.

Something like ensurePushUnregistered() or cleanUpStalePushRegistration() would describe what the method actually guarantees — idempotent, safe to call whenever we find ourselves unauthenticated — and make it hard to misuse. The doc comment is good; it's just doing work the name could do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed to cleanUpStalePushRegistration() in 04f3f07; the doc comment now states the actual guarantee — idempotent, safe to call whenever the client is unauthenticated.

final registered =
await getIt<TbStorage>().getItem(_pushRegisteredKey) as String?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NotificationService resolves its collaborators once as fields (_log, _tbClient, _localService); the three new storage accesses instead call getIt<TbStorage>() inline at each use site. That's inconsistent with the class's own injection style, and it's the main reason the test has to populate the global locator before it can even construct the subject. A final TbStorage _storage = getIt(); field alongside the others would match the surrounding code and put the coupling in one place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 04f3f07: the three inline lookups are gone — the storage coupling is a single ILocalDatabaseService field resolved like the class's other collaborators (see the thread on the key constant), which is also what let the tests construct the service against mocks.

if (registered != 'true') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The 'true' string sentinel creates a third state that nothing in this flow can produce but that the read side still has to defend against. TbStorage declares containsKey(String key) and TbSecureStorage implements it against the Hive box (_tb_secure_storage.dart:46-48), so await getIt<TbStorage>().containsKey(_pushRegisteredKey) would work as-is and drop both the stringly-typed comparison and the ambiguous "present but not 'true'" case. The box takes arbitrary values, so there's no reason to stringify a boolean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced in 04f3f07: the read is storage.containsKey(...) (via ILocalDatabaseService.isPushRegistered()) and the stored value is a plain true, so the "present but not 'true'" state no longer exists.

return;
}

_log.debug('NotificationService::handleSessionExpired()');
try {
await logout();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delegating to the full logout() couples the expired-session path to everything logout() will ever do. Today that's fine, but logout() also owns the server-side removeMobileSession call — precisely the part that can't work here, as your doc comment acknowledges — plus stream teardown, cancelAll() and badge clearing. The next person who adds an authenticated call or a navigation side-effect to logout() will silently change the session-expired behaviour, and the comment explaining why won't be anywhere near them.

Extracting the local teardown into a private _cleanupPushRegistration() (token delete, subscription cancels, autoInit off, cancelAll, badge, flag delete) and making logout() read as "remove server session, then _cleanupPushRegistration()" would make the shared part explicit and keep each path's intent obvious. As a bonus it gives the test a real seam instead of a stubbed-out logout() — see the comment on the test file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Split in 04f3f07 as described: _cleanupPushRegistration() holds the local teardown, logout() reads as "remove server session, then cleanup", and cleanUpStalePushRegistration() calls the cleanup directly — the expired path no longer attempts the doomed removeMobileSession at all. The cleanup also nulls _fcmToken after deleteToken(), and a test pins that a repeated logout doesn't reuse the deleted token.

} catch (e) {
_log.debug('NotificationService::handleSessionExpired() failed: $e');
}
}

Future<void> _configFirebaseMessaging() async {
Expand Down Expand Up @@ -198,6 +230,8 @@ class NotificationService {
if (fcmToken != null) {
await _saveToken(fcmToken);
}
} else {
await _markPushRegistered();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The flag is now written from two sites reachable through three paths in this method — _saveToken() at :231 and :237, and this else — and _saveToken() is also reached from the onTokenRefresh listener at :64. Working out which combination of mobileInfo == null / token-age branches ends up marked takes a moment, and it's the kind of thing that quietly goes wrong when a branch is added.

Since every path here that leaves a usable registration ends with a non-null fcmToken, could this collapse to a single await _markPushRegistered() at the end of _getAndSaveToken() (guarded on the token being non-null)? You'd want to keep the call in _saveToken() for the :64 refresh path, so it's about dropping the duplication inside _getAndSaveToken rather than removing a site outright.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Collapsed in 04f3f07 to the shape you suggested: _getAndSaveToken() marks once at the end, guarded on a non-null token, and _saveToken() keeps its call for the refresh-listener path. The write is idempotent, so the overlap on the _saveToken paths is harmless.

}
} else {
await _saveToken(fcmToken);
Expand All @@ -212,6 +246,11 @@ class NotificationService {
(b) => b..fcmTokenTimestamp = DateTime.now().millisecondsSinceEpoch,
),
);
await _markPushRegistered();
}

Future<void> _markPushRegistered() {
return getIt<TbStorage>().setItem(_pushRegisteredKey, 'true');
}

Future<void> showNotification(RemoteMessage message) async {
Expand Down
65 changes: 65 additions & 0 deletions test/utils/services/notification_service_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import 'package:flutter_test/flutter_test.dart';
import 'package:mocktail/mocktail.dart';
import 'package:thingsboard_app/core/logger/tb_logger.dart';
import 'package:thingsboard_app/locator.dart';
import 'package:thingsboard_app/thingsboard_client.dart';
import 'package:thingsboard_app/utils/services/notification_service.dart';
import 'package:thingsboard_app/utils/services/tb_client_service/i_tb_client_service.dart';

class MockTbStorage extends Mock implements TbStorage {}

class MockTbClientService extends Mock implements ITbClientService {}

class MockThingsboardClient extends Mock implements ThingsboardClient {}

class TestableNotificationService extends NotificationService {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overriding the collaborator under test means these tests only cover the flag predicate — the behaviour this PR is actually about (deleting the local FCM token, deleting the flag, tolerating a failed removeMobileSession) is stubbed away, and the manual logoutCalls counter reimplements what mocktail's verify() already does.

As the repo's first test file this also sets the pattern for everything that follows, and "subclass the SUT to neuter its real method" stops working the moment the interesting logic lives inside the overridden method — which is already the case here. If logout() gets split into a server part and a _cleanupPushRegistration() part (see the comment on notification_service.dart:141), these tests could drive the real handleSessionExpired() against a mocked TbStorage/ThingsboardClient and assert the observable effects — verify(() => storage.deleteItem(...)), verify(() => userApi.removeMobileSession(...)) — with no subclass at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rewritten in 04f3f07 with no subclass: FirebaseMessaging, the local-notifications plugin and the badge service are constructor-injectable with prod defaults unchanged (_messaging became a getter over FirebaseMessaging.instance, which also made the reassignment in _requestPermission unnecessary and keeps construction safe before Firebase init). The tests now drive the real cleanUpStalePushRegistration()/logout() and assert the observable effects — verify(() => messaging.deleteToken()), verify(() => localDatabase.clearPushRegistered()), verify(() => userApi.removeMobileSession(...)).

int logoutCalls = 0;

@override
Future<void> logout() async {
logoutCalls++;
}
}

void main() {
late MockTbStorage storage;

setUp(() {
storage = MockTbStorage();
final clientService = MockTbClientService();
when(() => clientService.client).thenReturn(MockThingsboardClient());

getIt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Every future service test in this repo will need this same locator bootstrap, and it's worth noting that two of the three registrations exist purely so the constructor's field initializers don't throw — TbLogger for _log, and ITbClientService with a MockThingsboardClient that's never asserted on, for _tbClient.

Since this is the first test file, pulling this into a shared test/helpers/ function — something like registerTestDependencies({TbStorage? storage}) with a matching getIt.reset() — would stop it being copy-pasted into the next dozen test files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Extracted in 04f3f07 to test/helpers/test_dependencies.dartregisterTestDependencies({tbClient, localDatabase, firebaseService}) plus resetTestDependencies(), shared by both test files. The logger registers as a mock, so constructor field initializers are satisfied without console noise.

..registerLazySingleton(() => TbLogger())
..registerLazySingleton<TbStorage>(() => storage)
..registerLazySingleton<ITbClientService>(() => clientService);
});

tearDown(() => getIt.reset());

group('NotificationService.handleSessionExpired', () {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Coverage gaps worth closing while the file is fresh:

  1. The registered != 'true' branch is only exercised with null — a stored-but-different value, the case the string sentinel actually creates, is untested.
  2. Nothing covers _markPushRegistered(), so neither _saveToken() nor the new fresh-token else in _getAndSaveToken() is verified to persist the flag — that's half the fix.
  3. Nothing covers the flag deletion in logout() or the new removeMobileSession try/catch; both are stubbed out by the subclass.
  4. The login_provider.dart side is untested — neither "unauthenticated triggers cleanup" nor "Firebase not configured skips it", and the latter is where the single-shot path I flagged at login_provider.dart:54 lives.

Since NotificationService is a plain class, one test that exercises the real handleSessionExpired() → cleanup path end to end against a mocked storage and client would cover most of these at once, rather than testing the branch predicate in isolation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mostly closed in 04f3f07: (1) the string sentinel no longer exists — the flag is a bool checked via containsKey; (2) flag persistence is pinned in the LocalDatabaseService tests; (3) logout()'s try/catch and the flag deletion run for real in the tests ("still cleans up when removeMobileSession fails"), plus an interrupted-cleanup-retries case. Still untested: the _getAndSaveToken wiring (needs built_value Response<MobileSessionInfo> stubbing — left out as low value for the fix) and the login_provider side; the single-shot risk there is gone now that main() awaits Firebase init, and the Firebase-not-configured skip is covered at the service level.

test(
'does nothing when push notifications were never registered',
() async {
when(() => storage.getItem(any())).thenAnswer((_) async => null);
final service = TestableNotificationService();

await service.handleSessionExpired();

expect(service.logoutCalls, 0);
},
);

test(
'cleans up the registration when the session expired after a login',
() async {
when(() => storage.getItem(any())).thenAnswer((_) async => 'true');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stubbing with any() means the test passes regardless of which key is read, so a renamed or mistyped _pushRegisteredKey — or a second unrelated storage read added later — would go unnoticed. Matching the literal key (getItem('push_notifications_registered')) and adding a verify on it would make the test actually pin the persisted contract, which is the part most likely to drift.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The flag read moved behind ILocalDatabaseService, so the persisted contract is pinned one level down: local_database_service_test.dart asserts the literal 'push_notifications_registered' on setItem/containsKey/deleteItem against a mocked TbStorage — deliberately the string, not the constant, so a key rename fails the test instead of drifting.

final service = TestableNotificationService();

await service.handleSessionExpired();

expect(service.logoutCalls, 1);
},
);
});
}