1
0
mirror of https://github.com/bitwarden/browser.git synced 2024-06-30 11:15:36 +02:00

[PM-5608] introduce passphrase generator strategy (#7690)

This commit is contained in:
✨ Audrey ✨ 2024-02-02 10:49:38 -05:00 committed by GitHub
parent 4e4e39e9f7
commit e8d0d56c5f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 292 additions and 56 deletions

View File

@ -1,5 +1,6 @@
import { GENERATOR_DISK, GENERATOR_MEMORY, KeyDefinition } from "../../platform/state";
import { PassphraseGenerationOptions } from "./passphrase/passphrase-generation-options";
import { GeneratedPasswordHistory } from "./password/generated-password-history";
import { PasswordGenerationOptions } from "./password/password-generation-options";
@ -13,7 +14,7 @@ export const PASSWORD_SETTINGS = new KeyDefinition<PasswordGenerationOptions>(
);
/** plaintext passphrase generation options */
export const PASSPHRASE_SETTINGS = new KeyDefinition<PasswordGenerationOptions>(
export const PASSPHRASE_SETTINGS = new KeyDefinition<PassphraseGenerationOptions>(
GENERATOR_DISK,
"passphraseGeneratorSettings",
{

View File

@ -0,0 +1,4 @@
// password generator "v2" interfaces
export { PassphraseGeneratorOptionsEvaluator } from "./passphrase-generator-options-evaluator";
export { PassphraseGeneratorPolicy } from "./passphrase-generator-policy";
export { PassphraseGeneratorStrategy } from "./passphrase-generator-strategy";

View File

@ -0,0 +1,26 @@
/** Request format for passphrase credential generation.
* The members of this type may be `undefined` when the user is
* generating a password.
*/
export type PassphraseGenerationOptions = {
/** The number of words to include in the passphrase.
* This value defaults to 4.
*/
numWords?: number;
/** The ASCII separator character to use between words in the passphrase.
* This value defaults to a dash.
* If multiple characters appear in the string, only the first character is used.
*/
wordSeparator?: string;
/** `true` when the first character of every word should be capitalized.
* This value defaults to `false`.
*/
capitalize?: boolean;
/** `true` when a number should be included in the passphrase.
* This value defaults to `false`.
*/
includeNumber?: boolean;
};

View File

@ -1,16 +1,19 @@
import { PasswordGeneratorPolicyOptions } from "../../../admin-console/models/domain/password-generator-policy-options";
/**
* include structuredClone in test environment.
* @jest-environment ../../../../shared/test.environment.ts
*/
import { PassphraseGenerationOptions } from "./passphrase-generation-options";
import {
DefaultBoundaries,
PassphraseGeneratorOptionsEvaluator,
} from "./passphrase-generator-options-evaluator";
import { PassphraseGenerationOptions } from "./password-generator-options";
import { DisabledPassphraseGeneratorPolicy } from "./passphrase-generator-policy";
describe("Password generator options builder", () => {
describe("constructor()", () => {
it("should set the policy object to a copy of the input policy", () => {
const policy = new PasswordGeneratorPolicyOptions();
policy.minLength = 10; // arbitrary change for deep equality check
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.minNumberWords = 10; // arbitrary change for deep equality check
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
@ -19,7 +22,7 @@ describe("Password generator options builder", () => {
});
it("should set default boundaries when a default policy is used", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
expect(builder.numWords).toEqual(DefaultBoundaries.numWords);
@ -28,7 +31,7 @@ describe("Password generator options builder", () => {
it.each([1, 2])(
"should use the default word boundaries when they are greater than `policy.minNumberWords` (= %i)",
(minNumberWords) => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.minNumberWords = minNumberWords;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
@ -40,7 +43,7 @@ describe("Password generator options builder", () => {
it.each([8, 12, 18])(
"should use `policy.minNumberWords` (= %i) when it is greater than the default minimum words",
(minNumberWords) => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.minNumberWords = minNumberWords;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
@ -53,7 +56,7 @@ describe("Password generator options builder", () => {
it.each([150, 300, 9000])(
"should use `policy.minNumberWords` (= %i) when it is greater than the default boundaries",
(minNumberWords) => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.minNumberWords = minNumberWords;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
@ -64,11 +67,44 @@ describe("Password generator options builder", () => {
);
});
describe("policyInEffect", () => {
it("should return false when the policy has no effect", () => {
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
expect(builder.policyInEffect).toEqual(false);
});
it("should return true when the policy has a numWords greater than the default boundary", () => {
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.minNumberWords = DefaultBoundaries.numWords.min + 1;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
expect(builder.policyInEffect).toEqual(true);
});
it("should return true when the policy has capitalize enabled", () => {
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.capitalize = true;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
expect(builder.policyInEffect).toEqual(true);
});
it("should return true when the policy has includeNumber enabled", () => {
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.includeNumber = true;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
expect(builder.policyInEffect).toEqual(true);
});
});
describe("applyPolicy(options)", () => {
// All tests should freeze the options to ensure they are not modified
it("should set `capitalize` to `false` when the policy does not override it", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({});
@ -78,7 +114,7 @@ describe("Password generator options builder", () => {
});
it("should set `capitalize` to `true` when the policy overrides it", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.capitalize = true;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ capitalize: false });
@ -89,7 +125,7 @@ describe("Password generator options builder", () => {
});
it("should set `includeNumber` to false when the policy does not override it", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({});
@ -99,7 +135,7 @@ describe("Password generator options builder", () => {
});
it("should set `includeNumber` to true when the policy overrides it", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
policy.includeNumber = true;
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ includeNumber: false });
@ -110,7 +146,7 @@ describe("Password generator options builder", () => {
});
it("should set `numWords` to the minimum value when it isn't supplied", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({});
@ -124,7 +160,7 @@ describe("Password generator options builder", () => {
(numWords) => {
expect(numWords).toBeLessThan(DefaultBoundaries.numWords.min);
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ numWords });
@ -140,7 +176,7 @@ describe("Password generator options builder", () => {
expect(numWords).toBeGreaterThanOrEqual(DefaultBoundaries.numWords.min);
expect(numWords).toBeLessThanOrEqual(DefaultBoundaries.numWords.max);
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ numWords });
@ -155,7 +191,7 @@ describe("Password generator options builder", () => {
(numWords) => {
expect(numWords).toBeGreaterThan(DefaultBoundaries.numWords.max);
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ numWords });
@ -166,7 +202,7 @@ describe("Password generator options builder", () => {
);
it("should preserve unknown properties", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({
unknown: "property",
@ -184,7 +220,7 @@ describe("Password generator options builder", () => {
// All tests should freeze the options to ensure they are not modified
it("should return the input options without altering them", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({ wordSeparator: "%" });
@ -194,7 +230,7 @@ describe("Password generator options builder", () => {
});
it("should set `wordSeparator` to '-' when it isn't supplied and there is no policy override", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({});
@ -204,7 +240,7 @@ describe("Password generator options builder", () => {
});
it("should preserve unknown properties", () => {
const policy = new PasswordGeneratorPolicyOptions();
const policy = Object.assign({}, DisabledPassphraseGeneratorPolicy);
const builder = new PassphraseGeneratorOptionsEvaluator(policy);
const options = Object.freeze({
unknown: "property",

View File

@ -1,6 +1,7 @@
import { PasswordGeneratorPolicyOptions } from "../../../admin-console/models/domain/password-generator-policy-options";
import { PolicyEvaluator } from "../abstractions/policy-evaluator.abstraction";
import { PassphraseGenerationOptions } from "./password-generator-options";
import { PassphraseGenerationOptions } from "./passphrase-generation-options";
import { PassphraseGeneratorPolicy } from "./passphrase-generator-policy";
type Boundary = {
readonly min: number;
@ -25,7 +26,9 @@ export const DefaultBoundaries = initializeBoundaries();
/** Enforces policy for passphrase generation options.
*/
export class PassphraseGeneratorOptionsEvaluator {
export class PassphraseGeneratorOptionsEvaluator
implements PolicyEvaluator<PassphraseGeneratorPolicy, PassphraseGenerationOptions>
{
// This design is not ideal, but it is a step towards a more robust passphrase
// generator. Ideally, `sanitize` would be implemented on an options class,
// and `applyPolicy` would be implemented on a policy class, "mise en place".
@ -36,7 +39,7 @@ export class PassphraseGeneratorOptionsEvaluator {
/** Policy applied by the evaluator.
*/
readonly policy: PasswordGeneratorPolicyOptions;
readonly policy: PassphraseGeneratorPolicy;
/** Boundaries for the number of words allowed in the password.
*/
@ -46,7 +49,7 @@ export class PassphraseGeneratorOptionsEvaluator {
* @param policy The policy applied by the evaluator. When this conflicts with
* the defaults, the policy takes precedence.
*/
constructor(policy: PasswordGeneratorPolicyOptions) {
constructor(policy: PassphraseGeneratorPolicy) {
function createBoundary(value: number, defaultBoundary: Boundary): Boundary {
const boundary = {
min: Math.max(defaultBoundary.min, value),
@ -56,10 +59,21 @@ export class PassphraseGeneratorOptionsEvaluator {
return boundary;
}
this.policy = policy.clone();
this.policy = structuredClone(policy);
this.numWords = createBoundary(policy.minNumberWords, DefaultBoundaries.numWords);
}
/** {@link PolicyEvaluator.policyInEffect} */
get policyInEffect(): boolean {
const policies = [
this.policy.capitalize,
this.policy.includeNumber,
this.policy.minNumberWords > DefaultBoundaries.numWords.min,
];
return policies.includes(true);
}
/** Apply policy to the input options.
* @param options The options to build from. These options are not altered.
* @returns A new password generation request with policy applied.

View File

@ -0,0 +1,13 @@
/** Policy options enforced during passphrase generation. */
export type PassphraseGeneratorPolicy = {
minNumberWords: number;
capitalize: boolean;
includeNumber: boolean;
};
/** The default options for password generation policy. */
export const DisabledPassphraseGeneratorPolicy: PassphraseGeneratorPolicy = Object.freeze({
minNumberWords: 0,
capitalize: false,
includeNumber: false,
});

View File

@ -0,0 +1,102 @@
/**
* include structuredClone in test environment.
* @jest-environment ../../../../shared/test.environment.ts
*/
import { mock } from "jest-mock-extended";
import { PolicyType } from "../../../admin-console/enums";
// FIXME: use index.ts imports once policy abstractions and models
// implement ADR-0002
import { Policy } from "../../../admin-console/models/domain/policy";
import { PASSPHRASE_SETTINGS } from "../key-definitions";
import { PasswordGenerationServiceAbstraction } from "../password/password-generation.service.abstraction";
import { PassphraseGeneratorOptionsEvaluator, PassphraseGeneratorStrategy } from ".";
describe("Password generation strategy", () => {
describe("evaluator()", () => {
it("should throw if the policy type is incorrect", () => {
const strategy = new PassphraseGeneratorStrategy(null);
const policy = mock<Policy>({
type: PolicyType.DisableSend,
});
expect(() => strategy.evaluator(policy)).toThrow(new RegExp("Mismatched policy type\\. .+"));
});
it("should map to the policy evaluator", () => {
const strategy = new PassphraseGeneratorStrategy(null);
const policy = mock<Policy>({
type: PolicyType.PasswordGenerator,
data: {
minNumberWords: 10,
capitalize: true,
includeNumber: true,
},
});
const evaluator = strategy.evaluator(policy);
expect(evaluator).toBeInstanceOf(PassphraseGeneratorOptionsEvaluator);
expect(evaluator.policy).toMatchObject({
minNumberWords: 10,
capitalize: true,
includeNumber: true,
});
});
});
describe("disk", () => {
it("should use password settings key", () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PassphraseGeneratorStrategy(legacy);
expect(strategy.disk).toBe(PASSPHRASE_SETTINGS);
});
});
describe("cache_ms", () => {
it("should be a positive non-zero number", () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PassphraseGeneratorStrategy(legacy);
expect(strategy.cache_ms).toBeGreaterThan(0);
});
});
describe("policy", () => {
it("should use password generator policy", () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PassphraseGeneratorStrategy(legacy);
expect(strategy.policy).toBe(PolicyType.PasswordGenerator);
});
});
describe("generate()", () => {
it("should call the legacy service with the given options", async () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PassphraseGeneratorStrategy(legacy);
const options = {
type: "passphrase",
minNumberWords: 1,
capitalize: true,
includeNumber: true,
};
await strategy.generate(options);
expect(legacy.generatePassphrase).toHaveBeenCalledWith(options);
});
it("should set the generation type to passphrase", async () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PassphraseGeneratorStrategy(legacy);
await strategy.generate({ type: "foo" } as any);
expect(legacy.generatePassphrase).toHaveBeenCalledWith({ type: "passphrase" });
});
});
});

View File

@ -0,0 +1,56 @@
import { GeneratorStrategy } from "..";
import { PolicyType } from "../../../admin-console/enums";
// FIXME: use index.ts imports once policy abstractions and models
// implement ADR-0002
import { Policy } from "../../../admin-console/models/domain/policy";
import { PASSPHRASE_SETTINGS } from "../key-definitions";
import { PasswordGenerationServiceAbstraction } from "../password/password-generation.service.abstraction";
import { PassphraseGenerationOptions } from "./passphrase-generation-options";
import { PassphraseGeneratorOptionsEvaluator } from "./passphrase-generator-options-evaluator";
import { PassphraseGeneratorPolicy } from "./passphrase-generator-policy";
const ONE_MINUTE = 60 * 1000;
/** {@link GeneratorStrategy} */
export class PassphraseGeneratorStrategy
implements GeneratorStrategy<PassphraseGenerationOptions, PassphraseGeneratorPolicy>
{
/** instantiates the password generator strategy.
* @param legacy generates the passphrase
*/
constructor(private legacy: PasswordGenerationServiceAbstraction) {}
/** {@link GeneratorStrategy.disk} */
get disk() {
return PASSPHRASE_SETTINGS;
}
/** {@link GeneratorStrategy.policy} */
get policy() {
return PolicyType.PasswordGenerator;
}
get cache_ms() {
return ONE_MINUTE;
}
/** {@link GeneratorStrategy.evaluator} */
evaluator(policy: Policy): PassphraseGeneratorOptionsEvaluator {
if (policy.type !== this.policy) {
const details = `Expected: ${this.policy}. Received: ${policy.type}`;
throw Error("Mismatched policy type. " + details);
}
return new PassphraseGeneratorOptionsEvaluator({
minNumberWords: policy.data.minNumberWords,
capitalize: policy.data.capitalize,
includeNumber: policy.data.includeNumber,
});
}
/** {@link GeneratorStrategy.generate} */
generate(options: PassphraseGenerationOptions): Promise<string> {
return this.legacy.generatePassphrase({ ...options, type: "passphrase" });
}
}

View File

@ -5,9 +5,9 @@ import { CryptoService } from "../../../platform/abstractions/crypto.service";
import { StateService } from "../../../platform/abstractions/state.service";
import { EFFLongWordList } from "../../../platform/misc/wordlist";
import { EncString } from "../../../platform/models/domain/enc-string";
import { PassphraseGeneratorOptionsEvaluator } from "../passphrase/passphrase-generator-options-evaluator";
import { GeneratedPasswordHistory } from "./generated-password-history";
import { PassphraseGeneratorOptionsEvaluator } from "./passphrase-generator-options-evaluator";
import { PasswordGenerationServiceAbstraction } from "./password-generation.service.abstraction";
import { PasswordGeneratorOptions } from "./password-generator-options";
import { PasswordGeneratorOptionsEvaluator } from "./password-generator-options-evaluator";

View File

@ -1,3 +1,5 @@
import { PassphraseGenerationOptions } from "../passphrase/passphrase-generation-options";
import { PasswordGenerationOptions } from "./password-generation-options";
/** Request format for credential generation.
@ -13,30 +15,3 @@ export type PasswordGeneratorOptions = PasswordGenerationOptions &
*/
type?: "password" | "passphrase";
};
/** Request format for passphrase credential generation.
* The members of this type may be `undefined` when the user is
* generating a password.
*/
export type PassphraseGenerationOptions = {
/** The number of words to include in the passphrase.
* This value defaults to 4.
*/
numWords?: number;
/** The ASCII separator character to use between words in the passphrase.
* This value defaults to a dash.
* If multiple characters appear in the string, only the first character is used.
*/
wordSeparator?: string;
/** `true` when the first character of every word should be capitalized.
* This value defaults to `false`.
*/
capitalize?: boolean;
/** `true` when a number should be included in the passphrase.
* This value defaults to `false`.
*/
includeNumber?: boolean;
};

View File

@ -67,6 +67,15 @@ describe("Password generation strategy", () => {
});
});
describe("cache_ms", () => {
it("should be a positive non-zero number", () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();
const strategy = new PasswordGeneratorStrategy(legacy);
expect(strategy.cache_ms).toBeGreaterThan(0);
});
});
describe("policy", () => {
it("should use password generator policy", () => {
const legacy = mock<PasswordGenerationServiceAbstraction>();