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: 공유보드 조회, 공유 보관함 사진 상세조회 , 공유 URL 생성 API #60

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

gwanhyeon
Copy link
Collaborator

@gwanhyeon gwanhyeon commented May 29, 2021

resolve #48

추가 기능

  • 공유 보드 조회 API
  • 공유 보관함 사진 상세조회 API
  • 공유 URL 생성 API

기능 추가시에 발생한 이슈는 HttpServletRequest에서 제공하는 port는 HTTP 80, HTTPS 443을 가져오는데 Tomcat 8080포트를 가져오지 못하는 이슈가 발생하였습니다. 이에 따라, application-security.yml상에 server.port = 80으로 처리하여 유동적인 변화를 꾀할지 아니면 localhost일 경우에만 8080포트만 처리할지에 대한 이슈가 있어서 일단은 로컬호스트 일경우 HttpservletReqeust에서 8080을 세팅시켜놓도록 하였습니다.

공유 암호화

공유시 암호화 AES256 관련

EncrypUtil

  1. encryptAES decryptAES 암호화 복호화 로직 추가

공유 암호화시 트러블 슈팅 과정

암호화 생성시 /가 임의적으로 생성되어서 현재 저희의 path에 추가되어 HTTP Request시 올바른 URL Request가 되지 않는 이슈가 발생하였습니다. case by case로 해당되는 암호화키가 "/"가 포함되는 경우, 포함되지 않는 경우가 매번 다르기 때문에 테스트 상에서 case마다 성공에 대한 응답이 다르게 동작하였습니다

moodof.net/api/public/boards/{key}/detail/{1}
공유 상세요청을 진행한다고 가정을 하면 key값이 start index: \, /, 의 암호화패딩이 적용되는 경우
moodof.net/api/public/boards//#$%^&*/detail/{1} 와 같이 Spring Security 정책상 URL에는 "//"가 허용되지 않기때문에 올바른 요청이 되지 않게 되었습니다

  1. issue
    더블슬래쉬로 인하여 Spring Security URL 관련 Exception이 발생하였습니다. 해당부분은 정규표현식으로
    [^0-9a-zA-Z]으로 처리하여 특수문자기호에 대해서 제거를 진행하였지만, AES256 암호화 과정에서 반드시 필요로 하는 패딩부분까지 모두 replace되는 이슈로 특수문자에 대해서 '%'으로 변경후 암호화를 생성시키고 해독시에도 해당 '%'로 변경하였습니다. 일단 임시방편으로 테스트 수행을 동작시켜놓기 위해서 해당 로직처럼 진행하였는데, 암호화 부분에 있어서 더 좋은 의견있으시면 반영하겠습니다. 테스트시에 이러한것들이 테스트상에서 제대로 Exception로그가 잡히지 않아, 비즈니스로직상에서 찾다보니 시간이 좀 지체되었습니다
Spring 5.0.3 RequestRejectedException: The request was rejected because the URL was not normalized

https://isntyet.github.io/java/RequestRejectedException-%ED%95%B8%EB%93%A4%EB%A7%81/

org.springframework.security.web.firewall.RequestRejectedException: The request was rejected because the URL contained a potentially malicious String "//"

더블슬래시이슈

  1. issue
    위에서 발생한 암호화과정에서 생긴 패딩부분에 대한 이슈입니다. 복호화 키가 맞지 않지 않아서 생기는 이슈로 컬럼사이즈가 작아서 암호화된 데이터가 온전히 저장되지 않고 짤려서 저장되어 복호화 할수없는 문제였기때문에 패딩일치여부를 확인하여 변경을 진행하였습니다.
javax.crypto.IllegalBlockSizeException: Input length must be multiple of 16 when decrypting with padded cipher
  1. issue
    정규표현식 [^0-9a-zA-Z]
java.lang.IllegalArgumentException: Last unit does not have enough valid bits

bit관련된 이슈

gwanhyeon and others added 18 commits April 16, 2021 14:18
@gwanhyeon gwanhyeon added the enhancement New feature or request label May 29, 2021
@gwanhyeon gwanhyeon requested a review from kouz95 May 29, 2021 03:51
@gwanhyeon gwanhyeon self-assigned this May 29, 2021
Copy link
Collaborator

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어져서 죄송합니다 😥
일이 갑자기 생겨서 정신이 없네요 ㅜㅜ..

제가 이해하고 있던 내용과 구현해 주신 내용이 달라서, 제가 이해하고 있던 방식에 대해서 말씀 드리려고 합니다.

구현해주신 내용은 "생성 되어있던 보드가 공유 보드가 된다" 라는 개념으로 보여지는데, 제가 이해하고 있던 방식은 "보드가 생성 될 때 부터, 고유값이 생성되어 저장된다" 였습니다.

서버에서 보드에 대한 정보를 클라이언트에게 전달할때 고유값을 같이 응답하면 (원래 있던 보드 생성 API의 BoardResponse에 추가가 되는식)
보드를 만든 사람이 공유하기 버튼을 눌렀을 때, 클라이언트에서 이미 고유값을 알고 있으니 고유값을 이용하여 공유 주소를 만들 수 있다고 생각했습니다. (지금의 방식과 비교하면, 공유 보드로 만드는 API를 호출할 필요가 없습니다.)

또한, 구현 내용 중 "공유 주소" 라는 개념이 있는데, 주소의 필요성은 없다고 생각합니다.
RequestParam으로 고유값을 받는 조회 API만 만들면 동작하지 않을까요?

그리고 AES를 사용하셨는데 단순히 보드의 고유값을 만들어내는 과정만 필요하다고 생각해서 암호화 알고리즘을 사용할 이유는 없다고 생각합니다. (고유값에 담겨야될 내용이 있나요?)
간단하게 해쉬(SHA) 사용하면 괜찮을 것 같습니다.

마지막으로 테스트가 실패하는데 확인한번 해주시면 감사하겠습니다 🙇‍♂️
Screen Shot 2021-05-31 at 5 19 39 AM

@gwanhyeon
Copy link
Collaborator Author

gwanhyeon commented May 31, 2021

제가 생각한 구현내용은

  1. 공유하기를 눌렀을시에 새로운 공유하기 URI가 생성되어 해당 컬럼에 저장된다.(AES256)
  2. 저장된 SharedKey를 요청하면 해당 공유 URI를 제공한다.(단 고유의 값으로 제공되어야한다)
  3. SharedKey에 따라 해당 보드에 대한 내용을 조회한다.
    였습니다. 굳이 공유하기에 있어서 고유의 값으로 제공될 필요는 없어보이네요!

따라서, 코드리뷰 주신 사항의 수정사항은 다음과 같습니다.

  1. AES256 암호화 과정을 SHA256 해시키값 생성으로 변경하였습니다. (SHA256해시키값이 MD5보다 보안성이 뛰어나다고 합니다)
  2. 보드생성시 해당되는 SHA256 해시키값이 동시에 생성됩니다.
  3. 보드의 SharedKey @Pathvariable값으로 해당 보드의 공유 URI를 제공 받는다.
  4. 불필요한 Utils 파일, Custom Annotation 사항 제거, 클래스 명명 변경
  5. REST API
  • 공유 보드 조회 API GET /api/public/boards/{sharedKey}
    sharedKey: 공유 hash key
  • 공유 보드 상세조회 GET /api/public/boards/{sharedKey}/detail/{id}
    sharedKey: 공유 hash key, id: 보드 ID
  • 공유 URI 생성 POST /api/boards
    Requestbody BoardDTO.CreateBoard, 보드생성시에 공유 URI가 동시에 저장됩니다.
  • 공유 URI 조회(인증 JWT키값 유효해야함) GET /api/boards/{id}
    id: 보드ID

그리고 의논나누고 싶은 부분이 있다면 보드가 생성후에 공유 URI를 생성하여 저장하는데 이 부분을 @transaction propagation 옵션을 적용하려 하였습니다. 기본의 Default의 REQUIERD를 변경하여 URI save 로직에서 MANDATORY 옵션을 처리하려고 하였는데, 어떻게 생각하시나요? 이렇게 트랜잭션내에서 Exception을 잡아주면 보드가 생성되고 난후 -> 보드 URI를 생성하는 트랜잭션을 분리하여 처리하는 방법은 전파옵션으로 조금 더 명확하게 트랜잭션이 처리되지 않을까해서요..!!

  • REQUIRED와 비슷하게 이미 시작된 트랜잭션이 있으면 참여한다.
  • 반면에 트랜잭션이 시작된 것이 없으면 새로 시작하는 대신 예외를 발생시킨다.
  • 혼자서는 독립적으로 트랜잭션을 진행하면 안 되는 경우에 사용한다
    와 같은 특성을 가지고 있습니다.
    시작된 트랙잭션 보드 생성 -> 보드 생성된 트랙잭션이 있으면 -> URI 트랜잭션을 시작하여 처리한다. 의 로직은 어떨까 싶어서요!!
  1. 통합 테스트 TDD 확인

스크린샷 2021-05-31 오후 4 08 05

@gwanhyeon gwanhyeon requested a review from kouz95 May 31, 2021 06:58
@gwanhyeon
Copy link
Collaborator Author

  • 공유 URI 응답 DTO 변경(BoardId, SharedKey)
  • 테스트 미사용코드 제거

Copy link
Collaborator

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어 죄송합니다 🙇‍♂️

전체 테스트 동작시 보드 생성 부분에서의 테스트가 전부 실패하는데, save 두번 호출하는 부분을 해결해야할 것 같고, develop에서 제대로 pull이 안되서 테스트가 실패하는 것 같습니다.
확인 한번 부탁드리겠습니다 🙏

private final BoardSequenceUpdater boardSequenceUpdater;

public static final String LOCALHOST = "localhost";


@Transactional
public BoardDTO.BoardResponse create(Long userId, BoardDTO.CreateBoard request) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

create 동작중, repository save 호출이 2번 일어나고 있는데, 한번으로 줄일 수 있을것 같습니다 🙂

Copy link
Collaborator Author

@gwanhyeon gwanhyeon Jun 13, 2021

Choose a reason for hiding this comment

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

더티체킹을 생각못하고 두개의 save를 사용하여서 바로 수정하였습니다 :)

현재 로직은

  1. board id가 생성 및 저장을 한다.(saved)
  2. 생성된 id로 sharedkey를 생성한 후 공유키를 저장한다.(saved)
    에서 Dirty Checking이 되는것을 간과했네요!

변경 로직

  1. board id가 생성 및 저장을 한다(saved)
  2. 더티체킹으로 sharedKey 업데이트

로직으로 하나의 save로 처리하였습니다!

@@ -45,4 +44,9 @@
boardService.delete(userId, id);
return ResponseEntity.noContent().build();
}
@GetMapping("/{id}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 GET /boards/{id} 가 공유키 조회로만 사용되고 있는데, 저 주소는 보드 상세 조회라고 생각할 수 있을것 같아요. 보드 정보가 있어야할 것 같고 (title 등), 해당 보드에 존재하는 BoardPhoto 리스트를 응답해야할 것 같은데 어떻게 생각하시나요?

Copy link
Collaborator Author

@gwanhyeon gwanhyeon Jun 13, 2021

Choose a reason for hiding this comment

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

현재 말씀해주신부분은 사용자가 공유 URI를 누르면 생성된 sharedKey값을 가져와서 해당 sharedKey 데이터를 반환해주는 로직입니다. @GetMapping("/sharedKey/{id}") 로 변경하여 해당 sharedKey를 조회하는 로직은 어떨까요?


@GetMapping("/{sharedKey}") 이 부분이 (지금은 변경된 `@GetMapping /api/public/boards?sharedKey={sharedKey}) 보드에 대한 전체 포토리스트를 가져오는 부분입니다. 밑에 말씀해주신데로 기획 정책상 어떤정보를 보여줄 것인지에 따라 변경될 소지가 있어보입니다.

private final BoardPhotoService boardPhotoService;

@Override
@GetMapping("/{sharedKey}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

/api/public/boards/{sharedKey} 도 괜찮지만, /api/public/boards?sharedKey={sharedKey}도 생각해 볼 수도 있겠네요.
PathVariable은 id의 경우 사용하는 편이라 이외의 프로퍼티로 조회를 할떈 RequestParam을 사용하곤 했었습니다 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀들어보니 Pathvariable은 id로 처리하고 sharedKey로 검색을 한다는것과 같은 의미이므로 RequestParam도 괜찮아 보이네요 :)

@Override
@GetMapping("/{sharedKey}")
public ResponseEntity<List<BoardPhotoDTO.BoardPhotoResponse>> findAllByBoard(@PathVariable String sharedKey) {
List<BoardPhotoDTO.BoardPhotoResponse> responses = boardPhotoService.findAllBySharedKey(sharedKey);
Copy link
Collaborator

Choose a reason for hiding this comment

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

BoardPhoto List 뿐만아니라, Board 정보도 필요하지 않을까요?
아직 공유 받은 사람이 페이지에 접속했을때 어떤 정보들이 필요한지 논의가 안되었기 때문에, 기획이 나오면 바뀌어야할 것 같아요.
예상하기론 보드 정보와, 보드 제작자 (User) 정보, 혹은 카테고리 정보까지 필요할지도 모르겠네요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 맞습니다. 그부분은 아직 기획상 명확하지 않아서 일단 포토리스트들만 가져오는로직으로 개발을 진행하였습니다 :) 기획에 따라 보드 정보, 보드 포트정보, 카테고리 정보 가 필요해보입니다!

}

@Override
@GetMapping("/{sharedKey}/detail/{id}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 API는 어떤 상황에서 사용될 것이라 생각하신 건가요?
공유 보드에서 공유 받은 사람이 사진 상세 조회를 한 경우인가요?

Copy link
Collaborator Author

@gwanhyeon gwanhyeon Jun 13, 2021

Choose a reason for hiding this comment

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

네 맞습니다. 공유보드에서 사진 상세조회를 한 경우입니다.

sharedKey를 같이 넘겨줄 필요는 없어보이기는 하는데.. 어떻게 생각하실까요? 진입시에만 sharedKey로 확인하고 그외 나머지 동작들은 sharedKey가 필요없다고 생각합니다.

위에서 말씀해주신것들을 들어보니 @GetMapping("/{id}") 가 적합해보이는데 어떻게 생각하시나요!


import java.time.LocalDateTime;

public class SharedDTO {
Copy link
Collaborator

Choose a reason for hiding this comment

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

사용 되지 않는 클래스인것 같네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

다른 feature에서 삭제된 내용이였는데 적용이 덜 되었었네요!

@gwanhyeon gwanhyeon requested a review from kouz95 June 13, 2021 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 공유
2 participants