-
Notifications
You must be signed in to change notification settings - Fork 0
[Step2] 로그인 기능을 구현한다. #3
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
31f28c3
c6700be
6ee28a0
82a7e21
410f375
34fb99c
e871fa4
4964077
f903a35
4961d8a
fc38099
9fec195
77c2908
e2f2b3d
3474f6f
a354f17
8680118
c111de5
120eca8
31aefb2
448da6f
437bb90
8b4d715
2aee685
86366f9
476b0bb
7ace8d2
0c8e9be
29d2a54
6fb162c
888f0d8
0c4ca1a
5848e30
3ce9889
6651a30
47425a5
7bf9371
8996bdf
115e351
ed2d131
4086fef
499210d
6ed6cf0
2ebb21f
ee949c3
21a2a98
b28f766
0dae279
34fe3e1
654720c
077e319
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,3 +1,70 @@ | ||
| plugins { | ||
| id("org.sonarqube") version("4.2.1.3168") | ||
| id("jacoco") | ||
| } | ||
|
|
||
| dependencies { | ||
| implementation(project(":mvc")) | ||
| } | ||
|
|
||
| tasks.named("test") { | ||
| useJUnitPlatform() | ||
| finalizedBy jacocoTestReport | ||
| } | ||
|
|
||
| jacoco { | ||
| toolVersion = project.getProperty("jacocoVersion") | ||
| } | ||
|
|
||
| jacocoTestReport { | ||
| dependsOn test | ||
| reports { | ||
| xml.required = true | ||
| csv.required = false | ||
| html.required = true | ||
| xml.destination file("${buildDir}/jacoco/index.xml") | ||
| csv.destination file("${buildDir}/jacoco/index.csv") | ||
| html.destination file("${buildDir}/jacoco/index.html") | ||
| } | ||
|
|
||
| afterEvaluate { | ||
| classDirectories.setFrom( | ||
| files(classDirectories.files.collect { | ||
| fileTree(dir: it, excludes: []) | ||
| }) | ||
| ) | ||
| } | ||
| finalizedBy("jacocoTestCoverageVerification") | ||
| } | ||
|
|
||
| jacocoTestCoverageVerification { | ||
| violationRules { | ||
| rule { | ||
| enabled = true | ||
| element = "CLASS" | ||
| limit { | ||
| counter = "BRANCH" | ||
| value = "COVEREDRATIO" | ||
| minimum = 0.50 | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| sonarqube { | ||
| properties { | ||
| property("sonar.host.url", System.getenv("SONAR_QUBE_SERVER_URL")) | ||
| property("sonar.login", System.getenv("SONAR_QUBE_TOKEN")) | ||
| property("sonar.sources", "src/main/java") | ||
| property("sonar.tests", "src/test/java") | ||
| property("sonar.language", "java") | ||
| property("sonar.projectKey", System.getenv("SONAR_PROJECT_KEY")) | ||
| property("sonar.projectName", System.getenv("SONAR_PROJECT_NAME")) | ||
| property("sonar.java.source", 17) | ||
| property("sonar.sourceEncoding", "UTF-8") | ||
| property("sonar.java.binaries", "${buildDir}/classes") | ||
| property("sonar.test.inclusions", "") | ||
| property("sonar.exclusions", "") | ||
| property("sonar.coverage.jacoco.xmlReportPaths", "${buildDir}/jacoco/index.xml") | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,15 @@ | ||
| package project.server.app; | ||
|
|
||
| import project.server.mvc.springframework.context.Application; | ||
| import project.server.mvc.Application; | ||
| import static project.server.mvc.tomcat.PortFinder.findPort; | ||
|
|
||
| public class BlogApplication { | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| Application.run(args); | ||
| public static void main(String[] args) { | ||
| String packages = "project"; | ||
| final int port = findPort(args); | ||
| final int tomcatThreadCount = 200; | ||
| Application application = new Application(packages, port, tomcatThreadCount); | ||
| application.start(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,10 @@ | |
| import project.server.mvc.servlet.http.HttpStatus; | ||
|
|
||
| public enum ErrorCodeAndMessages implements ErrorCodeAndMessage { | ||
| INVALID_SESSION(HttpStatus.UN_AUTHORIZED, "세션이 만료되었습니다."), | ||
| INVALID_PARAMETER(HttpStatus.BAD_REQUEST, "올바른 값을 입력해주세요."), | ||
| PAGE_NOT_FOUND(HttpStatus.NOT_FOUND, "페이지를 찾을 수 없습니다."); | ||
| PAGE_NOT_FOUND(HttpStatus.NOT_FOUND, "페이지를 찾을 수 없습니다."), | ||
| UN_AUTHORIZED(HttpStatus.UN_AUTHORIZED, "권한이 존재하지 않습니다."); | ||
|
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. 새로운 예외 메시지를 생성하고 싶을 때마다 enum 값이 추가되게 되나요? 만약 국가별로 다른 예외메시지를 띄워야되는 것처럼 예외 메시지에 변경이 일어나게 된다면 어떻게 처리하게 되나요?
Contributor
Author
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. 국가별로 다른 언어를 처리해야 하면 MessageSource 사용, Enum에 필드 추가 정도가 있을 것 같습니다. 만약 다국어가 많다면, 다른 방법을 사용할 것 같은데요, 이번 미션에서는 한국어 하나만 신경 쓰면 되기 때문에 다른 선택지는 고려하지 않았습니다. |
||
|
|
||
| private final HttpStatus httpStatus; | ||
| private final String errorMessage; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| package project.server.app.common.exception; | ||
|
|
||
| import static project.server.app.common.codeandmessage.failure.ErrorCodeAndMessages.INVALID_SESSION; | ||
|
|
||
| public class SessionExpiredException extends RuntimeException { | ||
|
|
||
| public SessionExpiredException() { | ||
| super(INVALID_SESSION.getErrorMessage()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| package project.server.app.common.exception; | ||
|
|
||
| import static project.server.app.common.codeandmessage.failure.ErrorCodeAndMessages.UN_AUTHORIZED; | ||
|
|
||
| public class UnAuthorizedException extends RuntimeException { | ||
|
|
||
| public UnAuthorizedException() { | ||
| super(UN_AUTHORIZED.getErrorMessage()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package project.server.app.common.login; | ||
|
|
||
| import static java.time.LocalDateTime.now; | ||
| import java.util.Objects; | ||
|
|
||
| public class LoginUser { | ||
|
|
||
|
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. 로그인한 유저에 대해서 객체로 만들어두니까 책임과 역할이 분리되서 좋은 것 같습니다! |
||
| private final Long userId; | ||
|
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. userId가 Wrapper타입이여야 했던 특별한 이유가 있나요?
Contributor
Author
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. 사용자 아이디가 long이면 값이 없다면 0으로 초기화 될거에요. Long이면 값이 없다면 null 값으로 들어오며, 값이 없다는 것을 |
||
| private final String loginIp; | ||
| private boolean valid; | ||
|
|
||
| public LoginUser( | ||
| Long userId, | ||
| String loginIp | ||
| ) { | ||
| this.userId = userId; | ||
| this.loginIp = loginIp; | ||
| } | ||
|
|
||
| public LoginUser(Session session) { | ||
|
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. 세션으로 로그인을 구현한 이유가 있나요?
Contributor
Author
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. 요구사항에 |
||
| this.userId = session.userId(); | ||
| this.loginIp = null; | ||
| this.valid = session.isValid(now()); | ||
| } | ||
|
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. username자리에 password를 넣으시는 이유가 뭐죠? 의도하신게 맞나요?
Contributor
Author
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. 이거 수정했는데 커밋에는 이렇게 남아있네요. 제 실수고, 변경됐습니다. |
||
|
|
||
| public Long getUserId() { | ||
| return userId; | ||
| } | ||
|
|
||
| public String getLoginIp() { | ||
| return loginIp; | ||
| } | ||
|
|
||
| public boolean isValid() { | ||
| return valid; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { | ||
| if (this == object) { | ||
| return true; | ||
| } | ||
| if (object == null || getClass() != object.getClass()) { | ||
| return false; | ||
| } | ||
| LoginUser loginUser = (LoginUser) object; | ||
| return userId.equals(loginUser.userId); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(userId); | ||
|
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. 단순하게 userId를 리턴하는 거에 비해서 Objects.hash를 이용하는 것이 이점이 있을까요?
Contributor
Author
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. Objects.hash 메서드는 사실 IntelliJ가 정의해주는대로 사용하고 있었는데, 학습해보니 이는 아래 두 가지 장점이 있네요. 이를 사용한다고 해서 성능이 좋아지거나, 혹은 이미 유니크한 값을 더 좋은 방법으로 식별/판별할 것 같지는 않구요, Null-Safe, 다중 인자 지원 정도 때문에 사용하는 것 같습니다.
|
||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return String.format("userId:%s", userId); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package project.server.app.common.login; | ||
|
|
||
| import java.time.LocalDateTime; | ||
| import java.util.Objects; | ||
|
|
||
| public record Session( | ||
|
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. 세션과 쿠키의 차이점에 대해 설명해주세요! |
||
| Long userId, | ||
| String sessionId, | ||
| LocalDateTime expiredAt | ||
| ) { | ||
|
|
||
| public boolean isValid(LocalDateTime expiredAt) { | ||
| return this.expiredAt.isAfter(expiredAt); | ||
|
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. 세션 키가 만료되면 Map에 남아있게 되나요?
Contributor
Author
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. 현재 구현에서는 세션이 만료되더라도 키가 Map에 남아있습니다. 간단하게 스케줄러를 사용해서 불필요한 세션을 주기적으로 제거해 주는 작업이 추가되면 좋을 것 같네요. 다음 PR 때 반영하겠습니다. |
||
| } | ||
|
|
||
| public String getUserIdAsString() { | ||
| return userId.toString(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { | ||
| if (this == object) { | ||
| return true; | ||
| } | ||
| if (object == null || getClass() != object.getClass()) { | ||
| return false; | ||
| } | ||
| Session session = (Session) object; | ||
| return sessionId.equals(session.sessionId); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(sessionId); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return String.format("userId:%s, sessionId:%s, expiredAt:%s", userId, sessionId, expiredAt); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package project.server.app.common.login; | ||
|
|
||
| import java.time.LocalDateTime; | ||
| import static java.time.LocalDateTime.now; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import static java.util.UUID.randomUUID; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import project.server.app.core.web.user.application.SessionManager; | ||
| import project.server.mvc.springframework.annotation.Component; | ||
|
|
||
| @Slf4j | ||
| @Component | ||
| public class SessionStore implements SessionManager { | ||
|
|
||
| private static final int FIFTEEN_MINUTES = 15; | ||
| private static final Map<Long, Session> factory = new ConcurrentHashMap<>(); | ||
|
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. HashMap 대신 ConcurrentHashMap을 사용하신 이유가 있나요?
Contributor
Author
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. 사실 세션, 쿠키와 같은 값들이 가로채지지 않으면 사용자 자신만 접근하기 때문에 HashMap을 사용해도 상관없습니다. 다만 사용자가 거의 없는 애플리케이션이기 그대로 두었습니다. 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. ConcurrentHashMap을 사용하면 동기화로 인한 성능저하가 크다고 생각하시나요? ConcurrnetHashMap의 동기화방식에 대해 설명해주세요. hashCode가 같은 세션이 동시에 여러개 생성되면 HashMap에서는 어떻게 되나요? |
||
|
|
||
|
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. 웹계층에서 사용자의 상태를 유지할 때 단점은 무엇이 있을까요?
Contributor
Author
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. 사용자 상태를 유지하면 가장 우려되는 점은 서버의 확장입니다. 멀티 서버 환경에서, 한 서버에 사용자 정보가 담겨있을 때, 로드 밸런싱을 사용하면, 해당 사용자가 이미 로그인 했더라도 로그인 하지 않은 것처럼 동작합니다. 다만, 이번 미션에서는 이를 고려하지 않아도 되기 때문에 서버에 세션 정보를 저장했습니다. |
||
| @Override | ||
| public Session createSession(Long userId) { | ||
| String uuid = randomUUID().toString(); | ||
|
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. randomUUID에 대해서 설명해주세요! 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. 또한 UUID 충돌에 대해 어떻게 생각하시나요 |
||
| LocalDateTime after15Minutes = now().plusMinutes(FIFTEEN_MINUTES); | ||
|
|
||
| Session newSession = new Session(userId, uuid, after15Minutes); | ||
| factory.put(userId, newSession); | ||
| return newSession; | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<Session> findByUserId(Long userId) { | ||
| return Optional.ofNullable(factory.get(userId)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package project.server.app.common.utils; | ||
|
|
||
| import java.util.Map; | ||
| import project.server.mvc.servlet.http.Cookie; | ||
| import project.server.mvc.servlet.http.Cookies; | ||
|
|
||
| public final class HeaderUtils { | ||
|
|
||
| private static final String SESSION_ID = "sessionId"; | ||
|
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. 오 좋네요! |
||
|
|
||
| private HeaderUtils() { | ||
| throw new AssertionError("올바른 방식으로 생성자를 호출해주세요."); | ||
| } | ||
|
|
||
| public static Long getSessionId(Cookies cookies) { | ||
| Map<String, Cookie> cookiesMap = cookies.getCookiesMap(); | ||
| Cookie findCookie = cookiesMap.get(SESSION_ID); | ||
| if (findCookie != null) { | ||
| return extractSessionId(findCookie); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static Long extractSessionId(Cookie cookie) { | ||
| if (cookie.value() != null) { | ||
| String sessionId = cookie.value(); | ||
| return parseLong(sessionId); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static Long parseLong(String sessionId) { | ||
| try { | ||
| return Long.valueOf(sessionId); | ||
| } catch (NumberFormatException exception) { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package project.server.app.core.domain.user; | ||
|
|
||
| public enum Deleted { | ||
| TRUE, | ||
|
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. boolean 값이 아닌 enum을 사용하신 이유가 있나요?
Contributor
Author
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. 이는 취향인데요, 저는
아래는 이전에 학습하면서 봤던 자료들인데요, 한 번 참조해보실 것을 추천드립니다. |
||
| FALSE | ||
| } | ||
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.
스레드 수 200개로 고정시키신 이유가 있나요?
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.
해당 자료를 참조해 200개로 맞추었습니다. 다만 이는 현재 내 CPU나 메모리 사용량 등 다른 추가적인 정보를 토대로 이를 선택하는게 좋을 것 같으므로 추후 문제가 있다면 수정하겠습니다.