Skip to content

feat(pnv): Add support for Firebase Phone Number Verification#1203

Open
lahirumaramba wants to merge 28 commits intomainfrom
lm-fpnv
Open

feat(pnv): Add support for Firebase Phone Number Verification#1203
lahirumaramba wants to merge 28 commits intomainfrom
lm-fpnv

Conversation

@lahirumaramba
Copy link
Copy Markdown
Member

Add support for Firebase Phone Number Verification

@lahirumaramba lahirumaramba added release-note release:stage Stage a release candidate labels May 5, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 service, implementing JWT token verification using the Nimbus JOSE + JWT library. It includes the core service entry point, model classes for verified tokens, custom exception handling, and a verifier that checks JWT headers and claims against a remote JWKS. Feedback highlights a security vulnerability where the issuer claim was not validated against the specific project ID, potentially allowing tokens from other projects. Additionally, improvements were suggested for the token model to handle claim types more robustly and prevent potential class cast exceptions.

@lahirumaramba
Copy link
Copy Markdown
Member Author

@gemini-code-assist review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 service to the Java Admin SDK. The changes include the main service entry point, internal token verification logic using the Nimbus JOSE + JWT library, custom exception handling, and a comprehensive suite of unit tests. Additionally, a new AGENTS.md file defines coding guidelines for AI assistants, and a minor formatting fix was applied to Template.java. Review feedback suggests improving null safety in the token verification logic by using a more robust equality check for the JWS algorithm.

@lahirumaramba
Copy link
Copy Markdown
Member Author

@gemini-code-assist review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 service to the Java SDK, including the main service entry point, custom exception handling, and JWT token verification logic using the Nimbus library. It also adds a new AGENTS.md file to define repository-wide coding standards for AI assistants. The review feedback focuses on improving the robustness and consistency of the FirebasePhoneNumberVerificationTokenVerifier class, specifically by addressing the risks of calling overridable methods in constructors, adopting more idiomatic null-check patterns, and utilizing more specific exception types for internal configuration errors.

@lahirumaramba
Copy link
Copy Markdown
Member Author

@gemini-code-assist review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 service to the Java SDK, including the main service entry point, token representation, and internal JWT verification logic using the Nimbus library. It also adds a new guidelines file for AI coding assistants and updates the project dependencies. A thread-safety issue was identified in the token verifier class, where a field used in a double-checked locking pattern lacks the volatile keyword, potentially leading to race conditions.

private static final String HEADER_TYP = "JWT";

private final String projectId;
private DefaultJWTProcessor<SecurityContext> jwtProcessor;
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.

high

The jwtProcessor field is used in a double-checked locking pattern within getJwtProcessor() but is not declared as volatile. In the Java Memory Model, without the volatile keyword, another thread could see a non-null reference to a partially initialized DefaultJWTProcessor object, leading to unpredictable behavior or crashes.

Suggested change
private DefaultJWTProcessor<SecurityContext> jwtProcessor;
private volatile DefaultJWTProcessor<SecurityContext> jwtProcessor;

…r strict double-checked locking memory safety
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:stage Stage a release candidate release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants