fix: when you change your email and re-verify your email, 2FA needs t…

…o be disabled Signed-off-by: Morten Svanaes <msvanaes@dhis2.org>
dhis2 · Jan 31, 2025 · 92c0eb6 · 92c0eb6
1 parent 80959f6
commit 92c0eb6
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 6 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java
@@ -224,6 +224,7 @@ public enum ErrorCode {
   E3049("Sending 2FA code with email failed"),
   E3050("2FA code can not be null or empty"),
   E3051("2FA code was sent to the user's email"),
+  E3052("Email 2FA is enabled on user, can not change email. Disable 2FA first"),
 
   /* Metadata Validation */
   E4000("Missing required property `{0}`"),

diff --git a/...-core/src/main/java/org/hisp/dhis/security/spring2fa/TwoFactorAuthenticationProvider.java b/...-core/src/main/java/org/hisp/dhis/security/spring2fa/TwoFactorAuthenticationProvider.java
@@ -136,15 +136,14 @@ public Authentication authenticate(Authentication auth) throws AuthenticationExc
 
     if (userDetails.isTwoFactorEnabled()
         && auth.getDetails() instanceof TwoFactorWebAuthenticationDetails authDetails) {
-      // Check if the user's 2FA type is enabled in config
+      // Check if the user's 2FA type is enabled in config, if not skip 2FA validation, even if user
+      // is enrolled.
       TwoFactorType type = userDetails.getTwoFactorType();
       boolean isTypeEnabled = false;
       if (type == TwoFactorType.EMAIL_ENABLED) {
-        isTypeEnabled =
-            configurationProvider.getProperty(ConfigurationKey.EMAIL_2FA_ENABLED).equals("on");
+        isTypeEnabled = configurationProvider.isEnabled(ConfigurationKey.EMAIL_2FA_ENABLED);
       } else if (type == TwoFactorType.TOTP_ENABLED) {
-        isTypeEnabled =
-            configurationProvider.getProperty(ConfigurationKey.TOTP_2FA_ENABLED).equals("on");
+        isTypeEnabled = configurationProvider.isEnabled(ConfigurationKey.TOTP_2FA_ENABLED);
       }
 
       // Only validate 2FA code if the type is enabled

diff --git a/...f2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/hooks/UserObjectBundleHook.java b/...f2/src/main/java/org/hisp/dhis/dxf2/metadata/objectbundle/hooks/UserObjectBundleHook.java
@@ -50,6 +50,7 @@
 import org.hisp.dhis.organisationunit.OrganisationUnit;
 import org.hisp.dhis.preheat.PreheatIdentifier;
 import org.hisp.dhis.security.acl.AclService;
+import org.hisp.dhis.security.twofa.TwoFactorType;
 import org.hisp.dhis.system.util.ValidationUtils;
 import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.User;
@@ -132,6 +133,15 @@ public void validate(User user, ObjectBundle bundle, Consumer<ErrorReport> addRe
           new ErrorReport(User.class, ErrorCode.E4027, user.getWhatsApp(), "whatsApp")
               .setErrorProperty("whatsApp"));
     }
+
+    if (existingUser != null
+        && existingUser.getTwoFactorType() != null
+        && existingUser.getTwoFactorType().equals(TwoFactorType.EMAIL_ENABLED)
+        && existingUser.isEmailVerified()
+        && user.getEmail() != null
+        && !existingUser.getVerifiedEmail().equals(user.getEmail())) {
+      addReports.accept(new ErrorReport(User.class, ErrorCode.E3052).setErrorProperty("email"));
+    }
   }
 
   @Override

diff --git a/...s-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TwoFactorControllerTest.java b/...s-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/TwoFactorControllerTest.java
@@ -56,13 +56,17 @@
 import lombok.extern.slf4j.Slf4j;
 import org.hisp.dhis.external.conf.DhisConfigurationProvider;
 import org.hisp.dhis.feedback.ConflictException;
+import org.hisp.dhis.feedback.ErrorCode;
 import org.hisp.dhis.http.HttpStatus;
 import org.hisp.dhis.jsontree.JsonMixed;
+import org.hisp.dhis.jsontree.JsonObject;
 import org.hisp.dhis.message.MessageSender;
 import org.hisp.dhis.outboundmessage.OutboundMessage;
 import org.hisp.dhis.security.twofa.TwoFactorAuthService;
 import org.hisp.dhis.security.twofa.TwoFactorType;
 import org.hisp.dhis.test.webapi.H2ControllerIntegrationTestBase;
+import org.hisp.dhis.test.webapi.json.domain.JsonErrorReport;
+import org.hisp.dhis.test.webapi.json.domain.JsonWebMessage;
 import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.SystemUser;
 import org.hisp.dhis.user.User;
@@ -99,7 +103,6 @@ void tearDown() {
   @Test
   void testEnrollTOTP2FA()
       throws ChecksumException, NotFoundException, DecodingException, IOException, FormatException {
-
     User user = createUserAndInjectSecurityContext(false);
 
     assertStatus(HttpStatus.OK, POST("/2fa/enrollTOTP2FA"));
@@ -449,4 +452,63 @@ void testDisable2FAWhenTypeDisabled() {
     assertNull(updatedUser.getSecret());
     assertEquals(TwoFactorType.NOT_ENABLED, updatedUser.getTwoFactorType());
   }
+
+  @Test
+  void testChangeEmailWhenEmail2FAIsEnabledWithMeEndpoint() {
+    // Given user has Email 2FA enabled
+    config.getProperties().put(EMAIL_2FA_ENABLED.getKey(), "on");
+
+    User user = createUserAndInjectSecurityContext(false);
+    user.setEmail("valid.x@email.com");
+    user.setVerifiedEmail("valid.x@email.com");
+    userService.encodeAndSetPassword(user, "Test123###...");
+
+    assertStatus(HttpStatus.OK, POST("/2fa/enrollEmail2FA"));
+    User enrolledUser = userService.getUserByUsername(user.getUsername());
+    String code = enrolledUser.getSecret().split("\\|")[0];
+    assertStatus(HttpStatus.OK, POST("/2fa/enable", "{'code':'" + code + "'}"));
+    assertEquals(TwoFactorType.EMAIL_ENABLED, user.getTwoFactorType());
+
+    // When trying to change email
+    assertWebMessage(
+        "Conflict",
+        409,
+        "ERROR",
+        "Email address cannot be changed, when email-based 2FA is enabled, please disable 2FA first",
+        PUT("/me", "{'email':'another@email.com'}").content(HttpStatus.CONFLICT));
+  }
+
+  @Test
+  void testChangeEmailWhenEmail2FAIsEnabledWithUserEndpoint() {
+    // Given user has Email 2FA enabled
+    config.getProperties().put(EMAIL_2FA_ENABLED.getKey(), "on");
+
+    User user = createUserAndInjectSecurityContext(false);
+    user.setEmail("valid.x@email.com");
+    user.setVerifiedEmail("valid.x@email.com");
+    userService.encodeAndSetPassword(user, "Test123###...");
+
+    assertStatus(HttpStatus.OK, POST("/2fa/enrollEmail2FA"));
+    User enrolledUser = userService.getUserByUsername(user.getUsername());
+    String code = enrolledUser.getSecret().split("\\|")[0];
+    assertStatus(HttpStatus.OK, POST("/2fa/enable", "{'code':'" + code + "'}"));
+    assertEquals(TwoFactorType.EMAIL_ENABLED, user.getTwoFactorType());
+
+    switchContextToUser(getAdminUser());
+
+    // When trying to change email
+    JsonObject jsonUser = GET("/users/{id}", user.getUid()).content();
+    String jsonUserString = jsonUser.toString();
+    jsonUserString = jsonUserString.replace("valid.x@email.com", "another.email@com");
+
+    JsonWebMessage msg =
+        assertWebMessage(
+            "Conflict",
+            409,
+            "WARNING",
+            "One or more errors occurred, please see full details in import report.",
+            PUT("/users/" + user.getUid(), jsonUserString).content(HttpStatus.CONFLICT));
+
+    msg.getResponse().find(JsonErrorReport.class, error -> error.getErrorCode() == ErrorCode.E3052);
+  }
 }
diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/MeController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/user/MeController.java
@@ -72,6 +72,7 @@
 import org.hisp.dhis.security.acl.AclService;
 import org.hisp.dhis.security.apikey.ApiToken;
 import org.hisp.dhis.security.apikey.ApiTokenService;
+import org.hisp.dhis.security.twofa.TwoFactorType;
 import org.hisp.dhis.setting.UserSettings;
 import org.hisp.dhis.system.util.ValidationUtils;
 import org.hisp.dhis.user.CredentialsInfo;
@@ -226,6 +227,15 @@ public void updateCurrentUser(
 
     User user = renderService.fromJson(request.getInputStream(), User.class);
 
+    if (currentUser.getTwoFactorType() != null
+        && currentUser.getTwoFactorType().equals(TwoFactorType.EMAIL_ENABLED)
+        && currentUser.isEmailVerified()
+        && user.getEmail() != null
+        && !currentUser.getVerifiedEmail().equals(user.getEmail())) {
+      throw new ConflictException(
+          "Email address cannot be changed, when email-based 2FA is enabled, please disable 2FA first");
+    }
+
     merge(currentUser, user);
 
     if (user.getWhatsApp() != null && !ValidationUtils.validateWhatsApp(user.getWhatsApp())) {