Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: 하나의 디바이스를 사용하는 회원 정보가 여러 개일 때, 로그아웃이 정상적으로 되지 않는 버그 해결 #544

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

fix: 하나의 디바이스를 사용하는 회원 정보가 여러 개일 때, 로그아웃이 정상적으로 되지 않는 버그 해결 #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2418a94
fix: 하나의 디바이스를 사용하는 회원 정보가 여러 개일 때, 로그아웃이 정상적으로 되지 않는 버그 해결
BackFoxx Nov 17, 2023
f1d7b33
fix: 깨지는 테스트 해결
BackFoxx Nov 17, 2023
d175c3d
test: authService만 사용하는 테스트를 AuthServiceTest로 이동
BackFoxx Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions backend/src/main/java/edonymyeon/backend/member/application/DeviceRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@

@Repository
public interface DeviceRepository extends JpaRepository<Device, Long> {
Optional<Device> findByDeviceToken(String deviceToken);

@EntityGraph(attributePaths = "member")
List<Device> findAllByDeviceToken(String deviceToken);

Expand Down
2 changes: 1 addition & 1 deletion backend/src/main/java/edonymyeon/backend/member/application/MemberService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public void deactivateOtherDevices(final Member member, final String deviceToken

@Transactional
public void deactivateDevice(final String deviceToken) {
final Optional<Device> device = deviceRepository.findByDeviceToken(deviceToken);
final Optional<Device> device = deviceRepository.findByDeviceTokenAndIsActiveIsTrue(deviceToken);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문제상황이 해결된 것 같군요 굿
그런데 isActive = true인 디바이스 토큰이 두개 이상 있는 경우는 없겠죠...??(정상적인 상황에선 없을 것 같긴 하다만요)

글구 오늘에서야 깨달았는데 갤럭시 보안폴더 이용하면 하나의 핸드폰으로 두개의 이돈이면을 동시에 이용가능 ~ㅋㅋ 디바이스 토큰도 다르더라구여 싱기방기

Copy link
Collaborator

@hectick hectick Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그리고 이건 예외는 발생하지 않는 사항이긴 한데,

보안폴더 내의 이돈이면A와 보안폴더 밖의 이돈이면B에서 같은 계정을 이용할 경우,(좀더 확장하면 A디바이스와 B디바이스)
A에서 계정 로그인 -> B에서 동일 계정 로그인하면
A의 디바이스 토큰이 비활성화되긴 하는데 A에서는 로그아웃되지 않고 디바이스토큰만 비활성화된 상태로 서비스는 이용 가능하더라구여. 즉 최근에 로그인한 B 디바이스로는 알람이 가지만 A에겐 알림이 가지 않는다는 소리.

그래서 생각해본 방안.

1번: B기기로 로그인할 때 A기기를 로그아웃 시킨다
이참에 깔끔하게 B기기로 로그인할 때 A 기기에 대해 강제로 로그아웃시키는 건 어떤가요 호이씨?? 세션저장소를 도입할 생각이 있으시다면 어케 잘 해볼 수 있지 않을까요?? 😁😁😁😁😁😁 @This2sho

머 저희가 보내는 알람이 중대한 알림은 아니라 알람만 안오는상태로 이용해도 괜찮다고 생각하긴 합니다만.... 애써 만든 알림이 무용지물 되는 것은 슬프자나여~

2번: 하나의 계정에서 여러 기기에 대해 디바이스 토큰을 활성화할 수 있게 한다
지금은 로그인시, 하나의 회원에 대하여 최근 로그인한 기기말곤 다른 기기들의 디바이스 토큰을 비활성화해주고 있는데요! (아마 예상치 못하게 디바이스 토큰이 비활성화되지 못하고 로그아웃당한 상황이나 세션만료된 상황을 염두해둔게 아닐까싶습니당)

(1) 자의적으로 기존 기기에 대한 로그인을 유지한채 다른 기기로 로그인해서 두 기기에 대해 디바이스 토큰을 활성화유지하는 것과
(2) 로그인하고 있던 기기가 세션만료당한 후 다른 기기로 다시 로그인해서 기존 세션만료당한 기기에 대한 디바이스 토큰을 비활성화 시키는 것
이거 두개를 깔끔히 구분해서 처리할 마땅한 방안이 떠오르지 않기도 하구여..
사용자가 명시적으로 "나 로그아웃 할래!"라고 한 디바이스만 잘 비활성화시켜 주면, 로그인 시 동일계정의 다른 기기들(예를들어 세션만료당한 기기)을 애써 비활성화할 필요가 없다는 생각이 들기도 하네용(세션이 만료되었어도 알림으로 자꾸 앱 들어와라 유인할 수 있긴 하니ㅎㅎㅎ)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 보안폴더를 사용하면 그런 종류의 실험도 가능하군요 ...??? 전 어릴때 네이버웹툰 보는 거 안 들키려고 보안폴더 써본 기억밖에 없는데 ㅋㅋㅋㅋㅋㅋ
생각해보니 JWT 사용을 극구 반대하면서 들었던 근거가 '세션을 사용해서 원격 로그아웃 등을 구현하게 될 가능성이 높다' 였는데, 막상 이 기능을 실제로 구현해보진 않은 것 같아요.
한 디바이스에서만 알림을 받도록 하는 것보다 아예 로그아웃을 시켜버리는 게 더 깔끔하겠죠? 이슈를 한 번 생성해 보겠습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 동일한 디바이스 토큰은 반드시 하나만 활성화되어 있도록 애플리케이션 단에서 설계했기 때문에, 그 걱정은 하지 않아도 될 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

케로&이리내 : 로그아웃을 할 때 토큰을 비활성화하는 것보다 그냥 삭제하는 편이 좋다고 생각한다. 왜냐하면 동일한 디바이스로 로그인하더라도 캐시를 삭제하거나 앱을 재설치하는 경우 다른 디바이스 토큰을 할당받기 때문이다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 논의에서 2번을 채택하기로 한다.

  • 사용자가 명시적으로 로그아웃을 하지 않는 이상 여러 디바이스에도 알림을 발송할 수 있다.
  • 디바이스에서 활성/비활성 여부를 없앤다.
  • 명시적으로 로그아웃하는 경우 그냥 디바이스 토큰 정보를 물리적 삭제한다.

device.ifPresent(Device::deactivate);
}
}
59 changes: 34 additions & 25 deletions backend/src/test/java/edonymyeon/backend/auth/application/AuthServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,5 @@
package edonymyeon.backend.auth.application;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import edonymyeon.backend.auth.application.dto.JoinRequest;
import edonymyeon.backend.auth.application.dto.KakaoLoginResponse;
import edonymyeon.backend.auth.application.dto.LoginRequest;
Expand All @@ -32,7 +19,6 @@
import edonymyeon.backend.setting.application.SettingService;
import edonymyeon.backend.support.IntegrationTest;
import jakarta.persistence.EntityManager;
import java.util.Optional;
import lombok.RequiredArgsConstructor;
import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.DisplayNameGeneration;
Expand All @@ -43,7 +29,16 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.test.context.jdbc.Sql;

import java.util.List;
import java.util.Optional;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.awaitility.Awaitility.await;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.mockito.Mockito.*;

@DisplayNameGeneration(ReplaceUnderscores.class)
@SuppressWarnings("NonAsciiCharacters")
Expand Down Expand Up @@ -128,11 +123,11 @@ class AuthServiceTest {
//then
assertSoftly(
softAssertions -> {
final Device originalDevice = deviceRepository.findByDeviceToken(joinRequest.deviceToken()).get();
final Optional<Device> changedDevice = deviceRepository.findByDeviceToken(loginRequest.deviceToken());
assertThat(changedDevice.isPresent()).isTrue();
assertThat(changedDevice.get().isActive()).isTrue();
assertThat(originalDevice.isActive()).isFalse();
final List<Device> originalDevice = deviceRepository.findAllByDeviceToken(joinRequest.deviceToken());
final List<Device> changedDevice = deviceRepository.findAllByDeviceToken(loginRequest.deviceToken());
assertThat(changedDevice).hasSize(1);
assertThat(changedDevice.get(0).isActive()).isTrue();
assertThat(originalDevice.get(0).isActive()).isFalse();
}
);
}
Expand All @@ -150,11 +145,11 @@ class AuthServiceTest {
//then
assertSoftly(
softAssertions -> {
final Device originalDevice = deviceRepository.findByDeviceToken(originalDeviceToken).get();
final Optional<Device> changedDevice = deviceRepository.findByDeviceToken(changedDeviceToken);
assertThat(changedDevice.isPresent()).isTrue();
assertThat(changedDevice.get().isActive()).isTrue();
assertThat(originalDevice.isActive()).isFalse();
final List<Device> originalDevice = deviceRepository.findAllByDeviceToken(originalDeviceToken);
final List<Device> changedDevice = deviceRepository.findAllByDeviceToken(changedDeviceToken);
assertThat(changedDevice).hasSize(1);
assertThat(changedDevice.get(0).isActive()).isTrue();
assertThat(originalDevice.get(0).isActive()).isFalse();
}
);
}
Expand Down Expand Up @@ -241,4 +236,18 @@ class AuthServiceTest {
}
);
}

@Test
void 동일한_디바이스_정보가_여러_회원에_걸쳐_존재할_때에도_로그아웃이_정상_동작해야_한다() {
// Given : 회원이 새로 가입하여 로그인한 후 로그아웃한다.
authService.joinMember(new JoinRequest(("[email protected]"), "password123!", "nickname", "sameDevice"));
authService.login(new LoginRequest("[email protected]", "password123!", "sameDevice"));
authService.logout("sameDevice");

// When : 동일한 디바이스를 가지고 새로운 회원이 가입하여 로그인한 후 로그아웃한다.
authService.joinMember(new JoinRequest(("[email protected]"), "password123!", "nickname2", "sameDevice"));
authService.login(new LoginRequest("[email protected]", "password123!", "sameDevice"));
assertThatCode(() -> authService.logout("sameDevice"))
.doesNotThrowAnyException();
}
}
18 changes: 8 additions & 10 deletions backend/src/test/java/edonymyeon/backend/member/application/MemberServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
package edonymyeon.backend.member.application;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.awaitility.Awaitility.await;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.verify;

import edonymyeon.backend.auth.application.AuthService;
import edonymyeon.backend.auth.application.dto.JoinRequest;
import edonymyeon.backend.auth.application.dto.KakaoLoginResponse;
Expand All @@ -22,14 +13,21 @@
import edonymyeon.backend.member.repository.MemberRepository;
import edonymyeon.backend.post.ImageFileCleaner;
import edonymyeon.backend.support.IntegrationFixture;
import java.io.IOException;
import lombok.RequiredArgsConstructor;
import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.mock.web.MockMultipartFile;

import java.io.IOException;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.awaitility.Awaitility.await;
import static org.mockito.Mockito.*;

@SuppressWarnings("NonAsciiCharacters")
@RequiredArgsConstructor
class MemberServiceTest extends IntegrationFixture implements ImageFileCleaner {
Expand Down