Feat/enhanced change listener #47

davidgarciaanton · 2019-07-03T10:21:51Z

As EncryptionProtocol is not accessible and there may be the requirement to respond only to specific keys changed, and the regular SharedPreferences.OnPreferenceChangeListener callback isn't enough as we get the stored version of the key, witch we cannot compare against constants.

I've created a specific OnSecurePreferenceChangeListener that gives a facility to at least compare the changed key against some arbitrary string.

I don't know in terms of security of implementation if this is the best way to achieve this. Please let me know if you think of another way more secure

patrickfav · 2019-07-03T10:28:15Z

Thanks for the PR. I will review it in the next days!

patrickfav

All in all looks good, I think the code can be a bit simplified and made a little easier to understand for the caller.

Important: Please add unit/integration test.

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

armadillo/src/main/java/at/favre/lib/armadillo/SharedPreferenceChangeListenerWrapper.java

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

armadillo/src/main/java/at/favre/lib/armadillo/SharedPreferenceChangeListenerWrapper.java

armadillo/src/main/java/at/favre/lib/armadillo/SecureSharedPreferences.java

patrickfav · 2019-07-07T10:35:57Z

armadillo/src/main/java/at/favre/lib/armadillo/SharedPreferenceChangeListenerWrapper.java

+    }
+
+    private final OnSecurePreferenceChangeListener.DerivedKeyComparison keyComparison;
+    private final WeakReference<OnSecurePreferenceChangeListener> wrappedRef;


I think using WeakReference here might lead to problems. This may be not the behaviour you want and it is quite difficult to understand from the outside why this may not work for a developer. I usually refrain from using Weak/Soft references as they not very clear to most devs how they work (and behave differently on Android ie. more aggresivly). I would make it the callers responsibility to unregister his listeners, as it is usually the case in Android.
As reference why this might be an issue: https://android.jlelse.eu/android-weakreferences-how-not-to-5522bb325935

I know and I myself did suffer some issues at this respect letting Kotlin's lambda to implement directly a OnSharedPreferencesChangeListener which was translated to an anonymous class instantiation, thus being garbage collected (I didn't know at the time how kotlin translated that piece of code, I learnt it by force).

I decided to go with the WeakReference approach just based on the same principle than the SharedPreferences, because well the system indeed treats SharedPreferences instances as singletons, then adding listeners and failing to unregister could cause potential leak.

For me is a design choice, if you prefer I can remove the WeakReferences, but here I like them indeed, for same reason than a user should know well to not register twice a listener, they should know well to keep a reference to the listener (I could add this fact to the javadoc). It's also true that armadillo isn't enforcing any singleton and will be the user of the library who decides how he/she manages that.

Fair argument, however I prefer the easier to understand, explicit way with a little more boilerplate, ie. please change it to register/unregister with a strong reference.

Ok, I've just updated to avoid usage of WeakReferences on listeners

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

patrickfav · 2019-07-07T10:49:38Z

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

+    /**
+     * Allows client side {@link OnSecurePreferenceChangeListener} to check if the changed key is the key of interest.
+     */
+    interface DerivedKeyComparison {


Instead of providing the caller with a generic doesMyKeyMatchWithTheDerivedKey() it may be easier to understand for the callback to provide a checker which has the derived key baked in. Therefore the onSecurePreferenceChanged() would not require to pass the String derivedKeyChanged argument and the caller would just check with e.g. comparison.matchesDerivedKey(String rawContentKey)

Yes! Here's the funny story...

I begun doing it that way indeed, but then I though that in case of positive match I could use the sharedPreferences and key parameters, to get the changed value.

I then realised that this didn't work because the sharedPreferences object was the original one, and requesting a preference with the key parameter would provide the encrypted value.

And then I probably left that as it was. I think that it might be nice to have both of the proposals. So let's see what do you think about having the method signature

void onSecurePreferenceChanged(@NonNull SharedPreferences sharedPreferences, @NonNull DerivedKeyComparison comparison)

Where the instance of sharedPreferences would be the SecureSharedPreferences instead of the underlying SharedPreferencesImpl so that client code could then get the changed value.

I didn't do that at the time, worried about security or something similar. If we're not interesting on allowing such behaviour, then I may suggest to change the signature of the method to just receive the DerivedKeyComparison parameter as right now is the only usable parameter

I like the improved version. What we should change is that the SharedPreferences sharedPreferences should be the Armadillo instance not the inner wrapped version. A) a lib user has no business changing the raw encrypted data B) the raw encrypted shared pref is practically useless for the user, with the Armadillo wrapper she would be able to retrieve the enecrypted data with the content-keys she knows.

patrickfav · 2019-07-07T10:50:20Z

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

+public interface OnSecurePreferenceChangeListener {
+
+    /**
+     * Variation of the regular {@link android.content.SharedPreferences.OnSharedPreferenceChangeListener#onSharedPreferenceChanged(SharedPreferences, String)} that


Please add a simple code example in javadoc on how to use it.

<pre> .... </pre>

davidgarciaanton · 2019-07-08T07:11:22Z

I'll try to give it another round when't I find a moment to it. Thanks for taking your time to review!

davidgarciaanton · 2019-07-15T10:34:49Z

Hi @patrickfav I've updated the PR with some of the corrections you requested, and added some Unit and Integration tests.

Let me know if I missed something.

patrickfav

Very sorry for leaving you hanging this long, had a lot on my mind lately - be sure that I fully appreciate your contribution and hope we can merge it soon.

Basically LGTM only change request worth mentioning is the weak reference, other minor stuff see comments

patrickfav · 2019-08-23T15:13:47Z

armadillo/src/main/java/at/favre/lib/armadillo/OnSecurePreferenceChangeListener.java

+    /**
+     * Allows client side {@link OnSecurePreferenceChangeListener} to check if the changed key is the key of interest.
+     */
+    interface DerivedKeyComparison {


I like the improved version. What we should change is that the SharedPreferences sharedPreferences should be the Armadillo instance not the inner wrapped version. A) a lib user has no business changing the raw encrypted data B) the raw encrypted shared pref is practically useless for the user, with the Armadillo wrapper she would be able to retrieve the enecrypted data with the content-keys she knows.

patrickfav · 2019-08-23T15:14:21Z

armadillo/src/main/java/at/favre/lib/armadillo/SharedPreferenceChangeListenerWrapper.java

-        if (sharedPreferences != null && key != null) {
-            wrapped.onSecurePreferenceChanged(keyComparison, sharedPreferences, key);
+        if (key != null) {
+            wrapped.onSecurePreferenceChanged(securedPrefs, new KeyComparisonImpl(encryptionProtocol, key));


See comment above. I think it would be better to pass to:

wrapped.onSecurePreferenceChanged(this, new KeyComparisonImpl(encryptionProtocol, key));

I'm not sure if I'm understanding correctly. I'm just passing the securedPrefs instance, with is actually initialised with the library SecureSharedPreferences instance, so that as you say in your comment the user doesn't have access to the raw encrypted data, but to the wrapper that will allow to get decrypted info.

yes, I m not sure it is of much use to get the encrypted data, now would it?

Well, but actually the user will be able to access the decrypted data through armadillo's SecureSharedPreferences instance.

What is exactly the change you're proposing here?

armadillo/src/main/java/at/favre/lib/armadillo/SecureSharedPreferences.java

patrickfav · 2019-08-23T15:21:24Z

armadillo/src/main/java/at/favre/lib/armadillo/SharedPreferenceChangeListenerWrapper.java

+    }
+
+    private final OnSecurePreferenceChangeListener.DerivedKeyComparison keyComparison;
+    private final WeakReference<OnSecurePreferenceChangeListener> wrappedRef;


Fair argument, however I prefer the easier to understand, explicit way with a little more boilerplate, ie. please change it to register/unregister with a strong reference.

patrickfav · 2019-09-08T18:03:00Z

LGTM.

davidgarciaanton added 2 commits July 3, 2019 10:59

Enhanced preference listener

6172dfb

Check style fixes

ebad186

patrickfav requested changes Jul 7, 2019

View reviewed changes

Applying comments and corrections

bca99b0

davidgarciaanton added 2 commits July 15, 2019 12:49

Couple of corrections on javadoc and checkstyle

896aaa9

Checkstyle on tests too

3c271a7

patrickfav requested changes Aug 23, 2019

View reviewed changes

Removing WeakReference for the listener

5911a01

patrickfav merged commit e580dfc into patrickfav:master Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/enhanced change listener #47

Feat/enhanced change listener #47

davidgarciaanton commented Jul 3, 2019

patrickfav commented Jul 3, 2019

patrickfav left a comment •

edited

Loading

patrickfav Jul 7, 2019

davidgarciaanton Jul 8, 2019

patrickfav Aug 23, 2019

davidgarciaanton Aug 24, 2019

patrickfav Jul 7, 2019 •

edited

Loading

davidgarciaanton Jul 8, 2019

patrickfav Aug 23, 2019

patrickfav Jul 7, 2019

davidgarciaanton commented Jul 8, 2019

davidgarciaanton commented Jul 15, 2019

patrickfav left a comment

patrickfav Aug 23, 2019

patrickfav Aug 23, 2019

davidgarciaanton Aug 24, 2019

patrickfav Aug 24, 2019

davidgarciaanton Aug 26, 2019

patrickfav Aug 23, 2019

patrickfav commented Sep 8, 2019

Feat/enhanced change listener #47

Feat/enhanced change listener #47

Conversation

davidgarciaanton commented Jul 3, 2019

patrickfav commented Jul 3, 2019

patrickfav left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfav Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgarciaanton commented Jul 8, 2019

davidgarciaanton commented Jul 15, 2019

patrickfav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfav commented Sep 8, 2019

patrickfav left a comment •

edited

Loading

patrickfav Jul 7, 2019 •

edited

Loading