Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support all authentication results for BiometricManager #7427

Conversation

charlesng
Copy link
Contributor

Overview

For #7214

Hello, this is my first PR to robolectric, and would like to contribute it 馃檱 Appreciate any comment or advices if you have.

Proposed Changes

  • Add Test cases for other result in function int canAuthenticate in BiometricManager and int canAuthenticate(int userId, int authenticators)
  • Add function setEnrolledBiometrics , hasHardware , setAuthenticatorType, requireSecurityUpdate to control different biometric result

@utzcoz
Copy link
Member

utzcoz commented Jul 2, 2022

@charlesng Could you help to squash two commits to one and follow https://github.com/robolectric/robolectric/wiki/Running-google-java-format to apply google java format to your commit?

@charlesng charlesng force-pushed the feature/support-all-authentication-results-for-biometric-manager branch from 969f50b to 8fdd360 Compare July 2, 2022 07:44
@charlesng
Copy link
Contributor Author

@utzcoz I have resolved the comments and feel free to check when you are free 馃檹

@utzcoz
Copy link
Member

utzcoz commented Jul 9, 2022

@charlesng Could you squash commits to one? Thanks.

@charlesng charlesng force-pushed the feature/support-all-authentication-results-for-biometric-manager branch from 0be52be to 613e3ac Compare July 9, 2022 14:44
@charlesng
Copy link
Contributor Author

@utzcoz I have squashed the commits to one, feel free to check when you are free 馃檱

Copy link
Member

@utzcoz utzcoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@utzcoz
Copy link
Member

utzcoz commented Sep 24, 2022

Hi @hoisie , could you help to take look at this PR? Thanks.

}
}
}

@RequiresPermission(USE_BIOMETRIC)
@Implementation
protected int canAuthenticate(int authenticators) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am seeing the error:

error: No such method in android.hardware.biometrics.BiometricManager for SDK 29
protected int canAuthenticate(int authenticators) {

Is this minSdk = R as well?
^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing. @charlesng Could you rebase and check this problem?

Copy link
Contributor Author

@charlesng charlesng Sep 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this method required minSdk = 30 as well, https://developer.android.com/reference/android/hardware/biometrics/BiometricManager#canAuthenticate(int)

I have updated it. Thanks for the review.

Btw, @hoisie could I ask how did you see the error? As I cannot see error before in both Android Studio and CI 馃檹

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done with the sdkCheckMode param:

https://github.com/robolectric/robolectric/blob/master/shadows/framework/build.gradle#L11

For some reason it's either not working in gradle or it's not causing a compile-time error.

@hoisie
Copy link
Contributor

hoisie commented Sep 24, 2022

I am also running tests internally at Google to check for compatibility issues

@charlesng charlesng force-pushed the feature/support-all-authentication-results-for-biometric-manager branch from 613e3ac to 2047986 Compare September 24, 2022 06:55
@hoisie hoisie merged commit 72cabf2 into robolectric:master Sep 24, 2022
@hoisie
Copy link
Contributor

hoisie commented Sep 24, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants