Skip to content

Commit 277efe6

Browse files
fedemkrfedemkr
authored
[PM-25993] Fix default collection automatic selection based on organization policies (#2016)
1 parent f99774a commit 277efe6

File tree

9 files changed

+203
-16
lines changed

9 files changed

+203
-16
lines changed

‎BitwardenShared/Core/Vault/Helpers/CollectionHelper.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ protocol CollectionHelper { // sourcery: AutoMockable
99
}
1010

1111
/// Default implementation of `CollectionHelper`.
12-
struct DefaultCollectionHelper : CollectionHelper {
12+
struct DefaultCollectionHelper: CollectionHelper {
1313
// MARK: Properties
1414

1515
/// The service used to manage syncing and updates to the user's organizations.

‎BitwardenShared/Core/Vault/Services/PolicyService.swift‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@ protocol PolicyService: AnyObject {
4343
///
4444
func isSendHideEmailDisabledByPolicy() async -> Bool
4545

46+
/// Gets the organizations IDs that are applying the policy to the active user.
47+
///
48+
/// - Parameter policyType: The policy to check.
49+
/// - Returns: The organizations applying the policy to the active user.
50+
///
51+
func organizationsApplyingPolicyToUser(_ policyType: PolicyType) async -> [String]
52+
4653
/// Determines whether a policy applies to the active user.
4754
///
4855
/// - Parameter policyType: The policy to check.
@@ -358,6 +365,11 @@ extension DefaultPolicyService {
358365
}
359366
}
360367

368+
func organizationsApplyingPolicyToUser(_ policyType: PolicyType) async -> [String] {
369+
let policies = await policiesApplyingToUser(policyType, filter: nil)
370+
return policies.map(\.organizationId)
371+
}
372+
361373
func policyAppliesToUser(_ policyType: PolicyType) async -> Bool {
362374
await policyAppliesToUser(policyType, filter: nil)
363375
}

‎BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,22 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod
415415
XCTAssertNil(policyValues?.action)
416416
}
417417

418+
/// `organizationsApplyingPolicyToUser(_:)` returns the organization IDs which apply the policy.
419+
func test_organizationsApplyingPolicyToUser() async {
420+
stateService.activeAccount = .fixture()
421+
organizationService.fetchAllOrganizationsResult = .success([
422+
.fixture(id: "org-1"),
423+
.fixture(id: "org-2"),
424+
])
425+
policyDataStore.fetchPoliciesResult = .success([
426+
.fixture(enabled: false, organizationId: "org-1", type: .twoFactorAuthentication),
427+
.fixture(enabled: true, organizationId: "org-2", type: .twoFactorAuthentication),
428+
])
429+
430+
let organizations = await subject.organizationsApplyingPolicyToUser(.twoFactorAuthentication)
431+
XCTAssertEqual(organizations, ["org-2"])
432+
}
433+
418434
/// `policyAppliesToUser(_:)` returns whether the policy applies to the user.
419435
func test_policyAppliesToUser() async {
420436
stateService.activeAccount = .fixture()

‎BitwardenShared/Core/Vault/Services/TestHelpers/MockPolicyService.swift‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ class MockPolicyService: PolicyService {
2222

2323
var fetchTimeoutPolicyValuesResult: Result<(SessionTimeoutAction?, Int)?, Error> = .success(nil)
2424

25+
var organizationsApplyingPolicyToUserResult: [PolicyType: [String]] = [:]
26+
2527
var policyAppliesToUserResult = [PolicyType: Bool]()
2628
var policyAppliesToUserPoliciesType = [PolicyType]()
2729
var policyAppliesToUserPolicies = [Policy]()
@@ -60,6 +62,10 @@ class MockPolicyService: PolicyService {
6062
try fetchTimeoutPolicyValuesResult.get()
6163
}
6264

65+
func organizationsApplyingPolicyToUser(_ policyType: BitwardenShared.PolicyType) async -> [String] {
66+
organizationsApplyingPolicyToUserResult[policyType] ?? []
67+
}
68+
6369
func policyAppliesToUser(_ policyType: PolicyType) async -> Bool {
6470
policyAppliesToUserPoliciesType.append(policyType)
6571
return policyAppliesToUserResult[policyType] ?? false

‎BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemProcessor.swift‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,9 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
280280
/// Fetches any additional data (e.g. organizations and folders) needed for adding or editing a cipher.
281281
private func fetchCipherOptions() async {
282282
do {
283-
let isPersonalOwnershipDisabled = await services.policyService.policyAppliesToUser(.personalOwnership)
283+
state.organizationsWithPersonalOwnershipPolicy = await services.policyService
284+
.organizationsApplyingPolicyToUser(.personalOwnership)
285+
let isPersonalOwnershipDisabled = !state.organizationsWithPersonalOwnershipPolicy.isEmpty
284286
let ownershipOptions = try await services.vaultRepository
285287
.fetchCipherOwnershipOptions(includePersonal: !isPersonalOwnershipDisabled)
286288

@@ -301,12 +303,9 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_
301303
state.owner = ownershipOptions.first
302304
}
303305

304-
if state.configuration.isAdding {
305-
let defaultCollection = state.collectionsForOwner.first(where: { $0.type == .defaultUserCollection })
306-
if let defaultCollectionId = defaultCollection?.id, state.collectionIds.isEmpty {
307-
state.collectionIds.append(defaultCollectionId)
308-
}
309-
} else {
306+
state.selectDefaultCollectionIfNeeded()
307+
308+
if !state.configuration.isAdding {
310309
await services.eventService.collect(eventType: .cipherClientViewed, cipherId: state.cipher.id)
311310
}
312311
} catch {

‎BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemProcessorTests.swift‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,7 +1056,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
10561056
]
10571057
vaultRepository.fetchCollectionsResult = .success(collections)
10581058

1059-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1059+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
10601060

10611061
await subject.perform(.fetchCipherOptions)
10621062

@@ -1092,7 +1092,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
10921092
.fixture(id: "2", name: "Engineering"),
10931093
])
10941094

1095-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1095+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
10961096

10971097
await subject.perform(.fetchCipherOptions)
10981098

@@ -1138,7 +1138,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
11381138
.fixture(id: "3", name: "Platform", organizationId: "1"),
11391139
]
11401140

1141-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1141+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
11421142
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
11431143
vaultRepository.fetchCollectionsResult = .success(collections)
11441144

@@ -1160,7 +1160,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
11601160
.fixture(id: "3", name: "Platform", organizationId: "1", type: .defaultUserCollection),
11611161
]
11621162

1163-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1163+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
11641164
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
11651165
vaultRepository.fetchCollectionsResult = .success(collections)
11661166

@@ -1180,7 +1180,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
11801180
.fixture(id: "2", name: "Engineering", organizationId: "1", type: .sharedCollection),
11811181
]
11821182

1183-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1183+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
11841184
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
11851185
vaultRepository.fetchCollectionsResult = .success(collections)
11861186

@@ -1200,7 +1200,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
12001200
.fixture(id: "2", name: "Engineering", organizationId: "1", type: .sharedCollection),
12011201
]
12021202

1203-
policyService.policyAppliesToUserResult[.personalOwnership] = false
1203+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = []
12041204
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
12051205
vaultRepository.fetchCollectionsResult = .success(collections)
12061206

@@ -1220,7 +1220,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
12201220
.fixture(id: "2", name: "Engineering", organizationId: "1"),
12211221
]
12221222

1223-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1223+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
12241224
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
12251225
vaultRepository.fetchCollectionsResult = .success(collections)
12261226

@@ -1245,7 +1245,7 @@ class AddEditItemProcessorTests: BitwardenTestCase {
12451245
.fixture(id: "2", name: "Engineering", organizationId: "1"),
12461246
]
12471247

1248-
policyService.policyAppliesToUserResult[.personalOwnership] = true
1248+
policyService.organizationsApplyingPolicyToUserResult[.personalOwnership] = ["1"]
12491249
vaultRepository.fetchCipherOwnershipOptions = [.organization(id: "1", name: "OrgTest")]
12501250
vaultRepository.fetchCollectionsResult = .success(collections)
12511251

‎BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemState.swift‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,9 @@ protocol AddEditItemState: Sendable {
8888
/// The organization ID of the cipher, if the cipher is owned by an organization.
8989
var organizationId: String? { get }
9090

91+
/// The organization IDs that have `.personalOwnership` policy applied.
92+
var organizationsWithPersonalOwnershipPolicy: [String] { get set }
93+
9194
/// The owner of this item.
9295
var owner: CipherOwner? { get set }
9396

@@ -123,6 +126,9 @@ protocol AddEditItemState: Sendable {
123126
///
124127
mutating func toggleCollection(newValue: Bool, collectionId: String)
125128

129+
/// Selects the `.defaultUserCollection` if needed, mainly checking the organization policies apply.
130+
mutating func selectDefaultCollectionIfNeeded()
131+
126132
/// Updates the `CipherView` fields of `CipherItemState` with an updated `CipherView`. This will
127133
/// preserve any additional UI properties on the state.
128134
///

‎BitwardenShared/UI/Vault/VaultItem/CipherItemState.swift‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,9 @@ struct CipherItemState: Equatable { // swiftlint:disable:this type_body_length
107107
/// The name of the organization the cipher belongs to, if any.
108108
var organizationName: String?
109109

110+
/// The organization IDs that have `.personalOwnership` policy applied.
111+
var organizationsWithPersonalOwnershipPolicy: [String] = []
112+
110113
/// The list of ownership options that can be selected for the cipher.
111114
var ownershipOptions = [CipherOwner]()
112115

@@ -248,6 +251,7 @@ struct CipherItemState: Equatable { // swiftlint:disable:this type_body_length
248251
set {
249252
organizationId = newValue?.organizationId
250253
collectionIds = []
254+
selectDefaultCollectionIfNeeded()
251255
}
252256
}
253257

@@ -425,6 +429,8 @@ struct CipherItemState: Equatable { // swiftlint:disable:this type_body_length
425429
}
426430

427431
extension CipherItemState: AddEditItemState {
432+
// MARK: Properties
433+
428434
var navigationTitle: String {
429435
switch configuration {
430436
case .add:
@@ -446,6 +452,25 @@ extension CipherItemState: AddEditItemState {
446452
}
447453
}
448454

455+
// MARK: Methods
456+
457+
mutating func selectDefaultCollectionIfNeeded() {
458+
guard configuration.isAdding else {
459+
return
460+
}
461+
462+
let defaultCollectionForOwner = collectionsForOwner.first(where: { $0.type == .defaultUserCollection })
463+
464+
guard let defaultCollectionId = defaultCollectionForOwner?.id,
465+
collectionIds.isEmpty,
466+
let ownerOrganizationId = owner?.organizationId,
467+
organizationsWithPersonalOwnershipPolicy.contains(ownerOrganizationId) else {
468+
return
469+
}
470+
471+
collectionIds.append(defaultCollectionId)
472+
}
473+
449474
mutating func update(from cipherView: CipherView) {
450475
apply(cipherView: cipherView)
451476
}

‎BitwardenShared/UI/Vault/VaultItem/CipherItemStateTests.swift‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,129 @@ class CipherItemStateTests: BitwardenTestCase { // swiftlint:disable:this type_b
408408
XCTAssertEqual(subjectSSHKey.navigationTitle, Localizations.newSSHKey)
409409
}
410410

411+
/// `setter:owner` adds the default user collection to the collection IDs
412+
/// when it's adding, there's a default user collection for the owner organization and such
413+
/// organization has the `.personalOwnership` policy turned on.
414+
func test_owner_addsDefaultCollection() {
415+
var subject = CipherItemState(hasPremium: false)
416+
subject.organizationId = "2"
417+
subject.ownershipOptions = [
418+
.organization(id: "1", name: "Org"),
419+
.organization(id: "2", name: "Org2"),
420+
.organization(id: "3", name: "Org3"),
421+
]
422+
subject.allUserCollections = [
423+
.fixture(id: "1", organizationId: "1", type: .defaultUserCollection),
424+
.fixture(id: "2", organizationId: "1"),
425+
.fixture(id: "3", organizationId: "2"),
426+
.fixture(id: "4", organizationId: "2", type: .defaultUserCollection),
427+
]
428+
subject.organizationsWithPersonalOwnershipPolicy = ["1", "2"]
429+
subject.collectionIds = []
430+
431+
subject.owner = .organization(id: "1", name: "Org")
432+
XCTAssertEqual(subject.collectionIds, ["1"])
433+
}
434+
435+
/// `selectDefaultCollectionIfNeeded()` adds the default user collection to the collection IDs
436+
/// when it's adding, there's a default user collection for the owner organization and such
437+
/// organization has the `.personalOwnership` policy turned on.
438+
func test_selectDefaultCollectionIfNeeded_addsDefaultCollection() {
439+
var subject = CipherItemState(hasPremium: false)
440+
subject.organizationId = "1"
441+
subject.ownershipOptions = [
442+
.organization(id: "1", name: "Org"),
443+
.organization(id: "2", name: "Org2"),
444+
.organization(id: "3", name: "Org3"),
445+
]
446+
subject.allUserCollections = [
447+
.fixture(id: "1", organizationId: "1", type: .defaultUserCollection),
448+
.fixture(id: "2", organizationId: "1"),
449+
.fixture(id: "3", organizationId: "2"),
450+
.fixture(id: "4", organizationId: "2", type: .defaultUserCollection),
451+
]
452+
subject.organizationsWithPersonalOwnershipPolicy = ["1", "2"]
453+
subject.collectionIds = []
454+
455+
subject.selectDefaultCollectionIfNeeded()
456+
XCTAssertEqual(subject.collectionIds, ["1"])
457+
458+
// added this to check that the collection doesn't get duplicated
459+
// when the function is called twice.
460+
subject.selectDefaultCollectionIfNeeded()
461+
XCTAssertEqual(subject.collectionIds, ["1"])
462+
}
463+
464+
/// `selectDefaultCollectionIfNeeded()` doesn't add the default user collection to the collection IDs
465+
/// when it's editing.
466+
func test_selectDefaultCollectionIfNeeded_editing() throws {
467+
var subject = try XCTUnwrap(CipherItemState(existing: .fixture(id: "1"), hasPremium: false))
468+
subject.organizationId = "1"
469+
subject.ownershipOptions = [
470+
.organization(id: "1", name: "Org"),
471+
.organization(id: "2", name: "Org2"),
472+
.organization(id: "3", name: "Org3"),
473+
]
474+
subject.allUserCollections = [
475+
.fixture(id: "1", organizationId: "1", type: .defaultUserCollection),
476+
.fixture(id: "2", organizationId: "1"),
477+
.fixture(id: "3", organizationId: "2"),
478+
.fixture(id: "4", organizationId: "2", type: .defaultUserCollection),
479+
]
480+
subject.organizationsWithPersonalOwnershipPolicy = ["1", "2"]
481+
subject.collectionIds = []
482+
483+
subject.selectDefaultCollectionIfNeeded()
484+
XCTAssertEqual(subject.collectionIds, [])
485+
}
486+
487+
/// `selectDefaultCollectionIfNeeded()` doesn't add the default user collection to the collection IDs
488+
/// when it's adding, but there's no default user collection for the owner organization.
489+
func test_selectDefaultCollectionIfNeeded_noDefaultCollection() {
490+
var subject = CipherItemState(hasPremium: false)
491+
subject.organizationId = "1"
492+
subject.ownershipOptions = [
493+
.organization(id: "1", name: "Org"),
494+
.organization(id: "2", name: "Org2"),
495+
.organization(id: "3", name: "Org3"),
496+
]
497+
subject.allUserCollections = [
498+
.fixture(id: "1", organizationId: "1"),
499+
.fixture(id: "2", organizationId: "1"),
500+
.fixture(id: "3", organizationId: "2"),
501+
.fixture(id: "4", organizationId: "2", type: .defaultUserCollection),
502+
]
503+
subject.organizationsWithPersonalOwnershipPolicy = ["1", "2"]
504+
subject.collectionIds = []
505+
506+
subject.selectDefaultCollectionIfNeeded()
507+
XCTAssertEqual(subject.collectionIds, [])
508+
}
509+
510+
/// `selectDefaultCollectionIfNeeded()` doesn't add the default user collection to the collection IDs
511+
/// when it's adding, there's default user collection for the owner organization and such
512+
/// organization has the `.personalOwnership` policy turned off.
513+
func test_selectDefaultCollectionIfNeeded_personalOwnershipOff() {
514+
var subject = CipherItemState(hasPremium: false)
515+
subject.organizationId = "1"
516+
subject.ownershipOptions = [
517+
.organization(id: "1", name: "Org"),
518+
.organization(id: "2", name: "Org2"),
519+
.organization(id: "3", name: "Org3"),
520+
]
521+
subject.allUserCollections = [
522+
.fixture(id: "1", organizationId: "1", type: .defaultUserCollection),
523+
.fixture(id: "2", organizationId: "1"),
524+
.fixture(id: "3", organizationId: "2"),
525+
.fixture(id: "4", organizationId: "2", type: .defaultUserCollection),
526+
]
527+
subject.organizationsWithPersonalOwnershipPolicy = ["2"]
528+
subject.collectionIds = []
529+
530+
subject.selectDefaultCollectionIfNeeded()
531+
XCTAssertEqual(subject.collectionIds, [])
532+
}
533+
411534
/// `shouldShowLearnNewLoginActionCard` should be `true`, if the cipher is a login type and configuration is `.add`.
412535
func test_shouldShowLearnNewLoginActionCard_true() {
413536
let cipher = CipherView.loginFixture(login: .fixture(fido2Credentials: [.fixture()]))

0 commit comments

Comments
 (0)