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-7] Token Location Cookie Parsing 지원 #15

Merged
merged 3 commits into from
Aug 3, 2022
Merged

[feat-7] Token Location Cookie Parsing 지원 #15

merged 3 commits into from
Aug 3, 2022

Conversation

akshxl09
Copy link
Collaborator

@akshxl09 akshxl09 commented Jul 31, 2022

close #7

@akshxl09 akshxl09 added the enhancement New feature or request label Jul 31, 2022
@akshxl09 akshxl09 requested a review from iml1111 July 31, 2022 11:08
@akshxl09 akshxl09 self-assigned this Jul 31, 2022
Copy link
Owner

@iml1111 iml1111 left a comment

Choose a reason for hiding this comment

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

다 잘짜여져 있는데, 간과한 점은 리프레시 쿠키를 간과한 것 같음.

백단으로부터 프론트단이 토큰을 총 2개를 리턴받게 될텐데 각각의 쿠키에 대한 이름을 설정할 수 있어야 함.
해당 쿠키에 secure 옵션이 걸려 있으면 프론트는 해당 쿠키에 일절 접근할 수 없기 때문에 꼭 해당 설정이 필요할듯.

@@ -23,6 +23,7 @@ def ready(self):
self.access_token_expires = data.access_token_expires
self.refresh_token_expires = data.refresh_token_expires
self.token_header_name = 'Authorization'
self.token_cookie_name = data.token_cookie_name
Copy link
Owner

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.

아 secure 옵션으로 인해 쿠키조작이 불가능한거였구나


elif refresh and config.refresh_cookie_name in request.COOKIES:
return request.COOKIES[config.refresh_cookie_name], location

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refresh를 받아서 구분할 수 있도록 만들어봤읍니다. @iml1111

Copy link
Owner

Choose a reason for hiding this comment

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

지금와서 보니까 COOKIES가 get 메소드를 가지고 있다면 아래와 같이 작성해볼 수도 있을듯.

if not refresh:
    return request.COOKIES.get(config.access_cookie_name), location
elif refresh:
    return request.COOKIES.get(config.refresh_cookie_name), location

위에 headers도 똑같은 방식으로 가능할 것 같기도 하고?

@@ -23,7 +23,8 @@ def ready(self):
self.access_token_expires = data.access_token_expires
self.refresh_token_expires = data.refresh_token_expires
self.token_header_name = 'Authorization'
self.token_cookie_name = data.token_cookie_name
self.access_cookie_name = data.access_cookie_name
self.refresh_cookie_name = data.refresh_cookie_name
Copy link
Owner

Choose a reason for hiding this comment

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

access_token_cookie_name
refresh_token_cookie_name
으로 하는게 어떨까 싶음

Copy link
Owner

Choose a reason for hiding this comment

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

그 외에 관련 코드 부분도 마찬가지로 ㅇㅇ


elif refresh and config.refresh_cookie_name in request.COOKIES:
return request.COOKIES[config.refresh_cookie_name], location

Copy link
Owner

Choose a reason for hiding this comment

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

지금와서 보니까 COOKIES가 get 메소드를 가지고 있다면 아래와 같이 작성해볼 수도 있을듯.

if not refresh:
    return request.COOKIES.get(config.access_cookie_name), location
elif refresh:
    return request.COOKIES.get(config.refresh_cookie_name), location

위에 headers도 똑같은 방식으로 가능할 것 같기도 하고?

Copy link
Owner

@iml1111 iml1111 left a comment

Choose a reason for hiding this comment

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

LGTM

@iml1111 iml1111 merged commit 9d5fbb1 into dev-1.0 Aug 3, 2022
@iml1111 iml1111 deleted the feat-7 branch August 3, 2022 07:32
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.

None yet

2 participants