#635 move authentication to a background thread #738
Conversation
Get rid of the need for separate checks for setting layout hints and input type. Add some clarifying comments.
… a background thread
…icating Began disabling the password input and unlock buttons while the task is running, for some UI feedback. Right now it's just waiting for the task to finish if the activity gets paused while it's running; this isn't great since there's potential there to lock the main thread, but the task is so short-lived that it may not be an issue.
…ck button) while authenticating
…ability to show/hide the password text while unlocking)
|
Thank you a lot, I hope I find the time to review this soon. |
|
I haven't found the time yet to review the PR in detail, but I noticed you used an AsyncTask. I looked into that as well for the creating backups in a background process and noticed that it is set to be deprecated in API level 30: https://developer.android.com/reference/android/os/AsyncTask Do you think it would be feasible to use something else to do this? I decided on using a native Java Runnable and an ExecutorService. You can take look at it in this commit: feeeba7 If you want to put in the extra work I would appreciate it if you could move it of AsyncTask yourself. But I can understand if you don't have the time or don't want to. In that case I will merge it as it is (after a more detailed review) and port it myself later on when I find some time. |
|
Oh sorry, I completely forgot that ASyncTask was being deprecated. I can definitely move it into using an executor with a main thread Handler instead. We could also pull in RxJava (though that's adding another dependency), but it has some nice threading utilities built in (default schedulers for background work, Android main thread scheduler for pushing to the main thread, etc). Let me know if you prefer one over the other, it's not a big change either way so if I don't hear back I'll just use an executor. |
|
I would prefer to avoid additional dependencies if not strictly necessary, so I would prefer an executor. Even though I posted my commit as example, you probably should not look at it to much. It was my first time doing something like this in an Android app, so it's probably not very clean. You seem to have some more experience in that regard, so I will probably look at your code afterwards trying to learn something from it ;) |
|
That makes sense, I'll stick with an executor service. Haha I have some experience with threading in Android, but any way you go it's never fun to tie background work back to the UI in a clean way (hence my .get() call in the code I put up). We might be able to use a base executor and send a lambda/callable in to do background work that'll make things like this easier in the future, I'll definitely take a look while modifying my code within the next few days |
… calls back to the UI upon completion
…d make sure that AuthenticateActivity can receive results across configuration changes Begin storing the task in a retained fragment so that if the task is begun, we will be able to get a callback when the activity is destroyed/recreated.
This ensure that if the user goes home, the callback will be reset when coming back (since onCreate doesn't get called most of the time in this instance).
|
Added a base I went ahead and implemented the retained fragment concept that I was talking about earlier while I was at it. This means that on configuration changes (i.e the user rotates their device, goes home, etc), the task will continue to run and if the activity is eventually brought back, we can reconnect to the task and get our callback. I made sure to set the callback to null when pausing so that we wouldn't ever get a callback to a dead activity instance. All in all, the user can rotate their device and the main thread won't be paused (it will just show progress again when recreated, until the task completes). If the task completes between configuration changes, it retains the result and just calls back immediately when a new callback is set (in the activity's onResume). |
| @Override | ||
| public void onBackPressed() { | ||
| finishWithResult(false, null); | ||
| super.onBackPressed(); | ||
| } | ||
|
|
||
| /** Retained instance fragment to hold a running @link{{@link AuthenticationTask}} between configuration changes.*/ |
There was a problem hiding this comment.
Just noticed this weird double javadoc link, I'll fix that real quick on my lunch break
…uthenticationTask
|
I finally found the time to review the code and I didn't really find anything I would change in the implementation. The only problem I found come from the "Re-lock on screen off" and "Re-lock when going into the background" options. Edit: And I'm definitely going to refactor my backup task to use your |
Good point, thank you for catching that. I'll make sure to check that setting for this case and cancel the task if it applies. I'm not sure off the top of my head how to tell when we're entering/coming back from a lock or background event vs a normal config change, but I'll take a look where that setting is being checked currently and add that in. |
…off if enabled in settings Note that we can't just remove the fragment from the fragment manager when we get a screen off callback, because Android doesn't allow you to remove fragments after onSaveInstanceState()
…oing to the background if set to do so
…o avoid memory leaks
|
I added the ability to cancel a |
| unlockButton.setOnClickListener(this); | ||
| private void checkBackgroundTask() { | ||
| TaskFragment taskFragment = findTaskFragment(); | ||
| if (taskFragment != null && !taskFragment.task.isCanceled()) { |
There was a problem hiding this comment.
Could you do something like this here instead (not sure if this is the best way but it works):
if (taskFragment != null) {
if (taskFragment.task.isCanceled()) {
passwordInput.setText("");
passwordInput.requestFocus();
InputMethodManager keyboard = (InputMethodManager) getSystemService(Context.INPUT_METHOD_SERVICE);
keyboard.showSoftInput(passwordInput, 0);
} else {
taskFragment.task.setCallback(this::handleResult);
setupUiForTaskState(true);
}
}
Otherwise the password stays entered when returning from background, which makes the re-lock useless.
And could you exclude the AuthenticateActivity from being finished in destroyIfNotMain() in the BaseActivity as well? Otherwise it will show the "Authentication failed, closing" warning after screen off and end the app because the AuthenticateActivity gets killed
The manual showSoftInput seems to be required after screen-off, otherwise it did not show the keyboard again for me.
|
Thanks for the review, I'll make sure to add all of that in. I noticed the failure toast when the Activity dies and that definitely makes sense to not show when it's killed. Did you think we should also prevent that toast from showing when finishing from onBackPressed()? |
…om cancelling authentication
This ensures that we aren't resetting password input on configuration changes after our task has been canceled
|
Added the requested changes:
Tested those changes and everything seems to work as expected, but let me know if you see anything else and I'll be happy to change it |
|
Looks good, thank you for implementing this! |
Ok, so I added an ASyncTask to run the authentication code, with a callback to the Activity when it completes (still finishing the Activity on the main thread). While the task is running, I added an indeterminate progress bar that replaces the "unlock" button to provide some feedback to the user:
andOTPUnlockProgress_2021-1-29.mp4
One small caveat here is that I call
.get()on the task if the Activity is being paused. I did this to prevent the task from calling back to a dead activity for the case where the user initiates the unlock process and rotates their phone (or some other config change).If this situation happens, the task will complete and the Activity will finish as expected, however the main thread will be locked while
.get()is called (as it was before). Locking the main thread here isn't ideal, but seeing as this is a short-lived (1-2 second) task and the situation is unlikely to happen it's probably not a big issue.To get around this we'd probably have to start using a retained fragment within the Activity (to store the task), and then in
onCreatecheck for that retained fragment and tie the newly created Activity into it for a callback. This is definitely doable, however it'd probably make more sense to wait and see if we implement a general background execution scheme for the app before doing that work.Let me know if you see any issues and I'll make necessary changes. I tested this on a Samsung device and on an emulator for both the cases of regular login and changing/upgrading authentication, with configuration changes in between and didn't run into any issues with the changes.