-
Notifications
You must be signed in to change notification settings - Fork 123
Add App Check support for Firestore #1228
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
Conversation
❌ Integration test FAILEDRequested by @a-maurice on commit d7ff369
Add flaky tests to go/fpl-cpp-flake-tracker |
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've reviewed the firestore appcheck tests and the changes in the "firestore" directory. For the latter, I didn't look in too much detail, especially the code that looks to have been copied and adapted from the existing credentials provider. Is there anything I should look at in more detail?
Yeah, it is mostly just copied from the AuthCredentialProvider logic, and just hooks into App Check instead. As long as that seems fine with you, that's good enough. |
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.
LGTM, for the parts that I reviewed. I'll withhold my approval unless you feel like my partial review is sufficient. But I think someone more familiar with the non-firestore parts of this PR should take a look too.
If possible, I'd like someone familiar with the non-firestore parts of the code to review those. I only looked at the firestore-specific parts.
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.
Looks ok, just some minor comments
Description
Implements an App Check Credential Provider for Firestore to use for the desktop implementation, and hook into the iOS provider for the iOS implementation.
Adds a few tests to the App Check integration test to use Firestore.
Testing
Run the desktop tests locally.
Type of Change
Place an
x
the applicable box:Notes
Release Notes
section ofrelease_build_files/readme.md
.