From de85bf848d697d1be20e52e0e2fdd863dbc0c7b3 Mon Sep 17 00:00:00 2001 From: "Alexander \"PapaTutuWawa" Date: Sat, 18 Mar 2023 14:54:39 +0100 Subject: [PATCH] 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. --- packages/moxxmpp/CHANGELOG.md | 1 + packages/moxxmpp/lib/moxxmpp.dart | 1 + packages/moxxmpp/lib/src/connection.dart | 25 +++++- .../moxxmpp/lib/src/connection_errors.dart | 32 +++++++- .../lib/src/negotiators/sasl/plain.dart | 2 - packages/moxxmpp/lib/src/settings.dart | 2 - packages/moxxmpp/test/helpers/manager.dart | 1 - packages/moxxmpp/test/helpers/xmpp.dart | 3 - packages/moxxmpp/test/negotiator_test.dart | 3 +- packages/moxxmpp/test/sasl/scram_test.dart | 12 +-- packages/moxxmpp/test/xeps/xep_0030_test.dart | 3 +- packages/moxxmpp/test/xeps/xep_0198_test.dart | 20 ++--- packages/moxxmpp/test/xeps/xep_0280_test.dart | 5 +- packages/moxxmpp/test/xeps/xep_0352_test.dart | 10 +-- packages/moxxmpp/test/xmpp_test.dart | 76 +++++++++++++++---- 15 files changed, 135 insertions(+), 61 deletions(-) diff --git a/packages/moxxmpp/CHANGELOG.md b/packages/moxxmpp/CHANGELOG.md index 2df65a2..c74b98b 100644 --- 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 diff --git a/packages/moxxmpp/lib/moxxmpp.dart b/packages/moxxmpp/lib/moxxmpp.dart index caa1cee..e8583ad 100644 --- 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'; diff --git a/packages/moxxmpp/lib/src/connection.dart b/packages/moxxmpp/lib/src/connection.dart index b9a12c2..33eb578 100644 --- 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 _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()) { _log.severe('Negotiator returned an error'); diff --git a/packages/moxxmpp/lib/src/connection_errors.dart b/packages/moxxmpp/lib/src/connection_errors.dart index 304dee2..f0db272 100644 --- 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; +} diff --git a/packages/moxxmpp/lib/src/negotiators/sasl/plain.dart b/packages/moxxmpp/lib/src/negotiators/sasl/plain.dart index dc56728..8ee4512 100644 --- 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 features) { - if (!attributes.getConnectionSettings().allowPlainAuth) return false; - if (super.matchesFeature(features)) { if (!attributes.getSocket().isSecure()) { _log.warning( diff --git a/packages/moxxmpp/lib/src/settings.dart b/packages/moxxmpp/lib/src/settings.dart index 9547e02..4368b14 100644 --- 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; } diff --git a/packages/moxxmpp/test/helpers/manager.dart b/packages/moxxmpp/test/helpers/manager.dart index 3466109..17706ea 100644 --- 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 _sendStanza( diff --git a/packages/moxxmpp/test/helpers/xmpp.dart b/packages/moxxmpp/test/helpers/xmpp.dart index b120b23..c096fb7 100644 --- 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 buildAuthenticatedPlay(ConnectionSettings settings) { - assert(settings.allowPlainAuth, 'SASL PLAIN must be allowed'); - final plain = base64.encode( utf8.encode('\u0000${settings.jid.local}\u0000${settings.password}'), ); diff --git a/packages/moxxmpp/test/negotiator_test.dart b/packages/moxxmpp/test/negotiator_test.dart index 37be7d1..5f1de3f 100644 --- 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( "", ''' @@ -74,7 +74,6 @@ void main() { jid: JID.fromString('user@test.server'), password: 'abc123', useDirectTLS: true, - allowPlainAuth: false, ), ); final features = [ diff --git a/packages/moxxmpp/test/sasl/scram_test.dart b/packages/moxxmpp/test/sasl/scram_test.dart index 4e15309..13c266b 100644 --- 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, diff --git a/packages/moxxmpp/test/xeps/xep_0030_test.dart b/packages/moxxmpp/test/xeps/xep_0030_test.dart index 938e32d..19826c8 100644 --- 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( "", ''' @@ -78,7 +78,6 @@ void main() { jid: JID.fromString('polynomdivision@test.server'), password: 'aaaa', useDirectTLS: true, - allowPlainAuth: true, ),); conn.registerManagers([ PresenceManager(), diff --git a/packages/moxxmpp/test/xeps/xep_0198_test.dart b/packages/moxxmpp/test/xeps/xep_0198_test.dart index 17ddd78..888daab 100644 --- 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( "", ''' @@ -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( "", ''' @@ -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( "", ''' @@ -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( "", ''' @@ -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( "", ''' @@ -721,7 +716,6 @@ void main() { jid: JID.fromString('polynomdivision@test.server'), password: 'aaaa', useDirectTLS: true, - allowPlainAuth: true, ),); conn.registerManagers([ PresenceManager(), diff --git a/packages/moxxmpp/test/xeps/xep_0280_test.dart b/packages/moxxmpp/test/xeps/xep_0280_test.dart index 8218c27..1426745 100644 --- 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(); diff --git a/packages/moxxmpp/test/xeps/xep_0352_test.dart b/packages/moxxmpp/test/xeps/xep_0352_test.dart index 49fd076..70240a5 100644 --- 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([])), ), ); diff --git a/packages/moxxmpp/test/xmpp_test.dart b/packages/moxxmpp/test/xmpp_test.dart index b70cfc3..274b81a 100644 --- a/packages/moxxmpp/test/xmpp_test.dart +++ b/packages/moxxmpp/test/xmpp_test.dart @@ -18,14 +18,13 @@ Future 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( "", ''' @@ -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( "", ''' @@ -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( "", ''' @@ -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( + "", + ''' + + + + PLAIN + + ''', + ), + ], + ); + + 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(), true); + }); }