diff --git a/RELEASES.md b/RELEASES.md index 7c1febd7e0..c385d6ae75 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -19,6 +19,7 @@ This file is meant as an easy way for us to collate notes and change logs across - Backing out of the PersonalID phone number screen while the app is contacting the server no longer crashes the app. - When too many incorrect email or phone OTP codes are entered, the OTP code is now reported as no longer usable and the user is prompted to request a new one, instead of showing a generic error. The wait before a new OTP code can be requested is now shown accurately. - Fixed an issue where the day and year fields of the date widget used by date questions with a `gregorian` appearance could not be edited with the keyboard in landscape. +- When a phone OTP code sent through Firebase expires or runs out of attempts, PersonalID now asks the user to request a new code instead of reporting the code as wrong. #### Internal Release Notes @@ -42,6 +43,8 @@ This file is meant as an easy way for us to collate notes and change logs across - On an opportunity with no daily visit limit, confirm the delivery dashboard and the job tile on the app home screen show only today's visit count with no progress bar, and no daily-limit warning appears. - Confirm the opportunity intro and learning-complete screens omit the "Up to N per day" text for such opportunities. - On an opportunity that still has daily limits, confirm the daily progress bar and daily-limit warnings work as before. +- With a phone number that gets its OTP code through Firebase, enter a wrong code three times. Confirm the field clears, the message says the code is no longer valid, and Request Code is offered straight away. + - Rotate the device in that state and confirm the code field stays disabled. ## CommCare 2.64.1 diff --git a/app/res/layout/screen_personalid_phone_verify.xml b/app/res/layout/screen_personalid_phone_verify.xml index c752bc4254..0a66f4f00d 100644 --- a/app/res/layout/screen_personalid_phone_verify.xml +++ b/app/res/layout/screen_personalid_phone_verify.xml @@ -98,14 +98,13 @@ android:layout_marginTop="@dimen/connect_space_xs" tools:text="Resend OTP in 60s" /> - + android:layout_marginBottom="@dimen/connect_space_lg"> + android:text="@string/connect_verify_phone_verify" + app:layout_constraintEnd_toEndOf="parent" + app:layout_constraintTop_toTopOf="parent" /> - + diff --git a/app/res/values-es/strings.xml b/app/res/values-es/strings.xml index 1909ef2543..fe893e7c91 100644 --- a/app/res/values-es/strings.xml +++ b/app/res/values-es/strings.xml @@ -223,6 +223,7 @@ Esperar Continuar sin correo Ha utilizado todos los intentos para este código, por lo que ya no es válido. Solicite un código nuevo para continuar. + Este código ya no es válido. Solicite un código nuevo para continuar. Demasiados intentos. Vuelva a intentarlo en %1$s. ¿No recibió su código? Reenviar en %1$s Espere %1$s para solicitar un código nuevo diff --git a/app/res/values-fr/strings.xml b/app/res/values-fr/strings.xml index 530ae69496..e2262ba6bd 100644 --- a/app/res/values-fr/strings.xml +++ b/app/res/values-fr/strings.xml @@ -284,6 +284,7 @@ License. Attendre Continuer sans e-mail Vous avez utilisé toutes les tentatives pour ce code, il n\'est donc plus valide. Demandez un nouveau code pour continuer. + Ce code n\'est plus valide. Demandez un nouveau code pour continuer. Trop de tentatives. Veuillez réessayer dans %1$s. Vous n\'avez pas reçu votre code ? Renvoyer dans %1$s Attendez %1$s pour demander un nouveau code diff --git a/app/res/values-ha/strings.xml b/app/res/values-ha/strings.xml index 9276510b4e..c7c0688b93 100644 --- a/app/res/values-ha/strings.xml +++ b/app/res/values-ha/strings.xml @@ -123,6 +123,7 @@ Don cikakken aiki na wurin zama na aikace-aikace da yawa, da fatan za a yi sabon Jira Ci gaba ba tare da imel ba Ka yi amfani da dukkan yunƙurin wannan lambar, don haka ba ta da inganci. Nemi sabuwar lamba don ci gaba. + Wannan lambar ba ta da inganci yanzu. Nemi sabuwar lamba don ci gaba. Yunƙuri da yawa. Don Allah sake gwadawa cikin %1$s. Ba ka karɓi lambarka ba? Sake aikawa cikin %1$s Jira %1$s don neman sabuwar lamba diff --git a/app/res/values-hi/strings.xml b/app/res/values-hi/strings.xml index dc6441f67a..6e2b216b6b 100644 --- a/app/res/values-hi/strings.xml +++ b/app/res/values-hi/strings.xml @@ -284,6 +284,7 @@ License. प्रतीक्षा करें ईमेल के बिना आगे बढ़ें आपने इस कोड के सभी प्रयास उपयोग कर लिए हैं, इसलिए यह अब मान्य नहीं है। जारी रखने के लिए नया कोड माँगें। + यह कोड अब मान्य नहीं है। जारी रखने के लिए नया कोड माँगें। बहुत अधिक प्रयास। कृपया %1$s में पुनः प्रयास करें। आपको अपना कोड नहीं मिला? %1$s में पुनः भेजें नया कोड माँगने के लिए %1$s प्रतीक्षा करें diff --git a/app/res/values-lt/strings.xml b/app/res/values-lt/strings.xml index b36beec418..db81f04823 100644 --- a/app/res/values-lt/strings.xml +++ b/app/res/values-lt/strings.xml @@ -228,6 +228,7 @@ Palaukti Tęsti be el. pašto Išnaudojote visus šio kodo bandymus, todėl jis nebegalioja. Norėdami tęsti, užsisakykite naują kodą. + Šis kodas nebegalioja. Norėdami tęsti, užsisakykite naują kodą. Per daug bandymų. Bandykite dar kartą po %1$s. Negavote kodo? Siųsti iš naujo po %1$s Palaukite %1$s, kad galėtumėte užsisakyti naują kodą diff --git a/app/res/values-no/strings.xml b/app/res/values-no/strings.xml index 2ae0e6b0a4..f7cc366b00 100644 --- a/app/res/values-no/strings.xml +++ b/app/res/values-no/strings.xml @@ -226,6 +226,7 @@ Vent Fortsett uten e-post Du har brukt alle forsøkene for denne koden, så den er ikke lenger gyldig. Be om en ny kode for å fortsette. + Denne koden er ikke lenger gyldig. Be om en ny kode for å fortsette. For mange forsøk. Prøv igjen om %1$s. Fikk du ikke koden? Send på nytt om %1$s Vent %1$s for å be om en ny kode diff --git a/app/res/values-pt/strings.xml b/app/res/values-pt/strings.xml index 72b1eb0e84..3ee51268a2 100644 --- a/app/res/values-pt/strings.xml +++ b/app/res/values-pt/strings.xml @@ -289,6 +289,7 @@ Aguardar Continuar sem e-mail Você usou todas as tentativas deste código, por isso ele não é mais válido. Solicite um novo código para continuar. + Este código não é mais válido. Solicite um novo código para continuar. Muitas tentativas. Tente novamente em %1$s. Não recebeu seu código? Reenviar em %1$s Aguarde %1$s para solicitar um novo código diff --git a/app/res/values-sw/strings.xml b/app/res/values-sw/strings.xml index 30379b9ff3..a7c058825b 100644 --- a/app/res/values-sw/strings.xml +++ b/app/res/values-sw/strings.xml @@ -285,6 +285,7 @@ Subiri Endelea bila barua pepe Umetumia majaribio yote ya msimbo huu, kwa hivyo hauna tena uhalali. Omba msimbo mpya ili kuendelea. + Msimbo huu hauna tena uhalali. Omba msimbo mpya ili kuendelea. Majaribio mengi mno. Tafadhali jaribu tena baada ya %1$s. Hukupokea msimbo wako? Tuma tena baada ya %1$s Subiri %1$s ili kuomba msimbo mpya diff --git a/app/res/values-ti/strings.xml b/app/res/values-ti/strings.xml index d1ac677e24..c97eea3d37 100644 --- a/app/res/values-ti/strings.xml +++ b/app/res/values-ti/strings.xml @@ -277,6 +277,7 @@ ተጸበዩ ብዘይ ኢመይል ቅፀል ንዚ ኮድ እዚ ኩሉ ፈተነታት ተጠቒምኩምሉ ኢኹም፣ ስለዚ ደጊም ኣይሰርሕን እዩ። ንምቕጻል ሓድሽ ኮድ ሕተቱ። + እዚ ኮድ እዚ ደጊም ኣይሰርሕን እዩ። ንምቕጻል ሓድሽ ኮድ ሕተቱ። ብዙሕ ፈተነታት። በጃኹም ድሕሪ %1$s ደጊምኩም ፈትኑ። ኮድኩም ኣይበጽሐኩምን ድዩ? ድሕሪ %1$s ደጊምኩም ስደዱ ሓድሽ ኮድ ንምሕታት %1$s ተጸበዩ diff --git a/app/res/values/strings.xml b/app/res/values/strings.xml index 78125671ed..0cb1d5da62 100644 --- a/app/res/values/strings.xml +++ b/app/res/values/strings.xml @@ -410,6 +410,7 @@ Wait Proceed without email You have used all the attempts for this code, so it is no longer valid. Request a new code to continue. + This code is no longer valid. Request a new code to continue. Too many attempts. Please try again in %1$s. Didn\'t receive your code? Resend in %1$s Wait %1$s to request a new code diff --git a/app/src/org/commcare/fragments/personalId/BasePersonalIdPhoneVerificationFragment.kt b/app/src/org/commcare/fragments/personalId/BasePersonalIdPhoneVerificationFragment.kt index a12ecaf82e..806f8fefd0 100644 --- a/app/src/org/commcare/fragments/personalId/BasePersonalIdPhoneVerificationFragment.kt +++ b/app/src/org/commcare/fragments/personalId/BasePersonalIdPhoneVerificationFragment.kt @@ -158,6 +158,11 @@ abstract class BasePersonalIdPhoneVerificationFragment : BasePersonalIdFragment( return } + if (errorType == OtpErrorType.SESSION_EXPIRED) { + showCodeNoLongerValid(getString(R.string.personalid_otp_session_expired), 0) + return + } + val userMessage = when (errorType) { OtpErrorType.INVALID_CREDENTIAL -> { @@ -265,6 +270,7 @@ abstract class BasePersonalIdPhoneVerificationFragment : BasePersonalIdFragment( otpRequestTime = DateTime.parse(otpRequestTimeString) } binding.connectPhoneVerifyButton.isEnabled = verifyButtonEnabled + binding.customOtpView.isEnabled = !otpLimitExceeded } private fun setupListeners() { @@ -308,21 +314,28 @@ abstract class BasePersonalIdPhoneVerificationFragment : BasePersonalIdFragment( } private fun onOtpLimitExceeded(throwable: Throwable?) { + val retryAfterSeconds = (throwable as? RateLimitedException)?.retryAfterSeconds + val limitExceededMessage = + PersonalIdOrConnectApiErrorHandler.handle( + requireActivity(), + PersonalIdOrConnectApiErrorCodes.OTP_LIMIT_EXCEEDED_ERROR, + throwable, + ) + showCodeNoLongerValid(limitExceededMessage, retryAfterSeconds ?: 0) + } + + private fun showCodeNoLongerValid( + message: String?, + retryAfterSeconds: Int, + ) { otpLimitExceeded = true binding.customOtpView.clearCode() binding.customOtpView.isEnabled = false binding.connectPhoneVerifyButton.isEnabled = false - val retryAfterSeconds = (throwable as? RateLimitedException)?.retryAfterSeconds - resendCooldownSeconds = retryAfterSeconds ?: 0 + resendCooldownSeconds = retryAfterSeconds otpRequestTime = DateTime() updateResendButtonState() - displayOtpError( - PersonalIdOrConnectApiErrorHandler.handle( - requireActivity(), - PersonalIdOrConnectApiErrorCodes.OTP_LIMIT_EXCEEDED_ERROR, - throwable, - ), - ) + displayOtpError(message) } private fun clearOtpError() { @@ -507,6 +520,8 @@ abstract class BasePersonalIdPhoneVerificationFragment : BasePersonalIdFragment( binding.connectResendButton.setText(resendButtonLabel) binding.connectResendButton.visibility = if (canResend) View.VISIBLE else View.GONE + binding.connectPhoneVerifyResend.visibility = + if (otpLimitExceeded && canResend) View.GONE else View.VISIBLE val resendStatusText = if (canResend) { diff --git a/app/src/org/commcare/utils/FirebaseAuthService.java b/app/src/org/commcare/utils/FirebaseAuthService.java index 188e236825..84e8c80d22 100644 --- a/app/src/org/commcare/utils/FirebaseAuthService.java +++ b/app/src/org/commcare/utils/FirebaseAuthService.java @@ -25,6 +25,8 @@ public class FirebaseAuthService implements OtpAuthService { + private static final String ERROR_SESSION_EXPIRED = "ERROR_SESSION_EXPIRED"; + private final FirebaseAuth firebaseAuth; private final OtpVerificationCallback callback; private final Activity activity; @@ -129,7 +131,10 @@ private void firebaseAuthenticator(PhoneAuthCredential credential) { @VisibleForTesting static OtpErrorType errorTypeFrom(Exception e) { if (e instanceof FirebaseAuthInvalidCredentialsException) { - return OtpErrorType.INVALID_CREDENTIAL; + String errorCode = ((FirebaseAuthInvalidCredentialsException) e).getErrorCode(); + return ERROR_SESSION_EXPIRED.equals(errorCode) + ? OtpErrorType.SESSION_EXPIRED + : OtpErrorType.INVALID_CREDENTIAL; } else if (e instanceof FirebaseTooManyRequestsException) { return OtpErrorType.TOO_MANY_REQUESTS; } else if (e instanceof FirebaseAuthMissingActivityForRecaptchaException) { diff --git a/app/src/org/commcare/utils/OtpErrorType.java b/app/src/org/commcare/utils/OtpErrorType.java index cff6ab37e5..ce987c2d63 100644 --- a/app/src/org/commcare/utils/OtpErrorType.java +++ b/app/src/org/commcare/utils/OtpErrorType.java @@ -2,18 +2,20 @@ public enum OtpErrorType { INVALID_CREDENTIAL, + SESSION_EXPIRED, TOO_MANY_REQUESTS, MISSING_ACTIVITY, GENERIC_ERROR, VERIFICATION_FAILED; /** - * INVALID_CREDENTIAL is the only error the user can act on themselves; it means the phone - * number or the code they supplied was malformed, missing or expired. Everything else - * indicates Firebase cannot deliver an OTP at all, so we treat unrecognised errors as - * non-recoverable and let the caller fall back to PersonalID SMS. + * INVALID_CREDENTIAL and SESSION_EXPIRED are the only errors the user can act on themselves: + * the phone number or code they supplied was malformed or missing, or the code is no longer + * valid and a new one must be requested. Everything else indicates Firebase cannot deliver an + * OTP at all, so we treat unrecognized errors as non-recoverable and let the caller fall back + * to PersonalID SMS. */ public boolean isNonRecoverable() { - return this != OtpErrorType.INVALID_CREDENTIAL; + return this != OtpErrorType.INVALID_CREDENTIAL && this != OtpErrorType.SESSION_EXPIRED; } } diff --git a/app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragmentTest.kt b/app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragmentTest.kt index 99fed775e2..cc731ccf30 100644 --- a/app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragmentTest.kt +++ b/app/unit-tests/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragmentTest.kt @@ -116,6 +116,29 @@ class PersonalIdPhoneVerificationFragmentTest : BasePersonalIdConfigurationTest< ReflectionHelpers.setField(authService, "firebaseAuth", firebaseAuth) } + /** Makes signInWithCredential on the live FirebaseAuthService fail with [exception]. */ + private fun stubFirebaseSignInFailsWith(exception: Exception) { + val otpManager = ReflectionHelpers.getField(fragment, "otpManager") + val authService = ReflectionHelpers.getField(otpManager, "authService") + + val firebaseAuth = mock(FirebaseAuth::class.java) + `when`(firebaseAuth.signInWithCredential(any())).thenReturn(Tasks.forException(exception)) + + ReflectionHelpers.setField(authService, "firebaseAuth", firebaseAuth) + } + + /** Launches on Firebase, waits for the code to be sent, and submits a code that Firebase rejects with [exception]. */ + private fun submitFirebaseCodeRejectedWith(exception: Exception) { + ShadowPhoneAuthProvider.sendCodeWith("test-verification-id") + launchWith(OtpManager.SMS_METHOD_FIREBASE, otpFallback = true) + ShadowLooper.idleMainLooper() + stubFirebaseSignInFailsWith(exception) + + codeView().setCode("123456") + verifyButton().performClick() + ShadowLooper.idleMainLooper() + } + private fun lastOtpMethod(): String? = ReflectionHelpers.getField(fragment, "lastOtpMethod") private fun codeView() = fragment.requireView().findViewById(R.id.customOtpView) @@ -296,6 +319,85 @@ class PersonalIdPhoneVerificationFragmentTest : BasePersonalIdConfigurationTest< ) } + // ========== Firebase verification errors ========== + + @Test + fun `a wrong firebase code is reported as a wrong code without switching to PersonalID`() { + submitFirebaseCodeRejectedWith( + FirebaseAuthInvalidCredentialsException( + "ERROR_INVALID_VERIFICATION_CODE", + "The verification code from SMS/TOTP is invalid.", + ), + ) + + assertEquals(OtpManager.SMS_METHOD_FIREBASE, lastOtpMethod()) + assertEquals(1, ShadowPhoneAuthProvider.getRequestCount()) + assertEquals( + activity.getString(R.string.personalid_incorrect_otp), + errorView().text.toString(), + ) + assertTrue("The user can correct a wrong code", codeView().isEnabled) + } + + @Test + fun `an expired firebase code tells the user to request a new one without switching to PersonalID`() { + submitFirebaseCodeRejectedWith( + FirebaseAuthInvalidCredentialsException( + "ERROR_SESSION_EXPIRED", + "The sms code has expired. Please re-send the verification code to try again.", + ), + ) + + assertEquals(OtpManager.SMS_METHOD_FIREBASE, lastOtpMethod()) + assertEquals(1, ShadowPhoneAuthProvider.getRequestCount()) + assertEquals( + activity.getString(R.string.personalid_otp_session_expired), + errorView().text.toString(), + ) + assertEquals("The dead code should not be left in the field", "", codeView().codeValue) + assertFalse("Nothing can be typed until a new code is requested", codeView().isEnabled) + } + + @Test + fun `an expired firebase code offers a new code straight away`() { + submitFirebaseCodeRejectedWith( + FirebaseAuthInvalidCredentialsException("ERROR_SESSION_EXPIRED", "expired"), + ) + + assertEquals(View.VISIBLE, resendButton().visibility) + assertEquals( + activity.getString(R.string.personalid_otp_request_code), + (resendButton() as TextView).text.toString(), + ) + assertEquals( + "The resend prompt does not apply to a code that is no longer valid", + View.GONE, + fragment.requireView().findViewById(R.id.connect_phone_verify_resend).visibility, + ) + } + + @Test + fun `an expired firebase code keeps code entry closed across a configuration change`() { + submitFirebaseCodeRejectedWith( + FirebaseAuthInvalidCredentialsException("ERROR_SESSION_EXPIRED", "expired"), + ) + + recreateFragment() + + assertFalse("Nothing can be typed until a new code is requested", codeView().isEnabled) + assertEquals(View.VISIBLE, resendButton().visibility) + } + + @Test + fun `a code that has run out of attempts keeps code entry closed across a configuration change`() { + launchOnPersonalIdPath() + submitCodeAgainst(otpLimitExceededResponse()) + + recreateFragment() + + assertFalse("Nothing can be typed until a new code is requested", codeView().isEnabled) + } + // ========== OTP_LIMIT_EXCEEDED ========== /** diff --git a/app/unit-tests/src/org/commcare/utils/FirebaseAuthServiceTest.kt b/app/unit-tests/src/org/commcare/utils/FirebaseAuthServiceTest.kt index 82e69f8f20..93ac88f420 100644 --- a/app/unit-tests/src/org/commcare/utils/FirebaseAuthServiceTest.kt +++ b/app/unit-tests/src/org/commcare/utils/FirebaseAuthServiceTest.kt @@ -37,6 +37,28 @@ class FirebaseAuthServiceTest { ) } + @Test + fun `expired verification session maps to SESSION_EXPIRED`() { + assertEquals( + OtpErrorType.SESSION_EXPIRED, + FirebaseAuthService.errorTypeFrom( + FirebaseAuthInvalidCredentialsException( + "ERROR_SESSION_EXPIRED", + "The sms code has expired. Please re-send the verification code to try again.", + ), + ), + ) + } + + @Test + fun `an expired code is classified as recoverable and does not trigger the fallback`() { + val errorType = + FirebaseAuthService.errorTypeFrom( + FirebaseAuthInvalidCredentialsException("ERROR_SESSION_EXPIRED", "expired"), + ) + assertEquals(false, errorType.isNonRecoverable) + } + @Test fun `exhausted SMS quota maps to TOO_MANY_REQUESTS`() { assertEquals( diff --git a/app/unit-tests/src/org/commcare/utils/OtpAnalyticsMapperTest.kt b/app/unit-tests/src/org/commcare/utils/OtpAnalyticsMapperTest.kt index 124d1004ec..11d70a50bd 100644 --- a/app/unit-tests/src/org/commcare/utils/OtpAnalyticsMapperTest.kt +++ b/app/unit-tests/src/org/commcare/utils/OtpAnalyticsMapperTest.kt @@ -64,6 +64,14 @@ class OtpAnalyticsMapperTest { ) } + @Test + fun `reasonFrom OtpErrorType maps SESSION_EXPIRED`() { + assertEquals( + "session_expired", + OtpAnalyticsMapper.reasonFrom(OtpErrorType.SESSION_EXPIRED), + ) + } + @Test fun `reasonFrom OtpErrorType maps TOO_MANY_REQUESTS`() { assertEquals(