From 1546a05568f5a097fbe1605225c944a108583993 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 12 Feb 2020 10:26:14 +0000 Subject: [PATCH 1/3] Catch exception if hardware-backed key can't be loaded. --- .../briarproject/briar/android/AndroidKeyStrengthener.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java index db44062f9..f0c7d6a26 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AndroidKeyStrengthener.java @@ -28,7 +28,9 @@ import static android.security.keystore.KeyProperties.PURPOSE_SIGN; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; import static java.util.logging.Level.INFO; +import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; +import static org.briarproject.bramble.util.LogUtils.logException; @RequiresApi(23) @NotNullByDefault @@ -79,7 +81,10 @@ class AndroidKeyStrengthener implements KeyStrengthener { return true; } return false; - } catch (GeneralSecurityException | IOException e) { + } catch (GeneralSecurityException e) { + logException(LOG, WARNING, e); + return false; + } catch (IOException e) { throw new RuntimeException(e); } } From ed50582e27f7638ee54c654f668419d8efb53a65 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Tue, 25 Feb 2020 14:56:32 +0000 Subject: [PATCH 2/3] Show a dialog if the DB key can't be decrypted due to a keystore error. --- .../bramble/api/account/AccountManager.java | 13 ++- .../bramble/api/crypto/CryptoComponent.java | 9 +- .../api/crypto/DecryptionException.java | 17 +++ .../bramble/api/crypto/DecryptionResult.java | 29 +++++ .../bramble/account/AccountManagerImpl.java | 29 +++-- .../bramble/crypto/CryptoComponentImpl.java | 28 +++-- .../account/AccountManagerImplTest.java | 55 ++++++--- .../crypto/PasswordBasedEncryptionTest.java | 106 +++++++++++++++--- .../briarproject/briar/android/AppModule.java | 7 +- .../android/activity/ActivityModule.java | 10 -- .../android/login/ChangePasswordActivity.java | 72 +++++++----- .../login/ChangePasswordControllerImpl.java | 43 ------- .../login/ChangePasswordViewModel.java | 53 +++++++++ .../briar/android/login/LoginModule.java | 23 ++++ .../briar/android/login/LoginUtils.java | 30 +++++ .../briar/android/login/PasswordFragment.java | 29 +++-- .../briar/android/login/StartupViewModel.java | 17 ++- .../briar/android/util/UiUtils.java | 3 +- .../android/viewmodel/ViewModelModule.java | 6 - briar-android/src/main/res/values/strings.xml | 2 + .../login/ChangePasswordActivityTest.java | 82 ++++++-------- .../ChangePasswordControllerImplTest.java | 58 ---------- .../login/TestChangePasswordActivity.java | 14 --- .../briar/headless/BriarService.kt | 5 +- .../briar/headless/BriarTestServiceImpl.kt | 5 +- 25 files changed, 452 insertions(+), 293 deletions(-) create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionException.java create mode 100644 bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionResult.java delete mode 100644 briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordControllerImpl.java create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordViewModel.java create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/login/LoginModule.java create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/login/LoginUtils.java delete mode 100644 briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordControllerImplTest.java delete mode 100644 briar-android/src/test/java/org/briarproject/briar/android/login/TestChangePasswordActivity.java diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/account/AccountManager.java b/bramble-api/src/main/java/org/briarproject/bramble/api/account/AccountManager.java index 2e1b5a951..eb69c8361 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/account/AccountManager.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/account/AccountManager.java @@ -1,5 +1,6 @@ package org.briarproject.bramble.api.account; +import org.briarproject.bramble.api.crypto.DecryptionException; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.identity.IdentityManager; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -54,17 +55,19 @@ public interface AccountManager { * Loads the encrypted database key from disk and decrypts it with the * given password. * - * @return true if the database key was successfully loaded and decrypted. + * @throws DecryptionException If the database key could not be loaded and + * decrypted. */ - boolean signIn(String password); + void signIn(String password) throws DecryptionException; /** * Loads the encrypted database key from disk, decrypts it with the old * password, encrypts it with the new password, and stores it on disk, * replacing the old key. * - * @return true if the database key was successfully loaded, re-encrypted - * and stored. + * @throws DecryptionException If the database key could not be loaded and + * decrypted. */ - boolean changePassword(String oldPassword, String newPassword); + void changePassword(String oldPassword, String newPassword) + throws DecryptionException; } diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java index d1cbc63da..9c3a42514 100644 --- a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/CryptoComponent.java @@ -142,16 +142,17 @@ public interface CryptoComponent { /** * Decrypts and authenticates the given ciphertext that has been read from * storage. The encryption and authentication keys are derived from the - * given password. Returns null if the ciphertext cannot be decrypted and - * authenticated (for example, if the password is wrong). + * given password. * * @param keyStrengthener Used to strengthen the password-based key. If * null, or if strengthening was not used when encrypting the ciphertext, * the password-based key will not be strengthened + * @throws DecryptionException If the ciphertext cannot be decrypted and + * authenticated (for example, if the password is wrong). */ - @Nullable byte[] decryptWithPassword(byte[] ciphertext, String password, - @Nullable KeyStrengthener keyStrengthener); + @Nullable KeyStrengthener keyStrengthener) + throws DecryptionException; /** * Returns true if the given ciphertext was encrypted using a strengthened diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionException.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionException.java new file mode 100644 index 000000000..9a2a5333d --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionException.java @@ -0,0 +1,17 @@ +package org.briarproject.bramble.api.crypto; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +@NotNullByDefault +public class DecryptionException extends Exception { + + private final DecryptionResult result; + + public DecryptionException(DecryptionResult result) { + this.result = result; + } + + public DecryptionResult getDecryptionResult() { + return result; + } +} diff --git a/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionResult.java b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionResult.java new file mode 100644 index 000000000..c2a70261b --- /dev/null +++ b/bramble-api/src/main/java/org/briarproject/bramble/api/crypto/DecryptionResult.java @@ -0,0 +1,29 @@ +package org.briarproject.bramble.api.crypto; + +/** + * The result of a password-based decryption operation. + */ +public enum DecryptionResult { + + /** + * Decryption succeeded. + */ + SUCCESS, + + /** + * Decryption failed because the format of the ciphertext was invalid. + */ + INVALID_CIPHERTEXT, + + /** + * Decryption failed because the {@link KeyStrengthener} used for + * encryption was not available for decryption. + */ + KEY_STRENGTHENER_ERROR, + + /** + * Decryption failed because the password used for decryption did not match + * the password used for encryption. + */ + INVALID_PASSWORD +} diff --git a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java index c89353fa9..bef5eada6 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/account/AccountManagerImpl.java @@ -2,6 +2,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.account.AccountManager; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.DecryptionException; import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; @@ -17,6 +18,7 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStreamReader; +import java.nio.charset.Charset; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -24,6 +26,7 @@ import javax.annotation.concurrent.GuardedBy; import javax.inject.Inject; import static java.util.logging.Level.WARNING; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT; import static org.briarproject.bramble.util.LogUtils.logException; import static org.briarproject.bramble.util.StringUtils.fromHexString; import static org.briarproject.bramble.util.StringUtils.toHexString; @@ -95,7 +98,7 @@ class AccountManagerImpl implements AccountManager { } try { BufferedReader reader = new BufferedReader(new InputStreamReader( - new FileInputStream(f), "UTF-8")); + new FileInputStream(f), Charset.forName("UTF-8"))); String key = reader.readLine(); reader.close(); return key; @@ -147,7 +150,7 @@ class AccountManagerImpl implements AccountManager { @GuardedBy("stateChangeLock") private void writeDbKeyToFile(String key, File f) throws IOException { FileOutputStream out = new FileOutputStream(f); - out.write(key.getBytes("UTF-8")); + out.write(key.getBytes(Charset.forName("UTF-8"))); out.flush(); out.close(); } @@ -193,31 +196,24 @@ class AccountManagerImpl implements AccountManager { } @Override - public boolean signIn(String password) { + public void signIn(String password) throws DecryptionException { synchronized (stateChangeLock) { - SecretKey key = loadAndDecryptDatabaseKey(password); - if (key == null) return false; - databaseKey = key; - return true; + databaseKey = loadAndDecryptDatabaseKey(password); } } @GuardedBy("stateChangeLock") - @Nullable - private SecretKey loadAndDecryptDatabaseKey(String password) { + private SecretKey loadAndDecryptDatabaseKey(String password) + throws DecryptionException { String hex = loadEncryptedDatabaseKey(); if (hex == null) { LOG.warning("Failed to load encrypted database key"); - return null; + throw new DecryptionException(INVALID_CIPHERTEXT); } byte[] ciphertext = fromHexString(hex); KeyStrengthener keyStrengthener = databaseConfig.getKeyStrengthener(); byte[] plaintext = crypto.decryptWithPassword(ciphertext, password, keyStrengthener); - if (plaintext == null) { - LOG.info("Failed to decrypt database key"); - return null; - } SecretKey key = new SecretKey(plaintext); // If the DB key was encrypted with a weak key and a key strengthener // is now available, re-encrypt the DB key with a strengthened key @@ -230,10 +226,11 @@ class AccountManagerImpl implements AccountManager { } @Override - public boolean changePassword(String oldPassword, String newPassword) { + public void changePassword(String oldPassword, String newPassword) + throws DecryptionException { synchronized (stateChangeLock) { SecretKey key = loadAndDecryptDatabaseKey(oldPassword); - return key != null && encryptAndStoreDatabaseKey(key, newPassword); + encryptAndStoreDatabaseKey(key, newPassword); } } } diff --git a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java index 6763193ac..7a30b7ce6 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/crypto/CryptoComponentImpl.java @@ -7,6 +7,7 @@ import net.i2p.crypto.eddsa.KeyPairGenerator; import org.briarproject.bramble.api.crypto.AgreementPrivateKey; import org.briarproject.bramble.api.crypto.AgreementPublicKey; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.DecryptionException; import org.briarproject.bramble.api.crypto.KeyPair; import org.briarproject.bramble.api.crypto.KeyParser; import org.briarproject.bramble.api.crypto.KeyStrengthener; @@ -39,6 +40,9 @@ import static java.lang.System.arraycopy; import static java.util.logging.Level.INFO; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_AGREEMENT; import static org.briarproject.bramble.api.crypto.CryptoConstants.KEY_TYPE_SIGNATURE; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD; +import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR; import static org.briarproject.bramble.util.ByteUtils.INT_32_BYTES; import static org.briarproject.bramble.util.LogUtils.logDuration; import static org.briarproject.bramble.util.LogUtils.now; @@ -359,16 +363,17 @@ class CryptoComponentImpl implements CryptoComponent { } @Override - @Nullable public byte[] decryptWithPassword(byte[] input, String password, - @Nullable KeyStrengthener keyStrengthener) { + @Nullable KeyStrengthener keyStrengthener) + throws DecryptionException { AuthenticatedCipher cipher = new XSalsa20Poly1305AuthenticatedCipher(); int macBytes = cipher.getMacBytes(); // The input contains the format version, salt, cost parameter, IV, // ciphertext and MAC if (input.length < 1 + PBKDF_SALT_BYTES + INT_32_BYTES - + STORAGE_IV_BYTES + macBytes) - return null; // Invalid input + + STORAGE_IV_BYTES + macBytes) { + throw new DecryptionException(INVALID_CIPHERTEXT); + } int inputOff = 0; // Format version byte formatVersion = input[inputOff]; @@ -376,7 +381,7 @@ class CryptoComponentImpl implements CryptoComponent { // Check whether we support this format version if (formatVersion != PBKDF_FORMAT_SCRYPT && formatVersion != PBKDF_FORMAT_SCRYPT_STRENGTHENED) { - return null; + throw new DecryptionException(INVALID_CIPHERTEXT); } // Salt byte[] salt = new byte[PBKDF_SALT_BYTES]; @@ -385,8 +390,9 @@ class CryptoComponentImpl implements CryptoComponent { // Cost parameter long cost = ByteUtils.readUint32(input, inputOff); inputOff += INT_32_BYTES; - if (cost < 2 || cost > Integer.MAX_VALUE) - return null; // Invalid cost parameter + if (cost < 2 || cost > Integer.MAX_VALUE) { + throw new DecryptionException(INVALID_CIPHERTEXT); + } // IV byte[] iv = new byte[STORAGE_IV_BYTES]; arraycopy(input, inputOff, iv, 0, iv.length); @@ -394,8 +400,10 @@ class CryptoComponentImpl implements CryptoComponent { // Derive the decryption key from the password SecretKey key = passwordBasedKdf.deriveKey(password, salt, (int) cost); if (formatVersion == PBKDF_FORMAT_SCRYPT_STRENGTHENED) { - if (keyStrengthener == null || !keyStrengthener.isInitialised()) - return null; // Can't derive the same strengthened key + if (keyStrengthener == null || !keyStrengthener.isInitialised()) { + // Can't derive the same strengthened key + throw new DecryptionException(KEY_STRENGTHENER_ERROR); + } key = keyStrengthener.strengthenKey(key); } // Initialise the cipher @@ -411,7 +419,7 @@ class CryptoComponentImpl implements CryptoComponent { cipher.process(input, inputOff, inputLen, output, 0); return output; } catch (GeneralSecurityException e) { - return null; // Invalid ciphertext + throw new DecryptionException(INVALID_PASSWORD); } } diff --git a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java index 4b6b63e7e..5c4976f85 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/account/AccountManagerImplTest.java @@ -1,6 +1,7 @@ package org.briarproject.bramble.account; import org.briarproject.bramble.api.crypto.CryptoComponent; +import org.briarproject.bramble.api.crypto.DecryptionException; import org.briarproject.bramble.api.crypto.KeyStrengthener; import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.api.db.DatabaseConfig; @@ -19,12 +20,15 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStreamReader; +import java.nio.charset.Charset; import javax.annotation.Nullable; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertNull; import static junit.framework.Assert.assertTrue; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD; import static org.briarproject.bramble.test.TestUtils.deleteTestDirectory; import static org.briarproject.bramble.test.TestUtils.getIdentity; import static org.briarproject.bramble.test.TestUtils.getRandomBytes; @@ -35,6 +39,7 @@ import static org.briarproject.bramble.util.StringUtils.toHexString; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; public class AccountManagerImplTest extends BrambleMockTestCase { @@ -83,8 +88,13 @@ public class AccountManagerImplTest extends BrambleMockTestCase { } @Test - public void testSignInReturnsFalseIfDbKeyCannotBeLoaded() { - assertFalse(accountManager.signIn(password)); + public void testSignInThrowsExceptionIfDbKeyCannotBeLoaded() { + try { + accountManager.signIn(password); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult()); + } assertFalse(accountManager.hasDatabaseKey()); assertFalse(keyFile.exists()); @@ -92,11 +102,11 @@ public class AccountManagerImplTest extends BrambleMockTestCase { } @Test - public void testSignInReturnsFalseIfPasswordIsWrong() throws Exception { + public void testSignInThrowsExceptionIfPasswordIsWrong() throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, keyStrengthener); - will(returnValue(null)); + will(throwException(new DecryptionException(INVALID_PASSWORD))); }}); storeDatabaseKey(keyFile, encryptedKeyHex); @@ -105,7 +115,12 @@ public class AccountManagerImplTest extends BrambleMockTestCase { assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); - assertFalse(accountManager.signIn(password)); + try { + accountManager.signIn(password); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_PASSWORD, expected.getDecryptionResult()); + } assertFalse(accountManager.hasDatabaseKey()); assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); @@ -128,7 +143,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); - assertTrue(accountManager.signIn(password)); + accountManager.signIn(password); assertTrue(accountManager.hasDatabaseKey()); SecretKey decrypted = accountManager.getDatabaseKey(); assertNotNull(decrypted); @@ -157,7 +172,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); - assertTrue(accountManager.signIn(password)); + accountManager.signIn(password); assertTrue(accountManager.hasDatabaseKey()); SecretKey decrypted = accountManager.getDatabaseKey(); assertNotNull(decrypted); @@ -315,26 +330,36 @@ public class AccountManagerImplTest extends BrambleMockTestCase { } @Test - public void testChangePasswordReturnsFalseIfDbKeyCannotBeLoaded() { - assertFalse(accountManager.changePassword(password, newPassword)); + public void testChangePasswordThrowsExceptionIfDbKeyCannotBeLoaded() { + try { + accountManager.changePassword(password, newPassword); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult()); + } assertFalse(keyFile.exists()); assertFalse(keyBackupFile.exists()); } @Test - public void testChangePasswordReturnsFalseIfPasswordIsWrong() + public void testChangePasswordThrowsExceptionIfPasswordIsWrong() throws Exception { context.checking(new Expectations() {{ oneOf(crypto).decryptWithPassword(encryptedKey, password, keyStrengthener); - will(returnValue(null)); + will(throwException(new DecryptionException(INVALID_PASSWORD))); }}); storeDatabaseKey(keyFile, encryptedKeyHex); storeDatabaseKey(keyBackupFile, encryptedKeyHex); - assertFalse(accountManager.changePassword(password, newPassword)); + try { + accountManager.changePassword(password, newPassword); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_PASSWORD, expected.getDecryptionResult()); + } assertEquals(encryptedKeyHex, loadDatabaseKey(keyFile)); assertEquals(encryptedKeyHex, loadDatabaseKey(keyBackupFile)); @@ -357,7 +382,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { storeDatabaseKey(keyFile, encryptedKeyHex); storeDatabaseKey(keyBackupFile, encryptedKeyHex); - assertTrue(accountManager.changePassword(password, newPassword)); + accountManager.changePassword(password, newPassword); assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyFile)); assertEquals(newEncryptedKeyHex, loadDatabaseKey(keyBackupFile)); @@ -366,7 +391,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { private void storeDatabaseKey(File f, String hex) throws IOException { f.getParentFile().mkdirs(); FileOutputStream out = new FileOutputStream(f); - out.write(hex.getBytes("UTF-8")); + out.write(hex.getBytes(Charset.forName("UTF-8"))); out.flush(); out.close(); } @@ -374,7 +399,7 @@ public class AccountManagerImplTest extends BrambleMockTestCase { @Nullable private String loadDatabaseKey(File f) throws IOException { BufferedReader reader = new BufferedReader(new InputStreamReader( - new FileInputStream(f), "UTF-8")); + new FileInputStream(f), Charset.forName("UTF-8"))); String hex = reader.readLine(); reader.close(); return hex; diff --git a/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java b/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java index e2380f655..f5513b613 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/crypto/PasswordBasedEncryptionTest.java @@ -1,25 +1,35 @@ package org.briarproject.bramble.crypto; +import org.briarproject.bramble.api.crypto.DecryptionException; +import org.briarproject.bramble.api.crypto.KeyStrengthener; +import org.briarproject.bramble.api.crypto.SecretKey; import org.briarproject.bramble.system.SystemClock; -import org.briarproject.bramble.test.BrambleTestCase; +import org.briarproject.bramble.test.BrambleMockTestCase; import org.briarproject.bramble.test.TestSecureRandomProvider; -import org.briarproject.bramble.test.TestUtils; +import org.jmock.Expectations; import org.junit.Test; -import java.util.Random; - +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_CIPHERTEXT; +import static org.briarproject.bramble.api.crypto.DecryptionResult.INVALID_PASSWORD; +import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR; +import static org.briarproject.bramble.test.TestUtils.getRandomBytes; +import static org.briarproject.bramble.test.TestUtils.getSecretKey; import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; -public class PasswordBasedEncryptionTest extends BrambleTestCase { +public class PasswordBasedEncryptionTest extends BrambleMockTestCase { + + private final KeyStrengthener keyStrengthener = + context.mock(KeyStrengthener.class); private final CryptoComponentImpl crypto = new CryptoComponentImpl(new TestSecureRandomProvider(), new ScryptKdf(new SystemClock())); @Test - public void testEncryptionAndDecryption() { - byte[] input = TestUtils.getRandomBytes(1234); + public void testEncryptionAndDecryption() throws Exception { + byte[] input = getRandomBytes(1234); String password = "password"; byte[] ciphertext = crypto.encryptWithPassword(input, password, null); byte[] output = crypto.decryptWithPassword(ciphertext, password, null); @@ -27,14 +37,80 @@ public class PasswordBasedEncryptionTest extends BrambleTestCase { } @Test - public void testInvalidCiphertextReturnsNull() { - byte[] input = TestUtils.getRandomBytes(1234); + public void testInvalidFormatVersionThrowsException() { + byte[] input = getRandomBytes(1234); String password = "password"; byte[] ciphertext = crypto.encryptWithPassword(input, password, null); - // Modify the ciphertext - int position = new Random().nextInt(ciphertext.length); - ciphertext[position] = (byte) (ciphertext[position] ^ 0xFF); - byte[] output = crypto.decryptWithPassword(ciphertext, password, null); - assertNull(output); + + // Modify the format version + ciphertext[0] ^= (byte) 0xFF; + try { + crypto.decryptWithPassword(ciphertext, password, null); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_CIPHERTEXT, expected.getDecryptionResult()); + } + } + + @Test + public void testInvalidPasswordThrowsException() { + byte[] input = getRandomBytes(1234); + byte[] ciphertext = crypto.encryptWithPassword(input, "password", null); + + // Try to decrypt with the wrong password + try { + crypto.decryptWithPassword(ciphertext, "wrong", null); + fail(); + } catch (DecryptionException expected) { + assertEquals(INVALID_PASSWORD, expected.getDecryptionResult()); + } + } + + @Test + public void testMissingKeyStrengthenerThrowsException() { + SecretKey strengthened = getSecretKey(); + context.checking(new Expectations() {{ + oneOf(keyStrengthener).strengthenKey(with(any(SecretKey.class))); + will(returnValue(strengthened)); + }}); + + // Use the key strengthener during encryption + byte[] input = getRandomBytes(1234); + String password = "password"; + byte[] ciphertext = + crypto.encryptWithPassword(input, password, keyStrengthener); + + // The key strengthener is missing during decryption + try { + crypto.decryptWithPassword(ciphertext, password, null); + fail(); + } catch (DecryptionException expected) { + assertEquals(KEY_STRENGTHENER_ERROR, expected.getDecryptionResult()); + } + } + + @Test + public void testKeyStrengthenerFailureThrowsException() { + SecretKey strengthened = getSecretKey(); + context.checking(new Expectations() {{ + oneOf(keyStrengthener).strengthenKey(with(any(SecretKey.class))); + will(returnValue(strengthened)); + oneOf(keyStrengthener).isInitialised(); + will(returnValue(false)); + }}); + + // Use the key strengthener during encryption + byte[] input = getRandomBytes(1234); + String password = "password"; + byte[] ciphertext = + crypto.encryptWithPassword(input, password, keyStrengthener); + + // The key strengthener fails during decryption + try { + crypto.decryptWithPassword(ciphertext, password, keyStrengthener); + fail(); + } catch (DecryptionException expected) { + assertEquals(KEY_STRENGTHENER_ERROR, expected.getDecryptionResult()); + } } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java index 41c728e5e..07b37f2a8 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/AppModule.java @@ -36,6 +36,7 @@ import org.briarproject.bramble.util.AndroidUtils; import org.briarproject.bramble.util.StringUtils; import org.briarproject.briar.android.account.LockManagerImpl; import org.briarproject.briar.android.keyagreement.ContactExchangeModule; +import org.briarproject.briar.android.login.LoginModule; import org.briarproject.briar.android.viewmodel.ViewModelModule; import org.briarproject.briar.api.android.AndroidNotificationManager; import org.briarproject.briar.api.android.DozeWatchdog; @@ -64,7 +65,11 @@ import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_ONIO import static org.briarproject.bramble.api.reporting.ReportingConstants.DEV_PUBLIC_KEY_HEX; import static org.briarproject.briar.android.TestingConstants.IS_DEBUG_BUILD; -@Module(includes = {ContactExchangeModule.class, ViewModelModule.class}) +@Module(includes = { + ContactExchangeModule.class, + LoginModule.class, + ViewModelModule.class +}) public class AppModule { static class EagerSingletons { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/activity/ActivityModule.java b/briar-android/src/main/java/org/briarproject/briar/android/activity/ActivityModule.java index 1986eb1af..e9ed9686c 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/activity/ActivityModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/activity/ActivityModule.java @@ -8,8 +8,6 @@ import org.briarproject.briar.android.controller.BriarController; import org.briarproject.briar.android.controller.BriarControllerImpl; import org.briarproject.briar.android.controller.DbController; import org.briarproject.briar.android.controller.DbControllerImpl; -import org.briarproject.briar.android.login.ChangePasswordController; -import org.briarproject.briar.android.login.ChangePasswordControllerImpl; import org.briarproject.briar.android.navdrawer.NavDrawerController; import org.briarproject.briar.android.navdrawer.NavDrawerControllerImpl; @@ -46,13 +44,6 @@ public class ActivityModule { return setupController; } - @ActivityScope - @Provides - ChangePasswordController providePasswordController( - ChangePasswordControllerImpl passwordController) { - return passwordController; - } - @ActivityScope @Provides protected BriarController provideBriarController( @@ -80,5 +71,4 @@ public class ActivityModule { BriarServiceConnection provideBriarServiceConnection() { return new BriarServiceConnection(); } - } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java index 7c4b4f36b..06685559a 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java @@ -15,27 +15,32 @@ import android.widget.Toast; import com.google.android.material.textfield.TextInputLayout; +import org.briarproject.bramble.api.crypto.DecryptionResult; import org.briarproject.briar.R; import org.briarproject.briar.android.activity.ActivityComponent; import org.briarproject.briar.android.activity.BriarActivity; -import org.briarproject.briar.android.controller.handler.UiResultHandler; -import org.briarproject.briar.android.util.UiUtils; import javax.inject.Inject; -import androidx.annotation.NonNull; +import androidx.lifecycle.ViewModelProvider; +import androidx.lifecycle.ViewModelProviders; import static android.view.View.INVISIBLE; import static android.view.View.VISIBLE; +import static android.widget.Toast.LENGTH_LONG; +import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR; +import static org.briarproject.bramble.api.crypto.DecryptionResult.SUCCESS; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.QUITE_WEAK; +import static org.briarproject.briar.android.login.LoginUtils.createKeyStrengthenerErrorDialog; import static org.briarproject.briar.android.util.UiUtils.hideSoftKeyboard; +import static org.briarproject.briar.android.util.UiUtils.setError; import static org.briarproject.briar.android.util.UiUtils.showSoftKeyboard; public class ChangePasswordActivity extends BriarActivity implements OnClickListener, OnEditorActionListener { @Inject - protected ChangePasswordController passwordController; + ViewModelProvider.Factory viewModelFactory; private TextInputLayout currentPasswordEntryWrapper; private TextInputLayout newPasswordEntryWrapper; @@ -47,11 +52,17 @@ public class ChangePasswordActivity extends BriarActivity private Button changePasswordButton; private ProgressBar progress; + // Package access for testing + ChangePasswordViewModel viewModel; + @Override public void onCreate(Bundle state) { super.onCreate(state); setContentView(R.layout.activity_change_password); + viewModel = ViewModelProviders.of(this, viewModelFactory) + .get(ChangePasswordViewModel.class); + currentPasswordEntryWrapper = findViewById(R.id.current_password_entry_wrapper); newPasswordEntryWrapper = findViewById(R.id.new_password_entry_wrapper); @@ -102,13 +113,12 @@ public class ChangePasswordActivity extends BriarActivity String firstPassword = newPassword.getText().toString(); String secondPassword = newPasswordConfirmation.getText().toString(); boolean passwordsMatch = firstPassword.equals(secondPassword); - float strength = - passwordController.estimatePasswordStrength(firstPassword); + float strength = viewModel.estimatePasswordStrength(firstPassword); strengthMeter.setStrength(strength); - UiUtils.setError(newPasswordEntryWrapper, + setError(newPasswordEntryWrapper, getString(R.string.password_too_weak), firstPassword.length() > 0 && strength < QUITE_WEAK); - UiUtils.setError(newPasswordConfirmationWrapper, + setError(newPasswordConfirmationWrapper, getString(R.string.passwords_do_not_match), secondPassword.length() > 0 && !passwordsMatch); changePasswordButton.setEnabled( @@ -127,32 +137,34 @@ public class ChangePasswordActivity extends BriarActivity // Replace the button with a progress bar changePasswordButton.setVisibility(INVISIBLE); progress.setVisibility(VISIBLE); - passwordController.changePassword(currentPassword.getText().toString(), - newPassword.getText().toString(), - new UiResultHandler(this) { - @Override - public void onResultUi(@NonNull Boolean result) { - if (result) { - Toast.makeText(ChangePasswordActivity.this, - R.string.password_changed, - Toast.LENGTH_LONG).show(); - setResult(RESULT_OK); - supportFinishAfterTransition(); - } else { - tryAgain(); - } + + String curPwd = currentPassword.getText().toString(); + String newPwd = newPassword.getText().toString(); + viewModel.changePassword(curPwd, newPwd).observeEvent(this, result -> { + if (result == SUCCESS) { + Toast.makeText(ChangePasswordActivity.this, + R.string.password_changed, + LENGTH_LONG).show(); + setResult(RESULT_OK); + supportFinishAfterTransition(); + } else { + tryAgain(result); } - }); + } + ); } - private void tryAgain() { - UiUtils.setError(currentPasswordEntryWrapper, - getString(R.string.try_again), true); + private void tryAgain(DecryptionResult result) { changePasswordButton.setVisibility(VISIBLE); progress.setVisibility(INVISIBLE); - currentPassword.setText(""); - - // show the keyboard again - showSoftKeyboard(currentPassword); + if (result == KEY_STRENGTHENER_ERROR) { + createKeyStrengthenerErrorDialog(this).show(); + } else { + setError(currentPasswordEntryWrapper, + getString(R.string.try_again), true); + currentPassword.setText(""); + // show the keyboard again + showSoftKeyboard(currentPassword); + } } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordControllerImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordControllerImpl.java deleted file mode 100644 index 79eaef917..000000000 --- a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordControllerImpl.java +++ /dev/null @@ -1,43 +0,0 @@ -package org.briarproject.briar.android.login; - -import org.briarproject.bramble.api.account.AccountManager; -import org.briarproject.bramble.api.crypto.PasswordStrengthEstimator; -import org.briarproject.bramble.api.lifecycle.IoExecutor; -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; -import org.briarproject.briar.android.controller.handler.ResultHandler; - -import java.util.concurrent.Executor; - -import javax.inject.Inject; - -@NotNullByDefault -public class ChangePasswordControllerImpl implements ChangePasswordController { - - protected final AccountManager accountManager; - protected final Executor ioExecutor; - private final PasswordStrengthEstimator strengthEstimator; - - @Inject - ChangePasswordControllerImpl(AccountManager accountManager, - @IoExecutor Executor ioExecutor, - PasswordStrengthEstimator strengthEstimator) { - this.accountManager = accountManager; - this.ioExecutor = ioExecutor; - this.strengthEstimator = strengthEstimator; - } - - @Override - public float estimatePasswordStrength(String password) { - return strengthEstimator.estimateStrength(password); - } - - @Override - public void changePassword(String oldPassword, String newPassword, - ResultHandler resultHandler) { - ioExecutor.execute(() -> { - boolean changed = - accountManager.changePassword(oldPassword, newPassword); - resultHandler.onResult(changed); - }); - } -} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordViewModel.java new file mode 100644 index 000000000..9a622a15c --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordViewModel.java @@ -0,0 +1,53 @@ +package org.briarproject.briar.android.login; + +import org.briarproject.bramble.api.account.AccountManager; +import org.briarproject.bramble.api.crypto.DecryptionException; +import org.briarproject.bramble.api.crypto.DecryptionResult; +import org.briarproject.bramble.api.crypto.PasswordStrengthEstimator; +import org.briarproject.bramble.api.lifecycle.IoExecutor; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.briar.android.viewmodel.LiveEvent; +import org.briarproject.briar.android.viewmodel.MutableLiveEvent; + +import java.util.concurrent.Executor; + +import javax.inject.Inject; + +import androidx.lifecycle.ViewModel; + +import static org.briarproject.bramble.api.crypto.DecryptionResult.SUCCESS; + +@NotNullByDefault +public class ChangePasswordViewModel extends ViewModel { + + private final AccountManager accountManager; + private final Executor ioExecutor; + private final PasswordStrengthEstimator strengthEstimator; + + @Inject + ChangePasswordViewModel(AccountManager accountManager, + @IoExecutor Executor ioExecutor, + PasswordStrengthEstimator strengthEstimator) { + this.accountManager = accountManager; + this.ioExecutor = ioExecutor; + this.strengthEstimator = strengthEstimator; + } + + float estimatePasswordStrength(String password) { + return strengthEstimator.estimateStrength(password); + } + + LiveEvent changePassword(String oldPassword, + String newPassword) { + MutableLiveEvent result = new MutableLiveEvent<>(); + ioExecutor.execute(() -> { + try { + accountManager.changePassword(oldPassword, newPassword); + result.postEvent(SUCCESS); + } catch (DecryptionException e) { + result.postEvent(e.getDecryptionResult()); + } + }); + return result; + } +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/LoginModule.java b/briar-android/src/main/java/org/briarproject/briar/android/login/LoginModule.java new file mode 100644 index 000000000..2606d9b2d --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/LoginModule.java @@ -0,0 +1,23 @@ +package org.briarproject.briar.android.login; + +import org.briarproject.briar.android.viewmodel.ViewModelKey; + +import androidx.lifecycle.ViewModel; +import dagger.Binds; +import dagger.Module; +import dagger.multibindings.IntoMap; + +@Module +public abstract class LoginModule { + + @Binds + @IntoMap + @ViewModelKey(StartupViewModel.class) + abstract ViewModel bindStartupViewModel(StartupViewModel viewModel); + + @Binds + @IntoMap + @ViewModelKey(ChangePasswordViewModel.class) + abstract ViewModel bindChangePasswordViewModel( + ChangePasswordViewModel viewModel); +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/LoginUtils.java b/briar-android/src/main/java/org/briarproject/briar/android/login/LoginUtils.java new file mode 100644 index 000000000..39dedadaf --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/LoginUtils.java @@ -0,0 +1,30 @@ +package org.briarproject.briar.android.login; + +import android.content.Context; +import android.graphics.drawable.Drawable; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.briar.R; + +import androidx.appcompat.app.AlertDialog; + +import static androidx.core.content.ContextCompat.getColor; +import static androidx.core.content.ContextCompat.getDrawable; +import static androidx.core.graphics.drawable.DrawableCompat.setTint; +import static java.util.Objects.requireNonNull; + +@NotNullByDefault +class LoginUtils { + + static AlertDialog createKeyStrengthenerErrorDialog(Context ctx) { + AlertDialog.Builder builder = + new AlertDialog.Builder(ctx, R.style.BriarDialogTheme); + Drawable icon = getDrawable(ctx, R.drawable.alerts_and_states_error); + setTint(requireNonNull(icon), getColor(ctx, R.color.color_primary)); + builder.setIcon(icon); + builder.setTitle(R.string.dialog_title_cannot_check_password); + builder.setMessage(R.string.dialog_message_cannot_check_password); + builder.setPositiveButton(R.string.ok, null); + return builder.create(); + } +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/PasswordFragment.java b/briar-android/src/main/java/org/briarproject/briar/android/login/PasswordFragment.java index 778259c76..755ca8833 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/login/PasswordFragment.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/PasswordFragment.java @@ -12,6 +12,7 @@ import android.widget.ProgressBar; import com.google.android.material.textfield.TextInputEditText; import com.google.android.material.textfield.TextInputLayout; +import org.briarproject.bramble.api.crypto.DecryptionResult; import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault; import org.briarproject.briar.R; @@ -28,6 +29,9 @@ import androidx.lifecycle.ViewModelProviders; import static android.view.View.INVISIBLE; import static android.view.View.VISIBLE; import static android.view.inputmethod.EditorInfo.IME_ACTION_DONE; +import static org.briarproject.bramble.api.crypto.DecryptionResult.KEY_STRENGTHENER_ERROR; +import static org.briarproject.bramble.api.crypto.DecryptionResult.SUCCESS; +import static org.briarproject.briar.android.login.LoginUtils.createKeyStrengthenerErrorDialog; import static org.briarproject.briar.android.util.UiUtils.enterPressed; import static org.briarproject.briar.android.util.UiUtils.hideSoftKeyboard; import static org.briarproject.briar.android.util.UiUtils.setError; @@ -58,12 +62,13 @@ public class PasswordFragment extends BaseFragment implements TextWatcher { @Nullable ViewGroup container, @Nullable Bundle savedInstanceState) { View v = inflater.inflate(R.layout.fragment_password, container, - false); + false); viewModel = ViewModelProviders.of(requireActivity(), viewModelFactory) .get(StartupViewModel.class); - viewModel.getPasswordValidated().observeEvent(this, valid -> { - if (!valid) onPasswordInvalid(); + + viewModel.getPasswordValidated().observeEvent(this, result -> { + if (result != SUCCESS) onPasswordInvalid(result); }); signInButton = v.findViewById(R.id.btn_sign_in); @@ -107,18 +112,20 @@ public class PasswordFragment extends BaseFragment implements TextWatcher { viewModel.validatePassword(password.getText().toString()); } - private void onPasswordInvalid() { - setError(input, getString(R.string.try_again), true); + private void onPasswordInvalid(DecryptionResult result) { signInButton.setVisibility(VISIBLE); progress.setVisibility(INVISIBLE); - password.setText(null); - - // show the keyboard again - showSoftKeyboard(password); + if (result == KEY_STRENGTHENER_ERROR) { + createKeyStrengthenerErrorDialog(requireContext()).show(); + } else { + setError(input, getString(R.string.try_again), true); + password.setText(null); + // show the keyboard again + showSoftKeyboard(password); + } } - public void onForgottenPasswordClick() { - // TODO Encapsulate the dialog in a re-usable fragment + private void onForgottenPasswordClick() { AlertDialog.Builder builder = new AlertDialog.Builder(requireContext(), R.style.BriarDialogTheme); builder.setTitle(R.string.dialog_title_lost_password); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/StartupViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/login/StartupViewModel.java index 192c085f3..1a8d9d1f9 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/login/StartupViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/StartupViewModel.java @@ -3,6 +3,8 @@ package org.briarproject.briar.android.login; import android.app.Application; import org.briarproject.bramble.api.account.AccountManager; +import org.briarproject.bramble.api.crypto.DecryptionException; +import org.briarproject.bramble.api.crypto.DecryptionResult; import org.briarproject.bramble.api.event.Event; import org.briarproject.bramble.api.event.EventBus; import org.briarproject.bramble.api.event.EventListener; @@ -24,6 +26,7 @@ import androidx.lifecycle.AndroidViewModel; import androidx.lifecycle.LiveData; import androidx.lifecycle.MutableLiveData; +import static org.briarproject.bramble.api.crypto.DecryptionResult.SUCCESS; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.COMPACTING_DATABASE; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.MIGRATING_DATABASE; import static org.briarproject.bramble.api.lifecycle.LifecycleManager.LifecycleState.STARTING_SERVICES; @@ -46,7 +49,7 @@ public class StartupViewModel extends AndroidViewModel @IoExecutor private final Executor ioExecutor; - private final MutableLiveEvent passwordValidated = + private final MutableLiveEvent passwordValidated = new MutableLiveEvent<>(); private final MutableLiveEvent accountDeleted = new MutableLiveEvent<>(); @@ -105,13 +108,17 @@ public class StartupViewModel extends AndroidViewModel void validatePassword(String password) { ioExecutor.execute(() -> { - boolean signedIn = accountManager.signIn(password); - passwordValidated.postEvent(signedIn); - if (signedIn) state.postValue(SIGNED_IN); + try { + accountManager.signIn(password); + passwordValidated.postEvent(SUCCESS); + state.postValue(SIGNED_IN); + } catch (DecryptionException e) { + passwordValidated.postEvent(e.getDecryptionResult()); + } }); } - LiveEvent getPasswordValidated() { + LiveEvent getPasswordValidated() { return passwordValidated; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/util/UiUtils.java b/briar-android/src/main/java/org/briarproject/briar/android/util/UiUtils.java index 83d59dddd..f4bdb966a 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/util/UiUtils.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/util/UiUtils.java @@ -381,7 +381,7 @@ public class UiUtils { /** * Same as {@link #observeOnce(LiveData, LifecycleOwner, Observer)}, * but without a {@link LifecycleOwner}. - * + *

* Warning: Do NOT call from objects that have a lifecycle. */ @UiThread @@ -401,5 +401,4 @@ public class UiUtils { return ctx.getResources().getConfiguration().getLayoutDirection() == LAYOUT_DIRECTION_RTL; } - } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/ViewModelModule.java b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/ViewModelModule.java index 4e4c9bb4c..ce7899915 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/ViewModelModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/ViewModelModule.java @@ -4,7 +4,6 @@ import org.briarproject.briar.android.contact.add.remote.AddContactViewModel; import org.briarproject.briar.android.contact.add.remote.PendingContactListViewModel; import org.briarproject.briar.android.conversation.ConversationViewModel; import org.briarproject.briar.android.conversation.ImageViewModel; -import org.briarproject.briar.android.login.StartupViewModel; import javax.inject.Singleton; @@ -17,11 +16,6 @@ import dagger.multibindings.IntoMap; @Module public abstract class ViewModelModule { - @Binds - @IntoMap - @ViewModelKey(StartupViewModel.class) - abstract ViewModel bindStartupViewModel(StartupViewModel startupViewModel); - @Binds @IntoMap @ViewModelKey(ConversationViewModel.class) diff --git a/briar-android/src/main/res/values/strings.xml b/briar-android/src/main/res/values/strings.xml index d7e5d7090..341e2c4ab 100644 --- a/briar-android/src/main/res/values/strings.xml +++ b/briar-android/src/main/res/values/strings.xml @@ -32,6 +32,8 @@ Password Wrong password, try again + Cannot Check Password + Briar cannot check your password. Please try rebooting your device to solve this problem. Sign In I have forgotten my password Lost Password diff --git a/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordActivityTest.java b/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordActivityTest.java index 27945abd6..444fe0a3d 100644 --- a/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordActivityTest.java +++ b/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordActivityTest.java @@ -5,28 +5,30 @@ import android.widget.EditText; import com.google.android.material.textfield.TextInputLayout; +import org.briarproject.bramble.api.crypto.DecryptionResult; import org.briarproject.briar.R; import org.briarproject.briar.android.TestBriarApplication; -import org.briarproject.briar.android.controller.handler.ResultHandler; +import org.briarproject.briar.android.viewmodel.MutableLiveEvent; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.robolectric.Robolectric; import org.robolectric.RobolectricTestRunner; import org.robolectric.annotation.Config; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertFalse; +import static junit.framework.Assert.assertTrue; +import static org.briarproject.bramble.api.crypto.DecryptionResult.SUCCESS; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.NONE; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.QUITE_STRONG; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.QUITE_WEAK; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.STRONG; import static org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.WEAK; +import static org.junit.Assert.assertNotEquals; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.times; @@ -37,7 +39,7 @@ import static org.mockito.Mockito.when; @Config(sdk = 21, application = TestBriarApplication.class) public class ChangePasswordActivityTest { - private TestChangePasswordActivity changePasswordActivity; + private ChangePasswordActivity changePasswordActivity; private TextInputLayout passwordConfirmationWrapper; private EditText currentPassword; private EditText newPassword; @@ -46,15 +48,14 @@ public class ChangePasswordActivityTest { private Button changePasswordButton; @Mock - private ChangePasswordController passwordController; - @Captor - private ArgumentCaptor> resultCaptor; + private ChangePasswordViewModel viewModel; @Before public void setUp() { MockitoAnnotations.initMocks(this); changePasswordActivity = - Robolectric.setupActivity(TestChangePasswordActivity.class); + Robolectric.setupActivity(ChangePasswordActivity.class); + changePasswordActivity.viewModel = viewModel; passwordConfirmationWrapper = changePasswordActivity .findViewById(R.id.new_password_confirm_wrapper); currentPassword = changePasswordActivity @@ -81,7 +82,7 @@ public class ChangePasswordActivityTest { // Password mismatch newPassword.setText("really.safe.password"); newPasswordConfirmation.setText("really.safe.pass"); - assertEquals(changePasswordButton.isEnabled(), false); + assertFalse(changePasswordButton.isEnabled()); assertEquals(passwordConfirmationWrapper.getError(), changePasswordActivity .getString(R.string.passwords_do_not_match)); @@ -89,70 +90,59 @@ public class ChangePasswordActivityTest { newPassword.setText("really.safe.pass"); newPasswordConfirmation.setText("really.safe.pass"); // Confirm that the password mismatch error message is not visible - Assert.assertNotEquals(passwordConfirmationWrapper.getError(), + assertNotEquals(passwordConfirmationWrapper.getError(), changePasswordActivity .getString(R.string.passwords_do_not_match)); // Nick has not been set, expect the button to be disabled - assertEquals(changePasswordButton.isEnabled(), false); + assertFalse(changePasswordButton.isEnabled()); } @Test public void testChangePasswordUI() { - changePasswordActivity.setPasswordController(passwordController); // Mock strong password strength answer - when(passwordController.estimatePasswordStrength(anyString())) + when(viewModel.estimatePasswordStrength(anyString())) .thenReturn(STRONG); + // Mock changing the password + MutableLiveEvent result = new MutableLiveEvent<>(); + when(viewModel.changePassword(anyString(), anyString())) + .thenReturn(result); String curPass = "old.password"; String safePass = "really.safe.password"; currentPassword.setText(curPass); newPassword.setText(safePass); newPasswordConfirmation.setText(safePass); // Confirm that the create account button is clickable - assertEquals(changePasswordButton.isEnabled(), true); + assertTrue(changePasswordButton.isEnabled()); changePasswordButton.performClick(); - // Verify that the controller's method was called with the correct - // params and get the callback - verify(passwordController, times(1)) - .changePassword(eq(curPass), eq(safePass), - resultCaptor.capture()); - // execute the callbacks - resultCaptor.getValue().onResult(true); - assertEquals(changePasswordActivity.isFinishing(), true); + // Verify that the view model was called with the correct params + verify(viewModel, times(1)).changePassword(eq(curPass), eq(safePass)); + // Return the result + result.postEvent(SUCCESS); + assertTrue(changePasswordActivity.isFinishing()); } @Test public void testStrengthMeterUI() { Assert.assertNotNull(changePasswordActivity); - // replace the password controller with our mocked copy - changePasswordActivity.setPasswordController(passwordController); // Mock answers for UI testing only - when(passwordController.estimatePasswordStrength("strong")).thenReturn( - STRONG); - when(passwordController.estimatePasswordStrength("qstrong")).thenReturn( - QUITE_STRONG); - when(passwordController.estimatePasswordStrength("qweak")).thenReturn( - QUITE_WEAK); - when(passwordController.estimatePasswordStrength("weak")).thenReturn( - WEAK); - when(passwordController.estimatePasswordStrength("empty")).thenReturn( - NONE); + when(viewModel.estimatePasswordStrength("strong")).thenReturn(STRONG); + when(viewModel.estimatePasswordStrength("qstrong")) + .thenReturn(QUITE_STRONG); + when(viewModel.estimatePasswordStrength("qweak")) + .thenReturn(QUITE_WEAK); + when(viewModel.estimatePasswordStrength("weak")).thenReturn(WEAK); + when(viewModel.estimatePasswordStrength("empty")).thenReturn(NONE); // Test the meters progress and color for several values testStrengthMeter("strong", STRONG, StrengthMeter.GREEN); - Mockito.verify(passwordController, Mockito.times(1)) - .estimatePasswordStrength(eq("strong")); + verify(viewModel, times(1)).estimatePasswordStrength(eq("strong")); testStrengthMeter("qstrong", QUITE_STRONG, StrengthMeter.LIME); - Mockito.verify(passwordController, Mockito.times(1)) - .estimatePasswordStrength(eq("qstrong")); + verify(viewModel, times(1)).estimatePasswordStrength(eq("qstrong")); testStrengthMeter("qweak", QUITE_WEAK, StrengthMeter.YELLOW); - Mockito.verify(passwordController, Mockito.times(1)) - .estimatePasswordStrength(eq("qweak")); + verify(viewModel, times(1)).estimatePasswordStrength(eq("qweak")); testStrengthMeter("weak", WEAK, StrengthMeter.ORANGE); - Mockito.verify(passwordController, Mockito.times(1)) - .estimatePasswordStrength(eq("weak")); + verify(viewModel, times(1)).estimatePasswordStrength(eq("weak")); // Not sure this should be the correct behaviour on an empty input ? testStrengthMeter("empty", NONE, StrengthMeter.RED); - Mockito.verify(passwordController, Mockito.times(1)) - .estimatePasswordStrength(eq("empty")); + verify(viewModel, times(1)).estimatePasswordStrength(eq("empty")); } - } diff --git a/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordControllerImplTest.java b/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordControllerImplTest.java deleted file mode 100644 index a588cb71b..000000000 --- a/briar-android/src/test/java/org/briarproject/briar/android/login/ChangePasswordControllerImplTest.java +++ /dev/null @@ -1,58 +0,0 @@ -package org.briarproject.briar.android.login; - -import org.briarproject.bramble.api.account.AccountManager; -import org.briarproject.bramble.api.crypto.PasswordStrengthEstimator; -import org.briarproject.bramble.test.BrambleMockTestCase; -import org.briarproject.bramble.test.ImmediateExecutor; -import org.jmock.Expectations; -import org.junit.Test; - -import java.util.concurrent.Executor; -import java.util.concurrent.atomic.AtomicBoolean; - -import static junit.framework.Assert.assertFalse; -import static junit.framework.Assert.assertTrue; -import static org.briarproject.bramble.util.StringUtils.getRandomString; - -public class ChangePasswordControllerImplTest extends BrambleMockTestCase { - - private final AccountManager accountManager = - context.mock(AccountManager.class); - private final PasswordStrengthEstimator estimator = - context.mock(PasswordStrengthEstimator.class); - - private final Executor ioExecutor = new ImmediateExecutor(); - - private final String oldPassword = getRandomString(10); - private final String newPassword = getRandomString(10); - - @Test - public void testChangePasswordReturnsTrue() { - context.checking(new Expectations() {{ - oneOf(accountManager).changePassword(oldPassword, newPassword); - will(returnValue(true)); - }}); - - ChangePasswordControllerImpl p = new ChangePasswordControllerImpl(accountManager, - ioExecutor, estimator); - - AtomicBoolean capturedResult = new AtomicBoolean(false); - p.changePassword(oldPassword, newPassword, capturedResult::set); - assertTrue(capturedResult.get()); - } - - @Test - public void testChangePasswordReturnsFalseIfOldPasswordIsWrong() { - context.checking(new Expectations() {{ - oneOf(accountManager).changePassword(oldPassword, newPassword); - will(returnValue(false)); - }}); - - ChangePasswordControllerImpl p = new ChangePasswordControllerImpl(accountManager, - ioExecutor, estimator); - - AtomicBoolean capturedResult = new AtomicBoolean(true); - p.changePassword(oldPassword, newPassword, capturedResult::set); - assertFalse(capturedResult.get()); - } -} diff --git a/briar-android/src/test/java/org/briarproject/briar/android/login/TestChangePasswordActivity.java b/briar-android/src/test/java/org/briarproject/briar/android/login/TestChangePasswordActivity.java deleted file mode 100644 index 594866115..000000000 --- a/briar-android/src/test/java/org/briarproject/briar/android/login/TestChangePasswordActivity.java +++ /dev/null @@ -1,14 +0,0 @@ -package org.briarproject.briar.android.login; - -/** - * This class exposes the PasswordController and offers the possibility to - * replace it. - */ -public class TestChangePasswordActivity extends ChangePasswordActivity { - - public void setPasswordController( - ChangePasswordController passwordController) { - this.passwordController = passwordController; - } - -} diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/BriarService.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/BriarService.kt index 08a2d38dc..c5b2e05ee 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/BriarService.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/BriarService.kt @@ -4,6 +4,7 @@ import com.github.ajalt.clikt.core.UsageError import com.github.ajalt.clikt.output.TermUi.echo import com.github.ajalt.clikt.output.TermUi.prompt import org.briarproject.bramble.api.account.AccountManager +import org.briarproject.bramble.api.crypto.DecryptionException import org.briarproject.bramble.api.crypto.PasswordStrengthEstimator import org.briarproject.bramble.api.crypto.PasswordStrengthEstimator.QUITE_WEAK import org.briarproject.bramble.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENGTH @@ -34,7 +35,9 @@ constructor( } else { val password = prompt("Password", hideInput = true) ?: throw UsageError("Could not get password. Is STDIN connected?") - if (!accountManager.signIn(password)) { + try { + accountManager.signIn(password) + } catch (e : DecryptionException) { echo("Error: Password invalid") exitProcess(1) } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/BriarTestServiceImpl.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/BriarTestServiceImpl.kt index 96ec8f1dd..fa0066d6e 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/BriarTestServiceImpl.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/BriarTestServiceImpl.kt @@ -1,6 +1,7 @@ package org.briarproject.briar.headless import org.briarproject.bramble.api.account.AccountManager +import org.briarproject.bramble.api.crypto.DecryptionException import org.briarproject.bramble.api.lifecycle.LifecycleManager import javax.annotation.concurrent.Immutable import javax.inject.Inject @@ -23,7 +24,9 @@ constructor( accountManager.deleteAccount() } accountManager.createAccount(user, pass) - if (!accountManager.signIn(pass)) { + try { + accountManager.signIn(pass) + } catch (e: DecryptionException) { throw AssertionError("Password invalid") } val dbKey = accountManager.databaseKey ?: throw AssertionError() From bde9800c890b9b2de89449a8cce99d4376d3caf8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Wed, 11 Mar 2020 13:54:01 +0000 Subject: [PATCH 3/3] Add annotation for visibility. --- .../briar/android/login/ChangePasswordActivity.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java index 06685559a..7aadbef8b 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/login/ChangePasswordActivity.java @@ -22,6 +22,7 @@ import org.briarproject.briar.android.activity.BriarActivity; import javax.inject.Inject; +import androidx.annotation.VisibleForTesting; import androidx.lifecycle.ViewModelProvider; import androidx.lifecycle.ViewModelProviders; @@ -52,7 +53,7 @@ public class ChangePasswordActivity extends BriarActivity private Button changePasswordButton; private ProgressBar progress; - // Package access for testing + @VisibleForTesting ChangePasswordViewModel viewModel; @Override