-
Notifications
You must be signed in to change notification settings - Fork 2
MOSU-150 feat: Profile 수정 변경되지 않는 필드 삭제 #153
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| package life.mosu.mosuserver.domain.user; | ||
|
|
||
| public enum UserRole { | ||
| ROLE_USER, ROLE_ADMIN, PENDING | ||
| ROLE_USER, ROLE_ADMIN, ROLE_PENDING | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,10 @@ | |||||||||||||||||||||||||
| import org.springframework.context.annotation.Bean; | ||||||||||||||||||||||||||
| import org.springframework.context.annotation.Configuration; | ||||||||||||||||||||||||||
| import org.springframework.http.HttpMethod; | ||||||||||||||||||||||||||
| import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; | ||||||||||||||||||||||||||
| import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; | ||||||||||||||||||||||||||
| import org.springframework.security.access.hierarchicalroles.RoleHierarchy; | ||||||||||||||||||||||||||
| import org.springframework.security.access.hierarchicalroles.RoleHierarchyImpl; | ||||||||||||||||||||||||||
| import org.springframework.security.config.Customizer; | ||||||||||||||||||||||||||
| import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; | ||||||||||||||||||||||||||
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | ||||||||||||||||||||||||||
|
|
@@ -54,6 +58,21 @@ public class SecurityConfig { | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private final AuthorizationRequestRedirectResolver authorizationRequestRedirectResolver; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Bean | ||||||||||||||||||||||||||
| public RoleHierarchy roleHierarchy() { | ||||||||||||||||||||||||||
| RoleHierarchyImpl hierarchy = new RoleHierarchyImpl(); | ||||||||||||||||||||||||||
| hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING"); | ||||||||||||||||||||||||||
| return hierarchy; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+61
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix role hierarchy inconsistency. The role hierarchy defines "PENDING" but according to the AI summary, this should be "ROLE_PENDING" to maintain consistency with the enum constant renaming mentioned in the PR. - hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > PENDING");
+ hierarchy.setHierarchy("ROLE_ADMIN > ROLE_USER \n ROLE_USER > ROLE_PENDING");📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Bean | ||||||||||||||||||||||||||
| public MethodSecurityExpressionHandler methodSecurityExpressionHandler( | ||||||||||||||||||||||||||
| RoleHierarchy roleHierarchy) { | ||||||||||||||||||||||||||
| DefaultMethodSecurityExpressionHandler expressionHandler = new DefaultMethodSecurityExpressionHandler(); | ||||||||||||||||||||||||||
| expressionHandler.setRoleHierarchy(roleHierarchy); | ||||||||||||||||||||||||||
| return expressionHandler; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Bean | ||||||||||||||||||||||||||
| public PasswordEncoder passwordEncoder() { | ||||||||||||||||||||||||||
| return PasswordEncoderFactories.createDelegatingPasswordEncoder(); | ||||||||||||||||||||||||||
|
|
@@ -74,7 +93,6 @@ public WebSecurityCustomizer webSecurityCustomizer() { | |||||||||||||||||||||||||
| @Bean | ||||||||||||||||||||||||||
| public SecurityFilterChain securityFilterChain(final HttpSecurity http) throws Exception { | ||||||||||||||||||||||||||
| http.csrf(AbstractHttpConfigurer::disable) | ||||||||||||||||||||||||||
| // .cors(Customizer.withDefaults()) | ||||||||||||||||||||||||||
| .cors(Customizer.withDefaults()) | ||||||||||||||||||||||||||
| .httpBasic(AbstractHttpConfigurer::disable) | ||||||||||||||||||||||||||
| .headers(c -> c.frameOptions(FrameOptionsConfig::disable)) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,14 +3,17 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import jakarta.persistence.EntityNotFoundException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.LinkedHashMap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import lombok.extern.slf4j.Slf4j; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.http.HttpStatus; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.http.ResponseEntity; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.http.converter.HttpMessageNotReadableException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.security.access.AccessDeniedException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.security.core.AuthenticationException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.web.bind.MethodArgumentNotValidException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Slf4j | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @RestControllerAdvice | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class GlobalExceptionHandler { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -31,6 +34,7 @@ public ResponseEntity<Map<String, Object>> handleMethodArgumentNotValidException | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| errors.put(error.getField(), error.getDefaultMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.BAD_REQUEST.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "유효성 검사에 실패했습니다."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", errors); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.badRequest().body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -46,6 +50,7 @@ public ResponseEntity<Map<String, Object>> handleIllegalArgumentException( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IllegalArgumentException ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Map<String, Object> response = new LinkedHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.BAD_REQUEST.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", "잘못된 요청입니다."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this handler, the user-friendly message ( To improve consistency, swap the values assigned here to align with the pattern used in other handlers.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.badRequest().body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,7 +66,9 @@ public ResponseEntity<Map<String, Object>> handleEntityNotFoundException( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| EntityNotFoundException ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Map<String, Object> response = new LinkedHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.NOT_FOUND.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "요청한 리소스가 존재하지 않습니다."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -75,7 +82,7 @@ public ResponseEntity<Map<String, Object>> handleAuthenticationException( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AuthenticationException ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Map<String, Object> response = new LinkedHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.UNAUTHORIZED.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "AUTHENTICATION_ERROR"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "인증에 실패했습니다"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -91,12 +98,27 @@ public ResponseEntity<Map<String, Object>> handleAccessDeniedException( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AccessDeniedException ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Map<String, Object> response = new LinkedHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.FORBIDDEN.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "ACCESS_DENIED"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "인가를 실패 했습니다"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.status(HttpStatus.FORBIDDEN).body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @return 409 Bad Request | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @ExceptionHandler(HttpMessageNotReadableException.class) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| HttpMessageNotReadableException ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Map<String, Object> response = new LinkedHashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("status", HttpStatus.CONFLICT.value()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.put("errors", ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ResponseEntity.status(HttpStatus.CONFLICT).body(response); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+111
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Javadoc at the top says Change the status code to /**
* @return 400 Bad Request
* @RequestBody JSON 파싱 실패 (필드명 불일치, 데이터 타입 불일치, JSON 형식 오류 등)
*/
@ExceptionHandler(HttpMessageNotReadableException.class)
public ResponseEntity<Map<String, Object>> handleHttpMessageNotReadableException(
HttpMessageNotReadableException ex) {
Map<String, Object> response = new LinkedHashMap<>();
response.put("status", HttpStatus.BAD_REQUEST.value());
response.put("message", "필드명 또는 데이터 타입이 일치하지 않습니다.");
response.put("errors", ex.getMessage());
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(response);
}
Comment on lines
+107
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Useful addition but consider using HTTP 400 status. The new handler for JSON parsing errors fills an important gap in error handling coverage. However, HTTP 409 Conflict is typically reserved for resource state conflicts rather than malformed request data. Consider using HTTP 400 Bad Request for JSON parsing errors: - response.put("status", HttpStatus.CONFLICT.value());
+ response.put("status", HttpStatus.BAD_REQUEST.value());
- return ResponseEntity.status(HttpStatus.CONFLICT).body(response);
+ return ResponseEntity.badRequest().body(response);HTTP 400 is more semantically appropriate for malformed request data, while HTTP 409 is better suited for business logic conflicts. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @ExceptionHandler(Exception.class) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public ResponseEntity<Map<String, Object>> handleGeneralException(Exception ex) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| System.out.println("Exception: " + ex.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role hierarchy definition uses
PENDING, but theUserRoleenum was updated in this pull request to useROLE_PENDING. This inconsistency will cause the role hierarchy to not function as expected for users with the pending role, as Spring Security will not recognize the relationship.To ensure the hierarchy works correctly, update the string to use the correct
ROLE_PENDINGname.