fix: biometric prompt is not shown#10031
Conversation
rafaeltonholo
left a comment
There was a problem hiding this comment.
Thanks for this PR. There are a few changes I believe we should do before merging this
...c/main/kotlin/app/k9mail/feature/account/server/settings/ui/common/ProtectedPasswordInput.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/net/thunderbird/feature/account/server/settings/ui/common/BiometricAuthenticator.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/net/thunderbird/feature/account/server/settings/ui/common/BiometricAuthenticator.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private fun Activity.setSecure(secure: Boolean) { | ||
| window.setFlags(if (secure) WindowManager.LayoutParams.FLAG_SECURE else 0, WindowManager.LayoutParams.FLAG_SECURE) |
There was a problem hiding this comment.
suggestion: Although this is nice to have, I don't think that this input should be responsible for setting the Activity in the secure mode.
Instead, I would have a onPasswordVisibleChange callback, and the screen that consumes this component would change the Activity flag.
Additionally, removing the FLAG_SECURE flag when disposing of this component opens the possibility of retrieving the password via a screenshot.
See video below:
Screen.Recording.2025-11-03.at.3.15.13.PM.mov
|
I agree that the implementation is not great and it doesn't work reliably. It basically still follows the original request to keep "existing feature that only reveals a saved password when the user has authenticated" from: #7202 This just patched the implementation to a state that it works, but it's not meant to be watertight. |
The comments I provided, aside from the one concerning FLAG_SECURE, are not meant to make the implementation watertight but rather to mitigate potential future issues (localization and performance). Furthermore, after reviewing the comment on #7202, I feel that the current implementation does not fulfill its intended purpose:
As demonstrated in the screen recording, I was able to take a screenshot that revealed the unmasked password. However, the FLAG_SECURE issue could be addressed in another task, as it was a suggestion comment. |
|
I think there might be a misunderstanding — I never meant that localization or performance shouldn’t be addressed (why would I?). I completely agree that both are important considerations, and I assumed it was obvious that these would be fixed, even if I didn’t say so explicitly. But if it helps, I can make that clearer next time — for example, by explicitly commenting or adding a 👍 to acknowledge. My point was simply that the previous implementation didn’t actually achieve its intended purpose, so the priority was to get it working first without rearchitecting the entire functionality. The only change beyond that was improving testability, since the original logic wasn’t functioning as intended. Once the feature works reliably, we can refine the FLAG_SECURE handling and other improvements in a follow-up. |
I'm sorry, I'm a bit slow today, and I might have misunderstood your first comment! Thanks for making it clear! I agree with you |
a40242e to
5a2c390
Compare
Resolves #10013
This refactors the password input components to use a flexible authentication interface, replacing the previous hardcoded biometric authentication logic. The changes introduce a new
Authenticatorabstraction, allowing for easier extension and testing. Updated the UI components and added tests accordingly. The authenticated reveal logic is now tested and working.