-
Notifications
You must be signed in to change notification settings - Fork 866
[PM-19107] Introduce user-trusted privileged apps for Credential Manager #4848
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4848 +/- ##
==========================================
+ Coverage 82.82% 82.84% +0.02%
==========================================
Files 678 693 +15
Lines 50918 51277 +359
Branches 6927 6943 +16
==========================================
+ Hits 42171 42481 +310
- Misses 6428 6471 +43
- Partials 2319 2325 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4c2645a
to
d587122
Compare
New Issues (5)Checkmarx found the following issues in this Pull Request
|
ac3ad3e
to
465c5ce
Compare
465c5ce
to
9beb3ec
Compare
9beb3ec
to
8ce9b2d
Compare
8ce9b2d
to
f5ad016
Compare
...est/kotlin/com/x8bit/bitwarden/data/credentials/model/Fido2CredentialAssertionRequestUtil.kt
Dismissed
Show dismissed
Hide dismissed
This change introduces the ability for users to trust specific privileged apps (browsers) for Credential Manager operations. When an operation originates from an untrusted browser, the user will be prompted to either trust the browser or cancel the operation. Key changes include: - Added a `PrivilegedAppDatabase` to store user-trusted apps. - Implemented `PrivilegedAppRepository` and related classes for managing trusted apps. - Updated `OriginManagerImpl` to check the user's trusted list in addition to Google and community allow lists. - Modified `VaultItemListingViewModel` to handle the trust prompt and subsequent actions. - Added new UI elements (`TrustPrivilegedAddPrompt` dialog) and string resources for the trust flow. - Updated `CallingAppInfoExtensions` to correctly validate against allow lists.
f5ad016
to
51c8a2e
Compare
* Removes a trusted privileged app. | ||
*/ | ||
@Suppress("MaxLineLength") | ||
@Query("DELETE FROM privileged_apps WHERE package_name = :packageName AND signature = :signature") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the suppression, you can also do this:
@Query(
"DELETE FROM privileged_apps WHERE package_name = :packageName AND signature = :signature"
)
```
@Database( | ||
entities = [PrivilegedAppEntity::class], | ||
version = 1, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be exporting the schema here
@Singleton | ||
fun providePrivilegedAppDiskSource( | ||
privilegedAppDao: PrivilegedAppDao, | ||
@UnencryptedPreferences sharedPreferences: SharedPreferences, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO we need these shared preferences here?
@@ -85,12 +76,17 @@ class OriginManagerImpl( | |||
|
|||
private suspend fun validatePrivilegedAppSignatureWithCommunityList( | |||
callingAppInfo: CallingAppInfo, | |||
): ValidateOriginResult = | |||
validatePrivilegedAppSignatureWithAllowList( | |||
): ValidateOriginResult = validatePrivilegedAppSignatureWithAllowList( | |||
callingAppInfo = callingAppInfo, | |||
fileName = COMMUNITY_ALLOW_LIST_FILE_NAME, | |||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is off here, wanna check the formatter
override suspend fun removeTrustedPrivilegedApp( | ||
packageName: String, | ||
signature: String, | ||
) = privilegedAppDiskSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some return type for these public methods
@@ -23,7 +23,7 @@ fun CallingAppInfo.getSignatureFingerprintAsHexString(): String? { | |||
*/ | |||
fun CallingAppInfo.validatePrivilegedApp(allowList: String): ValidateOriginResult { | |||
|
|||
if (!allowList.contains("\"package_name\": \"$packageName\"")) { | |||
if (!allowList.contains("\"$packageName\"")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this needed?
@@ -352,7 +362,7 @@ private fun VaultItemListingDialogs( | |||
is VaultItemListingState.DialogState.CredentialManagerOperationFail -> BitwardenBasicDialog( | |||
title = dialogState.title(), | |||
message = dialogState.message(), | |||
onDismissRequest = { onDismissFido2ErrorDialog(dialogState.message) }, | |||
onDismissRequest = { onDismissCredentialManagerErrorDialog(dialogState.message) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we send message back here?
onDismissRequest = { | ||
onDismissCredentialManagerErrorDialog( | ||
R.string.passkey_operation_failed_because_the_browser_is_not_trusted | ||
.asText(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a test?
🎟️ Tracking
PM-19107
📔 Objective
This change introduces the ability for users to trust specific privileged apps (browsers) for Credential Manager operations. When an operation originates from an untrusted browser, the user will be prompted to either trust the browser or cancel the operation.
Key changes include:
PrivilegedAppDatabase
to store user-trusted apps.PrivilegedAppRepository
and related classes for managingtrusted apps.
OriginManagerImpl
to check the user's trusted list inaddition to Google and community allow lists.
VaultItemListingViewModel
to handle the trust prompt andsubsequent actions.
TrustPrivilegedAddPrompt
dialog) and stringresources for the trust flow.
CallingAppInfoExtensions
to correctly validate against allowlists.
📸 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 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