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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[local_auth] Allow empty localizedReason for Android #8115

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Luciferx86
Copy link

@Luciferx86 Luciferx86 commented Nov 17, 2024

localizedReason should be optional for Android. There could be use case where we only want to show a single piece of text on the biometric prompt. Currently showing 2 pieces of texts is mandatory.

Proposal

Only set the description parameter to the BiometricPrompt if a non empty String is passed in the call.

code:

AuthenticationHelper.java

use

.setDescription(strings.getReason().isEmpty() ? null : strings.getReason())

instead of

.setDescription(strings.getReason())

** screenshots attached in the original issue

Related: flutter/flutter#116885

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

This PR makes the Android implementation inconsistent with the public documentation of authenticate, which says (at both the app-facing and platform interface level) that the parameter must not be empty. That would mean that only people who read the implementation source would know that they could do the opposite of what the docs say, which is not how we would want to land this.

A complete fix needs to update the docs on those methods, as well as provide clients a runtime way of knowing whether or not the current platform requires a non-empty reason, via a support query. So this needs a new platform interface method, called something like requiresLocalizedReason(), and for that to be wired up in the app-facing package as part of changing the documention for authenticate.

@Luciferx86
Copy link
Author

I understand that there needs to be a method like requiresLocalizedReason() for all the platforms, and for android in particular, this method will return false.

So to make this work, I would be required to update the documentation for authenticate method, as well as override this method in all platform implementations?

Also implementation for authenticate then becomes something like?

if(requiresLocalizedReason()){
assert(localizedReason.isNotEmpty);
}

Is this implementation correct, or you had something else in mind? @stuartmorgan

@stuartmorgan
Copy link
Contributor

as well as override this method in all platform implementations?

Since there needs to be a default implementation that returns true for backwards compatibility, it only actually needs to be overridden in the Android implementation.

Also implementation for authenticate then becomes something like?

if(requiresLocalizedReason()){
assert(localizedReason.isNotEmpty);
}

The assertions are currently at the platform implementation package level; every platform implementation already knows whether it needs the assertion or not, without a new conditional.

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

Successfully merging this pull request may close these issues.

2 participants