feat(appcheck): implement RecaptchaEnterpriseProvider and tests#16155
feat(appcheck): implement RecaptchaEnterpriseProvider and tests#16155ncooke3 wants to merge 19 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check, including the provider implementation, a factory for instantiation, and unit tests. The Package.swift file is updated to include the RecaptchaEnterpriseProvider dependency and a helper function for flexible dependency management. Feedback was provided to add error logging when the provider is initialized with an empty site key to assist with debugging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider for Firebase App Check, including its implementation, unit tests, and an E2E test application. Key changes involve adding the FIRRecaptchaEnterpriseProvider and its factory, updating the project configuration to support a new test target, and providing documentation for E2E testing. Feedback was provided regarding the requestRecaptchaToken method in the test app's AppDelegate, where the completion handler logic could lead to multiple calls or timeouts if not properly handled.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to the Firebase App Check SDK, including its implementation, factory, and unit tests. The test app has been significantly updated with new E2E tests, documentation, and logic to configure providers via environment variables. Additionally, Package.swift now supports local or branch-based dependency overrides for the App Check core library. Review feedback suggests enhancing test reliability by adding explicit non-nil assertions before comparisons and removing an unnecessary empty line in the provider implementation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to the Firebase App Check SDK, including the core implementation, unit tests, and a new E2E test suite within the sample app. The changes also improve dependency management in Package.swift and provide documentation for running E2E tests. Feedback highlights a potential redundancy in the test app's AppDelegate where a custom factory class duplicates functionality already present in the SDK.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check, including its implementation, factory, and unit tests. It also updates the E2E test application with a new test target, automated tests for token acquisition and caching, and comprehensive documentation. Feedback was provided regarding a hardcoded SDK path in the project file that could cause build failures across different environments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check, including the provider implementation, a factory for instantiation, and comprehensive unit tests. Additionally, it adds a new End-to-End (E2E) test target to the sample app along with documentation on how to configure and run these tests. Feedback was provided regarding the test app's AppDelegate, suggesting a default value for the APP_CHECK_PROVIDER environment variable to prevent the application from crashing when run without specific configurations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to the Firebase App Check SDK, including its implementation, a public factory for instantiation, and comprehensive unit tests. The sample test app is also updated with a new E2E test target, documentation, and logic to switch between providers via environment variables. Review feedback highlights several improvement opportunities: removing hardcoded SDK paths in the project file to ensure build portability, providing default values for environment variables in the test app to improve the developer experience, and using the SDK's public factory implementation instead of a local one within the test app.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check. Key changes include the implementation of the FIRRecaptchaEnterpriseProvider and its corresponding factory, the addition of unit tests for these new components, and updates to the FIRAppCheckTestApp to support end-to-end testing. The project configuration files (Package.swift and .podspec) were also updated to include the necessary dependencies. Feedback was provided regarding a potential issue in the test app's storage reading logic where the completion handler might not be invoked in all execution paths, which could cause tests to hang.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to the Firebase App Check SDK, including the core provider implementation, factory classes, and updated logging. It also adds a comprehensive E2E test suite and documentation for the FIRAppCheckTestApp. Feedback suggests improving robustness by verifying the availability of the heartbeat logger before use and simplifying redundant provider-check logic in the test app's delegate.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check, including the provider and factory implementations, comprehensive unit tests, and an updated E2E test suite within the sample app. A critical issue was identified in the FirebaseAppCheck.podspec where the missing RecaptchaEnterpriseProvider dependency will cause compilation errors for CocoaPods users. It is recommended to add this dependency, potentially as an optional subspec to avoid forcing the reCAPTCHA SDK on all users.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider for Firebase App Check, including the necessary factory and provider implementations, logger updates, and unit tests. It also enhances the FIRAppCheckTestApp with E2E testing capabilities, including new environment variable configuration and test targets. The reviewer suggested verifying that APIKey and resourceName are consistently available for the provider and recommended caution with fatalError in the test app. The addition of appCheckDependency() in Package.swift was noted as a positive improvement for development flexibility.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the reCAPTCHA Enterprise provider to Firebase App Check, including its implementation, factory, and associated unit tests. It also enhances the FIRAppCheckTestApp with E2E testing support, documentation, and dynamic provider configuration via environment variables. The reviewer suggested renaming the requestRecaptchaToken helper method in the AppDelegate to fetchAppCheckToken to more accurately describe its generic functionality of fetching tokens from the currently configured provider.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the reCAPTCHA Enterprise provider in FirebaseAppCheck, including the necessary factory and provider implementations. It also updates the FIRAppCheckTestApp to support end-to-end testing with this new provider, adds relevant documentation, and includes unit tests for token acquisition. The reviewer suggested adding null checks for internalToken in FIRRecaptchaEnterpriseProvider.m to improve robustness and discussed the use of fatalError in the test app's AppDelegate, suggesting that non-crashing failures might be more effective for debugging.
| return; | ||
| } | ||
|
|
||
| handler([[FIRAppCheckToken alloc] initWithInternalToken:internalToken], nil); |
There was a problem hiding this comment.
While the underlying provider is expected to return either a token or an error, it is safer to explicitly check for the existence of internalToken before passing it to the FIRAppCheckToken initializer to avoid potential crashes if the contract is violated.
if (internalToken == nil) {
handler(nil, [NSError errorWithDomain:@"FIRRecaptchaEnterpriseProvider" code:-1 userInfo:nil]);
return;
}
handler([[FIRAppCheckToken alloc] initWithInternalToken:internalToken], nil);| return; | ||
| } | ||
|
|
||
| handler([[FIRAppCheckToken alloc] initWithInternalToken:internalToken], nil); |
There was a problem hiding this comment.
Similar to getTokenWithCompletion:, adding a null check for internalToken here ensures robustness against unexpected behavior from the underlying provider.
if (internalToken == nil) {
handler(nil, [NSError errorWithDomain:@"FIRRecaptchaEnterpriseProvider" code:-1 userInfo:nil]);
return;
}
handler([[FIRAppCheckToken alloc] initWithInternalToken:internalToken], nil);| fatalError( | ||
| "Received both token and error from AppCheck. Token: \(token), Error: \(error)" | ||
| ) |
There was a problem hiding this comment.
Using fatalError in the AppDelegate of a test app is acceptable for failing fast during E2E tests, but consider if a non-crashing failure (like a failed assertion in the test itself) would provide a better debugging experience.
|
Status: Mostly code complete. Iterate on feedback from gemini review comments. Do another round of local testig. (1x SPM, 1x CP) And again with Apple account info in recaptcha. Next steps:
|
1 participant
Add reCAPTCHA Enterprise Provider to Firebase App Check
Metadata
app-checkrepository changes on branchnc/target-split.Description
This PR integrates the
RecaptchaEnterpriseProviderinto the Firebase App Check SDK within this repository. It provides the Objective-C wrapper implementation (FIRRecaptchaEnterpriseProvider), a factory for it, and significant updates to the test app to support automated E2E testing. It also updates CI workflows andPackage.swiftto allow testing against specific branches of theapp-checkdependency.Test App & E2E Testing
Significant updates were made to
FIRAppCheckTestAppto enable robust E2E testing:APP_CHECK_PROVIDER,RECAPTCHA_SITE_KEY) passed to the test runner.E2E_TESTING.mddetailing how to configure and run these tests locally.Dependencies
Package.swiftto support resolving theapp-checkdependency from a local path or a specific branch via environment variables (FIREBASE_APP_CHECK_LOCAL_PATH,FIREBASE_APP_CHECK_BRANCH).RecaptchaEnterpriseProviderproduct from theapp-checkpackage.FirebaseStorageto the test app to verify end-to-end flows with a Firebase service.New APIs
FIRRecaptchaEnterpriseProviderFIRAppCheckRecaptchaEnterpriseProviderFactoryCI & Build Workflow
.github/workflows/_spm.ymlto support injecting custom environment variables (via JSON) into the GitHub Actions environment..github/workflows/sdk.appcheck.ymlto use thenc/target-splitbranch of theapp-checkrepo for testing.scripts/build.shto usepython3instead ofpythonfor log exporting.Tests Added
FirebaseAppCheck/Tests/Unit/RecaptchaProvider):FIRRecaptchaEnterpriseProviderTests: Mocks the underlying core provider to verify success and failure completion paths.FIRRecaptchaEnterpriseProviderFactoryTests: Verifies the factory correctly instantiates the provider.FIRAppCheckTestAppTests):Other Changes
CHANGELOG.mdto note the addition of the reCAPTCHA Enterprise provider.FIRAppCheckLoggerfor incomplete options during instantiation.cc: @weixifan @rlazo