Skip to content

Commit

Permalink
store: Handle invalid API key on register-queue
Browse files Browse the repository at this point in the history
Fixes: zulip#737

Signed-off-by: Zixuan James Li <[email protected]>
  • Loading branch information
PIG208 committed Jan 27, 2025
1 parent 930ef5b commit 7949a71
Show file tree
Hide file tree
Showing 15 changed files with 343 additions and 7 deletions.
7 changes: 7 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,13 @@
"@topicValidationErrorMandatoryButEmpty": {
"description": "Topic validation error when topic is required but was empty."
},
"errorInvalidApiKeyMessage": "Your account at {url} could not be authenticated. Please try logging in again or use another account.",
"@errorInvalidApiKeyMessage": {
"description": "Error message in the dialog for invalid API key.",
"placeholders": {
"url": {"type": "String", "example": "http://chat.example.com/"}
}
},
"errorInvalidResponse": "The server sent an invalid response",
"@errorInvalidResponse": {
"description": "Error message when an API call returned an invalid response."
Expand Down
6 changes: 6 additions & 0 deletions lib/generated/l10n/zulip_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,12 @@ abstract class ZulipLocalizations {
/// **'Topics are required in this organization.'**
String get topicValidationErrorMandatoryButEmpty;

/// Error message in the dialog for invalid API key.
///
/// In en, this message translates to:
/// **'Your account at {url} could not be authenticated. Please try logging in again or use another account.'**
String errorInvalidApiKeyMessage(String url);

/// Error message when an API call returned an invalid response.
///
/// In en, this message translates to:
Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_ja.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_nb.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsNb extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'The server sent an invalid response';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_pl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Wątki są wymagane przez tę organizację.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'Nieprawidłowa odpowiedź serwera';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_ru.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Темы обязательны в этой организации.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'Получен недопустимый ответ сервера';

Expand Down
5 changes: 5 additions & 0 deletions lib/generated/l10n/zulip_localizations_sk.dart
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,11 @@ class ZulipLocalizationsSk extends ZulipLocalizations {
@override
String get topicValidationErrorMandatoryButEmpty => 'Topics are required in this organization.';

@override
String errorInvalidApiKeyMessage(String url) {
return 'Your account at $url could not be authenticated. Please try logging in again or use another account.';
}

@override
String get errorInvalidResponse => 'Server poslal nesprávnu odpoveď';

Expand Down
43 changes: 37 additions & 6 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import '../api/backoff.dart';
import '../api/route/realm.dart';
import '../log.dart';
import '../notifications/receive.dart';
import 'actions.dart';
import 'autocomplete.dart';
import 'database.dart';
import 'emoji.dart';
Expand Down Expand Up @@ -149,13 +150,31 @@ abstract class GlobalStore extends ChangeNotifier {
/// and/or [perAccountSync].
Future<PerAccountStore> loadPerAccount(int accountId) async {
assert(_accounts.containsKey(accountId));
final store = await doLoadPerAccount(accountId);
PerAccountStore? store;
try {
store = await doLoadPerAccount(accountId);
} catch (e) {
switch (e) {
case ZulipApiException(code: 'INVALID_API_KEY'):
// The API key is invalid and the store can never be loaded
// unless the user retries manually.
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
final account = getAccount(accountId)!;
reportErrorToUserModally(
zulipLocalizations.errorCouldNotConnectTitle,
details: zulipLocalizations.errorInvalidApiKeyMessage(
account.realmUrl.toString()));
await logOutAccount(this, accountId);
default:
rethrow;
}
}
if (!_accounts.containsKey(accountId)) {
// [removeAccount] was called during [doLoadPerAccount].
store.dispose();
// [removeAccount] was called during or after [doLoadPerAccount].
store?.dispose();
throw AccountNotFoundException();
}
return store;
return store!;
}

/// Load per-account data for the given account, unconditionally.
Expand Down Expand Up @@ -912,7 +931,13 @@ class UpdateMachine {
// at 1 kiB (at least on Android), and stack can be longer than that.
assert(debugLog('Stack:\n$s'));
assert(debugLog('Backing off, then will retry…'));
// TODO tell user if initial-fetch errors persist, or look non-transient
// TODO(#890): tell user if initial-fetch errors persist, or look non-transient
switch (e) {
case ZulipApiException(code: 'INVALID_API_KEY'):
// We cannot recover from this error through retrying.
// Leave it to [GlobalStore.loadPerAccount].
rethrow;
}
await (backoffMachine ??= BackoffMachine()).wait();
assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
}
Expand Down Expand Up @@ -1044,7 +1069,13 @@ class UpdateMachine {
}
} catch (e) {
if (_disposed) return;
await _handlePollError(e);
try {
await _handlePollError(e);
} on AccountNotFoundException {
// Cannot recover by replacing the store because the account
// was logged out.
return;
}
assert(_disposed);
return;
}
Expand Down
37 changes: 36 additions & 1 deletion lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
final themeData = zulipThemeData(context);
return GlobalStoreWidget(
child: Builder(builder: (context) {
final effectiveNavigatorObservers = [
_EmptyStackNavigatorObserver(),
if (widget.navigatorObservers != null)
...widget.navigatorObservers!,
];
final globalStore = GlobalStoreWidget.of(context);
// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;
Expand All @@ -199,7 +204,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
theme: themeData,

navigatorKey: ZulipApp.navigatorKey,
navigatorObservers: widget.navigatorObservers ?? const [],
navigatorObservers: effectiveNavigatorObservers,
builder: (BuildContext context, Widget? child) {
if (!ZulipApp.ready.value) {
SchedulerBinding.instance.addPostFrameCallback(
Expand Down Expand Up @@ -230,6 +235,36 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
}
}

class _EmptyStackNavigatorObserver extends NavigatorObserver {
void _pushIfEmpty() async {
final navigator = await ZulipApp.navigator;
bool isEmptyStack = true;
// TODO: find a better way to inspect the navigator stack
navigator.popUntil((route) {
isEmptyStack = false;
return true; // never actually pops
});
if (isEmptyStack) {
unawaited(navigator.push(
MaterialWidgetRoute(page: const ChooseAccountPage())));
}
}

@override
void didRemove(Route<void> route, Route<void>? previousRoute) async {
if (previousRoute == null) {
_pushIfEmpty();
}
}

@override
void didPop(Route<void> route, Route<void>? previousRoute) async {
if (previousRoute == null) {
_pushIfEmpty();
}
}
}

class ChooseAccountPage extends StatelessWidget {
const ChooseAccountPage({super.key});

Expand Down
34 changes: 34 additions & 0 deletions test/model/store_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:flutter/foundation.dart';
import 'package:http/http.dart' as http;
import 'package:test/scaffolding.dart';
import 'package:zulip/api/core.dart';
import 'package:zulip/api/exception.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/api/route/events.dart';
Expand Down Expand Up @@ -500,6 +501,20 @@ void main() {
users.map((expected) => (it) => it.fullName.equals(expected.fullName)));
}));

test('GlobalStore.perAccount on INVALID_API_KEY', () => awaitFakeAsync((async) async {
addTearDown(testBinding.reset);

await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false));
testBinding.globalStore.loadPerAccountException = ZulipApiException(
routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400,
data: {}, message: '');
await check(testBinding.globalStore.perAccount(eg.selfAccount.id))
.throws<AccountNotFoundException>();

check(testBinding.globalStore.takeDoRemoveAccountCalls())
.single.equals(eg.selfAccount.id);
}));

// TODO test UpdateMachine.load starts polling loop
// TODO test UpdateMachine.load calls registerNotificationToken
});
Expand Down Expand Up @@ -843,6 +858,25 @@ void main() {
check(store.debugMessageListViews).isEmpty();
}));

test('log out if fail to reload on unexpected errors', () => awaitFakeAsync((async) async {
await preparePoll();

prepareUnexpectedLoopError();
updateMachine.debugAdvanceLoop();
async.elapse(Duration.zero);
check(store).isLoading.isTrue();

globalStore.loadPerAccountException = ZulipApiException(
routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400,
data: {}, message: '');
// The reload doesn't happen immediately; there's a timer.
check(async.pendingTimers).length.equals(1);
async.flushTimers();

check(globalStore.takeDoRemoveAccountCalls().single)
.equals(eg.selfAccount.id);
}));

group('report error', () {
String? lastReportedError;
String? takeLastReportedError() {
Expand Down
4 changes: 4 additions & 0 deletions test/model/test_store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class TestGlobalStore extends GlobalStore {

static const Duration removeAccountDuration = Duration(milliseconds: 1);
Duration? loadPerAccountDuration;
Object? loadPerAccountException;

/// Consume the log of calls made to [doRemoveAccount].
List<int> takeDoRemoveAccountCalls() {
Expand All @@ -147,6 +148,9 @@ class TestGlobalStore extends GlobalStore {
if (loadPerAccountDuration != null) {
await Future<void>.delayed(loadPerAccountDuration!);
}
if (loadPerAccountException != null) {
throw loadPerAccountException!;
}
final initialSnapshot = _initialSnapshots[accountId]!;
final store = PerAccountStore.fromInitialSnapshot(
globalStore: this,
Expand Down
Loading

0 comments on commit 7949a71

Please sign in to comment.