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.

Sign up for GitHub

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 1864995 - Changes to fix bug 1841823 broke the use of recovery codes for accessing an account with MFA enabled #2140

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Bug 1864995 - Changes to fix bug 1841823 broke the use of recovery codes for accessing an account with MFA enabled #2140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Bugzilla/MFA.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ sub verify_check {
foreach my $i (1 .. 10) {
my $key = "recovery.$i";
if (($self->property_get($key) // '') eq $code) {
$self->property_delete($key);
$self->property_delete($key) if !$params->{no_delete};
return;
}
}
Expand Down
8 changes: 6 additions & 2 deletions Bugzilla/WebService/User.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ sub mfa_enroll {
return $provider->enroll_api();
}

# Pre-validate the users TOTP code using the mfa_verification_token
# Pre-validate the users TOTP code using the mfa_verification_token
# obtained from the cookie set when loading the verfication form.
sub mfa_verify_totp_code {
my ($self, $params) = @_;
Expand All @@ -559,7 +559,11 @@ sub mfa_verify_totp_code {

# verify
try {
$user->mfa_provider->verify_check({code => $params->{mfa_code}});
# Pass no_delete=1 just in case user is using a recovery code
# Normally a recovery code is deleted once it is used and we
# do no want that to happen yet here.
$user->mfa_provider->verify_check(
{code => $params->{mfa_code}, no_delete => 1});
}
catch {
return {error => 1, message => 'invalid verification code'};
Expand Down
65 changes: 65 additions & 0 deletions qa/t/2_test_login_mfa.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,25 @@ $sel->type_ok('mfa-totp-enable-code', $auth->code);
$sel->click_ok('update');
$sel->title_is('User Preferences');

# Generate recovery codes to use in testing
$sel->click_ok('mfa-recovery');
$sel->type_ok('mfa-password', $config->{admin_user_passwd});
$sel->type_ok('code', $auth->code);
$sel->click_ok('update');

# Hack needed view contents of TOTP iframe and return the recovery codes
my $recovery_string = $sel->driver->execute_script('
var iframe = document.getElementById("mfa-recovery-frame").contentWindow;
var codes = iframe.document.getElementById("codes");
return codes.innerText;
');
my @recovery_codes;
foreach my $code (split /\n/, $recovery_string) {
$code =~ s/^\s+//;
$code =~ s/\s+$//;
push @recovery_codes, $code;
}
ok(scalar @recovery_codes == 10, 'Ten recovery codes generated properly');
logout($sel);

# Log back in but this time we are asked for a TOTP code
Expand Down Expand Up @@ -79,6 +98,52 @@ ok($error eq 'Invalid verification code.', 'Correct error generated for invalid
$sel->type_ok('code', $auth->code);
$sel->click_ok('//input[@value="Submit"]');
$sel->title_is('Bugzilla Main Page');
logout($sel);

# Try to use one of the recovery codes as a MFA code
$sel->open_ok('/login', undef, 'Go to the home page');
$sel->title_is('Log in to Bugzilla');
$sel->type_ok(
'Bugzilla_login',
$config->{admin_user_login},
'Enter admin login name'
);
$sel->type_ok(
'Bugzilla_password',
$config->{admin_user_passwd},
'Enter admin password'
);
$sel->click_ok('log_in', undef, 'Submit credentials');
$sel->title_is('Account Verification');
$sel->type_ok('code', $recovery_codes[0]);
$sel->click_ok('//input[@value="Submit"]');
$sel->title_is('Bugzilla Main Page');
logout($sel);

# Reusing the same recovery code a second time should fail
$sel->open_ok('/login', undef, 'Go to the home page');
$sel->title_is('Log in to Bugzilla');
$sel->type_ok(
'Bugzilla_login',
$config->{admin_user_login},
'Enter admin login name'
);
$sel->type_ok(
'Bugzilla_password',
$config->{admin_user_passwd},
'Enter admin password'
);
$sel->click_ok('log_in', undef, 'Submit credentials');
$sel->title_is('Account Verification');
$sel->type_ok('code', $recovery_codes[0]);
$sel->click_ok('//input[@value="Submit"]');
$error = $sel->get_text('verify-totp-error');
ok($error eq 'Invalid verification code.', 'Correct error generated for invalid code');

# Now enter a different valid recovery code
$sel->type_ok('code', $recovery_codes[1]);
$sel->click_ok('//input[@value="Submit"]');
$sel->title_is('Bugzilla Main Page');

# Disable TOTP for the admin account
$sel->open_ok('/userprefs.cgi?tab=mfa');
Expand Down