Skip to content

Update app check debug provider to match other firebase sdks - #1911

Open
AustinBenoit wants to merge 1 commit into
mainfrom
app_check
Open

Update app check debug provider to match other firebase sdks#1911
AustinBenoit wants to merge 1 commit into
mainfrom
app_check

Conversation

@AustinBenoit

Copy link
Copy Markdown
Contributor

To alight with other SDK's the cpp app check debug provider will create a token on behalf of the user if they have not already created and registered one. Users will be prompted to register token

Description

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

Update app check debug provider to match other firebase sdks


Testing

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

Ran a test ensuring the new phrase is output


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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the desktop debug provider for App Check to automatically generate a UUID-based debug token if the APP_CHECK_DEBUG_TOKEN environment variable is not provided, and logs a warning message with registration instructions. Feedback points out a critical compilation error due to an incomplete declaration of GenerateDebugToken and suggests a minor optimization to avoid calling std::getenv twice.

Comment thread app_check/src/desktop/debug_provider_desktop.cc Outdated
Comment thread app_check/src/desktop/debug_provider_desktop.cc Outdated
@AustinBenoit

Copy link
Copy Markdown
Contributor Author

/gemini review

@AustinBenoit AustinBenoit assigned a-maurice and unassigned a-maurice Aug 19, 2026
@AustinBenoit
AustinBenoit requested a review from a-maurice August 19, 2026 17:48

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the App Check Desktop Debug Provider to automatically generate a local debug token using a UUID if the 'APP_CHECK_DEBUG_TOKEN' environment variable is not set, and logs instructions for registering it. The review feedback correctly identifies a potential data race in 'GetTokenInternal' when lazy-initializing the debug token and modifying the static logging flag concurrently, and suggests using a 'std::mutex' along with including the '' header to ensure thread safety.

Comment thread app_check/src/desktop/debug_provider_desktop.cc Outdated
Comment thread app_check/src/desktop/debug_provider_desktop.cc
To alight with other SDK's the cpp app check debug provider
will create a token on behalf of the user if they have not already
created and registered one. Users will be prompted to register token

@a-maurice a-maurice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unlike the other platforms, won't this generate a different debug token each time the app is run? I feel like this is more likely to cause confusion, cause the token we are telling them to use will change each time. I'd prefer if either it saved the generated token in someway and used it each time (which is admittedly going to be complicated), or to avoid doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants