api: Don't allow connecting to servers <4.0 #1410

chrisbobbe · 2025-03-15T03:33:36Z

From the README:

https://github.com/zulip/zulip-flutter?tab=readme-ov-file#server-compatibility

We support Zulip Server 4.0 and later.

This implementation isn't ideal because it logs you out, instead of
just taking down the account's UI as we discussed:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.23F267.20Disallow.20connecting.20to.20unsupported.20ancient.20servers/near/2104182
But it was simpler this way since programmatically logging out is
already an action we handle.

Fixes: #267

Screenshot (with `kMinSupportedZulipFeatureLevel` fudged to 9999):

PIG208

Thanks for the PR! I went through all the commits and left some comments. This will need a rebase because of some conflicts with #1400.

PIG208 · 2025-03-17T18:40:15Z

lib/model/store.dart

+/// The fields 'zulip_version', 'zulip_merge_base', and 'zulip_feature_level'
+/// from a /register response.
+class ZulipVersionData {
+  ZulipVersionData({


nit: This can be a const constructor.

PIG208 · 2025-03-17T18:55:14Z

lib/widgets/actions.dart

+  ///
+  /// In English, the text in [successContent] should be short, should start with
+  /// a capital letter, and should have no ending punctuation: "{noun} copied".
+  static void copyWithPopup({


I looked around for other methods to be moved here, and found ShareButton.onPressed and _uploadFiles from lib/widgets/compose_box.dart to fit conceptually, but neither of them needs to be reused now or in the near future. So I think the current collection of methods in PlatformActions is pretty complete.

PIG208 · 2025-03-17T18:59:20Z

lib/widgets/content.dart

+    final zulipLocalizations = ZulipLocalizations.of(context);
+    showErrorDialog(context: context,
+      title: zulipLocalizations.errorCouldNotOpenLinkTitle,
+      message: zulipLocalizations.errorCouldNotOpenLink(urlString));


nit: While the main motivation from fe3538b does apply here, would it make sense to use url.toString here to be consistent?

While we do have a test "error dialog if invalid link" that covers a similar error in PlatformActions.launchUrl, I saw that this branch does not have test coverage.

We should probably move that test to the corresponding file for PlatformActions.

nit: While the main motivation from fe3538b does apply here, would it make sense to use url.toString here to be consistent?

No, not here :) because this is in an if (url == null) {}, and url.toString() would always be "null".

PIG208 · 2025-03-17T19:15:41Z

lib/widgets/login.dart

@@ -343,6 +343,9 @@ class _LoginPageState extends State<LoginPage> {
      // Could set [_inProgress]… but we'd need to unset it if the web-auth
      // attempt is aborted (by the user closing the browser, for example),
      // and I don't think we can reliably know when that happens.
+
+      // Not using [PlatformActions.launchUrl] because web auth needs special
+      // error handling.


Perhaps as a part of #462, we can get rid of the block of iOS specific error handling code and reuse PlatofmActions.launchUrl, but I'm hesitant because of the question mark from // TODO(#462) remove this?.

Yeah; that sounds like it would need investigating when we get to #462.

PIG208 · 2025-03-17T19:29:28Z

lib/model/store.dart

+      zulipMergeBase: initialSnapshot.zulipMergeBase,
+      zulipFeatureLevel: initialSnapshot.zulipFeatureLevel);
+
+  /// A [ZulipVersionData] from a [MalformedServerResponseException],


I think it might be better to start this with a verb.

PIG208 · 2025-03-17T19:55:52Z

test/model/store_test.dart

+      async.flushTimers();
+      // Reload never succeeds and there are no unhandled errors.
+      check(globalStore.perAccountSync(eg.selfAccount.id)).isNull();
+    }));


Other than these tests, I think it would be valuable to have an end-to-end widget test that checks the error dialog when connecting to an ancient server, but I feel like it can be difficult to set up.

I agree. We'd need change TestZulipBinding so that it could work with a UpdateMachineTestGlobalStore, and I'm not sure how best to do that.

The TestZulipBinding.globalStore getter has a TestGlobalStore return type, and UpdateMachineTestGlobalStore doesn't extend TestGlobalStore. Both classes extend GlobalStore, but that doesn't make sense as a return type; it would break a lot of callers that use things like TestGlobalStore.add.

Thanks Zixuan for noticing we haven't been covering this: zulip#1410 (comment)

chrisbobbe · 2025-03-18T02:16:05Z

Thanks for the review! Revision pushed.

chrisbobbe · 2025-03-18T20:58:49Z

(Just rebased past #1416.)

PIG208

Thanks for the update! I read the new commit and have some small comments. Marking this for Greg's review.

PIG208 · 2025-03-19T00:29:23Z

test/model/binding.dart

+  /// See also:
+  ///   * [launchUrlResult]
+  ///   * [takeLaunchUrlCalls]
+  PlatformException? launchUrlException;


Looks like this should be reset in _resetLaunchUrl.

PIG208 · 2025-03-19T00:38:13Z

lib/api/core.dart

+/// and the README.
+const kMinSupportedZulipVersion = '4.0';
+
+/// The Zulip feature level reserved for the [minSupportedZulipVersion] release.


nit: the only minSupportedZulipVersion I can find is the parameter of errorServerVersionUnsupportedMessage, but it is not a defined symbol within the dartdoc's context; I think this is supposed to be the "k"-prefixed constant?

Indeed, thanks for the catch!

PIG208 · 2025-03-19T00:39:32Z

lib/widgets/actions.dart

+  }
+
+  /// Opens a URL with [ZulipBinding.launchUrl], with an error dialog on failure.
+  // TODO widget tests


Now that we have a dedicated test group, we should be able to remove the TODO, unless there is something else that needs tests here?

chrisbobbe · 2025-03-19T01:06:24Z

Thanks for the review! Revision pushed.

gnprice

Thanks! I've read most of the branch:
e13084a store test [nfc]: Replace old method name in a test group
8228d97 store [nfc]: Move some declarations up out of a switch case
ebe5f70 store [nfc]: Re-query account after _registerQueueWithRetry async gap
2983cdb store [nfc]: Helper code for UpdateMachine.load's update-feature-level
413bf1b store [nfc]: Stop using mutable Account variable in UpdateMachine.load
fb73e41 content [nfc]: Inline showError helper in _launchUrl
50379e2 actions test [nfc]: Put ZulipActions tests in new 'ZulipActions' group
b03c30a actions [nfc]: Add PlatformActions, alongside ZulipActions
ae8a3f7 content: Use url.toString() instead of urlString in error message
eb9883f actions [nfc]: Move open-link logic here, from lib/widgets/content

and part of the final/main commit:
2468db2 api: Don't allow connecting to servers <4.0

That leaves:
367a165 actions test: Cover PlatformActions.launchUrl
ad292ca dialog: Add "Learn more"-button param to showErrorDialog
and the rest of the final commit.

gnprice · 2025-03-20T00:49:43Z

test/model/store_test.dart

-  group('GlobalStore.updateGlobalSettings', () {
+  group('GlobalStore update settings', () {


This looks like it was meant to happen in 9201ae42f.

Yeah. #1421 has its own version of this fix.

gnprice · 2025-03-20T00:58:30Z

lib/model/store.dart

-      connection.zulipFeatureLevel = initialSnapshot.zulipFeatureLevel;
-    }
+    final zulipVersionData = ZulipVersionData.fromInitialSnapshot(initialSnapshot);
+    await updateZulipVersionData(zulipVersionData);


I don't love having this introduce an await when we usually don't need one (because the version data will usually be up to date already).

How about introducing a method to consolidate that version-comparison condition? Then this can look like

if (!zulipVersionData.matchesAccount(globalStore.getAccount(accountId)!)) { // … }

gnprice · 2025-03-20T01:01:36Z

lib/model/store.dart

+      connection.close();
+      await updateZulipVersionData(e.data);


This will mutate connection after closing it. Probably doesn't throw, but incorrect in principle.

With the suggestion in the previous comment, this can inline updateZulipVersionData, and just skip the line that updates connection.

Maybe further clean up by having a method on GlobalStore:

if (!zulipVersionData.matchesAccount(globalStore.getAccount(accountId)!)) { await globalStore.updateZulipVersionData(accountId, zulipVersionData); }

gnprice · 2025-03-20T01:10:42Z

lib/model/store.dart

+      return ZulipVersionData(
+        zulipVersion: data['zulip_version'] as String,
+        zulipMergeBase: data['zulip_merge_base'] as String?,
+        zulipFeatureLevel: data['zulip_feature_level'] as int);


If there's a zulip_version but no zulip_feature_level, I think we can infer it's indeed Zulip just ancient — so the feature level is zero. (I.e. it's from before Zulip 3.0 introduced feature levels.)

chrisbobbe · 2025-03-20T21:31:31Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Looks good, and I've now read the whole thing. Just a few small comments below.

gnprice · 2025-03-21T00:01:04Z

test/widgets/actions_test.dart

+      final httpUrl = Uri.parse('http://chat.zulip.org');
+      final nonHttpUrl = Uri.parse('mailto:[email protected]');


nit:

Suggested change

final httpUrl = Uri.parse('http://chat.zulip.org');

final nonHttpUrl = Uri.parse('mailto:chat@zulip.org');

final httpUrl = Uri.parse('https://chat.example');

final nonHttpUrl = Uri.parse('mailto:chat@example');

use reserved example domains (RFC 6761; https://en.wikipedia.org/wiki/Special-use_domain_name)

use https: rather than http:, since https: should be the norm and cleartext http: rare

gnprice · 2025-03-21T00:06:00Z

test/widgets/dialog_test.dart

+      final element = tester.element(find.byType(Placeholder));
+
+      showErrorDialog(context: element, title: 'hello',
+        learnMoreButtonUrl: Uri.parse('foo.example'));


nit: make it a proper absolute URL, for realism

Suggested change

learnMoreButtonUrl: Uri.parse('foo.example'));

learnMoreButtonUrl: Uri.parse('https://foo.example'));

(if it isn't, launching is unlikely to work)

gnprice · 2025-03-21T00:15:03Z

lib/model/store.dart

+      // `!` is OK because _registerQueueWithRetry would have thrown a
+      // not-_ServerVersionUnsupportedException if no account
+      if (!e.data.matchesAccount(globalStore.getAccount(accountId)!)) {
+        await globalStore.updateZulipVersionData(accountId, e.data);


I guess since the caller is going to log the account out anyway, this update isn't really needed. (Realized that when reading tests and seeing there doesn't seem to be a test exercising this.)

Having already done the work to write it, though, may as well keep it. It's more correct in principle, and will simplify if in the future we have it not log out in this case.

gnprice · 2025-03-21T00:38:17Z

lib/api/core.dart

+// TODO: Instead, link to new Help Center doc once we have it:
+//   https://github.com/zulip/zulip/issues/23842
+final kServerSupportDocUrl = Uri.parse(
+  'https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html#compatibility-and-upgrading');


Suggested change

'https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html#compatibility-and-upgrading');

'https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html#client-apps');

The other URL's fragment doesn't seem to exist on that page now.

(It's fine, the page still loads, but the user doesn't get taken to any particular spot on it.)

chrisbobbe · 2025-04-01T01:04:21Z

Thanks for the review! Revision pushed.

And rethrow AccountNotFoundExceptions in an explicit `on` block, rather than in the `default` case of this `switch`. (Otherwise the account-exists check would throw.)

I believe this is NFC because: - the only fields we read are zulipVersion, zulipMergeBase, and zulipFeatureLevel - we read those fields only to update them if needed - nothing else could have written to those fields during the async gap; this register-queue codepath is the only codepath that writes to them, and we don't allow concurrent register-queues for an account But we've been using an Account object from across an async gap, and that looks like a code smell that calls for a fix.

This will be helpful for disallowing ancient servers, coming up.

Some of _launchUrl is specific to opening links in message content, but a lot is generic and could helpfully be pulled out to be reused in other places where we open links. We'll do that next; first, inline this helper function, which has one caller in the content-specific code and one in the generic code.

And bring over one function that fits well here. This seems like a good place to put a function for opening a link; we'll do that next.

Just so when we extract out much of this code for reuse, it won't need a `urlString` param alongside `url`. I think the effect is just that when the URL is a relative URL, the error dialog will always show it as an absolute URL on the realm.

We'll use when we add a "Learn more"-button param to showErrorDialog, coming up.

And reportErrorToUserModally.

From the README: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#server-compatibility > We support Zulip Server 4.0 and later. This implementation isn't ideal because it logs you out, instead of just taking down the account's UI as we discussed: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/.23F267.20Disallow.20connecting.20to.20unsupported.20ancient.20servers/near/2104182 But it was simpler this way since programmatically logging out is already an action we handle. Fixes: zulip#267

gnprice · 2025-04-02T03:58:45Z

Thanks! Looks good; merging. Glad to have this complete. 🙂

chrisbobbe requested a review from PIG208 March 15, 2025 03:33

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Mar 15, 2025

chrisbobbe assigned PIG208 Mar 15, 2025

PIG208 reviewed Mar 17, 2025

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Mar 17, 2025

content test: Test tap-link behavior when invalid URL

4b1f484

Thanks Zixuan for noticing we haven't been covering this: zulip#1410 (comment)

chrisbobbe mentioned this pull request Mar 17, 2025

content test: Test tap-link behavior when invalid URL #1417

Merged

gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this pull request Mar 18, 2025

content test: Test tap-link behavior when invalid URL

cb9a849

Thanks Zixuan for noticing we haven't been covering this: zulip#1410 (comment)

chrisbobbe force-pushed the pr-ancient-servers branch from b38efb2 to 4188181 Compare March 18, 2025 02:15

chrisbobbe force-pushed the pr-ancient-servers branch from 4188181 to 1ef8b60 Compare March 18, 2025 20:58

PIG208 reviewed Mar 19, 2025

View reviewed changes

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Mar 19, 2025

PIG208 assigned gnprice and unassigned PIG208 Mar 19, 2025

PIG208 requested a review from gnprice March 19, 2025 00:40

chrisbobbe force-pushed the pr-ancient-servers branch from 1ef8b60 to 2468db2 Compare March 19, 2025 01:06

gnprice reviewed Mar 20, 2025

View reviewed changes

chrisbobbe force-pushed the pr-ancient-servers branch from 2468db2 to 3297253 Compare March 20, 2025 21:31

gnprice reviewed Mar 21, 2025

View reviewed changes

chrisbobbe mentioned this pull request Mar 27, 2025

notif ios: Handle opening of conversation on tap; take 2 #1379

Open

chrisbobbe force-pushed the pr-ancient-servers branch from 3297253 to 85f8829 Compare April 1, 2025 01:04

chrisbobbe added 4 commits April 1, 2025 20:56

store [nfc]: Move some declarations up out of a switch case

e5cd6cc

And rethrow AccountNotFoundExceptions in an explicit `on` block, rather than in the `default` case of this `switch`. (Otherwise the account-exists check would throw.)

store [nfc]: Helper code for UpdateMachine.load's update-feature-level

e7afe1b

This will be helpful for disallowing ancient servers, coming up.

store [nfc]: Stop using mutable Account variable in UpdateMachine.load

5133c45

chrisbobbe added 8 commits April 1, 2025 20:56

actions test [nfc]: Put ZulipActions tests in new 'ZulipActions' group

99684b9

actions [nfc]: Add PlatformActions, alongside ZulipActions

7a685c8

And bring over one function that fits well here. This seems like a good place to put a function for opening a link; we'll do that next.

actions [nfc]: Move open-link logic here, from lib/widgets/content

a107f00

We'll use when we add a "Learn more"-button param to showErrorDialog, coming up.

actions test: Cover PlatformActions.launchUrl

b652c8a

dialog: Add "Learn more"-button param to showErrorDialog

0094978

And reportErrorToUserModally.

gnprice force-pushed the pr-ancient-servers branch from 85f8829 to 0494723 Compare April 2, 2025 03:58

gnprice merged commit 0494723 into zulip:main Apr 2, 2025
1 check passed

chrisbobbe deleted the pr-ancient-servers branch April 2, 2025 04:06

gnprice mentioned this pull request Apr 2, 2025

dialog: Use Cupertino-flavored alert dialogs on iOS #1017

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: Don't allow connecting to servers <4.0 #1410

api: Don't allow connecting to servers <4.0 #1410

chrisbobbe commented Mar 15, 2025 •

edited

Loading

PIG208 left a comment

PIG208 Mar 17, 2025

PIG208 Mar 17, 2025

PIG208 Mar 17, 2025

PIG208 Mar 17, 2025

chrisbobbe Mar 17, 2025

PIG208 Mar 17, 2025

chrisbobbe Mar 18, 2025

PIG208 Mar 17, 2025

PIG208 Mar 17, 2025

chrisbobbe Mar 18, 2025

chrisbobbe commented Mar 18, 2025

chrisbobbe commented Mar 18, 2025

PIG208 left a comment

PIG208 Mar 19, 2025

PIG208 Mar 19, 2025

chrisbobbe Mar 19, 2025

PIG208 Mar 19, 2025

chrisbobbe Mar 19, 2025

chrisbobbe commented Mar 19, 2025

gnprice left a comment

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

chrisbobbe commented Mar 20, 2025

gnprice left a comment

gnprice Mar 21, 2025

gnprice Mar 21, 2025

gnprice Mar 21, 2025

gnprice Mar 21, 2025

chrisbobbe commented Apr 1, 2025

gnprice commented Apr 2, 2025

		group('GlobalStore.updateGlobalSettings', () {
		group('GlobalStore update settings', () {

		final httpUrl = Uri.parse('http://chat.zulip.org');
		final nonHttpUrl = Uri.parse('mailto:[email protected]');

-      final httpUrl = Uri.parse('http://chat.zulip.org');
-      final nonHttpUrl = Uri.parse('mailto:chat@zulip.org');
+      final httpUrl = Uri.parse('https://chat.example');
+      final nonHttpUrl = Uri.parse('mailto:chat@example');

	learnMoreButtonUrl: Uri.parse('foo.example'));
	learnMoreButtonUrl: Uri.parse('https://foo.example'));

	'https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html#compatibility-and-upgrading');
	'https://zulip.readthedocs.io/en/latest/overview/release-lifecycle.html#client-apps');

api: Don't allow connecting to servers <4.0 #1410

api: Don't allow connecting to servers <4.0 #1410

Conversation

chrisbobbe commented Mar 15, 2025 • edited Loading

Screenshot (with kMinSupportedZulipFeatureLevel fudged to 9999):

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 18, 2025

chrisbobbe commented Mar 18, 2025

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 19, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 20, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 1, 2025

gnprice commented Apr 2, 2025

chrisbobbe commented Mar 15, 2025 •

edited

Loading

Screenshot (with `kMinSupportedZulipFeatureLevel` fudged to 9999):