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

6주차 리뷰 부탁드립니다. #58

Merged
merged 69 commits into from
Oct 14, 2024
Merged

6주차 리뷰 부탁드립니다. #58

merged 69 commits into from
Oct 14, 2024

Conversation

Kdonghs
Copy link
Contributor

@Kdonghs Kdonghs commented Oct 11, 2024

빌드 및 배포 과정에서의 궁금증입니다.

앞서서 현재 진황과정에 대해 설명을 드리겠습니다.

  • Amazon Lightsail 인스턴스에 배포
  • github action을 통한 빌드 및 배포 자동화

HTTPS 설정 시 의문

  • 구글 로그인 관련 로직을 사용하기 위해서 HTTPS 설정이 필수였기 때문에 HTTPS 설정을 진행했습니다.
  • 첫번째로는 Let's encrypt인증서를 cert-bot을 통해 받고, application.yml에 해당 설정 부분들을 추가하는 방식으로 진행을 했었습니다.
    •           port: 443
                ssl:
                  enabled: true
                  key-store: /etc/letsencrypt/live/seamlessup.com/keystore.p12
                  key-store-password: ${{
                    secrets.KEYSTORE_PASSWORD
                  }}
                  key-store-type: PKCS12
                  key-alias: tomcat```
      
    • 이 방법을 써도 괜찮은가요?
    • 하지만 위의 방법을 쓰다가 막혔고 다른 방법을 찾아보게 되었습니다.
  • 두번째로 Lightsail 자체 로드밸런서 기능을 쓰면, SSL/TLS인증서를 발급해 주고 HTTPS 설정도 해주어서 이를 활용하기로 했습니다.
    • 도메인설정을 아래와 같이 변경했습니다.

      • seamlessup.com(저희의 도메인 입니다.) -> 연결된 로드밸런서의 IP
    • 그리고 설명을 읽어 보니 흐름이 아래와 같았습니다.

    • HTTP로 seamlessup.com 연결 시도시: 해당 도메인은 로드밸런서로 연결 -> 로드밸런서가 HTTP 포트 80에 대한 연결은 HTTPS 포트 443으로 리디렉션 -> 로드밸런서가 80포트로 인스턴스에 내보냄 -> 인스턴스는 스프링부트 앱 8080포트로 보내야함

    • 여기서 인스턴스80 -> 스프링8080으로 포트포워딩을 하고자

      • 첫째로 application.yml에 server: port: 80설정을 통해 받아보려했지만 안되었고
      • 둘째 Nginx를 설치하여
      server_name seamlessup.com;
      
      location / {
          proxy_pass http://localhost:8080;
          proxy_set_header Host $host;
          proxy_set_header X-Real-IP $remote_addr;
          proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
      }```
      위와 가은 설정을 통해 80 -> 8080으로 포트포워딩을 하여 이를 해결했습니다.
      
    • 둘 중 어느 방향이 맞는 방향인가요?

    • 처음 도전해보는 분야라서, 어떠한 이유가 있어서 해당 기술을 사용한 것이 아닌 "그저 문제를 해결하다보니 이런 기술을 쓰게 되었다." 이기 때문에 해당 방법들이 맞는 것인지 궁금합니다.

seoyoung-dev and others added 30 commits October 3, 2024 16:57
조서영 5주차 기능 구현
- PathVariable로 memberId 받아 쓰던 것을 헤더의 jwt토큰에서 email을 뽑아 쓰는 것으로 대체하도록 수정
- 멤버 CRUD에 팀장 or 팀원들만 접근할 수 있게 서비스 계층에서 jwt토큰에서 뽑아 권한 검증 로직 추가.
sunandrabbit and others added 25 commits October 9, 2024 22:35
Week6 조서영 기능 구현
- 일단 빌드 되도록 수정해놓고 PR넣으라고 해서 이렇게 commit함
- 역할 인증 때문에 memberId받지않고 HttpServletRequest를 받아서 구현했는데, 이렇게 하니까 정상적인 테스트 안됨.
- 반환 타입 Entity로 두니까 테스트할 때 불편해서 임의로 ResponseDTO 씌워놓음
- 토큰 없는 init데이터라서, 토큰으로부터 email뽑는 테스트가 안됨. 그래서 memberId로 CRUD하도록 다시 수정함 -> 추후에 팀장님이 테스트 토큰 완성하시면 다시 또 수정해놓을 예정
…weekly

# Conflicts:
#	src/main/java/team1/BE/seamless/repository/UserRepository.java
#	src/main/java/team1/BE/seamless/util/auth/SecurityConfig.java
- 프로젝트 기간에 관한 인증 로직
- 프로젝트 기간에 관한 인증 로직
- 팀장인지에 대한 검증
# Conflicts:
#	src/main/java/team1/BE/seamless/repository/ProjectRepository.java
#	src/main/java/team1/BE/seamless/repository/UserRepository.java
#	src/main/java/team1/BE/seamless/util/auth/SecurityConfig.java
w6 Weekly2develop
w6 Develop2master
@yoo-jaein yoo-jaein self-requested a review October 12, 2024 10:42
Copy link

@yoo-jaein yoo-jaein left a comment

Choose a reason for hiding this comment

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

6주차 고생 많으셨습니다!
코멘트 확인 부탁드립니다

@@ -78,6 +78,9 @@ public static class Update {
public Update(String name, String remark, Integer progress, Long memberId,
LocalDateTime startDate,
LocalDateTime endDate) {
if (endDate.isBefore(startDate)) {
throw new BaseHandler(HttpStatus.FORBIDDEN, "종료시간은 시작시간보다 이전일 수 없습니다.");

Choose a reason for hiding this comment

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

FORBIDDEN은 요청한 사람이 그 작업을 수행할 권한이 없음을 나타냅니다.
(예: 일반 유저가 관리자 페이지에 접속 시도)
여기선 FORBIDDEN보다 BAD_REQUEST가 더 적절한 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

오 FORBIDDEN과 BAD_REQUEST를 정확하게 구분해서 사용하지 않았던 것 같아요. 수정하겠습니다!

@Valid @PathVariable("project_id") Long projectId) {
return new SingleResult<>(attendURLService.generateAttendURL(req, projectId));
@Valid @PathVariable("project_id") Long projectId,
@Valid @PathVariable("user-id") Long userId) {

Choose a reason for hiding this comment

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

언더바를 쓸 것인지, 하이픈을 쓸 것인지 (project_id, user-id) 한 쪽으로 네이밍 컨벤션을 맞추면 좋겠습니다.

}

@Operation(summary = "팀원 전체 조회")
@GetMapping
public PageResult<MemberEntity> getMemberList(@Valid @PathVariable("project_id") Long projectId,
@Valid MemberRequestDTO.getMemberList memberListRequestDTO) {
@Valid MemberRequestDTO.getMemberList memberListRequestDTO,
HttpServletRequest req) {

Choose a reason for hiding this comment

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

인증을 위해 HttpServletRequest를 서비스 레이어에서 직접 다루고 있네요.
HttpServletRequest에 의존하면 테스트 작성이 어렵습니다. (서비스 로직을 테스트해야 하는데 목 http 요청이 필요해짐)

ParsingParam을 통한 토큰 검사 로직도 메서드마다 반복되고 있어 개선이 이뤄지면 좋을 부분 같습니다.

(예: 스프링 시큐리티로 UserDetails를 구현하고 꺼내 쓰기 or TokenAuthenticationFilter를 거칠 때 userId, email 등 정보를 setAttribute하고 ArgumentResolver를 구현해서 getAttribute로 꺼내 쓰기..)

// throw new BaseHandler(HttpStatus.BAD_REQUEST, "프로젝트는 종료되었습니다.");
// } 프로젝트 initData에 EndDate 설정이 안되어있어서 지금 테스트하면 오류걸림 그래서 주석처리 해놓음ㅇㅇ

// 팀장인지 확인(팀원인지 굳이 한번 더 확인하지 않음. 팀장인지만 검증.)

Choose a reason for hiding this comment

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

권한 체크 로직은 메서드 로직 초반에 실행하면 좋겠습니다!

}

public TaskEntity createTask(HttpServletRequest req, @Valid Long projectId, Create create) {
public TaskDetail createTask(HttpServletRequest req, @Valid Long projectId, Create create) {

Choose a reason for hiding this comment

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

컨트롤러 레이어에서 이미 @Valid로 검증을 하고 있다면 서비스 레이어에서는 생략해도 됩니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

넵 코드 수정하겠습니다!

@yoo-jaein
Copy link

두 방식 모두 다 괜찮은 방법이고 잘 쓰는 방법입니다.

HTTPS 인증서

무료 인증서 방식도 많이 사용합니다.
다만 ssl/tls 설정을 애플리케이션 레벨에서 직접 관리해줘야 하고 인증서를 갱신해야하는 단점이 있습니다.

Lightsail 기능 / Nginx

로드밸런서 + Nginx 방식도 많이 사용합니다.
인증서 관리, 보안 측면에서 현업에서는 이 방식을 더 선호합니다.

application.yml 80번 포트 설정이 안됐던 이유는 직접 확인해봐야 알겠지만, 해당 포트 권한이 없거나 해당 포트가 이미 사용중이거나... 정도가 의심되네요.

처음 도전하셨는데 안된다고 멈추지 않고 끝까지 해결하신 모습이 멋있습니다 👍

Copy link

@yoo-jaein yoo-jaein left a comment

Choose a reason for hiding this comment

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

진행을 위해 머지하겠습니다!! :)

@yoo-jaein yoo-jaein merged commit 0b20120 into review Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants