Add Password protection option #192

MoDevby · 2019-01-22T03:40:44Z

Close #187

Store plain password in SharedPreferences.

brarcher · 2019-01-22T06:27:57Z

I'll take a look at your change in a bit. The build failure is from the findbugs task, which runs the FindBugs Java static analysis tool. Could you run that tool locally to see if it is pointing out any bugs or issues that need to be addressed. To run it:

./gradlew findbugs

brarcher · 2019-01-22T06:52:14Z

app/src/main/java/protect/budgetwatch/MainActivity.java

        public final int iconId;
        public final int menuTextId;
        public final int menuDescId;

-        public MainMenuItem(int iconId, int menuTextId, int menuDescId)
-        {
+        public MainMenuItem(int iconId, int menuTextId, int menuDescId) {


Can you keep the style the same as before, namely where the { } are?

brarcher · 2019-01-22T07:10:53Z

FYI, I was able to bypass the password protection in the following ways:

When the password prompt is shown press "back". This leads to the main activity without requiring the password.
Add an icon to the home screen. Long press the icon. This brings up a static menu to add a revenue or expense. Click on either opens the TransactionViewActivity without requiring a password.

brarcher · 2019-01-22T07:15:25Z

app/src/main/java/protect/budgetwatch/MainActivity.java

+    }
+
+    private void startAuthorization() {
+        authorized = true;


How is the main activity receiving a response from the authentication activity that the authentication was successful? The expectation here is that the authentication activity will not "return" until a password is entered, however this is not the case. A better pattern may be to startActivityForResult(). See documentation here:

https://developer.android.com/training/basics/intents/result

That will allow the return result from the authentication activity to be captured, and a more informed decision made about authentication success or failure.

You are correct, i changed this and that allowed me to fix the issue related to bypassing the password when clicking back button.

brarcher · 2019-01-22T07:24:34Z

app/src/main/java/protect/budgetwatch/PasswordManager.java

+            // Generate Hash
+            byte[] hash = generateHash(password, salt);
+            // Write to SharedPreferences
+            _prefs.edit().putString(PasswordKey, toHex(salt) + ":" + toHex(hash)).commit();


Why not store the salt and password hash as separate entries, so you do not need to parse it out when you retrieve it?

I don't think that's necessary, usually saveing the hash and the salt together is a common practice.

brarcher · 2019-01-22T07:25:09Z

app/src/main/java/protect/budgetwatch/PasswordManager.java

+            byte[] hash = generateHash(password, salt);
+            // Write to SharedPreferences
+            _prefs.edit().putString(PasswordKey, toHex(salt) + ":" + toHex(hash)).commit();
+        } catch (Exception e) {


Is this a bit of a wide net to catch? It would be better to catch the specific exception(s), as well as log them.

The issue is i don't know what kind of exception can be thrown here.
The main propose of this catch is to prevent the App from crashing in my opinion.

brarcher · 2019-01-22T07:27:51Z

app/src/main/java/protect/budgetwatch/PasswordManager.java

+        byte[] passwordHash = generateHash(password, salt);
+
+        int diff = hash.length ^ passwordHash.length;
+        for (int i = 0; i < hash.length && i < passwordHash.length; i++) {


Why not Arrays.equals()?

https://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html#equals(byte[],%20byte[])

Correct, changed.

brarcher · 2019-01-22T07:45:18Z

app/src/main/java/protect/budgetwatch/PasswordManager.java

+            return skf.generateSecret(spec).getEncoded();
+        } catch (Exception e) {
+            e.printStackTrace();
+            return new byte[0];


I'm thinking if generating the key failed the error should be presented to the user, instead of attempting to save an empty password. I simulated this failing by editing the preferences file and setting an empty password hash. The result was when a password was entered to authorize when it was compared an out of bounds exception resulted:

01-21 23:43:40.888 6693 6693 E AndroidRuntime: FATAL EXCEPTION: main 01-21 23:43:40.888 6693 6693 E AndroidRuntime: Process: protect.budgetwatch, PID: 6693 01-21 23:43:40.888 6693 6693 E AndroidRuntime: java.lang.ArrayIndexOutOfBoundsException: length=1; index=1 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at protect.budgetwatch.PasswordManager.isPasswordCorrect(PasswordManager.java:58) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at protect.budgetwatch.PasswordAuthenticationActivity.attemptLogin(PasswordAuthenticationActivity.java:65) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at protect.budgetwatch.PasswordAuthenticationActivity.access$000(PasswordAuthenticationActivity.java:14) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at protect.budgetwatch.PasswordAuthenticationActivity$2.onClick(PasswordAuthenticationActivity.java:44) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.view.View.performClick(View.java:6597) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.view.View.performClickInternal(View.java:6574) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.view.View.access$3100(View.java:778) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.view.View$PerformClick.run(View.java:25885) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.os.Handler.handleCallback(Handler.java:873) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.os.Handler.dispatchMessage(Handler.java:99) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.os.Looper.loop(Looper.java:193) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at android.app.ActivityThread.main(ActivityThread.java:6669) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at java.lang.reflect.Method.invoke(Native Method) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493) 01-21 23:43:40.888 6693 6693 E AndroidRuntime: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

brarcher · 2019-01-23T05:35:52Z

Looking over most of the changes and playing with it on an emulator, I understand the change is focused on preventing a user from accessing the application rather than protecting the underlying data. I would support a change to encrypt the underlying data, which would require a password to unlock. Further, the key would need to be stored in Android's Keystore. If you are interested in pursuing this change I can help with advice and code reviews. I've not used the Java Keystore APIs before, so perhaps we can learn together.

I appreciate your sending the changes you have. The nature of the proposed changes I'll not be able to accept, however, as they only attempt to hide the data rather than encrypting it.

brarcher · 2019-01-23T05:37:54Z

app/src/main/java/protect/budgetwatch/PasswordManager.java

+        }
+    }
+
+    private String toHex(byte[] array) {


Would something like Guava's BaseEncoding be useful?

https://google.github.io/guava/releases/16.0/api/docs/com/google/common/io/BaseEncoding.html

Small fixes

MoDevby · 2019-04-02T16:56:08Z

FYI, I was able to bypass the password protection in the following ways:

1. When the password prompt is shown press "back". This leads to the main activity without requiring the password.

2. Add an icon to the home screen. Long press the icon. This brings up a static menu to add a revenue or expense. Click on either opens the TransactionViewActivity without requiring a password.

I solved this by using startActivityForResult as suggested by you.
I intentionally want this behaviour, as if you remember from the discussion in my request Feature Request: password or pattern protection #187, the whole goal is to prevent the peeking into the financial info but adding an expense or revenue shouldn't be a problem (specially that these are shortcuts it should be for fast actions so no password protection is needed here).

MoDevby added 4 commits November 14, 2018 23:21

Implement simple password protection mechanism.

96637f4

Store plain password in SharedPreferences.

Extract to Password Manager and Change Activities Name

898ac3d

Add Test Covarage

5c50226

Hash The Password

cfe2178

brarcher reviewed Jan 22, 2019

View reviewed changes

brarcher reviewed Jan 23, 2019

View reviewed changes

MoDevby added 4 commits April 2, 2019 20:15

Revert the bracket style

f548af7

Use startActivityForResult

c505d6a

Fix bypass using the back button

6e7ed69

Make use of existing libraries in PasswordManager

bf52a43

Small fixes

MoDevby added 2 commits April 3, 2019 01:01

Merge branch 'master' into master

1fb5fcd

Merge branch 'master' into master

615bb5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Password protection option #192

Add Password protection option #192

MoDevby commented Jan 22, 2019

brarcher commented Jan 22, 2019

brarcher Jan 22, 2019

MoDevby Apr 2, 2019

brarcher commented Jan 22, 2019

brarcher Jan 22, 2019

MoDevby Apr 2, 2019

brarcher Jan 22, 2019

MoDevby Apr 2, 2019

brarcher Jan 22, 2019

MoDevby Apr 2, 2019

brarcher Jan 22, 2019

MoDevby Apr 2, 2019

brarcher Jan 22, 2019

brarcher commented Jan 23, 2019

brarcher Jan 23, 2019

MoDevby Apr 2, 2019

MoDevby commented Apr 2, 2019 •

edited

Add Password protection option #192

Are you sure you want to change the base?

Add Password protection option #192

Conversation

MoDevby commented Jan 22, 2019

brarcher commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Jan 22, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoDevby commented Apr 2, 2019 • edited

MoDevby commented Apr 2, 2019 •

edited