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

Logout 기능 개발 & Password Update 기능 개선 #33

Merged
merged 4 commits into from
Apr 19, 2023

Conversation

cho4036
Copy link
Contributor

@cho4036 cho4036 commented Apr 19, 2023

No description provided.

@cho4036 cho4036 requested a review from ktkfree April 19, 2023 00:04

_, ok = request.UserFrom(r.Context())
if !ok {
internalHttp.ErrorJSON(w, httpErrors.NewInternalServerError(fmt.Errorf("user not found")))
Copy link
Contributor

Choose a reason for hiding this comment

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

401 auth 처리가 좋지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

먼저 코드 문맥을 간단히 설명드리자면,
사용자의 요청이 잘못된 경우, a.auth.AuthenticateRequest(r) 함수 내부에서 error를 리턴하게 되며 401 응답을 하게 됩니다.
내부적인 로직에서의 context가 잘못 처리 되었을 때 request.UserFrom(r.Context())의 ok가 false가 됩니다.
따라서, 이렇게 구현하였는데 혹시 원인과 무관하게 401을 리턴하는 것이 좋을까요?

Copy link
Member

Choose a reason for hiding this comment

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

다른 코드는 읽을 줄을 몰라서 400, 401에 대한 의견만 드리면
400, 401 은 이런 용도로 쓰면 좋겠다는 생각입니다.

401: (권한 체크가 필요한데 토큰이 없거나 만료되어) 너 누군지 모르겠어.
400: (권한 체크가 필요없거나, 토큰은 유효한데) 내가 처리할 수 없는 요청이야. 요청이 잘못되었어.

Copy link
Member

Choose a reason for hiding this comment

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

다른 내용입니다.
이렇게 생성된 에러메시지는 유저에게 노출됩니다.
우리 언어체계는 한글이 기본이므로 한글로 변경하는 건 어떨까요?

나중에는 에러코드와 메시지코드가 (메시지코드를 기반으로 화면에서 한국어/영어로 변환)
들어가야 할 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cho4036 코드 이름으로 유추하기에 "token 에 user 정보가 없을때" 라고 생각했었는데, 이런일이 일어 났다는건 뭔가 token 자체가 잘못 생성된 케이스일 것 같기도 합니다. 이 코드에서는 500 도 괜찮겠습니다.
시엽님이 말씀하시는 케이스가 지금 여기의 에러코드와 연관이 있는지는 한번 검토해주세요.

@Siyeop 한글로 변경하는 작업은 일괄로 처리하도록 하겠습니다. 티켓 만들어두고 금번 스프린트에서 작업하는걸 목표로 하겠습니다.

internal/usecase/auth.go Outdated Show resolved Hide resolved
@ktkfree ktkfree merged commit 7eea50f into openinfradev:develop Apr 19, 2023
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.

3 participants