-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(users): Create terminate 2fa API #4731
Conversation
crates/router/src/core/user.rs
Outdated
if !(check_totp_in_redis(&state, &user_token.user_id).await? | ||
|| check_access_code_in_redis(&state, &user_token.user_id).await?) | ||
{ | ||
return Err(UserErrors::TotpRequired.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new error 2FARequired
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New error added
crates/router/src/core/user.rs
Outdated
.change_context(UserErrors::InternalServerError)? | ||
.into(); | ||
|
||
if skip_2fa.is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be !skip_2fa.unwrap_or(false)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
crates/router/src/core/user.rs
Outdated
if !(check_totp_in_redis(&state, &user_token.user_id).await? | ||
|| check_access_code_in_redis(&state, &user_token.user_id).await?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !(check_totp_in_redis(&state, &user_token.user_id).await? | |
|| check_access_code_in_redis(&state, &user_token.user_id).await?) | |
if !check_totp_in_redis(&state, &user_token.user_id).await? | |
&& !check_access_code_in_redis(&state, &user_token.user_id).await? |
This is more readable imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
crates/router/src/core/user.rs
Outdated
pub async fn terminate_two_factor_auth( | ||
state: AppState, | ||
user_token: auth::UserFromSinglePurposeToken, | ||
skip_two_factor_auth: Option<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the parent route function to resolve the option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
36a14b4
Type of Change
Description
Currently completion of 2fa and setting the status of the 2fa status as "SET" is handled by
verify_totp
API. We want to remove this fromverify_totp
, so this PR creates a new API to terminate the 2fa flow.Additional Changes
Motivation and Context
This will close the issue #4730
How did you test it?
This can only be tested locally as it requires some changes in the redis.
a. If the keys with
TOTP_{user_id}
orRC_{user_id}
is not present in redisRequest
Response
b. Add the keys to the redis with prefix as
TOTP_{user_id}
orRC_{user_id}
Request
Response
This will also set the totp_status for the user as "set"
a. skip_two_factor_auth=true
Irrespective if the keys are present in redis or not if skip_two_factor_auth is sent as true it will not change the status and will give the token and token_type for the next flow
Request
Response
b. skip_two_factor_auth=false
Request
Response
This will also set the totp_status for the user as "set"
Checklist
cargo +nightly fmt --all
cargo clippy