From 8d4c5c2bb50c2e45926467a7faa8f64aabec7548 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Wed, 18 Sep 2024 14:55:41 +0200 Subject: [PATCH 1/5] Rename defaultType to overridePasswordType. Force password generator to follow overridePasswordType --- .../Models/Domain/PasswordGenerationOptions.swift | 3 +++ .../Core/Vault/Models/Enum/PolicyOptionType.swift | 2 +- .../Core/Vault/Services/PolicyService.swift | 12 ++++++++---- .../Generator/Generator/GeneratorProcessor.swift | 4 +--- .../UI/Tools/Generator/Generator/GeneratorView.swift | 2 +- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift index 2cd587a5b..f7c42816d 100644 --- a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift +++ b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift @@ -48,6 +48,9 @@ struct PasswordGenerationOptions: Codable, Equatable { /// The separator to put between words in the passphrase. var wordSeparator: String? + + /// Whether the password type should be enforced or not + var overridePasswordType: Bool? } extension PasswordGenerationOptions { diff --git a/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift b/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift index d84969faf..967861395 100644 --- a/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift +++ b/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift @@ -10,7 +10,7 @@ enum PolicyOptionType: String { case capitalize /// A policy option for the default type of the password generator. - case defaultType + case overridePasswordType /// A policy option for whether to include a number in a passphrase. case includeNumber diff --git a/BitwardenShared/Core/Vault/Services/PolicyService.swift b/BitwardenShared/Core/Vault/Services/PolicyService.swift index 04ce87ebc..76eb63f73 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyService.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyService.swift @@ -179,15 +179,19 @@ extension DefaultPolicyService { // When determining the generator type, ignore the existing option's type to find the preferred // default type based on the policies. Then set it on `options` below. var generatorType: PasswordGeneratorType? + options.overridePasswordType = false for policy in policies { - if let defaultTypeString = policy[.defaultType]?.stringValue, - let defaultType = PasswordGeneratorType(rawValue: defaultTypeString), - generatorType != .password { + if let overridePasswordTypeString = policy[.overridePasswordType]?.stringValue, + let overridePasswordType = PasswordGeneratorType(rawValue: overridePasswordTypeString), + generatorType != .password, + options.overridePasswordType != true { // If there's multiple policies with different default types, the password type // should take priority. Use `generateType` as opposed to `options.type` to ignore // the existing type in the options. - generatorType = defaultType + generatorType = overridePasswordType } + // if the stringValue is NOT emptu or null, overridePasswordType should be true + options.overridePasswordType = !(policy[.overridePasswordType]?.stringValue?.isEmpty ?? true) if let minLength = policy[.minLength]?.intValue { options.setMinLength(minLength) diff --git a/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessor.swift b/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessor.swift index b46dbd011..7a5285060 100644 --- a/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessor.swift +++ b/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessor.swift @@ -348,9 +348,7 @@ final class GeneratorProcessor: StateProcessor Date: Wed, 18 Sep 2024 14:56:25 +0200 Subject: [PATCH 2/5] fix and add tests for password generator policy change --- .../Vault/Services/PolicyServiceTests.swift | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift index f60bd2be6..4f7aa7883 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift @@ -41,7 +41,7 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod let passwordGeneratorPolicy = Policy.fixture( data: [ PolicyOptionType.capitalize.rawValue: .bool(true), - PolicyOptionType.defaultType.rawValue: .string(PasswordGeneratorType.passphrase.rawValue), + PolicyOptionType.overridePasswordType.rawValue: .string(PasswordGeneratorType.passphrase.rawValue), PolicyOptionType.includeNumber.rawValue: .bool(false), PolicyOptionType.minLength.rawValue: .int(30), PolicyOptionType.minNumbers.rawValue: .int(3), @@ -84,7 +84,7 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod /// `applyPasswordGenerationOptions(options:)` applies the password generation policy to the /// options and if the existing option has a type set, the policies will override that. - func test_applyPasswordGenerationOptions_defaultType_existingOption() async throws { + func test_applyPasswordGenerationOptions_overridePasswordType_existingOption() async throws { stateService.activeAccount = .fixture() organizationService.fetchAllOrganizationsResult = .success([.fixture()]) policyDataStore.fetchPoliciesResult = .success([passwordGeneratorPolicy]) @@ -145,7 +145,8 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod numWords: 4, special: true, type: .passphrase, - uppercase: nil + uppercase: nil, + overridePasswordType: true ) ) } @@ -159,7 +160,7 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod [ .fixture( data: [ - PolicyOptionType.defaultType.rawValue: .string("password"), + PolicyOptionType.overridePasswordType.rawValue: .string("password"), ], type: .passwordGenerator ), @@ -211,11 +212,32 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod numWords: 4, special: true, type: .passphrase, - uppercase: nil + uppercase: nil, + overridePasswordType: true ) ) } + /// `applyPasswordGenerationOptions(options:)` applies the password generation policy to the + /// options. + func test_applyPasswordGenerationOptions_policy_noOverride() async throws { + stateService.activeAccount = .fixture() + organizationService.fetchAllOrganizationsResult = .success([.fixture()]) + policyDataStore.fetchPoliciesResult = .success( + [ + .fixture( + data: [:], + type: .passwordGenerator + ), + ]) + + var options = PasswordGenerationOptions() + let appliedPolicy = try await subject.applyPasswordGenerationPolicy(options: &options) + + XCTAssertTrue(appliedPolicy) + XCTAssertEqual(options.overridePasswordType, false) + } + /// `applyPasswordGenerationOptions(options:)` applies the password generation policy to the /// options when there's existing options. func test_applyPasswordGenerationOptions_policy_existingOptions() async throws { @@ -248,7 +270,8 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod numWords: 4, special: true, type: .passphrase, - uppercase: true + uppercase: true, + overridePasswordType: true ) ) } From 2e525baebc06aa6009fe4ff717e63e49764175ef Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Thu, 19 Sep 2024 12:14:04 +0200 Subject: [PATCH 3/5] address pr feedback --- .../Models/Domain/PasswordGenerationOptions.swift | 2 +- .../Core/Vault/Models/Enum/PolicyOptionType.swift | 2 +- .../Core/Vault/Services/PolicyService.swift | 8 +++----- .../Core/Vault/Services/PolicyServiceTests.swift | 10 ++++++++-- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift index f7c42816d..3e680ec77 100644 --- a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift +++ b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift @@ -50,7 +50,7 @@ struct PasswordGenerationOptions: Codable, Equatable { var wordSeparator: String? /// Whether the password type should be enforced or not - var overridePasswordType: Bool? + var overridePasswordType: Bool = false } extension PasswordGenerationOptions { diff --git a/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift b/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift index 967861395..f61ef0926 100644 --- a/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift +++ b/BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift @@ -9,7 +9,7 @@ enum PolicyOptionType: String { /// A policy option for whether to capitalize the passphrase words. case capitalize - /// A policy option for the default type of the password generator. + /// A policy option for the enforced type of the password generator. case overridePasswordType /// A policy option for whether to include a number in a passphrase. diff --git a/BitwardenShared/Core/Vault/Services/PolicyService.swift b/BitwardenShared/Core/Vault/Services/PolicyService.swift index 76eb63f73..a8c46bfda 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyService.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyService.swift @@ -183,16 +183,14 @@ extension DefaultPolicyService { for policy in policies { if let overridePasswordTypeString = policy[.overridePasswordType]?.stringValue, let overridePasswordType = PasswordGeneratorType(rawValue: overridePasswordTypeString), - generatorType != .password, - options.overridePasswordType != true { + generatorType != .password { // If there's multiple policies with different default types, the password type // should take priority. Use `generateType` as opposed to `options.type` to ignore // the existing type in the options. generatorType = overridePasswordType + options.overridePasswordType = true } - // if the stringValue is NOT emptu or null, overridePasswordType should be true - options.overridePasswordType = !(policy[.overridePasswordType]?.stringValue?.isEmpty ?? true) - + if let minLength = policy[.minLength]?.intValue { options.setMinLength(minLength) } diff --git a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift index 4f7aa7883..22f719264 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift @@ -158,13 +158,19 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod organizationService.fetchAllOrganizationsResult = .success([.fixture()]) policyDataStore.fetchPoliciesResult = .success( [ + passwordGeneratorPolicy, .fixture( data: [ - PolicyOptionType.overridePasswordType.rawValue: .string("password"), + PolicyOptionType.overridePasswordType.rawValue: .string(PasswordGeneratorType.password.rawValue), + ], + type: .passwordGenerator + ), + .fixture( + data: [ + PolicyOptionType.overridePasswordType.rawValue: .string(PasswordGeneratorType.passphrase.rawValue), ], type: .passwordGenerator ), - passwordGeneratorPolicy, ] ) From 86ea80aeaf9d826c9b147bb9c64267168baef3d2 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Thu, 19 Sep 2024 15:11:02 +0200 Subject: [PATCH 4/5] fixed test_generatePassword_appliesPolicies_generatorTypeChange and swiftformat changes on PolicyService --- BitwardenShared/Core/Vault/Services/PolicyService.swift | 2 +- .../Tools/Generator/Generator/GeneratorProcessorTests.swift | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/BitwardenShared/Core/Vault/Services/PolicyService.swift b/BitwardenShared/Core/Vault/Services/PolicyService.swift index a8c46bfda..3163dce5e 100644 --- a/BitwardenShared/Core/Vault/Services/PolicyService.swift +++ b/BitwardenShared/Core/Vault/Services/PolicyService.swift @@ -190,7 +190,7 @@ extension DefaultPolicyService { generatorType = overridePasswordType options.overridePasswordType = true } - + if let minLength = policy[.minLength]?.intValue { options.setMinLength(minLength) } diff --git a/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessorTests.swift b/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessorTests.swift index ba2fa55c6..cb0b55113 100644 --- a/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessorTests.swift +++ b/BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessorTests.swift @@ -247,7 +247,7 @@ class GeneratorProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ } /// Generating a new password applies any policies to the options before generating the value, - /// but doesn't override the generator type. + /// and overrides the generator type. @MainActor func test_generatePassword_appliesPolicies_generatorTypeChange() throws { waitFor(subject.didLoadGeneratorOptions) @@ -260,9 +260,9 @@ class GeneratorProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ subject.state.passwordState.passwordGeneratorType = .passphrase subject.receive(.refreshGeneratedValue) - waitFor { generatorRepository.passphraseGeneratorRequest != nil } + waitFor { generatorRepository.passwordGeneratorRequest != nil } - XCTAssertEqual(subject.state.passwordState.passwordGeneratorType, .passphrase) + XCTAssertEqual(subject.state.passwordState.passwordGeneratorType, .password) } /// If an error occurs generating an username, an alert is shown. From 2f1ff82fdee23c186dffd22f43f928bfc08035df Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Fri, 20 Sep 2024 14:48:49 +0200 Subject: [PATCH 5/5] fixed PasswordGenerationOptions tests --- .../Tools/Models/Domain/PasswordGenerationOptions.swift | 2 +- .../Models/Domain/PasswordGenerationOptionsTests.swift | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift index 3e680ec77..f7c42816d 100644 --- a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift +++ b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift @@ -50,7 +50,7 @@ struct PasswordGenerationOptions: Codable, Equatable { var wordSeparator: String? /// Whether the password type should be enforced or not - var overridePasswordType: Bool = false + var overridePasswordType: Bool? } extension PasswordGenerationOptions { diff --git a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptionsTests.swift b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptionsTests.swift index fab18e886..2a5c41fb1 100644 --- a/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptionsTests.swift +++ b/BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptionsTests.swift @@ -23,7 +23,8 @@ class PasswordGenerationOptionsTests: BitwardenTestCase { "special": true, "type": "passphrase", "uppercase": true, - "wordSeparator": "-" + "wordSeparator": "-", + "overridePasswordType": false } """ let data = try XCTUnwrap(json.data(using: .utf8)) @@ -45,7 +46,8 @@ class PasswordGenerationOptionsTests: BitwardenTestCase { special: true, type: .passphrase, uppercase: true, - wordSeparator: "-" + wordSeparator: "-", + overridePasswordType: false ) ) }