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.

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

feat-be: 동아리 이메일, 비밀번호 정보 수정 #962

Merged
merged 8 commits into from
Jan 1, 2025
Merged
24 changes: 24 additions & 0 deletions backend/src/docs/asciidoc/member.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,27 @@ operation::member/signup-fail/invalid-request[snippets="http-request,request-fie
==== 실패: 인증되지 않은 이메일

operation::member/signup-fail/not-verified-email[snippets="http-request,request-fields,http-response"]

=== 사용자 이메일 변경

==== 성공

operation::member/change-email[snippets="http-request,request-cookies,path-parameters,request-fields,http-response"]

==== 실패: 인증되지 않은 사용자 이메일

operation::member/change-email-fail/not-verified-email[snippets="http-request,request-cookies,path-parameters,request-fields,http-response"]

==== 실패: 적절하지 않은 이메일 형식

operation::member/change-email-fail/invalid-email[snippets="http-request,request-cookies,path-parameters,request-fields,http-response"]

=== 사용자 비밀번호 변경

==== 성공

operation::member/change-password[snippets="http-request,request-cookies,path-parameters,request-fields,http-response"]

==== 실패: 적절하지 않은 비밀번호 형식

operation::member/change-password-fail/invalid-password[snippets="http-request,request-cookies,path-parameters,request-fields,http-response"]
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
package com.cruru.member.controller;

import com.cruru.auth.annotation.RequireAuth;
import com.cruru.auth.annotation.ValidAuth;
import com.cruru.global.LoginProfile;
import com.cruru.member.controller.request.EmailChangeRequest;
import com.cruru.member.controller.request.MemberCreateRequest;
import com.cruru.member.controller.request.PasswordChangeRequest;
import com.cruru.member.domain.Member;
import com.cruru.member.facade.MemberFacade;
import jakarta.validation.Valid;
import java.net.URI;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PatchMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
Expand All @@ -23,4 +31,24 @@ public ResponseEntity<Void> create(@RequestBody @Valid MemberCreateRequest reque
long memberId = memberFacade.create(request);
return ResponseEntity.created(URI.create("/v1/members/" + memberId)).build();
}

@PatchMapping("/{memberId}/email")
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분 동작확인 해보셨나요~?
지금 구현된 AOP 동작을 봤을때 정상동작하지 않을 것 같아요.

  1. 첫번째로 인가 AOP 동작 Joinpoint가 @ValidAuth 이 존재한 컨트롤러 메서드입니다. AOP를 동작시키려면 메서드단에 해당 어노테이션이 추가되어야 합니다.
  2. 인가 AOP 동작은 ArgumentResolver가 넘겨준 LoginProfile을 기준으로 인가를 진행합니다. 현재 컨트롤러는 LoginProfile을 받지 않고 있습니다.
  3. LoginProfile을 통해 찾은 멤버와 @RequiredAuth로 넘겨진 id와 findByIdFetchingMember 메서드를 통해 찾은 맴버를 비교해서 인가를 진행합니다. 현재 MemberRepository에는 해당 메서드가 존재하지 않습니다.

AOP로 인가를 진행하려면 다음 사항이 추가되어야 할 것 같습니다.

  1. @ValidAuth 추가
  2. LoginProfile을 파라미터로 받아와 통해 현재 로그인한 멤버 정보를 받아온다.
  3. MemberRepository에 findByIdFetchingMember 메서드 추가

AOP 인가가 적용된 다른 컨트롤러 보시면 이해하기 쉬울 것 같습니다.!
이번에는 secureResource가 Member 자체라서 어색한 부분이 있긴하네요

Copy link
Member

Choose a reason for hiding this comment

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

적용했습니다!

타사용자가 비밀번호를 변경할 수 있는지의 테스트도 인수테스트 계층으로 추가했습니다.
혹시 다른 분들도 reviewer로 지정하는게 좋을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

인가 작동방식에 대한 공유 목적이라면 좋은것 같아요~

@ValidAuth
public ResponseEntity<Void> changeEmail(
@RequireAuth(targetDomain = Member.class) @PathVariable Long memberId,
@RequestBody @Valid EmailChangeRequest request,
LoginProfile loginProfile) {
memberFacade.changeEmail(request, memberId);
return ResponseEntity.ok().build();
}

@PatchMapping("/{memberId}/password")
@ValidAuth
public ResponseEntity<Void> changePassword(
@RequireAuth(targetDomain = Member.class) @PathVariable Long memberId,
@RequestBody @Valid PasswordChangeRequest request,
LoginProfile loginProfile) {
memberFacade.changePassword(request, memberId);
return ResponseEntity.ok().build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.cruru.member.controller.request;

import jakarta.validation.constraints.Email;

public record EmailChangeRequest(
@Email
String email
) {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.cruru.member.controller.request;

import jakarta.validation.constraints.NotBlank;

public record PasswordChangeRequest(
@NotBlank(message = "비밀번호를 입력해주세요.")
String password
) {

}
8 changes: 7 additions & 1 deletion backend/src/main/java/com/cruru/member/domain/Member.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static com.cruru.member.domain.MemberRole.CLUB_OWNER;

import com.cruru.BaseEntity;
import com.cruru.auth.util.SecureResource;
import com.cruru.member.exception.badrequest.MemberIllegalPhoneNumberException;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
Expand All @@ -22,7 +23,7 @@
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
@Getter
public class Member extends BaseEntity {
public class Member extends BaseEntity implements SecureResource {

private static final Pattern VALID_PHONE_NUMBER_PATTERN = Pattern.compile(
"^(010)\\d{3,4}\\d{4}$|^(02|0[3-6][1-5])\\d{3,4}\\d{4}$");
Expand Down Expand Up @@ -62,6 +63,11 @@ private void validatePhoneNumber(String phoneNumber) {
}
}

@Override
public boolean isAuthorizedBy(Member member) {
return member.id.equals(this.id);
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,26 @@
import com.cruru.member.domain.Member;
import java.util.Optional;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Modifying;
import org.springframework.data.jpa.repository.Query;
import org.springframework.transaction.annotation.Transactional;

public interface MemberRepository extends JpaRepository<Member, Long> {

boolean existsByEmail(String email);

Optional<Member> findByEmail(String email);

@Transactional
@Modifying
Copy link
Contributor

Choose a reason for hiding this comment

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

제가 기억이 잘 안나서 그러는데, 변경감지 말고 jpql로 update 하기로 통일했었나요..?ㅎㅎ 가물가물합니다

Copy link
Member

Choose a reason for hiding this comment

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

저도 가물가물하네요! 저는 가독성이 좋아서 일단 이렇게 작성했습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 통일 관련 얘기는 들은 적이 없지만, 예전에 변경감지를 사용하지 말자는 제안을 초코칩이 했다가 그냥 사라졌던 기억은 있네요. 다시 이야기해보면 좋을 것 같아요.

@Query("UPDATE Member m SET m.email = :email WHERE m.id = :memberId")
void updateEmailById(long memberId, String email);

@Transactional
@Modifying
@Query("UPDATE Member m SET m.password = :password WHERE m.id = :memberId")
void updatePasswordById(long memberId, String password);

@Query("SELECT m FROM Member m WHERE m.id = :id")
Optional<Member> findByIdFetchingMember(Long id);
}
13 changes: 13 additions & 0 deletions backend/src/main/java/com/cruru/member/facade/MemberFacade.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import com.cruru.club.service.ClubService;
import com.cruru.email.service.EmailRedisClient;
import com.cruru.member.controller.request.EmailChangeRequest;
import com.cruru.member.controller.request.MemberCreateRequest;
import com.cruru.member.controller.request.PasswordChangeRequest;
import com.cruru.member.domain.Member;
import com.cruru.member.service.MemberService;
import lombok.RequiredArgsConstructor;
Expand All @@ -25,4 +27,15 @@ public long create(MemberCreateRequest request) {
clubService.create(request.clubName(), savedMember);
return savedMember.getId();
}

@Transactional
public void changeEmail(EmailChangeRequest request, long memberId) {
emailRedisClient.verifyEmail(request.email());
memberService.updateEmail(memberId, request.email());
}

@Transactional
public void changePassword(PasswordChangeRequest request, long memberId) {
memberService.updatePassword(memberId, request.password());
}
}
18 changes: 13 additions & 5 deletions backend/src/main/java/com/cruru/member/service/MemberService.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@ public Member create(MemberCreateRequest request) {
throw new MemberEmailDuplicatedException();
}

String encodedPassword = generateEncodedPassword(request);
String encodedPassword = encodePassword(request.password());
Member newMember = new Member(request.email(), encodedPassword, request.phone());
return memberRepository.save(newMember);
}

private String generateEncodedPassword(MemberCreateRequest request) {
String rawPassword = request.password();
validatePassword(rawPassword);
return passwordValidator.encode(rawPassword);
private String encodePassword(String password) {
validatePassword(password);
Copy link
Contributor

@HyungHoKim00 HyungHoKim00 Dec 24, 2024

Choose a reason for hiding this comment

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

passwordValidator가 존재하는데 검증은 memberService가 하고 encode를 validator가 하는게 뭔가 어색하네요 ㅎㅎ.. 어떻게 생각하시나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

음 이게 클래스명 때문에 더 그렇게 느껴지겠네요. 비밀번호 정책관련 검증은 비즈니스로직이라서 서비스에 위치하고, 단순 인코딩, 인코딩시 동일한지 판단은 유틸적인 기능이라 생각해서 passwordValidator가 담당하고 있었습니다.

return passwordValidator.encode(password);
}

private void validatePassword(String rawPassword) {
Expand All @@ -66,4 +65,13 @@ public Member findByEmail(String email) {
public boolean existsByEmail(String email) {
return memberRepository.existsByEmail(email);
}

public void updateEmail(long memberId, String email) {
memberRepository.updateEmailById(memberId, email);
}

public void updatePassword(long memberId, String password) {
String encodedPassword = encodePassword(password);
memberRepository.updatePasswordById(memberId, encodedPassword);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package com.cruru.member.acceptance;

import static org.mockito.Mockito.doNothing;

import com.cruru.auth.service.AuthService;
import com.cruru.club.domain.repository.ClubRepository;
import com.cruru.email.service.EmailRedisClient;
import com.cruru.member.controller.request.EmailChangeRequest;
import com.cruru.member.controller.request.PasswordChangeRequest;
import com.cruru.member.domain.Member;
import com.cruru.member.domain.repository.MemberRepository;
import com.cruru.util.AcceptanceTest;
import com.cruru.util.fixture.ClubFixture;
import com.cruru.util.fixture.MemberFixture;
import io.restassured.RestAssured;
import io.restassured.http.ContentType;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.mock.mockito.MockBean;

public class MemberAcceptanceTest extends AcceptanceTest {

@MockBean
private EmailRedisClient emailRedisClient;

@Autowired
private MemberRepository memberRepository;

@Autowired
private ClubRepository clubRepository;

@Autowired
private AuthService authService;

private Member member;
private String newToken;

@BeforeEach
void setUp() {
member = memberRepository.save(MemberFixture.DOBBY);
clubRepository.save(ClubFixture.create(member));
newToken = authService.createAccessToken(member.getEmail(), member.getRole()).getToken();
}

@DisplayName("사용자는 다른 사용자의 이메일을 변경할 수 없다.")
Chocochip101 marked this conversation as resolved.
Show resolved Hide resolved
@Test
void userCannotChangeAnotherUsersEmail() {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 실패 케이스들은 문서화하지 않는 이유가 뭔가요? 이 실패 케이스에 어떤 응답이 돌아오는지 프론트 측도 아는 것이 좋을 것 같아서요 ㅎㅎ.

Copy link
Member

Choose a reason for hiding this comment

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

인수(시나리오) 테스트가 API 문서화를 하는지에 대한 개념과 팀 문화가 없어서 저도 고민했던 부분이에요!

문서화하지 않은 이유는

  1. API응답의 모든 것을 문서화하면 문서의 양이 방대해져서 백엔드의 관리가 힘들어진다.
  2. 프론트가 찾아야할 부분이 많다. -> 어느 정도는 문서화하지 않아도 될듯. -> "그럼 어떤 기준으로 문서화하지 않을 것인가?"에 대한 기준은 딱히 없음... 인가는 대부분 API에서 프론트가 직관적으로 알수 있을 것 같아서....
  3. 시나리오 테스트도 문서화 하나요? (진짜 모름)

정도네용

@xogns1514 어떻게 생각하시나용

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 2번 의견에 동의해서 꼭 문서화가 필요한 부분인지 느끼지 못했습니다.
인가부분은 공통적으로 접근하지 못하면 403을 던져주고 있어서, 모든 상황마다 문서를 만들 필요는 없을 것 같다고 생각했습니다.

// given
String changeEmail = "change@email.com";
EmailChangeRequest request = new EmailChangeRequest(changeEmail);
doNothing().when(emailRedisClient).verifyEmail(request.email());

// when&then
RestAssured.given().log().all()
.contentType(ContentType.JSON)
.cookie("accessToken", newToken)
.body(request)
.when().patch("/v1/members/{memberId}/email", defaultMember.getId())
.then().log().all().statusCode(403);
}

@DisplayName("사용자는 다른 사용자의 비밀번호를 변경할 수 없다.")
@Test
void userCannotChangeAnotherUsersPassword() {
// given
String changePassword = "NewPassword123!!";
PasswordChangeRequest request = new PasswordChangeRequest(changePassword);

// when&then
RestAssured.given().log().all()
.contentType(ContentType.JSON)
.cookie("accessToken", newToken)
.body(request)
.when().patch("/v1/members/{memberId}/password", defaultMember.getId())
.then().log().all().statusCode(403);
}
}
Loading
Loading