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

Feature/member security #20 #48

Closed
wants to merge 15 commits into from
Closed

Conversation

HiSeungmin
Copy link
Collaborator

@HiSeungmin HiSeungmin commented Jan 7, 2025

close: #20
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!

✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)

  • 기능 추가
  • 리뷰 반영 수정
  • 리팩토링
  • 버그 수정
  • 컨벤션 수정
  • 브랜치 최신화

🏆 구현 목표

security & jwt 설정


📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)

  1. security 설정 ( security 모든 요청 허용 )
  2. jwt 토큰 설정 개발

*Controller와 Service부분은 jwt 토큰 테스트로 임의로 만들어둔 부분입니다.
추후에 로그인 API 개발할 때 수정할 예정입니다.

@HiSeungmin HiSeungmin added the ✨ Feature 기능 개발 label Jan 7, 2025
@HiSeungmin HiSeungmin self-assigned this Jan 7, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

// Implementation and JSON support (runtime dependencies)
runtimeOnly 'io.jsonwebtoken:jjwt-impl:0.11.5'
runtimeOnly 'io.jsonwebtoken:jjwt-jackson:0.11.5'

compileOnly 'org.projectlombok:lombok'
developmentOnly 'org.springframework.boot:spring-boot-devtools'
runtimeOnly 'com.mysql:mysql-connector-j'
Copy link

Choose a reason for hiding this comment

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

코드 패치를 리뷰하겠습니다. 다음은 주목해야 할 사항들과 개선 방안입니다.

  1. 주석 처리된 코드:

    • implementation 'org.thymeleaf.extras:thymeleaf-extras-springsecurity6'가 주석 처리되었습니다. 만약 Spring Security와 Thymeleaf를 같이 사용할 계획이라면 이 의존성이 필요할 수 있습니다. 필요한지 여부를 분석하고, 필요하다면 제거하거나 설정해야 합니다.
  2. Security 의존성 추가:

    • org.springframework.boot:spring-boot-starter-security를 추가한 것은 보안 강화에 도움이 됩니다. 하지만, 추가 후 보안 설정이 제대로 필요합니다. 이 부분에 대한 설정이 미비하다면 실제로 보안이 강화되지 않을 수 있으므로 주의해야 합니다.
  3. JWT 라이브러리:

    • JWT 관련 의존성을 추가한 것은 좋지만, JWT를 사용할 경우 적절한 토큰 생성 및 검증 로직이 구현되어야 합니다. 추가된 라이브러리의 사용에 대한 예제나 문서도 확인하는 것이 좋습니다.
  4. 버전 관리:

    • 의존성의 버전을 하드코딩했지만, 향후 라이브러리의 업데이트로 인한 호환성 문제가 발생할 수 있습니다. Gradle의 BOM이나 의존성 관리를 통해 버전 관리를 더 효율적으로 할 수 있습니다.
  5. 구성 최적화:

    • compileOnlyruntimeOnly의 사용은 적절한 수준으로 보이지만, 필요에 따라 이들을 implementation으로 바꿔야 할 경우도 있습니다. 예를 들어, 애플리케이션이 이 라이브러리에 의존하지 않도록 설계할 수 있습니다.
  6. 테스트:

    • 새로운 의존성을 추가한 후, 모든 기능이 정상적으로 작동하는지 확인하기 위한 테스트가 필요합니다. 보안 관련 기능이 추가되면 더더욱 테스트를 통해 문제가 없음을 확인해야 합니다.

전반적으로 의존성 추가와 보안 강화를 위한 좋은 변화입니다. 하지만 추가된 기능을 제대로 사용할 수 있도록 설정 및 테스트가 중요합니다.

public String userSettingPage() {
return "user_setting";
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 진행하겠습니다. 몇 가지 버그 위험 요소와 개선 사항을 제안합니다.

1. memberController 클래스 이름

  • 클래스 이름은 Java의 관례에 따라 대문자로 시작하는 것이 좋습니다. memberController보다는 MemberController로 변경하는 것이 좋습니다.

2. 로그인 로직 수정

  • ObjectUtils.isEmpty(member) 조건문에서 get() 메서드를 사용하고 있습니다. Optional을 사용하고 있다면 isPresent()를 통해 존재 여부를 먼저 체크한 후 값을 가져오는 방식이 더 안전합니다. 현재 논리에서는 존재하지 않는 사용자가 로그인 시도할 경우 NoSuchElementException이 발생할 수 있습니다.

    Optional<Member> optionalMember = memberService.getUserWithAuthorities(dto.getUserid());
    if (!optionalMember.isPresent()) {
        // 사용자 존재하지 않음
    }

3. 에러 처리

  • 로그인 실패 시 HttpStatus.BAD_REQUEST를 반환하고 있습니다. 그러나 이는 잘못된 요청을 의미하므로 HttpStatus.UNAUTHORIZED(401) 또는 HttpStatus.FORBIDDEN(403)과 같은 상태 코드가 더 적절할 수 있습니다.

4. 예외 처리

  • join 메서드에서 예외 처리 시 모든 예외를 처리하고 있습니다. 잘못된 입력에 대한 유효성 검사나 특정 예외를 처리하면 더 구체적이고 이해하기 쉬운 오류 메시지를 제공할 수 있습니다.

5. DTO 검증

  • join 메서드에서는 DTO에 대한 검증이 없습니다. @Valid와 같은 어노테이션을 사용하여 DTO 검증을 수행하는 것이 좋습니다.

6. API 응답 일관성

  • 로그인 및 회원가입 API의 응답 구조가 다릅니다. 동일한 형식의 DTO를 사용하여 일관된 응답 구조를 유지하는 것이 좋습니다.

7. 보안 고려

  • 비밀번호를 직접 DTO에서 가져오는데, 비밀번호는 대칭 암호화된 형태로 저장되고 처리되어야 합니다. 암호화 관련 처리를 추가하는 것이 좋습니다.

8. 주석 및 문서화

  • 주석이 있지만 더 명확하게 동작을 설명하는 것이 좋습니다. 특히 비즈니스 로직에서 중요한 결정 사항을 명시하는 것이 좋습니다.

이러한 부분들을 고려하면 코드의 안정성과 가독성을 높일 수 있습니다. 추가적인 질문이 있으시면 말씀해 주세요!

@NotBlank
@Size(min = 3, max = 50)
private String nickname;
}
Copy link

Choose a reason for hiding this comment

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

코드 리뷰 결과를 아래와 같이 작성하였습니다.

  1. 의존 라이브러리 확인:

    • com.fasterxml.jackson.annotation.JsonPropertyjakarta.validation.constraints 패키지를 통해 외부 라이브러리에 의존하고 있습니다. 이 라이브러리들이 프로젝트에 잘 포함되어 있는지 확인해야 합니다.
  2. 비즈니스 로직 검토:

    • MemberDto가 단순한 DTO(Data Transfer Object)인 점을 감안할 때, 필드 유효성 검증을 잘 수행하고 있습니다. 특히 @NotBlank@Size 애노테이션을 통해 입력값을 적절히 검증하고 있습니다.
  3. JsonProperty 설정:

    • password 필드에 @JsonProperty(access = JsonProperty.Access.WRITE_ONLY)를 사용하여 JSON 직렬화 시 비밀번호가 노출되지 않도록 하였습니다. 이는 보안상 좋은 접근입니다. 그러나 이 필드가 반드시 저장되어야 하는 경우, 시스템 내에서 어떻게 관리할지에 대한 방안도 고민해볼 필요가 있습니다.
  4. 명명 규칙:

    • username, password, nickname이라는 필드 명칭은 일관성이 있어 읽기 쉽습니다. 그러나 프로젝트의 다른 DTO와의 일관성을 검토하여 필요시 변경할 수 있을 것입니다.
  5. 추가적인 검증:

    • 현재 각 필드에 대해 기본적인 유효성 검증만 존재합니다. 만약 username이나 nickname에 대해 유일성 검증이 필요한 경우, 별도의 로직 추가가 필요할 수 있습니다.
  6. 불필요한 애노테이션 제거:

    • @Setter 애노테이션을 사용하면 객체의 값을 수정할 수 있습니다. 만약 DTO가 불변 객체로 사용될 계획이라면 @Setter를 제거하고 @Value로 변경할 수 있습니다.
  7. 주석 추가:

    • 필드에 대한 설명이나 사용 목적을 간단히 주석으로 추가하면 코드 가독성이 향상될 수 있습니다. 특히 나중에 코드를 유지보수할 때 큰 도움이 됩니다.

결론적으로, 이 코드 패치에서는 전반적으로 좋은 설계와 구현이 이루어져 있습니다. 위에 언급된 사항들을 고려하여 필요시 개선할 수 있을 것입니다.


private String userid;
private String pw;
}
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 하겠습니다.

  1. 패키지 구조: 패키지 이름 com.seveneleven.devlens.domain.member.dto는 적절해 보입니다. 하지만, 패키지의 의미가 명확하게 드러나는지 검토해볼 필요가 있습니다.

  2. Lombok 사용: Lombok을 사용하여 getter와 setter를 자동으로 생성하는 것은 코드의 간결성을 높이는 좋은 접근입니다. 그러나 Lombok을 사용하는 경우, 팀 내에서 Lombok을 사용하는 것에 대한 합의가 필요합니다. 만약 다른 팀원이 Lombok 보다는 명시적인 getter/setter 패턴을 선호한다면 혼란을 초래할 수 있습니다.

  3. 변수명: useridpw는 직관적이지만, 변수명을 조금 더 구체적으로 설정할 수 있습니다. 예를 들어, userIdpassword로 변경하면 코드의 가독성과 이해도를 높일 수 있습니다.

  4. 유효성 검사: 현재 코드에서는 useridpw의 내용이 유효한지 확인하는 로직이 없습니다. 이러한 유효성 검사를 추가하여 null 체크, 길이 제한, 형식 확인 등을 수행하는 것이 좋습니다.

  5. 암호화: pw 같은 비밀번호는 퍼블릭 필드로 결정하는것은 보안 상의 위험이 있습니다. 비밀번호는 암호화하여 저장하는 것을 고려해야 하며, DTO에서 암호화 및 복호화 로직을 포함하지 않도록 주의해야 합니다.

  6. 딱딱한 DTO: DTO에서 비즈니스 로직을 처리하는 것은 권장되지 않으므로, 필요한 경우 DTO에 대한 검사나 변환은 서비스 계층에서 처리하는 것이 좋습니다.

이 외에도 코드 문서화 및 주석을 추가하는 것이 다른 개발자들이 이해하는 데 도움이 될 수 있습니다. 전체적으로 보았을 때, 기본적인 구조는 괜찮지만 안전성과 가독성을 높이기 위한 몇 가지 개선 제안을 고려해보시기 바랍니다.

public class TokenDto {

private String token;
}
Copy link

Choose a reason for hiding this comment

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

코드 패치를 검토해본 결과, 전반적으로 잘 작성된 코드입니다. 그러나 몇 가지 개선 사항과 버그 위험 요소를 제안드리겠습니다.

  1. 도메인 주석 추가: TokenDto 클래스의 역할에 대한 설명이 없기 때문에, 클래스 위에 간단한 주석을 추가하여 어떤 용도로 사용되는지 명확히 하는 것이 좋습니다.

  2. 접근 제어자: 현재 필드 token은 기본 접근 제어자인 package-private로 설정되어 있습니다. 다른 패키지에서 접근할 필요가 없는 경우, private로 한정하는 것이 좋습니다.

  3. 필드 유효성 검사: token 필드는 빈 문자열이나 null일 수 있습니다. 이러한 경우에 대한 유효성 검사 로직을 추가하면 좋을 것입니다. 예를 들어, 적어도 하나의 값이 필요하다면, 생성자나 빌더에서 검사를 시행할 수 있습니다.

  4. @tostring 어노테이션: @ToString 어노테이션을 추가하면 객체 내용을 로그에 출력할 때 유용할 수 있습니다. 그러나 보안상의 이유로 token을 로그에 출력하는 것을 피해야 할 경우, 해당 필드를 제외하는 설정도 필요합니다.

  5. 불변성 고려: TokenDto 객체가 불변 객체로 사용될 필요가 있다면, @Setter를 제거하고 생성자를 통해 초기화될 수 있도록 하면 좋습니다. 이러한 방식은 객체의 상태 변경을 방지하여 안전성을 높일 수 있습니다.

이러한 점들을 고려하여 코드를 조금 더 개선할 수 있을 것입니다.

return false;
}
}

Copy link

Choose a reason for hiding this comment

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

이 코드는 JWT 토큰을 생성하고 검증하는 기능을 제공하는 TokenProvider 클래스입니다. 아래는 코드 리뷰입니다.

잠재적인 버그 리스크

  1. 비밀키 관리: 비밀키가 외부에 노출될 위험이 있음. @Value("${jwt.secret}")로 환경 설정에서 로드되지만, 이를 안전하게 관리하는 것이 중요합니다.
  2. 예외 처리: JWT 검증 시 다양한 예외를 잡고 로깅만 하고 있습니다. 애플리케이션의 오류 처리 전략에 따라, 이러한 예외에 대한 사용자 친화적인 피드백을 고려해야 할 수 있습니다.
  3. Claim의 타입 관리: AUTHORITIES_KEY에서 얻은 접근 권한이 항상 비어있지 않을 것이라는 가정이 있습니다. 만약 권한이 없다면, Arrays.stream()에서 NullPointerException이 발생할 수 있습니다. 이를 방지하기 위해 기본 값을 설정하거나 체크가 필요합니다.

개선 사항

  1. 성능 최적화: getAuthentication 메서드에서 claims.get(AUTHORITIES_KEY)의 결과로 toString()을 호출하고 있습니다. 대체로 Map에서 직접 Collection으로 가져오는 방법을 고려해 볼 수 있습니다. 예를 들어, List<GrantedAuthority>로 직접 가져오는 방법이 좋습니다.
  2. 제네릭 타입 사용: Collection<? extends GrantedAuthority> 대신에 Collection<GrantedAuthority>를 사용하면 타입 안정성이 증가합니다.
  3. 비밀번호 체크: User 객체를 생성할 때 비밀번호를 빈 문자열로 설정하고 있습니다. 실제 애플리케이션에서는 비밀번호를 필요로 하지 않을 수 있지만, 사용자가 정의한 사용자 객체를 사용하는 것이 더 안전할 수 있습니다.
  4. 메서드 문서화: 각 메서드에 대한 JavaDoc 주석은 잘 작성되어 있으나, 예외 발생 가능성이나 각 입력값의 설명을 추가하면 좋습니다.

보안 고려 사항

  • JWT 서명의 알고리즘이 HS512로 설정되어 있습니다. 경우에 따라 추가적인 보안 요구사항(예: 알고리즘 변경 가능성)을 고려하여 필요 시 메서드에 파라미터로 받도록 하거나 설정 파일로부터 동적으로 로드할 수 있습니다.
  • JWT의 유효 기간(tokenValidityInMilliseconds) 설정이 유연하게 변경 가능하므로, 필요 시 환경 변수나 설정 파일에서 이 값을 조정할 수 있는지 확인하고 문서화해야 합니다.

종합적으로, 코드 구조는 잘 작성되어 있으나, 보안, 성능, 예외 처리 등의 측면에서 개선할 점이 있습니다.

@@ -2,7 +2,6 @@

import com.seveneleven.devlens.global.response.APIResponse;
import com.seveneleven.devlens.global.response.ErrorCode;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.bind.annotation.RestControllerAdvice;
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.

  1. 불필요한 임포트 제거: org.springframework.http.HttpStatus 클래스가 제거되었습니다. 만약 코드에서 이 클래스를 사용하지 않는다면, 제거하는 것은 좋은 선택입니다. 하지만 이후에 HttpStatus를 사용할 필요가 있다면 다시 임포트해야 합니다.

  2. 예외 처리: 패치에서 @ExceptionHandler@RestControllerAdvice를 사용하는 맥락이 보이지 않아서 확실하게 확인할 수 없지만, 예외 처리 로직이 제대로 구현되어 있는지 확인해야 합니다. 예외 처리가 누락되면 애플리케이션 안정성에 영향을 미칠 수 있습니다.

  3. API 응답 처리: APIResponseErrorCode를 사용하는 부분에 대한 구체적인 코드가 보이지 않지만, 이들이 올바르게 사용되고 있는지, 또한 적절한 오류 코드를 반환하고 있는지 검토할 필요가 있습니다.

  4. 주석 및 문서화: 코드 내에 주석이 없는데, 특정한 처리 방식이나 비즈니스 로직이 복잡할 경우 주석을 추가하여 코드의 가독성을 높이는 것이 좋습니다.

  5. 테스트 코드: 예외 처리 및 응답 형식에 대한 테스트가 충분히 이루어지고 있는지 확인하는 것이 좋습니다. 테스트가 누락된다면, 새로운 버그가 발생할 위험이 증가합니다.

이상의 점들을 고려하여 코드를 점검하고 수정하는 것이 좋겠습니다. 추가적인 코드나 컨텍스트가 주어지면 더 구체적인 피드백을 줄 수 있습니다.

COMPANY_DUPLICATED_NUMBER(1051, HttpStatus.BAD_REQUEST,"이미 등록된 회사입니다."),
COMPANY_IS_DEACTIVATED(1052, HttpStatus.BAD_REQUEST,"비활성화된 회사 정보입니다."),
COMPANY_IS_NOT_FOUND(1053,HttpStatus.BAD_REQUEST,"회사 정보를 찾을 수 없습니다."),

// 2000번대 코드 : 프로젝트 관련


Copy link

Choose a reason for hiding this comment

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

코드 리뷰를 하겠습니다.

  1. 새로운 에러 코드 추가: 새로 추가된 FORBIDDEN, NOT_FOUND_TOKEN, EXPIRED_TOKEN, JWT_FILTER_ERROR 에러 코드는 명확한 의미를 가지고 있으므로 좋습니다. 하지만 각 코드의 의미와 사용 범위를 주석으로 추가하면, 다른 개발자들에게 더욱 도움이 될 것입니다.

  2. HTTP 상태 코드: NOT_FOUND_TOKEN, EXPIRED_TOKEN, JWT_FILTER_ERROR가 모두 HttpStatus.INTERNAL_SERVER_ERROR(500)로 설정되어 있습니다. 이 상태 코드는 서버의 문제를 나타내는데, JWT 관련 오류는 클라이언트의 문제일 수 있습니다. 예를 들어, 토큰이 유효하지 않거나 만료되었을 경우 HttpStatus.UNAUTHORIZED(401) 또는 HttpStatus.FORBIDDEN(403) 같은 코드가 더 적절할 수 있습니다.

  3. 주석 사용: 코드에 주석을 추가하여 각 에러 코드의 의도를 설명하면 읽기 쉬워집니다.

  4. 확장성: 만약 추가적인 에러 코드가 필요할 경우, 이후에 유지 관리가 쉬운 구조인지 확인하는 것이 좋습니다. 에러 코드가 많아질 경우, 카테고리를 나누거나 다른 클래스로 분리하는 방안을 고려해볼 수 있습니다.

  5. 유지 보수성: 에러 코드와 메시지를 관리하는 방법(예: enum을 갖고 있는 한 파일) 외에도, JSON이나 XML 같은 외부 파일로 관리하면 운영 환경에서 수정하기가 용이할 수 있습니다.

전반적으로 코드는 잘 구조화되어 있으며 새로운 에러 코드를 적절하게 추가하였습니다. 위에서 언급한 개선점을 고려해보면 더욱 나은 품질의 코드가 될 것입니다.


return Optional.ofNullable(username);
}
}
Copy link

Choose a reason for hiding this comment

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

이 코드 패치는 SecurityUtil 클래스에 대한 추가 및 개선을 다루고 있습니다. 다음은 코드 검토 및 개선 사항입니다.

  1. 문서화: 클래스 및 메소드에 대한 주석이 잘 작성되어 있습니다. 다만, 메소드에 대해 고유한 사례나 예시를 추가하면 코드 이해에 더욱 도움이 될 수 있습니다.

  2. 로그 레벨: 현재 로그 레벨을 debug로 설정한 것은 의미가 있습니다. 그러나 이 정보가 실제로 문제가 발생했을 때 적절히 기록되도록 고려해야 할 수 있습니다. 따라서, logger.warn이나 logger.error를 사용하는 것도 생각해볼 수 있습니다.

  3. Optional 사용: Optional.ofNullable(username)을 사용하는 것은 올바른 패턴입니다. 하지만, 이름이 username으로 명확하게 지정되었으므로 nullable을 다루는 대신, username이 없는 경우 어떻게 처리해야 하는지를 명시하는 경우가 좋습니다.

  4. 이중 조건 검사: getPrincipal의 결과를 두 번 검사하고 있기 때문에 코드가 조금 복잡해질 수 있습니다. 이를 개선하기 위해, Optional을 사용한 일관된 접근 방식을 고려하면 좋을 것입니다.

  5. 타입 안전성: authentication.getPrincipal()에서 반환되는 타입을 검사하는 방식은 좋으나, 새로운 타입이 추가될 경우 추가적인 유지 관리가 필요할 수 있습니다. 만약 특정 타입을 제한적으로 다루도록 요구된다면, 이 부분을 재구성할 수 있을 것입니다.

  6. 스레드 안전성: 해당 코드가 멀티스레드 환경에서 호출될 수 있다면, SecurityContextHolder의 사용이 문제가 될 수 있습니다. 현재 스레드와 관련된 보안 컨텍스트를 적절히 관리하는 방법에 대해 보증해야 합니다.

  7. Null 검사: getCurrentUsername 메소드에서 username이 null인 경우엔 Optional.empty()를 반환하게 되어 있습니다. 이 처리는 유효하나, 두 개의 경우에 대한 조건문이 모두 통과하지 않을 경우의 처리를 다시 한 번 고민해 볼 수 있습니다.

결론적으로, 이 코드는 주석과 함께 이해하기 쉽게 잘 작성되었습니다. 여기에 위에서 언급한 몇 가지 제안을 추가한다면, 더욱 강력하고 유지 관리하기 쉬운 코드가 될 것입니다.

jwt.token-validity=${SPRING_JWT_TOKEN_VALIDITY}

# Security on/off
spring.security.enabled=false No newline at end of file
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 검토 결과입니다. 일부 버그 위험 및 개선 사항을 아래에 제안합니다.

  1. DDL 설정 변경:

    • spring.jpa.hibernate.ddl-autocreate에서 update로 변경한 것은 데이터베이스 스키마 관리에 있어 안전성을 높입니다. 그러나 이 설정은 운영 환경에서는 주의가 필요합니다. 데이터베이스 스키마 충돌이나 데이터 손실 가능성이 있을 수 있으므로, 중요한 운영 환경에서는 validate 또는 none으로 설정하는 것이 좋습니다.
  2. JWT 설정:

    • jwt.secretjwt.token-validity를 추가하는 것은 보안 측면에서 긍정적입니다. 그러나 이 변수들에 대한 기본 값을 명시적으로 제공하거나 초기화하는 방법을 추가하는 것이 좋습니다. 그렇지 않으면 환경 변수에 적절한 값이 설정되지 않을 경우 애플리케이션이 예상치 못한 동작을 할 수 있습니다.
  3. 보안 설정:

    • spring.security.enabled=false로 설정한 것은 보안 기능을 비활성화하는 것입니다. 이는 테스트 환경에서는 괜찮을 수 있지만, 프로덕션 환경에서는 심각한 보안 결함을 초래할 수 있습니다. 보안을 항상 활성화하고, 필요한 경우 특정 설정을 조정하는 것이 더 안전합니다.
  4. 주석 추가:

    • 코드에 주석이 잘 추가되어 있어 이해가 쉬워졌습니다. 그러나 JWT 및 보안 설정에 대한 주석을 좀 더 명확하게 추가하여 이 설정의 목적과 환경에 따른 권장 사항을 설명하는 것이 좋습니다.
  5. 구조적 일관성:

    • 설정 파일 끝에 새로운 값이 추가되었지만, 마지막 줄에 줄 바꿈이 없습니다. 이를 추가하여 파일 형식의 일관성을 유지하는 것이 좋습니다.

위의 내용들을 참고하여 코드 품질과 보안을 향상시키시기 바랍니다.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

@@ -22,6 +22,7 @@ jobs:
AWS_REGION: ${{ secrets.AWS_REGION }}
AWS_ACCESS_KEY: ${{ secrets.AWS_ACCESS_KEY }}
AWS_SECRET_KEY: ${{ secrets.AWS_SECRET_KEY }}
SPRING_JWT_TOKEN_VALIDITY: ${{ secrets.SPRING_JWT_TOKEN_VALIDITY }}

permissions:
pull-requests: write
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 아래에 제공합니다:

  1. 변수 추가에 대한 확인: SPRING_JWT_TOKEN_VALIDITY 변수가 비밀 정도에 적합한지 검토해보세요. JWT 토큰의 유효성을 설정하는 값이라면, 보안적으로 중요한 정보일 수 있습니다. 이 값이 잘 보호되고 있는지 확인이 필요합니다.

  2. 주석 추가: 새로운 환경 변수가 추가된 이유와 그 사용법에 대한 주석을 코드에 추가하면, 이후에 코드를 이해하는 데 도움이 될 것입니다.

  3. 테스트 체크: 이 변수를 사용하는 코드 또는 워크플로우가 제대로 동작하는지 테스트가 필요합니다. 특히 JWT 관련 코드에는 보안 결함이 발생할 수 있으므로, 유효 기간 설정이 올바르게 작동하는지 확인하세요.

  4. 베스트 프랙티스: 환경 변수는 정해진 형식으로 제공되는지 확인하고, 가능한 경우 데이터 형식 검사를 수행하여 유효성을 높이는 것이 좋은 방법입니다.

  5. 버전 관리: 만약 이 수정 사항이 중요한 변경이라면, 이를 설명하는 커밋 메시지를 추가하는 것이 좋습니다. 변경 사항에 대한 문서화는 협업에 큰 도움이 됩니다.

이 외에도 추가적인 코드 패턴이나 전체적인 구조와 관련된 개선점이 있을 수 있으니, 필요한 경우 심도 있는 코드 리뷰도 고려해보세요.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT


/*
* security, jwt 테스트 확인을 위해 임시로 만든 dto
*
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 진행하겠습니다.

  1. 코드 주석:

    • 주석이 코드의 목적을 설명하기 위해 추가되었습니다. 그러나 주석이 임시로 만든 DTO에 관한 것이라면, 이는 코드의 유지보수성에 영향을 줄 수 있습니다. 임시 코드는 나중에 필요에 따라 업데이트하거나 삭제하는 것이 좋습니다.
    • "임시로 만든 dto"라는 표현은 다소 모호할 수 있습니다. "dto"의 구체적인 목적이나 사용처를 명확히 하는 주석이 필요합니다.
  2. 주석 스타일:

    • 여러 줄 주석으로 작성되었는데, 주석 내용이 간단한 경우 한 줄 주석으로 간단히 설명하는 것이 더 깔끔할 수 있습니다. 예를 들어:
      // Security 및 JWT 테스트를 위한 임시 DTO
  3. 주석 언어:

    • 코드 주석은 한국어로 작성되어 있어 한국어 사용자에게는 이해하기 쉽습니다. 하지만 팀의 다국적 환경을 고려하여 영어로 작성하는 것도 좋은 선택일 수 있습니다.
  4. Lombok 어노테이션:

    • Lombok의 @Getter, @Setter, @Builder 어노테이션이 적절하게 사용되었습니다. 그러나 데이터 클래스의 변수가 어떤 것이며, 기본 생성자나 equals(), hashCode() 메서드 등이 필요하면 이러한 기능을 위해 추가적인 Lombok 어노테이션을 고려할 수 있습니다.
  5. 유효성 검사:

    • jakarta.validation.constraints.Size가 임포트되어 있지만, 이를 해당 클래스 내에서 사용하는 모습이 보이지 않습니다. 실제로 DTO가 어떻게 사용될지를 고려하여 필요없는 임포트는 제거하는 것이 좋습니다.
  6. 테스트 확인:

    • "테스트 확인을 위해"라는 표현이 있지만, 코드의 최종 상태가 어떻게 될지는 알 수 없습니다. 개발 단계에서 만든 DTO의 최종 목적이 명확하다면, 이를 기초로 한 테스트 케이스 작성도 고려합니다.

버그 리스크는 현재로서는 발견되지 않았으나, 실제 사용 맥락에 따라 주의가 필요할 수 있습니다. 전반적으로 코드가 명확하고 체계적으로 유지될 수 있도록 주석을 세심하게 작성하고 불필요한 코드를 최소화하는 것을 권장합니다.

* security, jwt 테스트 확인을 위해 임시로 만든 dto
*
* */
@Getter
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 코드 리뷰를 제공하겠습니다.

  1. 주석 처리: 주석은 보통 좋은 습관이지만, 해당 주석이 "임시로 만든 dto"라는 설명이 너무 일반적입니다. 이 DTO가 왜 임시로 만들어졌는지를 좀 더 구체적으로 설명하는 것이 좋습니다. 예를 들어, "테스트 용도로 작성된 DTO로, 나중에 삭제될 예정입니다" 스러운 내용을 추가하면 후에 코드를 보는 사람들이 이해하는데 도움이 될 것입니다.

  2. DTO의 이름: ‘MemberJoinDto’라는 이름은 명확하지만, 사용할 때 해당 DTO가 가입과 관련된 모든 필드를 포함하고 있는지 확인해보는 것이 좋습니다. 필요하다면 필드를 추가하는 것도 고려해보세요.

  3. Lombok 사용: @Getter@Setter를 사용하는 것은 코드의 간결성을 높이는 좋은 방식입니다. 하지만, DTO 객체에 대해 불변성을 원한다면 @Value를 사용하는 것이 더 나을 수 있습니다. 이렇게 하면 불변 객체로 설계하게 되어 데이터 무결성을 보장할 수 있습니다.

  4. 취약점: 현재 코드에는 보안과 관련된 어떤 점도 포함되어 있지 않습니다. 만약 이 DTO가 보안 관련 데이터를 포함해야 한다면, 예를 들어 비밀번호나 민감한 정보가 포함될 수 있으므로 이를 안전하게 다룰 필요가 있습니다.

  5. 추가적인 필드 정의 필요: DTO가 비어있는 상태인 것처럼 보입니다. 실제 사용할 필드가 무엇인지 정의해야 합니다.

종합적으로, 주석을 보완하고, DTO의 비즈니스 로직에 맞는 필드를 정의하고, 보안적 요소를 고려하는 것이 필요합니다. 이 외에도 구현 방식에 따라 다를 수 있으니, 주변 코드와 명확하게 연관짓는 것이 중요합니다.

*
* */
@Getter
@Builder
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 하겠습니다.

  1. 주석 사용:

    • 주석에서 "jwt 토큰 생성 확인을 위해 임시로 만든 dto"라고 명시하였으나, 주석이 코드 상단에 위치했기 때문에 해당 코드의 목적과 사용을 명확히 설명하는 데 도움이 됩니다. 그러나 만약 이 DTO가 임시로 만들어졌다면, 해당 코드가 실제로 필요한지, 또는 제거해야 하는지를 팀과 논의하는 것이 좋습니다.
  2. 주석 삭제 예정:

    • "삭제 예정"이라는 언급이 있지만, 주석이 삭제되기 전에 코드 리뷰에 남겨둬야 할 중요한 점들을 기록해 두는 것이 좋습니다. 특정 기능을 포함하는 DTO가 잠재적으로 삭제될 경우, 다른 개발자나 유지 관리자가 이 코드의 존재 이유를 이해하기 어려울 수 있습니다.
  3. 코드 형식:

    • 코드 형식과 관련하여 주석과 코드 간의 간격을 조정하여 가독성을 높일 수 있습니다. 예를 들어, 주석과 DTO 클래스 정의 간에 추가적인 공백 라인을 넣는 것도 도움이 될 수 있습니다.
  4. Lombok 사용:

    • Lombok 어노테이션(@Getter, @builder, @AllArgsConstructor)이 잘 사용되었습니다. 다만, Lombok을 사용할 때는 관련 의존성이 프로젝트에 포함되어 있는지 확인해야 합니다. 그렇지 않으면 빌드 에러가 발생할 수 있습니다.
  5. 변수 및 메서드 명명:

    • DTO에 포함된 필드가 없으므로, 나중에 필드를 추가할 때는 명확하고 일관된 명명 규칙을 따르는 것이 좋습니다.

이 외에도 전체적인 코드 구조나 프로젝트의 특정 요구 사항에 따라 추가적인 검토가 필요할 수 있습니다. 코드 수정이 필요한 경우, 추가적인 정보와 함께 요청해 주시면 좋습니다.


Member member = memberService.getUserWithAuthorities(dto.getUserid()).get();

if (ObjectUtils.isEmpty(member)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if문의 조건식이 반대로 적용되어 있는 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

로그인 API 구현할 때 수정하도록 하겠습니다.

httpHeaders.add(JwtFilter.AUTHORIZATION_HEADER, "Bearer " + jwt);

// tokenDto를 이용해 response body에도 넣어서 리턴
return new ResponseEntity<>(new TokenDto(jwt), httpHeaders, HttpStatus.OK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

tokenDto(jwt)는 확인용으로 추가하신 것 같은데, 나중에 수정하실 땐 httpHeaders만 전달해도 될 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 삭제 예정으로 주석 남겼습니다.

// memberService.join(dto.getUserid(), dto.getPw());
return ResponseEntity.ok("join success");
} catch (Exception e) {
return ResponseEntity.badRequest().body(e.getMessage());
Copy link
Collaborator

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.

API 개발 시 반환타입 일괄 수정하겠습니다.

@AllArgsConstructor
public class TokenDto {

private final String token;
Copy link
Collaborator

Choose a reason for hiding this comment

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

값이 하나만 전달된다면 dto를 만들지 않아도 괜찮을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네! 알겠습니다.

@Autowired
MemberRepository repository;
@Autowired
MemberRepository memberRepository;
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 MemberRepository를 두 번 작성하신 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

하나 삭제 하겠습니다👍

Optional<Member> findByLoginId(String loginId);

@EntityGraph(attributePaths = {"role"})
Optional<Member> findOneWithAuthoritiesByLoginId(String loginId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

role을 같이 확인하고 싶다면 Authentication을 활용해 보는 방법도 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Authentication 인증 객체에 대해 알아보고 적용해보겠습니다.

public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
throws IOException, ServletException {
HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse;
Copy link
Collaborator

Choose a reason for hiding this comment

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

변환하지 않고 바로 HttpServletRequest 와 HttpServletResponse를 받으면 안 될까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

javax.servlet.Filter 인터페이스를 오버라이딩 사용하기 때문에 매개변수 타입을 변경할 수 없습니다.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

@Service
public class MemberService {
@Autowired
PasswordEncoder passwordEncoder;
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰를 제공하겠습니다.

  1. 변수명 변경: repository에서 memberRepository로 변경하였습니다. 변수명 변경은 코드의 가독성을 높이는 좋은 개선입니다. 이로 인해 객체의 역할이 명확해져 이해하기 쉬워집니다.

  2. 주석 처리된 코드: //public Long join(String userid, String pw)와 같은 주석 처리된 코드는 향후 필요하지 않은 경우 삭제하는 것이 좋습니다. 이는 코드 베이스를 더 깔끔하게 유지하며, 혼란을 줄입니다.

  3. 주입 방식 확인: @Autowired 애너테이션으로 의존성을 주입하고 있습니다. Spring의 의존성 주입을 통해 올바르게 설정되었는지 확인해야 합니다. 그렇지 않으면 NullPointerException 발생 위험이 있습니다.

  4. Optional 반환 타입: findOne 메서드가 Optional<Member>를 반환하는 것은 좋은 접근입니다. 그러나 이를 호출하는 부분에서 Optional을 적절히 처리하도록 주의해야 합니다. Optional의 ifPresentorElse 메서드를 사용하는 것을 권장합니다.

  5. 에러 처리: findOne 메서드는 존재하지 않는 loginId에 대해 Optional.empty를 반환합니다. 클라이언트 측에서 적절한 에러 처리가 이루어지도록 문서화하거나, 필요한 경우 예외를 던지는 방식도 검토해볼 필요가 있습니다.

개선 사항을 반영하면 코드의 안정성과 가독성을 더욱 향상시킬 수 있습니다. 잘 진행되고 있는 것 같습니다!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

// public String adminSettingPage() {
// return "admin_setting";
// }
//
Copy link

Choose a reason for hiding this comment

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

코드 리뷰를 진행하겠습니다. 다음은 제안 사항 및 잠재적인 버그 리스크입니다.

  1. 응답 타입 변경:

    • loginjoin 메서드의 반환 타입이 변경되었습니다. 이를 통해 API 응답을 표준화했지만, 현재 반환하는 정보가 성공 코드 외에 jwt 토큰이나 성공 메시지를 포함하지 않아서 클라이언트 측에서 더 많은 정보를 얻기 어려울 수 있습니다. 필요하다면 TokenDto를 HTTP 응답에 포함할 수도 있습니다.
  2. 유저 존재 검사 로직:

    • login 메서드에서 ObjectUtils.isEmpty(member)를 체크하는 부분이 조건이 반대로 수정되었습니다. member가 존재할 경우에만 다음 로직을 수행하도록 되어 있는데, 실제로 getUserWithAuthorities(dto.getUserid())Optional을 반환하므로, 직접적으로 isEmpty()를 사용하기 보다는 isPresent()를 사용하는 것이 더 명확할 수 있습니다.
  3. 예외 처리:

    • join 메서드에서 발생할 수 있는 모든 예외를 BusinessException으로 변환하고 있습니다. 이는 예외가 발생한 원인을 명확하게 하지 못하고, 디버깅이나 로그에서 문제의 본질을 파악하기 어렵게 만들 수 있습니다. 발생 가능한 특정 예외를 잡아서 더 의미 있는 메시지를 로그 또는 응답으로 제공하는 것을 고려해보세요.
  4. 테스트 코드/주석 처리된 코드:

    • 주석 처리된 관리자 및 사용자 설정 관련 메서드가 보이는데, 주석으로 남겨두기보다는 필요할 때 필요한 기능만 포함하도록 클래스 구조를 정리하는 것이 좋습니다. 사용하지 않는 코드는 제거하는 것이 바람직합니다.
  5. 코드의 일관성:

    • 메서드에서 사용하는 ResponseEntity 구성 방식이 다릅니다. join 메서드에서는 body()로 설정하는 반면, login에서는 build()를 사용하고 있습니다. 일관된 방식으로 작성하는 것이 가독성을 높이는 데 도움이 됩니다.
  6. 주석 및 설명:

    • 코드에 설명이 부족합니다. 각 메서드에 대해 간단한 JavaDoc을 추가하는 것이 좋습니다. 특히 비즈니스 로직이 복잡해지거나 팀원들과 협업하는 경우, API의 기능에 대한 설명은 소스 코드 유지보수에 도움이 됩니다.

이러한 개선 사항들을 반영하면 코드 품질 향상에 기여할 수 있을 것입니다.

NOT_FOUND_TOKEN(1002, HttpStatus.INTERNAL_SERVER_ERROR, "유효하지 않은 JWT 토큰입니다."),
EXPIRED_TOKEN(1003, HttpStatus.INTERNAL_SERVER_ERROR, "만료된 JWT 토큰입니다."),
JWT_FILTER_ERROR(1004, HttpStatus.INTERNAL_SERVER_ERROR, "필터 처리 중 예외가 발생했습니다."),
DUPLICATE_USER_ID(1005, HttpStatus.BAD_REQUEST,"이미 존재하는 ID 입니다."),
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 리뷰입니다.

버그 리스크

  1. 에러 코드 중복 가능성: 추가된 오류 코드를 기존의 에러 코드와 비교했을 때, 10051006이 다른 에러 코드와 겹치지 않는지 확인해야 합니다. 중복이 발생하면 예기치 않은 동작이 일어날 수 있습니다.
  2. 정의된 상태 코드 확인: HttpStatus.BAD_REQUEST에 대한 올바른 사용인지 확인해야 합니다. 클라이언트 측의 잘못된 입력에 대한 응답으로 잘 적절하게 설정되어 있는지 점검하는 것이 중요합니다.

개선 사항

  1. 코드 주석 추가: 각 에러 코드에 대한 간단한 주석을 추가하면 유지보수를 더욱 용이하게 할 수 있습니다. 예를 들어, DUPLICATE_USER_ID는 어떤 경우에 발생하는지 설명하는 주석을 추가하는 것이 좋습니다.
  2. 에러 메시지 국제화: 현재 에러 메시지는 한국어로 작성되어 있습니다. 만약 다국적 사용자 기반을 고려한다면, 국제화(i18n)를 통해 다양한 언어 지원을 검토할 필요가 있습니다.
  3. 명명 규칙: 에러 코드의 이름을 좀 더 명확하게 하여, 에러 발생 상황을 반영하도록 개선할 수 있습니다. 예를 들어 USER_NOT_FOUNDUSER_NOT_FOUND_ERROR로 수정할 수 있습니다.

전반적으로 코드 패치의 범위에서 잘 처리된 부분이 많지만, 위에서 언급한 요소들을 고려하면 더욱 안정적이고 유지보수하기 쉬운 코드를 만들 수 있을 것입니다.

@HiSeungmin HiSeungmin closed this Jan 8, 2025
@HiSeungmin HiSeungmin deleted the feature/member-security-#20 branch January 16, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 회원 Security 설정 클래스
3 participants