For #12289 Add lib-auth for authentication using biometrics or PIN.

[iorgamgabriel]

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

[escapewindow]

The taskcluster.ci.config change looks reasonable. I'll leave the actual review to people who understand the rest of the patch :)

[jonalmeida]

Looks good! A few more nits to fix and I think we are almost there to land this.

[jonalmeida]

Since we only want this method, can we just make this one an extension function so that we don't have to expose a utils class in the component for only this one?

We try to avoid util classes because they have potential to be a dumping ground for functions that never end up getting used anywhere else. With extension functions, they are more likely to be used (in my biased experience) since the autocomplete for the IDE lists them out as an option.

[iorgamgabriel]

If I do this I can't mock with Mockito extension functions . I need to use mockk . @jonalmeida

[jonalmeida]

Replied back to this thread in another one here: #12291 (comment)

[mergify]

This pull request has conflicts when rebasing. Could you fix it @iorgamgabriel? 🙏

[jonalmeida]

I don't see any usages for this dependency in the tests. Can we remove it if it's not needed?

[iorgamgabriel]

Done

[jonalmeida]

nit: Separate test file for each class file that we have.

BiometricPromptAuth -> BiometricPromptAuthTest
BiometricUtils -> BiometricUtilsTest

etc.

[iorgamgabriel]

Done

[jonalmeida]

Replying back to #12291 (comment) over here as well:

Ah, I didn't catch this earlier. I see that you making the utils class an instance for testing. Consider now though, that if you want to use canUseFeature in your application code, you now have to create an instance of this class every time.

// Expanded into two lines for clarity.
val utils = BiometricUtils()
utils.canUseFeature(context)

// A better API would not need an instance if it was static.
BiometricUtils.canUseFeature(context)

So we've made testing "easier" but it's at the cost of bad design pattern. You have tests for isHardwareAvailable and isEnrolled which is where our logic is and the things we should be testing. The last bit of logic that you would want to test is the bit in canUseFeature:

return isHardwareAvailable(manager) && isEnrolled(manager)

We can write this code in a way so that we pass the manager to it in an internal function (that can be mocked and tested just like you did for the other methods, then leave the Android platform code as a wrapper. What that would look like:

// Our public API for the extension function. Only test to write is for the SDK_INT case when it's false.
fun Context.canUseBiometricFeature(): Boolean {
    return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
        val manager = BiometricManager.from(this)
        return BiometricUtils.canUseFeature(manager)
    } else {
        false
    }
}

// note that this is an object and not a class.
internal object BiometricUtils {

    // Keep our logic in a static function that takes in it's dependencies (here, it's the BiometricManager) as a parameter which we can mock)
    internal fun canUseFeature(manager: BiometricManager): Boolean {
        return isHardwareAvailable(manager) && isEnrolled(manager)
    }

    internal fun isHardwareAvailable(manager: BiometricManager): Boolean {}

    internal fun isEnrolled(manager: BiometricManager): Boolean {}
}

Let me know if that makes sense or not. Happy to talk about it more. 🙂

[iorgamgabriel]

Done

[jonalmeida]

Just one last fix to add the tests and we should be good to land this.

[jonalmeida]

Sorry, maybe I wasn't clear: you can still write tests for these easily I think, but you don't have to make the utils class public.

So the methods can be internal:

    internal fun isHardwareAvailable(biometricManager: BiometricManager): Boolean { }
    internal fun isEnrolled(biometricManager: BiometricManager): Boolean { }

And the tests can take a mocked biometric manager to write the tests for those methods:

    @Test
    fun `isHardwareAvailable is true based on AuthenticationStatus`() {
        val manager: BiometricManager = mock {
            whenever(canAuthenticate(BiometricManager.Authenticators.BIOMETRIC_WEAK))
                .thenReturn(BiometricManager.BIOMETRIC_SUCCESS)
                .thenReturn(BiometricManager.BIOMETRIC_ERROR_HW_UNAVAILABLE)
                .thenReturn(BiometricManager.BIOMETRIC_ERROR_NO_HARDWARE)
        }

        assertTrue(BiometricUtils.isHardwareAvailable(manager))
        assertFalse(BiometricUtils.isHardwareAvailable(manager))
        assertFalse(BiometricUtils.isHardwareAvailable(manager))
    }

    @Test
    fun `isEnrolled is true based on AuthenticationStatus`() {
        val manager: BiometricManager = mock {
            whenever(canAuthenticate(BiometricManager.Authenticators.BIOMETRIC_WEAK))
                .thenReturn(BiometricManager.BIOMETRIC_SUCCESS)
        }
        assertTrue(BiometricUtils.isEnrolled(manager))
    }

The test you added for canUseFeature is good enough coverage for that method.

[iorgamgabriel]

Done

[jonalmeida]

This looks good to go!

Related: I noticed when reviewing this PR, that there very similar code in mozilla.components.feature.autofill.authenticator that has the same flow. I think we should eventually replace that code with what you've built here in a future issue. I've filed #12379 if you're interested in also picked that up.

Since you have multiple commits, we should really make this into one commit before landing. For this PR, I can use "needs landing (squash)" instead.

[iorgamgabriel]

@jonalmeida Thank you ! I will take #12379