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

7주차 미션 / 서버 3조 김나은 #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Nico1eKim
Copy link

No description provided.

Copy link

@JangIkhwan JangIkhwan left a comment

Choose a reason for hiding this comment

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

코드 리뷰가 늦어서 미안해요 😢 미션 수행하느라 고생하셨어요

Comment on lines +48 to +53
log.info("createQuestionV1");

Question question = new Question(writer, title, contents, 0);
questionRepository.insert(question);

return "redirect:/";

Choose a reason for hiding this comment

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

여기에 로그인이 되어 있는지 확인하는 인가 처리가 추가되면 좋을 것 같아요. 실제 서비스에서는 post api마다 해당 사용자가 요청을 수행할 자격이 있는지 확인하는 인가 처리가 필요해요. 이미 form을 받을 때 확인했는데 또 확인할 필요가 있나 싶을 수 있지만 안전한 서버를 만드려면 최소한의 확인은 필요해요

Comment on lines +43 to +46
log.info("createUserV1");
User createdUser = new User(userId, password, name, email);
userRepository.insert(createdUser);
return "redirect:/user/list";

Choose a reason for hiding this comment

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

userId가 일치하는 회원이 존재하는지 확인하는 절차가 추가하면 좋을 것 같아요. 실제 서비스에서는 동일한 id를 갖는 회원이 존재하면 안되니까요.

Comment on lines +96 to +104
public String updateUserV1(@RequestParam("userId") String userId,
@RequestParam("password") String password,
@RequestParam("name") String name,
@RequestParam("email") String email) {
log.info("updateUserV1");
User updatedUser = new User(userId, password, name, email);
userRepository.update(updatedUser);
return "redirect:/user/list";
}

Choose a reason for hiding this comment

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

다른 수정 api에도 인가 처리를 구현하면 좋을 것 같아요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants