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

[피오 & 노리] 웹서버 4단계 - 쿠키를 이용한 로그인 구현 #67

Open
wants to merge 4 commits into
base: pio-nori
Choose a base branch
from

Conversation

NB993
Copy link

@NB993 NB993 commented Mar 31, 2022

안녕하세요!! 피오 & 노리 입니다. 👬

"웹서버 4 단계 - 쿠키를 이용한 로그인 구현" 완료하여 pull request 요청드립니다.

감사합니다! 🙇

3단계 리뷰 항목

  • 중복 회원가입 요청 처리 보완
  • 리다이렉션 메서드 재사용 가능하도록 보완
  • RequestLine 생성자 매개변수 변경

기능요구사항

  • 회원가입한 사용자로 로그인을 할 수 있어야 한다.
  • “로그인” 메뉴를 클릭하면 http://localhost:8080/user/login.html 으로 이동해 로그인할 수 있다.
  • 로그인이 성공하면 index.html로 이동하고, 로그인이 실패하면 /user/login_failed.html로 이동해야 한다.

프로그래밍 요구사항

  • 앞 단계에서 회원가입할 때 생성한 User 객체를 DataBase.addUser() 메서드를 활용해 메모리에 저장한다.
  • 아이디와 비밀번호가 같은지를 확인해서 로그인이 성공하면 응답 header의 Set-Cookie 값을 sessionId=적당한값으로 설정한다.
  • Set-Cookie 설정시 모든 요청에 대해 Cookie 처리가 가능하도록 Path 설정 값을 /(Path=/)로 설정한다.
  • 응답 header에 Set-Cookie값을 설정한 후 요청 header에 Cookie이 전달되는지 확인한다.

추가 요구 사항

  • (선택) 로그아웃을 구현한다.

NB993 and others added 4 commits March 31, 2022 14:11
- 로그인이 성공하면 index.html로 이동하고, 로그인이 실패하면
/user/login_failed.html로 이동시킨다.

- 아이디와 비밀번호가 같은지를 확인해서 로그인이 성공하면 응답 header의
Set-Cookie 값을 sessionId로 지정해준다. 값은 UUID로 생성한다.
- 로그아웃 요청이 오면 쿠키를 만료시킨다.
requestLine 정보를 ReqeustLine이 파싱하도록 함.
- 중복된 userId로 회원가입 요청이 들어올시, 예외를 발생시킨 후 /user/form.html로
이동시킨다.

- HttpResponse클래스의 리다이렉션 메서드의 재활용성을 높이기 위해
location을 매개변수로 받는다.
@NB993 NB993 added the review-BE Improvements or additions to documentation label Mar 31, 2022
@ksundong ksundong self-assigned this Apr 2, 2022
Copy link

@ksundong ksundong left a comment

Choose a reason for hiding this comment

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

안녕하세요. 피오 & 노리, 리뷰어 dion 입니다.

4단계 미션을 수행하느라 고생하셨습니다.

기능은 문제가 없으나, 보다 좋은 코드를 작성할 필요가 있다는 생각이 들어서 변경요청 드립니다. 고생하셨습니다.


public class SessionDataBase {

private static final Map<String, String> SESSIONS = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

HashMap 을 사용해주신 이유는 무엇인가요?

Comment on lines +10 to +16
public static void save(String sessionId, String userId) {
SESSIONS.put(sessionId, userId);
}

public static void remove(String sessionId) {
SESSIONS.remove(sessionId);
}
Copy link

Choose a reason for hiding this comment

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

메서드명 👍

@@ -22,8 +22,7 @@ public HttpRequest(BufferedReader br) throws IOException {

private void init() throws IOException {
String requestLine = URLDecoder.decode(br.readLine(), StandardCharsets.UTF_8);
String[] requestLineSplit = requestLine.split(" ");
this.requestLine = new RequestLine(requestLineSplit[0], requestLineSplit[1], requestLineSplit[2]);
this.requestLine = new RequestLine(requestLine);
this.headers = IOUtils.readRequestHeader(br);
Copy link

Choose a reason for hiding this comment

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

init() 메서드 같은건 필요없죠. 앞으로도 계속 지양해주세요.

Copy link

Choose a reason for hiding this comment

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

BufferedReader 가 상태로 관리되는 이유가 있나요?

Comment on lines +28 to +30
} catch (IOException e) {
log.error(e.getMessage());
}
Copy link

Choose a reason for hiding this comment

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

이건 올바른 예외처리가 아닙니다. 지양해주세요.

Comment on lines 24 to +47
dos.writeBytes("HTTP/1.1 302 FOUND \r\n");
dos.writeBytes("Location: http://localhost:8080/index.html\r\n");
dos.writeBytes("Location: http://localhost:8080" + path + "\r\n");
dos.writeBytes("Set-Cookie: sessionId=" + cookie + "; Max-Age=-1; path=/");
dos.writeBytes("\r\n");
} catch (IOException e) {
log.error(e.getMessage());
}
}

public void response302WithCookieHeader(String path, String cookie) {
try {
dos.writeBytes("HTTP/1.1 302 FOUND \r\n");
dos.writeBytes("Location: http://localhost:8080" + path + "\r\n");
dos.writeBytes("Set-Cookie: sessionId=" + cookie + "; path=/");
dos.writeBytes("\r\n");
} catch (IOException e) {
log.error(e.getMessage());
}
}

public void response302Header(String path) {
try {
dos.writeBytes("HTTP/1.1 302 FOUND \r\n");
dos.writeBytes("Location: http://localhost:8080" + path + "\r\n");
Copy link

Choose a reason for hiding this comment

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

중복되는 코드가 많네요. 재사용을 고려할 필요는 없을까요?

try {
dos.writeBytes("HTTP/1.1 302 FOUND \r\n");
dos.writeBytes("Location: http://localhost:8080/index.html\r\n");
dos.writeBytes("Location: http://localhost:8080" + path + "\r\n");
Copy link

Choose a reason for hiding this comment

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

이 객체가 stream 을 직접 가지고서 응답을 반환하는 것은 도메인을 담당하는 객체가 출력을 하는 것과 동일합니다. 지양해주세요.

Comment on lines +39 to +86
HttpResponse httpResponse = new HttpResponse(out);

if (httpRequest.getPath().contains("/user/create")) {
if (httpRequest.getPath().equals("/user/create")) {
User user = new User(
httpRequest.getParameter("userId"),
httpRequest.getParameter("password"),
httpRequest.getParameter("name"),
httpRequest.getParameter("email")
);
DataBase.addUser(user);
HttpResponse httpResponse = new HttpResponse(out);
httpResponse.response302Header();

try {
DataBase.addUser(user);
httpResponse.response302Header("/index.html");
} catch (IllegalArgumentException e) {
log.debug("exception: {}", e.getMessage());
httpResponse.response302Header("/user/form.html");
}
return;
}

if (httpRequest.getPath().equals("/user/login")) {
User user = DataBase.findUserById(httpRequest.getParameter("userId"));
if (user == null) {
httpResponse.response302Header("/user/login_failed.html");
return;
}
if (!user.getPassword().equals(httpRequest.getParameter("password"))) {
httpResponse.response302Header("/user/login_failed.html");
return;
}
String sessionId = UUID.randomUUID().toString();
log.debug("return cookie: {}", sessionId);
SessionDataBase.save(sessionId, user.getUserId());
httpResponse.response302WithCookieHeader("/index.html", sessionId);
return;
}

if (httpRequest.getPath().equals("/user/logout")) {
Map<String, String> cookies = HttpRequestUtils.parseCookies(
httpRequest.getHeader("Cookie"));
String sessionId = cookies.get("sessionId");
log.debug("sessionId = {}", sessionId);
if (sessionId == null) {
httpResponse.response302Header("/index.html");
return;
}
httpResponse.response302WithExpiredCookieHeader("/index.html", sessionId);
SessionDataBase.remove(sessionId);
Copy link

Choose a reason for hiding this comment

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

메서드가 너무 길죠. 리팩토링 해주세요. depth 2단계는 여기서도 지양해주셔야 합니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-BE Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants