[PM-38866] feat: Integrate SDK-based password generation into AutoFill extension - #2828
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the SDK-based password generation integration into the AutoFill extension across the no-UI ( Code Review DetailsNo new blocking findings. Notes on items reviewed and intentionally not flagged:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| mutating func setMinLength(_ minimumLength: Int) { | ||
| if let length, length < minimumLength { | ||
| self.length = minimumLength | ||
| } else if length == nil { | ||
| if let length { | ||
| if length > minimumLength { | ||
| self.length = minimumLength | ||
| } | ||
| } else { | ||
| length = minimumLength | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ CRITICAL: Inverted comparison breaks minimum-length enforcement (org policy + password rules).
Details and fix
This PR inverted the comparison in setMinLength. The previous implementation raised a too-short length up to the minimum; the new one lowers length down to minimumLength when the current length is larger, and does nothing when the current length is shorter — the opposite of "set minimum".
Impact:
PolicyService.applyPasswordGenerationPolicy(PolicyService.swift:350) callssetMinLength(minLength)to enforce an organization's minimum password-length policy. With this change, when the org policy minimum (e.g. 20) exceeds the user's saved length (e.g. 14), the policy is not enforced and a shorter password is generated. When the policy minimum is below the saved length, the length is truncated down, weakening the user's chosen length.apply(_:)(this file, line 179) callssetMinLength(Int(request.length))for AutoFill site rules, so site-required lengths are likewise ignored.
This also contradicts the unchanged existing test test_setMinLength (PasswordGenerationOptionsTests.swift:64, expects raise-up) and the new test_apply_raisesLength / test_apply_preservesHigherMinimums, plus the apply(_:) doc ("raised to the maximum of the two values"). All four sibling setMin* functions still raise up.
mutating func setMinLength(_ minimumLength: Int) {
if let length, length < minimumLength {
self.length = minimumLength
} else if length == nil {
length = minimumLength
}
}
github-actions
Bot
removed
the
app:authenticator
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2828 +/- ##
==========================================
- Coverage 81.26% 81.23% -0.04%
==========================================
Files 1028 1029 +1
Lines 66273 66378 +105
==========================================
+ Hits 53858 53919 +61
- Misses 12415 12459 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… flow Password generation doesn't require vault access, so skip the auth/unlock screen by passing an initialRoute directly to AppProcessor.start and guard saveGeneratedValue against adding to password history when the vault is not unlocked.
- Unify all setMin* methods in PasswordGenerationOptions to `field = max(field ?? minimum, minimum)` for consistency; also fix the inverted condition in setMinLength and a wrong param name in the setMinSpecial doc comment - Simplify MockGeneratorRepository.passwordRulesRequestResult from Result<PasswordGeneratorRequest, Error> to PasswordGeneratorRequest? - Extract CredentialProviderMode.generatePasswordRules computed property and add CredentialProviderModeTests
…tial A merge conflict resolution in f292ba1 reintroduced the unlockVaultWithNeverlockKey() call that had been intentionally removed in 3713d75, since password generation only relies on the SDK/local settings and has no vault dependency. This restores that fix so AppProcessorTests.test_generatePasswordCredential passes again.
…edPasswordRules, add Info.plist capabilities
b5150e0 to
c55da60
Compare
| options: &passwordOptions, | ||
| ) | ||
| let (passwordOptions, isPolicyInEffect) = try await services.generatorRepository | ||
| .getEffectivePasswordGenerationOptions(rules: state.forcedPasswordRules) |
There was a problem hiding this comment.
❓ QUESTION: forcedPasswordRules enforcement appears to be dropped after this initial load.
Details
forcedPasswordRules is only consumed here in loadGeneratorOptions(). On .appeared, reloadGeneratorOptions() runs (setting state.isPolicyInEffect from the rules-derived value), then generateValue(shouldSavePassword: true) immediately calls validatePasswordOptionsAndApplyPolicies(), which recomputes state.isPolicyInEffect from applyPasswordGenerationPolicy (org policy only) and re-applies only the org policy — the site rules are not re-applied.
Two consequences:
- The rules-derived
isPolicyInEffect == truereturned bygetEffectivePasswordGenerationOptionsis overwritten on the first generation, so the "policy in effect" indicator/locking for site rules effectively never persists in the with-UI flow. - Since
validateOptions()/org-policy application no longer treat the site-rule minimums as floors, a user lowering a slider below a rule minimum won't be re-clamped on regeneration.
Is this intended (rules seed the initial options, user may freely override), or should the site rules remain enforced across regenerations? If the former, the true return value from getEffectivePasswordGenerationOptions when rules are applied may be misleading for the UI path.
| let view = try XCTUnwrap((action.view as? UIHostingController<VaultAutofillListView>)?.rootView) | ||
| XCTAssertEqual(view.store.state.group, .identity) | ||
| } | ||
|
|
There was a problem hiding this comment.
❓ QUESTION: This PR deletes test_navigateTo_flightRecorderSettings, which covered existing behavior unrelated to SDK password generation.
Details
The .flightRecorderSettings route and its handler in VaultCoordinator.swift (delegate?.switchToSettingsTab(route: .about)) still exist and are unchanged by this PR. Removing this test drops coverage for that path with no corresponding production change — this looks like an accidental deletion (possibly a rebase artifact). Was this intentional? If not, please restore the test.
…tions forcedPasswordRules was only applied once during loadGeneratorOptions. validatePasswordOptionsAndApplyPolicies (called on every generation) only applied org policy, so a user could lower sliders below site-rule minimums after the initial load, and isPolicyInEffect was overwritten to false for rules-only flows. Exposes passwordRulesRequest on GeneratorRepository so the processor can cache the parsed constraint on load and re-apply it as floors on each call to validatePasswordOptionsAndApplyPolicies, without affecting isPolicyInEffect (which remains org-policy-only). Also fixes pre-existing test failures introduced in c55da60 where the mock's getEffectivePasswordGenerationOptions bypassed getPasswordGenerationOptions.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-38866
📔 Objective
Integrates the Bitwarden SDK into the AutoFill extension for iOS 26.2+
password generation requests:
AppProcessor.generatePasswordCredential(request:)calls
GeneratorRepository.getEffectivePasswordGenerationOptions(rules:)toapply developer-provided password rules and org policies, then generates a
password or passphrase via the SDK.
VaultCoordinator.showGeneratePassword()extractspassword rules from the extension mode and passes them to the generator view,
so the UI pre-selects options consistent with the site's rules.
GeneratorRepositorygainsgetEffectivePasswordGenerationOptions(rules:)— a single entry point thatmerges SDK-parsed rules with org policy and user preferences, replacing the
scattered hardcoded
PasswordGeneratorRequestplaceholder.