Skip to content

Commit 14fc0de

Browse files
chrisbobbechrisbobbe
committed
login: Parse and validate realm-url input more like we do in zulip-mobile
For the corresponding logic in zulip-mobile, see src/start/RealmInputScreen.js. Between the URL parser we use here and the one we use in zulip-mobile, probably the set of rejected strings won't match exactly, but it'd be a surprising bug if either one rejects reasonable strings that we could've used. As in zulip-mobile, we distinguish validation errors we want to report immediately, from ones we'll wait to report until the user presses submit. (Compare this revision's `shouldDeferFeedback` to the first bit of zulip-mobile's `getSuggestion`.) Also add some tests, in an improvement over zulip-mobile. Fixes-partly: zulip#35
1 parent 7ab7d59 commit 14fc0de

File tree

2 files changed

+149
-13
lines changed

2 files changed

+149
-13
lines changed

‎lib/widgets/login.dart

Lines changed: 112 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import '../api/route/realm.dart';
66
import '../api/route/users.dart';
77
import '../model/store.dart';
88
import 'app.dart';
9+
import 'dialog.dart';
910
import 'store.dart';
1011

1112
class _LoginSequenceRoute extends MaterialPageRoute<void> {
@@ -14,6 +15,85 @@ class _LoginSequenceRoute extends MaterialPageRoute<void> {
1415
});
1516
}
1617

18+
enum ServerUrlValidationError {
19+
empty,
20+
invalidUrl,
21+
noUseEmail,
22+
unsupportedSchemeZulip,
23+
unsupportedSchemeOther;
24+
25+
/// Whether to wait until the user presses "submit" to give error feedback.
26+
///
27+
/// True for errors that will often happen when the user just hasn't finished
28+
/// typing a good URL. False for errors that strongly signal a wrong path was
29+
/// taken, like when we recognize the form of an email address.
30+
bool shouldDeferFeedback() {
31+
switch (this) {
32+
case empty:
33+
case invalidUrl:
34+
return true;
35+
case noUseEmail:
36+
case unsupportedSchemeZulip:
37+
case unsupportedSchemeOther:
38+
return false;
39+
}
40+
}
41+
42+
String message() { // TODO(i18n)
43+
switch (this) {
44+
case empty:
45+
return 'Please enter a URL.';
46+
case invalidUrl:
47+
return 'Please enter a valid URL.';
48+
case noUseEmail:
49+
return 'Please enter the server URL, not your email.';
50+
case unsupportedSchemeZulip:
51+
case unsupportedSchemeOther:
52+
return 'The server URL must start with http:// or https://.';
53+
}
54+
}
55+
}
56+
57+
class ServerUrlParseResult {
58+
ServerUrlParseResult({required this.url, required this.error});
59+
60+
final Uri? url;
61+
final ServerUrlValidationError? error;
62+
}
63+
64+
class ServerUrlTextEditingController extends TextEditingController {
65+
ServerUrlParseResult tryParse() {
66+
final trimmedText = text.trim();
67+
68+
if (trimmedText.isEmpty) {
69+
return ServerUrlParseResult(url: null, error: ServerUrlValidationError.empty);
70+
}
71+
72+
Uri? url = Uri.tryParse(trimmedText);
73+
if (!RegExp(r'^https?://').hasMatch(trimmedText)) {
74+
if (url != null && url.hasScheme && url.scheme == 'zulip') {
75+
// Someone might get the idea to try one of the "zulip://" URLs that
76+
// are discussed sometimes.
77+
// TODO(log): Log to Sentry? How much does this happen, if at all? Maybe
78+
// log once when the input enters this error state, but don't spam
79+
// on every keystroke/render while it's in it.
80+
return ServerUrlParseResult(url: null, error: ServerUrlValidationError.unsupportedSchemeZulip);
81+
} else if (url != null && url.hasScheme && !RegExp(r'^https?$').hasMatch(url.scheme)) {
82+
return ServerUrlParseResult(url: null, error: ServerUrlValidationError.unsupportedSchemeOther);
83+
}
84+
url = Uri.tryParse('https://$trimmedText');
85+
}
86+
87+
if (url == null || !url.isAbsolute) {
88+
return ServerUrlParseResult(url: null, error: ServerUrlValidationError.invalidUrl);
89+
}
90+
if (url.userInfo.isNotEmpty) {
91+
return ServerUrlParseResult(url: null, error: ServerUrlValidationError.noUseEmail);
92+
}
93+
return ServerUrlParseResult(url: url, error: null);
94+
}
95+
}
96+
1797
class AddAccountPage extends StatefulWidget {
1898
const AddAccountPage({super.key});
1999

@@ -27,27 +107,39 @@ class AddAccountPage extends StatefulWidget {
27107
}
28108

29109
class _AddAccountPageState extends State<AddAccountPage> {
30-
final TextEditingController _controller = TextEditingController();
110+
final ServerUrlTextEditingController _controller = ServerUrlTextEditingController();
111+
late ServerUrlParseResult _parseResult;
112+
113+
_serverUrlChanged() {
114+
setState(() {
115+
_parseResult = _controller.tryParse();
116+
});
117+
}
118+
119+
@override
120+
void initState() {
121+
super.initState();
122+
_parseResult = _controller.tryParse();
123+
_controller.addListener(_serverUrlChanged);
124+
}
31125

32126
@override
33127
void dispose() {
34128
_controller.dispose();
35129
super.dispose();
36130
}
37131

38-
Future<void> _onSubmitted(BuildContext context, String value) async {
39-
final Uri? url = Uri.tryParse(value);
40-
switch (url) {
41-
case Uri(scheme: 'https' || 'http'):
42-
// TODO(#35): validate realm URL further?
43-
break;
44-
default:
45-
// TODO(#35): give feedback to user on bad realm URL
46-
return;
132+
Future<void> _onSubmitted(BuildContext context) async {
133+
final ServerUrlParseResult(:url, :error) = _parseResult;
134+
if (error != null) {
135+
showErrorDialog(context: context,
136+
title: 'Invalid input', message: error.message());
137+
return;
47138
}
139+
assert(url != null);
48140

49141
// TODO(#35): show feedback that we're working, while fetching server settings
50-
final serverSettings = await getServerSettings(realmUrl: url);
142+
final serverSettings = await getServerSettings(realmUrl: url!);
51143
if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
52144
else {
53145
return;
@@ -61,6 +153,8 @@ class _AddAccountPageState extends State<AddAccountPage> {
61153
@override
62154
Widget build(BuildContext context) {
63155
assert(!PerAccountStoreWidget.debugExistsOf(context));
156+
final ServerUrlParseResult(:error) = _parseResult;
157+
64158
// TODO(#35): more help to user on entering realm URL
65159
return Scaffold(
66160
appBar: AppBar(title: const Text('Add an account')),
@@ -71,12 +165,17 @@ class _AddAccountPageState extends State<AddAccountPage> {
71165
constraints: const BoxConstraints(maxWidth: 400),
72166
child: TextField(
73167
controller: _controller,
74-
onSubmitted: (value) => _onSubmitted(context, value),
168+
onSubmitted: (value) => _onSubmitted(context),
75169
keyboardType: TextInputType.url,
76170
decoration: InputDecoration(
77171
labelText: 'Your Zulip server URL',
172+
errorText: error == null || error.shouldDeferFeedback()
173+
? null
174+
: error.message(),
78175
suffixIcon: IconButton(
79-
onPressed: () => _onSubmitted(context, _controller.text),
176+
onPressed: error == null || error.shouldDeferFeedback()
177+
? () => _onSubmitted(context)
178+
: null, // disabled; errorText will be present
80179
icon: const Icon(Icons.arrow_forward))))))));
81180
}
82181
}

‎test/widgets/login_test.dart

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import 'package:checks/checks.dart';
2+
import 'package:flutter_test/flutter_test.dart';
3+
import 'package:zulip/widgets/login.dart';
4+
5+
void main() {
6+
group('ServerUrlTextEditingController.tryParse', () {
7+
final controller = ServerUrlTextEditingController();
8+
9+
expectUrlFromText(String text, String expectedUrl) {
10+
test('text "$text" gives URL "$expectedUrl"', () {
11+
controller.text = text;
12+
check(controller.tryParse().url.toString()).equals(expectedUrl);
13+
});
14+
}
15+
16+
expectErrorFromText(String text, ServerUrlValidationError expectedError) {
17+
test('text "$text" gives error "$expectedError"', () {
18+
controller.text = text;
19+
check(controller.tryParse().error).equals(expectedError);
20+
});
21+
}
22+
23+
expectUrlFromText('https://chat.zulip.org', 'https://chat.zulip.org');
24+
expectUrlFromText('https://chat.zulip.org/', 'https://chat.zulip.org/');
25+
expectUrlFromText('http://chat.zulip.org', 'http://chat.zulip.org');
26+
expectUrlFromText('chat.zulip.org', 'https://chat.zulip.org');
27+
expectUrlFromText('192.168.1.21:9991', 'https://192.168.1.21:9991');
28+
expectUrlFromText('http://192.168.1.21:9991', 'http://192.168.1.21:9991');
29+
30+
expectErrorFromText('', ServerUrlValidationError.empty);
31+
expectErrorFromText(' ', ServerUrlValidationError.empty);
32+
expectErrorFromText('zulip://foo', ServerUrlValidationError.unsupportedSchemeZulip);
33+
expectErrorFromText('ftp://foo', ServerUrlValidationError.unsupportedSchemeOther);
34+
expectErrorFromText('!@#*asd;l4fkj', ServerUrlValidationError.invalidUrl);
35+
expectErrorFromText('[email protected]', ServerUrlValidationError.noUseEmail);
36+
});
37+
}

0 commit comments

Comments
 (0)