Add signInWithNewPassword GraphQL API #291

henry0715-dev · 2024-08-29T05:38:14Z

Close #14

Changed sign-in GraphQL API logic.

Returns Err if last_signin_time of account is None.

codecov · 2024-08-29T05:46:28Z

Codecov Report

Attention: Patch coverage is 98.62543% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.87%. Comparing base (6506e95) to head (a743b94).

Files with missing lines	Patch %	Lines
src/graphql/account.rs	98.62%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   66.21%   66.87%   +0.66%     
==========================================
  Files          68       68              
  Lines       13011    13259     +248     
==========================================
+ Hits         8615     8867     +252     
+ Misses       4396     4392       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sehkone · 2024-08-29T06:03:53Z

CHANGELOG.md

@@ -68,6 +68,9 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 - Added a `language` field to the `Account`. Consequently, the `account` and
  `accountList` API responses now include this field. The `insertAccount` and
  `updateAccount` GraphQL API endpoints are also updated to support the field.
+- Added `common_sign-in_logic` to handle `sign-in` GraphQL API logic.


This is not necessary because the method is not for users.

I'll delete it.

src/graphql/account.rs

sehkone · 2024-08-29T06:18:13Z

src/graphql/account.rs

+    /// - Authenticates with the given username and password.
+    /// - If the `last_signin_time` value of the `account` is `None`, the operation will fail, and
+    /// it should be guided to call common_sign_in_logic.


Since this is an open method to users, we need to fulfill the Rust documentation according to the convention

sehkone · 2024-08-29T07:26:07Z

src/graphql/account.rs

+    /// }"#
+    /// ```
+    ///
+    /// # Returns


We usually don't mention returning values in a separate section. You can refer to the standard library to check how to write the Rust documentation.

I'll delete it.

If you deleted that part, that’s not what I meant. I just wanted to say that the return values should be mentioned according to the Rust documentation convention, like this:

/// /// Returns blah blah blah ///

The title, # Returns, is not necessary.

The sentence starts with Returns, a third-person singular verb.

I'll be careful.

sehkone · 2024-08-29T07:28:16Z

src/graphql/account.rs

@@ -420,6 +400,96 @@ impl AccountMutation {
    }
 }

+async fn common_sign_in_logic(


This method doesn't seem to behave consistently. Its behavior depends on is_update_password, which is the only differing argument between the two callers. In other words, this method is not the common part between the two callers. When we extract the common part, it should be identical across all participants.

If there were any reason to keep this, it should have at least a different name rather than "common".

What do you think of the function name below?

sign_in_logic_by_update_password

It would be better to explicitly outline the process in both methods, sign_in and sign_in_with_new_password.

I suggest the following pseudocode:

sign_in
- Check the password
- Check if there was no previous sign-in
- Check the IP address
- Check the number of parallel sessions
- Sign in

sign_in_with_new_password
- Check the password
- Check the IP address
- Check the number of parallel sessions
- Change the password
- Sign in

While revealing this skeleton in both methods, we need to extract the common parts as separate methods.

I want to point out we need to check the password first. The information about whether the user is signing in for the first time or again, whether the number of sessions exceeds the limit, or whether the IP address is valid, is so confidential that it should not be accessible to anyone who has not provided the correct password. The original code was not properly implemented in this regard.

While revealing this skeleton in both methods, we need to extract the common parts as separate methods.

The common part might involve validating the IP address and the number of sessions.

I considered splitting the function because it is responsible for so many functions, but there is a lot of overlap, so we made it into one function.
As you mentioned above, we will refactor the checking part by splitting it into functions and calling them.

sehkone · 2024-09-02T00:25:08Z

src/graphql/account.rs

@@ -641,6 +742,7 @@ fn initial_credential() -> anyhow::Result<types::Account> {
        None,
        None,
    )?;
+    initial_account.update_last_signin_time();


Could you explain why you put this?

This is the part set to satisfy the existing test case.
There is also a part where admin/admin does not affect the part you are currently using when running for the first time.
In the future, I thought of the expected behavior of creating a new account after logging in with the admin account, and requesting password change when attempting to log in with the created account.

I don't quite understand. Could you elaborate on what you answered?

This initial_credential seems to create the first admin account and immediately set its sign-in time. This makes it pointless to alter sign_in to check the last sign-in time. What do you think?

This part has been restored to its original state.

sehkone · 2024-09-02T00:26:43Z

CHANGELOG.md

@@ -68,6 +68,7 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 - Added a `language` field to the `Account`. Consequently, the `account` and
  `accountList` API responses now include this field. The `insertAccount` and
  `updateAccount` GraphQL API endpoints are also updated to support the field.
+- Added `signInWithNewPassword` GraphQL API for signing in with a new password.


This is a new API, so should be included in the section "Added".

Since the signIn has been changed, it should be documented.

I'll change it.

sehkone · 2024-09-02T02:01:55Z

CHANGELOG.md

@@ -68,6 +69,7 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 - Added a `language` field to the `Account`. Consequently, the `account` and
  `accountList` API responses now include this field. The `insertAccount` and
  `updateAccount` GraphQL API endpoints are also updated to support the field.
+- Added logic to check the `lastSigninTime` value in the `signIn` GraphQL API.


I think we need to document how the new logic works, as well as the fact that it has changed, because in this case, the new behavior is the primary concern for users.

Added content.

The key point is that this API rejects a user's access if it is their first attempt. Therefore, it's not enough to simply mention checking the lastSigninTime. Additionally, the word "logic" seems unnecessary. I suggest:

Changed the signIn to reject the first sign-in attempt unless it is done through the new signInWithNewPassword API.

In this case, "Changed" or "Updated" is preferred over "Added" because the behavior of the API has actually changed from the user's perspective. Also, the word "reject" or any similar one should be included to convey the key point.

I will delete the explanation below and change it to the sentence you suggested.
"Apply a requirement for a password change upon the initial signIn."

sehkone · 2024-09-02T04:34:16Z

When reseting the admin's password, we need to reset the last_signin_time as well.

sehkone · 2024-09-03T12:10:39Z

src/graphql/account.rs

+    /// ```ignore
+    /// r#"mutation {
+    ///     signInWithNewPassword(username: "user", password: "pw", newPassword: "pw2") {
+    ///         token
+    ///     }
+    /// }"#
+    /// ```


We don't need to present the signatures of GraphQL APIs in the code because users can not only infer them on their own but also check them in the GraphQL playground.

I'll delete it.

sehkone · 2024-09-03T12:10:54Z

src/graphql/account.rs

+    /// If the `signIn` GraphQL API response is the error message "a password change is
+    /// required to proceed", you can call the `signInWithNewPassword` GraphQL API.


I don't think this part is necessary because it seems to repeat what was mentioned right above.

The part I think is unnecessary:

/// If the `signIn` GraphQL API response is the error message "a password change is /// required to proceed", you can call the `signInWithNewPassword` GraphQL API.

I'll delete it.

sehkone · 2024-09-03T12:19:59Z

src/graphql/account.rs

+    ///
+    /// # Errors
+    ///
+    /// An error message (`String`) if any of the checks fail or an error occurs during the process.


I suggest:

/// # Errors /// /// Returns `Err` if the password is invalid, this is the first sign-in attempt, the access doesn't originate from a permitted IP address, or the number of sessions exceeds the maximum limit.

The style where the sentence starts with a third-person singular verb is a convention in Rust documentation. Additionally, it would be better to specify exactly the cases where errors occur.

I'll change it.

sophie-cluml · 2024-09-03T12:26:56Z

@sehkone I know we are still in pr-review stage for this PR, but I think merging PR would require implement new UX flow and design in the client side. So I would like to suggest in advance, that we might merge this PR after we release the upcoming review-web version.

sehkone · 2024-09-03T12:38:46Z

@sehkone I know we are still in pr-review stage for this PR, but I think merging PR would require implement new UX flow and design in the client side. So I would like to suggest in advance, that we might merge this PR after we release the upcoming review-web version.

Yes. I won't merge this until what is required is done.

src/graphql/account.rs

Close #14 Changed `sign-in` GraphQL API logic. - Returns `Err` if `last_signin_time` of `account` is `None`.

sehkone reviewed Aug 29, 2024

View reviewed changes

src/graphql/account.rs Outdated Show resolved Hide resolved

sehkone reviewed Aug 29, 2024

View reviewed changes

henry0715-dev force-pushed the henry/sign-in-with-new-password branch 2 times, most recently from ed652fa to 97ffb90 Compare August 29, 2024 07:08

sehkone reviewed Aug 29, 2024

View reviewed changes

henry0715-dev force-pushed the henry/sign-in-with-new-password branch 3 times, most recently from a5c9935 to 5548e55 Compare August 30, 2024 05:03

henry0715-dev requested a review from sehkone September 1, 2024 23:58

sehkone reviewed Sep 2, 2024

View reviewed changes

henry0715-dev force-pushed the henry/sign-in-with-new-password branch 2 times, most recently from e72ae31 to 6ebe11b Compare September 2, 2024 01:39

sehkone reviewed Sep 2, 2024

View reviewed changes

henry0715-dev force-pushed the henry/sign-in-with-new-password branch 2 times, most recently from e4320fd to acbb168 Compare September 2, 2024 02:41

henry0715-dev force-pushed the henry/sign-in-with-new-password branch 2 times, most recently from fb6cc77 to 17d7dc0 Compare September 3, 2024 06:18

sehkone reviewed Sep 3, 2024

View reviewed changes

henry0715-dev force-pushed the henry/sign-in-with-new-password branch from 17d7dc0 to 8912b70 Compare September 4, 2024 00:05

sehkone reviewed Sep 4, 2024

View reviewed changes

src/graphql/account.rs Show resolved Hide resolved

Add signInWithNewPassword GraphQL API

a743b94

Close #14 Changed `sign-in` GraphQL API logic. - Returns `Err` if `last_signin_time` of `account` is `None`.

henry0715-dev force-pushed the henry/sign-in-with-new-password branch from 8912b70 to a743b94 Compare September 4, 2024 05:01

		/// If the `signIn` GraphQL API response is the error message "a password change is
		/// required to proceed", you can call the `signInWithNewPassword` GraphQL API.

Add signInWithNewPassword GraphQL API #291

Are you sure you want to change the base?

Add signInWithNewPassword GraphQL API #291

Conversation

henry0715-dev commented Aug 29, 2024

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehkone Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehkone commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophie-cluml commented Sep 3, 2024

sehkone commented Sep 3, 2024

codecov bot commented Aug 29, 2024 •

edited

Loading

sehkone Aug 29, 2024 •

edited

Loading