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

[PM-13387] Skip unneeded confirmation button when using passive biometrics such as face unlock #4064

Merged

Conversation

victor-marino
Copy link
Contributor

🎟️ Tracking

I opened this discussion explaining the change a few weeks ago:
https://github.com/orgs/bitwarden/discussions/11286

Got no response so far so I decided to go ahead and submit the PR. Hope that's ok!

📔 Objective

As explained in that discussion, by default the Biometrics API treats any authentication as if it were a high-risk operation (such as a purchase), and requires the user to press a "Confirm" button after successful authentication when using a passive biometric system (such as the secure face unlock present in newer Pixels).

But this doesn't really make sense for low-risk operations such as logging into an app, where you'd expect an iPhone-like authentication that seamlessly takes you to the app after success (I question whether it makes sense for the API to default to true here, but that's besides this PR).

This extremely simple PR just adds the .setConfirmationRequired(false) flag to the biometric prompt, which will greatly improve the user experience for people using devices with secure face unlock.

📸 Screenshots

The way it currently works, which requires pressing the "Confirm" button after successful face authentication:
image

The way it should work, showing the face unlock icon but requiring no user interaction to progress into the app activity:
image

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-13387

@bitwarden-bot bitwarden-bot changed the title Skip unneeded confirmation button when using passive biometrics such as face unlock [PM-13387] Skip unneeded confirmation button when using passive biometrics such as face unlock Oct 10, 2024
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details0ab74201-ac83-426c-8feb-447fd12a7a9c

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Privacy_Violation /app/src/main/java/com/x8bit/bitwarden/data/tools/generator/repository/utils/GeneratorRepositoryExtensions.kt: 11 Attack Vector
MEDIUM Unpinned Actions Full Length Commit SHA /crowdin-pull.yml: 27 Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
LOW Use_of_Hardcoded_Password /app/src/main/java/com/x8bit/bitwarden/data/autofill/util/HtmlInfoExtensions.kt: 20 Attack Vector

Fixed Issues

Severity Issue Source File / Package
MEDIUM Privacy_Violation /app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/completeregistration/CompleteRegistrationViewModel.kt: 349
MEDIUM Privacy_Violation /app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/completeregistration/CompleteRegistrationViewModel.kt: 349
MEDIUM Privacy_Violation /app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/completeregistration/CompleteRegistrationViewModel.kt: 241

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.87%. Comparing base (537281f) to head (e235801).
Report is 18 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4064   +/-   ##
=======================================
  Coverage   88.87%   88.87%           
=======================================
  Files         429      429           
  Lines       35447    35447           
  Branches     5221     5221           
=======================================
  Hits        31505    31505           
  Misses       2116     2116           
  Partials     1826     1826           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SaintPatrck SaintPatrck added this pull request to the merge queue Oct 15, 2024
Merged via the queue into bitwarden:main with commit 12afbea Oct 15, 2024
9 checks passed
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.

3 participants