From d6edea8ddfb4f9b5663da3c33813eacc35a3f615 Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Sat, 18 Feb 2023 09:55:17 +0100 Subject: [PATCH] avoid accounts being connected multiple times the pool should not be asked to connect a specific account it should only be called to do a full reconfiguration --- .../android/database/model/Account.java | 13 ++-- .../android/database/model/Credential.java | 8 +++ .../android/repository/AccountRepository.java | 33 ++++++++-- .../android/ui/model/SetupViewModel.java | 61 +++++++++++++------ .../android/xmpp/ConnectionPool.java | 22 ++++--- .../android/xmpp/XmppConnection.java | 9 ++- 6 files changed, 105 insertions(+), 41 deletions(-) diff --git a/app/src/main/java/im/conversations/android/database/model/Account.java b/app/src/main/java/im/conversations/android/database/model/Account.java index a534eb8d0..2398552b6 100644 --- a/app/src/main/java/im/conversations/android/database/model/Account.java +++ b/app/src/main/java/im/conversations/android/database/model/Account.java @@ -20,11 +20,14 @@ public class Account { @Override public String toString() { - return "Account{" + - "id=" + id + - ", address=" + address + - ", randomSeed=" + Arrays.toString(randomSeed) + - '}'; + return "Account{" + + "id=" + + id + + ", address=" + + address + + ", randomSeed=" + + Arrays.toString(randomSeed) + + '}'; } public Account(final long id, @NonNull final BareJid address, @NonNull byte[] randomSeed) { diff --git a/app/src/main/java/im/conversations/android/database/model/Credential.java b/app/src/main/java/im/conversations/android/database/model/Credential.java index aeefcb747..055413c62 100644 --- a/app/src/main/java/im/conversations/android/database/model/Credential.java +++ b/app/src/main/java/im/conversations/android/database/model/Credential.java @@ -1,5 +1,7 @@ package im.conversations.android.database.model; +import java.util.Objects; + public class Credential { public final String password; @@ -44,6 +46,12 @@ public class Credential { this.privateKeyAlias = privateKeyAlias; } + public boolean isEmpty() { + return Objects.isNull(this.password) + && Objects.isNull(this.fastToken) + && Objects.isNull(this.privateKeyAlias); + } + public static Credential empty() { return new Credential(); } diff --git a/app/src/main/java/im/conversations/android/repository/AccountRepository.java b/app/src/main/java/im/conversations/android/repository/AccountRepository.java index 4a4556ec6..dadf99859 100644 --- a/app/src/main/java/im/conversations/android/repository/AccountRepository.java +++ b/app/src/main/java/im/conversations/android/repository/AccountRepository.java @@ -39,13 +39,21 @@ public class AccountRepository extends AbstractRepository { if (password != null) { CredentialStore.getInstance(context).setPassword(account, password); } - ConnectionPool.getInstance(context).reconfigure(); return account; } public ListenableFuture createAccountAsync( final @NonNull BareJid address, final String password, final boolean loginAndBind) { - return Futures.submit(() -> createAccount(address, password, loginAndBind), IO_EXECUTOR); + final var creationFuture = + Futures.submit(() -> createAccount(address, password, loginAndBind), IO_EXECUTOR); + return Futures.transformAsync( + creationFuture, + account -> + Futures.transform( + ConnectionPool.getInstance(context).reconfigure(), + v -> account, + MoreExecutors.directExecutor()), + MoreExecutors.directExecutor()); } public ListenableFuture createAccountAsync( @@ -62,16 +70,25 @@ public class AccountRepository extends AbstractRepository { MoreExecutors.directExecutor()); } - public ListenableFuture deleteAccountAsync(@NonNull Account account) { + public ListenableFuture deleteAccountAsync(@NonNull Account account) { return Futures.submit(() -> deleteAccount(account), IO_EXECUTOR); } - private Boolean deleteAccount(@NonNull Account account) { - return database.accountDao().delete(account.id) > 0; + private Void deleteAccount(@NonNull Account account) { + database.accountDao().delete(account.id); + ConnectionPool.getInstance(context).reconfigure(); + return null; } public ListenableFuture getConnectedFuture(@NonNull final Account account) { - return ConnectionPool.getInstance(context).get(account).asConnectedFuture(); + final var optional = ConnectionPool.getInstance(context).get(account); + if (optional.isPresent()) { + return optional.get().asConnectedFuture(); + } else { + return Futures.immediateFailedFuture( + new IllegalStateException( + String.format("Account %s is not configured", account.address))); + } } public ListenableFuture setPasswordAsync( @@ -86,6 +103,10 @@ public class AccountRepository extends AbstractRepository { return account; } + public void reconnect(final Account account) { + ConnectionPool.getInstance(context).reconnect(account); + } + public static class AccountAlreadyExistsException extends IllegalStateException { public AccountAlreadyExistsException(BareJid address) { super(String.format("The account %s has already been setup", address)); diff --git a/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java b/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java index 12fc7d48e..3665185f9 100644 --- a/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java +++ b/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java @@ -74,6 +74,7 @@ public class SetupViewModel extends AndroidViewModel { } public boolean submitXmppAddress() { + final var account = this.account; final var userInput = Strings.nullToEmpty(this.xmppAddress.getValue()).trim(); if (userInput.isEmpty()) { this.xmppAddressError.postValue( @@ -88,14 +89,28 @@ public class SetupViewModel extends AndroidViewModel { return true; } - // TODO do we already have an account in this viewModel? is it the same? if so go to that - // one with the next step + if (account != null) { + if (account.address.equals(address)) { + this.accountRepository.reconnect(account); + decideNextStep(Target.ENTER_ADDRESS, account); + return true; + } else { + this.account = null; + this.accountRepository.deleteAccountAsync(account); + } + } + createAccount(address); + return true; + } - final String password = this.password.getValue(); + private void createAccount(final BareJid address) { + + // if the user hasn't entered anything we want this to be null so we don't store credentials + final String password = Strings.emptyToNull(this.password.getValue()); // post parsed/normalized jid back into UI this.xmppAddress.postValue(address.toString()); - this.loading.postValue(true); final var creationFuture = this.accountRepository.createAccountAsync(address, password); + this.setCurrentOperation(creationFuture); Futures.addCallback( creationFuture, new FutureCallback<>() { @@ -116,7 +131,11 @@ public class SetupViewModel extends AndroidViewModel { } }, MoreExecutors.directExecutor()); - return true; + } + + private void setCurrentOperation(ListenableFuture currentOperation) { + this.loading.postValue(true); + this.currentOperation = currentOperation; } private void setAccount(@NonNull final Account account) { @@ -125,9 +144,9 @@ public class SetupViewModel extends AndroidViewModel { } private void decideNextStep(final Target current, @NonNull final Account account) { - LOGGER.info("Get connected future for {}", account.address); final ListenableFuture connectedFuture = this.accountRepository.getConnectedFuture(account); + this.setCurrentOperation(connectedFuture); Futures.addCallback( connectedFuture, new FutureCallback<>() { @@ -135,6 +154,8 @@ public class SetupViewModel extends AndroidViewModel { public void onSuccess(final XmppConnection result) { // TODO only when configured for loginAndBind LOGGER.info("Account setup successful"); + SetupViewModel.this.account = null; + redirect(Target.DONE); } @Override @@ -197,19 +218,22 @@ public class SetupViewModel extends AndroidViewModel { } final String password = Strings.nullToEmpty(this.password.getValue()); final var setPasswordFuture = this.accountRepository.setPasswordAsync(account, password); - this.loading.postValue(true); - Futures.addCallback(setPasswordFuture, new FutureCallback<>() { - @Override - public void onSuccess(final Account account) { - decideNextStep(Target.ENTER_PASSWORD, account); - } + this.setCurrentOperation(setPasswordFuture); + Futures.addCallback( + setPasswordFuture, + new FutureCallback<>() { + @Override + public void onSuccess(final Account account) { + decideNextStep(Target.ENTER_PASSWORD, account); + } - @Override - public void onFailure(@NonNull Throwable throwable) { - // TODO show some sort of error message - loading.postValue(false); - } - },MoreExecutors.directExecutor()); + @Override + public void onFailure(@NonNull Throwable throwable) { + // TODO show some sort of error message + loading.postValue(false); + } + }, + MoreExecutors.directExecutor()); return true; } @@ -224,6 +248,7 @@ public class SetupViewModel extends AndroidViewModel { public void cancelSetup() { final var account = this.account; if (account != null) { + this.account = null; this.accountRepository.deleteAccountAsync(account); } } diff --git a/app/src/main/java/im/conversations/android/xmpp/ConnectionPool.java b/app/src/main/java/im/conversations/android/xmpp/ConnectionPool.java index ed6a5c609..6409aca8d 100644 --- a/app/src/main/java/im/conversations/android/xmpp/ConnectionPool.java +++ b/app/src/main/java/im/conversations/android/xmpp/ConnectionPool.java @@ -3,7 +3,6 @@ package im.conversations.android.xmpp; import android.content.Context; import android.os.SystemClock; import com.google.common.base.Optional; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -62,7 +61,7 @@ public class ConnectionPool { reconfigurationExecutor); } - public synchronized XmppConnection get(final Account account) { + private synchronized XmppConnection reconfigure(final Account account) { final Optional xmppConnectionOptional = Iterables.tryFind(this.connections, c -> c.getAccount().equals(account)); if (xmppConnectionOptional.isPresent()) { @@ -71,13 +70,19 @@ public class ConnectionPool { return setupXmppConnection(context, account); } + public synchronized Optional get(final Account account) { + return Iterables.tryFind(this.connections, c -> c.getAccount().equals(account)); + } + public synchronized void reconnect(final Account account) { - final Optional xmppConnectionOptional = - Iterables.tryFind(this.connections, c -> c.getAccount().equals(account)); + final Optional xmppConnectionOptional = get(account); if (xmppConnectionOptional.isPresent()) { reconnectAccount(xmppConnectionOptional.get()); } else { - setupXmppConnection(context, account); + throw new IllegalStateException( + String.format( + "Attempted to reconnect %s but the account was not configured", + account.address)); } } @@ -95,7 +100,7 @@ public class ConnectionPool { String.format( "No enabled account with address %s", address.toString())); } - return get(account); + return reconfigure(account); }, reconfigurationExecutor); } @@ -112,7 +117,7 @@ public class ConnectionPool { throw new IllegalStateException( String.format("No enabled account with id %d", id)); } - return get(account); + return reconfigure(account); }, reconfigurationExecutor); } @@ -121,7 +126,6 @@ public class ConnectionPool { return Iterables.any(this.connections, c -> id == c.getAccount().id); } - private synchronized Void reconfigure(final Set accounts) { final Set current = getAccounts(); final Set removed = Sets.difference(current, accounts); @@ -356,7 +360,7 @@ public class ConnectionPool { } private XmppConnection setupXmppConnection(final Context context, final Account account) { - LOGGER.info("Setting up XMPP connection for {}",account.address); + LOGGER.info("Setting up XMPP connection for {}", account.address); final XmppConnection xmppConnection = new XmppConnection(context, account); this.connections.add(xmppConnection); xmppConnection.setOnStatusChangedListener(this::onStatusChanged); diff --git a/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java b/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java index 99ad0320a..ac5847bfc 100644 --- a/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java +++ b/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java @@ -1238,9 +1238,12 @@ public class XmppConnection implements Runnable { final Element cbElement = this.streamFeatures.findChild("sasl-channel-binding", Namespace.CHANNEL_BINDING); final Collection channelBindings = ChannelBinding.of(cbElement); - final SaslMechanism.Factory saslFactory = - new SaslMechanism.Factory( - account, CredentialStore.getInstance(context).get(account)); + final var credential = CredentialStore.getInstance(context).get(account); + if (credential.isEmpty()) { + LOGGER.warn("No credentials configured. Going to bail out before actual attempt."); + throw new StateChangingException(ConnectionState.UNAUTHORIZED); + } + final SaslMechanism.Factory saslFactory = new SaslMechanism.Factory(account, credential); final SaslMechanism saslMechanism = saslFactory.of( mechanisms, channelBindings, version, SSLSockets.version(this.socket));