From 0344686c73d6324791f7251704af0adc273fa405 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 8 Nov 2024 20:29:53 -0800 Subject: [PATCH] login: Support logging out of an account Fixes: #463 --- assets/l10n/app_en.arb | 16 +++ lib/widgets/actions.dart | 37 +++++ lib/widgets/app.dart | 36 +++++ lib/widgets/page.dart | 1 + lib/widgets/store.dart | 24 +++- test/model/test_store.dart | 18 ++- test/widgets/actions_test.dart | 237 ++++++++++++++++++++++++++++++++- test/widgets/app_test.dart | 36 +++++ 8 files changed, 399 insertions(+), 6 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index aea0c83d38..c14d06fa22 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -19,6 +19,22 @@ "@chooseAccountPageTitle": { "description": "Title for ChooseAccountPage" }, + "chooseAccountPageLogOutButton": "Log out", + "@chooseAccountPageLogOutButton": { + "description": "Label for the 'Log out' button for an account on the choose-account page" + }, + "logOutConfirmationDialogTitle": "Log out?", + "@logOutConfirmationDialogTitle": { + "description": "Title for a confirmation dialog for logging out." + }, + "logOutConfirmationDialogMessage": "To use this account in the future, you will have to re-enter the URL for your organization and your account information.", + "@logOutConfirmationDialogMessage": { + "description": "Message for a confirmation dialog for logging out." + }, + "logOutConfirmationDialogConfirmButton": "Log out", + "@logOutConfirmationDialogConfirmButton": { + "description": "Label for the 'Log out' button on a confirmation dialog for logging out." + }, "chooseAccountButtonAddAnAccount": "Add an account", "@chooseAccountButtonAddAnAccount": { "description": "Label for ChooseAccountPage button to add an account" diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index 934e04934e..c9b7fdcb48 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -6,6 +6,8 @@ /// in order to present success or error feedback to the user through the UI. library; +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; @@ -13,9 +15,44 @@ import '../api/model/model.dart'; import '../api/model/narrow.dart'; import '../api/route/messages.dart'; import '../model/narrow.dart'; +import '../model/store.dart'; +import '../notifications/receive.dart'; import 'dialog.dart'; import 'store.dart'; +Future logOutAccount(BuildContext context, int accountId) async { + final globalStore = GlobalStoreWidget.of(context); + + final account = globalStore.getAccount(accountId); + if (account == null) return; // TODO(log) + + // Unawaited, to not block removing the account on this request. + unawaited(unregisterToken(globalStore, accountId)); + + await globalStore.removeAccount(accountId); +} + +Future unregisterToken(GlobalStore globalStore, int accountId) async { + final account = globalStore.getAccount(accountId); + if (account == null) return; // TODO(log) + + // TODO(#322) use actual acked push token; until #322, this is just null. + final token = account.ackedPushToken + // Try the current token as a fallback; maybe the server has registered + // it and we just haven't recorded that fact in the client. + ?? NotificationService.instance.token.value; + if (token == null) return; + + final connection = globalStore.apiConnectionFromAccount(account); + try { + await NotificationService.unregisterToken(connection, token: token); + } catch (e) { + // TODO retry? handle failures? + } finally { + connection.close(); + } +} + Future markNarrowAsRead(BuildContext context, Narrow narrow) async { final store = PerAccountStoreWidget.of(context); final connection = store.connection; diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 7283311b37..d0944d663b 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -8,8 +8,10 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import '../log.dart'; import '../model/localizations.dart'; import '../model/narrow.dart'; +import '../model/store.dart'; import '../notifications/display.dart'; import 'about_zulip.dart'; +import 'actions.dart'; import 'app_bar.dart'; import 'dialog.dart'; import 'inbox.dart'; @@ -232,11 +234,45 @@ class ChooseAccountPage extends StatelessWidget { required Widget title, Widget? subtitle, }) { + final designVariables = DesignVariables.of(context); + final zulipLocalizations = ZulipLocalizations.of(context); + final materialLocalizations = MaterialLocalizations.of(context); return Card( clipBehavior: Clip.hardEdge, child: ListTile( title: title, subtitle: subtitle, + trailing: MenuAnchor( + menuChildren: [ + MenuItemButton( + onPressed: () { + showSuggestedActionDialog(context: context, + title: zulipLocalizations.logOutConfirmationDialogTitle, + message: zulipLocalizations.logOutConfirmationDialogMessage, + // TODO(#1032) "destructive" style for action button + actionButtonText: zulipLocalizations.logOutConfirmationDialogConfirmButton, + onActionButtonPress: () { + // TODO error handling if db write fails? + logOutAccount(context, accountId); + }); + }, + child: Text(zulipLocalizations.chooseAccountPageLogOutButton)), + ], + builder: (BuildContext context, MenuController controller, Widget? child) { + return IconButton( + tooltip: materialLocalizations.showMenuTooltip, // "Show menu" + onPressed: () { + if (controller.isOpen) { + controller.close(); + } else { + controller.open(); + } + }, + icon: Icon(Icons.adaptive.more, color: designVariables.icon)); + }), + // The default trailing padding with M3 is 24px. Decrease by 12 because + // IconButton (the "…" button) comes with 12px padding on all sides. + contentPadding: const EdgeInsetsDirectional.only(start: 16, end: 12), onTap: () => Navigator.push(context, HomePage.buildRoute(accountId: accountId)))); } diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index a039769d4f..2008c627a1 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -40,6 +40,7 @@ mixin AccountPageRouteMixin on PageRoute { return PerAccountStoreWidget( accountId: accountId, placeholder: const LoadingPlaceholderPage(), + routeToRemoveOnLogout: this, child: super.buildPage(context, animation, secondaryAnimation)); } } diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index 12100efd03..1d37bcfd17 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -1,7 +1,9 @@ import 'package:flutter/material.dart'; +import 'package:flutter/scheduler.dart'; import '../model/binding.dart'; import '../model/store.dart'; +import 'page.dart'; /// Provides access to the app's data. /// @@ -112,11 +114,19 @@ class PerAccountStoreWidget extends StatefulWidget { super.key, required this.accountId, this.placeholder = const LoadingPlaceholder(), + this.routeToRemoveOnLogout, required this.child, }); final int accountId; final Widget placeholder; + + /// A per-account [Route] that should be removed on logout. + /// + /// Use this when the widget is a page on a route that should go away + /// when the account is logged out, instead of lingering with [placeholder]. + final AccountPageRouteMixin? routeToRemoveOnLogout; + final Widget child; /// The user's data for the relevant Zulip account for this widget. @@ -195,6 +205,16 @@ class _PerAccountStoreWidgetState extends State { void didChangeDependencies() { super.didChangeDependencies(); final globalStore = GlobalStoreWidget.of(context); + final accountExists = globalStore.getAccount(widget.accountId) != null; + if (!accountExists) { + // logged out + _setStore(null); + if (widget.routeToRemoveOnLogout != null) { + SchedulerBinding.instance.addPostFrameCallback( + (_) => Navigator.of(context).removeRoute(widget.routeToRemoveOnLogout!)); + } + return; + } // If we already have data, get it immediately. This avoids showing one // frame of loading indicator each time we have a new PerAccountStoreWidget. final store = globalStore.perAccountSync(widget.accountId); @@ -212,13 +232,13 @@ class _PerAccountStoreWidgetState extends State { // The account was logged out while its store was loading. // This widget will be showing [placeholder] perpetually, // but that's OK as long as other code will be removing it from the UI - // (for example by removing a per-account route from the nav). + // (usually by using [routeToRemoveOnLogout]). } }(); } } - void _setStore(PerAccountStore store) { + void _setStore(PerAccountStore? store) { if (store != this.store) { setState(() { this.store = store; diff --git a/test/model/test_store.dart b/test/model/test_store.dart index ad6d7a729b..964c22669f 100644 --- a/test/model/test_store.dart +++ b/test/model/test_store.dart @@ -47,6 +47,10 @@ class TestGlobalStore extends GlobalStore { /// but that breach is sometimes convenient for tests. bool useCachedApiConnections = false; + void clearCachedApiConnections() { + _apiConnections.clear(); + } + /// Get or construct a [FakeApiConnection] with the given arguments. /// /// To access the same [FakeApiConnection] that the code under test will get, @@ -120,9 +124,21 @@ class TestGlobalStore extends GlobalStore { // Nothing to do. } + static const Duration removeAccountDuration = Duration(milliseconds: 1); + + /// Consume the log of calls made to [doRemoveAccount]. + List takeDoRemoveAccountCalls() { + final result = _doRemoveAccountCalls; + _doRemoveAccountCalls = null; + return result ?? []; + } + List? _doRemoveAccountCalls; + @override Future doRemoveAccount(int accountId) async { - // Nothing to do. + (_doRemoveAccountCalls ??= []).add(accountId); + await Future.delayed(removeAccountDuration); + // Nothing else to do. } @override diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart index 68d192b903..8b4a355b3e 100644 --- a/test/widgets/actions_test.dart +++ b/test/widgets/actions_test.dart @@ -1,9 +1,13 @@ +import 'dart:async'; import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; @@ -11,13 +15,20 @@ import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; +import 'package:zulip/notifications/receive.dart'; import 'package:zulip/widgets/actions.dart'; +import 'package:zulip/widgets/app.dart'; +import 'package:zulip/widgets/inbox.dart'; +import 'package:zulip/widgets/page.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; +import '../model/store_checks.dart'; +import '../model/test_store.dart'; import '../model/unreads_checks.dart'; import '../stdlib_checks.dart'; +import '../test_navigation.dart'; import 'dialog_checks.dart'; import 'test_app.dart'; @@ -30,19 +41,239 @@ void main() { Future prepare(WidgetTester tester, { UnreadMessagesSnapshot? unreadMsgs, + String? ackedPushToken = '123', }) async { addTearDown(testBinding.reset); - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( + final selfAccount = eg.selfAccount.copyWith(ackedPushToken: Value(ackedPushToken)); + await testBinding.globalStore.add(selfAccount, eg.initialSnapshot( unreadMsgs: unreadMsgs)); - store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + store = await testBinding.globalStore.perAccount(selfAccount.id); connection = store.connection as FakeApiConnection; - await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, + await tester.pumpWidget(TestZulipApp(accountId: selfAccount.id, child: const Scaffold(body: Placeholder()))); await tester.pump(); context = tester.element(find.byType(Placeholder)); } + /// Creates and caches a new [FakeApiConnection] in [TestGlobalStore]. + /// + /// In live code, [unregisterToken] makes a new [ApiConnection] for the + /// unregister-token request instead of reusing the store's connection. + /// To enable callers to prepare responses for that request, this function + /// creates a new [FakeApiConnection] and caches it in [TestGlobalStore] + /// for [unregisterToken] to pick up. + /// + /// Call this instead of just turning on + /// [TestGlobalStore.useCachedApiConnections] so that [unregisterToken] + /// doesn't try to call `close` twice on the same connection instance, + /// which isn't allowed. (Once by the unregister-token code + /// and once as part of removing the account.) + FakeApiConnection separateConnection() { + testBinding.globalStore + ..clearCachedApiConnections() + ..useCachedApiConnections = true; + return testBinding.globalStore + .apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection; + } + + String unregisterApiPathForPlatform(TargetPlatform platform) { + return switch (platform) { + TargetPlatform.android => '/api/v1/users/me/android_gcm_reg_id', + TargetPlatform.iOS => '/api/v1/users/me/apns_device_token', + _ => throw Error(), + }; + } + + void checkSingleUnregisterRequest( + FakeApiConnection connection, { + String? expectedToken, + }) { + final subject = check(connection.takeRequests()).single.isA() + ..method.equals('DELETE') + ..url.path.equals(unregisterApiPathForPlatform(defaultTargetPlatform)); + if (expectedToken != null) { + subject.bodyFields.deepEquals({'token': expectedToken}); + } + } + + group('logOutAccount', () { + testWidgets('smoke', (tester) async { + await prepare(tester); + check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); + const unregisterDelay = Duration(seconds: 5); + assert(unregisterDelay > TestGlobalStore.removeAccountDuration); + final newConnection = separateConnection() + ..prepare(delay: unregisterDelay, json: {'msg': '', 'result': 'success'}); + + final future = logOutAccount(context, eg.selfAccount.id); + // Unregister-token request and account removal dispatched together + checkSingleUnregisterRequest(newConnection); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + // Account removal not blocked on unregister-token response + check(testBinding.globalStore).accountIds.isEmpty(); + check(connection.isOpen).isFalse(); + check(newConnection.isOpen).isTrue(); // still busy with unregister-token + + await tester.pump(unregisterDelay - TestGlobalStore.removeAccountDuration); + check(newConnection.isOpen).isFalse(); + }); + + testWidgets('unregister request has an error', (tester) async { + await prepare(tester); + check(testBinding.globalStore).accountIds.single.equals(eg.selfAccount.id); + const unregisterDelay = Duration(seconds: 5); + assert(unregisterDelay > TestGlobalStore.removeAccountDuration); + final exception = ZulipApiException( + httpStatus: 401, + code: 'UNAUTHORIZED', + data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + routeName: 'removeEtcEtcToken', + message: 'Invalid API key', + ); + final newConnection = separateConnection() + ..prepare(delay: unregisterDelay, exception: exception); + + final future = logOutAccount(context, eg.selfAccount.id); + // Unregister-token request and account removal dispatched together + checkSingleUnregisterRequest(newConnection); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + // Account removal not blocked on unregister-token response + check(testBinding.globalStore).accountIds.isEmpty(); + check(connection.isOpen).isFalse(); + check(newConnection.isOpen).isTrue(); // for the unregister-token request + + await tester.pump(unregisterDelay - TestGlobalStore.removeAccountDuration); + check(newConnection.isOpen).isFalse(); + }); + + testWidgets("logged-out account's routes removed from nav; other accounts' remain", (tester) async { + Future makeUnreadTopicInInbox(int accountId, String topic) async { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream, topic: topic); + final store = await testBinding.globalStore.perAccount(accountId); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await store.addMessage(message); + await tester.pump(); + } + + addTearDown(testBinding.reset); + + final account1 = eg.account(id: 1, user: eg.user()); + final account2 = eg.account(id: 2, user: eg.user()); + await testBinding.globalStore.add(account1, eg.initialSnapshot()); + await testBinding.globalStore.add(account2, eg.initialSnapshot()); + + final testNavObserver = TestNavigatorObserver(); + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); + final navigator = await ZulipApp.navigator; + navigator.popUntil((_) => false); // clear starting routes + await tester.pumpAndSettle(); + + final pushedRoutes = >[]; + testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); + final account1Route = InboxPage.buildRoute(accountId: account1.id); + final account2Route = InboxPage.buildRoute(accountId: account2.id); + unawaited(navigator.push(account1Route)); + unawaited(navigator.push(account2Route)); + await tester.pumpAndSettle(); + check(pushedRoutes).deepEquals([account1Route, account2Route]); + + await makeUnreadTopicInInbox(account1.id, 'topic in account1'); + final findAccount1PageContent = find.text('topic in account1', skipOffstage: false); + + await makeUnreadTopicInInbox(account2.id, 'topic in account2'); + final findAccount2PageContent = find.text('topic in account2', skipOffstage: false); + + final findLoadingPage = find.byType(LoadingPlaceholderPage, skipOffstage: false); + + check(findAccount1PageContent).findsOne(); + check(findLoadingPage).findsNothing(); + + final removedRoutes = >[]; + testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + + final context = tester.element(find.byType(MaterialApp)); + final future = logOutAccount(context, account1.id); + await tester.pump(TestGlobalStore.removeAccountDuration); + await future; + check(removedRoutes).single.identicalTo(account1Route); + check(findAccount1PageContent).findsNothing(); + check(findLoadingPage).findsOne(); + + await tester.pump(); + check(findAccount1PageContent).findsNothing(); + check(findLoadingPage).findsNothing(); + check(findAccount2PageContent).findsOne(); + }); + }); + + group('unregisterToken', () { + testWidgets('smoke, happy path', (tester) async { + await prepare(tester, ackedPushToken: '123'); + + final newConnection = separateConnection() + ..prepare(json: {'msg': '', 'result': 'success'}); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: '123'); + check(newConnection.isOpen).isFalse(); + }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + + testWidgets('fallback to current token if acked is missing', (tester) async { + await prepare(tester, ackedPushToken: null); + NotificationService.instance.token = ValueNotifier('asdf'); + + final newConnection = separateConnection() + ..prepare(json: {'msg': '', 'result': 'success'}); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: 'asdf'); + check(newConnection.isOpen).isFalse(); + }); + + testWidgets('no error if acked token and current token both missing', (tester) async { + await prepare(tester, ackedPushToken: null); + NotificationService.instance.token = ValueNotifier(null); + + final newConnection = separateConnection(); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + await tester.pumpAndSettle(); + await future; + check(newConnection.takeRequests()).isEmpty(); + }); + + testWidgets('connection closed if request errors', (tester) async { + await prepare(tester, ackedPushToken: '123'); + + final newConnection = separateConnection() + ..prepare(exception: ZulipApiException( + httpStatus: 401, + code: 'UNAUTHORIZED', + data: {"result": "error", "msg": "Invalid API key", "code": "UNAUTHORIZED"}, + routeName: 'removeEtcEtcToken', + message: 'Invalid API key', + )); + final future = unregisterToken(testBinding.globalStore, eg.selfAccount.id); + await tester.pump(Duration.zero); + await future; + checkSingleUnregisterRequest(newConnection, expectedToken: '123'); + check(newConnection.isOpen).isFalse(); + }); + }); + group('markNarrowAsRead', () { testWidgets('smoke test on modern server', (tester) async { final narrow = TopicNarrow.ofMessage(eg.streamMessage()); diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index 683298f017..c21ac1915e 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -10,6 +10,8 @@ import 'package:zulip/widgets/page.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; +import '../model/store_checks.dart'; +import '../model/test_store.dart'; import '../test_navigation.dart'; import 'dialog_checks.dart'; import 'page_checks.dart'; @@ -158,6 +160,40 @@ void main() { ..top.isGreaterThan(1 / 3 * screenHeight) ..bottom.isLessThan(2 / 3 * screenHeight); }); + + group('log out', () { + Future<(Widget, Widget)> prepare(WidgetTester tester, {required Account account}) async { + await setupChooseAccountPage(tester, accounts: [account]); + + final findThreeDotsButton = find.descendant( + of: find.widgetWithText(Card, eg.selfAccount.realmUrl.toString()), + matching: find.byIcon(Icons.adaptive.more)); + + await tester.tap(findThreeDotsButton); + await tester.pump(); + await tester.tap(find.descendant( + of: find.byType(MenuItemButton), matching: find.text('Log out'))); + await tester.pumpAndSettle(); // TODO just `pump`? But the dialog doesn't appear. + return checkSuggestedActionDialog(tester, + expectedTitle: 'Log out?', + expectedMessage: 'To use this account in the future, you will have to re-enter the URL for your organization and your account information.', + expectedActionButtonText: 'Log out'); + } + + testWidgets('user confirms logging out', (tester) async { + final (actionButton, _) = await prepare(tester, account: eg.selfAccount); + await tester.tap(find.byWidget(actionButton)); + await tester.pump(TestGlobalStore.removeAccountDuration); + check(testBinding.globalStore).accounts.isEmpty(); + }); + + testWidgets('user cancels logging out', (tester) async { + final (_, cancelButton) = await prepare(tester, account: eg.selfAccount); + await tester.tap(find.byWidget(cancelButton)); + await tester.pumpAndSettle(); + check(testBinding.globalStore).accounts.deepEquals([eg.selfAccount]); + }); + }); }); group('scaffoldMessenger', () {