Skip to content

Update Auth to use PasswordPolicyImpl for validation #7451

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

Merged
merged 3 commits into from
Jul 12, 2023
Merged

Update Auth to use PasswordPolicyImpl for validation #7451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e47ba6
Update Auth to use PasswordPolicyImpl for validation and update tests
ch5zzy Jul 12, 2023
c81f7d9
Update API reports
ch5zzy Jul 12, 2023
928a7b5
Rename _getPasswordPolicy to _getPasswordPolicyInternal
ch5zzy Jul 12, 2023
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
2 changes: 1 addition & 1 deletion common/api-review/auth.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ export interface ParsedToken {

// @public
export interface PasswordPolicy {
readonly allowedNonAlphanumericCharacters?: string[];
readonly allowedNonAlphanumericCharacters: string[];
readonly customStrengthOptions: {
readonly minPasswordLength?: number;
readonly maxPasswordLength?: number;
Expand Down
64 changes: 29 additions & 35 deletions packages/auth/src/core/auth/auth_impl.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,64 +790,58 @@ describe('core/auth/auth_impl', () => {
context('passwordPolicy', () => {
const TEST_ALLOWED_NON_ALPHANUMERIC_CHARS = ['!', '(', ')'];
const TEST_MIN_PASSWORD_LENGTH = 6;
const TEST_SCHEMA_VERSION = 1;
const TEST_TENANT_ID = 'tenant-id';
const TEST_TENANT_ID_UNSUPPORTED_POLICY_VERSION =
'tenant-id-unsupported-policy-version';

const passwordPolicyResponse = {
const PASSWORD_POLICY_RESPONSE = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS,
schemaVersion: 1
schemaVersion: TEST_SCHEMA_VERSION
};
const passwordPolicyResponseRequireNumeric = {
const PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH,
containsNumericCharacter: true
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS,
schemaVersion: 1
schemaVersion: TEST_SCHEMA_VERSION
};
const passwordPolicyResponseUnsupportedVersion = {
const PASSWORD_POLICY_RESPONSE_UNSUPPORTED_VERSION = {
customStrengthOptions: {
maxPasswordLength: TEST_MIN_PASSWORD_LENGTH,
unsupportedPasswordPolicyProperty: 10
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS,
schemaVersion: 0
};
const cachedPasswordPolicy = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS
};
const cachedPasswordPolicyRequireNumeric = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH,
containsNumericCharacter: true
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS
};
const CACHED_PASSWORD_POLICY = PASSWORD_POLICY_RESPONSE;
const CACHED_PASSWORD_POLICY_REQUIRE_NUMERIC =
PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC;

beforeEach(async () => {
mockFetch.setUp();
mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{},
passwordPolicyResponse
PASSWORD_POLICY_RESPONSE
);
mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{
tenantId: 'tenant-id'
tenantId: TEST_TENANT_ID
},
passwordPolicyResponseRequireNumeric
PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC
);
mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{
tenantId: 'tenant-id-with-unsupported-policy-version'
tenantId: TEST_TENANT_ID_UNSUPPORTED_POLICY_VERSION
},
passwordPolicyResponseUnsupportedVersion
PASSWORD_POLICY_RESPONSE_UNSUPPORTED_VERSION
);
});

Expand All @@ -860,16 +854,16 @@ describe('core/auth/auth_impl', () => {
auth.tenantId = null;
await auth._updatePasswordPolicy();

expect(auth._getPasswordPolicy()).to.eql(cachedPasswordPolicy);
expect(auth._getPasswordPolicyInternal()).to.eql(CACHED_PASSWORD_POLICY);
});

it('password policy should be set for tenant if tenant ID is not null', async () => {
auth = await testAuth();
auth.tenantId = 'tenant-id';
auth.tenantId = TEST_TENANT_ID;
await auth._updatePasswordPolicy();

expect(auth._getPasswordPolicy()).to.eql(
cachedPasswordPolicyRequireNumeric
expect(auth._getPasswordPolicyInternal()).to.eql(
CACHED_PASSWORD_POLICY_REQUIRE_NUMERIC
);
});

Expand All @@ -878,27 +872,27 @@ describe('core/auth/auth_impl', () => {
auth.tenantId = null;
await auth._updatePasswordPolicy();

auth.tenantId = 'tenant-id';
auth.tenantId = TEST_TENANT_ID;
await auth._updatePasswordPolicy();

auth.tenantId = null;
expect(auth._getPasswordPolicy()).to.eql(cachedPasswordPolicy);
auth.tenantId = 'tenant-id';
expect(auth._getPasswordPolicy()).to.eql(
cachedPasswordPolicyRequireNumeric
expect(auth._getPasswordPolicyInternal()).to.eql(CACHED_PASSWORD_POLICY);
auth.tenantId = TEST_TENANT_ID;
expect(auth._getPasswordPolicyInternal()).to.eql(
CACHED_PASSWORD_POLICY_REQUIRE_NUMERIC
);
auth.tenantId = 'other-tenant-id';
expect(auth._getPasswordPolicy()).to.be.undefined;
expect(auth._getPasswordPolicyInternal()).to.be.undefined;
});

it('password policy should not be set when the schema version is not supported', async () => {
auth = await testAuth();
auth.tenantId = 'tenant-id-with-unsupported-policy-version';
auth.tenantId = TEST_TENANT_ID_UNSUPPORTED_POLICY_VERSION;
await expect(auth._updatePasswordPolicy()).to.be.rejectedWith(
AuthErrorCode.UNSUPPORTED_PASSWORD_POLICY_SCHEMA_VERSION
);

expect(auth._getPasswordPolicy()).to.be.undefined;
expect(auth._getPasswordPolicyInternal()).to.be.undefined;
});
});
});
24 changes: 13 additions & 11 deletions packages/auth/src/core/auth/auth_impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
ErrorFn,
NextFn,
Unsubscribe,
PasswordPolicy,
PasswordValidationStatus
} from '../../model/public_types';
import {
Expand Down Expand Up @@ -70,6 +69,8 @@ import { AuthMiddlewareQueue } from './middleware';
import { RecaptchaConfig } from '../../platform_browser/recaptcha/recaptcha';
import { _logWarn } from '../util/log';
import { _getPasswordPolicy } from '../../api/password_policy/get_password_policy';
import { PasswordPolicyInternal } from '../../model/password_policy';
import { PasswordPolicyImpl } from './password_policy_impl';

interface AsyncAction {
(): Promise<void>;
Expand Down Expand Up @@ -105,8 +106,8 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
_DEFAULT_AUTH_ERROR_FACTORY;
_agentRecaptchaConfig: RecaptchaConfig | null = null;
_tenantRecaptchaConfigs: Record<string, RecaptchaConfig> = {};
_projectPasswordPolicy: PasswordPolicy | null = null;
_tenantPasswordPolicies: Record<string, PasswordPolicy> = {};
_projectPasswordPolicy: PasswordPolicyInternal | null = null;
_tenantPasswordPolicies: Record<string, PasswordPolicyInternal> = {};
readonly name: string;

// Tracks the last notified UID for state change listeners to prevent
Expand Down Expand Up @@ -429,11 +430,14 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
}

async validatePassword(password: string): Promise<PasswordValidationStatus> {
// TODO(chazzy): Implement.
return Promise.reject(password);
if (!this._getPasswordPolicyInternal()) {
await this._updatePasswordPolicy();
}

return this._getPasswordPolicyInternal()!.validatePassword(password);
}

_getPasswordPolicy(): PasswordPolicy | null {
_getPasswordPolicyInternal(): PasswordPolicyInternal | null {
if (this.tenantId === null) {
return this._projectPasswordPolicy;
} else {
Expand All @@ -457,11 +461,9 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
);
}

const passwordPolicy: PasswordPolicy = {
customStrengthOptions: response.customStrengthOptions,
allowedNonAlphanumericCharacters:
response.allowedNonAlphanumericCharacters
};
const passwordPolicy: PasswordPolicyInternal = new PasswordPolicyImpl(
response
);

if (this.tenantId === null) {
this._projectPasswordPolicy = passwordPolicy;
Expand Down
50 changes: 22 additions & 28 deletions packages/auth/src/core/strategies/email_and_password.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,34 +757,24 @@ describe('core/strategies/email_and_password/createUserWithEmailAndPassword', ()
const PASSWORD_ERROR_MSG =
'Firebase: The password does not meet the requirements. (auth/password-does-not-meet-requirements).';

const passwordPolicyResponse = {
const PASSWORD_POLICY_RESPONSE = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS,
schemaVersion: TEST_SCHEMA_VERSION
};
const passwordPolicyResponseRequireNumeric = {
const PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH,
containsNumericCharacter: true
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS,
schemaVersion: TEST_SCHEMA_VERSION
};
const cachedPasswordPolicy = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS
};
const cachedPasswordPolicyRequireNumeric = {
customStrengthOptions: {
minPasswordLength: TEST_MIN_PASSWORD_LENGTH,
containsNumericCharacter: true
},
allowedNonAlphanumericCharacters: TEST_ALLOWED_NON_ALPHANUMERIC_CHARS
};
const CACHED_PASSWORD_POLICY = PASSWORD_POLICY_RESPONSE;
const CACHED_PASSWORD_POLICY_REQUIRE_NUMERIC =
PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC;
let policyEndpointMock: mockFetch.Route;
let policyEndpointMockWithTenant: mockFetch.Route;
let policyEndpointMockWithOtherTenant: mockFetch.Route;
Expand All @@ -793,21 +783,21 @@ describe('core/strategies/email_and_password/createUserWithEmailAndPassword', ()
policyEndpointMock = mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{},
passwordPolicyResponse
PASSWORD_POLICY_RESPONSE
);
policyEndpointMockWithTenant = mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{
tenantId: TEST_TENANT_ID
},
passwordPolicyResponse
PASSWORD_POLICY_RESPONSE
);
policyEndpointMockWithOtherTenant = mockEndpointWithParams(
Endpoint.GET_PASSWORD_POLICY,
{
tenantId: TEST_REQUIRE_NUMERIC_TENANT_ID
},
passwordPolicyResponseRequireNumeric
PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC
);
});

Expand All @@ -817,7 +807,7 @@ describe('core/strategies/email_and_password/createUserWithEmailAndPassword', ()
).to.be.fulfilled;

expect(policyEndpointMock.calls.length).to.eq(0);
expect(auth._getPasswordPolicy()).to.be.null;
expect(auth._getPasswordPolicyInternal()).to.be.null;
});

it('does not update the cached password policy upon successful sign up when there is an existing policy cache', async () => {
Expand All @@ -828,7 +818,7 @@ describe('core/strategies/email_and_password/createUserWithEmailAndPassword', ()
).to.be.fulfilled;

expect(policyEndpointMock.calls.length).to.eq(1);
expect(auth._getPasswordPolicy()).to.eql(cachedPasswordPolicy);
expect(auth._getPasswordPolicyInternal()).to.eql(CACHED_PASSWORD_POLICY);
});

context('handles password validation errors', () => {
Expand All @@ -848,43 +838,47 @@ describe('core/strategies/email_and_password/createUserWithEmailAndPassword', ()
it('updates the cached password policy when password does not meet backend requirements', async () => {
await auth._updatePasswordPolicy();
expect(policyEndpointMock.calls.length).to.eq(1);
expect(auth._getPasswordPolicy()).to.eql(cachedPasswordPolicy);
expect(auth._getPasswordPolicyInternal()).to.eql(
CACHED_PASSWORD_POLICY
);

// Password policy changed after previous fetch.
policyEndpointMock.response = passwordPolicyResponseRequireNumeric;
policyEndpointMock.response = PASSWORD_POLICY_RESPONSE_REQUIRE_NUMERIC;
await expect(
createUserWithEmailAndPassword(auth, 'some-email', 'some-password')
).to.be.rejectedWith(FirebaseError, PASSWORD_ERROR_MSG);

expect(policyEndpointMock.calls.length).to.eq(2);
expect(auth._getPasswordPolicy()).to.eql(
cachedPasswordPolicyRequireNumeric
expect(auth._getPasswordPolicyInternal()).to.eql(
CACHED_PASSWORD_POLICY_REQUIRE_NUMERIC
);
});

it('does not update the cached password policy upon error if policy has not previously been fetched', async () => {
expect(auth._getPasswordPolicy()).to.be.null;
expect(auth._getPasswordPolicyInternal()).to.be.null;

await expect(
createUserWithEmailAndPassword(auth, 'some-email', 'some-password')
).to.be.rejectedWith(FirebaseError, PASSWORD_ERROR_MSG);

expect(policyEndpointMock.calls.length).to.eq(0);
expect(auth._getPasswordPolicy()).to.be.null;
expect(auth._getPasswordPolicyInternal()).to.be.null;
});

it('does not update the cached password policy upon error if tenant changes and policy has not previously been fetched', async () => {
auth.tenantId = TEST_TENANT_ID;
await auth._updatePasswordPolicy();
expect(policyEndpointMockWithTenant.calls.length).to.eq(1);
expect(auth._getPasswordPolicy()).to.eql(cachedPasswordPolicy);
expect(auth._getPasswordPolicyInternal()).to.eql(
CACHED_PASSWORD_POLICY
);

auth.tenantId = TEST_REQUIRE_NUMERIC_TENANT_ID;
await expect(
createUserWithEmailAndPassword(auth, 'some-email', 'some-password')
).to.be.rejectedWith(FirebaseError, PASSWORD_ERROR_MSG);
expect(policyEndpointMockWithOtherTenant.calls.length).to.eq(0);
expect(auth._getPasswordPolicy()).to.be.undefined;
expect(auth._getPasswordPolicyInternal()).to.be.undefined;
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/auth/src/core/strategies/email_and_password.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ export async function createUserWithEmailAndPassword(
if (
error.code ===
`auth/${AuthErrorCode.PASSWORD_DOES_NOT_MEET_REQUIREMENTS}` &&
authInternal._getPasswordPolicy()
authInternal._getPasswordPolicyInternal()
) {
await authInternal._updatePasswordPolicy();
}
Expand Down
3 changes: 2 additions & 1 deletion packages/auth/src/model/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { PopupRedirectResolverInternal } from './popup_redirect';
import { UserInternal } from './user';
import { ClientPlatform } from '../core/util/version';
import { RecaptchaConfig } from '../platform_browser/recaptcha/recaptcha';
import { PasswordPolicyInternal } from './password_policy';

export type AppName = string;
export type ApiKey = string;
Expand Down Expand Up @@ -87,7 +88,7 @@ export interface AuthInternal extends Auth {
_stopProactiveRefresh(): void;
_getPersistence(): string;
_getRecaptchaConfig(): RecaptchaConfig | null;
_getPasswordPolicy(): PasswordPolicy | null;
_getPasswordPolicyInternal(): PasswordPolicyInternal | null;
_updatePasswordPolicy(): Promise<void>;
_logFramework(framework: string): void;
_getFrameworks(): readonly string[];
Expand Down