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

[BE] websocket configuration #977

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

Conversation

SproutMJ
Copy link
Collaborator

[BE] PR websocket configuration

이슈번호

해당되는 모든 번호를 태그해주세요
#975

PR 내용

웹소켓 handshake 이후 stomp CONNECT 과정 까지의 설정입니다.

참고자료

의논할 거리

  1. 생각해보니 에러관련된 이야기를 해본적이 없긴 한데 웹소켓 에러처리는 REST API랑 다르게 가져가야되나 고민입니다. 일단은 return null;을 해서 요청을 무시시킵니다.
  2. JwtTokenExtractor는 HttpServletRequest로 받고 있어서 호환이 되지 않고, JwtTokenProvider는 검증용으로 사용하는 중이긴 한데 적절히 책임분리를 추가로 진행할 필요가 있어보입니다. 근데 지금 태스크가 아니라 일단 넘어갔습니다.

Copy link
Collaborator

@pilyang pilyang left a comment

Choose a reason for hiding this comment

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

  1. 에러핸들링
    아 그러네요... 에러 헨들링쪽은 그때 이야기를 못했었네요.
    이거는 한번 더 고민해보고 해야할것같네여

  2. JWT 검증
    이건 맞아요, 저도 이거 여기에있는거 맘에 안들었었는데,,
    나중에 여유나면이나 아니면 다른작업들 좀 순서땜시 붕 뜰때 한번 처리해주면 좋을것같슴다


추가로 하나 확인필요할것같은거 코멘트 남겼어요

+) websocket연결 유지되는거 우리 nginx reversproxy로 쓰고있는데 이거 추가적으로 설정 들어가야할것들 있는지 확인해봐야것네..

Comment on lines 20 to 22
registry.addEndpoint("/ws/chat")
.setAllowedOrigins("*")
.withSockJS();
Copy link
Collaborator

Choose a reason for hiding this comment

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

CORS 설정은 왜 있는것일까용...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저희가 CORS를 스프링에서 담당하지 않고 엔진엑스에서만하는걸까요? 혹시 몰라 열어둔건데 스프링에서 담당하지 않고 있으면 닫겠습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

넹ㅇ 브라우저랑 연결되는건 nginx라서 닫으면 될것같아용

Copy link
Collaborator

@pilyang pilyang left a comment

Choose a reason for hiding this comment

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

좋아용~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants