mjwheatley / cordova-plugin-android-fingerprint-auth

A cordova plugin for fingerprint authentication using the hardware fingerprint scanner on devices running Android 6+
Apache License 2.0
169 stars 131 forks source link

Mixed up configurations for encrypt and decrypt #129

Open massic80 opened 5 years ago

massic80 commented 5 years ago

Hi all, I'm using this plugin and it works nicely, but I just noticed a bug: forgive me for not submitting an example, but I think you can easily reproduce it in your environment:

  1. On first access, users enter username and password to login. When they login, I encrypt their data passing

    encryptConfig = {
    clientId: 'someclientid',
    locale: 'it',
    username: 'someuser',
    password: 'somepass
    }
  2. everything works as expected and I get no custom messages in the Authentication Dialog

  3. on second landing I run decrypt with a custom decryptConfig.dialogMessage:

    decryptConfig = {
    clientId: 'someclientid',
    locale: 'it',
    username: 'someuser',
    token: 'sometoken',
    dialogMessage: 'hey there!'
    }
  4. everything works as expected and I get the custom hey there! message in the Authentication Dialog

  5. when users cancel the dialog, they may choose to login with another account.

  6. they enter username and password.

  7. I repeat step 1, with the same but I get the same hey there! message

I figure out there must be something wrong in plugin's Java code, since my config objects are completely separate

mjwheatley commented 5 years ago

@massic80 That is a good catch. The custom strings are saved as member variables at the class level and are not being reset because the class is not re-instantiated for subsequent method calls and therefore the custom string carries over from your decryptConfig to your next call to encrypt.

To fix the issue, the Java class for FingerprintAuth will need to be updated to reset the member variables to null if the custom property is not found in the params. If you would like to contribute, you can make the changes and send a pull request or I can make the updates on your behalf.

public static String mDialogTitle;
public static String mDialogMessage;
public static String mDialogHint;
...

if (arg_object.has("dialogTitle")) {
    mDialogTitle = arg_object.getString("dialogTitle");
} else {
    mDialogTitle = null;
}

if (arg_object.has("dialogMessage")) {
    mDialogMessage = arg_object.getString("dialogMessage");
} else {
    mDialogMessage = null;
 }

if (arg_object.has("dialogHint")) {
    mDialogHint = arg_object.getString("dialogHint");
} else {
    mDialogHint = null;
}
massic80 commented 5 years ago

Hi @mjwheatley , thanks for the good catch :) I still didn't attempt to brush up on my Java skill and dare to merge the code you kindly submitted, since I have a doubt: does it make sense to "keep" those variables alive when they are not needed anymore? Why don't you throw them away, once the encrypt or decrypt end?

massic80 commented 5 years ago

Hi, I created a pull request with your code: I still wonder why you don't garbage collect those variables when they are not anymore needed :)

mjwheatley commented 5 years ago

Could you describe how those variables would be eligible for garbage collection or how to make them so? I'm not terribly familiar with it. https://www.google.com/amp/s/www.geeksforgeeks.org/how-to-make-object-eligible-for-garbage-collection/amp/

massic80 commented 5 years ago

Forgive me if I'm not technical, since it's been ten years since my last line of Java code :) With garbage collecting I was meaning throwing the variables away once they have been used (in general, they are garbage-collected when not referenced). I didn't dare to analyze the code, and I figured out you had an encrypt and a decrypt method, which would have encapsulated the local variables, and I wondered why you used a global one, instead of just taking the parameters and let them die when the methods finished. Now I notice that you have a big switch, I figure out due to how plugins are made. Those variables are global and public, probably cause read from an external module. Never mind :)