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

[Feat] change login to OIDC #52

Merged
merged 9 commits into from
Nov 20, 2024
Merged

[Feat] change login to OIDC #52

merged 9 commits into from
Nov 20, 2024

Conversation

goldentrash
Copy link
Contributor

@goldentrash goldentrash commented Nov 17, 2024

하려고 한 것

  • 학번/비밀번호 기반 사용자 인증을 Google OIDC로 변경
  • Session 기반 인증에서 JWT 기반 인증으로 변경
    • check-login 삭제
    • change-passowrd 삭제

몰랐던 부분

  • Spring Seucirty의 OAuth2 기능이 JWT base인 줄 알았습니다. 그래서 자동으로 JWT 만들어줄줄
  • 그런데 Session Base더라고요? 클로드랑 함께 열심히 만들어 보았습니다.

추가 변경 사항

  • 기존 password 기반 인증 로직 삭제
    • test annotation (WithCustomUser) 삭제 (header에 jwt 첨부하는 방식으로 수정)
  • 출석 후 redirection url 수정 (추후 대안 마련 필요) 출석 후 404 응답 #50
  • Github Actions Secret 수정 (application-dev.yaml 최신화)

@goldentrash goldentrash added the enhancement New feature or request label Nov 17, 2024
@goldentrash goldentrash self-assigned this Nov 17, 2024
@goldentrash goldentrash marked this pull request as ready for review November 18, 2024 13:59
Copy link
Collaborator

@ekgns33 ekgns33 left a comment

Choose a reason for hiding this comment

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

비밀번호 없애는 작업이 매우 번거로웠을 것 같네요 😢 고생하셨습니다! 리뷰는 그냥 개인적 소견만 달아서 문제 없다싶으면 머지하셔도 상관없어요!

try {
OidcUser oidcUser = super.loadUser(userRequest);
return processOAuth2User(oidcUser);
} catch (Exception ex) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[질문] Exception은 범위가 조금 큰 것 같습니당 혹시 좁힐 수 있을까요? 아니면 그냥 어떤 에러라도 처리하는게 맞을까요

Copy link
Contributor Author

@goldentrash goldentrash Nov 20, 2024

Choose a reason for hiding this comment

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

이 부분에 대해서는 저 역시 고민입니다, 하지만 우선 기능상 Critical하지 않고, Filter 특성 상 ControllerAdvice로 처리할 수 없기 때문에 발생 가능한 모든 Case에 대비하려고 합니다.

.compact();
}

public Authentication getAuthentication(String token) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[제안-답할필요X]
메서드의 내용을 보면 token의 claims의 필드를 get하여 Principal에 필요한 데이터로 파싱하고 있네요.
메서드의 인자를 token으로 주는데 claims로 넘기는 것은 어떨까요?

import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;

public Claims extractClaimsFromToken(String token) {
  return parseClaims(token);
}

public Authentication getAuthenticationFromTokenClaims(Claims claims) {
  authorities = getAuthroritiesFromClaims(claims);
  principal = createPrincipalFromClaims(claims);
  return new UsernamePasswordAuthenticationToken(principal, "", authorities);
}

Copy link
Contributor Author

@goldentrash goldentrash Nov 20, 2024

Choose a reason for hiding this comment

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

반영했습니다! 더해서 실수로 삭제한 코멘트를 반영해서 인증 실패 시 log를 추가했습니다.

[제안 - 답할 필요 X]여기서 Error-catch하면 로그를 남기는건 어떤가요

단, 로그인하지 않은 경우(IllegalArgumentException)에는 별도로 분기해서 log를 남기지 않습니다.

@gdsc-konkuk gdsc-konkuk deleted a comment from ekgns33 Nov 20, 2024
Copy link

sonarcloud bot commented Nov 20, 2024

@goldentrash goldentrash merged commit 2243d61 into main Nov 20, 2024
2 checks passed
@goldentrash goldentrash deleted the feat/change-login-to-OIDC branch November 20, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants