From 556e65a618d397c731192bdb18c44f0f263ce405 Mon Sep 17 00:00:00 2001
From: John Niang <johnniang@foxmail.com>
Date: Thu, 27 Jun 2024 17:40:54 +0800
Subject: [PATCH] Fix the problem of enabling 2FA unexpectedly (#6174)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#### What type of PR is this?

/kind bug
/area core
/milestone 2.17.x

#### What this PR does / why we need it:

Some users encountered 2FA required issue after upgrading Halo 2.16, because they enabled 2FA but didn't configure TOTP before. The issue was introduced by <https://github.com/halo-dev/halo/pull/6005>.

This PR checks if TOTP configured to determine whether 2FA is required.

#### Does this PR introduce a user-facing change?

```release-note
修复在没有配置 TOTP 验证器的情况下仍被要求二步验证的问题
```
---
 .../security/DefaultUserDetailService.java    |  6 +--
 .../DefaultUserDetailServiceTest.java         | 49 +++++++++++++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/application/src/main/java/run/halo/app/security/DefaultUserDetailService.java b/application/src/main/java/run/halo/app/security/DefaultUserDetailService.java
index 2d7f6ff84..b56e2625e 100644
--- a/application/src/main/java/run/halo/app/security/DefaultUserDetailService.java
+++ b/application/src/main/java/run/halo/app/security/DefaultUserDetailService.java
@@ -22,6 +22,7 @@ import run.halo.app.core.extension.service.UserService;
 import run.halo.app.extension.GroupKind;
 import run.halo.app.infra.exception.UserNotFoundException;
 import run.halo.app.security.authentication.login.HaloUser;
+import run.halo.app.security.authentication.twofactor.TwoFactorUtils;
 
 public class DefaultUserDetailService
     implements ReactiveUserDetailsService, ReactiveUserDetailsPasswordService {
@@ -63,10 +64,9 @@ public class DefaultUserDetailService
                     .doOnNext(userBuilder::authorities);
 
                 return setAuthorities.then(Mono.fromSupplier(() -> {
-                    var twoFactorAuthEnabled =
-                        requireNonNullElse(user.getSpec().getTwoFactorAuthEnabled(), false);
+                    var twoFactorAuthSettings = TwoFactorUtils.getTwoFactorAuthSettings(user);
                     return new HaloUser.Builder(userBuilder.build())
-                        .twoFactorAuthEnabled(twoFactorAuthEnabled)
+                        .twoFactorAuthEnabled(twoFactorAuthSettings.isAvailable())
                         .totpEncryptedSecret(user.getSpec().getTotpEncryptedSecret())
                         .build();
                 }));
diff --git a/application/src/test/java/run/halo/app/security/DefaultUserDetailServiceTest.java b/application/src/test/java/run/halo/app/security/DefaultUserDetailServiceTest.java
index 975a71dbb..a6251fdfb 100644
--- a/application/src/test/java/run/halo/app/security/DefaultUserDetailServiceTest.java
+++ b/application/src/test/java/run/halo/app/security/DefaultUserDetailServiceTest.java
@@ -1,6 +1,10 @@
 package run.halo.app.security;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -107,6 +111,51 @@ class DefaultUserDetailServiceTest {
             .verifyComplete();
     }
 
+    @Test
+    void shouldFindHaloUserDetailsWith2faDisabledWhen2faNotEnabled() {
+        var fakeUser = createFakeUser();
+        when(userService.getUser("faker")).thenReturn(Mono.just(fakeUser));
+        when(roleService.listRoleRefs(any())).thenReturn(Flux.empty());
+        userDetailService.findByUsername("faker")
+            .as(StepVerifier::create)
+            .assertNext(userDetails -> {
+                assertInstanceOf(HaloUserDetails.class, userDetails);
+                assertFalse(((HaloUserDetails) userDetails).isTwoFactorAuthEnabled());
+            })
+            .verifyComplete();
+    }
+
+    @Test
+    void shouldFindHaloUserDetailsWith2faDisabledWhen2faEnabledButNoTotpConfigured() {
+        var fakeUser = createFakeUser();
+        fakeUser.getSpec().setTwoFactorAuthEnabled(true);
+        when(userService.getUser("faker")).thenReturn(Mono.just(fakeUser));
+        when(roleService.listRoleRefs(any())).thenReturn(Flux.empty());
+        userDetailService.findByUsername("faker")
+            .as(StepVerifier::create)
+            .assertNext(userDetails -> {
+                assertInstanceOf(HaloUserDetails.class, userDetails);
+                assertFalse(((HaloUserDetails) userDetails).isTwoFactorAuthEnabled());
+            })
+            .verifyComplete();
+    }
+
+    @Test
+    void shouldFindHaloUserDetailsWith2faEnabledWhen2faEnabledAndTotpConfigured() {
+        var fakeUser = createFakeUser();
+        fakeUser.getSpec().setTwoFactorAuthEnabled(true);
+        fakeUser.getSpec().setTotpEncryptedSecret("fake-totp-encrypted-secret");
+        when(userService.getUser("faker")).thenReturn(Mono.just(fakeUser));
+        when(roleService.listRoleRefs(any())).thenReturn(Flux.empty());
+        userDetailService.findByUsername("faker")
+            .as(StepVerifier::create)
+            .assertNext(userDetails -> {
+                assertInstanceOf(HaloUserDetails.class, userDetails);
+                assertTrue(((HaloUserDetails) userDetails).isTwoFactorAuthEnabled());
+            })
+            .verifyComplete();
+    }
+
     @Test
     void shouldFindUserDetailsByExistingUsernameButKindOfRoleRefIsNotRole() {
         var foundUser = createFakeUser();