fix(core): Fix crash when no negotiator matches

Fixes #30. Also removes the `allowPlainAuth` attribute of `ConnectionSettings` as users who want to disable SASL PLAIN can just not register the negotiator or extend it.
2023-03-18 14:54:39 +01:00 · 2023-03-18 14:54:39 +01:00 · de85bf848d
commit de85bf848d
parent 7a6bf468bc
15 changed files with 135 additions and 61 deletions
--- a/packages/moxxmpp/CHANGELOG.md
+++ b/packages/moxxmpp/CHANGELOG.md
@ -1,6 +1,7 @@
 ## 0.3.0

 - **BREAKING**: Removed `connectAwaitable` and merged it with `connect`.
+- **BREAKING**: Removed `allowPlainAuth` from `ConnectionSettings`. If you don't want to use SASL PLAIN, don't register the negotiator. If you want to only conditionally use SASL PLAIN, extend the `SaslPlainNegotiator` and override its `matchesFeature` method to only call the super method when SASL PLAIN should be used.

 ## 0.1.6+1

--- a/packages/moxxmpp/lib/moxxmpp.dart
+++ b/packages/moxxmpp/lib/moxxmpp.dart
@ -1,6 +1,7 @@
 library moxxmpp;

 export 'package:moxxmpp/src/connection.dart';
+export 'package:moxxmpp/src/connection_errors.dart';
 export 'package:moxxmpp/src/connectivity.dart';
 export 'package:moxxmpp/src/errors.dart';
 export 'package:moxxmpp/src/events.dart';
--- a/packages/moxxmpp/lib/src/connection.dart
+++ b/packages/moxxmpp/lib/src/connection.dart
@ -395,9 +395,7 @@ class XmppConnection {
      );
      _connectionCompleter?.complete(
        Result(
-          StreamFailureError(
-            error,
-          ),
+          error,
        ),
      );
      _connectionCompleter = null;
@ -875,7 +873,7 @@ class XmppConnection {
  }

  Future<void> _executeCurrentNegotiator(XMLNode nonza) async {
-    // If we don't have a negotiator get one
+    // If we don't have a negotiator, get one
    _currentNegotiator ??= getNextNegotiator(_streamFeatures);
    if (_currentNegotiator == null &&
        _isMandatoryNegotiationDone(_streamFeatures) &&
@ -886,6 +884,25 @@ class XmppConnection {
      return;
    }

+    // If we don't have a next negotiator, we have to bail
+    if (_currentNegotiator == null &&
+        !_isMandatoryNegotiationDone(_streamFeatures) &&
+        !_isNegotiationPossible(_streamFeatures)) {
+      // We failed before authenticating
+      if (!_isAuthenticated) {
+        _log.severe('No negotiator could be picked while unauthenticated');
+        await _resetIsConnectionRunning();
+        await handleError(NoMatchingAuthenticationMechanismAvailableError());
+        return;
+      } else {
+        _log.severe(
+            'No negotiator could be picked while negotiations are not done');
+        await _resetIsConnectionRunning();
+        await handleError(NoAuthenticatorAvailableError());
+        return;
+      }
+    }
+
    final result = await _currentNegotiator!.negotiate(nonza);
    if (result.isType<NegotiatorError>()) {
      _log.severe('Negotiator returned an error');
--- a/packages/moxxmpp/lib/src/connection_errors.dart
+++ b/packages/moxxmpp/lib/src/connection_errors.dart
@ -2,16 +2,22 @@ import 'package:moxxmpp/src/errors.dart';
 import 'package:moxxmpp/src/negotiators/negotiator.dart';

 /// The reason a call to `XmppConnection.connect` failed.
-abstract class XmppConnectionError {}
+abstract class XmppConnectionError extends XmppError {}

 /// Returned by `XmppConnection.connect` when a connection is already active.
-class ConnectionAlreadyRunningError extends XmppConnectionError {}
+class ConnectionAlreadyRunningError extends XmppConnectionError {
+  @override
+  bool isRecoverable() => true;
+}

 /// Returned by `XmppConnection.connect` when a negotiator returned an unrecoverable
 /// error. Only returned when waitUntilLogin is true.
 class NegotiatorReturnedError extends XmppConnectionError {
  NegotiatorReturnedError(this.error);

+  @override
+  bool isRecoverable() => error.isRecoverable();
+
  /// The error returned by the negotiator.
  final NegotiatorError error;
 }
@ -19,10 +25,30 @@ class NegotiatorReturnedError extends XmppConnectionError {
 class StreamFailureError extends XmppConnectionError {
  StreamFailureError(this.error);

+  @override
+  bool isRecoverable() => error.isRecoverable();
+
  /// The error that causes a connection failure.
  final XmppError error;
 }

 /// Returned by `XmppConnection.connect` when no connection could
 /// be established.
-class NoConnectionPossibleError extends XmppConnectionError {}
+class NoConnectionPossibleError extends XmppConnectionError {
+  @override
+  bool isRecoverable() => true;
+}
+
+/// Returned if no matching authentication mechanism has been presented
+class NoMatchingAuthenticationMechanismAvailableError
+    extends XmppConnectionError {
+  @override
+  bool isRecoverable() => false;
+}
+
+/// Returned if no negotiator was picked, even though negotiations are not done
+/// yet.
+class NoAuthenticatorAvailableError extends XmppConnectionError {
+  @override
+  bool isRecoverable() => false;
+}
--- a/packages/moxxmpp/lib/src/negotiators/sasl/plain.dart
+++ b/packages/moxxmpp/lib/src/negotiators/sasl/plain.dart
@ -28,8 +28,6 @@ class SaslPlainNegotiator extends SaslNegotiator {

  @override
  bool matchesFeature(List<XMLNode> features) {
-    if (!attributes.getConnectionSettings().allowPlainAuth) return false;
-
    if (super.matchesFeature(features)) {
      if (!attributes.getSocket().isSecure()) {
        _log.warning(
--- a/packages/moxxmpp/lib/src/settings.dart
+++ b/packages/moxxmpp/lib/src/settings.dart
@ -5,10 +5,8 @@ class ConnectionSettings {
    required this.jid,
    required this.password,
    required this.useDirectTLS,
-    required this.allowPlainAuth,
  });
  final JID jid;
  final String password;
  final bool useDirectTLS;
-  final bool allowPlainAuth;
 }
--- a/packages/moxxmpp/test/helpers/manager.dart
+++ b/packages/moxxmpp/test/helpers/manager.dart
@ -26,7 +26,6 @@ class TestingManagerHolder {
    jid: jid,
    password: 'abc123',
    useDirectTLS: true,
-    allowPlainAuth: true,
  );

  Future<XMLNode> _sendStanza(
--- a/packages/moxxmpp/test/helpers/xmpp.dart
+++ b/packages/moxxmpp/test/helpers/xmpp.dart
@ -51,11 +51,8 @@ class StanzaExpectation extends ExpectationBase {
  }
 }

-/// Use [settings] to build the beginning of a play that can be used with StubTCPSocket. [settings]'s allowPlainAuth must
 /// be set to true.
 List<ExpectationBase> buildAuthenticatedPlay(ConnectionSettings settings) {
-  assert(settings.allowPlainAuth, 'SASL PLAIN must be allowed');
-
  final plain = base64.encode(
    utf8.encode('\u0000${settings.jid.local}\u0000${settings.password}'),
  );
--- a/packages/moxxmpp/test/negotiator_test.dart
+++ b/packages/moxxmpp/test/negotiator_test.dart
@ -36,7 +36,7 @@ void main() {
  initLogger();

  final stubSocket = StubTCPSocket(
-    play: [
+    [
      StringExpectation(
        "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
        '''
@ -74,7 +74,6 @@ void main() {
        jid: JID.fromString('user@test.server'),
        password: 'abc123',
        useDirectTLS: true,
-        allowPlainAuth: false,
      ),
    );
  final features = [
--- a/packages/moxxmpp/test/sasl/scram_test.dart
+++ b/packages/moxxmpp/test/sasl/scram_test.dart
@ -36,13 +36,13 @@ final scramSha256StreamFeatures = XMLNode(
 );

 void main() {
-  final fakeSocket = StubTCPSocket(play: []);
+  final fakeSocket = StubTCPSocket([]);
  test('Test SASL SCRAM-SHA-1', () async {
      final negotiator = SaslScramNegotiator(0, 'n=user,r=fyko+d2lbbFgONRv9qkxdawL', 'fyko+d2lbbFgONRv9qkxdawL', ScramHashType.sha1);
      negotiator.register(
        NegotiatorAttributes(
          (XMLNode _, {String? redact}) {},
-          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true, allowPlainAuth: true),
+          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true),
          (_) async {},
          getNegotiatorNullStub,
          getManagerNullStub,
@ -106,7 +106,7 @@ void main() {
      negotiator.register(
        NegotiatorAttributes(
          (XMLNode n, {String? redact}) => lastMessage = n.innerText(),
-          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true, allowPlainAuth: true),
+          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true),
          (_) async {},
          getNegotiatorNullStub,
          getManagerNullStub,
@ -138,7 +138,7 @@ void main() {
      negotiator.register(
        NegotiatorAttributes(
          (XMLNode _, {String? redact}) {},
-          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true, allowPlainAuth: true),
+          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true),
          (_) async {},
          getNegotiatorNullStub,
          getManagerNullStub,
@ -160,7 +160,7 @@ void main() {
      negotiator.register(
        NegotiatorAttributes(
          (XMLNode _, {String? redact}) {},
-          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true, allowPlainAuth: true),
+          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true),
          (_) async {},
          getNegotiatorNullStub,
          getManagerNullStub,
@ -186,7 +186,7 @@ void main() {
      negotiator.register(
        NegotiatorAttributes(
          (XMLNode _, {String? redact}) {},
-          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true, allowPlainAuth: true),
+          () => ConnectionSettings(jid: JID.fromString('user@server'), password: 'pencil', useDirectTLS: true),
          (_) async {},
          getNegotiatorNullStub,
          getManagerNullStub,
--- a/packages/moxxmpp/test/xeps/xep_0030_test.dart
+++ b/packages/moxxmpp/test/xeps/xep_0030_test.dart
@ -10,7 +10,7 @@ void main() {

  test('Test having multiple disco requests for the same JID', () async {
    final fakeSocket = StubTCPSocket(
-      play: [
+      [
        StringExpectation(
          "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
          '''
@ -78,7 +78,6 @@ void main() {
        jid: JID.fromString('polynomdivision@test.server'),
        password: 'aaaa',
        useDirectTLS: true,
-        allowPlainAuth: true,
    ),);
    conn.registerManagers([
      PresenceManager(),
--- a/packages/moxxmpp/test/xeps/xep_0198_test.dart
+++ b/packages/moxxmpp/test/xeps/xep_0198_test.dart
@ -29,12 +29,11 @@ XmppManagerAttributes mkAttributes(void Function(Stanza) callback) {
      jid: JID.fromString('hallo@example.server'),
      password: 'password',
      useDirectTLS: true,
-      allowPlainAuth: false,
    ),
    isFeatureSupported: (_) => false,
    getFullJID: () => JID.fromString('hallo@example.server/uwu'),
-    getSocket: () => StubTCPSocket(play: []),
-    getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+    getSocket: () => StubTCPSocket([]),
+    getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
    getNegotiatorById: getNegotiatorNullStub,
  );
 }
@ -180,7 +179,7 @@ void main() {

    test('Test counting incoming stanzas for which handlers end early', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -242,7 +241,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      final sm = StreamManagementManager();
      conn.registerManagers([
@ -297,7 +295,7 @@ void main() {

    test('Test counting incoming stanzas that are awaited', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -369,7 +367,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      final sm = StreamManagementManager();
      conn.registerManagers([
@ -467,7 +464,7 @@ void main() {
  group('Test the negotiator', () {
    test('Test successful stream enablement', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -529,7 +526,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      conn.registerManagers([
          PresenceManager(),
@ -559,7 +555,7 @@ void main() {

    test('Test a failed stream resumption', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -625,7 +621,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      conn.registerManagers([
          PresenceManager(),
@ -664,7 +659,7 @@ void main() {

    test('Test a successful stream resumption', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -721,7 +716,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      conn.registerManagers([
          PresenceManager(),
--- a/packages/moxxmpp/test/xeps/xep_0280_test.dart
+++ b/packages/moxxmpp/test/xeps/xep_0280_test.dart
@ -17,12 +17,11 @@ void main() {
        jid: JID.fromString('bob@xmpp.example'),
        password: 'password',
        useDirectTLS: true,
-        allowPlainAuth: false,
      ),
      isFeatureSupported: (_) => false,
      getFullJID: () => JID.fromString('bob@xmpp.example/uwu'),
-      getSocket: () => StubTCPSocket(play: []),
-      getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+      getSocket: () => StubTCPSocket([]),
+      getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
      getNegotiatorById: getNegotiatorNullStub,
    );
    final manager = CarbonsManager();
--- a/packages/moxxmpp/test/xeps/xep_0352_test.dart
+++ b/packages/moxxmpp/test/xeps/xep_0352_test.dart
@ -43,14 +43,13 @@ void main() {
            jid: JID.fromString('some.user@example.server'),
            password: 'password',
            useDirectTLS: true,
-            allowPlainAuth: false,
          ),
          getManagerById: getManagerNullStub,
          getNegotiatorById: getUnsupportedCSINegotiator,
          isFeatureSupported: (_) => false,
          getFullJID: () => JID.fromString('some.user@example.server/aaaaa'),
-          getSocket: () => StubTCPSocket(play: []),
-          getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+          getSocket: () => StubTCPSocket([]),
+          getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
        ),
      );

@ -72,14 +71,13 @@ void main() {
            jid: JID.fromString('some.user@example.server'),
            password: 'password',
            useDirectTLS: true,
-            allowPlainAuth: false,
          ),
          getManagerById: getManagerNullStub,
          getNegotiatorById: getSupportedCSINegotiator,
          isFeatureSupported: (_) => false,
          getFullJID: () => JID.fromString('some.user@example.server/aaaaa'),
-          getSocket: () => StubTCPSocket(play: []),
-          getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+          getSocket: () => StubTCPSocket([]),
+          getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
        ),
      );

--- a/packages/moxxmpp/test/xmpp_test.dart
+++ b/packages/moxxmpp/test/xmpp_test.dart
@ -18,14 +18,13 @@ Future<bool> testRosterManager(String bareJid, String resource, String stanzaStr
        jid: JID.fromString(bareJid),
        password: 'password',
        useDirectTLS: true,
-        allowPlainAuth: false,
      ),
      getManagerById: getManagerNullStub,
      getNegotiatorById: getNegotiatorNullStub,
      isFeatureSupported: (_) => false,
      getFullJID: () => JID.fromString('$bareJid/$resource'),
-      getSocket: () => StubTCPSocket(play: []),
-      getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+      getSocket: () => StubTCPSocket([]),
+      getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
  ),);

  final stanza = Stanza.fromXMLNode(XMLNode.fromString(stanzaString));
@ -41,7 +40,7 @@ void main() {

  test('Test a successful login attempt with no SM', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -121,12 +120,12 @@ void main() {
      final XmppConnection conn = XmppConnection(
        TestingReconnectionPolicy(),
        AlwaysConnectedConnectivityManager(),
-        fakeSocket);
+        fakeSocket,
+      );
      conn.setConnectionSettings(ConnectionSettings(
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      conn.registerManagers([
        PresenceManager(),
@ -153,7 +152,7 @@ void main() {

  test('Test a failed SASL auth', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -185,7 +184,6 @@ void main() {
        jid: JID.fromString('polynomdivision@test.server'),
        password: 'aaaa',
        useDirectTLS: true,
-        allowPlainAuth: true,
      ),);
      conn.registerManagers([
        PresenceManager(),
@ -212,7 +210,7 @@ void main() {

  test('Test another failed SASL auth', () async {
      final fakeSocket = StubTCPSocket(
-        play: [
+        [
          StringExpectation(
            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='test.server' xml:lang='en'>",
            '''
@ -244,7 +242,6 @@ void main() {
          jid: JID.fromString('polynomdivision@test.server'),
          password: 'aaaa',
          useDirectTLS: true,
-          allowPlainAuth: true,
      ),);
      conn.registerManagers([
        PresenceManager(),
@ -300,7 +297,6 @@ void main() {
        jid: JID.fromString('polynomdivision@test.server'),
        password: 'aaaa',
        useDirectTLS: true,
-        allowPlainAuth: false,
      ),);
      conn.registerManagers([
        PresenceManager('http://moxxmpp.example'),
@ -333,14 +329,13 @@ void main() {
                jid: JID.fromString('some.user@example.server'),
                password: 'password',
                useDirectTLS: true,
-                allowPlainAuth: false,
              ),
              getManagerById: getManagerNullStub,
              getNegotiatorById: getNegotiatorNullStub,
              isFeatureSupported: (_) => false,
              getFullJID: () => JID.fromString('some.user@example.server/aaaaa'),
-              getSocket: () => StubTCPSocket(play: []),
-              getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket(play: [])),
+              getSocket: () => StubTCPSocket([]),
+              getConnection: () => XmppConnection(TestingReconnectionPolicy(), AlwaysConnectedConnectivityManager(), StubTCPSocket([])),
          ),);

          // NOTE: Based on https://gultsch.de/gajim_roster_push_and_message_interception.html
@ -365,4 +360,57 @@ void main() {
          expect(result2, true, reason: 'Roster pushes should be accepted if the bare JIDs are the same');
      });
  });
+
+  test('Test failing due to the server only allowing SASL PLAIN', () async {
+      final fakeSocket = StubTCPSocket(
+        [
+          StringExpectation(
+            "<stream:stream xmlns='jabber:client' version='1.0' xmlns:stream='http://etherx.jabber.org/streams' to='example.org' xml:lang='en'>",
+            '''
+<stream:stream
+    xmlns="jabber:client"
+    version="1.0"
+    xmlns:stream="http://etherx.jabber.org/streams"
+    from="test.server"
+    xml:lang="en">
+  <stream:features xmlns="http://etherx.jabber.org/streams">
+    <mechanisms xmlns="urn:ietf:params:xml:ns:xmpp-sasl">
+      <mechanism>PLAIN</mechanism>
+    </mechanisms>
+  </stream:features>''',
+          ),
+        ],
+      );
+
+      final conn = XmppConnection(
+        TestingReconnectionPolicy(),
+        AlwaysConnectedConnectivityManager(),
+        fakeSocket,
+      );
+      conn.registerManagers([
+        PresenceManager(),
+        RosterManager(TestingRosterStateManager('', [])),
+        DiscoManager([]),
+        PingManager(),
+      ]);
+      conn.registerFeatureNegotiators(
+        [
+          // SaslPlainNegotiator(),
+          ResourceBindingNegotiator(),
+        ]
+      );
+      conn.setConnectionSettings(
+        ConnectionSettings(
+          jid: JID.fromString('testuser@example.org'),
+          password: 'abc123',
+          useDirectTLS: false,
+        ),
+      );
+
+      final result = await conn.connect(
+        waitUntilLogin: true,
+      );
+
+      expect(result.isType<NoMatchingAuthenticationMechanismAvailableError>(), true);
+  });
 }