- Added firebase otp send and receive helper class #3113
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughA new OTP (One-Time Password) authentication framework was introduced using Firebase in the Android app. This includes adding the Firebase Auth library as a dependency. The following components were created: an Sequence Diagram(s)OTP Authentication Flow Using FirebasesequenceDiagram
participant User
participant OTPManager
participant AuthService
participant FirebaseAuthService
participant Firebase
participant OTPVerificationCallback
User->>OTPManager: requestOTP(phoneNumber)
OTPManager->>AuthService: sendOTP(phoneNumber)
AuthService->>FirebaseAuthService: sendOTP(phoneNumber)
FirebaseAuthService->>Firebase: initiate OTP send
Firebase-->>FirebaseAuthService: onCodeSent(verificationId)
FirebaseAuthService->>OTPVerificationCallback: onCodeSent(verificationId)
User->>OTPManager: submitOTP(code)
OTPManager->>AuthService: verifyOTP(code)
AuthService->>FirebaseAuthService: verifyOTP(code)
FirebaseAuthService->>Firebase: verify code with verificationId
alt Verification Success
Firebase-->>FirebaseAuthService: onSuccess(FirebaseUser)
FirebaseAuthService->>OTPVerificationCallback: onSuccess(FirebaseUser)
else Verification Failure
Firebase-->>FirebaseAuthService: onFailure(errorMessage)
FirebaseAuthService->>OTPVerificationCallback: onFailure(errorMessage)
end
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
app/src/org/commcare/utils/AuthService.java (1)
1-6: Interface is clean but lacks documentationThe AuthService interface provides a clean contract for OTP operations with focused methods. However, it could benefit from JavaDoc comments explaining the expected behavior of each method, error handling approach, and how asynchronous results are communicated.
package org.commcare.utils; +/** + * Interface defining operations for phone number authentication using OTP (One-Time Password). + * Implementations are expected to handle the actual authentication logic and + * communicate results asynchronously through callbacks. + */ public interface AuthService { + /** + * Initiates the process of sending an OTP to the specified phone number. + * Results and errors should be communicated via callback mechanisms. + * + * @param phoneNumber The phone number to send the OTP to, in E.164 format (+[country code][number]) + */ void sendOTP(String phoneNumber); + + /** + * Verifies the OTP code entered by the user. + * Results and errors should be communicated via callback mechanisms. + * + * @param code The OTP code to verify + */ void verifyOTP(String code); }app/src/org/commcare/utils/FirebaseAuthService.java (2)
12-17: Looks good, but consider implementing Firebase instance reuse pattern.The class implements AuthService interface correctly with appropriate fields. However, consider storing the FirebaseAuth instance as a class field instead of obtaining new instances each time in methods.
public class FirebaseAuthService implements AuthService { private final Activity activity; private final OTPVerificationCallback callback; private String verificationId; + private final FirebaseAuth firebaseAuth; // Constructor for sending OTP public FirebaseAuthService(Activity activity, OTPVerificationCallback callback) { this.activity = activity; this.callback = callback; + this.firebaseAuth = FirebaseAuth.getInstance(); }
50-55: Store the ForceResendingToken for potential OTP resend functionality.The ForceResendingToken is received in onCodeSent but not stored, which prevents implementing a "Resend OTP" feature.
private String verificationId; + private PhoneAuthProvider.ForceResendingToken resendToken; // Rest of the code remains the same @Override public void onCodeSent(@NonNull String id, @NonNull PhoneAuthProvider.ForceResendingToken token) { verificationId = id; // Save verification ID for later use + resendToken = token; // Save token for potential resend callback.onCodeSent(id); }Plus adding a resend method:
public void resendOTP(String phoneNumber) { if (resendToken == null) { sendOTP(phoneNumber); // Fall back to regular send if no token return; } PhoneAuthOptions options = PhoneAuthOptions.newBuilder(firebaseAuth) .setPhoneNumber(phoneNumber) .setTimeout(getOtpTimeout(), TimeUnit.SECONDS) .setActivity(activity) .setCallbacks(/* same callbacks as in sendOTP */) .setForceResendingToken(resendToken) .build(); PhoneAuthProvider.verifyPhoneNumber(options); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/build.gradle(1 hunks)app/src/org/commcare/utils/AuthService.java(1 hunks)app/src/org/commcare/utils/FirebaseAuthService.java(1 hunks)app/src/org/commcare/utils/OTPManager.java(1 hunks)app/src/org/commcare/utils/OTPVerificationCallback.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/build.gradle (1)
163-163: Firebase Auth dependency added correctlyThe Firebase Authentication dependency has been properly added to enable OTP (One-Time Password) functionality using Firebase's phone authentication services. This aligns with the existing Firebase dependencies in the project (analytics, messaging, crashlytics).
| package org.commcare.utils; | ||
|
|
||
| import com.google.firebase.auth.FirebaseUser; | ||
|
|
||
| public interface OTPVerificationCallback { | ||
| void onCodeSent(String verificationId); | ||
| void onSuccess(FirebaseUser user); | ||
| void onFailure(String errorMessage); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider decoupling from Firebase-specific types
The callback interface is well-structured with methods for different states of OTP verification. However, it's tightly coupled to Firebase by directly using the FirebaseUser type in the onSuccess method, making it difficult to use with other authentication providers.
package org.commcare.utils;
import com.google.firebase.auth.FirebaseUser;
+/**
+ * Callback interface for OTP verification operations.
+ * Implementations receive notifications about the OTP process status.
+ */
public interface OTPVerificationCallback {
+ /**
+ * Called when an OTP code has been successfully sent to the user's phone.
+ *
+ * @param verificationId A unique identifier for this verification session
+ */
void onCodeSent(String verificationId);
- void onSuccess(FirebaseUser user);
+
+ /**
+ * Called when OTP verification completes successfully.
+ *
+ * @param user The authenticated user information
+ */
+ void onSuccess(Object user);
+
+ /**
+ * Called when an error occurs during the OTP process.
+ *
+ * @param errorMessage A description of the error that occurred
+ */
void onFailure(String errorMessage);
}Alternatively, you could create a provider-agnostic user model:
/**
* Provider-agnostic user model to decouple from Firebase
*/
public class AuthenticatedUser {
private final String userId;
private final String phoneNumber;
public AuthenticatedUser(String userId, String phoneNumber) {
this.userId = userId;
this.phoneNumber = phoneNumber;
}
public String getUserId() {
return userId;
}
public String getPhoneNumber() {
return phoneNumber;
}
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/OTPVerificationCallback.java lines 1 to 9, the
interface method onSuccess uses the FirebaseUser type, which couples the
interface to Firebase. To fix this, create a provider-agnostic user model class
(e.g., AuthenticatedUser) with userId and phoneNumber fields, then change the
onSuccess method to accept an instance of this new class instead of
FirebaseUser. This decouples the interface from Firebase and allows easier
integration with other authentication providers.
There was a problem hiding this comment.
its good to have proper comments on the public functions and interfaces
There was a problem hiding this comment.
@pm-dimagi You're absolutely right. Here's how the interface could look with proper JavaDoc comments:
package org.commcare.utils;
import com.google.firebase.auth.FirebaseUser;
/**
* Callback interface for OTP verification operations.
* Implementations receive notifications about the OTP process status.
*/
public interface OTPVerificationCallback {
/**
* Called when an OTP code has been successfully sent to the user's phone.
*
* @param verificationId A unique identifier for this verification session
*/
void onCodeSent(String verificationId);
/**
* Called when OTP verification completes successfully.
*
* @param user The authenticated user information
*/
void onSuccess(FirebaseUser user);
/**
* Called when an error occurs during the OTP process.
*
* @param errorMessage A description of the error that occurred
*/
void onFailure(String errorMessage);
}These comments clearly document the purpose of the interface and each method, making the code more maintainable and easier to understand for other developers.
✏️ Learnings added
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3113
File: app/src/org/commcare/utils/OTPVerificationCallback.java:1-9
Timestamp: 2025-05-16T15:00:46.972Z
Learning: Public interfaces, classes, and methods in the CommCare Android codebase should include proper JavaDoc comments that describe their purpose, parameters, return values, and exceptions where applicable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| package org.commcare.utils; | ||
|
|
||
|
|
||
| public class OTPManager { | ||
| private final AuthService authService; | ||
|
|
||
| public OTPManager(AuthService authService) { | ||
| this.authService = authService; | ||
| } | ||
|
|
||
| public void requestOTP(String phoneNumber) { | ||
| authService.sendOTP(phoneNumber); | ||
| } | ||
|
|
||
| public void submitOTP(String code) { | ||
| authService.verifyOTP(code); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and null checks for improved robustness
The OTPManager class uses a clean delegation pattern with dependency injection, but lacks input validation and null checks which could lead to runtime exceptions.
package org.commcare.utils;
-
+/**
+ * Manager class that wraps authentication service operations for OTP functionality.
+ * Provides a simplified interface for requesting and submitting OTPs.
+ */
public class OTPManager {
private final AuthService authService;
public OTPManager(AuthService authService) {
+ if (authService == null) {
+ throw new IllegalArgumentException("AuthService cannot be null");
+ }
this.authService = authService;
}
public void requestOTP(String phoneNumber) {
+ if (phoneNumber == null || phoneNumber.trim().isEmpty()) {
+ throw new IllegalArgumentException("Phone number cannot be null or empty");
+ }
authService.sendOTP(phoneNumber);
}
public void submitOTP(String code) {
+ if (code == null || code.trim().isEmpty()) {
+ throw new IllegalArgumentException("OTP code cannot be null or empty");
+ }
authService.verifyOTP(code);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| package org.commcare.utils; | |
| public class OTPManager { | |
| private final AuthService authService; | |
| public OTPManager(AuthService authService) { | |
| this.authService = authService; | |
| } | |
| public void requestOTP(String phoneNumber) { | |
| authService.sendOTP(phoneNumber); | |
| } | |
| public void submitOTP(String code) { | |
| authService.verifyOTP(code); | |
| } | |
| } | |
| package org.commcare.utils; | |
| /** | |
| * Manager class that wraps authentication service operations for OTP functionality. | |
| * Provides a simplified interface for requesting and submitting OTPs. | |
| */ | |
| public class OTPManager { | |
| private final AuthService authService; | |
| public OTPManager(AuthService authService) { | |
| if (authService == null) { | |
| throw new IllegalArgumentException("AuthService cannot be null"); | |
| } | |
| this.authService = authService; | |
| } | |
| public void requestOTP(String phoneNumber) { | |
| if (phoneNumber == null || phoneNumber.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Phone number cannot be null or empty"); | |
| } | |
| authService.sendOTP(phoneNumber); | |
| } | |
| public void submitOTP(String code) { | |
| if (code == null || code.trim().isEmpty()) { | |
| throw new IllegalArgumentException("OTP code cannot be null or empty"); | |
| } | |
| authService.verifyOTP(code); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/OTPManager.java lines 1 to 18, add input
validation and null checks in the requestOTP and submitOTP methods to prevent
runtime exceptions. Specifically, check if phoneNumber and code are not null or
empty before calling authService methods, and handle invalid inputs
appropriately, such as throwing IllegalArgumentException or logging errors.
| package org.commcare.utils; | ||
|
|
||
| import android.app.Activity; | ||
|
|
||
| import androidx.annotation.NonNull; | ||
|
|
||
| import com.google.firebase.FirebaseException; | ||
| import com.google.firebase.auth.*; | ||
|
|
||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class FirebaseAuthService implements AuthService { | ||
|
|
||
| private final Activity activity; | ||
| private final OTPVerificationCallback callback; | ||
| private String verificationId; | ||
|
|
||
| // Constructor for sending OTP | ||
| public FirebaseAuthService(Activity activity, OTPVerificationCallback callback) { | ||
| this.activity = activity; | ||
| this.callback = callback; | ||
| } | ||
|
|
||
| // Overloaded constructor for verifying OTP (with verification ID) | ||
| public FirebaseAuthService(Activity activity, OTPVerificationCallback callback, String verificationId) { | ||
| this(activity, callback); | ||
| this.verificationId = verificationId; | ||
| } | ||
|
|
||
| // 🔹 Called to initiate OTP sending | ||
| @Override | ||
| public void sendOTP(String phoneNumber) { | ||
| PhoneAuthOptions options = PhoneAuthOptions.newBuilder(FirebaseAuth.getInstance()) | ||
| .setPhoneNumber(phoneNumber) | ||
| .setTimeout(60L, TimeUnit.SECONDS) | ||
| .setActivity(activity) | ||
| .setCallbacks(new PhoneAuthProvider.OnVerificationStateChangedCallbacks() { | ||
|
|
||
| @Override | ||
| public void onVerificationCompleted(@NonNull PhoneAuthCredential credential) { | ||
| // Auto-verification or Instant verification | ||
| signInWithCredential(credential); | ||
| } | ||
|
|
||
| @Override | ||
| public void onVerificationFailed(@NonNull FirebaseException e) { | ||
| callback.onFailure("Verification failed: " + e.getMessage()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onCodeSent(@NonNull String id, @NonNull PhoneAuthProvider.ForceResendingToken token) { | ||
| verificationId = id; // Save verification ID for later use | ||
| callback.onCodeSent(id); | ||
| } | ||
| }) | ||
| .build(); | ||
|
|
||
| PhoneAuthProvider.verifyPhoneNumber(options); | ||
| } | ||
|
|
||
| // 🔹 Called to verify the OTP code entered by user | ||
| @Override | ||
| public void verifyOTP(String code) { | ||
| if (verificationId == null || verificationId.isEmpty()) { | ||
| callback.onFailure("Missing verification ID"); | ||
| return; | ||
| } | ||
|
|
||
| PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code); | ||
| signInWithCredential(credential); | ||
| } | ||
|
|
||
| // 🔐 Signs in using the provided credential and reports via callback | ||
| private void signInWithCredential(PhoneAuthCredential credential) { | ||
| FirebaseAuth.getInstance().signInWithCredential(credential) | ||
| .addOnCompleteListener(activity, task -> { | ||
| if (task.isSuccessful()) { | ||
| FirebaseUser user = task.getResult().getUser(); | ||
| callback.onSuccess(user); // ✅ Verified | ||
| } else { | ||
| String error = (task.getException() != null) | ||
| ? task.getException().getMessage() | ||
| : "Unknown error"; | ||
| callback.onFailure("Sign-in failed: " + error); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing secure storage for verification ID.
The verification ID is stored as a class field without any persistence mechanism. If the app is killed or the process is destroyed, this value will be lost, breaking the verification flow.
Consider implementing a secure storage solution:
import android.content.SharedPreferences;
import android.security.keystore.KeyGenParameterSpec;
import android.security.keystore.KeyProperties;
import androidx.security.crypto.EncryptedSharedPreferences;
import androidx.security.crypto.MasterKey;
// Add to class fields
private SharedPreferences securePrefs;
private static final String VERIFICATION_ID_KEY = "verification_id";
// In constructor
try {
KeyGenParameterSpec spec = new KeyGenParameterSpec.Builder(
MasterKey.DEFAULT_MASTER_KEY_ALIAS,
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
.setBlockModes(KeyProperties.BLOCK_MODE_GCM)
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE)
.setKeySize(256)
.build();
MasterKey masterKey = new MasterKey.Builder(activity)
.setKeyGenParameterSpec(spec)
.build();
securePrefs = EncryptedSharedPreferences.create(
activity,
"auth_prefs",
masterKey,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
);
// Retrieve stored verification ID if any
String storedVerificationId = securePrefs.getString(VERIFICATION_ID_KEY, null);
if (storedVerificationId != null) {
this.verificationId = storedVerificationId;
}
} catch (Exception e) {
// Fall back to in-memory storage only
android.util.Log.e("FirebaseAuthService", "Failed to initialize secure storage", e);
}
// Modified onCodeSent method
@Override
public void onCodeSent(@NonNull String id, @NonNull PhoneAuthProvider.ForceResendingToken token) {
verificationId = id;
// Store securely
if (securePrefs != null) {
securePrefs.edit().putString(VERIFICATION_ID_KEY, id).apply();
}
callback.onCodeSent(id);
}
// Add cleanup method to clear sensitive data
public void clearAuthData() {
verificationId = null;
if (securePrefs != null) {
securePrefs.edit().remove(VERIFICATION_ID_KEY).apply();
}
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java lines 1 to 88, the
verificationId is stored only in memory, which causes loss of this value if the
app is killed, breaking OTP verification. To fix this, implement secure
persistent storage using EncryptedSharedPreferences: initialize securePrefs in
the constructor with a MasterKey, retrieve any stored verificationId on startup,
and update onCodeSent to save the verificationId securely. Also, add a method to
clear the stored verificationId when needed to maintain security.
💡 Verification agent
❓ Verification inconclusive
Add lifecycle management and sign-out functionality.
The current implementation doesn't handle Activity lifecycle events, which could lead to memory leaks or disrupted authentication flows if the Activity is destroyed. Additionally, there's no method to sign out or clean up after verification.
Consider adding these methods to improve the service's lifecycle management:
/**
* Clean up resources when the activity is destroyed
*/
public void cleanup() {
// If there are any listeners or tasks that need to be cancelled
}
/**
* Sign out the current user from Firebase
*/
public void signOut() {
if (firebaseAuth != null) {
firebaseAuth.signOut();
}
}Also, the Activity should call the cleanup method in its onDestroy() method to prevent memory leaks.
Can you confirm if you're handling the Activity lifecycle events appropriately in the code that uses this class? For instance, do you call cleanup in onDestroy() and recreate the service with stored verificationId if needed in onResume()?
Add explicit sign-out and cleanup to prevent leaks
The FirebaseAuthService currently holds a strong reference to the Activity via its callbacks and inlines calls to FirebaseAuth.getInstance(). To avoid memory leaks and support explicit sign-out, please:
-
In
FirebaseAuthService, store aFirebaseAuthinstance and addcleanup()andsignOut()methods:public class FirebaseAuthService implements AuthService { private final Activity activity; private final OTPVerificationCallback callback; private final FirebaseAuth firebaseAuth = FirebaseAuth.getInstance(); private String verificationId; // existing constructors… /** * Cancel pending callbacks and clear references. */ public void cleanup() { // If you register any listeners on firebaseAuth or PhoneAuthProvider, // remove or cancel them here. verificationId = null; } /** * Sign out the current Firebase user. */ public void signOut() { firebaseAuth.signOut(); } @Override public void sendOTP(String phoneNumber) { PhoneAuthOptions options = PhoneAuthOptions.newBuilder(firebaseAuth) // … } private void signInWithCredential(PhoneAuthCredential credential) { firebaseAuth.signInWithCredential(credential) // … } }
-
In your Activity:
• CallauthService.cleanup()fromonDestroy().
• InvokeauthService.signOut()when the user logs out.
• PersistverificationId(e.g., inonSaveInstanceState) and recreate the service inonResume()if OTP verification spans configuration changes.
This ensures you won’t leak the Activity and you support proper sign-out flows.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java lines 1 to 88, add a
FirebaseAuth instance field to avoid repeated calls to
FirebaseAuth.getInstance(). Implement cleanup() to clear verificationId and
cancel any listeners if added, and signOut() to call firebaseAuth.signOut().
Update sendOTP() and signInWithCredential() to use this firebaseAuth instance.
Ensure the Activity using this service calls cleanup() in onDestroy() and
signOut() when logging out, and manages verificationId persistence across
lifecycle events to prevent memory leaks and support proper sign-out.
| private void signInWithCredential(PhoneAuthCredential credential) { | ||
| FirebaseAuth.getInstance().signInWithCredential(credential) | ||
| .addOnCompleteListener(activity, task -> { | ||
| if (task.isSuccessful()) { | ||
| FirebaseUser user = task.getResult().getUser(); | ||
| callback.onSuccess(user); // ✅ Verified | ||
| } else { | ||
| String error = (task.getException() != null) | ||
| ? task.getException().getMessage() | ||
| : "Unknown error"; | ||
| callback.onFailure("Sign-in failed: " + error); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle potential null result and add error logging.
The code retrieves user from task.getResult() without null checking, which could cause NullPointerException. Additionally, consider adding error logging for better diagnostics.
private void signInWithCredential(PhoneAuthCredential credential) {
- FirebaseAuth.getInstance().signInWithCredential(credential)
+ firebaseAuth.signInWithCredential(credential)
.addOnCompleteListener(activity, task -> {
if (task.isSuccessful()) {
- FirebaseUser user = task.getResult().getUser();
+ if (task.getResult() != null) {
+ FirebaseUser user = task.getResult().getUser();
+ callback.onSuccess(user); // ✅ Verified
+ } else {
+ callback.onFailure("Authentication successful but user data is missing");
+ }
} else {
+ // Log the exception for debugging
+ if (task.getException() != null) {
+ // Consider using a proper logging framework instead of Android Log
+ android.util.Log.e("FirebaseAuthService", "Sign-in failed", task.getException());
+ }
String error = (task.getException() != null)
? task.getException().getMessage()
: "Unknown error";
callback.onFailure("Sign-in failed: " + error);
}
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java around lines 74 to 87,
the code calls task.getResult().getUser() without checking if task.getResult()
is null, risking a NullPointerException. Fix this by adding a null check for
task.getResult() before accessing getUser(). Also, add error logging in the
failure branch to record the exception details for better diagnostics.
| public void verifyOTP(String code) { | ||
| if (verificationId == null || verificationId.isEmpty()) { | ||
| callback.onFailure("Missing verification ID"); | ||
| return; | ||
| } | ||
|
|
||
| PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code); | ||
| signInWithCredential(credential); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the OTP code input.
The method doesn't validate if the code parameter is null or empty before proceeding with verification.
@Override
public void verifyOTP(String code) {
+ if (code == null || code.trim().isEmpty()) {
+ callback.onFailure("OTP code cannot be empty");
+ return;
+ }
+
if (verificationId == null || verificationId.isEmpty()) {
callback.onFailure("Missing verification ID");
return;
}
PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code);
signInWithCredential(credential);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void verifyOTP(String code) { | |
| if (verificationId == null || verificationId.isEmpty()) { | |
| callback.onFailure("Missing verification ID"); | |
| return; | |
| } | |
| PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code); | |
| signInWithCredential(credential); | |
| } | |
| @Override | |
| public void verifyOTP(String code) { | |
| if (code == null || code.trim().isEmpty()) { | |
| callback.onFailure("OTP code cannot be empty"); | |
| return; | |
| } | |
| if (verificationId == null || verificationId.isEmpty()) { | |
| callback.onFailure("Missing verification ID"); | |
| return; | |
| } | |
| PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code); | |
| signInWithCredential(credential); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java around lines 63 to 71,
the verifyOTP method lacks validation for the code parameter. Add a check at the
start of the method to verify if the code is null or empty, and if so, call
callback.onFailure with an appropriate error message and return early to prevent
further processing.
| public void sendOTP(String phoneNumber) { | ||
| PhoneAuthOptions options = PhoneAuthOptions.newBuilder(FirebaseAuth.getInstance()) | ||
| .setPhoneNumber(phoneNumber) | ||
| .setTimeout(60L, TimeUnit.SECONDS) | ||
| .setActivity(activity) | ||
| .setCallbacks(new PhoneAuthProvider.OnVerificationStateChangedCallbacks() { | ||
|
|
||
| @Override | ||
| public void onVerificationCompleted(@NonNull PhoneAuthCredential credential) { | ||
| // Auto-verification or Instant verification | ||
| signInWithCredential(credential); | ||
| } | ||
|
|
||
| @Override | ||
| public void onVerificationFailed(@NonNull FirebaseException e) { | ||
| callback.onFailure("Verification failed: " + e.getMessage()); | ||
| } | ||
|
|
||
| @Override | ||
| public void onCodeSent(@NonNull String id, @NonNull PhoneAuthProvider.ForceResendingToken token) { | ||
| verificationId = id; // Save verification ID for later use | ||
| callback.onCodeSent(id); | ||
| } | ||
| }) | ||
| .build(); | ||
|
|
||
| PhoneAuthProvider.verifyPhoneNumber(options); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for phone number and make timeout configurable.
The method calls Firebase APIs directly without validating the phone number format. Additionally, the timeout is hardcoded to 60 seconds.
Consider implementing these improvements:
@Override
public void sendOTP(String phoneNumber) {
+ if (phoneNumber == null || phoneNumber.trim().isEmpty()) {
+ callback.onFailure("Phone number cannot be empty");
+ return;
+ }
+
+ // Optional: Add phone number format validation using regex
+ // if (!phoneNumber.matches("\\+[0-9]{10,15}")) {
+ // callback.onFailure("Invalid phone number format. Use international format: +XXXXXXXXXX");
+ // return;
+ // }
PhoneAuthOptions options = PhoneAuthOptions.newBuilder(firebaseAuth)
.setPhoneNumber(phoneNumber)
- .setTimeout(60L, TimeUnit.SECONDS)
+ .setTimeout(getOtpTimeout(), TimeUnit.SECONDS)
.setActivity(activity)
.setCallbacks(new PhoneAuthProvider.OnVerificationStateChangedCallbacks() {
// Rest of the code remains the same
}
+ // Add this method to make timeout configurable
+ private long getOtpTimeout() {
+ return 60L; // Can be modified to get from configuration or preferences
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java between lines 32 and 59,
the sendOTP method lacks validation for the phoneNumber input and uses a
hardcoded timeout of 60 seconds. To fix this, add validation logic to check the
phone number format before proceeding with Firebase calls, and modify the method
to accept a configurable timeout parameter instead of using a fixed value. This
will ensure only valid phone numbers are processed and allow flexibility in
timeout settings.
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Seeing an example of how this code will be used (once the UI is ready) will be helpful. Otherwise my suggestions are mostly about consistent naming within the codebase.
| package org.commcare.utils; | ||
|
|
||
| import com.google.firebase.auth.FirebaseUser; | ||
|
|
||
| public interface OTPVerificationCallback { | ||
| void onCodeSent(String verificationId); | ||
| void onSuccess(FirebaseUser user); | ||
| void onFailure(String errorMessage); | ||
| } |
There was a problem hiding this comment.
its good to have proper comments on the public functions and interfaces
|
@jaypanchal-13 It would be nice if you can show the usage of this code, You can mention here in comment only. Sorry I am bit confused like which / how calls should be made to send Otp and then to verify Otp. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Almost there, a couple more things to do:
-Add Example Usage to the PR description to show how the code will be called from other places.
-Add javadoc comments to interfaces and public methods
|
|
|
Thanks @jaypanchal-13 for clarification here If UI is interacting with |
|
…157-sending-receiving-otp-firebase # Conflicts: # app/build.gradle
…eceiving-otp-firebase
|
@jaypanchal-13 I can see that you are trying to achieve the simple solution for otp send and verify. Its really good you have introduced the OtpManager in between UI and FirebaseAuthService. As per the current implementation, fragment is instantiating FirebaseAuthService and providing to OtpManager. But it has problem here, OtpManager still depends upon the fragment's (UI module) FirebaseAuthService (data module) instantiation and OtpManager cannot function independently. Also, whenever FirebaseAuthService is instantiated, activity reference is stored in it which is again problem from life cycle point of view. For better solution, If we can implement simple solution: Fragement -> instantiate OtpManager(activity, callback) -> FirebaseAuthService() and create and store PhoneAuthOptions.Builder using activity and callback. OtpManager has FirebaseAuthService reference Send Otp: OtpManager.sendOtp(phoneNumber) -> firebaseAuthService.sendOtp(phoneNumber) -> add phone number in PhoneAuthOptions.Builder -> build PhoneAuthOptions -> PhoneAuthProvider.verifyPhoneNumber(PhoneAuthOptions ) Verify Otp: OtpManager.verifyOtp(code) Your thoughts? We can even call to discuss the same. @shubham1g5 @OrangeAndGreen if you have other thoughts, please share. |
@Jignesh-dimagi The current implementation allows |
|
@shubham1g5 Outer layer should decide the approach for authenticating the Otp. With current implementation, if we change Otp authentication method (say from Dimagi server), changes are required at all layers including UI. Better to decouple completely from UI. Unit tests should be separately written for UI and OtpManager without the need for mocking FirebaseAuthService in UI. This way we can reduce the code interference between layers. |
|
@Jignesh-dimagi yes that makes sense to me. Assuming a world where we have multiple
Right ? |
|
@shubham1g5 yes, this is the perfect way to explain! +1 |
|
Great! @jaypanchal-13 Any concerns with the approach discussed above ? |
|
…eceiving-otp-firebase
| * @param callback Callback to handle OTP events | ||
| */ | ||
| public FirebaseAuthService(Activity activity, OtpVerificationCallback callback) { | ||
| this.activity = activity; |
There was a problem hiding this comment.
@jaypanchal-13 We should not store the reference of activity here, instead used create PhoneAuthOptions.Builder using activity and callback as mentioned here
| * @param callback Callback interface to notify UI of OTP events | ||
| * @throws NullPointerException if any parameter is null | ||
| */ | ||
| public OtpManager(PhoneAuthOptions.Builder optionsBuilder, OtpVerificationCallback callback) { |
There was a problem hiding this comment.
@jaypanchal-13 This PhoneAuthOptions.Builder should be initialised in FirebaseAuthService only and not be provided while instantiating the OtpManager. So
OtpManager(Activity,callback) should call FirebaseAuthService(Activity, callback) which infact should initialise PhoneAuthOptions.Builder using this activity but reference to this activity should not be stored in FirebaseAuthService, but only in PhoneAuthOptions.Builder
…eceiving-otp-firebase
|
@pm-dimagi can you have another look at this, it's getting blocked because you have requested changes here. |
…eceiving-otp-firebase
Added firebase otp send and receive helper classes
Jira ticket -> https://dimagi.atlassian.net/browse/CCCT-1157