Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PM-11894] update password generator policy #948

Merged
merged 5 commits into from
Sep 24, 2024
Merged

[PM-11894] update password generator policy #948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d4c5c2
Rename defaultType to overridePasswordType. Force password generator …
aj-rosado Sep 18, 2024
3feb1fb
fix and add tests for password generator policy change
aj-rosado Sep 18, 2024
2e525ba
address pr feedback
aj-rosado Sep 19, 2024
86ea80a
fixed test_generatePassword_appliesPolicies_generatorTypeChange and s…
aj-rosado Sep 19, 2024
2f1ff82
fixed PasswordGenerationOptions tests
aj-rosado Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Why is this boolean optional? What does a nil value represent as different from a false value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should never be nil actually, removed the optional

Copy link
Contributor Author

@aj-rosado aj-rosado Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is making the test_decode_empty on PasswordGenerationOptionsTests to fail.

I'm afraid it might throw an exception when calling the decodePasswordOptions and there is no overridePasswordType.

Not sure on what scenarios this is used

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This model is persisted to UserDefaults to maintain the user's options between app runs. The optional bool allows decoding to succeed even if it doesn't exist in the previously persisted model.

}

extension PasswordGenerationOptions {
Expand Down
6 changes: 4 additions & 2 deletions BitwardenShared/Core/Tools/Models/Domain/PasswordGenerationOptionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -45,7 +46,8 @@ class PasswordGenerationOptionsTests: BitwardenTestCase {
special: true,
type: .passphrase,
uppercase: true,
wordSeparator: "-"
wordSeparator: "-",
overridePasswordType: false
)
)
}
Expand Down
4 changes: 2 additions & 2 deletions BitwardenShared/Core/Vault/Models/Enum/PolicyOptionType.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ 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.
case defaultType
/// A policy option for the enforced type of the password generator.
case overridePasswordType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ The documentation comment should probably be updated as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!


/// A policy option for whether to include a number in a passphrase.
case includeNumber
Expand Down
8 changes: 5 additions & 3 deletions BitwardenShared/Core/Vault/Services/PolicyService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,16 @@ 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),
if let overridePasswordTypeString = policy[.overridePasswordType]?.stringValue,
let overridePasswordType = PasswordGeneratorType(rawValue: overridePasswordTypeString),
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 = defaultType
generatorType = overridePasswordType
options.overridePasswordType = true
}

if let minLength = policy[.minLength]?.intValue {
Expand Down
43 changes: 36 additions & 7 deletions BitwardenShared/Core/Vault/Services/PolicyServiceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -145,7 +145,8 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod
numWords: 4,
special: true,
type: .passphrase,
uppercase: nil
uppercase: nil,
overridePasswordType: true
)
)
}
Expand All @@ -157,13 +158,19 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod
organizationService.fetchAllOrganizationsResult = .success([.fixture()])
policyDataStore.fetchPoliciesResult = .success(
[
passwordGeneratorPolicy,
.fixture(
data: [
PolicyOptionType.defaultType.rawValue: .string("password"),
PolicyOptionType.overridePasswordType.rawValue: .string(PasswordGeneratorType.password.rawValue),
],
type: .passwordGenerator
),
.fixture(
data: [
PolicyOptionType.overridePasswordType.rawValue: .string(PasswordGeneratorType.passphrase.rawValue),
],
type: .passwordGenerator
),
passwordGeneratorPolicy,
]
)

Expand Down Expand Up @@ -211,11 +218,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 {
Expand Down Expand Up @@ -248,7 +276,8 @@ class PolicyServiceTests: BitwardenTestCase { // swiftlint:disable:this type_bod
numWords: 4,
special: true,
type: .passphrase,
uppercase: true
uppercase: true,
overridePasswordType: true
)
)
}
Expand Down
4 changes: 1 addition & 3 deletions BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,7 @@ final class GeneratorProcessor: StateProcessor<GeneratorState, GeneratorAction,
var passwordOptions = state.passwordState.passwordGenerationOptions
state.isPolicyInEffect = await (try? services.policyService
.applyPasswordGenerationPolicy(options: &passwordOptions)) ?? false
// When updating the state, don't update the password generator type or it can override the
// user's current selection.
state.passwordState.update(with: passwordOptions, shouldUpdateGeneratorType: false)
state.passwordState.update(with: passwordOptions, shouldUpdateGeneratorType: true)
fedemkr marked this conversation as resolved.
Show resolved Hide resolved

var policyOptions = PasswordGenerationOptions()
_ = try? await services.policyService.applyPasswordGenerationPolicy(options: &policyOptions)
Expand Down
6 changes: 3 additions & 3 deletions BitwardenShared/UI/Tools/Generator/Generator/GeneratorProcessorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion BitwardenShared/UI/Tools/Generator/Generator/GeneratorView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ struct GeneratorView: View {
case let .menuPasswordGeneratorType(menuField):
FormMenuFieldView(field: menuField) { newValue in
store.send(.passwordGeneratorTypeChanged(newValue))
}
}.disabled(store.state.policyOptions?.overridePasswordType ?? false)
case let .menuUsernameForwardedEmailService(menuField):
FormMenuFieldView(field: menuField) { newValue in
store.send(.usernameForwardedEmailServiceChanged(newValue))
Expand Down
Loading