diff --git a/orcid-core/src/main/java/org/orcid/core/manager/RecoveryPhoneManager.java b/orcid-core/src/main/java/org/orcid/core/manager/RecoveryPhoneManager.java index 68dc695880..df387dbd4c 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/RecoveryPhoneManager.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/RecoveryPhoneManager.java @@ -12,11 +12,14 @@ public interface RecoveryPhoneManager { /** * The stored number in E.164 form, or null when there is none. * - * This is the only path that decrypts the stored number, and it exists for - * the flows that have to send a text to it without the user re-typing it. + * This is the only path that decrypts the stored number. It exists for the + * flows that have to send a text to it without the user re-typing it, for + * the ones that check a code against it, and for the one response allowed + * to carry it: the manage page's prefilled field, owner only, behind a + * passed authentication challenge (F4.2). * The value must never be written to a log line, put in a RUM attribute key - * or value, or returned over HTTP (R1.2): anything that reaches the user - * carries the last four digits only. + * or value, or returned over HTTP anywhere else (R1.2): everything else + * that reaches the user carries the last four digits only. */ String getDecryptedPhoneNumber(String orcid); diff --git a/orcid-core/src/main/java/org/orcid/core/manager/impl/RecoveryPhoneManagerImpl.java b/orcid-core/src/main/java/org/orcid/core/manager/impl/RecoveryPhoneManagerImpl.java index 26c29c4c49..702f2ad312 100644 --- a/orcid-core/src/main/java/org/orcid/core/manager/impl/RecoveryPhoneManagerImpl.java +++ b/orcid-core/src/main/java/org/orcid/core/manager/impl/RecoveryPhoneManagerImpl.java @@ -45,8 +45,9 @@ public String getDecryptedPhoneNumber(String orcid) { return null; } // Decrypting lives here and not in getRecoveryPhone because the Account settings panel polls - // that one for the mask and the dates alone; only the flows that have to text the number pay - // for the crypto, and the result must not reach a log line, a RUM attribute or an HTTP response. + // that one for the mask and the dates alone; only the flows that have to text the number, and + // the manage page's owner-only read behind a passed challenge (F4.2), pay for the crypto. The + // result must not reach a log line, a RUM attribute or any other HTTP response. return encryptionManager.decryptForInternalUse(entity.getEncryptedPhoneNumber()); } diff --git a/orcid-core/src/main/java/org/orcid/core/togglz/Features.java b/orcid-core/src/main/java/org/orcid/core/togglz/Features.java index 50c6d70b20..818cd7bb7e 100644 --- a/orcid-core/src/main/java/org/orcid/core/togglz/Features.java +++ b/orcid-core/src/main/java/org/orcid/core/togglz/Features.java @@ -121,6 +121,9 @@ public enum Features implements Feature { @Label("Login - recovery phone interstitial") LOGIN_RECOVERY_PHONE_INTERSTITIAL, + @Label("OAUTH - recovery phone interstitial") + OAUTH_RECOVERY_PHONE_INTERSTITIAL, + @Label("Mandatory password reset: block password sign in and show the reset notice for records flagged with force_password_reset") FORCE_PASSWORD_RESET; public boolean isActive() { diff --git a/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneNumberResponse.java b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneNumberResponse.java new file mode 100644 index 0000000000..8087c348fc --- /dev/null +++ b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneNumberResponse.java @@ -0,0 +1,55 @@ +package org.orcid.frontend.recoveryphone; + +/** + * The stored recovery number in full, in E.164 form, for the one field that + * shows it: the manage page's phone field, which starts from the number on + * file (F4.2). This is the only response that carries the number in the + * clear; every other one carries the mask `***********NNNN` (R1.2). + */ +public class RecoveryPhoneNumberResponse { + + private boolean success; + + private String errorCode; + + private String phoneNumber; + + public static RecoveryPhoneNumberResponse failure(String errorCode) { + RecoveryPhoneNumberResponse response = new RecoveryPhoneNumberResponse(); + response.setSuccess(false); + response.setErrorCode(errorCode); + return response; + } + + public static RecoveryPhoneNumberResponse success(String phoneNumber) { + RecoveryPhoneNumberResponse response = new RecoveryPhoneNumberResponse(); + response.setSuccess(true); + response.setPhoneNumber(phoneNumber); + return response; + } + + public boolean isSuccess() { + return success; + } + + public void setSuccess(boolean success) { + this.success = success; + } + + public String getErrorCode() { + return errorCode; + } + + public void setErrorCode(String errorCode) { + this.errorCode = errorCode; + } + + public String getPhoneNumber() { + return phoneNumber; + } + + public void setPhoneNumber(String phoneNumber) { + this.phoneNumber = phoneNumber; + } + +} diff --git a/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusRequest.java b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusRequest.java new file mode 100644 index 0000000000..2c2c2a2d45 --- /dev/null +++ b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusRequest.java @@ -0,0 +1,29 @@ +package org.orcid.frontend.recoveryphone; + +/** + * Asks whether the account the credentials belong to has a recovery number, + * so the 2FA step of sign in knows which way out to offer (F1.2). + */ +public class RecoveryPhoneSigninStatusRequest { + + private String username; + + private String password; + + public String getUsername() { + return username; + } + + public void setUsername(String username) { + this.username = username; + } + + public String getPassword() { + return password; + } + + public void setPassword(String password) { + this.password = password; + } + +} diff --git a/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusResponse.java b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusResponse.java new file mode 100644 index 0000000000..0ffa37b776 --- /dev/null +++ b/orcid-web/src/main/java/org/orcid/frontend/recoveryphone/RecoveryPhoneSigninStatusResponse.java @@ -0,0 +1,55 @@ +package org.orcid.frontend.recoveryphone; + +/** + * Whether the account behind the credentials has a recovery number stored + * (F1.2). It carries that one boolean and nothing else - not the mask, not the + * dates - because the screen asking only has to choose between offering a + * text and offering the help centre. + */ +public class RecoveryPhoneSigninStatusResponse { + + private boolean success; + + private String errorCode; + + private boolean hasRecoveryPhone; + + public static RecoveryPhoneSigninStatusResponse failure(String errorCode) { + RecoveryPhoneSigninStatusResponse response = new RecoveryPhoneSigninStatusResponse(); + response.setSuccess(false); + response.setErrorCode(errorCode); + return response; + } + + public static RecoveryPhoneSigninStatusResponse success(boolean hasRecoveryPhone) { + RecoveryPhoneSigninStatusResponse response = new RecoveryPhoneSigninStatusResponse(); + response.setSuccess(true); + response.setHasRecoveryPhone(hasRecoveryPhone); + return response; + } + + public boolean isSuccess() { + return success; + } + + public void setSuccess(boolean success) { + this.success = success; + } + + public String getErrorCode() { + return errorCode; + } + + public void setErrorCode(String errorCode) { + this.errorCode = errorCode; + } + + public boolean isHasRecoveryPhone() { + return hasRecoveryPhone; + } + + public void setHasRecoveryPhone(boolean hasRecoveryPhone) { + this.hasRecoveryPhone = hasRecoveryPhone; + } + +} diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninController.java b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninController.java index 418dbfb5e4..cf481bb005 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninController.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninController.java @@ -17,6 +17,8 @@ import org.orcid.frontend.recoveryphone.RecoveryPhoneSendCodeResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninSendCodeRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninSendCodeResponse; +import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninStatusRequest; +import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninStatusResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninVerifyRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninVerifyResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneVerificationService; @@ -99,6 +101,39 @@ public class RecoveryPhoneSigninController extends BaseController { @Resource private RecordEmailSender recordEmailSender; + /** + * Tells the 2FA step of sign in whether the account behind the credentials + * has a recovery number, so the step offers to text one only where there is + * a number to text, and the help centre everywhere else (F1.2). + * + * The answer goes only to a caller holding the password: the check is the + * one sendCode makes, through the same provider, so a wrong password here + * counts toward the sign in lockout like any other. The screen asks right + * after the sign in has accepted that password and said 2FA is on, so all + * this adds for the password holder is whether a number is stored. + */ + @RequestMapping(value = "/status.json", method = RequestMethod.POST) + public @ResponseBody RecoveryPhoneSigninStatusResponse status(HttpServletRequest request, + @RequestBody RecoveryPhoneSigninStatusRequest form) { + if (!Features.TWO_FACTOR_RECOVERY_PHONE.isActive()) { + return RecoveryPhoneSigninStatusResponse.failure(FEATURE_DISABLED); + } + + String credentialsFailure = verifyCredentials(request, form.getUsername(), form.getPassword()); + if (credentialsFailure != null) { + return RecoveryPhoneSigninStatusResponse.failure(credentialsFailure); + } + + String orcid = resolveOrcid(form.getUsername()); + if (orcid == null) { + return RecoveryPhoneSigninStatusResponse.failure(BAD_CREDENTIALS); + } + + // The read that costs no decryption: whether there is a number is all + // this answers + return RecoveryPhoneSigninStatusResponse.success(recoveryPhoneManager.getRecoveryPhone(orcid) != null); + } + /** * Sends a fresh code to the recovery number stored on the account the given * credentials belong to (R3.2). diff --git a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationController.java b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationController.java index 922e456ce4..850330a678 100644 --- a/orcid-web/src/main/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationController.java +++ b/orcid-web/src/main/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationController.java @@ -15,6 +15,7 @@ import org.orcid.frontend.email.RecordEmailSender; import org.orcid.frontend.recoveryphone.RecoveryPhoneChallengeSendCodeResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneChallengeVerifyRequest; +import org.orcid.frontend.recoveryphone.RecoveryPhoneNumberResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSaveRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSaveResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSendCodeRequest; @@ -46,6 +47,13 @@ public class TwoFactorAuthenticationController extends BaseController { private static final String RECOVERY_PHONE_ELEVATION_ATTRIBUTE = "RECOVERY_PHONE_ELEVATION_TS"; + /** + * When the account verification challenge was last passed on this session. + * Kept apart from the elevation, which 2FA setup also grants, because only a + * passed challenge lets the number on file be read back (F4.2). + */ + private static final String RECOVERY_PHONE_CHALLENGE_ATTRIBUTE = "RECOVERY_PHONE_CHALLENGE_TS"; + /** * How long a passed authentication challenge, or the sign in the * interstitial rides on, lets the user keep working. @@ -78,6 +86,12 @@ public class TwoFactorAuthenticationController extends BaseController { /** The password given with a challenge is not the account's password. */ static final String INVALID_PASSWORD = "INVALID_PASSWORD"; + /** + * The session's real user is not the record's owner: a delegate, or an + * admin switched into the record. + */ + static final String NOT_ACCOUNT_OWNER = "NOT_ACCOUNT_OWNER"; + /** * Where the recovery phone form is being shown. The context decides which * proof of identity the request is accepted on; it never widens what the @@ -153,11 +167,47 @@ public class TwoFactorAuthenticationController extends BaseController { return form; } - request.getSession().setAttribute(RECOVERY_PHONE_ELEVATION_ATTRIBUTE, System.currentTimeMillis()); + long passedAt = System.currentTimeMillis(); + request.getSession().setAttribute(RECOVERY_PHONE_ELEVATION_ATTRIBUTE, passedAt); + request.getSession().setAttribute(RECOVERY_PHONE_CHALLENGE_ATTRIBUTE, passedAt); form.setSuccess(true); return form; } + /** + * Hands the stored recovery number back in full for the manage page, whose + * phone field starts from the number on file rather than empty (F4.2). + * + * Who may see it is decided here and nowhere else. Only the account owner - + * never a delegate, never an admin switched into the record - and only on + * a session that passed the authentication challenge inside the elevation + * window. The interstitial's recent sign in does not count: the + * interstitial only ever adds a first number, so it has none to show. A + * POST, answered no-store, so no cache along the way keeps a copy. + */ + @RequestMapping(value = "/recoveryPhone/number.json", method = RequestMethod.POST) + public @ResponseBody RecoveryPhoneNumberResponse getRecoveryPhoneNumber(HttpServletRequest request, HttpServletResponse response) { + response.setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); + String orcid = getCurrentUserOrcid(); + if (!Features.TWO_FACTOR_RECOVERY_PHONE.isActive()) { + return RecoveryPhoneNumberResponse.failure(FEATURE_DISABLED); + } + if (!orcid.equals(getRealUserOrcid())) { + return RecoveryPhoneNumberResponse.failure(NOT_ACCOUNT_OWNER); + } + if (!twoFactorAuthenticationManager.userUsing2FA(orcid)) { + return RecoveryPhoneNumberResponse.failure(TWO_FACTOR_DISABLED); + } + if (!challengePassedRecently(request)) { + return RecoveryPhoneNumberResponse.failure(CHALLENGE_REQUIRED); + } + String phoneNumber = recoveryPhoneManager.getDecryptedPhoneNumber(orcid); + if (phoneNumber == null) { + return RecoveryPhoneNumberResponse.failure(NO_RECOVERY_PHONE); + } + return RecoveryPhoneNumberResponse.success(phoneNumber); + } + @RequestMapping(value = "/recoveryPhone/sendCode.json", method = RequestMethod.POST) public @ResponseBody RecoveryPhoneSendCodeResponse sendRecoveryPhoneCode(HttpServletRequest request, @RequestBody RecoveryPhoneSendCodeRequest form) { @@ -185,6 +235,7 @@ public class TwoFactorAuthenticationController extends BaseController { String phoneE164 = recoveryPhoneVerificationService.normalize(form.getPhoneNumber()); RecoveryPhone saved = recoveryPhoneManager.saveRecoveryPhone(orcid, phoneE164); request.getSession().removeAttribute(RECOVERY_PHONE_ELEVATION_ATTRIBUTE); + request.getSession().removeAttribute(RECOVERY_PHONE_CHALLENGE_ATTRIBUTE); // The answer is built from the row the save wrote, not read back: a read goes to // the read-only pool, and on a deployed environment that is a replica which can @@ -317,6 +368,7 @@ public class TwoFactorAuthenticationController extends BaseController { // 2FA is off, so the action this challenge was guarding proceeds on the // password alone; an elevation granted earlier has nothing left to guard request.getSession().removeAttribute(RECOVERY_PHONE_ELEVATION_ATTRIBUTE); + request.getSession().removeAttribute(RECOVERY_PHONE_CHALLENGE_ATTRIBUTE); result.setSuccess(true); return result; } @@ -340,6 +392,19 @@ private String guardRecoveryPhoneRequest(HttpServletRequest request, String orci return CHALLENGE_REQUIRED; } + /** + * The account verification challenge itself passed on this session within + * the window. Narrower than {@link #sessionIsElevated}: 2FA setup elevates + * the session too, and that grant must not read the number back (F4.2). + */ + private boolean challengePassedRecently(HttpServletRequest request) { + Object passedAt = request.getSession().getAttribute(RECOVERY_PHONE_CHALLENGE_ATTRIBUTE); + if (!(passedAt instanceof Long)) { + return false; + } + return System.currentTimeMillis() - (Long) passedAt <= RECOVERY_PHONE_ELEVATION_TTL_MILLIS; + } + /** A challenge passed on this session within the elevation window. */ private boolean sessionIsElevated(HttpServletRequest request) { Object elevatedAt = request.getSession().getAttribute(RECOVERY_PHONE_ELEVATION_ATTRIBUTE); @@ -371,8 +436,12 @@ private boolean interstitialIsElevatedByRecentLogin(String orcid, String context if (!CONTEXT_INTERSTITIAL.equals(resolveContext(context))) { return false; } - if (!Features.LOGIN_RECOVERY_PHONE_INTERSTITIAL.isActive()) { - // The relaxed path dies with the interstitial that justifies it (R7.4) + if (!Features.LOGIN_RECOVERY_PHONE_INTERSTITIAL.isActive() && !Features.OAUTH_RECOVERY_PHONE_INTERSTITIAL.isActive()) { + // The relaxed path dies with the interstitial that justifies it + // (R7.4). Either flow's flag keeps it open: which flow the form is + // in is as much a claim of the client's as the context is, so the + // server cannot tell the two apart, and both rest on the same + // proof - a sign in completed inside the window (F2.2) return false; } if (recoveryPhoneManager.getRecoveryPhone(orcid) != null) { diff --git a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninControllerTest.java b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninControllerTest.java index 33e4ffcbc6..8328dd53b0 100644 --- a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninControllerTest.java +++ b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/RecoveryPhoneSigninControllerTest.java @@ -23,6 +23,8 @@ import org.orcid.frontend.recoveryphone.RecoveryPhoneSendCodeResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninSendCodeRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninSendCodeResponse; +import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninStatusRequest; +import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninStatusResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninVerifyRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSigninVerifyResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneVerificationService; @@ -535,4 +537,97 @@ public void testAUsernameThatResolvesToNoRecordIsACredentialFailure() { assertEquals(RecoveryPhoneSigninController.BAD_CREDENTIALS, response.getErrorCode()); verify(recoveryPhoneManager, never()).getRecoveryPhone(anyString()); } + // Which way out the 2FA step offers (F1.2): the recovery number where one + // is stored, the help centre everywhere else + + private static RecoveryPhoneSigninStatusRequest statusRequest(String username) { + RecoveryPhoneSigninStatusRequest form = new RecoveryPhoneSigninStatusRequest(); + form.setUsername(username); + form.setPassword(PASSWORD); + return form; + } + + @Test + public void testStatusIsInertWhenTheFeatureIsOff() { + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(ORCID)); + + assertFalse(response.isSuccess()); + assertEquals(RecoveryPhoneSigninController.FEATURE_DISABLED, response.getErrorCode()); + verify(authenticationProvider, never()).authenticate(any(Authentication.class)); + verify(recoveryPhoneManager, never()).getRecoveryPhone(anyString()); + } + + /** + * The answer goes to the password holder only, and a wrong password goes + * through the provider, which is what counts it toward the lockout (R3.2). + */ + @Test + public void testStatusChecksAWrongPasswordThroughTheProviderAndAnswersNothing() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + passwordIsWrong(); + + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(ORCID)); + + assertFalse(response.isSuccess()); + assertEquals(RecoveryPhoneSigninController.BAD_CREDENTIALS, response.getErrorCode()); + assertFalse(response.isHasRecoveryPhone()); + verify(authenticationProvider).authenticate(any(Authentication.class)); + verify(recoveryPhoneManager, never()).getRecoveryPhone(anyString()); + } + + @Test + public void testStatusRefusesAnAccountThatIsNotUsing2FA() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + passwordIsCorrectAnd2FAIsOff(); + + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(ORCID)); + + assertFalse(response.isSuccess()); + assertEquals(RecoveryPhoneSigninController.TWO_FACTOR_DISABLED, response.getErrorCode()); + verify(recoveryPhoneManager, never()).getRecoveryPhone(anyString()); + } + + @Test + public void testStatusSaysSoWhenANumberIsStored() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + passwordIsCorrectAnd2FAIsOn(); + aRecoveryPhoneIsStored(); + + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(ORCID)); + + assertTrue(response.isSuccess()); + assertNull(response.getErrorCode()); + assertTrue(response.isHasRecoveryPhone()); + // Asking costs no decryption and sends nothing + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + verify(recoveryPhoneVerificationService, never()).sendCode(anyString(), any(RecoveryPhoneSendCodeRequest.class)); + } + + @Test + public void testStatusSaysSoWhenNoNumberIsStored() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + passwordIsCorrectAnd2FAIsOn(); + when(recoveryPhoneManager.getRecoveryPhone(ORCID)).thenReturn(null); + + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(ORCID)); + + assertTrue(response.isSuccess()); + assertFalse(response.isHasRecoveryPhone()); + verify(recoveryPhoneVerificationService, never()).sendCode(anyString(), any(RecoveryPhoneSendCodeRequest.class)); + } + + /** An email signs in as well as an iD, so it has to reach the same record. */ + @Test + public void testStatusResolvesAnEmailToTheRecord() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + passwordIsCorrectAnd2FAIsOn(); + when(emailManagerReadOnly.findOrcidIdByEmail(EMAIL)).thenReturn(ORCID); + aRecoveryPhoneIsStored(); + + RecoveryPhoneSigninStatusResponse response = controller.status(request, statusRequest(EMAIL)); + + assertTrue(response.isSuccess()); + assertTrue(response.isHasRecoveryPhone()); + verify(recoveryPhoneManager).getRecoveryPhone(ORCID); + } } diff --git a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationControllerTest.java b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationControllerTest.java index ca474b63fd..4c9cb6d326 100644 --- a/orcid-web/src/test/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationControllerTest.java +++ b/orcid-web/src/test/java/org/orcid/frontend/web/controllers/TwoFactorAuthenticationControllerTest.java @@ -18,6 +18,7 @@ import org.orcid.frontend.email.RecordEmailSender; import org.orcid.frontend.recoveryphone.RecoveryPhoneChallengeSendCodeResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneChallengeVerifyRequest; +import org.orcid.frontend.recoveryphone.RecoveryPhoneNumberResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSaveRequest; import org.orcid.frontend.recoveryphone.RecoveryPhoneSaveResponse; import org.orcid.frontend.recoveryphone.RecoveryPhoneSendCodeRequest; @@ -286,6 +287,13 @@ private void elevateSession() { session.setAttribute("RECOVERY_PHONE_ELEVATION_TS", System.currentTimeMillis()); } + /** What a passed account verification challenge leaves on the session. */ + private void passChallenge(long millisAgo) { + long passedAt = System.currentTimeMillis() - millisAgo; + session.setAttribute("RECOVERY_PHONE_ELEVATION_TS", passedAt); + session.setAttribute("RECOVERY_PHONE_CHALLENGE_TS", passedAt); + } + private static RecoveryPhoneSendCodeRequest sendCodeRequest() { RecoveryPhoneSendCodeRequest form = new RecoveryPhoneSendCodeRequest(); form.setPhoneNumber("+441234567890"); @@ -399,6 +407,7 @@ public void testAuthChallengeRejectsAWrongPassword() { assertTrue(result.isInvalidPassword()); assertNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + assertNull(session.getAttribute("RECOVERY_PHONE_CHALLENGE_TS")); } @Test @@ -416,6 +425,8 @@ public void testAuthChallengeElevatesTheSessionOnSuccess() { assertTrue(result.isSuccess()); assertNotNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + // and the narrower grant only a passed challenge gives (F4.2) + assertNotNull(session.getAttribute("RECOVERY_PHONE_CHALLENGE_TS")); } @Test @@ -536,7 +547,7 @@ public void testSaveReportsAFailedCodeAndKeepsTheElevation() { @Test public void testSaveStoresTheNumberAndClearsTheElevation() { enableRecoveryPhoneFeature(); - elevateSession(); + passChallenge(0); java.util.Date now = new java.util.Date(); when(recoveryPhoneVerificationService.verifyCode(eq(ORCID), anyString(), anyString())).thenReturn(null); when(recoveryPhoneVerificationService.normalize("+441234567890")).thenReturn("+441234567890"); @@ -553,6 +564,8 @@ public void testSaveStoresTheNumberAndClearsTheElevation() { // previous number or none at all verify(recoveryPhoneManager, never()).getRecoveryPhone(anyString()); assertNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + // the number was just replaced, so nothing on the session reads it back + assertNull(session.getAttribute("RECOVERY_PHONE_CHALLENGE_TS")); } // Onboarding: the live 2FA code posted to register.json stands in for a @@ -567,6 +580,9 @@ public void testOnboardingElevatesTheSessionForTheRecoveryPhoneStep() { assertTrue(controller.validateVerificationCode(request, registration).isValid()); assertNotNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + // Setup is not a challenge: what it grants does not read the number + // back (F4.2) + assertNull(session.getAttribute("RECOVERY_PHONE_CHALLENGE_TS")); // and that elevation is what carries step 2 of setup, with no second // challenge in between @@ -919,7 +935,7 @@ public void testChallengeVerifyIsRefusedWhenTheFeatureIsOff() { public void testChallengeVerifyDisables2FAOnAGoodCode() { enableRecoveryPhoneFeature(); // An elevation from an earlier challenge has nothing left to guard - elevateSession(); + passChallenge(60 * 1000L); profileWithLastLogin(60 * 1000L); when(encryptionManager.hashMatches("correct", "hashed")).thenReturn(true); when(recoveryPhoneManager.getDecryptedPhoneNumber(ORCID)).thenReturn("+441234567890"); @@ -935,6 +951,7 @@ public void testChallengeVerifyDisables2FAOnAGoodCode() { verify(recordEmailSender).send2FADisabledEmail(ORCID); verify(profileEntityCacheManager).remove(ORCID); assertNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + assertNull(session.getAttribute("RECOVERY_PHONE_CHALLENGE_TS")); } @Test @@ -957,4 +974,173 @@ public void testChallengeVerifySucceedsWhenTheEmailCannotBeSent() { verify(twoFactorAuthenticationManager).disable2FAByRecoveryPhone(ORCID); verify(profileEntityCacheManager).remove(ORCID); } + // The full number, for the manage page's prefilled field (F4.2): the one + // response allowed to carry it, so each refusal below is asserted as never + // having decrypted it at all + + private static final String STORED_E164 = "+441234567890"; + + @Test + public void testNumberIsInertWhenTheFeatureIsOff() { + when(twoFactorAuthenticationManager.userUsing2FA(ORCID)).thenReturn(true); + passChallenge(0); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertFalse(result.isSuccess()); + assertEquals(TwoFactorAuthenticationController.FEATURE_DISABLED, result.getErrorCode()); + assertNull(result.getPhoneNumber()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + @Test + public void testNumberIsRefusedToADelegateEvenOnAnElevatedSession() { + // A delegate, or an admin switched into the record, who somehow holds + // an elevated session is still not the account owner (F4.2) + enableRecoveryPhoneFeature(); + passChallenge(0); + doReturn("0000-0000-0000-0002").when(controller).getRealUserOrcid(); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertFalse(result.isSuccess()); + assertEquals(TwoFactorAuthenticationController.NOT_ACCOUNT_OWNER, result.getErrorCode()); + assertNull(result.getPhoneNumber()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + @Test + public void testNumberIsRefusedWhen2FAIsOff() { + togglzRule.enable(Features.TWO_FACTOR_RECOVERY_PHONE); + when(twoFactorAuthenticationManager.userUsing2FA(ORCID)).thenReturn(false); + passChallenge(0); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertEquals(TwoFactorAuthenticationController.TWO_FACTOR_DISABLED, result.getErrorCode()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + @Test + public void testNumberDemandsThePassedChallenge() { + enableRecoveryPhoneFeature(); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertFalse(result.isSuccess()); + assertEquals(TwoFactorAuthenticationController.CHALLENGE_REQUIRED, result.getErrorCode()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + @Test + public void testNumberIsRefusedOnceTheChallengeHasExpired() { + enableRecoveryPhoneFeature(); + passChallenge(9 * 60 * 1000L); + + assertEquals(TwoFactorAuthenticationController.CHALLENGE_REQUIRED, + controller.getRecoveryPhoneNumber(request, response).getErrorCode()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + /** + * The interstitial's recent sign in lets it add a first number without a + * challenge. It is no proof for reading one back: the interstitial never + * shows a number, and a sign in is not the challenge the manage page asks. + */ + @Test + public void testNumberIsNotLetThroughByARecentSignIn() { + enableInterstitialFeature(); + togglzRule.enable(Features.OAUTH_RECOVERY_PHONE_INTERSTITIAL); + profileWithLastLogin(60 * 1000L); + + assertEquals(TwoFactorAuthenticationController.CHALLENGE_REQUIRED, + controller.getRecoveryPhoneNumber(request, response).getErrorCode()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + /** + * Turning 2FA on elevates the session for the onboarding step that follows + * (R2.6). That elevation is not the account verification challenge, so it + * does not read the number back (F4.2). + */ + @Test + public void testNumberIsNotLetThroughBy2FASetup() { + enableRecoveryPhoneFeature(); + elevateSession(); + + assertEquals(TwoFactorAuthenticationController.CHALLENGE_REQUIRED, + controller.getRecoveryPhoneNumber(request, response).getErrorCode()); + verify(recoveryPhoneManager, never()).getDecryptedPhoneNumber(anyString()); + } + + @Test + public void testNumberSaysSoWhenNoneIsStored() { + enableRecoveryPhoneFeature(); + passChallenge(0); + when(recoveryPhoneManager.getDecryptedPhoneNumber(ORCID)).thenReturn(null); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertFalse(result.isSuccess()); + assertEquals(TwoFactorAuthenticationController.NO_RECOVERY_PHONE, result.getErrorCode()); + } + + @Test + public void testNumberReturnsTheStoredNumberToTheOwnerAndIsNeverCached() { + enableRecoveryPhoneFeature(); + passChallenge(7 * 60 * 1000L); + when(recoveryPhoneManager.getDecryptedPhoneNumber(ORCID)).thenReturn(STORED_E164); + + RecoveryPhoneNumberResponse result = controller.getRecoveryPhoneNumber(request, response); + + assertTrue(result.isSuccess()); + assertNull(result.getErrorCode()); + assertEquals(STORED_E164, result.getPhoneNumber()); + verify(response).setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); + // Reading it is not a change: the elevation stays for the save + assertNotNull(session.getAttribute("RECOVERY_PHONE_ELEVATION_TS")); + } + + // The OAuth interstitial carries a flag of its own (F2.1, PD-14423), and + // either flow's flag keeps the relaxed path open (F2.2) + + @Test + public void testSendCodeAcceptsTheInterstitialWithOnlyTheOauthFlagOn() { + enableRecoveryPhoneFeature(); + togglzRule.enable(Features.OAUTH_RECOVERY_PHONE_INTERSTITIAL); + profileWithLastLogin(60 * 1000L); + when(recoveryPhoneVerificationService.sendCode(eq(ORCID), any(RecoveryPhoneSendCodeRequest.class))) + .thenReturn(RecoveryPhoneSendCodeResponse.success(30)); + RecoveryPhoneSendCodeRequest form = sendCodeRequest(); + form.setContext(TwoFactorAuthenticationController.CONTEXT_INTERSTITIAL); + + assertTrue(controller.sendRecoveryPhoneCode(request, form).isSuccess()); + } + + @Test + public void testSaveAcceptsTheInterstitialWithOnlyTheOauthFlagOn() { + enableRecoveryPhoneFeature(); + togglzRule.enable(Features.OAUTH_RECOVERY_PHONE_INTERSTITIAL); + profileWithLastLogin(60 * 1000L); + when(recoveryPhoneVerificationService.verifyCode(ORCID, "+441234567890", "123456")).thenReturn(null); + when(recoveryPhoneVerificationService.normalize("+441234567890")).thenReturn("+441234567890"); + java.util.Date now = new java.util.Date(); + when(recoveryPhoneManager.saveRecoveryPhone(ORCID, "+441234567890")).thenReturn(storedRecoveryPhone("7890", now, now)); + RecoveryPhoneSaveRequest form = saveRequest(); + form.setContext(TwoFactorAuthenticationController.CONTEXT_INTERSTITIAL); + + assertTrue(controller.saveRecoveryPhone(request, form).isSuccess()); + verify(recoveryPhoneManager).saveRecoveryPhone(ORCID, "+441234567890"); + } + + @Test + public void testTheOauthFlagDoesNotRelaxTheGuardOutsideTheInterstitial() { + enableRecoveryPhoneFeature(); + togglzRule.enable(Features.OAUTH_RECOVERY_PHONE_INTERSTITIAL); + profileWithLastLogin(60 * 1000L); + + assertEquals(TwoFactorAuthenticationController.CHALLENGE_REQUIRED, + controller.sendRecoveryPhoneCode(request, sendCodeRequest()).getErrorCode()); + verify(recoveryPhoneVerificationService, never()).sendCode(anyString(), any(RecoveryPhoneSendCodeRequest.class)); + } }