[PM-38176] feat: Expose policies client SDK for policy evaluation#2727
[PM-38176] feat: Expose policies client SDK for policy evaluation#2727fedemkr wants to merge 7 commits into
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR delegates policy filtering to the Bitwarden SDK via Code Review DetailsNo new findings. Prior critical/suggested/question feedback on threads has been addressed in subsequent commits or answered by the author. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return try await sdkFilterPolicies( | ||
| organizations: organizations, | ||
| policyType: policyType, | ||
| userId: userId, | ||
| ) |
There was a problem hiding this comment.
❌ CRITICAL: SDK path drops the caller's filter, producing incorrect results for filtered callers.
Details and fix
policiesApplyingToUser receives a filter: ((Policy) -> Bool)? argument (line 191) and forwards it to the native path via policiesForUser, but the SDK branch calls sdkFilterPolicies(organizations:policyType:userId:) without passing filter. Inside sdkFilterPolicies the parameter defaults to nil, so the caller's filter is silently dropped.
This changes behavior when policiesInAcceptedState is enabled. Two real callers are affected:
isSendHideEmailDisabledByPolicy()(line 495) passes{ policy[.disableHideEmail]?.boolValue == true }. In the SDK path the filter is lost, so the method returnstruefor any active.sendOptionspolicy on the user — even whendisableHideEmailisfalseor absent. The existing testtest_isSendHideEmailDisabledByPolicy_optionDisabledonly covers the native path and would not catch this regression.getMasterPasswordPolicyOptions()(line 414) passes{ $0.data != nil }. In the SDK path, policies withdata == nilare no longer pre-filtered, which (while less severe) is a behavior divergence from the legacy path.
Fix:
| return try await sdkFilterPolicies( | |
| organizations: organizations, | |
| policyType: policyType, | |
| userId: userId, | |
| ) | |
| do { | |
| return try await sdkFilterPolicies( | |
| organizations: organizations, | |
| policyType: policyType, | |
| userId: userId, | |
| filter: filter, | |
| ) | |
| } catch { |
Also recommend adding SDK-path test coverage that exercises a non-nil filter (e.g. mirror test_isSendHideEmailDisabledByPolicy_optionDisabled and test_getMasterPasswordPolicyOptions_* with configService.featureFlagsBool[.policiesInAcceptedState] = true).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PM-37962/provider-organizations-is-provider-user #2727 +/- ##
==================================================================================
Coverage 87.26% 87.26%
==================================================================================
Files 1918 1919 +1
Lines 172394 173206 +812
==================================================================================
+ Hits 150436 151151 +715
- Misses 21958 22055 +97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fedemkr
changed the base branch from
main
to
PM-37962/provider-organizations-is-provider-user
fedemkr
changed the base branch from
PM-37962/provider-organizations-is-provider-user
to
main
fedemkr
changed the base branch from
main
to
PM-37962/provider-organizations-is-provider-user
| XCTAssertFalse(Organization.fixture(type: .custom).isAdmin) | ||
| XCTAssertFalse(Organization.fixture(type: .custom).isAdmin) |
There was a problem hiding this comment.
🎨 SUGGESTED: Duplicate assertion — both lines check .custom after the .manager → .custom rename.
Details and fix
Line 27 was previously XCTAssertFalse(Organization.fixture(type: .manager).isAdmin) and was rewritten to .custom when the enum case was removed. The result is two identical assertions on adjacent lines that add no coverage. Drop the duplicate:
| XCTAssertFalse(Organization.fixture(type: .custom).isAdmin) | |
| XCTAssertFalse(Organization.fixture(type: .custom).isAdmin) | |
| XCTAssertFalse(Organization.fixture(type: .custom).isAdmin) |
The same find/replace pattern was applied correctly in test_canManagePolicies and test_isExemptFromPolicies (each ends up with a single .custom assertion); only test_isAdmin ended up duplicated.
fedemkr
force-pushed
the
PM-37962/provider-organizations-is-provider-user
branch
from
5764120 to
561474d
Compare
Delegates policy filtering to the Bitwarden SDK via PoliciesClient.filterByType when the policiesInAcceptedState feature flag is enabled. Falls back to the native filter when the flag is off.
…k policies so it's applied appropriately
fedemkr
force-pushed
the
PM-38176/expose-policies-client-sdk
branch
from
04bd2e7 to
3ed1311
Compare
|
Warning @fedemkr Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
| import BitwardenSdk | ||
| import Foundation | ||
|
|
||
| class MockPoliciesClient: PoliciesClientProtocol { |
There was a problem hiding this comment.
❓ Could this use AutoMockable?
| var mockPlatform: MockPlatformClientService | ||
| var mockPlatformIsPreAuth = false | ||
| var mockPolicies: MockPoliciesClient | ||
| var policiesError: Error? |
There was a problem hiding this comment.
⛏️ Can you alphabetize this?
| /// - Parameter userId: The user ID mapped to the client instance. | ||
| /// - Returns: A `PoliciesClientProtocol` for policy data tasks. | ||
| /// | ||
| func policies(for userId: String?) async throws -> PoliciesClientProtocol |
There was a problem hiding this comment.
⛏️ Can you alphabetize this (there's a few instances down below too)?
| } | ||
|
|
||
| /// `policies(for:)` returns a non-nil `PoliciesClientProtocol` for every user. | ||
| func test_policies() async throws { |
There was a problem hiding this comment.
⛏️ Can you alphabetize this?
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38176
📔 Objective
Delegates policy filtering to the Bitwarden SDK via
PoliciesClient.filterByTypewhen the
policiesInAcceptedStatefeature flag is enabled. When the flag is off,the existing native filter is used unchanged.