-
Notifications
You must be signed in to change notification settings - Fork 300
feat: add FPNV #1170
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
base: main
Are you sure you want to change the base?
feat: add FPNV #1170
Conversation
Summary of ChangesHello @boikoa-gl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates the Firebase Phone Number Verification (FPNV) feature into the SDK. It provides a new set of APIs and internal logic for verifying FPNV JWTs, allowing applications to securely authenticate users based on phone number verification. This enhancement streamlines the process of validating tokens issued by Firebase's phone authentication services, ensuring data integrity and user trust. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces the Firebase Phone Number Verification (FPNV) feature, including the main service entry point, token and exception types, and the internal token verification logic. The overall structure is sound and follows existing patterns in the Firebase Admin SDK.
However, I've identified several critical issues, particularly concerning type safety in the FirebasePnvToken class that will lead to ClassCastException at runtime. There are also security-related gaps in the token validation logic, such as incomplete issuer verification. Additionally, there are inconsistencies in exception handling and some issues in the test suite that make it less robust.
My detailed comments below provide specific suggestions to address these points, which should be resolved before merging.
src/main/java/com/google/firebase/fpnv/FirebasePnvException.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Show resolved
Hide resolved
src/test/java/com/google/firebase/fpnv/FpnvTokenVerifierTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/FirebasePnvException.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
1 similar comment
|
/gemini review |
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.
Code Review
This pull request introduces a new Firebase Phone Number Verification (FPNV) service. The implementation follows the established patterns of the Firebase Admin SDK, with a clear entry point and a dedicated token verifier. The code is well-structured and includes a good suite of unit tests. However, I have identified a critical dependency issue that will prevent the project from building, as well as several high-severity issues related to exception handling that would impede debugging. I have provided detailed comments and suggestions to address these points.
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Show resolved
Hide resolved
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.
Code Review
This pull request introduces the Firebase Phone Number Verification (FPNV) feature, including the main service entry point, token and exception classes, and the internal token verification logic. The implementation follows the existing Firebase service pattern well. However, I've identified a critical security vulnerability in the token verification logic where the issuer is not validated against the project ID. Additionally, there are a few correctness issues in the FirebasePnvToken class regarding return value units and type safety, as well as opportunities to improve exception handling by preserving the original cause of errors. My detailed comments provide specific suggestions to address these points.
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request introduces the Firebase Phone Number Verification (FPNV) service, a new feature for the Admin SDK. The changes include adding the main service entry point, token and exception classes, and the internal token verification logic. The implementation follows existing patterns in the SDK, and the addition of unit tests is great.
My review includes a few critical fixes related to time unit conversion in FirebasePnvToken, which currently returns milliseconds instead of the documented seconds. I've also suggested improvements to error handling in the token verifier to provide more accurate feedback to developers, and some minor Javadoc and test setup enhancements. Overall, this is a solid addition, and addressing these points will improve its correctness and usability.
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/FirebasePnvException.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/firebase/fpnv/FirebasePnvErrorCodeTest.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request introduces a new Firebase Phone Number Verification (FPNV) service. The implementation is well-structured, following existing patterns for Firebase services. The core token verification logic is robust, with explicit checks for JWT headers and claims. The addition of the nimbus-jose-jwt dependency is appropriate for this feature. The unit tests are comprehensive, covering various success and failure scenarios.
I have two main suggestions:
- The new
FirebasePnvExceptionshould extend the baseFirebaseExceptionto maintain consistency across the SDK, which will improve the developer experience for error handling. - A minor documentation update in
FirebasePnvTokento improve clarity.
Overall, this is a solid contribution.
src/main/java/com/google/firebase/fpnv/FirebasePnvException.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/firebase/fpnv/FirebasePnvException.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request introduces a new Firebase Phone Number Verification (FPNV) feature. The implementation is well-structured, following existing patterns in the Firebase Admin SDK. It includes a public API entry point (FirebasePnv), data models (FirebasePnvToken), custom exceptions (FirebasePnvException), and internal verification logic (FirebasePnvTokenVerifier). The token verification logic correctly handles JWT header validation, signature/claim processing via the nimbus-jose-jwt library, and FPNV-specific claim assertions. The addition of comprehensive unit tests is also a great plus. I have a couple of minor suggestions to improve code clarity and robustness.
src/main/java/com/google/firebase/fpnv/internal/FirebasePnvTokenVerifier.java
Outdated
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:
Discussion
If not, go file an issue about this before creating a pull request to discuss.
Testing
API Changes
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.