Skip to content

Commit 31020b6

Browse files
committed
store: Handle invalid API key on register-queue
This fixes the user reported issue by navigating them back to the choose-account page if the API key is found invalid: https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042 A follow-up would be bringing the user back to the login page (so that the realm url is pre-filled). Signed-off-by: Zixuan James Li <[email protected]>
1 parent 08e4f6a commit 31020b6

14 files changed

+159
-7
lines changed

assets/l10n/app_en.arb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,13 @@
458458
"@topicValidationErrorMandatoryButEmpty": {
459459
"description": "Topic validation error when topic is required but was empty."
460460
},
461+
"errorInvalidApiKeyMessage": "Your account at {url} cannot be authenticated. Please try again or use another account.",
462+
"@errorInvalidApiKeyMessage": {
463+
"description": "Error message in the dialog for invalid API key.",
464+
"placeholders": {
465+
"url": {"type": "String", "example": "http://chat.example.com/"}
466+
}
467+
},
461468
"errorInvalidResponse": "The server sent an invalid response",
462469
"@errorInvalidResponse": {
463470
"description": "Error message when an API call returned an invalid response."

lib/generated/l10n/zulip_localizations.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,6 +721,12 @@ abstract class ZulipLocalizations {
721721
/// **'Topics are required in this organization.'**
722722
String get topicValidationErrorMandatoryButEmpty;
723723

724+
/// Error message in the dialog for invalid API key.
725+
///
726+
/// In en, this message translates to:
727+
/// **'Your account at {url} cannot be authenticated. Please try again or use another account.'**
728+
String errorInvalidApiKeyMessage(String url);
729+
724730
/// Error message when an API call returned an invalid response.
725731
///
726732
/// In en, this message translates to:

lib/generated/l10n/zulip_localizations_ar.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

lib/generated/l10n/zulip_localizations_en.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

lib/generated/l10n/zulip_localizations_fr.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

lib/generated/l10n/zulip_localizations_ja.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

lib/generated/l10n/zulip_localizations_pl.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';
362367

lib/generated/l10n/zulip_localizations_ru.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
357357
@override
358358
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
359359

360+
@override
361+
String errorInvalidApiKeyMessage(String url) {
362+
return 'Your account at $url cannot be authenticated. Please try again or use another account.';
363+
}
364+
360365
@override
361366
String get errorInvalidResponse => 'The server sent an invalid response';
362367

lib/model/store.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -912,7 +912,12 @@ class UpdateMachine {
912912
// at 1 kiB (at least on Android), and stack can be longer than that.
913913
assert(debugLog('Stack:\n$s'));
914914
assert(debugLog('Backing off, then will retry…'));
915-
// TODO tell user if initial-fetch errors persist, or look non-transient
915+
// TODO(#890): tell user if initial-fetch errors persist, or look non-transient
916+
switch (e) {
917+
case ZulipApiException(code: 'INVALID_API_KEY'):
918+
// See also: [_PerAccountStoreWidgetState.didChangeDependencies]
919+
rethrow;
920+
}
916921
await (backoffMachine ??= BackoffMachine()).wait();
917922
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
918923
}

lib/widgets/app.dart

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,14 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
233233
class ChooseAccountPage extends StatelessWidget {
234234
const ChooseAccountPage({super.key});
235235

236+
/// Navigate to [ChooseAccountPage], ensuring that its route is at the root level.
237+
static void navigate(BuildContext context) {
238+
final navigator = Navigator.of(context);
239+
navigator.popUntil((route) => route.isFirst);
240+
unawaited(navigator.pushReplacement(
241+
MaterialWidgetRoute(page: const ChooseAccountPage())));
242+
}
243+
236244
Widget _buildAccountItem(
237245
BuildContext context, {
238246
required int accountId,

lib/widgets/store.dart

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
1+
import 'dart:async';
2+
13
import 'package:flutter/material.dart';
24
import 'package:flutter/scheduler.dart';
35

6+
import '../api/exception.dart';
7+
import '../generated/l10n/zulip_localizations.dart';
8+
import '../log.dart';
49
import '../model/binding.dart';
510
import '../model/store.dart';
11+
import 'actions.dart';
12+
import 'app.dart';
613
import 'page.dart';
714

815
/// Provides access to the app's data.
@@ -228,7 +235,16 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
228235
_setStore(null);
229236
if (widget.routeToRemoveOnLogout != null) {
230237
SchedulerBinding.instance.addPostFrameCallback(
231-
(_) => Navigator.of(context).removeRoute(widget.routeToRemoveOnLogout!));
238+
(_) {
239+
final navigator = Navigator.of(context);
240+
navigator.removeRoute(widget.routeToRemoveOnLogout!);
241+
if (!navigator.canPop()) {
242+
// This ensures that the navigator stack is non-empty after the
243+
// removal of the route.
244+
unawaited(navigator.push(
245+
MaterialWidgetRoute(page: const ChooseAccountPage())));
246+
}
247+
});
232248
}
233249
return;
234250
}
@@ -245,11 +261,28 @@ class _PerAccountStoreWidgetState extends State<PerAccountStoreWidget> {
245261
// [didChangeDependencies] will run again, this time in the
246262
// `store != null` case above.
247263
await globalStore.perAccount(widget.accountId);
248-
} on AccountNotFoundException {
249-
// The account was logged out while its store was loading.
250-
// This widget will be showing [placeholder] perpetually,
251-
// but that's OK as long as other code will be removing it from the UI
252-
// (usually by using [routeToRemoveOnLogout]).
264+
} catch (e) {
265+
switch (e) {
266+
case AccountNotFoundException():
267+
// The account was logged out while its store was loading.
268+
// This widget will be showing [placeholder] perpetually,
269+
// but that's OK as long as other code will be removing it from the UI
270+
// (usually by using [routeToRemoveOnLogout]).
271+
return;
272+
case ZulipApiException(code: 'INVALID_API_KEY'):
273+
// The API key is invalid and the store can never be loaded
274+
// unless the user retries manually.
275+
if (!mounted) return;
276+
final zulipLocalizations = ZulipLocalizations.of(context);
277+
reportErrorToUserModally(
278+
zulipLocalizations.errorCouldNotConnectTitle,
279+
details: zulipLocalizations.errorInvalidApiKeyMessage(
280+
globalStore.getAccount(widget.accountId)!.realmUrl.toString()));
281+
unawaited(logOutAccount(context, widget.accountId));
282+
return;
283+
default:
284+
rethrow;
285+
}
253286
}
254287
}();
255288
}

test/model/store_test.dart

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:flutter/foundation.dart';
77
import 'package:http/http.dart' as http;
88
import 'package:test/scaffolding.dart';
99
import 'package:zulip/api/core.dart';
10+
import 'package:zulip/api/exception.dart';
1011
import 'package:zulip/api/model/events.dart';
1112
import 'package:zulip/api/model/model.dart';
1213
import 'package:zulip/api/route/events.dart';
@@ -500,6 +501,19 @@ void main() {
500501
users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
501502
}));
502503

504+
test('does not retry registerQueue on invalid API key', () => awaitFakeAsync((async) async {
505+
await prepareStore();
506+
507+
// Try to load, but the API key is told to be invalid.
508+
globalStore.useCachedApiConnections = true;
509+
connection.prepare(httpStatus: 400, json: {
510+
'result': 'error', 'code': 'INVALID_API_KEY',
511+
'msg': 'Invalid API key',
512+
});
513+
await check(UpdateMachine.load(globalStore, eg.selfAccount.id))
514+
.throws<ZulipApiException>();
515+
}));
516+
503517
// TODO test UpdateMachine.load starts polling loop
504518
// TODO test UpdateMachine.load calls registerNotificationToken
505519
});

test/model/test_store.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ class TestGlobalStore extends GlobalStore {
126126

127127
static const Duration removeAccountDuration = Duration(milliseconds: 1);
128128
Duration? loadPerAccountDuration;
129+
Object? loadPerAccountException;
129130

130131
/// Consume the log of calls made to [doRemoveAccount].
131132
List<int> takeDoRemoveAccountCalls() {
@@ -147,6 +148,9 @@ class TestGlobalStore extends GlobalStore {
147148
if (loadPerAccountDuration != null) {
148149
await Future<void>.delayed(loadPerAccountDuration!);
149150
}
151+
if (loadPerAccountException != null) {
152+
throw loadPerAccountException!;
153+
}
150154
final initialSnapshot = _initialSnapshots[accountId]!;
151155
final store = PerAccountStore.fromInitialSnapshot(
152156
globalStore: this,

test/widgets/store_test.dart

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,19 @@
11
import 'package:checks/checks.dart';
22
import 'package:flutter/material.dart';
3+
import 'package:flutter_checks/flutter_checks.dart';
34
import 'package:flutter_test/flutter_test.dart';
5+
import 'package:zulip/api/exception.dart';
46
import 'package:zulip/model/store.dart';
7+
import 'package:zulip/widgets/app.dart';
8+
import 'package:zulip/widgets/home.dart';
59
import 'package:zulip/widgets/store.dart';
610

711
import '../flutter_checks.dart';
812
import '../model/binding.dart';
913
import '../example_data.dart' as eg;
1014
import '../model/store_checks.dart';
15+
import '../model/test_store.dart';
16+
import 'dialog_checks.dart';
1117

1218
/// A widget whose state uses [PerAccountStoreAwareStateMixin].
1319
class MyWidgetWithMixin extends StatefulWidget {
@@ -113,6 +119,45 @@ void main() {
113119
.contains('consider MaterialAccountWidgetRoute');
114120
});
115121

122+
testWidgets('PerAccountStoreWidget reset to choose-account page'
123+
' when logging out on invalid API key', (tester) async {
124+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
125+
addTearDown(testBinding.reset);
126+
127+
const loadPerAccountDuration = Duration(seconds: 30);
128+
assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod);
129+
testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration;
130+
testBinding.globalStore.loadPerAccountException = ZulipApiException(
131+
routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400,
132+
data: {}, message: '');
133+
await tester.pumpWidget(const ZulipApp());
134+
await tester.pump(); // start to load account
135+
check(find.byType(CircularProgressIndicator)).findsOne();
136+
check(find.byType(ChooseAccountPage)).findsNothing();
137+
check(find.byType(BackButton)).findsNothing();
138+
139+
await tester.pump(kTryAnotherAccountWaitPeriod);
140+
await tester.tap(find.text('Try another account'));
141+
await tester.pump(); // tap the button
142+
await tester.pump(const Duration(milliseconds: 250)); // wait for animation
143+
check(find.byType(CircularProgressIndicator)).findsOne();
144+
check(find.byType(ChooseAccountPage)).findsOne();
145+
check(find.byType(BackButton)).findsOne();
146+
147+
await tester.pump(loadPerAccountDuration);
148+
await tester.pump(TestGlobalStore.removeAccountDuration);
149+
await tester.pump(const Duration(milliseconds: 250)); // wait for animation
150+
check(find.byType(CircularProgressIndicator)).findsNothing();
151+
check(find.byType(ChooseAccountPage)).findsOne();
152+
// Choose-account page's route should be at the root level.
153+
check(find.byType(BackButton)).findsNothing();
154+
checkErrorDialog(tester,
155+
expectedTitle: 'Could not connect',
156+
expectedMessage:
157+
'Your account at https://chat.example/ cannot be authenticated.'
158+
' Please try again or use another account.');
159+
});
160+
116161
testWidgets('PerAccountStoreWidget immediate data after first loaded', (tester) async {
117162
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
118163
addTearDown(testBinding.reset);

0 commit comments

Comments
 (0)