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

Feature/#41 깃허브 로그인 기능 구현 #50

Merged
merged 29 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ba1c7c3
chore: 깃허브 로그인 변수 포맷 추가
hyeonjerry Jul 18, 2023
2f33d70
feat: 깃허브 accessToken 요청 클라이언트 구현
hyeonjerry Jul 18, 2023
8c528a6
feat: 깃허브 프로필 요청 클라이언트 구현
hyeonjerry Jul 18, 2023
52eb783
feat: github code로부터 사용자 토큰 반환 기능 구현
hyeonjerry Jul 18, 2023
1f9b17c
feat: github 로그인 redirect api 구현
hyeonjerry Jul 18, 2023
9d99baf
Merge branch 'main' of https://github.com/woowacourse-teams/2023-emms…
hyeonjerry Jul 18, 2023
d1cd05c
fix: ServiceIntegrationTestHelper 추가
hyeonjerry Jul 19, 2023
85e3454
refactor: 커스텀 예외 추가
hyeonjerry Jul 19, 2023
eff599f
test: 로그인 서비스 테스트 구현
hyeonjerry Jul 19, 2023
1a101cb
chore: jwt 라이브러리 추가
hyeonjerry Jul 19, 2023
bb1b348
feat: 토큰 생성 및 발급 기능 구현
hyeonjerry Jul 19, 2023
5422e89
feat: 새로운 유저 여부 정보 추가
hyeonjerry Jul 19, 2023
38a1afb
refactor: JwtTokenProvider 패키지 이동
hyeonjerry Jul 20, 2023
4701ddb
feat: JwtTokenProvider Subject 추출 기능 구현
hyeonjerry Jul 20, 2023
6179009
feat: Authorization 헤더 추출 기능 구현
hyeonjerry Jul 20, 2023
7a7b853
feat: ArgumentResolver에 TokenProvider 등록
hyeonjerry Jul 20, 2023
5abbf9b
test: TokenProvider 테스트 구현
hyeonjerry Jul 20, 2023
28f71c0
refactor: 불필요한 코드 제거
hyeonjerry Jul 20, 2023
feeab0e
refactor: Custom 예외 추가
hyeonjerry Jul 20, 2023
e7f913d
refactor: 피드백 반영
hyeonjerry Jul 22, 2023
da1decf
refactor: 패키지 및 클래스명 변경
hyeonjerry Jul 22, 2023
0cd66d0
test: 불필요한 검증 제거
hyeonjerry Jul 22, 2023
c79cb57
fix: default 생성자 오류 수정
hyeonjerry Jul 22, 2023
39c97a4
feat: githubId 컬럼 추가
hyeonjerry Jul 22, 2023
c003c2a
feat: githubId로 사용자를 조회하는 기능 구현
hyeonjerry Jul 22, 2023
4d99f76
refactor: Authorization 추출 메서드 리팩터링
hyeonjerry Jul 22, 2023
1cc219d
refactor: 메서드 추출 및 예외처리 추가
hyeonjerry Jul 22, 2023
b52fc15
refactor: 패키지면 변경
hyeonjerry Jul 23, 2023
fac42f4
refactor: MemberOnly 어노테이션 제거
hyeonjerry Jul 24, 2023
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
3 changes: 3 additions & 0 deletions backend/emm-sale/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ dependencies {
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.springframework.restdocs:spring-restdocs-mockmvc'
asciidoctorExt 'org.springframework.restdocs:spring-restdocs-asciidoctor'

// jwt
implementation 'io.jsonwebtoken:jjwt:0.9.1'
}

tasks.named('test') {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.emmsale.login.application;
Copy link
Collaborator

Choose a reason for hiding this comment

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

패키지 상으로도 그렇고 기능적으로도 Service에 해당하는 것 같은데 클래스명을 GithubClientService가 아니라 GithubClient로 지어주신 이유가 있을까요??

제가 OAuth쪽은 익숙하지 않아서 잘 모르는 것일 수도 있습니다...!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

컨트롤러의 요청을 받아 도메인 로직을 실행하고 이것을 반환하는 것이 아니므로 서비스라고 짓지 않았습니다.


import com.emmsale.login.application.dto.GithubAccessTokenRequest;
import com.emmsale.login.application.dto.GithubAccessTokenResponse;
import com.emmsale.login.application.dto.GithubProfileResponse;
import com.emmsale.login.exception.LoginException;
import com.emmsale.login.exception.LoginExceptionType;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.HttpEntity;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;
import org.springframework.http.MediaType;
import org.springframework.stereotype.Component;
import org.springframework.web.client.HttpClientErrorException;
import org.springframework.web.client.RestTemplate;

@Component
public class GithubClient {

@Value("${github.client.id}")
private String clientId;
@Value("${github.client.secret}")
private String clientSecret;
@Value("${github.url.access-token}")
private String accessTokenUrl;
@Value("${github.url.profile}")
private String profileUrl;

public String getAccessTokenFromGithub(final String code) {
final GithubAccessTokenRequest githubAccessTokenRequest = new GithubAccessTokenRequest(
code,
clientId,
clientSecret
);

final HttpHeaders headers = new HttpHeaders();
headers.add("Accept", MediaType.APPLICATION_JSON_VALUE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

상수는 따로 빼주거나, 아니면 spring 이 제공해주는 HttpHeaders.ACCEPT 를 사용해보시는건 어떠신가요?


final HttpEntity httpEntity = new HttpEntity(githubAccessTokenRequest, headers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

HttpEntity를 row 타입으로 사용하고 있는데 이유가 있으실까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

다른 코드를 참고하며 작성하다 보니 놓친 것 같습니다.
raw 타입을 사용할 경우 타입 안정성이 떨어질 뿐만 아니라 슈퍼 클래스의 메서드들도 모두 지워져서 GithubAccessTokenRequest 클래스의 메서드 또한 사용할 수 없는 문제가 발생하네요!!

HttpEntity<GithubAccessTokenRequest> httpEntity;
httpEntity.getBody().getClientId(); // O

HttpEntity httpEntity;
httpEntity.getBody().getClientId(); // Cannot resolve method 'getClientId' in 'Object'

final RestTemplate restTemplate = new RestTemplate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

RestTemplate 객체를 싱글톤으로 관리해보시는건 어떠신가요? 반복적으로 사용되는 것 같아서요~


final String accessToken = restTemplate
.exchange(accessTokenUrl, HttpMethod.POST, httpEntity, GithubAccessTokenResponse.class)
.getBody()
.getAccessToken();
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 잘 몰라서 그러는데 get함과 동시에 accessToken이 존재하지 않는다면 바로 NPE가 발생하나요??

Method invocation 'getAccessToken' may produce 'NullPointerException' 인텔리제이에서 이런 문구가 보여서요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

final String accessToken = REST_TEMPLATE
    .exchange(accessTokenUrl, HttpMethod.POST, httpEntity, GithubAccessTokenResponse.class)
    .getBody()
    .getAccessToken();

위 코드에서 .exchange() 메서드의 내부를 보면 return nonNull(execute(...));을 반환하고 있습니다.
여기서 nonNull() 메서드를 보면 Assert.state(result != null, "No result");를 통해 resultnull일 경우 에러를 발생시킵니다.
따라서, .getBody().getAccessToken()을 했을 때 NPE가 발생할 일은 없을 것 같습니다!!

RestTemplate과 동일한 라이브러리는 아니지만 유사항 사례가 있어 참고로 남깁니다.
Method invocation may produce NullPointerException Retrofit Body


if (accessToken == null) {
throw new LoginException(LoginExceptionType.NOT_FOUND_GITHUB_ACCESS_TOKEN);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

유효성 검사 부분은 메서드로 추출해주는 편이 가독성에 더 좋을 것 같습니다!

return accessToken;
}

public GithubProfileResponse getGithubProfileFromGithub(final String accessToken) {
final HttpHeaders headers = new HttpHeaders();
headers.add("Authorization", "token " + accessToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

위 리뷰와 같이 상수화를 시키거나 스프링이 제공해주는 것을 사용해보는건 어떨까요?


final HttpEntity httpEntity = new HttpEntity(headers);
final RestTemplate restTemplate = new RestTemplate();

try {
return restTemplate
.exchange(profileUrl, HttpMethod.GET, httpEntity, GithubProfileResponse.class)
.getBody();
} catch (final HttpClientErrorException e) {
throw new LoginException(LoginExceptionType.INVALID_GITHUB_ACCESS_TOKEN);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.emmsale.login.application;

import io.jsonwebtoken.Claims;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SignatureAlgorithm;
import java.util.Date;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;

@Component
public class JwtTokenProvider {

@Value("${security.jwt.token.secret-key}")
private String secretKey;
@Value("${security.jwt.token.expire-length}")
private long additionalValidityMillisecond;

public String createToken(final String memberId) {
final Claims claims = Jwts.claims().setSubject(memberId);
final Date now = new Date();
final Date expirationTime = new Date(now.getTime() + additionalValidityMillisecond);

return Jwts.builder()
.setClaims(claims)
.setIssuedAt(now)
.setExpiration(expirationTime)
.signWith(SignatureAlgorithm.HS256, secretKey)
.compact();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.emmsale.login.application;

import com.emmsale.login.application.dto.GithubProfileResponse;
import com.emmsale.login.application.dto.TokenResponse;
import org.springframework.stereotype.Service;

@Service
public class LoginService {

private final GithubClient githubClient;
private final JwtTokenProvider tokenProvider;

public LoginService(final GithubClient githubClient, final JwtTokenProvider tokenProvider) {
this.githubClient = githubClient;
this.tokenProvider = tokenProvider;
}

public TokenResponse createToken(final String code) {
final String githubAccessToken = githubClient.getAccessTokenFromGithub(code);
final GithubProfileResponse githubProfileFromGithub = githubClient.getGithubProfileFromGithub(
githubAccessToken);

final Long githubId = githubProfileFromGithub.getGithubId();
//TODO: githubId를 통해 memberId 조회 기능 구현
final Long memberId = 1L;
final String accessToken = tokenProvider.createToken(memberId.toString());

return new TokenResponse(memberId, accessToken);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.emmsale.login.application.dto;

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.AllArgsConstructor;
import lombok.Getter;

@AllArgsConstructor
@Getter
public class GithubAccessTokenRequest {

private final String code;
@JsonProperty("client_id")
private final String clientId;
@JsonProperty("client_secret")
private final String clientSecret;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.emmsale.login.application.dto;

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;

@AllArgsConstructor
@NoArgsConstructor(access = AccessLevel.PROTECTED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 NoArgsCounstructor를 PROTECTED로 추가하신 이유가 있을까요?

@Getter
public class GithubAccessTokenResponse {

@JsonProperty("access_token")
private String accessToken;
@JsonProperty("token_type")
private String tokenType;
private String scope;
private String bearer;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.emmsale.login.application.dto;

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;

@AllArgsConstructor
@NoArgsConstructor(access = AccessLevel.PROTECTED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

얘도요!!

@Getter
public class GithubProfileResponse {

@JsonProperty("id")
private String githubId;
@JsonProperty("name")
private String name;
@JsonProperty("login")
private String username;
@JsonProperty("avatar_url")
private String imageUrl;

public Long getGithubId() {
return Long.valueOf(githubId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.emmsale.login.application.dto;

import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;

@AllArgsConstructor
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
public class TokenResponse {

private Long memberId;
private String accessToken;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.emmsale.login.exception;

import com.emmsale.base.BaseException;
import com.emmsale.base.BaseExceptionType;

public class LoginException extends BaseException {

private final LoginExceptionType exceptionType;

public LoginException(final LoginExceptionType exceptionType) {
super(exceptionType.errorMessage());
this.exceptionType = exceptionType;
}

@Override
public BaseExceptionType exceptionType() {
return exceptionType;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.emmsale.login.exception;

import com.emmsale.base.BaseExceptionType;
import org.springframework.http.HttpStatus;

public enum LoginExceptionType implements BaseExceptionType {

NOT_FOUND_GITHUB_ACCESS_TOKEN(
HttpStatus.BAD_REQUEST,
"GitHub Access Token을 찾을 수 없습니다."
),
INVALID_GITHUB_ACCESS_TOKEN(
HttpStatus.NOT_FOUND,
"GitHub Profile을 찾을 수 없습니다."
Copy link
Collaborator

Choose a reason for hiding this comment

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

'~을 찾을 수 없습니다'라는 종류의 에러라면 INVALID_~보다는 NOT_FOUND_~라는 수식어가 더 적절한 것 같다는 생각이 듭니다!
INVALID_GITHUB_ACCESS_TOKEN=>NOT_FOUND_GITHUB_PROFILE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저는 GithubProfile을 찾을 수 없는 이유가 유효하지 않은 github access token 때문이라고 생각하여 INVALID_GITHUB_ACCESS_TOKEN라고 작성하였습니다.
오류 메시지를 통해 현상을 알려주는게 좋을까요? 원인을 알려주는게 좋을까요?🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 프롤로그 에러 목록에서 에러 명과 에러 메시지가 전달하는 내용이 동일한 것을 보고 코멘트를 드렸었는데,
말씀을 듣고 보니 원인과 현상을 어떻게 나타내야 할지 고민이 되네요🤔
이건 저희끼리 함께 이야기해보고 커스텀 예외에 대한 규칙을 보완해도 좋을 것 같아요.

제 생각을 말씀드리자면 원인이 조금 더 디테일한 정보를 줄 수 있으니 오류 메시지를 'Github Access Token이 유효하지 않다'라고 바꾸는 것도 좋아 보여요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이러한 에러 메시지의 경우에는, 어떻게 하면 이 API를 사용하는 사람(저희한테선 안드로이드겠죠?)에게 더 잘 이해가 되는지를 고려해볼 필요가 있다고 생각합니다.

저는 그런 입장에서 유효하지 않다는 의미보다 "찾을 수 없다"는 의미가 더 중요한 것 같아요.
그래서 NotFound를 유지하는 것이 더 괜찮아 보입니다.

);

private final HttpStatus httpStatus;
private final String errorMessage;

LoginExceptionType(final HttpStatus httpStatus, final String errorMessage) {
this.httpStatus = httpStatus;
this.errorMessage = errorMessage;
}

@Override
public HttpStatus httpStatus() {
return httpStatus;
}

@Override
public String errorMessage() {
return errorMessage;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.emmsale.login.ui;

import com.emmsale.login.application.LoginService;
import com.emmsale.login.application.dto.TokenResponse;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/login")
public class LoginController {

private final LoginService loginService;

public LoginController(final LoginService loginService) {
this.loginService = loginService;
}

@GetMapping("/github/callback")
public ResponseEntity<TokenResponse> login(@RequestParam final String code) {
if (code == null) {
return ResponseEntity.badRequest().build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

badRequest에 code관련된 에러 메시지를 추가해주면 어떨까요?
CustomException으로 처리해줘도 좋을 것 같아요

}
return ResponseEntity.ok().body(loginService.createToken(code));
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 코드를 이해한 바로는 TokenResponse의 accessToken이
실제 저희가 사용하는 Jwt 토큰인 거 같은데, 혹시 jwt 토큰만이 아닌 id까지 포함된
TokenResponse를 반환하는 이유가 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

안드로이드에서 사용자의 id를 함께 보내줄 것을 요청했기 때문입니다.
사용자의 id를 복사해서 친구에게 공유하여 친구신청 할 수 있는 기능 때문에 id를 함께 반환하도록 협의 했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그렇게 논의했군요. 이해했습니다.

음.. 친구 신청에서 필요한 사용자의 Id는 실제 DB에서 관리되는 member pk인 member_id가 아니라,
member의 uuid(figma에서 정의된 영어+숫자로 구성된 id목록)잖아요?

지금은 member에서 uuid가 지정되진 않아서, 뭔가 추가적인 작업을 할 순 없는데, uuid가 추가되면 변경해야할 것 같긴하네요

지금은 이대로만 두고, 추후에 추가해야 할 것 같아요. 수고했습니다.

}
}
14 changes: 14 additions & 0 deletions backend/emm-sale/src/main/resources/application.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,17 @@ logging:
descriptor:
sql:
BasicBinder: TRACE

github:
client:
id: client_id
secret: client_secret
url:
access-token: access-token_url
profile: profile_url

security:
jwt:
token:
secret-key: secret_key
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 데이터를 추후 어디에 보관할지에 대한 논의가 필요할듯 하네요!(.env 파일을 사용한다든지)
참고 링크
추후 이 글의 yml 파일 내 환경변수 설정 섹션을 참조해보면 좋을 것 같아요. 저도 잘 몰라서 남겨봅니다ㅎㅎ;

expire-length: 3_600_000_000
Loading