Skip to content

Add App Check support to Database #1260

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Apr 20, 2023
Merged

Add App Check support to Database #1260

merged 18 commits into from
Apr 20, 2023

Conversation

a-maurice
Copy link
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Add App Check support to Realtime Database.

Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Tested App Check test manually on desktop, mobile to follow.

Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@a-maurice a-maurice changed the title Am app check database Add App Check support to Database Apr 4, 2023
@a-maurice a-maurice added the skip-release-notes Skip release notes check label Apr 4, 2023
@a-maurice a-maurice requested a review from AlmostMatt April 4, 2023 22:47
@a-maurice a-maurice added the tests-requested: quick Trigger a quick set of integration tests. label Apr 5, 2023
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Apr 5, 2023
@github-actions
Copy link

github-actions bot commented Apr 5, 2023 •
edited
Loading

❌  Integration test FAILED

Requested by @a-maurice on commit 9c20916
Last updated: Thu Apr 20 16:23 PDT 2023
View integration test log & download artifacts

Failures Configs
firestore [TEST] [ERROR] [tvOS] [macos] [tvos_simulator]

Add flaky tests to go/fpl-cpp-flake-tracker

@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 5, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 5, 2023
AlmostMatt
AlmostMatt reviewed Apr 6, 2023
View reviewed changes
Copy link
Contributor

@AlmostMatt AlmostMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AppCheck part of the changes looks reasonable, I can see that it calls the registered getappchecktoken function, gets the token from the future, and so on.
I'm less familiar with the database logic and so I am unfamiliar with some of the subtlety of what should happen in the cases like one or the other of app check and auth tokens failing. I also couldn't follow the logic from token update to websocketclientimpl getting an updated token.

@a-maurice a-maurice requested a review from chkuang-g April 6, 2023 21:31
AlmostMatt
AlmostMatt reviewed Apr 6, 2023
View reviewed changes
Copy link
Contributor

@AlmostMatt AlmostMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though it will be good to have Shawn also review the database logic.

chkuang-g
chkuang-g reviewed Apr 18, 2023
View reviewed changes
@a-maurice a-maurice requested a review from chkuang-g April 18, 2023 17:54
@a-maurice a-maurice merged commit 9c20916 into main Apr 20, 2023
@a-maurice a-maurice deleted the am-app_check_database branch April 20, 2023 21:00
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: succeeded This PR's integration tests succeeded. labels Apr 20, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 20, 2023
AlmostMatt pushed a commit that referenced this pull request Apr 21, 2023
@a-maurice @AlmostMatt
Add App Check support to Database (#1260)
8558ba5 
* Add App Check logic to database

* Formatting

* Fix lint issues

* Fix for unit tests

* Formatting

* Update the WebSocket app check token

* Update persistent_connection.cc

* Update connection.cc

* Update connection.cc

* Changes to address feedback, and other fixes

* Formatting

* Use a const ref string with the function registry

* Formatting

* Fixes for the App Check Android testapp

* Revert "Fixes for the App Check Android testapp"

This reverts commit 9583509.
@firebase firebase locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AlmostMatt AlmostMatt AlmostMatt approved these changes

@chkuang-g chkuang-g chkuang-g approved these changes

Assignees
No one assigned
Labels
skip-release-notes Skip release notes check tests: failed This PR's integration tests failed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@a-maurice @AlmostMatt @chkuang-g