AppCheck integration with Firestore #3027
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (20463279) is created by Prow via merging commits: 0f2e98a 7c942dc. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (20463279) is created by Prow via merging commits: 0f2e98a 7c942dc. |
|
@google-oss-bot /retest |
|
/retest |
ehsannas
force-pushed
the
ehsann/firestore-appcheck-3
branch
from
1bf108a to
fbc0307
Compare
ehsannas
force-pushed
the
ehsann/firestore-appcheck-3
branch
from
8e97db4 to
ab128a5
Compare
ehsannas
force-pushed
the
ehsann/firestore-appcheck-3
branch
from
ab128a5 to
19b7e93
Compare
...irestore/src/main/java/com/google/firebase/firestore/auth/FirebaseAppCheckTokenProvider.java
Show resolved
Hide resolved
| /** Creates a new FirebaseAppCheckTokenProvider. */ | ||
| @SuppressLint("ProviderAssignment") // TODO: Remove this @SuppressLint once b/181014061 is fixed. | ||
| public FirebaseAppCheckTokenProvider( | ||
| Provider<InternalAppCheckTokenProvider> appCheckTokenProvider) { |
There was a problem hiding this comment.
I think you still want to use Deferred here to match auth. The one simplification we can make is that when getToken is called we can synchronously check whether the AppCheckProvider is already available.
...tore/src/test/java/com/google/firebase/firestore/auth/FirebaseAppCheckTokenProviderTest.java
Outdated
Show resolved
Hide resolved
...tore/src/test/java/com/google/firebase/firestore/auth/FirebaseAppCheckTokenProviderTest.java
Outdated
Show resolved
Hide resolved
...e-firestore/src/main/java/com/google/firebase/firestore/remote/FirestoreCallCredentials.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Sebastian Schmidt <[email protected]>
|
/test smoke-tests |
| @SuppressLint("ProviderAssignment") // TODO: Remove this @SuppressLint once b/181014061 is fixed. | ||
| public FirebaseAppCheckTokenProvider( | ||
| Deferred<InternalAppCheckTokenProvider> deferredAppCheckTokenProvider) { | ||
| deferredAppCheckTokenProvider.whenAvailable( |
There was a problem hiding this comment.
@vkryachko Can you double check this initialization and the getToken() call?
There was a problem hiding this comment.
I'll land this PR and address any concerns in a follow up PR. I'd appreciate your feedback.
There was a problem hiding this comment.
Overall lgtm. one thing to check is whether calling addAppCheckTokenListener could actually end up calling the tokenListener and whether the tokenListener could be expensive, the reason I bring this up is because whenAvailable() runs on the UI thread so we should be careful executing anything expensive inside whenAvailable
| @@ -85,7 +88,8 @@ | |||
| // databaseId itself that needs locking; it just saves us creating a separate lock object. | |||
| private final DatabaseId databaseId; | |||
| private final String persistenceKey; | |||
| private final CredentialsProvider credentialsProvider; | |||
| private final CredentialsProvider<User> authProvider; | |||
| private final CredentialsProvider<String> appCheckProvider; | |||
There was a problem hiding this comment.
Optional nit: I like the type-safety of CredentialsProvider, where as CredentialsProvider<String> can be any string not related to app check. Consider making it a CredentialsProvider<AppCheckToken> for increase type-safety
No description provided.