Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multithreaded Correctness and Callback support #14

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Copyright (c) 2016. Bottle Rocket LLC
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.bottlerocketstudios.vault.test;

import android.content.SharedPreferences;
import android.test.AndroidTestCase;
import android.util.Log;

import com.bottlerocketstudios.vault.SharedPreferenceVault;
import com.bottlerocketstudios.vault.SharedPreferenceVaultFactory;
import com.bottlerocketstudios.vault.StandardSharedPreferenceVault;
import com.bottlerocketstudios.vault.keys.generator.Aes256RandomKeyFactory;

import java.security.GeneralSecurityException;

import javax.crypto.SecretKey;

/*
* Test to see if the listener is called correctly when it should be.
*/
public class TestSharedPrefListener extends AndroidTestCase {
private static final String TAG = TestSharedPrefListener.class.getSimpleName();

private static final String KEY_FILE_NAME = "listenerKeyTest";
private static final String PREF_FILE_NAME = "listenerPrefTest";
private static final String KEY_ALIAS_1 = "listenerKeyAlias";
private static final int KEY_INDEX_1 = 1;
private static final String PRESHARED_SECRET_1 = "a;sdl564546a6s6w2828d4fsdfbsijd;saj;9dj9";

private static final String TEST_STRING_KEY = "testKey";
private static final String TEST_STRING_VALUE = " This is a test. ";

private SharedPreferenceVault mSharedPreferenceVault;

// Initialization function. Should be called by all tests but only initialized once. On previous
// invocations, the storage is cleared and that is it
private boolean shouldInit = true;
private void init() {
Log.i(TAG, "Initializing");
if(shouldInit) {
mSharedPreferenceVault = null;
try {
Log.i(TAG, "Creating SharedPreference vault.");
mSharedPreferenceVault = SharedPreferenceVaultFactory.getAppKeyedCompatAes256Vault(
getContext(),
PREF_FILE_NAME,
KEY_FILE_NAME,
KEY_ALIAS_1,
KEY_INDEX_1,
PRESHARED_SECRET_1);
} catch (GeneralSecurityException e) {
Log.e(TAG, "Caught java.security.GeneralSecurityException", e);
assertTrue("Exception creating vault", false);
}

assertNotNull("Unable to create initial vault", mSharedPreferenceVault);
assertTrue("Testing StandardSharedPreferenceVault",
mSharedPreferenceVault instanceof StandardSharedPreferenceVault);

shouldInit = false;
}
cleanStorage();
}

private void cleanStorage() {
mSharedPreferenceVault.rekeyStorage(Aes256RandomKeyFactory.createKey());
assertNull("Rekey of storage did not clear existing value",
mSharedPreferenceVault.getString(TEST_STRING_KEY, null));
}

public void testSuccessCommit() {
init();
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertTrue("Error when committing to sharedPrefs", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit();
}

public void testSuccessApply() {
init();
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertTrue("Error when applying to sharedPrefs", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply();
}

public void testNullSecretKeyFailureCommit() {
init();
mSharedPreferenceVault.clearStorage();
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertFalse("The key is still initialized", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazysp333 Should this be editor.putString(null, TEST_STRING_VALUE).commit();

Copy link
Author

@JRProd JRProd Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dev777 This is the null secret key, AKA the user is trying to encrypt data without having a secret key set. That is why I clear the storage before.

If you would also like, I can add a test case for a attempt to set a null shared pref key as well

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazysp333 I think that renaming this test case to testNullSecretKeyFailureCommit and then adding the test case for null shared pref key makes sense. Same recommendation for the testNullKeyFailureApply comment as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dev777 Another thing, putting a null value in this IS acceptable in its current state, the editor.commit() returns true (same with apply). Is this expected behavior?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have been explained and fixed

}

public void testNullSecretKeyFailureApply() {
init();
mSharedPreferenceVault.clearStorage();
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertFalse("The key is still initialized", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazysp333 Should this be editor.putString(null, TEST_STRING_VALUE).apply();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have been explained and fixed

}

private final SecretKey malformedSecretKey = new SecretKey() {
private final String mKey = "ThisIsMySuperSecretKey";

@Override
public String getAlgorithm() {
return "BestAlgorithEver";
}

@Override
public String getFormat() {
return mKey;
}

@Override
public byte[] getEncoded() {
return mKey.getBytes();
}
};
public void testMalformedKeyFailureCommit() {
init();
mSharedPreferenceVault.rekeyStorage(malformedSecretKey);
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertFalse("Malformed key still works", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).commit();
}

public void testMalformedKeyFailureApply() {
init();
mSharedPreferenceVault.rekeyStorage(malformedSecretKey);
SharedPreferences.Editor editor = mSharedPreferenceVault.setSharedPrefVaultWriteListener(
new SharedPrefVaultWriteListenerExtension(new OnComplete() {
@Override
public void onComplete(boolean success) {
assertFalse("Malformed key still works", success);
}
})
).edit();

editor.putString(TEST_STRING_KEY, TEST_STRING_VALUE).apply();
}

public interface OnComplete {
void onComplete(boolean success);
}

private class SharedPrefVaultWriteListenerExtension implements SharedPreferenceVault.SharedPrefVaultWriteListener {
private final OnComplete mListener;

public SharedPrefVaultWriteListenerExtension(OnComplete listener) {
mListener = listener;
}

@Override
public void onSuccess() {
mListener.onComplete(true);
}

@Override
public void onError() {
mListener.onComplete(false);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@
* Shared Preferences backed vault for storing sensitive information.
*/
public interface SharedPreferenceVault extends SharedPreferences {

/**
* Interface to handle atypical results from the standard {@link Editor#commit()} and {@link Editor#apply()}
*/
interface SharedPrefVaultWriteListener {
void onSuccess();
void onError();
}

/**
* Remove all stored values and destroy cryptographic keys associated with the vault instance.
* <strong>This will permanently destroy all data in the preference file.</strong>
Expand Down Expand Up @@ -65,4 +74,10 @@ public interface SharedPreferenceVault extends SharedPreferences {
* Method to find out expected security level of KeyStorage implementation being used.
*/
KeyStorageType getKeyStorageType();

/**
* Add a listener to handle atypical behavior when {@link Editor#commit()} or {@link Editor#apply()} is used
* with Vault.
*/
SharedPreferenceVault setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener);
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public class StandardSharedPreferenceVault implements SharedPreferenceVault {
private final List<OnSharedPreferenceChangeListener> mSharedPreferenceChangeListenerList = Collections.synchronizedList(new LinkedList<OnSharedPreferenceChangeListener>());

private SharedPreferences mSharedPreferences;
private SharedPrefVaultWriteListener mListener;
private boolean mDebugEnabled;

public StandardSharedPreferenceVault(Context context, KeyStorage keyStorage, String prefFileName, String transform, boolean enableExceptions) {
Expand All @@ -66,6 +67,7 @@ public StandardSharedPreferenceVault(Context context, KeyStorage keyStorage, Str
mSharedPreferenceName = prefFileName;
mTransform = transform;
mEnableExceptions = enableExceptions;
mListener = null;
}

boolean writeValues(boolean commit, boolean wasCleared, Set<String> removalSet, StronglyTypedBundle stronglyTypedBundle) {
Expand Down Expand Up @@ -291,7 +293,7 @@ public boolean contains(String key) {

@Override
public Editor edit() {
return new StandardSharedPreferenceVaultEditor(this);
return new StandardSharedPreferenceVaultEditor(this, mListener);
}

private void notifyListeners(Set<String> preferenceKeySet) {
Expand Down Expand Up @@ -351,6 +353,13 @@ public KeyStorageType getKeyStorageType() {
return mKeyStorage.getKeyStorageType();
}

@Override
public SharedPreferenceVault setSharedPrefVaultWriteListener(SharedPrefVaultWriteListener listener) {
log("Adding a write listener to the SharedPreferenceVault.");
mListener = listener;
return this;
}

private void log(String message) {
if (isDebugEnabled()) Log.e(TAG, message);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
public class StandardSharedPreferenceVaultEditor implements SharedPreferences.Editor {

private final StandardSharedPreferenceVault mStandardSharedPreferenceVault;
private final SharedPreferenceVault.SharedPrefVaultWriteListener mListener;
private StronglyTypedBundle mStronglyTypedBundle = new StronglyTypedBundle();
private boolean mCleared;
private Set<String> mRemovalSet = new HashSet<>();

public StandardSharedPreferenceVaultEditor(StandardSharedPreferenceVault standardSharedPreferenceVault) {
public StandardSharedPreferenceVaultEditor(StandardSharedPreferenceVault standardSharedPreferenceVault,
SharedPreferenceVault.SharedPrefVaultWriteListener listener) {
mStandardSharedPreferenceVault = standardSharedPreferenceVault;
mListener = listener;
}

@Override
Expand Down Expand Up @@ -85,11 +88,31 @@ public SharedPreferences.Editor clear() {

@Override
public boolean commit() {
return mStandardSharedPreferenceVault.writeValues(true, mCleared, mRemovalSet, mStronglyTypedBundle);
boolean success = mStandardSharedPreferenceVault.writeValues(
true,
mCleared,
mRemovalSet,
mStronglyTypedBundle);
if(mListener != null && success) {
mListener.onSuccess();
} else if(mListener != null && !success) {
mListener.onError();
}

return success;
}

@Override
public void apply() {
mStandardSharedPreferenceVault.writeValues(false, mCleared, mRemovalSet, mStronglyTypedBundle);
boolean success = mStandardSharedPreferenceVault.writeValues(
false,
mCleared,
mRemovalSet,
mStronglyTypedBundle);
if(mListener != null && success) {
mListener.onSuccess();
} else if(mListener != null && !success) {
mListener.onError();
}
}
}
Loading