[PM-27119] Filtering card types if user has an ITEM_RESTRICTED org policy on CXP import

[aj-rosado]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27119

📔 Objective

If the user has active ITEM_RESTRICTION policies. They should not be able to import card types.
This aims to filter the card types out of the import.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) 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]

Checkmarx One – Scan Summary & Details – 7d7559be-f41f-4637-bcd4-f6b2d764f2b2

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.82%. Comparing base (d86443c) to head (020aa85).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...ult/manager/CredentialExchangeImportManagerImpl.kt	78.94%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6104      +/-   ##
==========================================
- Coverage   84.82%   84.82%   -0.01%     
==========================================
  Files         721      721              
  Lines       52824    52842      +18     
  Branches     7667     7672       +5     
==========================================
+ Hits        44808    44822      +14     
  Misses       5328     5328              
- Partials     2688     2692       +4     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Claude finished @aj-rosado's task —— View job

---

Code Review Complete

Summary of Changes

This PR implements filtering of card-type items during CXP (Credential Exchange Protocol) import when users have active ITEM_RESTRICTED org policies. The changes span 3 files with 227 additions and 1 deletion:

• CredentialExchangeImportManagerImpl.kt: Added filtering logic to remove card items when policy is active
• VaultManagerModule.kt: Added PolicyManager dependency injection
• CredentialExchangeImportManagerTest.kt: Added comprehensive test coverage for the new functionality

---

Critical Issues

1. Incorrect Policy Type Check

Severity: ⚠️ High - Logic Bug

The implementation checks for PolicyTypeJson.RESTRICT_ITEM_TYPES but the Jira ticket and PR description mention "ITEM_RESTRICTION" policies. The policy type appears mismatched.

Location: CredentialExchangeImportManagerImpl.kt:157-159

val shouldFilterCreditCards = policyManager
    .getActivePolicies(type = PolicyTypeJson.RESTRICT_ITEM_TYPES)
    .any { it.isEnabled }

Question: Is RESTRICT_ITEM_TYPES the correct policy type? The ticket mentions "ITEM_RESTRICTION" - please verify this is the intended policy.

2. Unsafe Type Casting and Silent Failures

Severity: ⚠️ Medium - Robustness Issue

The isCardType function uses unsafe type casting with as? operators which silently returns false on any cast failure. This means malformed or unexpected JSON structures will be treated as "not a card" and could potentially allow restricted items through.

Location: CredentialExchangeImportManagerImpl.kt:173-180

private fun isCardType(item: Any): Boolean {
    val jsonObject = item as? JsonObject ?: return false
    val credentials = jsonObject.get("credentials") as? JsonArray ?: return false
    val firstCredential = credentials.firstOrNull() as? JsonObject ?: return false
    val type = firstCredential.get("type")?.jsonPrimitive?.content
    
    return type == "credit-card"
}

Issues:

• If item is not a JsonObject, it returns false (not filtered)
• If credentials is missing or not an array, returns false (not filtered)
• Only checks the first credential - what if there are multiple credentials in an item and the second one is a card?

Recommendation: Add logging or explicit error handling for unexpected structures. Consider checking all credentials, not just the first one.

3. Magic String "credit-card"

Severity: 📝 Low - Code Quality

The credential type "credit-card" is hardcoded as a magic string without documentation or constant definition.

Location: CredentialExchangeImportManagerImpl.kt:179

return type == "credit-card"

Recommendation: Define as a constant or reference existing constants from the CXF model if they exist.

---

Suggested Improvements

4. Missing KDoc Documentation

Severity: 📝 Medium - Style Violation

Per the project's STYLE_AND_BEST_PRACTICES.md, all public APIs require KDoc documentation. The new private helper functions should also be documented given their importance to security policy enforcement.

Location: CredentialExchangeImportManagerImpl.kt:154-181

Recommendation: Add KDoc to explain:

• filterRestrictedItems(): Purpose, parameters, return value, and what policy it enforces
• isCardType(): What credential types qualify as "cards"

Example:

/**
 * Filters out card-type items from accounts when the user has active RESTRICT_ITEM_TYPES policies.
 *
 * @param accounts The list of accounts to filter
 * @return A new list of accounts with card items removed if policy is active, otherwise the original list
 */
private fun filterRestrictedItems(...)

5. Incomplete Test Coverage

Severity: 💭 Medium - Test Gap

The test when user has restrict item types policy, importCxf should not contain any card item only tests one scenario. Missing test cases:

1. ✗ Multiple items where only some are cards (mixed scenario)
2. ✗ Items with multiple credentials where second/third is a card
3. ✗ Disabled policy (isEnabled = false) should NOT filter
4. ✗ Account with no items
5. ✗ Items with malformed credential structures
6. ✗ Empty credentials array

Location: CredentialExchangeImportManagerTest.kt:360-411

Recommendation: Add edge case tests to ensure robustness.

6. Potential Performance Consideration

Severity: 💭 Low - Optimization

The filtering logic iterates through all accounts and all items even when policy is inactive. The early return happens after the policy check, which is good, but the filterRestrictedItems function is called before knowing if accounts exist or are valid.

Location: CredentialExchangeImportManagerImpl.kt:91-97

val accountsJson = try {
    json.encodeToString(
        value = filterRestrictedItems(
            accounts = exportResponse.accounts,
        )
            .firstOrNull(),
    )

Consideration: This is likely negligible for typical account sizes, but worth noting that filtering happens even for .firstOrNull() which might discard the results.

7. Naming Inconsistency

Severity: 🎨 Low - Code Style

The variable shouldFilterCreditCards checks for RESTRICT_ITEM_TYPES policy but is named specifically for credit cards. This coupling makes it less flexible if other item types need filtering in the future.

Location: CredentialExchangeImportManagerImpl.kt:157

Recommendation: Consider hasItemRestrictionPolicy or shouldFilterRestrictedItems for better clarity and extensibility.

---

Good Practices Observed

✓ Proper DI Integration: PolicyManager correctly injected via Hilt
✓ Test Structure: Comprehensive test file with good mock setup
✓ Immutability: Uses account.copy() to create new instances rather than mutating
✓ Error Handling: Maintains existing Result-based error handling patterns
✓ Early Returns: Checks policy before performing expensive operations

---

Action Items for Author

1. [Critical] Verify and confirm PolicyTypeJson.RESTRICT_ITEM_TYPES is the correct policy type for this feature
2. [Important] Consider the security implications of silent failures in isCardType() - should malformed items be filtered (fail-safe) or allowed through?
3. [Important] Clarify behavior: Should filtering check ALL credentials in an item or just the first one?
4. [Recommended] Add KDoc documentation to the new helper functions
5. [Recommended] Add test cases for edge scenarios (mixed items, multiple credentials, disabled policy)
6. [Optional] Extract "credit-card" magic string to a constant

Code Coverage Details

Per Codecov report:

• Patch coverage: 78.94%
• 4 lines missing coverage (0 missing, 4 partials)
• These are likely the unsafe cast fallback paths in isCardType()

The test coverage could be improved by adding the edge case tests mentioned above.

---

Overall Assessment: The implementation follows the project's architecture patterns and integrates well with existing code. However, there are concerns about the robustness of the type-checking logic and potential security implications of silent failures. The main blocker is confirming the correct policy type being used.

---