[PM-27120] cxp hide user account when remove individual export is enabled #6089
Conversation
Added getUserPolicies to PolicyManager that will return all the policies associated with the given userId VaultSyncManager re ordered to ensure when UserState is updated policies are also updated SelectAccount verifying if account isExportable Added more info to VerifyPasswordNavigation
|
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6089 +/- ##
==========================================
+ Coverage 84.74% 84.82% +0.07%
==========================================
Files 737 721 -16
Lines 55439 52824 -2615
Branches 7655 7667 +12
==========================================
- Hits 46983 44808 -2175
+ Misses 5775 5328 -447
- Partials 2681 2688 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ated to isExportable behaviour
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
| userId = userId, | ||
| type = type, | ||
| policies = authDiskSource.getPolicies(userId = userId), | ||
| ) ?: emptyList() |
There was a problem hiding this comment.
Can we use orEmpty()?
Also, this should use chain formatting
override fun getUserPolicies(
userId: String,
type: PolicyTypeJson,
): List<SyncResponseJson.Policy> =
this
.filterPolicies(
userId = userId,
type = type,
policies = authDiskSource.getPolicies(userId = userId),
)
.orEmpty()
app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/util/UserStateJsonExtensions.kt
Show resolved
Hide resolved
| private fun existingPolicies( | ||
| userId: String, | ||
| policyType: PolicyTypeJson, | ||
| ): List<SyncResponseJson.Policy>? = policyManager.getUserPolicies( |
There was a problem hiding this comment.
Why is this return type nullable when getUserPolicies returns a non-null value?
There was a problem hiding this comment.
had a different implementation that was nullable, not necessary anymore. Removed
| userId = userId, | ||
| policies = syncResponse.policies.orEmpty(), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Is there a functional reason this was moved?
There was a problem hiding this comment.
Yes, the UserState was being updated before the policies were updated, so the existingPolicies wasn't returning the most recent data.
There was a problem hiding this comment.
Can you add a comment explaining that order matters here
| navController.navigateToVerifyPassword( | ||
| userId = currentState.userId, | ||
| navOptions = rootNavOptions, | ||
| hasOtherAccounts = false, |
There was a problem hiding this comment.
So this can never be true?
There was a problem hiding this comment.
this is on the RootNavState.CredentialExchangeExportSkipAccountSelection scenario and this is only called when only exists 1 valid account, so yes
| vaultLockManager = vaultLockManager, | ||
| dispatcherManager = dispatcherManager, | ||
| policyManager = policyManager, | ||
|
|
There was a problem hiding this comment.
Can we drop this blank line
app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/UserStateManagerTest.kt
Show resolved
Hide resolved
| isBiometricsEnabledProvider: (userId: String) -> Boolean, | ||
| vaultUnlockTypeProvider: (userId: String) -> VaultUnlockType, | ||
| isDeviceTrustedProvider: (userId: String) -> Boolean, | ||
| getUserPolicies: (userId: String, policy: PolicyTypeJson) -> List<SyncResponseJson.Policy>?, |
There was a problem hiding this comment.
This return type should not be nullable.
Added test for getUserPolicies Removed optional on getUserPolicies
| userId = USER_ID, | ||
| type = PolicyTypeJson.DISABLE_PERSONAL_VAULT_EXPORT, | ||
| ).any()) | ||
| } |
There was a problem hiding this comment.
Please format this
assertTrue(
policyManager
.getUserPolicies(
userId = USER_ID,
type = PolicyTypeJson.DISABLE_PERSONAL_VAULT_EXPORT,
)
.any()
)There was a problem hiding this comment.
Better yet, can we assert it gets the correct value back
assertEquals(
policyManager.getUserPolicies(
userId = USER_ID,
type = PolicyTypeJson.DISABLE_PERSONAL_VAULT_EXPORT,
),
// actual value
)| userId = USER_ID, | ||
| type = PolicyTypeJson.PERSONAL_OWNERSHIP, | ||
| ) | ||
| .isEmpty()) |
There was a problem hiding this comment.
Formatting, or assert the actual value:
assertEquals(
policyManager.getUserPolicies(
userId = USER_ID,
type = PolicyTypeJson.DISABLE_PERSONAL_VAULT_EXPORT,
),
emptyList<...>(),
)| isUsingKeyConnector = false, | ||
| onboardingStatus = OnboardingStatus.NOT_STARTED, | ||
| firstTimeState = FirstTimeState(showImportLoginsCard = true), | ||
| isExportable = false, |
| val actualItem = awaitItem() | ||
| assertEquals(userState, actualItem) | ||
|
|
||
| assertEquals(false, actualItem?.accounts[0]?.isExportable) |
There was a problem hiding this comment.
Does this line do anything?
If the isExportable value was true, wouldn't the assertion above have failed?
There was a problem hiding this comment.
it does because we are checking if the organizations id match the policy orgId, but now that I think about it, as we are returning the user policies, it seems to be a redundant check 🤔
There was a problem hiding this comment.
Updated the implementation, now it does not check with organization as it was redundant. Only verifies if the user has the policy.
Removed the isExportable assert
| userId, | ||
| PolicyTypeJson.PERSONAL_OWNERSHIP, | ||
| ) | ||
| .orEmpty() |
There was a problem hiding this comment.
You shouldn't need this orEmpty() anymore.
Reworked some UserStateManager and PolicyManager tests
| val hasPersonalOwnershipRestrictedOrg = getUserPolicies( | ||
| userId, | ||
| PolicyTypeJson.PERSONAL_OWNERSHIP, | ||
| ).isNotEmpty() |
There was a problem hiding this comment.
Can we fix this formatting
val hasPersonalOwnershipRestrictedOrg = getUserPolicies(
userId,
PolicyTypeJson.PERSONAL_OWNERSHIP,
)
.isNotEmpty()| val hasPersonalVaultExportRestrictedOrg = getUserPolicies( | ||
| userId, | ||
| PolicyTypeJson.DISABLE_PERSONAL_VAULT_EXPORT, | ||
| ).isNotEmpty() |
Added test to verify if policy isExportable respects if policy is enabled
aj-rosado
deleted the
PM-27120/cxp-hide-user-account-remove-individual-export
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27120
📔 Objective
Hide the accounts that contain an organization that does not allow Individual vault export on the Account Selector.
In order to achieve that and to ensure skipping the selection screen, when taking into consideration accounts that cannot export, some changes were needed to make
Saving account exportable info into UserState (in order to RootNav know which screen to navigate to)
Added getUserPolicies to PolicyManager that will return all the policies associated with the given userId (in order to get data for the iven userId and not only the current active)
VaultSyncManager re ordered to ensure when UserState is updated policies are also updated
SelectAccount verifying if account isExportable
Added more info to VerifyPasswordNavigation in order to properly know when to display the back button.
📸 Screenshots
Two accounts scenario
Screen.Recording.2025-10-27.at.12.48.35.mov
More than two accounts
Screen.Recording.2025-10-27.at.12.54.03.mov
⏰ 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