feat(users): add support to verify 2FA using recovery code #4737

apoorvdixit88 · 2024-05-22T19:54:33Z

Type of Change

Description

The PR adds support to verify 2FA using recovery code, in case TOTP is lost

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

Closes #4736

How did you test it?

First Sign in for user for which 2FA is already set.
Then to verify 2FA:

Use the below curl to test it

curl --location 'http://localhost:8080/user/recovery_code/verify' \
--header 'Content-Type: application/json' \
--header 'Authorization: Bearer SPT' \
--header 'Cookie: Cookie_1=value' \
--data '{
    "recovery_code": "j4Az-W7Fk"
}'

If the recovery code is correct then response will be 200 ok.
Else proper error would be thrown

{
    "error": {
        "type": "invalid_request",
        "message": "Invalid Recovery Code",
        "code": "UR_39"
    }
}

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed the submitted code
I added unit tests for my changes where possible

apoorvdixit88 · 2024-05-22T19:56:24Z

crates/router/src/routes/app.rs

+            .service(web::resource("/recovery_codes/verify").route(web::post().to(recovery_code_verify)))
            .service(
                web::resource("/recovery_codes/generate")
                    .route(web::get().to(generate_recovery_codes)),


#recovery_code or recovery_codes ?

config/config.example.toml

ThisIsMani · 2024-05-23T06:59:55Z

crates/api_models/src/user.rs

@@ -258,6 +258,11 @@ pub struct VerifyTotpRequest {
    pub totp: Option<Secret<String>>,
 }

+#[derive(Debug, serde::Deserialize, serde::Serialize)]
+pub struct VerifyAccessCodeRequest {


Rename to recovery code.

ThisIsMani · 2024-05-23T07:03:05Z

crates/router/src/routes/lock_utils.rs

@@ -215,6 +215,7 @@ impl From<Flow> for ApiIdentifier {
            | Flow::UpdateUserAccountDetails
            | Flow::TotpBegin
            | Flow::TotpVerify
+            | Flow::AccessCodeVerify


ThisIsMani · 2024-05-23T07:13:27Z

crates/router/src/utils/user/password.rs

+    Ok(recovery_codes
+        .into_iter()
+        .map(|recovery_code| is_correct_password(candidate.clone(), recovery_code))
+        .collect::<Result<Vec<_>, _>>()?
+        .into_iter()
+        .position(|x| x))


Suggested change

Ok(recovery_codes

.into_iter()

.map(|recovery_code| is_correct_password(candidate.clone(), recovery_code))

.collect::<Result<Vec<_>, _>>()?

.into_iter()

.position(|x| x))

for (index, recovery_code) in recovery_codes.enumerate() {

let is_match = is_correct_password(candidate, recovery_code)?;

if is_match {

return Ok(Some(index));

}

}

return Ok(None)

ThisIsMani · 2024-05-23T07:24:31Z

crates/router/src/core/user.rs

+    let matching_index =
+        password::get_index_for_correct_recovery_code(req.recovery_code, recovery_codes.clone())?;
+
+    if matching_index.is_none() {
+        return Err(UserErrors::InvalidRecoveryCode.into());
+    }


Suggested change

let matching_index =

password::get_index_for_correct_recovery_code(req.recovery_code, recovery_codes.clone())?;

if matching_index.is_none() {

return Err(UserErrors::InvalidRecoveryCode.into());

}

let matching_index =

password::get_index_for_correct_recovery_code(req.recovery_code, recovery_codes.clone())?

.ok_or(UserErrors::InvalidRecoveryCode.into())?;

ThisIsMani · 2024-05-23T07:24:46Z

crates/router/src/core/user.rs

+        return Err(UserErrors::TotpNotSetup.into());
+    }
+
+    let recovery_codes = user_from_db


Make this mut directly.

racnan · 2024-05-23T12:47:48Z

config/config.example.toml

@@ -351,7 +351,8 @@ email_role_arn = ""        # The amazon resource name ( arn ) of the role which
 sts_role_session_name = "" # An identifier for the assumed role session, used to uniquely identify a session.

 [user]
-password_validity_in_days = 90        # Number of days after which password should be updated
+password_validity_in_days = 90             # Number of days after which password should be updated
+two_factor_auth_expiry_in_secs = 300       # Number of seconds after which 2FA should be done again if doing update/change from inside


imo, if we've two different keys in redis for totp and recovery codes then the expiry for them should be different.

we can consider this when will be adding other 2FA methods, for now anyways value for all of them will be same

apoorvdixit88 added 4 commits May 22, 2024 19:31

feat(users): add support to verify using recovery code

8623f66

fix: config for 2fa

db64e4a

fix: rename access code to recovery code

7343626

fix: add error for invalid recovery code

27784d3

apoorvdixit88 added C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed M-api-contract-changes Metadata: This PR involves API contract changes A-users Area: Users labels May 22, 2024

apoorvdixit88 requested review from racnan and ThisIsMani May 22, 2024 19:54

apoorvdixit88 self-assigned this May 22, 2024

apoorvdixit88 requested review from a team as code owners May 22, 2024 19:54

chore: run formatter

ce598b9

apoorvdixit88 commented May 22, 2024

View reviewed changes

fix: rename get index func for recovery code

9b4bdfe

ThisIsMani reviewed May 23, 2024

View reviewed changes

apoorvdixit88 and others added 4 commits May 23, 2024 15:19

fix: resolve review comments

4b26004

fix: change error for verify access code

774b482

chore: run formatter

ed42420

fix: change is correct password params to accpet reference

acf85f4

ThisIsMani previously approved these changes May 23, 2024

View reviewed changes

apoorvdixit88 added 2 commits May 23, 2024 17:56

Merge branch 'main' into verify-access-code

cadc8f2

fix: resolve conflicts changes

c12decc

apoorvdixit88 dismissed ThisIsMani’s stale review via c12decc May 23, 2024 12:31

racnan approved these changes May 23, 2024

View reviewed changes

ThisIsMani approved these changes May 23, 2024

View reviewed changes

likhinbopanna added this pull request to the merge queue May 23, 2024

Merged via the queue into main with commit f04c6ac May 23, 2024
13 checks passed

likhinbopanna deleted the verify-access-code branch May 23, 2024 13:56

SanchithHegde removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(users): add support to verify 2FA using recovery code #4737

feat(users): add support to verify 2FA using recovery code #4737

apoorvdixit88 commented May 22, 2024 •

edited

Loading

apoorvdixit88 May 22, 2024

ThisIsMani May 23, 2024

ThisIsMani May 23, 2024

ThisIsMani May 23, 2024

ThisIsMani May 23, 2024

ThisIsMani May 23, 2024

ThisIsMani May 23, 2024

racnan May 23, 2024

apoorvdixit88 May 23, 2024

feat(users): add support to verify 2FA using recovery code #4737

feat(users): add support to verify 2FA using recovery code #4737

Conversation

apoorvdixit88 commented May 22, 2024 • edited Loading

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

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

apoorvdixit88 commented May 22, 2024 •

edited

Loading