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
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/build_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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. 버전 관리: 만약 이 수정 사항이 중요한 변경이라면, 이를 설명하는 커밋 메시지를 추가하는 것이 좋습니다. 변경 사항에 대한 문서화는 협업에 큰 도움이 됩니다.

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

Expand Down
11 changes: 9 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
implementation 'org.springframework.boot:spring-boot-starter-thymeleaf'
implementation 'org.springframework.boot:spring-boot-starter-web'
//implementation 'org.springframework.boot:spring-boot-starter-security'
//implementation 'org.thymeleaf.extras:thymeleaf-extras-springsecurity6'
implementation 'org.springframework.boot:spring-boot-starter-security'
// implementation 'org.thymeleaf.extras:thymeleaf-extras-springsecurity6'
// https://mvnrepository.com/artifact/io.github.cdimascio/dotenv-java
implementation group: 'io.github.cdimascio', name: 'dotenv-java', version: '3.0.0'
// https://mvnrepository.com/artifact/org.springdoc/springdoc-openapi-starter-webmvc-ui
Expand All @@ -39,6 +39,13 @@ dependencies {
implementation 'com.sun.xml.bind:jaxb-core:2.3.0.1'
implementation 'com.sun.xml.bind:jaxb-impl:2.3.3'

// JSON Web Token (JWT) Core API
implementation 'io.jsonwebtoken:jjwt-api:0.11.5'

// 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. 테스트:

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

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

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.seveneleven.devlens.domain.member.controller;

import com.seveneleven.devlens.domain.member.dto.MemberJoinDto;
import com.seveneleven.devlens.domain.member.dto.TokenDto;
import com.seveneleven.devlens.domain.member.entity.Member;
import com.seveneleven.devlens.domain.member.service.MemberService;
import com.seveneleven.devlens.global.config.Annotation.AdminAuthorize;
import com.seveneleven.devlens.global.config.Annotation.UserAuthorize;
import com.seveneleven.devlens.global.config.JwtFilter;
import com.seveneleven.devlens.global.config.TokenProvider;
import lombok.AllArgsConstructor;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder;
import org.springframework.security.core.Authentication;
import org.springframework.util.ObjectUtils;
import org.springframework.web.bind.annotation.*;

@RestController
@RequestMapping("/api")
@AllArgsConstructor
public class memberController {

private final TokenProvider tokenProvider;
private final AuthenticationManagerBuilder authenticationManagerBuilder;
private final MemberService memberService;

@PostMapping("/login")
public ResponseEntity<?> login(MemberJoinDto dto) {
HiSeungmin marked this conversation as resolved.
Show resolved Hide resolved

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 구현할 때 수정하도록 하겠습니다.


UsernamePasswordAuthenticationToken authenticationToken =
new UsernamePasswordAuthenticationToken(dto.getUserid(), dto.getPw());

// authenticate 메소드가 실행이 될 때 CustomUserDetailsService class의 loadUserByUsername 메소드가 실행
Authentication authentication = authenticationManagerBuilder.getObject().authenticate(authenticationToken);

// authentication 객체를 createToken 메소드를 통해서 JWT Token을 생성
String jwt = tokenProvider.createToken(authentication);

HttpHeaders httpHeaders = new HttpHeaders();
// response header에 jwt token에 넣어줌
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.

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

}
return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
}
HiSeungmin marked this conversation as resolved.
Show resolved Hide resolved

@PostMapping("/join")
public ResponseEntity<String> join(@RequestBody MemberJoinDto dto) {
try {
// 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 개발 시 반환타입 일괄 수정하겠습니다.

}
}

@GetMapping("/setting/admin")
@AdminAuthorize
public String adminSettingPage() {
return "admin_setting";
}

@GetMapping("/setting/user")
@UserAuthorize
public String userSettingPage() {
return "user_setting";
}
}
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. 주석 및 문서화

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.seveneleven.devlens.domain.member.dto;

import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.Size;
import lombok.*;

@Getter
@Setter
@Builder
@AllArgsConstructor
@NoArgsConstructor
public class MemberDto {

@NotBlank
@Size(min = 3, max = 50)
private String username;

@JsonProperty(access = JsonProperty.Access.WRITE_ONLY)
@NotBlank
@Size(min = 3, max = 100)
private String password;

@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. 주석 추가:

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.seveneleven.devlens.domain.member.dto;

import lombok.Getter;
import lombok.Setter;

@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의 비즈니스 로직에 맞는 필드를 정의하고, 보안적 요소를 고려하는 것이 필요합니다. 이 외에도 구현 방식에 따라 다를 수 있으니, 주변 코드와 명확하게 연관짓는 것이 중요합니다.

@Setter
public class MemberJoinDto {

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에 대한 검사나 변환은 서비스 계층에서 처리하는 것이 좋습니다.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.seveneleven.devlens.domain.member.dto;

import lombok.*;

@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에 포함된 필드가 없으므로, 나중에 필드를 추가할 때는 명확하고 일관된 명명 규칙을 따르는 것이 좋습니다.

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

@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.

네! 알겠습니다.

}
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를 제거하고 생성자를 통해 초기화될 수 있도록 하면 좋습니다. 이러한 방식은 객체의 상태 변경을 방지하여 안전성을 높일 수 있습니다.

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

Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
package com.seveneleven.devlens.domain.member.entity;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.seveneleven.devlens.domain.member.constant.MemberStatus;
import com.seveneleven.devlens.domain.member.constant.Role;
import com.seveneleven.devlens.domain.member.constant.YN;
import com.seveneleven.devlens.global.entity.BaseEntity;
import jakarta.persistence.*;
import lombok.*;
import org.springframework.security.crypto.password.PasswordEncoder;

import java.time.LocalDate;
import java.time.LocalDateTime;

@Entity
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@JsonIgnoreProperties({"hibernateLazyInitializer", "handler"})
@Table(name = "member")
public class Member extends BaseEntity {

Expand All @@ -21,7 +24,7 @@ public class Member extends BaseEntity {
@Column(name = "id")
private Long id; // 회원 ID

@ManyToOne(fetch = FetchType.LAZY)
@ManyToOne(fetch = FetchType.EAGER)
@JoinColumn(name = "company_id", nullable = false, referencedColumnName = "id")
private Company company; // 회사 ID (연관관계)

Expand All @@ -40,7 +43,7 @@ public class Member extends BaseEntity {

@Column(name = "profile_image_exists", nullable = false)
@Enumerated(EnumType.STRING)
private YN profileImageExists; // 프로필 이미지 유무
private YN profileImageExists = YN.N; // 프로필 이미지 유무

@Column(name = "name", nullable = false, length = 100)
private String name; // 이름
Expand All @@ -62,15 +65,15 @@ public class Member extends BaseEntity {


// 생성 메서드
public static Member createMember(String loginId, String password, Company companyId, Role role, String name, String email,
LocalDate birthDate, String phoneNumber, Long departmentId, Long positionId) {
public static Member createMember(String loginId, String password, Company company, Role role, String name, String email,
LocalDate birthDate, String phoneNumber, Long departmentId, Long positionId, PasswordEncoder pwdEncoder) {
Member member = new Member();
member.name = name;
member.role = role;
member.email = email;
member.loginId = loginId;
member.password = password;
member.company = companyId;
member.password = pwdEncoder.encode(password);
member.company = company;
member.birthDate = birthDate;
member.phoneNumber = phoneNumber;
member.positionId = positionId;
Copy link

Choose a reason for hiding this comment

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

코드 패치에 대한 간략한 코드 리뷰를 아래와 같이 제안합니다.

버그 리스크:

  1. 페치 타입 변경:

    • @ManyToOne(fetch = FetchType.LAZY)에서 @ManyToOne(fetch = FetchType.EAGER)로 변경되었습니다. 이로 인해 연관된 Company 객체가 항상 로드되므로, 불필요한 메모리 사용 및 성능 저하를 초래할 수 있습니다. 실제로 이 엔티티의 사용 패턴에 따라 적절한 페치 전략을 고려해야 합니다.
  2. 패스워드 인코딩:

    • createMember 메서드에서 PasswordEncoder를 통해 패스워드를 인코딩하는 것은 보안적으로 바람직하나, pwdEncoder가 null이 아닐지 검증하는 로직이 필요합니다. 인코더가 제공되지 않을 경우 NPE(NullPointerException)가 발생할 수 있습니다.

개선 제안:

  1. 제네릭 클래스 사용:

    • Company와 같은 관계형 엔티티는 Optional<Company>로 선언하여, 관계가 꼭 필요한 경우를 보다 쉽게 처리할 수 있습니다. 이로 인해 null 체크를 줄이고 코드 안전성을 강화할 수 있습니다.
  2. 다양한 생성자 오버로딩 제공:

    • createMember 메서드가 필요로 하는 많은 파라미터가 있습니다. 설계에 따라 여러 생성자를 제공하여 사용자가 필요한 정보만 제공할 수 있게 하는 것도 좋은 접근입니다.
  3. @JsonIgnoreProperties 사용 이유 명확화:

    • hibernateLazyInitializerhandler를 무시하는 이유를 주석으로 명시해주면, 코드 유지보수 시 도움이 될 것입니다.
  4. 상수 사용:

    • profileImageExists의 기본값을 YN.N으로 설정한 것은 좋습니다. 하지만 이와 같은 상수값을 나중에 사용하기 쉽게 클래스 상단에 선언하여 필요에 따라 재사용할 수 있도록 하는 것이 좋습니다.
  5. 주석 추가:

    • 코드 변경 부분에 대해 주석을 추가하는 것이 좋습니다. 특히 변경된 주요 로직이나 이유를 기록하면 다른 개발자들이 이해하는 데 도움이 됩니다.

이러한 사항들을 고려하여 코드를 리팩토링하면 더욱 안전하고 효율적인 코드가 될 것으로 보입니다.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.seveneleven.devlens.domain.member.repository;


import com.seveneleven.devlens.domain.member.entity.Member;
import org.springframework.data.jpa.repository.EntityGraph;
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.Optional;

public interface MemberRepository extends JpaRepository<Member, Long> {

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 인증 객체에 대해 알아보고 적용해보겠습니다.

}
Copy link

Choose a reason for hiding this comment

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

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

  1. 패키지 및 클래스 구조: 패키지 구조와 클래스 이름이 적절해 보입니다. 도메인 모델과의 일관성도 잘 유지되고 있습니다.

  2. JPA 레포지토리 인터페이스: Spring Data JPA의 JpaRepository를 상속받아 기본 CRUD 기능을 향상시키고 User 정의 메서드도 추가한 점이 좋습니다.

  3. Optional 사용: Optional<Member>를 사용하여 null 체크를 명시적으로 처리하고 있다는 점은 좋은 접근입니다. 하지만, 이러한 반환값을 사용할 때, 사용자가 이를 적절히 처리해야 함을 명확히 문서화하는 것이 중요합니다.

  4. EntityGraph 애너테이션: @EntityGraph를 사용하여 관련된 객체를 함께 로딩하는 것은 성능 최적화에 도움이 될 수 있습니다. 그러나 attributePaths에 더 많은 속성을 추가할 경우, 로딩 성능에 영향을 미칠 수 있습니다. 필요한 속성만 포함되도록 주의해야 합니다.

  5. 메서드 명명: findOneWithAuthoritiesByLoginId 메서드는 의도를 잘 나타내고 있으며, 이력을 검색하는 데 적합한 이름입니다. 그러나 사용자의 요구에 따라 더 간결한 이름을 고려할 수도 있습니다.

  6. 문서화 및 주석: 코드 자체는 비교적 간단하지만, 각 메서드에 대한 주석을 추가하여 사용자가 이해하는 데 도움이 될 수 있습니다.

  7. 끝에 개행 없음: No newline at end of file 경고는 코드 스타일에 따라 다를 수 있지만, 코드를 작성할 때 파일의 끝에 개행 문자를 추가하는 것은 일반적인 좋은 습관입니다.

버그 리스크:

  • 코드 자체에는 명시적인 버그가 보이지 않으나, 호출하는 측에서 Optional을 사용하는 방법을 잘못 처리할 가능성이 있으므로 주의해야 합니다.

개선 제안:

  • 메서드의 사용 예시나 반환값에 대한 설명을 문서화 하여, 코드의 가독성을 높이는 것이 좋습니다.
  • findByLoginId 메서드에 대해 인풋 값에 대한 검증 로직을 추가하는 것도 Consider할 수 있습니다.

위의 내용들을 참고하여 코드의 품질을 더욱 높일 수 있을 것입니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package com.seveneleven.devlens.domain.member.service;

import com.seveneleven.devlens.domain.member.entity.Member;
import com.seveneleven.devlens.domain.member.repository.MemberRepository;
import com.seveneleven.devlens.global.util.security.SecurityUtil;
import jakarta.transaction.Transactional;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service;

import java.util.Optional;

@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를 반환합니다. 클라이언트 측에서 적절한 에러 처리가 이루어지도록 문서화하거나, 필요한 경우 예외를 던지는 방식도 검토해볼 필요가 있습니다.

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

@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.

하나 삭제 하겠습니다👍



public Optional<Member> findOne(String loginId) {
return repository.findByLoginId(loginId);
}

// public Long join(String userid, String pw) {
// Optional<Company> companyOptional = companyRepository.findById(1L);
//
// Company company = companyOptional.orElseThrow(() ->
// new IllegalStateException("Company with ID 1L not found")
// );
//
// Member member = Member.createMember(userid, pw, company, Role.USER, "박철수", "[email protected]", LocalDate.now(),"010-111-1111",1L,1L,passwordEncoder);
// repository.save(member);
//
// return member.getId();
// }

// 유저,권한 정보를 가져오는 메소드
@Transactional
public Optional<Member> getUserWithAuthorities(String memberId) {
return memberRepository.findOneWithAuthoritiesByLoginId(memberId);
}

// 현재 securityContext에 저장된 username의 정보만 가져오는 메소드
@Transactional
public Optional<Member> getMyUserWithAuthorities() {
return SecurityUtil.getCurrentUsername()
.flatMap(memberRepository::findOneWithAuthoritiesByLoginId);
}
}
Copy link

Choose a reason for hiding this comment

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

코드 리뷰를 진행하겠습니다.

코드 리뷰

  1. 중복된 MemberRepository 주입:

    • repositorymemberRepository가 같은 타입인 MemberRepository입니다. 이러한 중복은 혼란을 초래할 수 있으며, 단일 인스턴스로 관리하는 것이 좋습니다.
    • 개선 제안: 하나의 MemberRepository만 사용하고, 그 이름을 명확하게 정하세요. 필요에 따라 function명을 다르게 부여하는 방법도 있습니다.
  2. 주석 처리된 코드:

    • join 메소드가 주석 처리되어 있습니다. 이 메소드를 나중에 사용할 계획이 없다면 삭제하는 것이 좋습니다. 코드가 명확하고 간결한 것이 유지 보수에 도움이 됩니다.
    • 개선 제안: 주석 처리된 코드는 필요하지 않으면 삭제하고, 추후에 필요하면 적절한 버전 관리를 통해 복구하세요.
  3. 예외 처리:

    • 현재 join 메소드에서 사용된 예외 처리 패턴이 좋습니다. 하지만 getUserWithAuthoritiesgetMyUserWithAuthorities 메소드에서의 예외 처리가 부족합니다. Optional 프리미티브를 반환하기 때문에, 호출 측에서 필요 시 예외를 처리해야 합니다.
    • 개선 제안: 예외가 발생할 수 있는 지점에서 명확한 예외 처리 로직을 추가하는 것이 좋습니다.
  4. 메소드 명확성:

    • 메소드 getUserWithAuthoritiesgetMyUserWithAuthorities의 이름에서 어떤 정보를 가져오는지가 명확히 드러나지만, 주석이 없으면 이해하기 어려운 경우도 있습니다.
    • 개선 제안: 메소드 이름과 주석을 조화롭게 사용하여 가독성을 높이세요.
  5. 보안 및 인코딩:

    • PasswordEncoder를 사용하여 비밀번호를 암호화하는 방식이 필요할 경우, 주의 깊게 구현되어야 합니다. join 메소드가 주석 처리되어 있어 사용되지 않지만, 실제로 사용할 때 적절하게 비밀번호가 인코딩되었는지 확인해야 합니다.
  6. 트랜잭션 관리:

    • @Transactional을 적절히 사용하고 있지만, 메소드에서 어떤 비즈니스 로직이 실행되는지를 주석으로 상세히 설명하는 것이 좋습니다.

결론

전반적으로 코드가 깔끔하게 작성되어 있으나, 몇 가지 중복 문제와 가독성을 높일 수 있는 부분에서 개선할 여지가 있습니다. 코드 유지보수성을 높이기 위해 불필요한 부분은 제거하고, 예외 처리와 관련된 부분을 강화하는 것이 좋습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.seveneleven.devlens.domain.member.service;

import com.seveneleven.devlens.domain.member.entity.Member;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.stereotype.Component;

@Component
public class MyUserDetailsService implements UserDetailsService {
private final MemberService memberService;

public MyUserDetailsService(MemberService memberService) {
this.memberService = memberService;
}

@Override
public UserDetails loadUserByUsername(String insertedUserId){
HiSeungmin marked this conversation as resolved.
Show resolved Hide resolved
Member member = memberService.findOne(insertedUserId)
.orElseThrow(() -> new UsernameNotFoundException("User with ID '" + insertedUserId + "' not found."));

return User.builder()
.username(member.getLoginId())
.password(member.getPassword())
.roles(member.getRole().toString())
.build();
}
}
Copy link

Choose a reason for hiding this comment

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

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

장점

  1. 클래스 구조: MyUserDetailsService 클래스는 UserDetailsService 인터페이스를 구현하고 있어 Spring Security와 통합되는 좋은 구조입니다.
  2. 의존성 주입: MemberService를 생성자 주입으로 사용하여 코드의 의존성을 관리하는 방식을 잘 따르고 있습니다.
  3. 예외 처리: 사용자 조회 시 UsernameNotFoundException을 던져 사용자가 없는 경우를 명시적으로 처리하고 있어 좋습니다.

개선 제안 및 버그 위험

  1. 비밀번호 암호화: member.getPassword()로 가져온 비밀번호가 암호화된 상태에서 전달되는지 확인해야 합니다. 만약 평문 비밀번호가 전달된다면 보안상 위험합니다. 암호화된 비밀번호를 사용하여 사용자 인증을 수행해야 합니다.

  2. 역할 처리: member.getRole().toString()을 사용하려는 경우, getRole()이 반환하는 값이 String 형식인지 확인해야 합니다. 만약 enum이라면, name() 메서드를 사용하여 안전하게 문자열로 변환해야 합니다. 예를 들면, member.getRole().name()과 같이 사용하세요.

  3. Null 체크: member.getLoginId()member.getPassword()가 null일 가능성도 고려해야 합니다. 이를 통해 NullPointerException을 방지할 수 있습니다. 필요한 경우 이러한 필드를 확인하고 예외를 던지는 로직을 추가하세요.

  4. 클래스명: MyUserDetailsService는 일반적인 이름으로, 이를 좀 더 의미 있게 수정할 수 있습니다. 예를 들어, CustomUserDetailsService와 같은 구체적인 이름으로 변경하는 것이 좋습니다.

  5. 매직 문자열: UsernameNotFoundException 메시지에서 사용되는 문자열을 상수로 추출하면 유지 보수성이 향상됩니다. 예외 메시지를 상수로 정의하면, 향후 변경 시 코드 전체를 수정하지 않고도 손쉽게 수정할 수 있습니다.

이상의 사항들을 고려하면서 코드를 개선하면 보다 견고하고 안전한 서비스를 구축할 수 있을 것입니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.seveneleven.devlens.global.config.Annotation;

import org.springframework.security.access.prepost.PreAuthorize;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target({ ElementType.METHOD, ElementType.TYPE })
@Retention(RetentionPolicy.RUNTIME)
@PreAuthorize("hasAnyRole('ADMIN')")
public @interface AdminAuthorize {
}
Copy link

Choose a reason for hiding this comment

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

코드 리뷰에 대한 피드백은 다음과 같습니다:

  1. 주석 부족: 이 어노테이션의 목적이 무엇인지 설명하는 주석이 없어서 코드의 이해가 어렵습니다. 개발자들이 이 어노테이션을 사용할 때 이해할 수 있도록 간단한 설명을 추가하는 것이 좋습니다.

  2. 역할 확인: 현재 "ADMIN" 역할 만을 허용하고 있는데, 향후 다른 역할을 추가할 필요가 있을 수 있습니다. hasAnyRole 대신 hasRole로 변경하고 여러 역할을 처리할 수 있도록 하는 것도 고려해 볼 수 있습니다.

  3. 일관성: 전체 프로젝트에서 다른 커스텀 어노테이션이 있다면 명명 규칙이나 스타일이 일관되게 유지되는지 확인하는 것이 좋습니다.

  4. 테스트 케이스 부족: 이 어노테이션을 사용하는 메소드에 대해 적절한 단위 테스트가 작성되어야 합니다. 이를 통해 보안 기능이 의도한 대로 작동하는지 확인할 수 있습니다.

  5. 마지막 줄 개행: 파일의 마지막에 개행이 없으면 버전 관리 시스템에서 문제가 발생할 수 있으므로, 마지막 줄에 개행을 추가하는 것이 좋습니다.

이 외에도 보안을 염두에 두고, 주석과 문서화가 잘 되어 있어야 이 기능이 어떻게 사용되는지 쉽게 이해할 수 있습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.seveneleven.devlens.global.config.Annotation;

import org.springframework.security.access.prepost.PreAuthorize;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target({ ElementType.METHOD, ElementType.TYPE })
@Retention(RetentionPolicy.RUNTIME)
@PreAuthorize("hasAnyRole('USER')")
public @interface UserAuthorize {
}
Copy link

Choose a reason for hiding this comment

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

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

  1. 주석 추가: 이 어노테이션의 목적과 사용 예시를 설명하는 주석을 추가하는 것이 좋습니다. 코드의 가독성을 높이고 다른 개발자가 이해하는 데 도움이 됩니다.

  2. Role 설정: hasAnyRole('USER') 사용이 적절한지 확인해야 합니다. 만약 'USER' 역할이 더 많은 컨텍스트에서 사용된다면, 명확한 역할(예: 'ROLE_USER')로 변경하는 것을 권장합니다. Spring Security에서는 역할에 'ROLE_' 접두사를 붙이는 것이 일반적입니다.

  3. 어노테이션의 확장성: 만약 이 어노테이션을 더 많은 역할에 대해 사용하고자 한다면, 매개변수를 추가하여 유연성을 제공하는 방법도 고려해 볼 수 있습니다. 예를 들어, String[] roles()와 같은 매개변수를 추가하여 다양한 역할을 처리할 수 있습니다.

  4. 테스트 추가: 이 어노테이션이 제대로 작동하는지 확인하기 위해 단위 테스트 케이스를 작성해야 합니다. Spring Security와 관련된 사항이므로 보안 관련 테스트가 특히 중요합니다.

  5. 종속성 주의: 사용 중인 Spring 버전에 따라 @PreAuthorize의 설정이 다를 수 있으므로, 해당 버전의 문서를 참조하여 호환성 문제를 확인하는 것이 좋습니다.

이러한 점들을 검토하면 코드의 품질과 유지보수성을 향상시킬 수 있을 것입니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.seveneleven.devlens.global.config;

import com.seveneleven.devlens.global.response.ErrorCode;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.web.access.AccessDeniedHandler;
import org.springframework.stereotype.Component;

import java.io.IOException;

/**
* 인증된 사용자가 필요한 권한 없이 보호된 리소스에 접근하려고 할 때 처리하는 클래스
*
* - Spring Security에서 권한 부족으로 인한 접근 거부 시 403 (Forbidden) 에러를 반환합니다.
*/
@Component
public class JwtAccessDeniedHandler implements AccessDeniedHandler {

/**
* 권한이 없는 사용자가 요청을 보낼 때 호출됩니다.
*
* @param request 클라이언트 요청 (HttpServletRequest)
* @param response 서버 응답 (HttpServletResponse)
* @param accessDeniedException 발생한 접근 거부 예외 (AccessDeniedException)
* @throws IOException 입출력 예외 발생 시
*/
@Override
public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException accessDeniedException)
throws IOException {
// 403 Forbidden 상태 코드 반환
response.sendError(ErrorCode.FORBIDDEN.getCode());
}
}

Copy link

Choose a reason for hiding this comment

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

코드 검토를 진행하겠습니다. 아래는 코드에 대한 피드백입니다:

  1. 주석 및 문서화:

    • 클래스와 메서드에 대한 주석이 잘 작성되어 있어, 기능 설명이 명확합니다. 다만, ErrorCode의 정의가 코드 내에 직접 나타나지 않으므로, 이 코드 조각을 보는 사용자에게는 ErrorCode.FORBIDDEN.getCode()의 반환 값이 뭔지에 대한 추가적인 설명이 필요할 수 있습니다. 이를 위해 'ErrorCode' 클래스에 대한 간단한 주석이나 문서를 추가하는 것이 좋습니다.
  2. 예외 처리:

    • IOException를 던지는 부분에서, 실제로 시스템에서 발생할 수 있는 다른 예외에 대한 처리가 없습니다. AccessDeniedException이 발생했을 경우, 이 외에 추가적인 로그를 남기거나 별도의 처리를 하는 것이 좋습니다. 예를 들어, 누가 어떤 요청을 했는지 기록하는 로깅을 추가하면 좋을 것입니다.
  3. HTTP 상태 코드:

    • response.sendError(ErrorCode.FORBIDDEN.getCode());에서 FORBIDDEN.getCode()가 제대로 403(Forbidden)으로 매핑되는지 확인이 필요합니다. 코드가 수정될 경우에 이 부분이 제대로 작동하는지 여부를 보장하기 위한 테스트가 필요합니다.
  4. 테스트 커버리지:

    • 이 클래스의 기능을 보장하기 위해, 단위 테스트와 통합 테스트가 필요합니다. 특히 권한이 없는 사용자의 접근 시나리오를 다루는 테스트 케이스를 포함하는 것이 좋습니다.
  5. 코드 스타일:

    • 전체적으로 코드 스타일은 잘 유지되고 있습니다. Spring의 일반적인 코드 컨벤션에 따라 잘 작성되어 있습니다.

이 수정 사항들이 반영되면 코드의 가독성과 안정성을 높일 수 있을 것으로 예상됩니다.

Loading
Loading