diff --git a/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/SSOService.java b/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/SSOService.java index 68dab1997..8a59a2097 100644 --- a/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/SSOService.java +++ b/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/SSOService.java @@ -57,6 +57,13 @@ public interface SSOService { */ List getUsersWithActions(GetUsersWithActionsRequest request); + /** + * Updates the OTP type of a user. + *

+ * An unknown OTP type code is rejected. Setting type 'none' for a user whose OTP + * is mandatory (is_otp_optional = false) is rejected with a conflict error: make + * OTP optional first. + */ void updateUserOtpType(UpdateUserOtpTypeRequest request); /** @@ -108,6 +115,13 @@ void registerUser(UserRegisterRequest request) */ String getUrlToGoogleAuthQrCode(GetGoogleAuthQrCodeRequest request); + /** + * Updates the "is OTP optional" flag of a user. + *

+ * When OTP is made mandatory (isOtpOptional = false) and the user's OTP type is + * 'none' or not set, the OTP type is automatically set to 'google_auth', so this + * call may change not only the flag. + */ void updateUserIsOtpOptionalValue(UpdateUserIsOtpOptionalValueRequest request); /** diff --git a/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/serialization/ExceptionSerializationHelper.java b/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/serialization/ExceptionSerializationHelper.java index 36d4f3ae9..49cb164a8 100644 --- a/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/serialization/ExceptionSerializationHelper.java +++ b/superfly-remote-api/src/main/java/com/payneteasy/superfly/api/serialization/ExceptionSerializationHelper.java @@ -26,6 +26,8 @@ public class ExceptionSerializationHelper { registerExceptionClass(SsoUserException.class); registerExceptionClass(SsoSystemException.class); registerExceptionClass(SsoDataException.class); + registerExceptionClass(SsoBadRequestException.class); + registerExceptionClass(SsoConflictException.class); } /** diff --git a/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/OtpTypeDefaults.java b/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/OtpTypeDefaults.java new file mode 100644 index 000000000..2d3c0ae86 --- /dev/null +++ b/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/OtpTypeDefaults.java @@ -0,0 +1,27 @@ +package com.payneteasy.superfly.model.ui.user; + +import com.payneteasy.superfly.api.OTPType; + +/** + * The rule "OTP mandatory => OTP type is not none", in one place. + *

+ * The service layer applies it on save, and the admin pages apply it in the form so that + * the administrator sees the same outcome before saving. Keeping the default type and the + * condition here stops the two layers from drifting apart. + */ +public final class OtpTypeDefaults { + + /** OTP type assigned when OTP is mandatory but no type has been chosen. */ + public static final OTPType MANDATORY_DEFAULT = OTPType.GOOGLE_AUTH; + + private OtpTypeDefaults() { + } + + /** + * @return true when the user would end up with mandatory OTP and no OTP type, + * which the authentication code silently treats as "no second factor" + */ + public static boolean needsDefaultType(UIUser user) { + return !user.isOtpOptional() && OTPType.fromCode(user.getOtpType()) == OTPType.NONE; + } +} diff --git a/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/UIUser.java b/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/UIUser.java index e369ce100..ffec087cd 100644 --- a/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/UIUser.java +++ b/superfly-service/src/main/java/com/payneteasy/superfly/model/ui/user/UIUser.java @@ -26,7 +26,12 @@ public class UIUser implements Serializable { private String salt; private String publicKey; private String otpType = OTPType.NONE.code(); - private boolean isOtpOptional; + /** + * Mirrors the DB default {@code is_otp_optional='Y'}: a user built in code and not + * loaded from the DB must not silently end up with mandatory OTP (which would now + * also force an OTP type on them). + */ + private boolean isOtpOptional = true; private UISubsystemForFilter subsystemForEmail; private String organization; diff --git a/superfly-service/src/main/java/com/payneteasy/superfly/service/impl/UserServiceImpl.java b/superfly-service/src/main/java/com/payneteasy/superfly/service/impl/UserServiceImpl.java index a19eef32f..3339554c4 100644 --- a/superfly-service/src/main/java/com/payneteasy/superfly/service/impl/UserServiceImpl.java +++ b/superfly-service/src/main/java/com/payneteasy/superfly/service/impl/UserServiceImpl.java @@ -2,6 +2,8 @@ import com.payneteasy.superfly.api.OTPType; import com.payneteasy.superfly.api.exceptions.PolicyValidationException; +import com.payneteasy.superfly.api.exceptions.SsoBadRequestException; +import com.payneteasy.superfly.api.exceptions.SsoConflictException; import com.payneteasy.superfly.api.exceptions.SsoDecryptException; import com.payneteasy.superfly.dao.DaoConstants; import com.payneteasy.superfly.dao.UserDao; @@ -165,8 +167,12 @@ public UIUserDetails getUser(long userId) { public RoutineResult updateUser(UIUser user) { UIUserForCreate userForDao = new UIUserForCreate(); copyUserAndEncryptPassword(user, userForDao); + boolean otpTypeAssigned = assignDefaultOtpTypeIfOtpMandatory(userForDao); // password and salt are not updated here RoutineResult result = userDao.updateUser(userForDao); + if (otpTypeAssigned) { + loggerSink.info(logger, "AUTO_SET_OTP_TYPE", result.isOk(), user.getUsername()); + } loggerSink.info(logger, "UPDATE_USER", result.isOk(), user.getUsername()); return result; } @@ -462,7 +468,12 @@ public String getUserSalt(String userName) { @Override public RoutineResult createUser(UIUserForCreate user) { - return userDao.createUser(user); + boolean otpTypeAssigned = assignDefaultOtpTypeIfOtpMandatory(user); + RoutineResult result = userDao.createUser(user); + if (otpTypeAssigned) { + loggerSink.info(logger, "AUTO_SET_OTP_TYPE", result.isOk(), user.getUsername()); + } + return result; } @Override @@ -556,12 +567,60 @@ public void clearHOTPLoginsFailed(String username) { @Override public void updateUserOtpType(String username, String otpType) { - userDao.updateUserOtpType(username,otpType); + OTPType newOtpType = normalizeOtpType(otpType); + if (newOtpType == OTPType.NONE) { + UserForDescription user = userDao.getUserForDescription(username); + if (user != null && !user.isOtpOptional()) { + throw new SsoConflictException("Cannot set OTP type 'none' for user '" + username + + "': OTP is mandatory for this user, make OTP optional first"); + } + } + userDao.updateUserOtpType(username, newOtpType.code()); } @Override public void updateUserIsOtpOptionalValue(String username, boolean isOtpOptional) { userDao.updateUserIsOtpOptionalValue(username,isOtpOptional); + if (!isOtpOptional) { + UserForDescription user = userDao.getUserForDescription(username); + if (user != null && user.getOtpType() == OTPType.NONE) { + userDao.updateUserOtpType(username, OtpTypeDefaults.MANDATORY_DEFAULT.code()); + loggerSink.info(logger, "AUTO_SET_OTP_TYPE", true, username); + } + } + } + + /** + * Authentication code enforces mandatory OTP only when the user has a concrete OTP type, + * so a user with mandatory OTP must never remain with type 'none'/null. + * + * @return true if the OTP type was assigned, so that the caller can audit it + * together with the result of the DAO call + */ + private boolean assignDefaultOtpTypeIfOtpMandatory(UIUser user) { + // reject an unknown code instead of letting it pass as "no OTP" below + normalizeOtpType(user.getOtpType()); + + if (!OtpTypeDefaults.needsDefaultType(user)) { + return false; + } + user.setOtpType(OtpTypeDefaults.MANDATORY_DEFAULT.code()); + return true; + } + + /** + * Maps an OTP type code to the enum: an absent or blank code means "no OTP", + * an unknown code is rejected instead of being silently stored as no OTP. + */ + private static OTPType normalizeOtpType(String otpTypeCode) { + if (otpTypeCode == null || otpTypeCode.trim().isEmpty()) { + return OTPType.NONE; + } + try { + return OTPType.strictFromCode(otpTypeCode); + } catch (IllegalStateException e) { + throw new SsoBadRequestException("Unknown OTP type code: '" + otpTypeCode + "'"); + } } @Override diff --git a/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceImplTest.java b/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceImplTest.java index 778a479b6..d3b0b6d7d 100644 --- a/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceImplTest.java +++ b/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceImplTest.java @@ -1,6 +1,9 @@ package com.payneteasy.superfly.service.impl; +import com.payneteasy.superfly.api.OTPType; import com.payneteasy.superfly.api.exceptions.MessageSendException; +import com.payneteasy.superfly.api.exceptions.SsoBadRequestException; +import com.payneteasy.superfly.api.exceptions.SsoConflictException; import com.payneteasy.superfly.dao.UserDao; import com.payneteasy.superfly.lockout.LockoutStrategy; import com.payneteasy.superfly.model.LockoutType; @@ -9,6 +12,7 @@ import com.payneteasy.superfly.model.ui.user.UICloneUserRequest; import com.payneteasy.superfly.model.ui.user.UIUser; import com.payneteasy.superfly.model.ui.user.UIUserForCreate; +import com.payneteasy.superfly.model.ui.user.UserForDescription; import com.payneteasy.superfly.password.ConstantSaltSource; import com.payneteasy.superfly.password.MessageDigestPasswordEncoder; import com.payneteasy.superfly.password.PlaintextPasswordEncoder; @@ -26,6 +30,7 @@ import static org.easymock.EasyMock.anyObject; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; public class UserServiceImplTest { @@ -166,6 +171,219 @@ public void testGetUserLoginStatusTemp() { EasyMock.verify(userDao); } + @Test + public void testUpdateUserIsOtpOptionalValueMandatorySetsDefaultOtpType() { + userDao.updateUserIsOtpOptionalValue("pete", false); + EasyMock.expect(userDao.getUserForDescription("pete")).andReturn(userForDescription("none", false)); + userDao.updateUserOtpType("pete", "google_auth"); + EasyMock.replay(userDao); + + userService.updateUserIsOtpOptionalValue("pete", false); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserIsOtpOptionalValueMandatoryKeepsExistingOtpType() { + userDao.updateUserIsOtpOptionalValue("pete", false); + EasyMock.expect(userDao.getUserForDescription("pete")).andReturn(userForDescription("google_auth", false)); + EasyMock.replay(userDao); + + userService.updateUserIsOtpOptionalValue("pete", false); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserIsOtpOptionalValueOptionalDoesNotTouchOtpType() { + userDao.updateUserIsOtpOptionalValue("pete", true); + EasyMock.replay(userDao); + + userService.updateUserIsOtpOptionalValue("pete", true); + + EasyMock.verify(userDao); + } + + @Test + public void testCreateUserWithMandatoryOtpGetsDefaultOtpType() { + EasyMock.expect(userDao.createUser(anyObject(UIUserForCreate.class))).andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUserForCreate user = (UIUserForCreate) EasyMock.getCurrentArguments()[0]; + assertEquals("google_auth", user.getOtpType()); + user.setId(1L); + return RoutineResult.okResult(); + } + }); + EasyMock.replay(userDao); + + UIUserForCreate user = new UIUserForCreate(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpType(null); + user.setOtpOptional(false); + userService.createUser(user, "subsystem"); + + EasyMock.verify(userDao); + } + + @Test + public void testCreateUserWithOptionalOtpKeepsNoneOtpType() { + EasyMock.expect(userDao.createUser(anyObject(UIUserForCreate.class))).andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUserForCreate user = (UIUserForCreate) EasyMock.getCurrentArguments()[0]; + assertEquals(OTPType.NONE.code(), user.getOtpType()); + user.setId(1L); + return RoutineResult.okResult(); + } + }); + EasyMock.replay(userDao); + + UIUserForCreate user = new UIUserForCreate(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpOptional(true); + userService.createUser(user, "subsystem"); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserWithMandatoryOtpGetsDefaultOtpType() { + EasyMock.expect(userDao.updateUser(anyObject(UIUser.class))).andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUser user = (UIUser) EasyMock.getCurrentArguments()[0]; + assertEquals("google_auth", user.getOtpType()); + return RoutineResult.okResult(); + } + }); + EasyMock.replay(userDao); + + UIUser user = new UIUser(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpType("none"); + user.setOtpOptional(false); + userService.updateUser(user); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserWithOptionalOtpKeepsNoneOtpType() { + EasyMock.expect(userDao.updateUser(anyObject(UIUser.class))).andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUser user = (UIUser) EasyMock.getCurrentArguments()[0]; + assertEquals("none", user.getOtpType()); + return RoutineResult.okResult(); + } + }); + EasyMock.replay(userDao); + + UIUser user = new UIUser(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpType("none"); + user.setOtpOptional(true); + userService.updateUser(user); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserWithMandatoryOtpKeepsChosenOtpType() { + EasyMock.expect(userDao.updateUser(anyObject(UIUser.class))).andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUser user = (UIUser) EasyMock.getCurrentArguments()[0]; + assertEquals("google_auth", user.getOtpType()); + return RoutineResult.okResult(); + } + }); + EasyMock.replay(userDao); + + UIUser user = new UIUser(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpType("google_auth"); + user.setOtpOptional(false); + userService.updateUser(user); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserOtpTypeNoneRejectedWhenOtpMandatory() { + EasyMock.expect(userDao.getUserForDescription("pete")).andReturn(userForDescription("none", false)); + EasyMock.replay(userDao); + + assertThrows(SsoConflictException.class, () -> userService.updateUserOtpType("pete", "none")); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserOtpTypeNoneAllowedWhenOtpOptional() { + EasyMock.expect(userDao.getUserForDescription("pete")).andReturn(userForDescription("google_auth", true)); + userDao.updateUserOtpType("pete", "none"); + EasyMock.replay(userDao); + + userService.updateUserOtpType("pete", "none"); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserOtpTypeUnknownCodeRejected() { + EasyMock.replay(userDao); + + assertThrows(SsoBadRequestException.class, () -> userService.updateUserOtpType("pete", "garbage")); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserOtpTypeBlankCodeMeansNone() { + EasyMock.expect(userDao.getUserForDescription("pete")).andReturn(userForDescription("google_auth", true)); + userDao.updateUserOtpType("pete", "none"); + EasyMock.replay(userDao); + + userService.updateUserOtpType("pete", " "); + + EasyMock.verify(userDao); + } + + @Test + public void testCreateUserWithUnknownOtpTypeRejected() { + EasyMock.replay(userDao); + + UIUserForCreate user = new UIUserForCreate(); + user.setUsername("pete"); + user.setPassword("secret"); + user.setOtpType("garbage"); + user.setOtpOptional(false); + + assertThrows(SsoBadRequestException.class, () -> userService.createUser(user, "subsystem")); + + EasyMock.verify(userDao); + } + + @Test + public void testUpdateUserOtpTypeGoogleAuth() { + userDao.updateUserOtpType("pete", "google_auth"); + EasyMock.replay(userDao); + + userService.updateUserOtpType("pete", "google_auth"); + + EasyMock.verify(userDao); + } + + private static UserForDescription userForDescription(String otpTypeCode, boolean isOtpOptional) { + UserForDescription user = new UserForDescription(); + user.setUsername("pete"); + user.setOtpTypeCode(otpTypeCode); + user.setOtpOptional(isOtpOptional); + return user; + } + private static class TestLockoutStrategy implements LockoutStrategy { public String username; public LockoutType lockoutType; diff --git a/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceLoggingTest.java b/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceLoggingTest.java index ce2ef7ce5..0f8a9ef12 100644 --- a/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceLoggingTest.java +++ b/superfly-service/src/test/java/com/payneteasy/superfly/service/impl/UserServiceLoggingTest.java @@ -6,6 +6,7 @@ import com.payneteasy.superfly.model.ui.user.UICloneUserRequest; import com.payneteasy.superfly.model.ui.user.UIUser; import com.payneteasy.superfly.model.ui.user.UIUserForCreate; +import com.payneteasy.superfly.model.ui.user.UserForDescription; import com.payneteasy.superfly.password.NullSaltSource; import com.payneteasy.superfly.password.PlaintextPasswordEncoder; import com.payneteasy.superfly.password.SHA256RandomGUIDSaltGenerator; @@ -115,6 +116,82 @@ public void testUpdateUserFail() throws Exception { EasyMock.verify(loggerSink); } + @Test + public void testCreateUserAutoSetsOtpType() throws Exception { + userDao.createUser(anyObject(UIUserForCreate.class)); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public RoutineResult answer() throws Throwable { + UIUserForCreate user = (UIUserForCreate) EasyMock.getCurrentArguments()[0]; + user.setId(1L); + return okResult(); + } + }); + loggerSink.info(anyObject(Logger.class), eq("AUTO_SET_OTP_TYPE"), eq(true), eq("test-user")); + loggerSink.info(anyObject(Logger.class), eq("CREATE_USER"), eq(true), eq("test-user")); + EasyMock.replay(loggerSink, userDao); + + UIUserForCreate user = new UIUserForCreate(); + user.setUsername("test-user"); + user.setOtpOptional(false); + userService.createUser(user, "subsystem"); + + EasyMock.verify(loggerSink); + } + + /** + * The audit trail must not claim an OTP type was set on a user that was never created. + */ + @Test + public void testCreateUserAutoSetOtpTypeFail() throws Exception { + userDao.createUser(anyObject(UIUserForCreate.class)); + EasyMock.expectLastCall().andReturn(failureResult()); + loggerSink.info(anyObject(Logger.class), eq("AUTO_SET_OTP_TYPE"), eq(false), eq("test-user")); + loggerSink.info(anyObject(Logger.class), eq("CREATE_USER"), eq(false), eq("test-user")); + EasyMock.replay(loggerSink, userDao); + + UIUserForCreate user = new UIUserForCreate(); + user.setUsername("test-user"); + user.setOtpOptional(false); + userService.createUser(user, "subsystem"); + + EasyMock.verify(loggerSink); + } + + @Test + public void testUpdateUserAutoSetsOtpType() throws Exception { + userDao.updateUser(anyObject(UIUser.class)); + EasyMock.expectLastCall().andReturn(okResult()); + loggerSink.info(anyObject(Logger.class), eq("AUTO_SET_OTP_TYPE"), eq(true), eq("test-user")); + loggerSink.info(anyObject(Logger.class), eq("UPDATE_USER"), eq(true), eq("test-user")); + EasyMock.replay(loggerSink, userDao); + + UIUser user = new UIUser(); + user.setUsername("test-user"); + user.setOtpOptional(false); + userService.updateUser(user); + + EasyMock.verify(loggerSink); + } + + @Test + public void testUpdateUserIsOtpOptionalValueAutoSetsOtpType() throws Exception { + userDao.updateUserIsOtpOptionalValue("test-user", false); + EasyMock.expectLastCall(); + UserForDescription userForDescription = new UserForDescription(); + userForDescription.setUsername("test-user"); + userForDescription.setOtpTypeCode("none"); + userForDescription.setOtpOptional(false); + EasyMock.expect(userDao.getUserForDescription("test-user")).andReturn(userForDescription); + userDao.updateUserOtpType("test-user", "google_auth"); + EasyMock.expectLastCall(); + loggerSink.info(anyObject(Logger.class), eq("AUTO_SET_OTP_TYPE"), eq(true), eq("test-user")); + EasyMock.replay(loggerSink, userDao); + + userService.updateUserIsOtpOptionalValue("test-user", false); + + EasyMock.verify(loggerSink, userDao); + } + @Test public void testDeleteUser() throws Exception { userDao.deleteUser(anyLong()); diff --git a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication.properties b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication.properties index db285ca2b..9e55643a4 100644 --- a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication.properties +++ b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication.properties @@ -113,4 +113,5 @@ smtpServer.ssl=SSL # OTP TYPE # otpType.none=None -otpType.google_auth=Google Auth \ No newline at end of file +otpType.google_auth=Google Auth +user.otpTypeAutoSet=OTP type was automatically set to Google Auth \ No newline at end of file diff --git a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication_ru.xml b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication_ru.xml index 1a1d59dec..077db08e0 100644 --- a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication_ru.xml +++ b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/SuperflyApplication_ru.xml @@ -106,5 +106,6 @@ --> None Google Auth +Тип OTP автоматически установлен в Google Auth \ No newline at end of file diff --git a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/component/otp/OtpTypeAutoSetBehavior.java b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/component/otp/OtpTypeAutoSetBehavior.java new file mode 100644 index 000000000..aca2bb49d --- /dev/null +++ b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/component/otp/OtpTypeAutoSetBehavior.java @@ -0,0 +1,50 @@ +package com.payneteasy.superfly.web.wicket.component.otp; + +import com.payneteasy.superfly.model.ui.user.OtpTypeDefaults; +import com.payneteasy.superfly.model.ui.user.UIUser; +import com.payneteasy.superfly.web.wicket.component.field.LabelDropDownChoiceRow; +import org.apache.wicket.Component; +import org.apache.wicket.ajax.AjaxRequestTarget; +import org.apache.wicket.ajax.form.OnChangeAjaxBehavior; + +/** + * Keeps the invariant "OTP mandatory => OTP type is not none" visible in the form: + * as soon as the edited user would end up with mandatory OTP and no OTP type, the type + * is set to the default one and the administrator is told about it. + *

+ * Attach it to both the "is OTP optional" check box and the OTP type drop down: an ajax + * request submits only the component it originates from, so a behavior on the check box + * alone would decide on a stale OTP type (and vice versa). + * + * @see com.payneteasy.superfly.service.UserService#updateUser the same rule is enforced server-side + */ +public class OtpTypeAutoSetBehavior extends OnChangeAjaxBehavior { + + private static final String MESSAGE_KEY = "user.otpTypeAutoSet"; + + private final UIUser user; + private final LabelDropDownChoiceRow otpTypeRow; + private final Component feedbackPanel; + + public OtpTypeAutoSetBehavior(UIUser user, LabelDropDownChoiceRow otpTypeRow, Component feedbackPanel) { + this.user = user; + this.otpTypeRow = otpTypeRow; + this.feedbackPanel = feedbackPanel; + } + + @Override + protected void onUpdate(AjaxRequestTarget target) { + if (!OtpTypeDefaults.needsDefaultType(user)) { + return; + } + + user.setOtpType(OtpTypeDefaults.MANDATORY_DEFAULT.code()); + // a form component renders the value it received, not the model, so a stale + // selection (a previous failed submit, or the choice we have just overridden) + // would survive the re-render + otpTypeRow.clearInput(); + + getComponent().info(getComponent().getString(MESSAGE_KEY)); + target.add(otpTypeRow, feedbackPanel); + } +} diff --git a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPage.java b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPage.java index 13a4190d2..5c92ebbb5 100644 --- a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPage.java +++ b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPage.java @@ -15,6 +15,7 @@ import com.payneteasy.superfly.web.wicket.component.field.LabelPasswordTextFieldRow; import com.payneteasy.superfly.web.wicket.component.field.LabelTextAreaRow; import com.payneteasy.superfly.web.wicket.component.field.LabelTextFieldRow; +import com.payneteasy.superfly.web.wicket.component.otp.OtpTypeAutoSetBehavior; import com.payneteasy.superfly.web.wicket.page.BasePage; import com.payneteasy.superfly.web.wicket.validation.PasswordInputValidator; import com.payneteasy.superfly.web.wicket.validation.PublicKeyValidator; @@ -142,9 +143,15 @@ protected void onUpdate(AjaxRequestTarget target) { form.add(new LabelTextFieldRow(user,"organization","user.create.organization", false)); - form.add(new LabelDropDownChoiceRow<>("otpType", user, "user.create.otpTypeCode", otpTypes(), otpRender())); + final LabelDropDownChoiceRow otpTypeRow = + new LabelDropDownChoiceRow<>("otpType", user, "user.create.otpTypeCode", otpTypes(), otpRender()); + otpTypeRow.setOutputMarkupId(true); + otpTypeRow.getDropDownChoice().add(new OtpTypeAutoSetBehavior(user, otpTypeRow, getFeedbackPanel())); + form.add(otpTypeRow); - form.add(new LabelCheckBoxRow("isOtpOptional", user, "user.create.isOtpOptional")); + LabelCheckBoxRow isOtpOptionalRow = new LabelCheckBoxRow("isOtpOptional", user, "user.create.isOtpOptional"); + isOtpOptionalRow.getCheckBox().add(new OtpTypeAutoSetBehavior(user, otpTypeRow, getFeedbackPanel())); + form.add(isOtpOptionalRow); form.add(new Button("add") { @Override diff --git a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPage.java b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPage.java index a73170f57..fd91e0293 100644 --- a/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPage.java +++ b/superfly-web/src/main/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPage.java @@ -8,6 +8,7 @@ import com.payneteasy.superfly.web.wicket.component.field.LabelDropDownChoiceRow; import com.payneteasy.superfly.web.wicket.component.field.LabelTextAreaRow; import com.payneteasy.superfly.web.wicket.component.field.LabelTextFieldRow; +import com.payneteasy.superfly.web.wicket.component.otp.OtpTypeAutoSetBehavior; import com.payneteasy.superfly.web.wicket.page.BasePage; import com.payneteasy.superfly.web.wicket.validation.PublicKeyValidator; import org.apache.wicket.Page; @@ -92,9 +93,15 @@ protected void onSubmit() { form.add(new LabelTextFieldRow(user,"organization","user.create.organization", false)); - form.add(new LabelDropDownChoiceRow<>("otpType", user, "user.create.otpTypeCode", otpTypes(), otpRender())); + final LabelDropDownChoiceRow otpTypeRow = + new LabelDropDownChoiceRow<>("otpType", user, "user.create.otpTypeCode", otpTypes(), otpRender()); + otpTypeRow.setOutputMarkupId(true); + otpTypeRow.getDropDownChoice().add(new OtpTypeAutoSetBehavior(user, otpTypeRow, getFeedbackPanel())); + form.add(otpTypeRow); - form.add(new LabelCheckBoxRow( "isOtpOptional",user, "user.create.isOtpOptional")); + LabelCheckBoxRow isOtpOptionalRow = new LabelCheckBoxRow("isOtpOptional", user, "user.create.isOtpOptional"); + isOtpOptionalRow.getCheckBox().add(new OtpTypeAutoSetBehavior(user, otpTypeRow, getFeedbackPanel())); + form.add(isOtpOptionalRow); form.add(new BookmarkablePageLink("cancel", ListUsersPage.class)); } diff --git a/superfly-web/src/main/resources/release-notes.xml b/superfly-web/src/main/resources/release-notes.xml index 4362db884..8d25f6ce7 100644 --- a/superfly-web/src/main/resources/release-notes.xml +++ b/superfly-web/src/main/resources/release-notes.xml @@ -1,5 +1,19 @@ + + + OTP type is set automatically when OTP becomes mandatory + + When the "is OTP optional" flag is cleared for a user whose OTP type + is 'none' or not set, the type is automatically set to Google Auth + (both in the admin UI and in the service layer, so the Remote API is + covered too). Setting OTP type 'none' for a user with mandatory OTP + is now rejected with a conflict error, and an unknown OTP type code + is rejected instead of silently resetting the type. + + + + UI design and usability diff --git a/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPageTest.java b/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPageTest.java new file mode 100644 index 000000000..1c97ec8ac --- /dev/null +++ b/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/CreateUserPageTest.java @@ -0,0 +1,125 @@ +package com.payneteasy.superfly.web.wicket.page.user; + +import com.payneteasy.superfly.api.OTPType; +import com.payneteasy.superfly.crypto.PublicKeyCrypto; +import com.payneteasy.superfly.service.RoleService; +import com.payneteasy.superfly.service.SettingsService; +import com.payneteasy.superfly.service.SubsystemService; +import com.payneteasy.superfly.service.UserService; +import com.payneteasy.superfly.web.wicket.page.AbstractPageTest; +import org.apache.wicket.feedback.FeedbackMessage; +import org.apache.wicket.markup.html.form.CheckBox; +import org.apache.wicket.markup.html.form.DropDownChoice; +import org.apache.wicket.util.tester.FormTester; +import org.easymock.EasyMock; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.core.context.SecurityContextHolder; + +import java.util.Collections; + +import static org.easymock.EasyMock.expect; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; + +public class CreateUserPageTest extends AbstractPageTest { + + private static final String OTP_TYPE_PATH = "form:otpType:container:field-id"; + private static final String OTP_TYPE_ROW_PATH = "form:otpType"; + private static final String OTP_OPTIONAL_PATH = "form:isOtpOptional:field-id"; + + private UserService userService; + private RoleService roleService; + private SubsystemService subsystemService; + private SettingsService settingsService; + private PublicKeyCrypto crypto; + + @Before + public void setUpBeans() { + userService = EasyMock.createNiceMock(UserService.class); + roleService = EasyMock.createNiceMock(RoleService.class); + subsystemService = EasyMock.createMock(SubsystemService.class); + settingsService = EasyMock.createNiceMock(SettingsService.class); + crypto = EasyMock.createNiceMock(PublicKeyCrypto.class); + + expect(subsystemService.getSubsystemsForFilter()).andReturn(Collections.emptyList()); + + SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken( + "admin", "admin", AuthorityUtils.createAuthorityList("ROLE_ADMIN"))); + } + + @After + public void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Override + protected Object getBean(Class type) { + if (type == UserService.class) { + return userService; + } + if (type == RoleService.class) { + return roleService; + } + if (type == SubsystemService.class) { + return subsystemService; + } + if (type == SettingsService.class) { + return settingsService; + } + if (type == PublicKeyCrypto.class) { + return crypto; + } + return super.getBean(type); + } + + private void startPage() { + EasyMock.replay(userService, roleService, subsystemService, settingsService, crypto); + tester.getApplication().getResourceSettings().setThrowExceptionOnMissingResource(false); + tester.startPage(CreateUserPage.class); + tester.assertRenderedPage(CreateUserPage.class); + } + + @Test + public void otpIsOptionalByDefaultOnTheCreateForm() { + startPage(); + + CheckBox checkBox = (CheckBox) tester.getComponentFromLastRenderedPage(OTP_OPTIONAL_PATH); + assertEquals(Boolean.TRUE, checkBox.getDefaultModelObject()); + } + + @Test + public void clearingOptionalAutoSetsGoogleAuth() { + startPage(); + + FormTester formTester = tester.newFormTester("form"); + formTester.setValue("isOtpOptional:field-id", false); + tester.executeAjaxEvent(OTP_OPTIONAL_PATH, "change"); + + DropDownChoice otpType = (DropDownChoice) tester.getComponentFromLastRenderedPage(OTP_TYPE_PATH); + assertEquals(OTPType.GOOGLE_AUTH.code(), otpType.getDefaultModelObject()); + tester.assertComponentOnAjaxResponse(OTP_TYPE_ROW_PATH); + assertFalse("expected an info message about the automatically set OTP type", + tester.getMessages(FeedbackMessage.INFO).isEmpty()); + } + + @Test + public void choosingNoneWhileOtpIsMandatoryRevertsToGoogleAuth() { + startPage(); + + FormTester optionalTester = tester.newFormTester("form"); + optionalTester.setValue("isOtpOptional:field-id", false); + tester.executeAjaxEvent(OTP_OPTIONAL_PATH, "change"); + + FormTester typeTester = tester.newFormTester("form"); + // otpTypes() lists OTPType.values() in declaration order, so the ordinal is the choice index + typeTester.select("otpType:container:field-id", OTPType.NONE.ordinal()); + tester.executeAjaxEvent(OTP_TYPE_PATH, "change"); + + DropDownChoice otpType = (DropDownChoice) tester.getComponentFromLastRenderedPage(OTP_TYPE_PATH); + assertEquals(OTPType.GOOGLE_AUTH.code(), otpType.getDefaultModelObject()); + } +} diff --git a/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPageTest.java b/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPageTest.java new file mode 100644 index 000000000..e86f0d686 --- /dev/null +++ b/superfly-web/src/test/java/com/payneteasy/superfly/web/wicket/page/user/EditUserPageTest.java @@ -0,0 +1,176 @@ +package com.payneteasy.superfly.web.wicket.page.user; + +import com.payneteasy.superfly.api.OTPType; +import com.payneteasy.superfly.crypto.PublicKeyCrypto; +import com.payneteasy.superfly.model.ui.user.UIUserDetails; +import com.payneteasy.superfly.service.SettingsService; +import com.payneteasy.superfly.service.UserService; +import com.payneteasy.superfly.web.wicket.page.AbstractPageTest; +import org.apache.wicket.feedback.FeedbackMessage; +import org.apache.wicket.util.tester.FormTester; +import org.apache.wicket.request.mapper.parameter.PageParameters; +import org.easymock.EasyMock; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.core.context.SecurityContextHolder; + +import static org.easymock.EasyMock.expect; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class EditUserPageTest extends AbstractPageTest { + + private static final String OTP_TYPE_PATH = "form:otpType:container:field-id"; + private static final String OTP_TYPE_ROW_PATH = "form:otpType"; + private static final String OTP_OPTIONAL_PATH = "form:isOtpOptional:field-id"; + + private UserService userService; + private SettingsService settingsService; + private PublicKeyCrypto crypto; + + @Before + public void setUpBeans() { + userService = EasyMock.createMock(UserService.class); + settingsService = EasyMock.createNiceMock(SettingsService.class); + crypto = EasyMock.createNiceMock(PublicKeyCrypto.class); + + // BasePage renders the logged-in user name from the security context + SecurityContextHolder.getContext().setAuthentication(new UsernamePasswordAuthenticationToken( + "admin", "admin", AuthorityUtils.createAuthorityList("ROLE_ADMIN"))); + } + + @After + public void tearDown() { + SecurityContextHolder.clearContext(); + } + + @Override + protected Object getBean(Class type) { + if (type == UserService.class) { + return userService; + } + if (type == SettingsService.class) { + return settingsService; + } + if (type == PublicKeyCrypto.class) { + return crypto; + } + return super.getBean(type); + } + + private void startPageForUser(String otpTypeCode, boolean isOtpOptional) { + UIUserDetails user = new UIUserDetails(); + user.setId(1L); + user.setUsername("john"); + user.setOtpType(otpTypeCode); + user.setOtpOptional(isOtpOptional); + expect(userService.getUser(1L)).andReturn(user); + EasyMock.replay(userService, settingsService, crypto); + + // MockApplication does not load the app-level SuperflyApplication.properties bundle, + // and this test only cares about the OTP wiring, not label texts + tester.getApplication().getResourceSettings().setThrowExceptionOnMissingResource(false); + tester.startPage(EditUserPage.class, new PageParameters().add("userId", 1L)); + tester.assertRenderedPage(EditUserPage.class); + } + + private void setOtpOptionalAndFireAjax(boolean optional) { + FormTester formTester = tester.newFormTester("form"); + formTester.setValue("isOtpOptional:field-id", optional); + tester.executeAjaxEvent(OTP_OPTIONAL_PATH, "change"); + } + + private void selectOtpTypeAndFireAjax(OTPType otpType) { + FormTester formTester = tester.newFormTester("form"); + // otpTypes() lists OTPType.values() in declaration order, so the ordinal is the choice index + formTester.select("otpType:container:field-id", otpType.ordinal()); + tester.executeAjaxEvent(OTP_TYPE_PATH, "change"); + } + + private Object renderedOtpType() { + return tester.getComponentFromLastRenderedPage(OTP_TYPE_PATH).getDefaultModelObject(); + } + + /** + * Asserting the model alone would still pass if the row were never repainted or the + * administrator never told, so pin both down. + */ + private void assertOtpTypeAutoSetReported() { + tester.assertComponentOnAjaxResponse(OTP_TYPE_ROW_PATH); + assertFalse("expected an info message about the automatically set OTP type", + tester.getMessages(FeedbackMessage.INFO).isEmpty()); + } + + private void assertNothingReported() { + assertTrue("expected no info message", tester.getMessages(FeedbackMessage.INFO).isEmpty()); + } + + @Test + public void makingOtpMandatoryAutoSetsGoogleAuthWhenTypeIsNone() { + startPageForUser(OTPType.NONE.code(), true); + + setOtpOptionalAndFireAjax(false); + + assertEquals(OTPType.GOOGLE_AUTH.code(), renderedOtpType()); + assertOtpTypeAutoSetReported(); + } + + @Test + public void makingOtpMandatoryKeepsAlreadyChosenType() { + startPageForUser(OTPType.GOOGLE_AUTH.code(), true); + + setOtpOptionalAndFireAjax(false); + + assertEquals(OTPType.GOOGLE_AUTH.code(), renderedOtpType()); + assertNothingReported(); + } + + @Test + public void keepingOtpOptionalDoesNotChangeType() { + startPageForUser(OTPType.NONE.code(), false); + + setOtpOptionalAndFireAjax(true); + + assertEquals(OTPType.NONE.code(), renderedOtpType()); + assertNothingReported(); + } + + /** + * The drop down is not submitted with the check box ajax request, so without a behavior + * of its own the rule would be decided on a stale OTP type. + */ + @Test + public void choosingNoneWhileOtpIsMandatoryRevertsToGoogleAuth() { + startPageForUser(OTPType.GOOGLE_AUTH.code(), false); + + selectOtpTypeAndFireAjax(OTPType.NONE); + + assertEquals(OTPType.GOOGLE_AUTH.code(), renderedOtpType()); + assertOtpTypeAutoSetReported(); + } + + @Test + public void choosingNoneWhileOtpIsOptionalIsKept() { + startPageForUser(OTPType.GOOGLE_AUTH.code(), true); + + selectOtpTypeAndFireAjax(OTPType.NONE); + + assertEquals(OTPType.NONE.code(), renderedOtpType()); + assertNothingReported(); + } + + @Test + public void choosingNoneThenMakingOtpMandatoryAutoSetsGoogleAuth() { + startPageForUser(OTPType.GOOGLE_AUTH.code(), true); + + selectOtpTypeAndFireAjax(OTPType.NONE); + setOtpOptionalAndFireAjax(false); + + assertEquals(OTPType.GOOGLE_AUTH.code(), renderedOtpType()); + assertOtpTypeAutoSetReported(); + } +}