-
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?
feat(auth): cache password policy per-app in validatePassword #8779
Conversation
Cache the fetched password policy per Firebase app to avoid redundant API calls to the identity toolkit endpoint.
|
@gyula-s is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
|
🤔 Hmmm - I really appreciate the effort here, the tests are great as well. I'm just not sure we can make caching decisions like this for library consumers. I believe this would fit well at the app level as a React useState var set via useEffect in the component where passwords were set. I am just not sure there is any caching policy we can pick that will be correct in 100% of the consuming app use cases |
I respectfully disagree - caching is important here, otherwise it will (and currently does) spam the server on every keystroke during password validation on mobile, which breaks the user experience. The Firebase JS SDK actually implements this same caching pattern, but more robustly:
This is a "fetch once, validate locally, refresh on failure" pattern - no TTL, no complexity, just pragmatic caching that ensures the policy is always fresh when it matters. I'm currently working on an implementation that mimics the JS SDK much more closely, including tenant support and reactive cache invalidation. |
Refactor validatePassword to use instance-level caching with tenant
support and reactive cache invalidation, matching the Firebase JS SDK
implementation.
Changes:
- Move cache from module-level to Auth instance (_projectPasswordPolicy,
_tenantPasswordPolicies) for proper multi-app and multi-tenant support
- Add reactive cache invalidation on PASSWORD_DOES_NOT_MEET_REQUIREMENTS
errors in createUserWithEmailAndPassword, signInWithEmailAndPassword,
and confirmPasswordReset
- Add schema version validation (rejects unsupported versions)
- Delegate modular API validatePassword to auth instance
This ensures the password policy is:
- Fetched once per tenant/project (lazy loading)
- Validated locally without server calls
- Automatically refreshed when the server rejects a password
Matches: https://github.com/firebase/firebase-js-sdk/blob/main/packages/auth/src/core/auth/auth_impl.ts
|
PR is now updated @mikehardy |
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.
I respectfully disagree - caching is important here, otherwise it will (and currently does) spam the server on every keystroke during password validation on mobile, which breaks the user experience.
I do agree with the need, my question was just where to cache, and how to do so without making a hidden policy decision on behalf of API consumers
Cache is only refreshed reactively when PASSWORD_DOES_NOT_MEET_REQUIREMENTS errors occur from actual auth operations
Now this is a pattern that can work and doesn't involve deciding for the API consumers, plus has the benefit of knowing firebase-js-sdk "blesses" it upstream as a pattern.
Honestly, I had thought something like this would work but I'll admit that I hesitated and in the end did not ask as it was a pretty big departure from the original implementation and a lot more work for a community-provided PR 😅 . Normally community PR authors are a bit more casual and not quite as dedicated! I completely underestimated your interest in the feature though :-), my apologies. I appreciate your implementing something that does the caching without making policy decisions for app consumers, the implementation looks fantastic, the testing looks great as well.
I left what I would consider to be the tiniest of nitpicks on the tests mostly because if the rest of the code is going to be basically perfect might as well make the tests perfect as well. But maybe my analysis on the beforeEach/afterEach chunks is incorrect?
What do you think?
Otherwise - and speaking specifically of the feature implementation - definitely ready to merge. Thank you
|
does appear there is a legitimate issue that the tests have probed and either the test isn't quite correct or there may be a small implementation detail? If you check the CI run logs you'll see it... |
gyula-s
left a comment
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.
I think I've replied to your comments.
The tests were failing yesterday (and coverage).
I think I've addressed some of it, but locally it's a bit of a pain to get the same results.
waiting for GHA to report back on any failures to carry on.
mikehardy
left a comment
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.
+1 pending CI, thanks @gyula-s
|
sorry @mikehardy |
Cache the fetched password policy per Firebase app to avoid
redundant API calls to the identity toolkit endpoint.
Description
Every time you call
validatePassword(), the library was making an API call to Firebase's Identity Toolkitto fetch the password policy. When a user types their password character by character (e.g.,
"P-a-s-s-w-o-r-d-1-2-3-$"), that's 12 API calls just for one password entry. If users backspace and
retype, it adds up fast.
Firebase has rate limits on this endpoint, so you hit:
QUOTA_EXCEEDED: Exceeded quota for getting password policyThe Fix (Aligned with Firebase JS SDK)
This implementation mirrors the Firebase JS SDK's approach for password policy caching:
Instance-level caching
_projectPasswordPolicy) and tenant-specific policies (_tenantPasswordPolicies)Lazy loading
validatePassword()callMulti-tenant support
tenantIdis nullReactive cache invalidation
PASSWORD_DOES_NOT_MEET_REQUIREMENTSerror occursSchema version validation
Related issues
#8780
Release Summary
Fixed
validatePassword()causingQUOTA_EXCEEDEDerrors by caching the password policyper Auth instance. Implementation matches Firebase JS SDK behavior including tenant support
and reactive cache invalidation.
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2e(no e2e behaviour changed)jesttests added or updated inpackages/\*\*/__tests__Test Plan
Jest tests added covering:
_recachePasswordPolicy()