From 3db657e09abf354416cad4d42030167c0b8585f6 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Tue, 15 Sep 2015 00:18:27 +0200 Subject: Bug 1185241: Logging out when or after impersonating a user doesn't delete cookies from the logincookies table correctly r=dkl --- Bugzilla/Auth/Persist/Cookie.pm | 2 +- relogin.cgi | 69 +++++++++----------------------- template/en/default/admin/sudo.html.tmpl | 17 +++----- 3 files changed, 26 insertions(+), 62 deletions(-) diff --git a/Bugzilla/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm index 877d1907e..904de3699 100644 --- a/Bugzilla/Auth/Persist/Cookie.pm +++ b/Bugzilla/Auth/Persist/Cookie.pm @@ -94,7 +94,7 @@ sub logout { my $cgi = Bugzilla->cgi; my $input = Bugzilla->input_params; $param = {} unless $param; - my $user = $param->{user} || Bugzilla->user; + my $user = $param->{user} || Bugzilla->sudoer || Bugzilla->user; my $type = $param->{type} || LOGOUT_ALL; if ($type == LOGOUT_ALL) { diff --git a/relogin.cgi b/relogin.cgi index c4aae8d0b..286127495 100755 --- a/relogin.cgi +++ b/relogin.cgi @@ -54,22 +54,6 @@ elsif ($action eq 'prepare-sudo') { # Keep a temporary record of the user visiting this page $vars->{'token'} = issue_session_token('sudo_prepared'); - if ($user->authorizer->can_login) { - my $value = generate_random_password(); - my %args; - $args{'-secure'} = 1 if Bugzilla->params->{ssl_redirect}; - - $cgi->send_cookie(-name => 'Bugzilla_login_request_cookie', - -value => $value, - -httponly => 1, - %args); - - # The user ID must not be set when generating the token, because - # that information will not be available when validating it. - local Bugzilla->user->{userid} = 0; - $vars->{'login_request_token'} = issue_hash_token(['login_request', $value]); - } - # Show the sudo page $vars->{'target_login_default'} = $cgi->param('target_login'); $vars->{'reason_default'} = $cgi->param('reason'); @@ -77,32 +61,16 @@ elsif ($action eq 'prepare-sudo') { } # begin-sudo: Confirm login and start sudo session elsif ($action eq 'begin-sudo') { - # We must be sure that the user is authenticating by providing a login - # and password. - # We only need to do this for authentication methods that involve Bugzilla - # directly obtaining a login (i.e. normal CGI login), as opposed to other - # methods (like Environment vars login). - - # First, record if Bugzilla_login and Bugzilla_password were provided - my $credentials_provided; - if (defined($cgi->param('Bugzilla_login')) - && defined($cgi->param('Bugzilla_password'))) - { - $credentials_provided = 1; - } - - # Next, log in the user my $user = Bugzilla->login(LOGIN_REQUIRED); my $target_login = $cgi->param('target_login'); my $reason = $cgi->param('reason') || ''; - # At this point, the user is logged in. However, if they used a method - # where they could have provided a username/password (i.e. CGI), but they - # did not provide a username/password, then throw an error. - if ($user->authorizer->can_login && !$credentials_provided) { - ThrowUserError('sudo_password_required', - { target_login => $target_login, reason => $reason }); + if ($user->authorizer->can_login) { + my $password = $cgi->param('password') + or ThrowUserError('sudo_password_required', + { target_login => $target_login, reason => $reason }); + $user->check_current_password($password); } # The user must be in the 'bz_sudoers' group @@ -112,12 +80,25 @@ elsif ($action eq 'begin-sudo') { object => 'sudo_session' } ); } - + # Do not try to start a new session if one is already in progress! if (defined(Bugzilla->sudoer)) { ThrowUserError('sudo_in_progress', { target => $user->login }); } + # Get & verify the target user (the user who we will be impersonating) + my $target_user = new Bugzilla::User({ name => $target_login }); + unless (defined($target_user) + && $target_user->id + && $user->can_see_user($target_user)) + { + ThrowUserError('user_match_failed', { name => $target_login }); + } + + if ($target_user->in_group('bz_sudo_protect')) { + ThrowUserError('sudo_protected', { login => $target_user->login }); + } + # Did the user actually go trough the 'sudo-prepare' action? Do some # checks on the token the action should have left. my ($token_user, $token_timestamp, $token_data) = @@ -132,18 +113,6 @@ elsif ($action eq 'begin-sudo') { } delete_token($cgi->param('token')); - # Get & verify the target user (the user who we will be impersonating) - my $target_user = new Bugzilla::User({ name => $target_login }); - unless (defined($target_user) - && $target_user->id - && $user->can_see_user($target_user)) - { - ThrowUserError('user_match_failed', { name => $target_login }); - } - if ($target_user->in_group('bz_sudo_protect')) { - ThrowUserError('sudo_protected', { login => $target_user->login }); - } - # Calculate the session expiry time (T + 6 hours) my $time_string = time2str('%a, %d-%b-%Y %T %Z', time + MAX_SUDO_TOKEN_AGE, 'GMT'); diff --git a/template/en/default/admin/sudo.html.tmpl b/template/en/default/admin/sudo.html.tmpl index 8cdfb5204..8f7e3fcea 100644 --- a/template/en/default/admin/sudo.html.tmpl +++ b/template/en/default/admin/sudo.html.tmpl @@ -17,9 +17,9 @@

The sudo feature of Bugzilla allows you to impersonate a - user for a short time While an sudo session is in progress, every action you - perform will be taking place as if you had logged in as the user whom will be - impersonating. + user for a short time. While a sudo session is in progress, every action you + perform will be taking place as if you had logged in as the user who will be + impersonated.

@@ -67,12 +67,8 @@ [% IF user.authorizer.can_login %]

- Finally, enter : - - - + Finally, enter : +
This is done for two reasons. First of all, it is done to reduce the chances of someone doing large amounts of damage using your @@ -80,9 +76,8 @@ time to consider if you really need to use this feature.

[% END %] - +

- Click the button to begin the session: -- cgit v1.2.3-24-g4f1b