PM-9532: pt2. separate vault unlock logic and fail out on error during login. #3609

dseverns-livefront · 2024-07-23T14:22:07Z

🎟️ Tracking

📔 Objective

If a user is able to authenticate successfully with the server but there is an issue in the SDK when unlocking the vault (or related SDK operation like initializing crypto) they will be in a sort of limbo state where they will immediately be take to the unlock screen but unable to proceed. To prevent this and match iOS behavior we want to propagate any error from the SDK that occurs in the vault unlock process to the LoginResult and prevent a "successful" login, if they are unable to access the vault due to the issue stated above.

NOTE

The repro step which originally highlighted this issue has been fixed in the SDK according to the info on the ticket. To recreate you can force the result of VaultSdkSourceImpl.initializeCrypto to return a failure and see the difference on main vs this branch. Also testing to make sure no regression is added in the separation of some of the vault unlock logic from the storage of keys.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 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

github-actions · 2024-07-23T14:27:53Z

Checkmarx One – Scan Summary & Details – df21295c-b439-4523-a984-44e9dd289795

No New Or Fixed Issues Found

codecov · 2024-07-23T14:49:39Z

Codecov Report

Attention: Patch coverage is 88.77551% with 11 lines in your changes missing coverage. Please review.

Project coverage is 87.83%. Comparing base (bbe50ae) to head (7e72776).
Report is 17 commits behind head on main.

Files	Patch %	Lines
...twarden/data/auth/repository/AuthRepositoryImpl.kt	87.91%	1 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3609      +/-   ##
==========================================
+ Coverage   87.81%   87.83%   +0.02%     
==========================================
  Files         368      368              
  Lines       30469    30495      +26     
  Branches     4552     4561       +9     
==========================================
+ Hits        26755    26785      +30     
+ Misses       2128     2119       -9     
- Partials     1586     1591       +5

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

app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

SaintPatrck · 2024-07-26T13:36:33Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+            ssoToken = loginResponse.ssoToken,
+        )
+
+        // If this error was received, it also means any cached two-factor token is invalid.


SaintPatrck · 2024-07-26T13:37:57Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+                                    authRequestKey = loginResponse.key,
+                                )
+                            }
+                            ?: AuthRequestMethod.UserKey(protectedUserKey = model.asymmetricalKey),


Can we get a test for this?

app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt

david-livefront · 2024-08-05T15:04:56Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

        val userId = userStateJson.activeUserId
        val deviceKey = authDiskSource.getDeviceKey(userId = userId)
        if (deviceKey == null) {
            // A null device key means this device is not trusted.
-            val pendingRequest = authDiskSource.getPendingAuthRequest(userId = userId) ?: return
+            val pendingRequest =
+                authDiskSource.getPendingAuthRequest(userId = userId) ?: return null


Can we chain this:

val pendingRequest = authDiskSource .getPendingAuthRequest(userId = userId) ?: return null

david-livefront · 2024-08-05T15:30:20Z

app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/trusteddevice/TrustedDeviceScreen.kt

-            onNotYouButtonClick = {},
-        ),
-    )
+    BitwardenTheme {


david-livefront · 2024-08-05T15:35:03Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+        ) {
+            // if possible attempt to unlock the vault with trusted device data
+            if ((deviceData != null) ||
+                (loginResponse.userDecryptionOptions?.trustedDeviceUserDecryptionOptions != null)


Can we drop the extra parenthesis?

david-livefront · 2024-08-05T15:41:15Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+    ): VaultUnlockResult? {
+        // Attempt to unlock the vault with password if possible.
+        return password?.let {
+            if (loginResponse.privateKey != null && loginResponse.key != null) {


Could we replace this with some early returns:

): VaultUnlockResult? { // Attempt to unlock the vault with password if possible. val masterPassword = password ?: return null val privateKey = loginResponse.privateKey ?: return null val key = loginResponse.key ?: return null return vaultRepository.unlockVault( userId = userId, email = userStateJson.activeAccount.profile.email, kdf = userStateJson.activeAccount.profile.toSdkParams(), userKey = key, privateKey = privateKey, masterPassword = masterPassword, // We can separately unlock vault for organization data after // receiving the sync response if this data is currently absent. organizationKeys = null, ) }

david-livefront · 2024-08-05T15:43:21Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+        deviceData: DeviceDataModel?,
+    ): VaultUnlockResult? {
+
+        var vaultUnlockResult: VaultUnlockResult? = null


Do we need this?

Could we just do this:

): VaultUnlockResult? { // Attempt to unlock the vault with auth request if possible. // These values will only be null during the Just-in-Time provisioning flow. if (loginResponse.privateKey != null && loginResponse.key != null) { deviceData?.let { model -> return vaultRepository.unlockVault( userId = userId,

david-livefront · 2024-08-05T15:44:10Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+            }
+        }
+
+        if (vaultUnlockResult == null) {


If we return early above, we know that we should run the code inside this if if we make it this far.

So this should work, right?

// Handle the Trusted Device Encryption flow return loginResponse .userDecryptionOptions ?.trustedDeviceUserDecryptionOptions ?.let { options -> loginResponse.privateKey?.let { privateKey -> unlockVaultWithTrustedDeviceUserDecryptionOptionsAndStoreKeys( options = options, userStateJson = userStateJson, privateKey = privateKey, ) } }

david-livefront · 2024-08-05T15:51:20Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+        val result = block.invoke()
+        (result as? VaultUnlockError)?.let {
+            onVaultUnlockError.invoke(it)
+        }


Can this be simplified?

(block() as? VaultUnlockError)?.also(onVaultUnlockError)

david-livefront · 2024-08-06T16:12:52Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+        // Attempt to unlock the vault with password if possible.
+        val masterPassword = password ?: return null
+        val privateKey = loginResponse.privateKey ?: return null
+        val key = loginResponse.key ?: return null


david-livefront · 2024-08-06T16:13:26Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+
+        // Handle the Trusted Device Encryption flow
+        return loginResponse
+                .userDecryptionOptions


Is this formatted correctly?

I feels over indented here

weird I definitely cmd+shift+l on this several times, but yeah this a big ol \t\t

david-livefront · 2024-08-06T17:23:50Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

+        password: String?,
+    ): VaultUnlockResult? {
+        // Attempt to unlock the vault with password if possible.
+        // Attempt to unlock the vault with password if possible.


Double comment

david-livefront · 2024-08-06T17:24:56Z

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt

    /**
     * A helper method to handle the [TrustedDeviceUserDecryptionOptionsJson] specific to TDE.
+     * also store the necessary keys when appropriate.


Can we get the correct punctuation here:

* A helper method to handle the [TrustedDeviceUserDecryptionOptionsJson] specific to TDE * and store the necessary keys when appropriate.

dseverns-livefront requested review from SaintPatrck, shannon-livefront, david-livefront and brian-livefront July 23, 2024 14:22

dseverns-livefront changed the title ~~PM-9532/pt2-separate-vault-unlock-for-result-on-login~~ PM-9532: pt2. separate vault unlock logic and fail out on error during login. Jul 23, 2024

david-livefront reviewed Jul 24, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt Outdated Show resolved Hide resolved

david-livefront reviewed Jul 24, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt Outdated Show resolved Hide resolved

david-livefront reviewed Jul 24, 2024

View reviewed changes

app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt Outdated Show resolved Hide resolved

dseverns-livefront requested a review from david-livefront July 25, 2024 13:22

dseverns-livefront force-pushed the PM-9532/vault-error-not-propagated-on-login branch 2 times, most recently from b5ca08e to 1e31092 Compare July 25, 2024 21:22

SaintPatrck reviewed Jul 26, 2024

View reviewed changes

dseverns-livefront added 6 commits August 5, 2024 08:44

PM-9532/pt2-separate-vault-unlock-for-result-on-login

d9ad02f

detekt

46529a2

named args

4c66e1d

after rebase

b3213d4

feedback

e1bf70e

feedback

d8f368c

dseverns-livefront force-pushed the PM-9532/vault-error-not-propagated-on-login branch from 1e31092 to d8f368c Compare August 5, 2024 12:44

dseverns-livefront requested a review from SaintPatrck August 5, 2024 12:45

david-livefront reviewed Aug 5, 2024

View reviewed changes

style

5914a6d

dseverns-livefront requested a review from david-livefront August 5, 2024 20:18

david-livefront reviewed Aug 6, 2024

View reviewed changes

cleanup cleanup

7e72776

dseverns-livefront requested a review from david-livefront August 6, 2024 17:39

david-livefront approved these changes Aug 6, 2024

View reviewed changes

dseverns-livefront merged commit 18cd66a into main Aug 6, 2024
9 checks passed

dseverns-livefront deleted the PM-9532/vault-error-not-propagated-on-login branch August 6, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PM-9532: pt2. separate vault unlock logic and fail out on error during login. #3609

PM-9532: pt2. separate vault unlock logic and fail out on error during login. #3609

dseverns-livefront commented Jul 23, 2024 •

edited by jira bot

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

SaintPatrck Jul 26, 2024

SaintPatrck Jul 26, 2024

dseverns-livefront Aug 5, 2024

david-livefront Aug 5, 2024

david-livefront Aug 5, 2024

david-livefront Aug 5, 2024 •

edited

Loading

david-livefront Aug 5, 2024

david-livefront Aug 5, 2024 •

edited

Loading

david-livefront Aug 5, 2024 •

edited

Loading

dseverns-livefront Aug 5, 2024

david-livefront Aug 5, 2024

dseverns-livefront Aug 5, 2024

david-livefront Aug 6, 2024

david-livefront Aug 6, 2024

dseverns-livefront Aug 6, 2024

david-livefront Aug 6, 2024

david-livefront Aug 6, 2024

PM-9532: pt2. separate vault unlock logic and fail out on error during login. #3609

PM-9532: pt2. separate vault unlock logic and fail out on error during login. #3609

Conversation

dseverns-livefront commented Jul 23, 2024 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

NOTE

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Jul 23, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-livefront Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-livefront Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

david-livefront Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseverns-livefront commented Jul 23, 2024 •

edited by jira bot

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

david-livefront Aug 5, 2024 •

edited

Loading

david-livefront Aug 5, 2024 •

edited

Loading

david-livefront Aug 5, 2024 •

edited

Loading