Authenticators backport is broken on an API 29 Pixel 4 [170406186]

Fixed

Bug

Status Update

No update yet.

Description

ph...@monzo.com

created issue #1

Oct 9, 2020 09:43AM

On an API 29 Pixel 4 androidx.biometric.BiometricManager#canAuthenticate(Authenticators.BIOMETRIC_STRONG) returns BIOMETRIC_STATUS_UNKNOWN. I believe this is because the canAuthenticate(CryptoObject) method only works on Samsung devices - as far as I can tell it has never existed in AOSP.

Some possible solutions (there may well be reasons not to do these things):

Adding non-Samsung devices with strong face unlock to the assume_strong_biometrics_prefixes array, which is currently empty. Devices with fingerprint should be OK because they fall back to canAuthenticateWithFingerprintOrUnknown(), but I haven't actually tested this.
Attempt to create an auth-per-use key pair (setUserAuthenticationValidityDurationSeconds(-1)) in AndroidKeyStore, which fails with the following exception if the user doesn't have strong biometrics available & enrolled (I don't know if this is reliable across OEMs and API versions):

java.security.InvalidAlgorithmParameterException: java.lang.IllegalStateException: At least one biometric must be enrolled to create keys requiring user authentication for every use
        at android.security.keystore.AndroidKeyStoreKeyPairGeneratorSpi.initialize(AndroidKeyStoreKeyPairGeneratorSpi.java:349)
        at java.security.KeyPairGenerator$Delegate.initialize(KeyPairGenerator.java:697)
        at java.security.KeyPairGenerator.initialize(KeyPairGenerator.java:438)
        ...

BUILD INFO

Device type: Pixel 4
OS version: 10 (API 29)
Biometric library version: 1.1.0-beta01

STEPS TO REPRODUCE

Have API 29 Pixel 4 with face unlock turned on.
Call androidx.biometric.BiometricManager#canAuthenticate(Authenticators.BIOMETRIC_STRONG)

EXPECTED RESULTS

Receive BIOMETRIC_SUCCESS.

OBSERVED RESULTS

Receive BIOMETRIC_STATUS_UNKNOWN.

NUMBER OF TIMES YOU WERE ABLE TO REPRODUCE (e.g. 3/10)

10/10

Comments

ph...@monzo.com <ph...@monzo.com> #2Oct 9, 2020 04:01PM

Thanks for the detailed analysis and sample. I've prepared a CL, but I'd also be happy to review an external contribution since you already went through the trouble of identifying a fix. Just let me know here.

cu...@google.com <cu...@google.com> Oct 19, 2020 05:15PM

Reassigned to cu...@google.com.

cu...@google.com <cu...@google.com> Oct 21, 2020 12:25AM

Accepted by cu...@google.com.

ap...@google.com <ap...@google.com> #3Oct 21, 2020 08:33PM

Marked as fixed.

If you have a CL ready go for it.

That being said, I just went down git history and found when this was introduced: 8d4765883ff24f0f79bf64fea23d4687d50cec85 (and the null case was added in 740e50456ff7aa7b93d40e7bdc59f8a619197ed5.

As you can see, this was introduced to fix a "memory leak" though I don't quite understand why creating a new callback wrapper for every setSupportActionBar() call is a problem.

ph...@monzo.com <ph...@monzo.com> #4Oct 23, 2020 08:17AM

Project: platform/frameworks/support
Branch: androidx-main

commit 200ccecbc8c7c5ba555e7fa15890edff67affb58
Author: Alan Viverette <alanv@google.com>
Date: Fri Apr 30 14:55:07 2021

Fix case where setSupportActionBar overwrites the window callback

RelNote: """Fix scenario where calling setSupportActionBar after setting the
window callback would overwrite the callback."""
Test: SupportActionBarTestCase
Fixes: 186791590
Change-Id: Ie43eedf8322caa44e7b201a95cbc64197953e020

A appcompat/appcompat/src/androidTest/java/androidx/appcompat/app/SupportActionBarTestCase.kt
M appcompat/appcompat/src/main/java/androidx/appcompat/app/AppCompatDelegateImpl.java

https://android-review.googlesource.com/1692567

[Deleted User] <[Deleted User]> #5Dec 20, 2020 02:23PM

@4 May all bug be fixed this quick :)

re...@infinum.com <re...@infinum.com> #6Jan 7, 2021 10:16AM

I vaguely remember this. The issue was iirc:

You use multiple fragments (say switched via tabs). Each fragment calls setSupportActionBar() when visible.
The old code would wrap the current Window.Callback with something from ToolbarActionBar (TAB), then set it as the window callback.
When the user switches to Tab #2, we'd do the same thing, but this time the callback wrapper chain would be (((Root callback) TBAB1Wrapper) TBAB2Wrapper). TBAB1Wrapper would keep a reference a reference to the views even though the #1 fragment has likely been destroyed.

I've just +2'd Alan's CL, but the proper fix is properly to unwrap the callback from Window.getCallback().

re...@infinum.com <re...@infinum.com> #7Jan 7, 2021 10:18AM

Let me uh... let me go back and properly fix that.