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

카톡 친구추가 API #242

Merged
merged 11 commits into from
May 14, 2024
Merged

카톡 친구추가 API #242

merged 11 commits into from
May 14, 2024

Conversation

asp345
Copy link
Member

@asp345 asp345 commented Apr 21, 2024

GET /friends/generate-link : 친구 추가 링크를 위한 토큰 생성
POST /friends/accept-link/{requestToken} : 값을 받아서 링크로 친구 신청 수락
랜덤한 값 생성해서 redis에 userId와 같이 저장하는 방식으로 만들었고 친구추가 정상적으로 이루어지는거 확인했습니다

@asp345 asp345 requested review from PFCJeong and a team as code owners April 21, 2024 06:37
@asp345 asp345 requested review from davin111 and Hank-Choi and removed request for a team April 21, 2024 06:37
@asp345
Copy link
Member Author

asp345 commented May 2, 2024

다시 생각해보니 token으로 stateless 하게 관리하는게 나을것 같아 더 수정해보겠습니다

-> 수정 완료했습니다

Copy link
Contributor

@Hank-Choi Hank-Choi left a comment

Choose a reason for hiding this comment

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

안그래도 DB에 저장되는 게 조금 관리하기 힘들 것 같아서
변경요청 하려고 했었는데 좋네요

원래 리뷰할 당시에는 redis로 관리하는게 어떨까 생각을 했었는데요
이것도 괜찮아보이네요

혹시 jwt 로 만들었을 때 token 길이가 어느정도 되나요 너무 길어지지는 않나요?
그리고 원래 스펙이 10초 ttl 이에요?

@Hank-Choi
Copy link
Contributor

Hank-Choi commented May 2, 2024

또 branch 정리해주시면 좋을 것 같아요

git rebase --onto develop HEAD~7
git rebase --skip # 다빈커밋 무시
git push -f

해서 찬영님 커밋만 남기고 force push 한번 해주세요

Copy link
Contributor

@Hank-Choi Hank-Choi left a comment

Choose a reason for hiding this comment

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

그리고 슬랙에도 남겼지만
보안이 어느정도 필요한거에요?

일단 jwt 특성상 payload 복호화는 누구나 가능해서 user_id를 숨기려는 건 불가능할 것 같고요
만약 jwt를 쓴다면 유저 아이디 대신 닉네임,
아니라면 redis로 특정 길이의 key로 id 관리하는 것도 괜찮을 것 같아요

사실 전 개인적으로 signedUrl 도 필요없다고 생각하긴 합니다...
(닉네임 base64 인코딩해서 넘겨도 된다고 생각)

@asp345 asp345 force-pushed the feature/friend-link branch from fa8303d to 40147a9 Compare May 2, 2024 11:18
@asp345
Copy link
Member Author

asp345 commented May 2, 2024

ttl은 값 바꿔서 테스트하던걸 실수로 올렸는데 2주로 해두겠습니다

토큰 길이는 원래 190자에서 불필요한 부분 제거하니 135자 정도 나와요
링크를 더 짧게 만들어야 하면 jwt에 닉네임을 쓴다고 해도 바이트 수는 비슷해서 말씀하신 대로 redis를 쓰는 방법이 좋을 것 같아요

보안은 id를 숨기는 것보단 본인이 발급한 링크가 맞는지 검증하는 거(userId 안다고 아무나 친추 못하게)랑 일정 시간 지나면 링크 만료되는 정도가 필요할 것 같아서 jwt를 사용했습니다

@asp345 asp345 force-pushed the feature/friend-link branch from 40147a9 to 5e7552b Compare May 2, 2024 11:47
@asp345 asp345 force-pushed the feature/friend-link branch from 5e7552b to 6a7ec1c Compare May 8, 2024 15:38
@asp345
Copy link
Member Author

asp345 commented May 9, 2024

말씀하신 부분 반영해서 랜덤한 값 만들어서 redis에 이에 대한 userId를 저장하게 바꾸었습니다.

그런데 저희 쓰는 elasticache가 노드가 꺼지면 저장이 안되는 것 같은데 안정성이 괜찮을지 궁금합니다
그리고 슬랙에 올렸던 firebase 동적 링크 서비스 종료 관련한 것도 당장은 아니지만 논의해봐야 할거 같아요

Copy link
Contributor

@Hank-Choi Hank-Choi left a comment

Choose a reason for hiding this comment

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

LGTM
이대로 머지해도 될 듯 합니다.
코멘트 쓴 부분 조금 고치면 더 좋을 것 같아요

Comment on lines 171 to 172
redisTemplate.opsForValue().setAndAwait(redisKey, userId)
redisTemplate.expireAndAwait(redisKey, Duration.ofDays(14))
Copy link
Contributor

Choose a reason for hiding this comment

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

setAndAwait에 ttl 같이 넣을 수 있는 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 중복 체크하는거 하나 있으면 좋을 것 같아요
매우 낮은 확률이긴 한데 혹시 key 겹칠 수 있으니..

@asp345 asp345 merged commit 4fd31ad into develop May 14, 2024
2 checks passed
@Hank-Choi
Copy link
Contributor

"friend-link:" prefix가 여러번 나오는데요
redis 확장성, prefix 겹치는거 방지, 오타 방지
용도로 RedisPrefix enum 같은거 만들어서 관리하면 베스트일듯 해요
머지했으니 일단 나중에 생각나면 반영하겠습니다 아니면 커밋 날려주세요

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.

2 participants