-
Notifications
You must be signed in to change notification settings - Fork 31
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단계 - 쿠키를 이용한 로그인 구현 #68
base: sunullo_bugpigg
Are you sure you want to change the base?
Conversation
- 기존에 입력으로 받았던 호스트 정보를 상수로 수정하였습니다. - setRedirect** 메소드명을 changePathTo** 메소드명으로 변경하였습니다. - Login 실패시 리다이렉트할 주소도 추가하였습니다.
- requestline 을 관리하는 클래스를 추가하였습니다.
- RequestLine 클래스를 생성함에 따라, 기존에 필드로 관리되었던 Methodtype, URL 등을 requestLine 필드에서 호출할 수 있게 하였습니다.
- Request 객체를 가공하는 클래스를 추가하였습니다.
- 회원 정보를 추가, 조회, 삭제하는 클래스를 추가하였습니다. - SessionId 를 가공해서 반환하는 역할도 수행합니다.
- SessionId와 해당하는 유저 아이디를 관리하는 클래스를 추가하였습니다.
- 유저 로그인시, 생성한 SessionId를 Response header 에 추가하는 메소드를 수정하였습니다. - 유저 로그아웃시, 생성한 SessionId의 삭제를 위해 max-age=0으로 설정하는 메소드를 추가하였습니다.
- User 도메인 객체를 직접 생성하고 다뤘던 것을, UserManger 클래스를 사용하는 것으로 수정하였습니다.
- 로그아웃 기능 구현을 위해, 로그아웃 버튼의 href 경로를 수정하였습니다.
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.
안녕하세요. 선을로 & 벅픽, 리뷰어 dion입니다.
4단계 미션을 수행하느라 고생하셨습니다.
동작은 하지만 개선할 여지가 많이 보여 변경 요청을 드립니다. 코멘트 참고하시고 수정해주세요. 궁금한 점 있으면 코멘트 남기시거나 DM 주세요~
|
||
public class SessionDataBase { | ||
|
||
private static Map<String, String> sessions = Maps.newHashMap(); |
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.
왜 final
을 사용하지 않고, HashMap
을 사용해주신건가요?
public static void add(String sessionId, String userId) { | ||
sessions.put(sessionId, userId); | ||
} | ||
|
||
public static String findBySessionId(String sessionId) { | ||
return sessions.get(sessionId); | ||
} | ||
|
||
public static void remove(String sessionId) { | ||
sessions.remove(sessionId); | ||
} |
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.
static
메서드가 아니어도 동작하게끔 하는편이 좋을 것 같습니다.
public boolean isGetMethodType() { | ||
return requestLine.isGetMethodType(); | ||
} | ||
|
||
public boolean isPostMethodType() { | ||
return requestLine.isPostMethodType(); | ||
} |
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.
👍
String messageBody = request.getMessageBody(); | ||
userSave(messageBody, url); | ||
} | ||
UserManager userManager = new UserManager(request); |
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.
이 방식의 구현은 약간 의아하네요~request
를 의존성으로 받을 이유가 있을까요..?
userSave(messageBody, url); | ||
} | ||
UserManager userManager = new UserManager(request); | ||
String sessionId = userManager.action(); |
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.
왜 userManager.action
의 응답이 sessionId
를 리턴해주어야 했나요?
if (isLogin) { | ||
sb.append("Set-Cookie: sessionId = " + sessionId + "; Path=/\r\n"); | ||
} else { | ||
sb.append("Set-Cookie: sessionId = " + sessionId + "; max-age=0; Path=/\r\n"); | ||
} |
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.
굳이 나눌 필요가 있을까요?
public void changePathToHomePage() { | ||
redirectPath = BASE_PATH + HOME_PAGE_PATH; | ||
} | ||
|
||
public void changePathToSignUpPage() { | ||
redirectPath = BASE_PATH + SINE_UP_PAGE_PATH; | ||
} | ||
|
||
public void setRedirectSignUpPage() { | ||
path = signUpPage; | ||
public void changePathToLoginFailedPage() { | ||
redirectPath = BASE_PATH + LOGIN_FAILED_PAGE_PATH; | ||
} |
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.
이 코드는 매 번 다른 String
을 만들어 낼까요? 아니면 재사용할까요? 퀴즈입니다.
private void save(URL url, String messageBody) { | ||
Map<String, String> userInfo = HttpRequestUtils.parseQueryString(messageBody); | ||
User user = new User(userInfo.get("userId"), userInfo.get("password"), | ||
userInfo.get("name"), | ||
userInfo.get("email")); | ||
|
||
if (DataBase.findUserById(userInfo.get("userId")) == null) { | ||
DataBase.addUser(user); | ||
url.changePathToHomePage(); | ||
} else { | ||
url.changePathToSignUpPage(); | ||
} | ||
} | ||
|
||
private String login(URL url, String messageBody) { | ||
Map<String, String> userInfo = HttpRequestUtils.parseQueryString(messageBody); | ||
User user = DataBase.findUserById(userInfo.get("userId")); | ||
|
||
if (user.getUserId().equals(userInfo.get("userId")) && user.getPassword() | ||
.equals(userInfo.get("password"))) { | ||
String sessionId = UUID.randomUUID().toString(); | ||
SessionDataBase.add(sessionId, userInfo.get("userId")); | ||
|
||
url.changePathToHomePage(); | ||
return sessionId; | ||
} else { | ||
url.changePathToLoginFailedPage(); | ||
return ""; | ||
} | ||
} | ||
|
||
private String logout(URL url, Map<String, String> headers) { | ||
String value = headers.get("Cookie:"); | ||
String sessionId = value.split("=")[1]; | ||
|
||
SessionDataBase.remove(sessionId); | ||
url.changePathToHomePage(); | ||
|
||
return sessionId; | ||
} |
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.
메서드 각각이 중요한 로직을 다루고 있죠. 객체 분리의 신호입니다.
public String action() { | ||
URL url = request.getUrl(); | ||
String messageBody = request.getMessageBody(); | ||
|
||
if (request.isPostMethodType() && url.comparePath("/user/create")) { | ||
save(url, messageBody); | ||
} | ||
if (request.isPostMethodType() && url.comparePath("/user/login")) { | ||
return login(url, messageBody); | ||
} | ||
if (request.isGetMethodType() && url.comparePath("/user/logout")) { | ||
Map<String, String> headers = request.getHeaders(); | ||
return logout(url, headers); | ||
} | ||
|
||
return ""; |
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.
적절히 분기해주신건 좋습니다만, 이것이 UserManager
라는 이름과 어울리는지는 잘 모르겠습니다~
또한 action
같은 매우 일반적으로 쓰이는 이름보다 조금 더 구체적인 이름을 사용해주셨으면 좋겠네요.
private final Request request; | ||
|
||
public UserManager(Request request) { | ||
this.request = request; | ||
} |
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.
굳이 request
를 상태로 관리할 필요가 전혀 없겠죠. 심지어 이 Manager
는 request
를 바꿀 수도 없기에 재사용도 불가능합니다.
안녕하세요~ 선을로 & 벅픽입니다!
웹서버 4단계 PR 입니다
기능요구사항
http://localhost:8080/user/login.html
으로 이동해 로그인할 수 있다.프로그래밍 요구사항
추가 기능 구현
ㄴ max-age = 0 으로 하여, 쿠키 삭제
항상 좋은 리뷰 감사합니다~!