312439 - The user being impersonated has "moral" rights to keep informed

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Frédéric Buclin]

From userprefs.cgi, you can see who is watching you (great!). But when someone
is impersonating your account, you aren't notified!

As a user, and because impersonating another user's account is much severe than
watching it, I do want to keep informed (mainly by email) when someone has
started using my account. I want to know who and when. And again, I want to know
when the user stopped using my account.

Morally and legally, not only the impersonated user should be kept informed as
soon as possible, but this should also be logged and viewable from the
editusers.cgi page, for instance to prove that joel didn't clear the security
flag on the most critical bug of the year, but that I did (using his account)
for some unknwown reasons. And please don't tell me that only trustable users
can or should be able to impersonate others user account and that consequently,
this doesn't need to be logged. ;)

Oh... and as a second part of this bug, relogin.cgi should also ask for the
reason a user tries to impersonate someone else's account. This message could
then be added to the email sent to the impersonated user as well as added in the
log file:

On Friday, October 14, 2005, 14:22:37, Frédéric Buclin <LpSolit%gmail.com> has
impersonated Joel Peshkin's account <bugreport%peshkin.net> for the following
reasons:

"Joel is on vacation and asked me to change his email notification prefs to stop
watching wicked, who is definitely too active these days and generates a lot of
bugspam :)"

Comment 1

[Frédéric Buclin]

per discussion with joel, let's consider this as an enhancement, not a bug. But
I really want it fix before 2.22rc1 (the notification, not the logging)

Comment 2

[A. Karl Kornel]

Attachment: Patch v1

relogin.cgi has been updated to accept a 'reason', a 200-character explanation
of why the session is taking place (the field is trimmed if over 200
characters).  When the session starts, an email template (at
`email/sudo.txt.tmpl`) is processed and the result is mailed out to the user
being impersonated (even if no reason is provided).

Requesting review from LpSolit.

Comment 3

[A. Karl Kornel]

Attachment: Patch v1.3

Modification of attachment 199832 [details] [diff] [review] with respect to comments made by LpSolit on
IRC.

The email template has been modified to wrap all bugs at around 72 characters,
and no longer specifies a character set.  Rather than detainting the PATH
directly, globals.pl is now imported instead.  Also, filterexceptions is not
modified (since the email template is text/plain).

Requesting review from LpSolit.

Comment 4

[Frédéric Buclin]

Comment on attachment 200252 [details] [diff] [review]
Patch v1.3

>Index: relogin.cgi

>+    # If we have a reason passed in, keep it under 200 characters
>+    my $reason = $cgi->param('reason') || '';
>+    if (length($reason) > 200) {
>+        $reason =~ s/^(.{1,200}).*$/$1/;
>+    }

use substr().


>+    my $reason_string = '';
>+    $reason_string = ('&reason=' . url_quote($reason)) if (length($reason) > 0);

Nit: no need to test it. "&reason=" is fine when no reason is given. Just write '&reason=url_quote($reason)' when redirecting.



>Index: template/en/default/admin/sudo.html.tmpl

>+    Next, please take a moment to explain why you are doing this:<br>
>+    <input type="text" name="reason" size="80" maxlength="200">

I think you should use a <textarea>.



>+++ template/en/default/email/sudo.txt.tmpl	2005-10-20 11:54:47.000000000 -0700

By the discussion on IRC, we much prefer something of the form:

NOTICE: At [date/time}, user [name/email/etc..] used the "sudo" capability within bugzilla in order to operate bugzilla using your account.  The reason given for this action is [reason].  If you feel that this action was inappropriate, please contact [maintainer].

See also my suggestion in comment 0.

Comment 5

[A. Karl Kornel]

Attachment: Patch v1.6

Modification of attachment 200252 [details] [diff] [review] with respect to comment 4:

> >Index: relogin.cgi
> 
> >+    # If we have a reason passed in, keep it under 200 characters
> >+    my $reason = $cgi->param('reason') || '';
> >+    if (length($reason) > 200) {
> >+        $reason =~ s/^(.{1,200}).*$/$1/;
> >+    }
> 
> use substr().

Updated.

> >+    my $reason_string = '';
> >+    $reason_string = ('&reason=' . url_quote($reason)) if (length($reason) > 0);
> 
> Nit: no need to test it. "&reason=" is fine when no reason is given. Just
> write '&reason=url_quote($reason)' when redirecting.

Updated.

> >Index: template/en/default/admin/sudo.html.tmpl
> 
> >+    Next, please take a moment to explain why you are doing this:<br>
> >+    <input type="text" name="reason" size="80" maxlength="200">
> 
> I think you should use a <textarea>.

I'm still not sure about pushing the max. length of a query string.  Leaving alone.

> >+++ template/en/default/email/sudo.txt.tmpl	2005-10-20 11:54:47.000000000 -0700
> 
> By the discussion on IRC, we much prefer something of the form:
> <<<snip>>>

Updated.

I have also added a page.cgi document explaining this feature, which is linked to from the email.  I also made a change to the docs, to correct an error on how to get to the feature.

Requesting review from LpSolit.

Comment 6

[Frédéric Buclin]

Comment on attachment 200943 [details] [diff] [review]
Patch v1.6

>Index: relogin.cgi

>+use Bugzilla::BugMail;
> use Bugzilla::CGI;

Nit: we access CGI params using Bugzilla->cgi, so Bugzilla::CGI is not required here.


>+    $reason = substr($reason, $[, 200);

Nit: I prefer substr($reason, 0, 200) as is commonly used in the rest of the code.


>+    $reason = substr($reason, $[, 200);

Nit: same remark.


>+    # If a message has been provided, go ahead and send out the message now

Nit: err... we send an email even if no message has been provided.



>Index: docs/xml/administration.xml

>+        instructions on how to use it.  After reading the text, simply 
>+        enter the login of the user you would like to impersonate and press 
>+        the button.</para>

Nit: I would also mention that the user should give the reason to impersonate the other user.



>+++ template/en/default/email/sudo.txt.tmpl	2005-10-26 16:18:51.000000000 -0700

>+Subject: Someone else is using your [% terms.Bugzilla %] account

Nit: I would avoid the use of "someone". Maybe something like "your account on Bugzilla has been impersonated".


>+    A [% terms.Bugzilla %] user ([% sudoer.login %]) has used the 

Nit: remove "A Bugzilla user" and use sudoer.identity instead. This way you get something like:

Frederic Buclin <[e-mail]> has used the ...



>+'sudo' feature of [%+ terms.Bugzilla %] to access 

Nit: we are talking about Bugzilla, so I think it's clear that's it's a Bugzilla feature. I would remove "of Bugzilla".


>+[% IF reason %]
>+    [%+ sudoer.login %] provided the following reason for using this 
>+feature: [% reason %]

Nit: here again, use sudoer.identity and move [% reason %] on it's own line, maybe enclosed in double quotes to make clear it's a comment from the user.



>+++ template/en/default/pages/sudo.html.tmpl	2005-10-26 16:45:16.000000000 -0700

>+another, in something called an <i>sudo session</i>, so long as the person 

s/an/a/


>+  and for doing critical work when the impersonated using is unavailable.  The 

s/using/user/


>+  request access, contact the maintainer of this insteallation: 

s/insteallation/installation/


>+  <a href="mailto:[% Param("maintainer") FILTER html %]">
>+  [%- Param("maintainer") FILTER html %]</a>.

Don't filter them for now, see bug 159631.


Even if those comments are nits, please fix them in an updated patch. Else it looks good and works fine. r=LpSolit

Comment 7

[Frédéric Buclin]

Comment on attachment 200943 [details] [diff] [review]
Patch v1.6

>Index: relogin.cgi

> use Bugzilla::Auth::Login::WWW;

Another point: this is useless as you can access can_logout() by using $user->get_flag('can_logout'). Please fix it too.

Comment 8

[Marc Schumann [:Wurblzap]]

Maybe this belongs to IRC rather than here: $[ is actually a good choice, see http://aspn.activestate.com/ASPN/docs/ActivePerl/5.8/lib/Pod/perlfunc.html#item_substr... Why not use it?

Comment 9

[A. Karl Kornel]

Attachment: Patch v1.8

Modification of attachment 200943 [details] [diff] [review] with respect to comment 6 and comment 7:

> >Index: relogin.cgi
> 
> >+use Bugzilla::BugMail;
> > use Bugzilla::CGI;
> 
> Nit: we access CGI params using Bugzilla->cgi, so Bugzilla::CGI is not
> required here.

Updated.

> > use Bugzilla::Auth::Login::WWW;
> 
> Another point: this is useless as you can access can_logout() by using
> $user->get_flag('can_logout'). Please fix it too.
> 

Updated

> >+    $reason = substr($reason, $[, 200);
> 
> Nit: I prefer substr($reason, 0, 200) as is commonly used in the rest of the
> code.
> 
> 
> >+    $reason = substr($reason, $[, 200);

Ignored, per comment 8 and comments on IRC.

> >+    # If a message has been provided, go ahead and send out the message now
> 
> Nit: err... we send an email even if no message has been provided.

Updated.

> >Index: docs/xml/administration.xml
> 
> >+        instructions on how to use it.  After reading the text, simply 
> >+        enter the login of the user you would like to impersonate and press 
> >+        the button.</para>
> 
> Nit: I would also mention that the user should give the reason to impersonate
> the other user.

Expanded.

> >+++ template/en/default/email/sudo.txt.tmpl	2005-10-26 16:18:51.000000000 -0700
> 
> >+Subject: Someone else is using your [% terms.Bugzilla %] account
> 
> Nit: I would avoid the use of "someone". Maybe something like "your account on
> Bugzilla has been impersonated".

Replaced with "[Bugzilla] Your account ***** is being impersonated" 

> >+    A [% terms.Bugzilla %] user ([% sudoer.login %]) has used the 
> 
> Nit: remove "A Bugzilla user" and use sudoer.identity instead. This way you
> <<<snip>>>

Updated, although I'm making sure the first char. is uppercase.

> >+'sudo' feature of [%+ terms.Bugzilla %] to access 
> 
> Nit: we are talking about Bugzilla, so I think it's clear that's it's a
> Bugzilla feature. I would remove "of Bugzilla".

Updated.

> >+[% IF reason %]
> >+    [%+ sudoer.login %] provided the following reason for using this 
> >+feature: [% reason %]
> 
> Nit: here again, use sudoer.identity and move [% reason %] on it's own line,
> maybe enclosed in double quotes to make clear it's a comment from the user.

Updated.

> >+++ template/en/default/pages/sudo.html.tmpl	2005-10-26 16:45:16.000000000 -0700
> 
> >+another, in something called an <i>sudo session</i>, so long as the person 
> 
> s/an/a/

Updated.

> >+  and for doing critical work when the impersonated using is unavailable.  The 
> 
> s/using/user/

Updated.

> >+  request access, contact the maintainer of this insteallation: 
> 
> s/insteallation/installation/

Updated.

> >+  <a href="mailto:[% Param("maintainer") FILTER html %]">
> >+  [%- Param("maintainer") FILTER html %]</a>.
> 
> Don't filter them for now, see bug 159631.

Updated.

Comment 10

[Frédéric Buclin]

Comment on attachment 201468 [details] [diff] [review]
Patch v1.8

>Index: relogin.cgi

> use Bugzilla::Auth::Login::WWW;

Nit: please remove this line now that you use get_flag('can_logout').


>+    $vars->{'will_logout'} = 1 if Bugzilla->user->get_flag('can_logout');

Nit: even better: $vars->{'will_logout'} = $user->get_flag('can_logout');
as get_flag('can_logout') returns either 0 or 1, and $user has already be defined.



>+++ template/en/default/email/sudo.txt.tmpl	2005-10-31 14:37:06.000000000 -0800

>+    [% sudoer.identity FILTER ucfirst %] has used the 

This paragraph is not displayed anymore. You have to write [%+ instead of [% alone.
Moreover, as sudoer.identity is a username, you shouldn't change its case. So please remove 'FILTER ucfirst'.


>+[% IF reason %]
>+    [%+ sudoer.identity %] provided the following reason for doing this:
>+"[% reason %]"

The reason can be pretty long (200 characters) and should be wrapped. Write:
[% reason FILTER wrap_comment %]

Note that this cause the closing quote to jump to a new line (this is due to Bugzilla::Util::wrap_comment() itself, which ends with \n). So remove quotes and add a newline before displaying the reason, to make it as visible as possible.

I r- this patch due to problems in sudo.txt.tmpl (I missed the second one in my previous review).

Comment 11

[Frédéric Buclin]

Comment on attachment 201468 [details] [diff] [review]
Patch v1.8

>+++ template/en/default/email/sudo.txt.tmpl	2005-10-31 14:37:06.000000000 -0800

>+Content-Type: text/plain

BugMail::MessageToMTA() calls BugMail::encode_message() which does:

    $head->mime_attr('Content-Type' => 'text/plain')
        unless defined $head->mime_attr('content-type');
    $head->mime_attr('Content-Type.charset' => 'UTF-8');

So I guess this line is useless.

Comment 12

[A. Karl Kornel]

Attachment: Patch v1.9

Modification of attachment 201468 [details] [diff] [review] with respect to comment 10:

(In reply to comment #10)
> (From update of attachment 201468 [details] [diff] [review] [edit])
> >Index: relogin.cgi
> 
> > use Bugzilla::Auth::Login::WWW;
> 
> Nit: please remove this line now that you use get_flag('can_logout').

Interesting.  For some reason it's not in my file, but it's in the patch and it's in my SVN archive.  I guess I fixed this AFTER submitting the last patch 8-|

> >+    $vars->{'will_logout'} = 1 if Bugzilla->user->get_flag('can_logout');
> 
> Nit: even better: $vars->{'will_logout'} = $user->get_flag('can_logout');
> as get_flag('can_logout') returns either 0 or 1, and $user has already be
> defined.

OK

> >+++ template/en/default/email/sudo.txt.tmpl	2005-10-31 14:37:06.000000000 -0800
> 
> >+    [% sudoer.identity FILTER ucfirst %] has used the 
> 
> This paragraph is not displayed anymore. You have to write [%+ instead of [%
> alone.
> Moreover, as sudoer.identity is a username, you shouldn't change its case. So
> please remove 'FILTER ucfirst'.

Updated (on both counts).

> >+[% IF reason %]
> >+    [%+ sudoer.identity %] provided the following reason for doing this:
> >+"[% reason %]"
> 
> The reason can be pretty long (200 characters) and should be wrapped. Write:
> [% reason FILTER wrap_comment %]

Good catch.

> Note that this cause the closing quote to jump to a new line (this is due to
> Bugzilla::Util::wrap_comment() itself, which ends with \n). So remove quotes
> and add a newline before displaying the reason, to make it as visible as
> possible.

OK, although there is one other thing.  I was told before to wrap everything at 72 characters.  From what I saw, wrap_comment wraps at 80, and I can't see a way to change that without changing a constant (probably not a good idea) or changing the wrap_comment function (possible).  Although this may not be a big enough problem.

Comment 13

[Frédéric Buclin]

Comment on attachment 201683 [details] [diff] [review]
Patch v1.9

>+++ template/en/default/email/sudo.txt.tmpl	2005-11-02 13:55:00.000000000 -0800

>+    [%+ sudoer.identity %] has used the 
>+'sudo' to access [%+ terms.Bugzilla %] using your account.

Nit: has used the 'sudo' _feature_ to ... maybe?

works fine. r=LpSolit

Comment 14

[Dave Miller [:justdave]]

same nit as LpSolit...  make sure you adjust that on checkin.

Comment 15

[timeless]

Comment on attachment 201683 [details] [diff] [review]
Patch v1.9

>RCS file: /cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v
>retrieving revision 1.29
>@@ -113,11 +114,16 @@
>     # Log out and Redirect user to the new page

why is Redirect capitalized?

>RCS file: /cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v
>retrieving revision 1.52
>@@ -538,12 +538,15 @@
>+        If you have access to use this feature, you may start a session by

either "permission to use" or just "access to", not "access to use"

>+        going to the Edit Users page.  Search for a user and click on 

I don't think you should end your sentence at "Edit Users page", because that doesn't start a session.
Alternatively, replace "going" with "taking the following steps" or "following these steps" or something.

>+        their login.  You should notice a link below their login name titled 

s/notice/see/g ?

>+        "Impersonate this user".  Click on the link.  This will take you 
>+        to a page where you will see a description of the feature and 
>+        instructions on how to use it.  After reading the text, simply 

the "on how to" thing feels awkard. i don't know how to fix it, i'd like to see it rewritten.

>+        enter the login of the user you would like to impersonate, provide 
>+        a short message explaining why you are doing this, and press the 
>+        button.</para>

this doesn't make sense. if i just searched for a user and selected that user as my victim, why do i have to reenter a user login?

>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v
>retrieving revision 1.1
>@@ -65,7 +65,18 @@
>+    The message you enter here will be sent to the impersonated user by email.

is this guaranteed? what if email is turned off globally for the bugzilla install?

>+    You may leave this empty if you wish, but they will still know that you 
>+    are impersonating them.

>+    Finally, click the button to begin the session:

I don't have a mouse.

>+++ template/en/default/pages/sudo.html.tmpl	2005-10-31 15:01:59.000000000 -0800
>+  # The Initial Developer of the Original Code is Netscape Communications
>+  # Corporation. Portions created by Netscape are
>+  # Copyright (C) 2005 Netscape Communications Corporation. All
>+  # Rights Reserved.

Oh really?

>+  [%+ terms.Bugzilla %] includes the ability to have one user impersonate 
>+another, in something called a <i>sudo session</i>, so long as the person 
>+doing the impersonating has the appropriate privileges.

I don't like this. "[%+ terms.Bugzilla %] allows specially privileged users to impersonate 
>+other users (that they can see?), in something called a <i>sudo session</i>."

>+  To use this feature, you must be a member of the appropriate group.  The group 

Is this group not a specific group that can be explicitly listed here?

>+  If you would like to be protected from impersonation, you should contact the 
>+  maintainer of this installation to see if that is possible.  People with 

the code later sure seems to indicate that it is possible. perhaps you mean to see if the adminsitrator is willing?

>+  access to this feature are protected automatically.

I don't like "protected".

>+    You are a member of the <b>bz_sudoers</b> group.  You may use this 

Sure seems like the group is hard coded.


>+++ template/en/default/email/sudo.txt.tmpl	2005-11-02 13:55:00.000000000 -0800
>@@ -0,0 +1,43 @@
>+[%# [e-mail] %]
>+  # The Initial Developer of the Original Code is Netscape Communications
>+  # Corporation. Portions created by Netscape are
>+  # Copyright (C) 2005 Netscape Communications Corporation. All
>+  # Rights Reserved.

oh really?

>+Subject: [[% terms.Bugzilla %]] Your account [% user.login -%]
>+ is being impersonated
>+
>+    [%+ sudoer.identity %] has used the 
>+'sudo' to access [%+ terms.Bugzilla %] using your account.

"the sudo". yuck.

Comment 16

[A. Karl Kornel]

Attachment: Checked-In Version

Comment 17

[A. Karl Kornel]

Checking in relogin.cgi;
/cvsroot/mozilla/webtools/bugzilla/relogin.cgi,v  <--  relogin.cgi
new revision: 1.30; previous revision: 1.29
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/admin/sudo.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/sudo.html.tmpl,v  <--  sudo.html.tmpl
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/email/sudo.txt.tmpl,v
done
Checking in template/en/default/email/sudo.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/sudo.txt.tmpl,v  <--  sudo.txt.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/sudo.html.tmpl,v
done
Checking in template/en/default/pages/sudo.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/sudo.html.tmpl,v  <--  sudo.html.tmpl
initial revision: 1.1
done

Comment 18

[Max Kanat-Alexander]

I'd like to regsiter my general disagreement with this -- unix admins have been using sudo for decades without informing the user being impersonated.

But anyhow, I suppose it's already checked-in.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> I'd like to regsiter my general disagreement with this -- unix admins have been
> using sudo for decades without informing the user being impersonated.

Bugzilla is not *nix!!!

Comment 20

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960.