Skip to content

Commit 32939d7

Browse files
committed
store: Handle invalid API key on register-queue
The method loadPerAccount has two call sites, i.e. places where we send register-queue requests: 1. _reloadPerAccount through [UpdateMachine._handlePollError] (e.g.: expired event queue) 2. perAccount through [PerAccountStoreWidget] (e.g.: loading for the first time) Both sites already expect [AccountNotFoundException] by assuming that the `loadPerAccount` fail is irrecoverable and is handled elsewhere. This partly addresses #890 by handling authentication errors for register-queue. Fixes: #737 Signed-off-by: Zixuan James Li <[email protected]>
1 parent dab106a commit 32939d7

13 files changed

+207
-7
lines changed

assets/l10n/app_en.arb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,13 @@
523523
"@topicValidationErrorMandatoryButEmpty": {
524524
"description": "Topic validation error when topic is required but was empty."
525525
},
526+
"errorInvalidApiKeyMessage": "Your account at {url} could not be authenticated. Please try logging in again or use another account.",
527+
"@errorInvalidApiKeyMessage": {
528+
"description": "Error message in the dialog for invalid API key.",
529+
"placeholders": {
530+
"url": {"type": "String", "example": "http://chat.example.com/"}
531+
}
532+
},
526533
"errorInvalidResponse": "The server sent an invalid response",
527534
"@errorInvalidResponse": {
528535
"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
@@ -801,6 +801,12 @@ abstract class ZulipLocalizations {
801801
/// **'Topics are required in this organization.'**
802802
String get topicValidationErrorMandatoryButEmpty;
803803

804+
/// Error message in the dialog for invalid API key.
805+
///
806+
/// In en, this message translates to:
807+
/// **'Your account at {url} could not be authenticated. Please try logging in again or use another account.'**
808+
String errorInvalidApiKeyMessage(String url);
809+
804810
/// Error message when an API call returned an invalid response.
805811
///
806812
/// 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
@@ -404,6 +404,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
404404
@override
405405
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
406406

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'The server sent an invalid response';
409414

lib/generated/l10n/zulip_localizations_en.dart

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

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'The server sent an invalid response';
409414

lib/generated/l10n/zulip_localizations_ja.dart

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

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'The server sent an invalid response';
409414

lib/generated/l10n/zulip_localizations_nb.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,11 @@ class ZulipLocalizationsNb extends ZulipLocalizations {
404404
@override
405405
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
406406

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'The server sent an invalid response';
409414

lib/generated/l10n/zulip_localizations_pl.dart

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

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';
409414

lib/generated/l10n/zulip_localizations_ru.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
404404
@override
405405
String get topicValidationErrorMandatoryButEmpty => 'Темы обязательны в этой организации.';
406406

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'Получен недопустимый ответ сервера';
409414

lib/generated/l10n/zulip_localizations_sk.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,11 @@ class ZulipLocalizationsSk extends ZulipLocalizations {
404404
@override
405405
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';
406406

407+
@override
408+
String errorInvalidApiKeyMessage(String url) {
409+
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
410+
}
411+
407412
@override
408413
String get errorInvalidResponse => 'Server poslal nesprávnu odpoveď';
409414

lib/model/store.dart

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import '../api/backoff.dart';
1919
import '../api/route/realm.dart';
2020
import '../log.dart';
2121
import '../notifications/receive.dart';
22+
import 'actions.dart';
2223
import 'autocomplete.dart';
2324
import 'database.dart';
2425
import 'emoji.dart';
@@ -149,7 +150,34 @@ abstract class GlobalStore extends ChangeNotifier {
149150
/// and/or [perAccountSync].
150151
Future<PerAccountStore> loadPerAccount(int accountId) async {
151152
assert(_accounts.containsKey(accountId));
152-
final store = await doLoadPerAccount(accountId);
153+
final PerAccountStore store;
154+
try {
155+
store = await doLoadPerAccount(accountId);
156+
} catch (e) {
157+
switch (e) {
158+
case HttpException(httpStatus: 401):
159+
// The API key is invalid and the store can never be loaded
160+
// unless the user retries manually.
161+
final account = getAccount(accountId);
162+
if (account == null) {
163+
// The account was logged out during `await doLoadPerAccount`.
164+
// Here, that seems possible only by the user's own action;
165+
// the logout can't have been done programmatically.
166+
// Even if it were, it would have come with its own UI feedback.
167+
// Anyway, skip showing feedback, to not be confusing or repetitive.
168+
throw AccountNotFoundException();
169+
}
170+
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
171+
reportErrorToUserModally(
172+
zulipLocalizations.errorCouldNotConnectTitle,
173+
message: zulipLocalizations.errorInvalidApiKeyMessage(
174+
account.realmUrl.toString()));
175+
await logOutAccount(this, accountId);
176+
throw AccountNotFoundException();
177+
default:
178+
rethrow;
179+
}
180+
}
153181
if (!_accounts.containsKey(accountId)) {
154182
// TODO(#1354): handle this earlier
155183
// [removeAccount] was called during [doLoadPerAccount].
@@ -914,12 +942,19 @@ class UpdateMachine {
914942
try {
915943
return await registerQueue(connection);
916944
} catch (e, s) {
917-
assert(debugLog('Error fetching initial snapshot: $e'));
918-
// Print stack trace in its own log entry; log entries are truncated
919-
// at 1 kiB (at least on Android), and stack can be longer than that.
920-
assert(debugLog('Stack:\n$s'));
921-
assert(debugLog('Backing off, then will retry…'));
922945
// TODO(#890): tell user if initial-fetch errors persist, or look non-transient
946+
switch (e) {
947+
case HttpException(httpStatus: 401):
948+
// We cannot recover from this error through retrying.
949+
// Leave it to [GlobalStore.loadPerAccount].
950+
rethrow;
951+
default:
952+
assert(debugLog('Error fetching initial snapshot: $e'));
953+
// Print stack trace in its own log entry; log entries are truncated
954+
// at 1 kiB (at least on Android), and stack can be longer than that.
955+
assert(debugLog('Stack:\n$s'));
956+
}
957+
assert(debugLog('Backing off, then will retry…'));
923958
await (backoffMachine ??= BackoffMachine()).wait();
924959
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
925960
}

test/model/store_test.dart

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,29 @@ void main() {
121121
check(completers(1)).length.equals(1);
122122
});
123123

124+
test('GlobalStore.perAccount loading fails with HTTP status code 401', () => awaitFakeAsync((async) async {
125+
final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
126+
final future = globalStore.perAccount(eg.selfAccount.id);
127+
128+
globalStore.completers[eg.selfAccount.id]!
129+
.single.completeError(eg.apiExceptionUnauthorized());
130+
await check(future).throws<AccountNotFoundException>();
131+
}));
132+
133+
test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async {
134+
final globalStore = LoadingTestGlobalStore(accounts: [eg.selfAccount]);
135+
final future = globalStore.perAccount(eg.selfAccount.id);
136+
137+
await logOutAccount(globalStore, eg.selfAccount.id);
138+
check(globalStore.takeDoRemoveAccountCalls())
139+
.single.equals(eg.selfAccount.id);
140+
141+
globalStore.completers[eg.selfAccount.id]!
142+
.single.completeError(eg.apiExceptionUnauthorized());
143+
await check(future).throws<AccountNotFoundException>();
144+
check(globalStore.takeDoRemoveAccountCalls()).isEmpty();
145+
}));
146+
124147
// TODO test insertAccount
125148

126149
group('GlobalStore.updateAccount', () {
@@ -997,7 +1020,7 @@ void main() {
9971020
}
9981021

9991022
void checkReloadFailure({
1000-
required Future<void> Function() completeLoading,
1023+
required FutureOr<void> Function() completeLoading,
10011024
}) {
10021025
awaitFakeAsync((async) async {
10031026
await prepareReload(async);
@@ -1027,6 +1050,14 @@ void main() {
10271050
test('user logged out before new store is loaded', () => awaitFakeAsync((async) async {
10281051
checkReloadFailure(completeLoading: logOutAndCompleteWithNewStore);
10291052
}));
1053+
1054+
void completeWithApiExceptionUnauthorized() {
1055+
completers().single.completeError(eg.apiExceptionUnauthorized());
1056+
}
1057+
1058+
test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async {
1059+
checkReloadFailure(completeLoading: completeWithApiExceptionUnauthorized);
1060+
}));
10301061
});
10311062

10321063
group('UpdateMachine.registerNotificationToken', () {

test/model/test_store.dart

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

130130
static const Duration removeAccountDuration = Duration(milliseconds: 1);
131131
Duration? loadPerAccountDuration;
132+
Object? loadPerAccountException;
132133

133134
/// Consume the log of calls made to [doRemoveAccount].
134135
List<int> takeDoRemoveAccountCalls() {
@@ -150,6 +151,9 @@ class TestGlobalStore extends GlobalStore {
150151
if (loadPerAccountDuration != null) {
151152
await Future<void>.delayed(loadPerAccountDuration!);
152153
}
154+
if (loadPerAccountException != null) {
155+
throw loadPerAccountException!;
156+
}
153157
final initialSnapshot = _initialSnapshots[accountId]!;
154158
final store = PerAccountStore.fromInitialSnapshot(
155159
globalStore: this,

test/widgets/app_test.dart

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,15 +61,18 @@ void main() {
6161
group('_PreventEmptyStack', () {
6262
late List<Route<void>> pushedRoutes;
6363
late List<Route<void>> removedRoutes;
64+
late List<Route<void>> poppedRoutes;
6465

6566
Future<void> prepare(WidgetTester tester) async {
6667
addTearDown(testBinding.reset);
6768

6869
pushedRoutes = [];
6970
removedRoutes = [];
71+
poppedRoutes = [];
7072
final testNavObserver = TestNavigatorObserver();
7173
testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route);
7274
testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route);
75+
testNavObserver.onPopped = (route, prevRoute) => poppedRoutes.add(route);
7376

7477
await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver]));
7578
await tester.pump(); // start to load account
@@ -92,6 +95,85 @@ void main() {
9295
check(removedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
9396
check(pushedRoutes).single.isA<WidgetRoute>().page.isA<ChooseAccountPage>();
9497
});
98+
99+
testWidgets('push route when popping last route on stack', (tester) async {
100+
// Set up the loading of per-account data to fail.
101+
await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
102+
testBinding.globalStore.loadPerAccountDuration = Duration.zero;
103+
testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized();
104+
await prepare(tester);
105+
// The navigator stack should contain only a home page route.
106+
107+
// Await the failed load, causing the home page to be removed
108+
// and an error dialog pushed in its place.
109+
await tester.pump(Duration.zero);
110+
await tester.pump(TestGlobalStore.removeAccountDuration);
111+
check(testBinding.globalStore.takeDoRemoveAccountCalls())
112+
.single.equals(eg.selfAccount.id);
113+
check(removedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
114+
check(poppedRoutes).isEmpty();
115+
check(pushedRoutes).single.isA<DialogRoute<void>>();
116+
pushedRoutes.clear();
117+
118+
// Dismiss the error dialog, causing it to be popped from the stack.
119+
await tester.tap(find.byWidget(checkErrorDialog(tester,
120+
expectedTitle: 'Could not connect',
121+
expectedMessage:
122+
'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.'
123+
' Please try logging in again or use another account.')));
124+
// The choose-account page should appear, because the error dialog
125+
// was the only route remaining.
126+
check(poppedRoutes).single.isA<DialogRoute<void>>();
127+
check(pushedRoutes).single.isA<WidgetRoute>().page.isA<ChooseAccountPage>();
128+
});
129+
130+
testWidgets('do not push route to non-empty navigator stack', (tester) async {
131+
// Set up the loading of per-account data to fail, but only after a
132+
// long enough time for the "Try another account" button to appear.
133+
const loadPerAccountDuration = Duration(seconds: 30);
134+
assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod);
135+
await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
136+
testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration;
137+
testBinding.globalStore.loadPerAccountException = eg.apiExceptionUnauthorized();
138+
await prepare(tester);
139+
// The navigator stack should contain only a home page route.
140+
141+
// Await the "Try another account" button, and tap it.
142+
await tester.pump(kTryAnotherAccountWaitPeriod);
143+
await tester.tap(find.text('Try another account'));
144+
await tester.pump();
145+
// The navigator stack should contain the home page route
146+
// and a choose-account page route.
147+
check(removedRoutes).isEmpty();
148+
check(poppedRoutes).isEmpty();
149+
check(pushedRoutes).single.isA<WidgetRoute>().page.isA<ChooseAccountPage>();
150+
pushedRoutes.clear();
151+
152+
// Now await the failed load, causing the home page to be removed
153+
// and an error dialog pushed, while the choose-account page remains.
154+
await tester.pump(loadPerAccountDuration);
155+
await tester.pump(TestGlobalStore.removeAccountDuration);
156+
check(testBinding.globalStore.takeDoRemoveAccountCalls())
157+
.single.equals(eg.selfAccount.id);
158+
check(removedRoutes).single.isA<WidgetRoute>().page.isA<HomePage>();
159+
check(poppedRoutes).isEmpty();
160+
check(pushedRoutes).single.isA<DialogRoute<void>>();
161+
pushedRoutes.clear();
162+
// The navigator stack should now contain the choose-account page route
163+
// and the dialog route.
164+
165+
// Dismiss the error dialog, causing it to be popped from the stack.
166+
await tester.tap(find.byWidget(checkErrorDialog(tester,
167+
expectedTitle: 'Could not connect',
168+
expectedMessage:
169+
'Your account at ${eg.selfAccount.realmUrl} could not be authenticated.'
170+
' Please try logging in again or use another account.')));
171+
// No routes should be pushed after dismissing the error dialog,
172+
// because there was already another route remaining on the stack
173+
// (namely the choose-account page route).
174+
check(poppedRoutes).single.isA<DialogRoute<void>>();
175+
check(pushedRoutes).isEmpty();
176+
});
95177
});
96178

97179
group('ChooseAccountPage', () {

0 commit comments

Comments
 (0)