Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a password policy configuration and validation mechanism for Firebase UI Auth, including classes to parse, validate, fetch, and cache the policy from the Identity Toolkit API. The implementation is well-tested across various scenarios. However, a critical concurrency issue was identified in PasswordPolicyRepository.lockFor where the non-atomic ConcurrentHashMap.getOrPut extension function is used, which could lead to duplicate concurrent network requests under high load. Replacing this with putIfAbsent is recommended to ensure thread safety.
| } | ||
| } | ||
|
|
||
| private fun lockFor(key: Key): Mutex = locks.getOrPut(key) { Mutex() } |
There was a problem hiding this comment.
The use of ConcurrentHashMap.getOrPut is not thread-safe because getOrPut is an inline extension function on MutableMap that does not execute atomically. Under high concurrency, multiple threads calling lockFor for the same key can simultaneously observe a null value, instantiate separate Mutex instances, and overwrite each other in the map. This defeats the purpose of the lock and can lead to duplicate concurrent network requests.\n\nTo ensure atomicity across all Android API levels without relying on API 24+ computeIfAbsent (which requires library desugaring), you can use putIfAbsent to safely and atomically initialize the Mutex.
private fun lockFor(key: Key): Mutex {
val existing = locks[key]
if (existing != null) return existing
val newMutex = Mutex()
val old = locks.putIfAbsent(key, newMutex)
return old ?: newMutex
}
Adds an internal client for the Identity Toolkit
v2/passwordPolicyendpoint, the one the JS SDK'svalidatePassword()uses, plus an evaluator for its requirements. Nothing calls it yet.Changes
PasswordPolicyRepositoryfetches withHttpURLConnectionandorg.json(no new dependencies), sendsX-Android-PackageandX-Android-Cert, addstenantIdwhen set, and caches per app and tenant.schemaVersionother than 1) returns no policy instead of throwing. Logs carry exception class names only, since connection errors include the URL with the API key.PasswordPolicy.validate()mirrors JSpassword_policy_impl.ts: ASCII character classes, symbols count only when listed inallowedNonAlphanumericCharacters, min defaults to 6, max applies only when set.Maintainer note: Part of internal CPRN-449