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

#bug fix UpdateUser API, #Improvement TemporaryPWGeneration, API Filter #47

Merged
merged 4 commits into from
May 4, 2023

Conversation

cho4036
Copy link
Contributor

@cho4036 cho4036 commented May 3, 2023

Bug fix:

  • 내용: UpdateUserAPI 를 통해 user의 role을 변경이 Keycloak에 정상적으로 반영되지 않음
  • 원인: keycloak의 UpdateUser API만으로는 의도하는 동작 불가능
  • 변경사항: JoinGroup&LeaveGroup 방식으로 구현방식 수정

Improve:

  • 임시 비밀번호 생성 로직 개선
    • 임시 비밀번호 생성에 있어서 동일한 PW 발급되는 현상 리포트
    • 임시 비밀번호 생성 로직에서 "math/rand"를 사용하는 것 보다 "crypto/rand"를 사용하는 것이 더욱 secure하다는 권고 확인
    • "crypto/rand"를 사용하여 생성하도록 로직 수정
  • PW가 만료된 사용자에게 logout API 접근 허용 추가
    • PasswordFilter 로직에 허용 로직 추가

@cho4036 cho4036 changed the title bug fix: fix UpdateUserAPI not changing role in keycloak #bug fix UpdateUser API, #Improvement TemporaryPWGeneration, API Filter May 4, 2023
Comment on lines +23 to +36
func TestPasswordGeneration(t *testing.T) {
length := 8
sameCount := 0
for i := 0; i < 10000000; i++ {
firstGenPassword := helper.GenerateRandomString(length)
secondGenPassword := helper.GenerateRandomString(length)
if firstGenPassword == secondGenPassword {
fmt.Printf("Index: %d. It's same %s\n", i, firstGenPassword)
sameCount++
}
}
fmt.Println("Same count: ", sameCount)
fmt.Println("Same ratio: ", float64(sameCount)/100000)
}
Copy link
Member

Choose a reason for hiding this comment

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

assert 구문은 따로 없나요?

0.0001% 미만이면 성공이라든가 그런게 필요할 것 같네요.
10000000 정도면 충분히 큰 수라서 우연히 0.0001% 이상이 나온다던가 그런 일은 없을 것 같습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if 비교 구문 대신 assert를 사용하면 되나 편의상 간단히 보기 위해 이렇게 처리했어요

go test 정식 사용법은 assert()를 사용함이 맞습니다

internal.API_PREFIX + internal.API_VERSION + "/organizations/" + requestUserInfo.GetOrganizationId() + "/my-profile" + "/password",
internal.API_PREFIX + internal.API_VERSION + "/organizations/" + requestUserInfo.GetOrganizationId() + "/my-profile" + "/next-password-change",
internal.API_PREFIX + internal.API_VERSION + "/auth/logout",
Copy link
Member

Choose a reason for hiding this comment

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

반영 감사합니다 👍

@@ -1,15 +1,11 @@
package helper

import (
"crypto/rand"
Copy link
Member

Choose a reason for hiding this comment

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

변경하기 전에는 실제로 동일한 PW 가 나오는 케이스가 발생하고 있었나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

천만과 1억도 돌려봤는데 안나왔어요. 어제 말씀해주신 케이스는 정말 신기하네요..
suffle도 넣을까 하였는데 효과 확신도 안들어 넣지 않았어요. crypto package로 변환해보는게 적절할 꺼같아 이번 건은 이렇게 처리했습니다

Copy link
Member

@Siyeop Siyeop left a comment

Choose a reason for hiding this comment

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

image

@ktkfree ktkfree merged commit eeb2090 into openinfradev:develop May 4, 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.

4 participants