[PM-11886] Update handling of unprivileged apps and improve error messaging #4694
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Fixed Issues (7)Great job! The following issues were fixed in this Pull Request
|
SaintPatrck
force-pushed
the
PM-11886/update-origin-for-unprivileged-apps
branch
from
5ddfd15 to
dd31e4f
Compare
1 task
|
Ahh, no luck - it looks like there isn't a build for that. @SaintPatrck are you able to run a build, so I can install the build artifact? I don't think I have the ability to set up a build environment on my own. https://github.com/bitwarden/android/actions/runs/13147380497/job/36688357537?pr=4694 failed. |
SaintPatrck
force-pushed
the
PM-11886/update-origin-for-unprivileged-apps
branch
from
84abd71 to
962de84
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4694 +/- ##
==========================================
+ Coverage 89.40% 89.42% +0.01%
==========================================
Files 490 490
Lines 40980 41020 +40
Branches 5836 5823 -13
==========================================
+ Hits 36638 36681 +43
+ Misses 2321 2318 -3
Partials 2021 2021 ☔ View full report in Codecov by Sentry. |
|
Thanks! Watching the build now. |
|
@BJReplay builds are available: https://github.com/bitwarden/android/actions/runs/13149534226#artifacts |
Thanks for the heads up. Will install and test now. |
SaintPatrck
force-pushed
the
PM-11886/update-origin-for-unprivileged-apps
branch
from
24e174e to
89e7e99
Compare
This commit introduces the ability to copy Passkey logs for debugging purposes. Key changes: - Added `writeToFile` and `readFromFile` functions to `FileManager` to handle file writing and reading operations. - Implemented `getPasskeyLogs` function in `LogsManager` to fetch the logs. - Introduced a PasskeyTree class in `LogsManagerImpl` to write Passkey related logs to a designated log file. - Added a "Copy passkey logs" option in the About screen to allow users to copy the logs to the clipboard. - Updated `AboutViewModel` to handle the copy passkey logs action.
SaintPatrck
force-pushed
the
PM-11886/update-origin-for-unprivileged-apps
branch
from
89e7e99 to
d9601b3
Compare
|
@BJReplay I've added logs and a couple ways to capture the ones that would help nail this down.
The updated builds will be available here: https://github.com/bitwarden/android/actions/runs/13167643578 Feel free to paste those logs here or package them in a Send. No data is logged. Only call path indicators so I can track where the generic errors are spawning. Thanks again for all of the assistance with hunting this down. I really appreciate you taking the time. 🫶 |
@SaintPatrck do I need a specific version (beta? dev?)? I'm trying beta because that's how I'm running native alongside legacy, but with a test run against webauthn.io it doesn't appear to be capturing anything. The copied log from the About screen simply says Even running without the filterspec doesn't seem to log any lines: |
|
@BJReplay Ah, I forgot to mention logcat output is only enabled on the dev builds. The copy button is available on all of the variants. Also, logs are only emitted when failures occur. If everything works as expected on our end the file will be empty or not exist. |
Aha, that's why nothing was captured with my first test to test that I was capturing - it was a successful test! Back to it... |
|
Ok, no luck, @SaintPatrck I could get © Bitwarden Inc. 2015-2025 Version: 2025.2.0 (19778) to respond to passkey login attempts on:
However, when I started Ubank, it would only list the passkey stored in Bitwarden legacy and Keyguard. It would not list Bitwarden native. I set the passkey settings in the OS to only have Bitwarden native. At this point I could still log in to
However, when I attempt to log into Ubank, I get a message (from Ubank):
It looks like these changes should not prevent the passkey from being offered, but it looks like, for some reason, this is a regression to an earlier version of this bug where Bitwarden native simply won't offer the passkey. All test so far with beta - documenting now, and will soon try dev, just in case. |
|
@SaintPatrck got there in the end. Bizarrely, once I'd installed Dev, and set it up as preferred, Beta suddenly showed up for Ubank (but Dev did not). Logs were pretty simple - hope this helps - let me know if you want me to try to get Dev working for a logcat capture. |
|
@BJReplay Nice! We're getting here, but we're not able to parse the I'm going to add another log to capture the full request. Nothing in this request is compromising, but feel free to scrub the challenge and userId before posting if it makes you more comfortable. The most important thing we need is |
|
Testable builds will be available here: https://github.com/bitwarden/android/actions/runs/13187375689 |
|
Will do - may not be until late today - have a lot on today. |
|
Currently working remotely, so no USB cable, and no adb logcat possible. One of the issues I have testing (on a S22U) is that it seems really hard to get Passkey and Preferred Passkey settings to stick, and nominated apps to present passkeys on a login prompt. So, this login attempt I managed to get KeyGuard to present, and... This is the error message that popped up on KeyGuard
I don't know if that's because I've invalidated the passkey at UBank with all my cancelled login attempts (possible - I won't know until I successfully log in), but the error message felt like it was worth sharing as a hint. Will continue to test as possible. |
|
OK, @SaintPatrck finally got there after a couple of reboots and countless changing of auto-fill and passkey settings. Logs below. First logs are yesterday. I added a blank line to separate them. Second logs are today. Note that there are three different public keys, each repeated in two places. I've redacted all information that might be sensitive. The redactions are JSON extracted: {"challenge":"<Challenge Present but REDACTED>","timeout":120000,"rpId":"www.ubank.com.au","allowCredentials":[{"type":"public-key","id":"<Public Key 1 Present but REDACTED>"},{"type":"public-key","id":"<Public Key 2 Present but REDACTED>"},{"type":"public-key","id":"<Public Key 3 Present but REDACTED>"}],"userVerification":"required"}, androidx.credentials.BUNDLE_KEY_CLIENT_DATA_HASH=null, androidx.credentials.BUNDLE_KEY_TYPE_PRIORITY_VALUE=100, android.service.credentials.BeginGetCredentialOption.BUNDLE_ID_KEY=<Bundle ID Present but REDACTED>, androidx.credentials.BUNDLE_KEY_SUBTYPE=androidx.credentials.BUNDLE_VALUE_SUBTYPE_GET_PUBLIC_KEY_CREDENTIAL_OPTION, androidx.credentials.BUNDLE_KEY_IS_AUTO_SELECT_ALLOWED=true}], id=<Bundle ID Present but REDACTED>, userId=<User ID Present but REDACTED>, requestJson={"challenge":"<Challenge Present but REDACTED>","timeout":120000,"rpId":"www.ubank.com.au","allowCredentials":[{"type":"public-key","id":"<Public Key 1 Present but REDACTED>"},{"type":"public-key","id":"<Public Key 2 Present but REDACTED>"},{"type":"public-key","id":"<Public Key 3 Present but REDACTED>"}],"userVerification":"required"} |
- In `Fido2ProviderProcessorImpl.kt`, log the received create credential request and get credential request, including the calling app's package name and the request data. - In `Fido2IntentUtils.kt`, log the created `Fido2CreateCredentialRequest`, `Fido2CredentialAssertionRequest`, and `Fido2GetCredentialsRequest`.
SaintPatrck
force-pushed
the
PM-11886/update-origin-for-unprivileged-apps
branch
from
7d2de8f to
95e1bfd
Compare
I tested the build before this comment - I have been on a call, and had downloaded them prior to the call, but only just installed. I haven't yet installed the latest build. From that earlier build (dev). Note one successful warm up to make sure that dev was responding to passkeys at passkeys.io is also logged. |
|
More logs - from latest build. dev.apk |
|
@BJReplay give this build a shot when you can: https://github.com/bitwarden/android/actions/runs/13276476757 |
Will do, probably a couple of hours from now. Will report back. |
|
@SaintPatrck tried that build (dev) and it logged in! You have a fix. Send it! |
…n-for-unprivileged-apps # Conflicts: # app/src/main/res/values/strings.xml
SaintPatrck
requested review from
brian-livefront,
david-livefront,
dseverns-livefront,
ahaisting-livefront,
phil-livefront and
a team
as code owners
app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/manager/Fido2CredentialManagerImpl.kt
Show resolved
Hide resolved
...src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt
Outdated
Show resolved
Hide resolved
phil-livefront
approved these changes
Feb 12, 2025
|
|
| onFailure = { Fido2RegisterCredentialResult.Error }, | ||
| onFailure = { | ||
| Fido2RegisterCredentialResult.Error( | ||
| R.string.passkey_registration_failed_due_to_an_internal_error.asText(), |
There was a problem hiding this comment.
You should not have a Text inside the data package, that is a ui only class
| @@ -13,5 +15,5 @@ sealed class Fido2CredentialAssertionResult { | |||
| /** | |||
| * Indicates there was an error and the assertion was not successful. | |||
| */ | |||
| data object Error : Fido2CredentialAssertionResult() | |||
| data class Error(val message: Text) : Fido2CredentialAssertionResult() | |||
There was a problem hiding this comment.
You can't have this in the data package
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-11886
Resolves #4239
📔 Objective
Improve passkey processing with unprivileged applications and provide more descriptive error messages when failures occur.
📸 Screenshots
Coming soon!
⏰ Reminders before review
🦮 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 confirmedissue 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