-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(auth): cache password policy per-app in validatePassword #8779
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,269 @@ | ||
| /* | ||
| * Copyright (c) 2016-present Invertase Limited & Contributors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this library except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ | ||
|
|
||
| import { jest, describe, it, expect, beforeEach } from '@jest/globals'; | ||
|
|
||
| const mockPasswordPolicy = { | ||
| schemaVersion: 1, | ||
| customStrengthOptions: { | ||
| minPasswordLength: 8, | ||
| maxPasswordLength: 100, | ||
| containsLowercaseCharacter: true, | ||
| containsUppercaseCharacter: true, | ||
| containsNumericCharacter: true, | ||
| containsNonAlphanumericCharacter: true, | ||
| }, | ||
| allowedNonAlphanumericCharacters: ['!', '@', '#', '$', '%'], | ||
| enforcementState: 'ENFORCE', | ||
| }; | ||
|
|
||
| const mockFetchPasswordPolicy = jest.fn().mockResolvedValue(mockPasswordPolicy); | ||
|
|
||
| jest.unstable_mockModule('../lib/password-policy/passwordPolicyApi', () => ({ | ||
| fetchPasswordPolicy: mockFetchPasswordPolicy, | ||
| })); | ||
|
|
||
| describe('validatePassword', () => { | ||
| let validatePassword; | ||
| let mockAuth; | ||
|
|
||
| beforeEach(async () => { | ||
| jest.resetModules(); | ||
|
|
||
| mockFetchPasswordPolicy.mockClear(); | ||
|
Comment on lines
+45
to
+47
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these might fit better in an afterEach? |
||
| mockFetchPasswordPolicy.mockResolvedValue(mockPasswordPolicy); | ||
|
|
||
| jest.unstable_mockModule('../lib/password-policy/passwordPolicyApi', () => ({ | ||
| fetchPasswordPolicy: mockFetchPasswordPolicy, | ||
| })); | ||
|
|
||
| const modular = await import('../lib/modular/index.js'); | ||
| validatePassword = modular.validatePassword; | ||
|
|
||
| // Create a mock auth instance that mimics FirebaseAuthModule | ||
| mockAuth = { | ||
| app: { | ||
| name: '[DEFAULT]', | ||
| options: { apiKey: 'test-api-key-default' }, | ||
| }, | ||
| _tenantId: null, | ||
| _projectPasswordPolicy: null, | ||
| _tenantPasswordPolicies: {}, | ||
| }; | ||
|
|
||
| const { PasswordPolicyImpl } = await import('../lib/password-policy/PasswordPolicyImpl.js'); | ||
|
|
||
| mockAuth._getPasswordPolicyInternal = function () { | ||
| if (this._tenantId === null) { | ||
| return this._projectPasswordPolicy; | ||
| } | ||
| return this._tenantPasswordPolicies[this._tenantId]; | ||
| }; | ||
|
|
||
| mockAuth._updatePasswordPolicy = async function () { | ||
| const response = await mockFetchPasswordPolicy(this); | ||
| const passwordPolicy = new PasswordPolicyImpl(response); | ||
| if (this._tenantId === null) { | ||
| this._projectPasswordPolicy = passwordPolicy; | ||
| } else { | ||
| this._tenantPasswordPolicies[this._tenantId] = passwordPolicy; | ||
| } | ||
| }; | ||
|
|
||
| mockAuth._recachePasswordPolicy = async function () { | ||
| if (this._getPasswordPolicyInternal()) { | ||
| await this._updatePasswordPolicy(); | ||
| } | ||
| }; | ||
|
|
||
| mockAuth.validatePassword = async function (password) { | ||
| if (!this._getPasswordPolicyInternal()) { | ||
| await this._updatePasswordPolicy(); | ||
| } | ||
| const passwordPolicy = this._getPasswordPolicyInternal(); | ||
|
|
||
| if (passwordPolicy.schemaVersion !== 1) { | ||
| throw new Error( | ||
| 'auth/unsupported-password-policy-schema-version: The password policy received from the backend uses a schema version that is not supported by this version of the SDK.', | ||
| ); | ||
| } | ||
|
|
||
| return passwordPolicy.validatePassword(password); | ||
| }; | ||
| }); | ||
|
|
||
| describe('caching behavior', () => { | ||
| it('should fetch password policy on first call', async () => { | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
|
|
||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledWith(mockAuth); | ||
| }); | ||
|
|
||
| it('should use cached policy on subsequent calls for same auth instance', async () => { | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| await validatePassword(mockAuth, 'AnotherPassword1!'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| await validatePassword(mockAuth, 'YetAnother1@'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should cache at project level when tenantId is null', async () => { | ||
| mockAuth._tenantId = null; | ||
|
|
||
| await validatePassword(mockAuth, 'Password123$'); | ||
|
|
||
| expect(mockAuth._projectPasswordPolicy).not.toBeNull(); | ||
| expect(Object.keys(mockAuth._tenantPasswordPolicies).length).toBe(0); | ||
| }); | ||
|
|
||
| it('should cache separately per tenant', async () => { | ||
| // First tenant | ||
| mockAuth._tenantId = 'tenant-1'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Same tenant should use cache | ||
| await validatePassword(mockAuth, 'AnotherPassword1!'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Different tenant should fetch again | ||
| mockAuth._tenantId = 'tenant-2'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
|
|
||
| // Back to first tenant should use its cache | ||
| mockAuth._tenantId = 'tenant-1'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
|
|
||
| // Verify both tenant policies are cached | ||
| expect(mockAuth._tenantPasswordPolicies['tenant-1']).toBeDefined(); | ||
| expect(mockAuth._tenantPasswordPolicies['tenant-2']).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should keep project and tenant caches separate', async () => { | ||
| // Project level (no tenant) | ||
| mockAuth._tenantId = null; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Tenant level | ||
| mockAuth._tenantId = 'tenant-1'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
|
|
||
| // Back to project level should use project cache | ||
| mockAuth._tenantId = null; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
|
|
||
| // Verify both caches exist | ||
| expect(mockAuth._projectPasswordPolicy).not.toBeNull(); | ||
| expect(mockAuth._tenantPasswordPolicies['tenant-1']).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should return correct validation status using cached policy', async () => { | ||
| const status1 = await validatePassword(mockAuth, 'Password123$'); | ||
| expect(status1.isValid).toBe(true); | ||
|
|
||
| const status2 = await validatePassword(mockAuth, 'weak'); | ||
| expect(status2.isValid).toBe(false); | ||
|
|
||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('schema validation', () => { | ||
| it('should throw error on unsupported schema version', async () => { | ||
| const unsupportedPolicy = { | ||
| ...mockPasswordPolicy, | ||
| schemaVersion: 2, | ||
| }; | ||
| mockFetchPasswordPolicy.mockResolvedValueOnce(unsupportedPolicy); | ||
|
|
||
| await expect(validatePassword(mockAuth, 'Password123$')).rejects.toThrow( | ||
| 'auth/unsupported-password-policy-schema-version', | ||
| ); | ||
| }); | ||
|
|
||
| it('should accept schema version 1', async () => { | ||
| const validPolicy = { | ||
| ...mockPasswordPolicy, | ||
| schemaVersion: 1, | ||
| }; | ||
| mockFetchPasswordPolicy.mockResolvedValueOnce(validPolicy); | ||
|
|
||
| const status = await validatePassword(mockAuth, 'Password123$'); | ||
| expect(status.isValid).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('cache invalidation', () => { | ||
| it('should refresh cache when _recachePasswordPolicy is called with existing cache', async () => { | ||
| // First call caches the policy | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Simulate cache invalidation | ||
| await mockAuth._recachePasswordPolicy(); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('should not fetch when _recachePasswordPolicy is called without existing cache', async () => { | ||
| // No prior validation, so no cache exists | ||
| await mockAuth._recachePasswordPolicy(); | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('should refresh correct tenant cache on invalidation', async () => { | ||
| // Cache for tenant-1 | ||
| mockAuth._tenantId = 'tenant-1'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
|
|
||
| // Cache for tenant-2 | ||
| mockAuth._tenantId = 'tenant-2'; | ||
| await validatePassword(mockAuth, 'Password123$'); | ||
|
|
||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(2); | ||
|
|
||
| // Invalidate tenant-1 cache | ||
| mockAuth._tenantId = 'tenant-1'; | ||
| await mockAuth._recachePasswordPolicy(); | ||
|
|
||
| // Should have fetched again for tenant-1 | ||
| expect(mockFetchPasswordPolicy).toHaveBeenCalledTimes(3); | ||
| }); | ||
| }); | ||
|
|
||
| describe('input validation', () => { | ||
| it('should throw error for null password', async () => { | ||
| await expect(validatePassword(mockAuth, null)).rejects.toThrow( | ||
| "firebase.auth().validatePassword(*) expected 'password' to be a non-null or a defined value.", | ||
| ); | ||
| }); | ||
|
|
||
| it('should throw error for undefined password', async () => { | ||
| await expect(validatePassword(mockAuth, undefined)).rejects.toThrow( | ||
| "firebase.auth().validatePassword(*) expected 'password' to be a non-null or a defined value.", | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,10 @@ import TwitterAuthProvider from './providers/TwitterAuthProvider'; | |
| import { TotpSecret } from './TotpSecret'; | ||
| import version from './version'; | ||
| import fallBackModule from './web/RNFBAuthModule'; | ||
| import { fetchPasswordPolicy } from './password-policy/passwordPolicyApi'; | ||
| import { PasswordPolicyImpl } from './password-policy/PasswordPolicyImpl'; | ||
|
|
||
| const EXPECTED_PASSWORD_POLICY_SCHEMA_VERSION = 1; | ||
|
|
||
| const PhoneAuthState = { | ||
| CODE_SENT: 'sent', | ||
|
|
@@ -103,6 +107,8 @@ class FirebaseAuthModule extends FirebaseModule { | |
| this._authResult = false; | ||
| this._languageCode = this.native.APP_LANGUAGE[this.app._name]; | ||
| this._tenantId = null; | ||
| this._projectPasswordPolicy = null; | ||
| this._tenantPasswordPolicies = {}; | ||
|
|
||
| if (!this.languageCode) { | ||
| this._languageCode = this.native.APP_LANGUAGE['[DEFAULT]']; | ||
|
|
@@ -338,13 +344,37 @@ class FirebaseAuthModule extends FirebaseModule { | |
| createUserWithEmailAndPassword(email, password) { | ||
| return this.native | ||
| .createUserWithEmailAndPassword(email, password) | ||
| .then(userCredential => this._setUserCredential(userCredential)); | ||
| .then(userCredential => this._setUserCredential(userCredential)) | ||
| .catch(error => { | ||
| if (error.code === 'auth/password-does-not-meet-requirements') { | ||
| return this._recachePasswordPolicy() | ||
| .catch(() => { | ||
| // Silently ignore recache failures - the original error matters more | ||
| }) | ||
| .then(() => { | ||
| throw error; | ||
| }); | ||
| } | ||
| throw error; | ||
| }); | ||
| } | ||
|
|
||
| signInWithEmailAndPassword(email, password) { | ||
| return this.native | ||
| .signInWithEmailAndPassword(email, password) | ||
| .then(userCredential => this._setUserCredential(userCredential)); | ||
| .then(userCredential => this._setUserCredential(userCredential)) | ||
| .catch(error => { | ||
| if (error.code === 'auth/password-does-not-meet-requirements') { | ||
| return this._recachePasswordPolicy() | ||
| .catch(() => { | ||
| // Silently ignore recache failures - the original error matters more | ||
| }) | ||
| .then(() => { | ||
| throw error; | ||
| }); | ||
| } | ||
| throw error; | ||
| }); | ||
|
Comment on lines
+366
to
+377
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the triple repetition of this recaching catch block distasteful but it is exactly how firebase-js-sdk did it and there is a value in that. I bet you had the same thought. Best to leave it, but I'm surprised they did that upstream |
||
| } | ||
|
|
||
| signInWithCustomToken(customToken) { | ||
|
|
@@ -382,7 +412,18 @@ class FirebaseAuthModule extends FirebaseModule { | |
| } | ||
|
|
||
| confirmPasswordReset(code, newPassword) { | ||
| return this.native.confirmPasswordReset(code, newPassword); | ||
| return this.native.confirmPasswordReset(code, newPassword).catch(error => { | ||
| if (error.code === 'auth/password-does-not-meet-requirements') { | ||
| return this._recachePasswordPolicy() | ||
| .catch(() => { | ||
| // Silently ignore recache failures - the original error matters more | ||
| }) | ||
| .then(() => { | ||
| throw error; | ||
| }); | ||
| } | ||
| throw error; | ||
| }); | ||
| } | ||
|
|
||
| applyActionCode(code) { | ||
|
|
@@ -491,6 +532,44 @@ class FirebaseAuthModule extends FirebaseModule { | |
| getCustomAuthDomain() { | ||
| return this.native.getCustomAuthDomain(); | ||
| } | ||
|
|
||
| _getPasswordPolicyInternal() { | ||
| if (this._tenantId === null) { | ||
| return this._projectPasswordPolicy; | ||
| } | ||
| return this._tenantPasswordPolicies[this._tenantId]; | ||
| } | ||
|
|
||
| async _updatePasswordPolicy() { | ||
| const response = await fetchPasswordPolicy(this); | ||
| const passwordPolicy = new PasswordPolicyImpl(response); | ||
| if (this._tenantId === null) { | ||
| this._projectPasswordPolicy = passwordPolicy; | ||
| } else { | ||
| this._tenantPasswordPolicies[this._tenantId] = passwordPolicy; | ||
| } | ||
| } | ||
|
|
||
| async _recachePasswordPolicy() { | ||
| if (this._getPasswordPolicyInternal()) { | ||
| await this._updatePasswordPolicy(); | ||
| } | ||
| } | ||
|
|
||
| async validatePassword(password) { | ||
| if (!this._getPasswordPolicyInternal()) { | ||
| await this._updatePasswordPolicy(); | ||
| } | ||
| const passwordPolicy = this._getPasswordPolicyInternal(); | ||
|
|
||
| if (passwordPolicy.schemaVersion !== EXPECTED_PASSWORD_POLICY_SCHEMA_VERSION) { | ||
| throw new Error( | ||
| 'auth/unsupported-password-policy-schema-version: The password policy received from the backend uses a schema version that is not supported by this version of the SDK.', | ||
| ); | ||
| } | ||
|
|
||
| return passwordPolicy.validatePassword(password); | ||
| } | ||
| } | ||
|
|
||
| // import { SDK_VERSION } from '@react-native-firebase/auth'; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are in the beforeEach, so why do them here as well? 🤔