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

첫 로그인 시 계정 비밀번호 변경을 위한 새로운 GraphQL mutation 구현 #14

Closed
msk opened this issue May 9, 2023 · 4 comments · Fixed by #291
Closed
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@msk
Copy link
Contributor

msk commented May 9, 2023

이슈 설명:

계정 보안을 강화하는 일환으로, 모든 계정이 처음으로 로그인 시 비밀번호를 변경하도록 할 필요가 있습니다. 이렇게 하면 디폴트 비밀번호가 첫 로그인 즉시 교체되므로 남용될 위험이 최소화됩니다.

이 요구사항을 구현하기 위해, 새로운 GraphQL mutation인 sign_in_with_new_password(username: !String, oldPassword: !String, newPassword: !String)를 추가합니다. 이 mutation은 사용자의 usernameoldPassword를 사용하여 사용자를 인증하고, 데이터베이스에 저장된 Accountpassword 필드 값을 newPassword로 바꾸고 last_signin_time 필드를 갱신해야 합니다. oldPasswordnewPassword는 서로 달라야 합니다.

또한, 계정이 한 번도 로그인한 적이 없는 경우 로그인 시도를 받아들이지 않도록 기존의 signIn mutation을 수정해야 합니다.

구현 전:

signIn mutation은 사용자가 이전에 로그인한 적이 없더라도 로그인 시도를 받아들입니다.

구현 후:

  1. 첫 사용자가 로그인을 시도합니다.
  2. signIn mutation은 last_signin_timeNone인 경우 로그인 시도를 거부합니다.
  3. 사용자는 sign_in_with_new_password를 사용하여 로그인하고 동시에 비밀번호를 변경합니다.
  4. sign_in_with_new_password mutation은 사용자를 인증하고, passwordlast_signin_time을 업데이트하고, AuthPayload를 리턴합니다.

요청인: @sehkone

@msk msk added enhancement New feature or request good first issue Good for newcomers labels May 9, 2023
@msk msk changed the title Implement New GraphQL Mutation for First-Time Account Sign-In with Password Change 첫 로그인 시 계정 비밀번호 변경을 위한 새로운 GraphQL mutation 구현 May 9, 2023
@henry0715-dev henry0715-dev self-assigned this Aug 28, 2024
henry0715-dev added a commit that referenced this issue Aug 29, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Aug 29, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Aug 29, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Aug 30, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Aug 30, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Aug 30, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 2, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 2, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 2, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 2, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 2, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 3, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 4, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Sep 4, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
@msk
Copy link
Contributor Author

msk commented Oct 1, 2024

@sehkone in #291 (comment):

since requiring users to change their password at the first sign-in is still inconvenient, I think it would be better to make this optional based on a command line option or another approach. Could you suggest your idea on this?

I'd like to clarify that requiring users to change their password at the first sign-in was actually one of the original requirements you proposed. The wording was: "[...] forces the new user to change its password when signing in for the first time." Has this requirement changed?

Regarding your suggestion to make this optional, I have some concerns. While I understand that it may be inconvenient for users to change their password at the first sign-in, I don't think that's a strong enough argument to compromise on security. Security measures often come with a trade-off in convenience, but that doesn't mean we should make them optional.

Providing an option to choose between a secure and insecure practice can lead to users choosing the insecure option, which can have serious consequences. If a customer's system is compromised due to an unchanged initial password, it could damage ClumL's reputation and publicity.

I think it's better to stick with the original requirement and ensure that users change their password at the first sign-in.

@sehkone
Copy link
Contributor

sehkone commented Oct 1, 2024

@sehkone in #291 (comment):

since requiring users to change their password at the first sign-in is still inconvenient, I think it would be better to make this optional based on a command line option or another approach. Could you suggest your idea on this?

I'd like to clarify that requiring users to change their password at the first sign-in was actually one of the original requirements you proposed. The wording was: "[...] forces the new user to change its password when signing in for the first time." Has this requirement changed?

Regarding your suggestion to make this optional, I have some concerns. While I understand that it may be inconvenient for users to change their password at the first sign-in, I don't think that's a strong enough argument to compromise on security. Security measures often come with a trade-off in convenience, but that doesn't mean we should make them optional.

Providing an option to choose between a secure and insecure practice can lead to users choosing the insecure option, which can have serious consequences. If a customer's system is compromised due to an unchanged initial password, it could damage ClumL's reputation and publicity.

I think it's better to stick with the original requirement and ensure that users change their password at the first sign-in.

I apologize for the confusion in my argument. What I meant by "inconvenient" refers not to the general users but to our developers and testers. As for the users, as you mentioned, we need to stick to the requirements. I just hope that our internal team can save time on testing.

@msk
Copy link
Contributor Author

msk commented Oct 2, 2024

Thank you for the clarification, and I understand the need to streamline the process for developers and testers.

Rather than introducing a command-line option that might compromise security in the shipped binary, I propose creating a separate binary target within the review-web crate specifically for internal use. This binary would modify the database to set up test accounts and other necessary configurations. This internal tool will not be part of the shipped product, keeping security intact for our end-users. Does this approach sound reasonable to you?

@sehkone
Copy link
Contributor

sehkone commented Oct 3, 2024

Thank you for the clarification, and I understand the need to streamline the process for developers and testers.

Rather than introducing a command-line option that might compromise security in the shipped binary, I propose creating a separate binary target within the review-web crate specifically for internal use. This binary would modify the database to set up test accounts and other necessary configurations. This internal tool will not be part of the shipped product, keeping security intact for our end-users. Does this approach sound reasonable to you?

I agree with you on the separate binary target. If we proceed with that, I hope it will be incorporated into the central manager rather than the review-web.

On the other hand, while I am reconsidering, it occurs to me that this feature—enforcing a password change—likely won't be troublesome enough to bother our team significantly. The change only needs to happen once when the manager server is fully initiated, which doesn’t seem to take place very often. Therefore, I think it would be better to go ahead with this feature and see how it works out.

Thank you for your advice.

henry0715-dev added a commit that referenced this issue Oct 21, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Oct 30, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Oct 31, 2024
Close #14

Changed `sign-in` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Nov 1, 2024
Close #14

Changed `signIn` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Nov 7, 2024
Close #14

Changed `signIn` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Nov 13, 2024
Close #14

Changed `signIn` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
henry0715-dev added a commit that referenced this issue Nov 13, 2024
Close #14

Changed `signIn` GraphQL API logic.
- Returns `Err` if `last_signin_time` of `account` is `None`.
sehkone pushed a commit that referenced this issue Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants