[PM-27119] Prevent import card data when ITEM_RESTRICT_TYPES policy is active #6123
Conversation
…ITEM_TYPE policy is active
|
Great job! No new security vulnerabilities introduced in this pull request |
aj-rosado
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6123 +/- ##
==========================================
+ Coverage 84.82% 84.96% +0.13%
==========================================
Files 721 737 +16
Lines 52824 53069 +245
Branches 7667 7664 -3
==========================================
+ Hits 44808 45090 +282
+ Misses 5328 5296 -32
+ Partials 2688 2683 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-restricted-policy
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6123 +/- ##
==========================================
+ Coverage 84.96% 84.99% +0.02%
==========================================
Files 724 723 -1
Lines 52758 52747 -11
Branches 7659 7652 -7
==========================================
+ Hits 44826 44832 +6
+ Misses 5249 5242 -7
+ Partials 2683 2673 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @aj-rosado's task —— View job Code Review CompleteSummaryImplementation successfully prevents card imports when ITEM_RESTRICT_TYPES policy is active. Defense-in-depth approach (filtering at ViewModel and Manager layers) is sound. Test coverage is comprehensive. FindingsFinding 1: Unnecessary suppression annotation remains in test file DetailsThe PR implements a two-layer filtering approach:
This defense-in-depth pattern is appropriate given that credential providers may return cards regardless of request parameters. Helper method Test coverage is thorough with 4 new test cases covering all scenarios including edge case where all ciphers are cards. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
...rc/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/importitems/ImportItemsViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/importitems/ImportItemsViewModel.kt
Show resolved
Hide resolved
...rc/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerImpl.kt
Show resolved
Hide resolved
...rc/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerTest.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/di/VaultManagerModule.kt
Show resolved
Hide resolved
…s policy is active
app/src/main/kotlin/com/x8bit/bitwarden/data/platform/manager/util/PolicyManagerExtensions.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/importitems/ImportItemsViewModel.kt
Show resolved
Hide resolved
...rc/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerImpl.kt
Show resolved
Hide resolved
...rc/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerImpl.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/di/VaultManagerModule.kt
Show resolved
Hide resolved
...rc/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CredentialExchangeImportManagerTest.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/platform/manager/util/PolicyManagerExtensions.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/importitems/ImportItemsViewModel.kt
Show resolved
Hide resolved
| if (cipherList.isEmpty()) { | ||
| // Filter out card ciphers if RESTRICT_ITEM_TYPES policy is active | ||
| val filteredCipherList = if (policyManager.hasRestrictItemTypes()) { | ||
| cipherList.filter { cipher -> cipher.type != CipherType.CARD } |
There was a problem hiding this comment.
Could the restriction be for a different type?
There was a problem hiding this comment.
Right now we only have restrictions for cards and I do not think we have future work to change that.
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
I don't think we need this suppression.
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
Don't need the suppression
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
Don't need the suppression
| assertTrue(result is ImportCxfPayloadResult.Error) | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
We don't need this suppression
| } | ||
| } | ||
|
|
||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
The @Suppress("MaxLineLength") annotation is not needed for this test function. The function name length (97 characters) does not violate the max line length rule, which typically applies to code lines, not function names in tests using backticks.
Per david-livefront's review comment, this suppression should be removed.
aj-rosado
deleted the
PM-27119/cxp-prevent-card-item-imported-item-restricted-policy
branch
🎟️ Tracking
📔 Objective
If the user has active ITEM_RESTRICTION policies.
Will not request card types from other password managers
If the data still contains card data, this will filter the card types out of the import if the importing data
⏰ 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