Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.daramg.server.composer.domain.Composer;
import com.daramg.server.composer.domain.ComposerLike;
import com.daramg.server.composer.dto.ComposerCreateDto;
import com.daramg.server.composer.dto.ComposerUpdateDto;
import com.daramg.server.composer.dto.ComposerLikeResponseDto;
import com.daramg.server.composer.repository.ComposerLikeRepository;
import com.daramg.server.composer.repository.ComposerRepository;
Expand Down Expand Up @@ -55,6 +56,17 @@ public void createComposer(ComposerCreateDto dto, User user) {
* @param user 요청한 유저 (인증 객체)
* @return 좋아요 생성 시 {@code true}, 취소 시 {@code false}를 담은 DTO
*/
@Transactional
public void updateComposer(Long composerId, ComposerUpdateDto dto, User user) {
if (user.getId() != 5L) {
throw new BusinessException(CommonErrorStatus.FORBIDDEN);

Choose a reason for hiding this comment

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

security-high high

The use of a hardcoded user ID (5L) for administrative authorization is a significant security vulnerability. This is insecure as any user who happens to be assigned this ID could gain administrative privileges. This pattern is also present in other administrative methods like createComposer and deleteComposer. It is highly recommended to implement a robust role-based access control (RBAC) system by adding a role field to the User entity and checking for an ADMIN role instead of a specific user ID. Additionally, to improve maintainability and reduce duplication, this authorization logic should be extracted into a separate private method, as it is currently duplicated across createComposer, updateComposer, and deleteComposer.

}
Composer composer = entityUtils.getEntity(composerId, Composer.class);
composer.update(dto.getKoreanName(), dto.getEnglishName(), dto.getNativeName(),
dto.getGender(), dto.getNationality(), dto.getBirthYear(), dto.getDeathYear(),
dto.getBio(), dto.getEra(), dto.getContinent());
}

@Transactional
public void deleteComposer(Long composerId, User user) {
if (user.getId() != 5L) {
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/com/daramg/server/composer/domain/Composer.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,20 @@ public class Composer extends BaseEntity<Composer> {
@Column(name = "continent")
private Continent continent;

public void update(String koreanName, String englishName, String nativeName, Gender gender,
String nationality, Short birthYear, Short deathYear, String bio, Era era, Continent continent) {
this.koreanName = koreanName;
this.englishName = englishName;
this.nativeName = nativeName;
this.gender = gender;
this.nationality = nationality;
this.birthYear = birthYear;
this.deathYear = deathYear;
this.bio = bio;
this.era = era;
this.continent = continent;
}
Comment on lines +50 to +62

Choose a reason for hiding this comment

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

medium

update 메서드가 10개의 파라미터를 받아 'Long Parameter List' 코드 스멜에 해당합니다. 파라미터가 많아지면 가독성이 떨어지고, 향후 필드 변경 시 실수를 유발할 수 있습니다.

이를 개선하기 위해 파라미터를 객체로 묶는 것을 고려해 보세요. 예를 들어, ComposerUpdateDto를 직접 전달하거나, 도메인 계층의 독립성을 유지하기 위해 별도의 ComposerUpdateParam 객체를 정의하여 사용할 수 있습니다.

예시 (별도 파라미터 객체 사용):

// In Composer.java
public void update(ComposerUpdateParam param) {
    this.koreanName = param.getKoreanName();
    this.englishName = param.getEnglishName();
    // ... etc
}

이렇게 하면 update 메서드의 시그니처가 간결해지고, 관련 데이터를 하나의 객체로 관리할 수 있어 코드의 응집도가 높아집니다.


@Builder
private Composer(@NonNull String koreanName, @NonNull String englishName,
String nativeName, @NonNull Gender gender, String nationality,
Expand Down
20 changes: 20 additions & 0 deletions src/main/java/com/daramg/server/composer/dto/ComposerUpdateDto.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.daramg.server.composer.dto

import com.daramg.server.composer.domain.Continent
import com.daramg.server.composer.domain.Era
import com.daramg.server.composer.domain.Gender
import jakarta.validation.constraints.NotBlank
import jakarta.validation.constraints.NotNull

data class ComposerUpdateDto(
@get:NotBlank val koreanName: String,
@get:NotBlank val englishName: String,
val nativeName: String? = null,
@get:NotNull val gender: Gender,
@get:NotBlank val nationality: String,
@get:NotNull val birthYear: Short,
@get:NotNull val deathYear: Short,
@get:NotBlank val bio: String,
@get:NotNull val era: Era,
@get:NotNull val continent: Continent,
)
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.daramg.server.composer.application.ComposerService;
import com.daramg.server.composer.dto.ComposerCreateDto;
import com.daramg.server.composer.dto.ComposerUpdateDto;
import com.daramg.server.composer.dto.ComposerLikeResponseDto;
import com.daramg.server.user.domain.User;
import jakarta.validation.Valid;
Expand All @@ -23,6 +24,12 @@ public void createComposer(@Valid @RequestBody ComposerCreateDto dto, User user)
composerService.createComposer(dto, user);
}

@PutMapping("/{composerId}")
@ResponseStatus(HttpStatus.NO_CONTENT)
public void updateComposer(@PathVariable Long composerId, @Valid @RequestBody ComposerUpdateDto dto, User user) {
composerService.updateComposer(composerId, dto, user);
}

@DeleteMapping("/{composerId}")
@ResponseStatus(HttpStatus.NO_CONTENT)
public void deleteComposer(@PathVariable Long composerId, User user) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.springframework.restdocs.request.RequestDocumentation.parameterWithName;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@WebMvcTest(controllers = ComposerController.class)
Expand Down Expand Up @@ -77,6 +78,60 @@ public class ComposerControllerTest extends ControllerTestSupport {
));
}

@Test
void 작곡가를_수정한다() throws Exception {
// given
Long composerId = 1L;
Map<String, Object> requestBody = Map.of(
"koreanName", "베토벤",
"englishName", "Ludwig van Beethoven",
"gender", "MALE",
"nationality", "독일",
"birthYear", 1770,
"deathYear", 1827,
"bio", "독일의 작곡가",
"era", "CLASSICAL",
"continent", "EUROPE"
);
Cookie cookie = new Cookie(COOKIE_NAME, "access_token");

// when
ResultActions result = mockMvc.perform(put("/composers/{composerId}", composerId)
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(requestBody))
.cookie(cookie)
);

// then
result.andExpect(status().isNoContent())
.andDo(restDocsHandler.document(
resource(ResourceSnippetParameters.builder()
.tag("Composer API")
.summary("작곡가 수정")
.description("관리자가 작곡가 정보를 수정합니다.")
.pathParameters(
parameterWithName("composerId").description("수정할 작곡가 ID")
)
.requestFields(
fieldWithPath("koreanName").type(JsonFieldType.STRING).description("한국어 이름"),
fieldWithPath("englishName").type(JsonFieldType.STRING).description("영어 이름"),
fieldWithPath("nativeName").type(JsonFieldType.STRING).description("원어 이름").optional(),
fieldWithPath("gender").type(JsonFieldType.STRING).description("성별 (MALE / FEMALE / UNKNOWN)"),
fieldWithPath("nationality").type(JsonFieldType.STRING).description("국적"),
fieldWithPath("birthYear").type(JsonFieldType.NUMBER).description("출생 연도"),
fieldWithPath("deathYear").type(JsonFieldType.NUMBER).description("사망 연도"),
fieldWithPath("bio").type(JsonFieldType.STRING).description("소개"),
fieldWithPath("era").type(JsonFieldType.STRING).description("시대 (MEDIEVAL_RENAISSANCE / BAROQUE / CLASSICAL / ROMANTIC / MODERN_CONTEMPORARY)"),
fieldWithPath("continent").type(JsonFieldType.STRING).description("대륙 (ASIA / NORTH_AMERICA / EUROPE / SOUTH_AMERICA / AFRICA_OCEANIA)")
)
.build()
),
requestCookies(
cookieWithName(COOKIE_NAME).description("유저의 토큰")
)
));
}

@Test
void 작곡가를_삭제한다() throws Exception {
// given
Expand Down
Loading