218917 - Allow login_name != email_address, so address isn't displayed (anti-spam effect too)

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Stephen Lee]

Bug 120030 finished with a temporary stop-gap to hamper the use of general-
purpose email harvesters (which result in spam). That is a very straightforward 
address munging, which should not hamper "normal" users, but will almost 
certainly become ineffective when spammers "catch on". In short, bug 120030 
prevents it being a "paradise" for spammers, but it's still rather too easy for 
the "clever" or determined spammer.

Proposal as from my comment, bug 120030 comment 14,
: Add a column to the profiles table called "email_address" with the initial 
: value the same as login_name, change only the email sending code to use the
: NEW column, and then provide a mechanism for the login_name to be changed to 
: something other than an email address when confirmed by the owner of the
: email address.
...
: After all, spammers are in principle at least just as capable of unmunging an 
: address as anyone else, and some people might not want their address directly 
: revealed AT ALL. There is no reason to have a condition of reporting a bug to 
: be having ones email address displayed to the world.

I would suggest that *this* RFE is just about having a separate login name and 
email address, together with a simple script to obtain one from the other.
Non-legacy login names should not contain an @ sign.

At least during the transition period, the login form, quietly_check_login, 
etc. should accept either login_name or email_address as input.

For the "simple script" something similar to bug 120030 Comment #44 (B.Bober)
methods for bringing up the personal page would be:
show_user.cgi?userid=UNIQUE_ID_HERE
show_user.cgi?login_name=LOGIN_NAME_HERE
show_user.cgi?email=EMAIL_ADDRESS_HERE

(last one added by me - suppose you just try and CC someone or allocate a bug 
to them by email address and it failed, you need a way of discovering their 
login_name - a reasonable alternative might be to always allow email_address to 
be used in inputs, again this might only be during a transition period).

Initially, this should show the login_name, email_address, and Real Name. In 
any case, the proposed show_user.cgi would almost certainly be restricted to 
logged in users, meaning anonymous spammers / email harvesters cannot use it to 
get at the addresses, or validate existing addresses.

In Bug 120030 Comment #16 From Toms Baugis talked about:
: Privileged users (a new checkbox in user configuration - "can see email
: addresses")
which might be incorporated into this if it was felt necessary, so that only 
some users can obtain email addresses.

Further enhancements could replace this with something more advanced such as a 
user-defined "information" page (perhaps giving further user-defined 
restrictions on who can access it), email contact forms or options to avoid 
showing email addresses at all, etc. etc.


Some such as Mike Miller, bug 120030 comment 47 felt that:
: There's no reason to make it complicated.  The system should never list email
: addresses, period.  

: If someone needs to send someone a message, they should use the system, and
: negotiate for email if they want.

This may be a good long-term solution, but I would assert that the implied use 
of an email contact form for this would be beyond the remit of this RFE, and 
should instead be done as a separate enhancement later.


Bug 215439 deals with an email contact form, but attempts to do it without the 
necessary login_name / email_address split. I can't see how this can work 
without such a split and would suggest that bug should depend on this one (so 
have marked it as such).


A side issue (probably also to be considered as a different bug) is what to do 
about email addresses in comments, change logs, etc.

There may still be scope for the various address-munging techniques from bug 
120030 for these situations, but IMHO they don't belong with this RFE.


Another point to consider that occurred as I was putting all this in is:
How useful is it to know that a particular bugzilla user has a mozilla.org, or 
netscape.com etc. address? Would an alternative indication of "User XYZ is an 
official developer" be needed?

Comment 1

[Gervase Markham [:gerv]]

This sounds to me like an awful lot of work for very little benefit, and quite a
bit of loss. :-|

Given the research I've found on the web, I don't see that any more work is
needed in the email anti-spam arena. But, if we were to do something else, it
seems to me that bug 219021 is a watertight solution (no spammer will get a
Bugzilla login; our databases are far too small) for very little effort.

This solution seems to be harder to code, requires new UI work to change
handles, would cause more confusion and arguments ("I'm 'dave'!" - "No, I'm
'dave'"), and removes useful information (someone's email address, which, as you
say, says something about them) from the UI. 

Gerv

Comment 2

[Bradley Baetz (:bbaetz)]

I'm sure theres another bug on this... There is some infrastructure for this, so
it wouldn't be that hard to do in theory.

This is also useful for non-public bugzillas.

Comment 3

[Matthew Tuck [:CodeMachine]]

This is a privacy issue as well.

Furthermore, spammers are well aware of these sorts of evasion techniques, they
use them in their own mailings to try and avoid filters.  While they may not
have taken the effort to workaround HTML evasion techniques, it only takes one
and your address could spread.

Comment 4

[Christopher Nebergall]

Bug 237390 added as dependency.  If you want to use web server based
authentication, you will most likely need to be able handle to the case where
login name is different than email address.

Comment 5

[G Thornock]

This isn't just a spam-fighting issue.  It's also:

1 - Very much to be desired for internal company Bugzilla installations -- why
off Earth should we use a full email address to log into Bugzilla when everyone
is at the same domain anyway

2 - A great convenience for users of small databases whether private or public,
as they can then log in as "Fred" instead of
"[e-mail]"

3 - An enhancement for privacy.  There are plenty of us around who don't want to
have our email addresses available to the public in any form.

Comment 6

[Marc Schumann [:Wurblzap]]

*** Bug 270368 has been marked as a duplicate of this bug. ***

Comment 7

[Predrag Supurovic]

I agree. Nothing else matters too much but privacy. I do not want my email to
spread among unknown users. I have to use Bugzilla to get support for Mozilla.
It I do not have to I would stay away from this service. 

This goes beyond the line of commn courtesy. Not just that system shows my email
address but it also did not warn me that it would do so before I registered.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Comment 9

[Stephen Lee]

(In reply to comment #8)
> Created an attachment (id=179643) [edit]
> work in progress

Looks like this is going in the direction I was originally thinking, despite my 
confusing comment 0... I had a comment-in-progress to clarify my original 
thinking (basically showing that the main complication was in upgrading 
existing installations, but that this can be a surprisingly simple change 
otherwise).


Judging by the changes I see in your work in progress, a lot of the structure 
of bugzilla has changed sufficiently that the specifics of the comment I was 
going to get round to pasting sometime are suffering from a form of bit-rot!

So rephrasing slightly, the main points I was going to make were:

* Existing "login_name" remains, so most code need not be touched, and
  automatically keeps the email address private for a non-legacy login_name.

* Non-legacy login names SHOULD NOT contain an @ sign. A login_name that is a
  valid email address MUST be equal to the email_address for that account.

* Upgrading sites using emailsuffix should see little difference in operation.
  These sites are in effect already using the username!=email facility. It
  may be possible for this param to remain with a different function (such
  that the new account creation page needs only to ask for username).

* Other upgrading sites will either need to have admin-allocated or a
  "legacy" username. Large sites (b.m.o etc.) would need a UI to allow a
  user to change their username (perhaps as a once-only operation).

* If the UI for users changing their own names is difficult or contentious,
  it should be a separate RFE, as this aspect of it would not be needed on
  all sites, and the existing administrative UI (editusers.cgi) should be
  sufficient to handle changes of username in many cases.

* There may be some value to allowing either of username+password or
  email+password to be used for logging in - particularly if usernames are
  likely to be changed automatically or by the admin.

* A new RFE could be filed to make some group memberships "publicly visible",
  replacing the information lost by currently seeing @mozilla.org etc.

* I also had some thoughts about reserving candidate usernames where a user
  has a sufficiently unique part of their email address... I'm not sure if
  this is worth pursuing further.

* Some sites and/or users may actually want to publish email addresses to the
  world. Some thought could be given to this.

Comment 10

[Frédéric Buclin]

*** Bug 179622 has been marked as a duplicate of this bug. ***

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements separate "username" user attribute

Comment 12

[Frédéric Buclin]

Comment on attachment 181377 [details] [diff] [review]
patch v1: implements separate "username" user attribute

>--- checksetup.pl	19 Apr 2005 21:43:37 -0000	1.397
>+++ checksetup.pl	21 Apr 2005 02:24:28 -0000
>+# 2004-04-20 [e-mail] bug 218917
>+$dbh->bz_add_field('profiles', 'username', 'varchar(25)');

bz_add_field() does not exist anymore. Use bz_add_column() instead. Moreover, I
think it's safe to add a UNIQUE username index.


>--- editusers.cgi	19 Apr 2005 17:36:25 -0000	1.87
>+++ editusers.cgi	21 Apr 2005 02:24:29 -0000

>+my $username        = $cgi->param('username');

Nit: $username = trim($cgi->param('username') || ''); would ensure 1) that you
never get trim(undef) and 2) that $username is always defined.


>     $dbh->bz_lock_tables('profiles WRITE',
>                          'profiles_activity WRITE',
>                          'email_setting WRITE',
>+                         'groups READ',
>+                         'user_group_map READ',
>                          'namedqueries READ',
>                          'whine_queries READ',
>                          'tokens READ');

Why locking groups and user_group_map too?? I don't see any SQL request
requiring it.


>+    # Login and password are validated now, and realname, disabledtext,
>+    # and username are allowed to contain anything

username cannot contain anything. We checked that it was not already used!!


>     trick_taint($login);
>     trick_taint($realname);
>+    trick_taint($username) if $username;

Nit: No need to check if $username is defined using my "trick" above. As the
real name can be undefined (empty string) too, we should be consistent.

Moreover: we should not allow the username to be an empty string. We should be
sure that NULL is saved in the DB instead of ''. Actually, this goes wrong for
the real name and this should be fixed, IMO.


>+        if ($username ne $usernameold) {
>+            is_available_username($username)
>+              || ThrowUserError('account_exists', {'username' => $username});

What happens if $username eq '' ? Maybe the code is right, but I want to make
sure you have considered this case too.


>--- userprefs.cgi	12 Apr 2005 20:21:44 -0000	1.74
>+++ userprefs.cgi	21 Apr 2005 02:24:29 -0000

>+    SendSQL("SELECT username, realname FROM profiles WHERE userid = $userid");
>+    ($vars->{'username'}, $vars->{'realname'}) = FetchSQLData();

Please use Bugzilla->dbh stuff instead.


>+    SendSQL("SELECT username, realname FROM profiles WHERE userid = $userid");
>+    my ($old_username, $old_realname) = FetchSQLData();

Same here.

>+    my $new_username = trim($cgi->param('username'));
>+    my $new_realname = trim($cgi->param('realname'));

Nit: I think the template returns empty strings if none provided by the user.
But to avoid any "uninitialized value" error message, I would add || '' to make
sure there is something to trim.


>--- Bugzilla/User.pm	10 Apr 2005 17:49:48 -0000	1.54
>+++ Bugzilla/User.pm	21 Apr 2005 02:24:29 -0000

>+sub insert_new_user ($$;$$$) {

>     trick_taint($realname);
>+    trick_taint($username) if $username;

We should be consistent on how $realname and $username are considered. Be sure
to have NULL values in the DB if they are not defined or empty strings only.


>--- Bugzilla/Auth/Verify/DB.pm	31 Jan 2005 19:26:01 -0000	1.4
>+++ Bugzilla/Auth/Verify/DB.pm	21 Apr 2005 02:24:29 -0000
>+    my $sth1 = $dbh->prepare_cached("SELECT userid FROM profiles " .
>+                                   "WHERE username=?");
>+    my $sth2 = $dbh->prepare_cached("SELECT userid FROM profiles " .
>                                    "WHERE login_name=?");
>-    my ($userid) = $dbh->selectrow_array($sth, undef, $username);
>+    my ($userid) = $dbh->selectrow_array($sth1, undef, $username);
>+    if (!$userid) {
>+        ($userid) = $dbh->selectrow_array($sth2, undef, $username);
>+    }

Humm.. I'm not sure I like this. What happens if $user1->username() eq
$user2->login() ??? If I defined my username to be [e-mail], could I log
in using your account (by hacking the URL if necessary)?


>--- Bugzilla/DB/Schema.pm	19 Apr 2005 21:43:37 -0000	1.25
>+++ Bugzilla/DB/Schema.pm	21 Apr 2005 02:24:30 -0000
>             userid         => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>                                PRIMARYKEY => 1},
>+            username       => {TYPE => 'varchar(25)'},

Add a UNIQUE username index to the profiles table.


>--- template/en/default/account/create.html.tmpl	18 Jan 2004 18:39:11 -0000	1.8
>+++ template/en/default/account/create.html.tmpl	21 Apr 2005 02:24:30 -0000

>     <tr>
>       <td align="right">
>+        <b>Username:</b>
>+      </td>
>+      <td>
>+        <input size="15" name="username">
>+      </td>
>+    </tr>

size="15" or size="25" ?? From your field definition above, it should be 25.


>--- template/en/default/admin/users/userdata.html.tmpl	28 Feb 2005 20:41:45 -0000	1.1
>+++ template/en/default/admin/users/userdata.html.tmpl	21 Apr 2005 02:24:30 -0000

>+  <th><label for="username">Username:</label></th>
>+  <td>
>+    [% IF editusers %]
>+      <input size="64" maxlength="255" name="username" 
>+             id="username" value="[% otheruser.username FILTER html %]" />

size = 64, 255, 15 or 25???

Comment 13

[Stephen Lee]

(In reply to comment #12)
> think it's safe to add a UNIQUE username index.

I would suggest it is unsafe not to add one... the whole point is that this is 
a *unique* username that can be used to unambiguously refer to users without 
having to reveal their email address.

> Moreover: we should not allow the username to be an empty string. We should be
> sure that NULL is saved in the DB instead of ''.
[etc...]

Ummm.... is this patch allowing username to be NULL? How can we display 
usernames exclusively in place of email_address if some usernames are NULL?

> Humm.. I'm not sure I like this. What happens if $user1->username() eq
> $user2->login() ???

Surely the code MUST explicitly ensure that this cannot be the case...

My original suggestion was that "Non-legacy login names SHOULD NOT contain an @ 
sign. A login_name that is a valid email address MUST be equal to the 
email_address for that account."

Certainly there needs to be some rule about what constitutes a valid username.

I wondered about a stronger rule e.g. that additionally, a username SHOULD NOT 
be a substring of any other user's email address, unless it was a "better" 
substring of your own email address, but was not sure if that was workable.


I also note that the current patch works the opposite way around to my original 
proposal... adding a "username" field, but keeping the login_name as an email 
address, rather than adding an "email_address" field such that the login_name 
displayed to other users can relax the requirement of having to be a valid 
email address. Is this method is expected to work better than my original 
proposal?

My original "gut feeling" had been that the login_name would be used in fewer 
places as an email address than as a username, making the transition towards 
email address privacy easier by that route... I am not as familiar with post-
2.16 code, however, so may now be mistaken on that point.

In order to remove ambiguity, it may even be possible to [aim to] completely 
remove the existing "login_name" field, and use "email_address" and "username" 
fields in its place. That method would necessitate a full audit of every use of 
login_name in order to replace with a use of one or both of the new fields.

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> Ummm.... is this patch allowing username to be NULL?

Yes, it does. Simply because the user's username won't be set until he chooses one.


 How can we display 
> usernames exclusively in place of email_address if some usernames are NULL?

We could fall back to the email address if the username is missing. If the user
does not want his email address to be displayed, he is free to go to
userprefs.cgi and set one.


> > Humm.. I'm not sure I like this. What happens if $user1->username() eq
> > $user2->login() ???
> 
> Surely the code MUST explicitly ensure that this cannot be the case...

I agree. But my comments are based on my actual review, not on a possible future
patch. ;)


> I wondered about a stronger rule e.g. that additionally, a username SHOULD NOT 
> be a substring of any other user's email address

I'm fully opposed and would deny review for that. What if your username is foo
and a new user's email address is [e-mail]?? You violate your rule,
but we cannot tell this user "hey, you cannot use your email address because one
of our 13'345 users has a username containing foo".


> proposal... adding a "username" field, but keeping the login_name as an email 
> address, rather than adding an "email_address" field such that the login_name 
> displayed to other users can relax the requirement of having to be a valid 
> email address.

First, we don't want to change the whole code by renaming login_name to
email_address. Then, you have to provide a valid email address in all cases to
receive bugmail and your password.



> In order to remove ambiguity, it may even be possible to [aim to] completely 
> remove the existing "login_name" field, and use "email_address" and "username" 
> fields in its place.

No, per my previous comment. Changing the DB schema requires very sensible
reasons. I don't see one here.

Comment 15

[Stephen Lee]

(In reply to comment #14)
> I agree. But my comments are based on my actual review, not on a possible
> future patch. ;)

Sorry - should have made it clear that was directed to the author of such a 
possible future patch...

> > I wondered about a stronger rule [...]
> I'm fully opposed and would deny review for that. What if [...]

I was intending it to be a rule for *new* usernames (and even then possibly 
only during a transition period). i.e. nobody could request to use the 
username 'foo' unless either their email address contains 'foo', or no email 
address *currently* in the DB contains 'foo'. In any case, I'm still not 
convinced it's any better than the alternative of getting an admin to resolve 
disputes.

> First, we don't want to change the whole code by renaming login_name to
> email_address.

When filing this bug I wanted a username that is not necessarily an email 
address and is used throughout the codebase. This would seem to 
happen "automatically" if login_name could be changed to something that is not 
an email address (implying that the few places that actually need an email 
address would need to change, and have an actual email address preserved 
somewhere such as a new email_address column).

The existing patch seems to be *adding* a username field, but presumably 
leaving the login_name (== email address) displayed all over the place. I don't 
have a suitable installation to check this patch on, so I may be wrong, but if 
that is what it is doing, it would seem to me to defeat the purpose of this 
bug, and surely it is THIS method that forces you to change the entire codebase 
to use the username?


> Then, you have to provide a valid email address in all cases to
> receive bugmail and your password.

You already have to do that, and bugzilla also uses this email address as your 
login name, which is the main objection of this bug. I think I have failed to 
grasp the point you are making.


> Changing the DB schema requires very sensible
> reasons. I don't see one here.

Some schema change is necessary since we now have two pieces of data (a 
username and an email address) which were previously forced to be the same 
value (login_name). The question is over what direction the schema change 
should go in, and which of the new pieces of data (if either) should occupy the 
old field.

The options seem to be:
a) columns: "login_name" and "username"
  - all UI code must be changed to show the username instead of login_name.
b) columns: "email_address" and "login_name"
  - all email code must be changed to use email_address instead of login_name.
c) columns: "email_address" and "username"
  - full audit of all uses of login_name to determine which field to use.


Clearly there are advantages to each method, and I've not been in a position to 
try out this patch (so am talking just from inspection...). However, option b 
seems to me to be a "nicer" schema than option a, and I would be surprised if 
changing all UI code currently referring to a login_name was significantly 
easier than changing all email code currently referring to a login_name.

I'm happy to accept that I may be wrong... but it would be nice to have a 
summary of why I am wrong for the record.

Comment 16

[Gervase Markham [:gerv]]

Myk: if usernames were separate from email addresses, IMO this removes the need
for the current "nickname" code, and perhaps even for user auto-completion. What
do you think?

Is the plan to use "no @ sign" as a way of distinguishing between email
addresses and login names? If so, how does that interact with Param("emailsuffix")?

Gerv

Comment 17

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #16)
> Myk: if usernames were separate from email addresses, IMO this removes
> the need for the current "nickname" code

Right.  User->identity can then become just a synonym for User->username (or we
can just remove ->identity completely).


> and perhaps even for user auto-completion.

Auto-completion may still be useful, since it matches against names as well, so
it helps you find someone even if you don't know their username.


> Is the plan to use "no @ sign" as a way of distinguishing between email
> addresses and login names?

As long as we enforce the restriction that no user's username can be another
user's email address, we should be able to search for a user first by username
and then by email address without the risk of finding the wrong user.  But this
needs careful consideration.


> If so, how does that interact with Param("emailsuffix")?

It seems like emailsuffix could probably go away at some point; or perhaps we
can construct email addresses from the combination of username and emailsuffix.

Comment 18

[Frédéric Buclin]

myk: could we go on with this bug?

Comment 19

[Ray Booysen]

*** Bug 317401 has been marked as a duplicate of this bug. ***

Comment 20

[Frédéric Buclin]

*** Bug 320863 has been marked as a duplicate of this bug. ***

Comment 21

[chenwca]

We Really need this, separate the username and the email address, Please.
Thank you.

Comment 22

[Gunnar Wagenknecht]

Actually, I like to use my email address as a login name. It's easy to remember and unique. That's my preferred way in web applications.

If the reason is only because of display purposes and spam protection then I suggest making a configuration option that allows to hide the email address and use the provided real name only. In this case the real name field could become a required field.

If the real name field is empty because a user choose not to enter any value then it can always fallback to the email.

Comment 23

[Takanori MATSUURA]

(In reply to comment #22)

How about the Bug 219021?
Red Hat bugzilla already implement the suppression of email address for NON-login users.

Comment 24

[Gunnar Wagenknecht]

(In reply to comment #23)
> How about the Bug 219021?

I think that's also a good solution. However, it's useless it Spam-bots become registered users sometime. :(

Anyway, all I'd like to say is that it's absolutely valid have this request. But any implementation should provide an option to keep the current behavior and not force all installation to adapt the new scheme.

Comment 25

[Frédéric Buclin]

(In reply to comment #23)
> (In reply to comment #22)
> 
> How about the Bug 219021?
> Red Hat bugzilla already implement the suppression of email address for
> NON-login users.
> 

RedHat's solution isn't perfect yet, see https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=189835

Comment 26

[Denis Roy]

FWIW, we recently coded a MediaWiki plugin that uses the Bugzilla e-mail/password as the authentication source. To prevent/hamper SPAM, we simply replaced the "@" character with ".".  See this page for an example:

http://wiki.eclipse.org/index.php/Special:Recentchanges

Although a human can easily spot where the "@" goes, it should be fairly difficult to programatically determine this, while absolutely preserving the person's identity, as noted in comment 1.

It's by no means a guaranteed way to protect your e-mail address, but I think it's better than what bugzila does now, and it could be a path of least resistance for bugzilla to implement.  See the discussion we've had on this topic:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=118245#c20

D.

Comment 27

[Claus Berghammer]

Whatever decision will be made upon this issue, I hope it will be made soon...

Had massive spam on my, specially for bugzilla created and used, e-mail address, so I had to change it :-(

Off course, I'm not really into the problems, but I don't understand what has to be discussed here for so long? Isn't it possible to simply split the current username/e-mail field into two separate fields, and use the same value for the beginning? (That has been noted here more than once for shure ;-) Than an e-mail to all users that the username doesn't longer has to be the e-mail address. Within days, the spam problem would have been solved...

Claus

Comment 28

[Marc Schumann [:Wurblzap]]

Attachment: Patch 3

Let's see whether I can pick this up.

I see two fundamental modes of addressing other users.
For the first, b.m.o is a good example. Here, I don't care much about e-mail addresses; I mostly think of people's nicks. Plus, it would be good if we could offer some degree of anonymity.
The second one would be a corporate environment. There, I don't want to force users to remember each others' nicks. E-mail addresses are a good way to address a user there.

These two modes are mutually exclusive. I think we need a parameter use_email_as_login for this.

In order to handle the work on this, the patch does not promise anonymity. I propose we do this in follow-up bugs and patches.

Here's a patch which does this. It's not thoroughly tested yet (especially all things around a non-empty emailsuffix parameter are completely untested). It survives basic testing of all actions of createaccount.cgi, editusers.cgi, token.cgi and userprefs.cgi, though, so I'm asking for review anyway to get some feedback.

Comment 29

[Matthew Elvey]

I just landed at https://bugzilla.mozilla.org/post_bug.cgi, after filling out https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox&format=guided and the page source contains:
"<a href="show_bug.cgi?id=405811" title="NEW - RFE: Disable cookies for all prefetching (provide the the functionality and UI option).">Bug 405811</a> has been added to the database</dt>
  <dd><dl><dt>Email sent to:</dt>
  <dd>
      
        <code>bugbot@landfill..."  <== Unmunged email.

Seems the fix for Bug 120030 missed some things, but exploiting this would require a validated login, so probably not a big issue.  Not sure if the current patch would address this.

Comment 30

[Frédéric Buclin]

The UI still appears correctly, i.e. @, but the source code contains @, which is the expected behavior.

Comment 31

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 32

[Marc Schumann [:Wurblzap]]

Attachment: Patch 3.1

Unrotting. Takes bug 399163 into account. And thanks to David, who pointed out my flawed usage of bz_add_column, so that this is no longer broken.

Comment 33

[Frédéric Buclin]

Is there really no way to have this patch split into smaller pieces?

Comment 34

[Frédéric Buclin]

Comment on attachment 302355 [details] [diff] [review]
Patch 3.1

I heard you could split it in smaller pieces (which would make the review easier). Re-request for review if I misunderstood.

Comment 35

[Max Kanat-Alexander]

FWIW, I've thought about this, and I think I'm actually opposed to it. When people use their email address to log in to sites, it's easy to remember. When people have to start remembering an arbitrary username to log into every site, it's pretty tricky. I think more sites are actually moving toward people using their email address or OpenID, and not having unique usernames.

Comment 36

[Chris Lawson (gone)]

(In reply to comment #35)
> When people use their email address to log in to sites, it's easy to remember. 

Sure, but that doesn't address the root issue here -- allowing people the option not to display an e-mail address at all. This bug was originally filed with the intention of making Bugzilla more spam-resistant, and forcing e-mail addresses to be displayed goes against that.

I think it's fine to force the *login* to be an e-mail address, but I think if that's the decision, there needs to be a new bug filed on making the *display* of the e-mail address in bug pages entirely optional.

cl

Comment 37

[Marc Schumann [:Wurblzap]]

With the proposed patches in blocking bugs, installations will have the option whether or not to separate login and email.

Comment 38

[Frédéric Buclin]

What we need is a unique nick which can be displayed everywhere in place of the usual email address. At that point, the login form should/could accept either the nick or the email address to log in.

Comment 39

[Takanori MATSUURA]

If the problem is that email address is always shown in bugzilla, bug 219021 will solve some part of this bug.

Comment 40

[Jesse Ruderman]

Spam may be the most visible effect of this bug, but it's not the only one.

* I've wasted hours searching for bugmail addresses in order to CC people to bugs.  Bugmail addresses are too long to remember and change whenever someone switches ISPs or employers.

* In combination with server-side completion, this bug is a security problem for bugzilla.mozilla.org.  If I ever decide to use my @mozilla.com email address for Bugzilla, the next person who tries to CC me to a bug using my old address might be CCing an attacker.

Comment 41

[Frédéric Buclin]

(In reply to comment #40)
> * I've wasted hours searching for bugmail addresses

Type his name instead. Bugzilla also looks for your real name when doing completion.


> address for Bugzilla, the next person who tries to CC me to a bug using my old
> address might be CCing an attacker.

Unrelated, despite I understand your point of view. It's not that easy to recycle old email addresses, and even with the implementation of unique login names (decoupled from email addresses), one cannot guarantee you will always CC the correct guy, either because you changed your login name (say, from jruderman to jesse or jesse_ruderman and someone else changed his login to jruderman) or because someone else uses e.g jrudermann as login name, and may bring to confusion. We already discussed how to work around this in bug 301686.

Comment 42

[Denis Roy]

(In reply to comment #36)
> I think it's fine to force the *login* to be an e-mail address

Agreed.  As Max said, it makes a lot of things easier.


(In reply to comment #38)
> What we need is a unique nick which can be displayed everywhere

Why not just use the realname?  It could be a user pref to use the realname as the display name, or the email address (default to realname).  All the comments already display the realname anyway, and the cc/qa fields support searching email and realname.

Comment 44

[Ralph Schuster]

Hi Marc! I made various fixes to a 3.2.3 installation I have privately. This fix addresses:

- separate login and email addresses
- have the setup process working correctly (incl. administrator account setup)
- have the "New Account" feature behave accordingly
- unique login names and email addresses
- hide all email addresses constantly (even maintainer one) - template changes
- admin parameters about showing emails or not

If ok I would like to contribute my changes to this and depending bugs.

Comment 45

[Ralph Schuster]

Hi Marc! I made various fixes to a 3.2.3 installation I have privately. This
fix addresses:

- separate login and email addresses
- have the setup process working correctly (incl. administrator account setup)
- have the "New Account" feature behave accordingly
- unique login names and email addresses
- hide all email addresses constantly (even maintainer one) - template changes
- admin parameters about showing emails or not

If ok I would like to contribute my changes to this and depending bugs.

Comment 46

[Rajesh]

Attachment: test

sadfasd

Comment 47

[Jérôme Bouat]

This bug is critical because it makes a security hole into the worlwide mail system.

Comment 49

[Neville Hillyer]

578787 might be a duplicate but is 218917, which was started in 2003, ever going to
result in the sort of security 578787 proposes.

Seven years of discussion does not inspire confidence.

Comment 50

[Clifford Keele]

Uhhhh

Still No Resolution?
This shouldn't be considered an "Enhancement", but rather a "Critical Bug" & needs to get propagated to other versions of Bugzilla.

I see that the e-mail servers are hidden for those users not logged in.  This should be done for EVERYONE except certain administrators.

The problem then is the common e-mail servers (gmail, yahoo, hotmail, etc.).  It would be simple enough to harvest names and spam all those servers.

Perhaps consider different user privileges...

Super-Admin (able to see all names if this is necessary, perhaps for blocking accounts).
Normal-Admin (able to see names but not servers).
Everyone else (system generated obfuscation).  

Assign a specific obfuscation code to each account.  Or, as suggested above, perhaps obfuscate existing accounts, but force new users to select their desired username, and allow existing users that option too.

----- Clifford -----

Comment 51

[Neville Hillyer]

The world has changed since 2003.

I agree about the 'Importance' and think 'Severity' should be increased to 'major' on the basis that resulting spam could be detrimental. In addition to this I also request that those responsible increase 'Priority' so that 'Importance' gets raised very considerably.

Should another bug be started as we are now asking for far more than the original request? I tried this with https://bugzilla.mozilla.org/show_bug.cgi?id=578787 only to get it marked as a duplicate of this bug - very frustrating - what else can I do?

Comment 52

[Frédéric Buclin]

Filing new bugs won't help. They will simply be marked as duplicates. Complaining in this bug won't help either; developers are aware of this bug, as you can see with bugs blocking this one, and especially bug 425663.

If you *really* want to help, please provide patches. Nothing else would be as helpful as that.

Comment 53

[Neville Hillyer]

It is encouraging to see that there was considerable activity on 425663 up until about a year ago but a pity nothing appears to have happened since.

I would willingly provide patches if I had the skill/experience but I am an Electrical Engineer and not a software designer.

When do you think Bugzilla will have the same level of email security as MozillaZine?

Comment 54

[Denis Roy]

> If you *really* want to help, please provide patches. Nothing else would be
> as helpful as that.

In the case of bug 425663, I find it unfortunate, yet understandable, that the patch submitter abandoned their efforts. I think the Bugzilla dev team missed out on a great opportunity.

Comment 55

[Neville Hillyer]

I am unclear about the status of this bug. Can somebody explain it in simple terms?

Comment 57

[Thomas D. (:thomas8)]

Wow, 9 years after this anti-spam bug was filed, it is now confirmed "sec-want" - wanted for security! Looks like a step into the right direction :)

Comment 58

[Frédéric Buclin]

It's an enhancement bug, so not a blocker.

Comment 59

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 60

[Gervase Markham [:gerv]]

I intend to pick up the torch here. I recently posted the below to the Bugzilla developer group. Comments welcome on the design, which is pretty similar to what is proposed here.

<snip>

I have a need for a Bugzilla which implements email privacy, and I'd
like to write an upstreamable patch to do it. Can people please review
my design?

The last attempt by wurblzap died:
https://bugzilla.mozilla.org/show_bug.cgi?id=425663 [0]
on the grounds of what seems to me to be an incorrect objection
involving extern_id.

I plan to implement this:
https://wiki.mozilla.org/Bugzilla:Email_Change_Design
which follows what he did pretty closely.

It's not a long document; can people take 5 minutes to glance over it
and make sure the semantics are what they expect?

I'd much rather have feedback of that sort now than after I've taken the
effort to write/update the patch.

Gerv

[0] ...and the other dependencies of
    https://bugzilla.mozilla.org/show_bug.cgi?id=218917

Comment 61

[Gervase Markham [:gerv]]

Attachment: Gerv patch v.1

This is the first of a two-part patchset. It creates the new "email" column in the profiles table, with associated machinery for doing so, and then makes sure that it's always in sync with the "login_name" column.

The second patch in the set will teach all the different bits of Bugzilla about the "email" column, and then remove the restrictions which prevent the two from diverging (when the pref allows them to).

However, this patch is designed such that it should be possible to check it in first with no ill effects, and that is what I would hope to do, as a base for the second part of the work.

Gerv

Comment 62

[Simon Green]

Comment on attachment 8367947 [details] [diff] [review]
Gerv patch v.1

Sorry, I don't do upstream reviews any more. As per justdave's e-mail last week, this should be sent to Byron.

Comment 63

[:glob ✱]

Comment on attachment 8367947 [details] [diff] [review]
Gerv patch v.1

Review of attachment 8367947 [details] [diff] [review]:
-----------------------------------------------------------------

please provide a single unified patch rather than splitting it.
it's much easier to do the review/testing cycles, and we shouldn't commit a partially implemented feature.

here's a quick inspection-only review of this patch.

- this patch doesn't pass tests due to undocumented methods
- you need to update the webservices to accommodate this change

::: Bugzilla/Config/Common.pm
@@ +382,5 @@
> +    my $option = shift;
> +    if ($option) {
> +        Bugzilla->dbh->do('UPDATE profiles SET login_name = email');
> +    }
> +}

this sub needs a comment.
i suspect we should only be setting values where email is null, rather than always overwriting values when this param is set to true.
as use_email_as_login default to true, this needs to be executed when this param is added.

::: Bugzilla/DB.pm
@@ +2309,5 @@
>  C<Bugzilla::DB::Schema::ABSTRACT_SCHEMA>.
>  
>  =over
>  
> +=item C<bz_add_column>, C<bz_add_duplicate_column>

the docs here should explain the difference between these two

::: Bugzilla/User.pm
@@ +83,5 @@
>  use constant VALIDATORS => {
>      cryptpassword => \&_check_password,
>      disable_mail  => \&_check_disable_mail,
>      disabledtext  => \&_check_disabledtext,
>      login_name    => \&check_login_name,

check_login_name needs to be updated as it currently calls check_email_syntax

@@ +226,5 @@
> +        || ThrowUserError('illegal_email_address', { addr => $email });
> +
> +    # Check the email if it's a new user, or if we're changing the email.
> +    # XXX When it's possible to change email independently of login, this
> +    # check will be needed.

this patch enables changing the two independently, so this is confusing.

@@ +231,5 @@
> +    #f (!ref($invocant) || $invocant->email ne $email) {
> +    #   is_available_email($email)
> +    #        || ThrowUserError('account_exists', { email => $email });
> +    #}
> +

if use_email_as_login is true, we also need to call check_login_name

@@ +2275,5 @@
>  
>    # Class Functions
>    $user = Bugzilla::User->create({ 
>        login_name    => $username, 
> +      email         => $email, 

nit: remove trailing whitespace

::: template/en/default/admin/params/auth.html.tmpl
@@ +128,5 @@
> +                        "account. If switched off, they will log in with
> +                        a separate identifier.<br>" _
> +                        "<strong>Switching this from off to on results in " _ "the destructive act of the login " _
> +                        "name for all users being set to their email " _
> +                        "address!</strong>",

nit: the line lengths here are funky

Comment 64

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #63)
> please provide a single unified patch rather than splitting it.
> it's much easier to do the review/testing cycles, and we shouldn't commit a
> partially implemented feature.

Will do.

> - you need to update the webservices to accommodate this change

Can you be more specific? You mean, provide the email address on User.get? Or something else?

> ::: Bugzilla/Config/Common.pm
> @@ +382,5 @@
> > +    my $option = shift;
> > +    if ($option) {
> > +        Bugzilla->dbh->do('UPDATE profiles SET login_name = email');
> > +    }
> > +}
> 
> this sub needs a comment.
> i suspect we should only be setting values where email is null, rather than
> always overwriting values when this param is set to true.

I'm not quite sure what you mean - if email is null, then login_name will also become null! Do you mean "if login_name is null"? I don't think that's right either. If someone wants to turn on use_email_as_login, that requires that all login names become email addresses, which is what that code does.

> as use_email_as_login default to true, this needs to be executed when this
> param is added.

It defaults to false for new installations, and true for existing installations. But the new "email" column is created as a duplicate of the login_name column, so this code is 'run' anyway.

> ::: Bugzilla/DB.pm
> @@ +2309,5 @@
> >  C<Bugzilla::DB::Schema::ABSTRACT_SCHEMA>.
> >  
> >  =over
> >  
> > +=item C<bz_add_column>, C<bz_add_duplicate_column>
> 
> the docs here should explain the difference between these two

They do, further down in the description of these two methods. But I will add something at the top too.

> ::: Bugzilla/User.pm
> @@ +83,5 @@
> >  use constant VALIDATORS => {
> >      cryptpassword => \&_check_password,
> >      disable_mail  => \&_check_disable_mail,
> >      disabledtext  => \&_check_disabledtext,
> >      login_name    => \&check_login_name,
> 
> check_login_name needs to be updated as it currently calls check_email_syntax

I don't understand this comment. check_login_name() does not currently call check_email_syntax() in my patch.

> @@ +231,5 @@
> > +    #f (!ref($invocant) || $invocant->email ne $email) {
> > +    #   is_available_email($email)
> > +    #        || ThrowUserError('account_exists', { email => $email });
> > +    #}
> > +
> 
> if use_email_as_login is true, we also need to call check_login_name

Can you help me understand why?

We call the check...() function appropriate to whichever value or values is/are being updated. This is true whether or not use_email_as_login is true.

Gerv

Comment 65

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #64)
> > - you need to update the webservices to accommodate this change
> 
> You mean, provide the email address on User.get?

yes, and any other methods which returns a user.

for example, comments are created with the following in _translate_comment():
>  creator    => $self->type('email', $comment->author->login),
>  author     => $self->type('email', $comment->author->login),

to be honest i'm not sure what the right course of action is here.  ideally we should rename 'email' to 'login' in the response, but that's a breaking change.

> > i suspect we should only be setting values where email is null, rather than
> > always overwriting values when this param is set to true.
> 
> I'm not quite sure what you mean - if email is null, then login_name will
> also become null! Do you mean "if login_name is null"? I don't think that's
> right either. If someone wants to turn on use_email_as_login, that requires
> that all login names become email addresses, which is what that code does.

i was thinking about what happens if it's enabled, someone changes their login, then it's disabled and re-enabled.  in hindsight i'm probably worrying about nothing here; leave it as is.

> > check_login_name needs to be updated as it currently calls check_email_syntax
> 
> I don't understand this comment. check_login_name() does not currently call
> check_email_syntax() in my patch.

it does..

> sub check_login_name {
>     my ($invocant, $name) = @_;
>     $name = trim($name);
>     $name || ThrowUserError('user_login_required');
>     check_email_syntax($name);

> > if use_email_as_login is true, we also need to call check_login_name
> 
> Can you help me understand why?
> 
> We call the check...() function appropriate to whichever value or values
> is/are being updated. This is true whether or not use_email_as_login is true.

because the value provided is used to set both email and login fields, it needs to pass checks for both fields.

Comment 66

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Here's a second attempt. I've taken into account the feedback so far, run a test plan (and fixed the bugs I found :-), and done some greps to try and make sure we now have a clean split between login and email, with email used only when we are actually sending mail. The patch seems big, but there are lots of files with one or two small changes.

Some questions about the design and implementation also came up; they are:

* Currently, when email and login are split, login names can contain any character, including " " and @. Is this right and wise? If @ is allowed, someone could set their login name to someone else's email address. Or, if we check for that case, something looking very like someone else's email address. So I suggest we ban @. I also think things will be easier if we ban whitespace.

Banning @ also means that logins will pass unchanged through the email_filter, which simplifies the code.

We should make these bans on user-set only; if someone turns on email and login splitting, all of the logins start out with an @, but it's the user's real email, so that's OK. Or we could ban @ unless the value is identical to the user's email address.

* At the moment, when email and login are split, users can only login with the login value, not the email value. Is that the behaviour we want? I don't immediately see any reason why we couldn't allow them to log in with either.

* At the moment, even when login and email are split, email addresses are still revealed to logged in users (e.g. in the link at the top of each comment). Is it a design goal that, when login and email are split, email addresses are private to the user? This affects questions like:

   * When we do typedown address matching when login and email are split, do we 
     show and match on all three? Or just login and real name? Or just email 
     and real name?

   * Do we need to enhance the User.get() API call to allow matching on email 
     address?

* Do we want to prioritize exact matches on login name in an autocomplete list? So if someone types "gerv" or even, with special handling, ":gerv", should we put the person whose login name is "gerv" at the top of the list?

* How do I test the Web Service?

Gerv

Comment 67

[Gervase Markham [:gerv]]

BTW, this patch removes the "emailsuffix" parameter, because it is no longer useful for anything. One can split email and login and use shorter login names if one wants. The only hardship is having to type the full email address once, at account creation time.

Note that there is still a "RADIUS_email_suffix" parameter for RADIUS logins if it's needed. A similar param could be added for LDAP if LDAP needed it; at the moment, the 'mail' attribute is back to being required if the user is to have an email address.

Gerv

Comment 68

[:glob ✱]

after a few rounds of discussion regarding the overall approach to fixing this bug, we're going to take the following approach:

- remove the "login != email" parameter, instead always allow "login != email".
- the email address should appear only when creating an account, or when updating account details
- internally the login should be used for everything, with the email used only for email delivery.
    - this includes, but is not limited to: UI display, user matching, authentication
- at migration time create the email column and populate it with the current login
    - remove the emailsuffix as per current patch
- @ is accepted only if login == email, else it must be removed (enforced when using another email address)
    - eg. when changing your email, you must change your login to match
    - this restriction applies equally to users and admins
    - when a user changes their email address, the must change their login at the same time if it contains an @
      - implementation wise this probably means delaying the login change until the change-email token has been authorised   
- webservices shouldn't return the email, unless you belong to the editusers group, or are requesting details for your own account (for privacy reasons)

it should make for a much simpler patch, a less confusing experience for users and developers alike, and still accommodates sites who want to continue to use email addresses as logins.  a large difference is bugzilla will no longer be able to *enforce* login == email.

while i don't think there's value in this enforcement, if there's a massive outcry that this is required, i suggest its implementation be simply to require that a login contains an @, with no other UI or logic changes.


> Currently, when email and login are split, login names can contain any character,
> including " " and @.
@ i've covered above.  whitespace should definitely not be allowed in logins.
> How do I test the Web Service?
i write throw-away scripts to ensure all code branches are hit when working on web services
> this patch removes the "emailsuffix" parameter
excellent :)

Comment 69

[:glob ✱]

Comment on attachment 8377939 [details] [diff] [review]
Patch v.2

r- as per comment 68

Comment 70

[Gervase Markham [:gerv]]

I'm not sure the email thread consensus was to remove the parameter and always have login/email splits. justdave: can you confirm one way or the other?

In the split case, let's try and be absolutely clear about how these two fields will interact.

Terminology: If a user's login is the same as email, they are a "Samer". The other type of user is called a "Differ".

* Login name changes are not permitted while an email change is in progress. There are just too many permutations and complications, and it's not an onerous restriction.

* If a Samer tries to change their login to something without an @, that just works. They are now a Differ.
* If a Samer tries to change their login to something else with an @, that fails, and they are told to change their email address instead.
* If a Samer tries to change their email, it goes through the normal email change process. If, when the process completes with the "confirm" click from the email they got sent, login is the same as their *old* email, then their login name is also updated to be the same as their new email. (They are still a Samer.) 

* If a Differ tries to change their login to something else without an @, that just works.
* If a Differ tries to change their login to something with a @ which is not their email address, that fails, and they are told they can only use @ if their login is equal to their email address.
* If a Differ tries to change their login to their email, that just works. They are now a Samer.
* If a Differ tries to change their email, that just follows the normal process.

Is that right? glob?

Gerv

Comment 71

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #70)
> I'm not sure the email thread consensus was to remove the parameter and always
> have login/email splits.

my understanding from the discussion was a clear "yes, always split login and emails".
quoting justdave: "I'm in favor. Users have been begging for this for 15 years or so :-)"

the only outstanding question is if bugzilla should continue to support the enforcing login == email.  if we require that, i still think my suggestion in comment 68 to have a login_regex parameter would cover that requirement with minimal fuss.

> * Login name changes are not permitted while an email change is in progress.
> There are just too many permutations and complications, and it's not an
> onerous restriction.

agreed

> * If a Samer tries to change their login to something without an @, that
> just works. They are now a Differ.

yes

> * If a Samer tries to change their login to something else with an @, that
> fails, and they are told to change their email address instead.

not "instead", it should be "as well as".

> * If a Samer tries to change their email, it goes through the normal email
> change process. If, when the process completes with the "confirm" click from
> the email they got sent, login is the same as their *old* email, then their
> login name is also updated to be the same as their new email. (They are
> still a Samer.) 

no.  that should fail, and they are told to change their email address as well.

for any case where the login is changed, and it contains @, and it matches their email address, then embed that information into the token and send the confirmation email.  upon token validation change both their login and email.

> * If a Differ tries to change their login to something else without an @,
> that just works.

yes

> * If a Differ tries to change their login to something with a @ which is not
> their email address, that fails, and they are told they can only use @ if
> their login is equal to their email address.

yes.  there's no need to treat differs and samers differently.

> * If a Differ tries to change their login to their email, that just works.
> They are now a Samer.

yes.

> * If a Differ tries to change their email, that just follows the normal
> process.

yes

Comment 72

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #71)
> my understanding from the discussion was a clear "yes, always split login
> and emails".
> quoting justdave: "I'm in favor. Users have been begging for this for 15
> years or so :-)"

I'm fairly sure in context he meant he was in favour of the patch, and allowing users to split login and email, not in favour of forcing them to. Later in that thread, he says:

"Personally I lean towards letting the admins be flexible (param whether to enforce or not)".

> > * If a Samer tries to change their login to something else with an @, that
> > fails, and they are told to change their email address instead.
> 
> not "instead", it should be "as well as".

So you are saying that if a Samer wants to change their login and email from [e-mail] to [e-mail], they have to type "[e-mail]" into _both_ boxes, and hit Submit? And if they type it into only one box, they get an error telling them they have to type it into both boxes? That sounds to me like the sort of error which people send to the Daily WTF.

I think it's better to simply require them to type it into the email box, go through the email change process, then auto-update the login name to match when the process completes. There may be even better UIs than this (although none of them are as good as having just one box, but you can only do that if we keep the pref).

Gerv

Comment 73

[Dave Miller [:justdave]]

I can easily see situations where people would want to keep using email addresses for identifying people.  In a closed site where the public can't access anyway it might be easier to indentify people that way and they wouldn't have to worry about spammers scraping the site.  So I think a pref to allow it to be enforced would be a good thing.

One other thing I'm not sure has been covered here yet or not, is there any time restriction for how often you can change your login name?  I'd kind of annoyed if someone changed their login every 5 minutes to try to avoid getting found when the spammed or something.  They do that on IRC enough as it is ;)

Comment 74

[Gervase Markham [:gerv]]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #73)
> I can easily see situations where people would want to keep using email
> addresses for identifying people.  In a closed site where the public can't
> access anyway it might be easier to indentify people that way and they
> wouldn't have to worry about spammers scraping the site.  So I think a pref
> to allow it to be enforced would be a good thing.

OK.

> One other thing I'm not sure has been covered here yet or not, is there any
> time restriction for how often you can change your login name?  I'd kind of
> annoyed if someone changed their login every 5 minutes to try to avoid
> getting found when the spammed or something.  They do that on IRC enough as
> it is ;)

All restrictions are based on email address, not login name, so login name changes won't help you evade anti-spam. And no-one gets emailed when someone changes their login name. So I don't see this as a problem. It's just updating a field in a database. If that turns out to be wrong, we can implement a rate limit.

Gerv

Comment 75

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #68)
> - webservices shouldn't return the email, unless you belong to the editusers
> group, or are requesting details for your own account (for privacy reasons)

What should happen to the webservices_email_filter parameter? I assume it should it go away, in favour of the above unconditional behaviour?

Gerv

Comment 76

[Gervase Markham [:gerv]]

(In reply to Gervase Markham [:gerv] from comment #75)
> What should happen to the webservices_email_filter parameter? I assume it
> should it go away, in favour of the above unconditional behaviour?

With more detail: seems like we want to not return email addresses (unless you are in editusers) when email and login are split, as above, but we need to keep returning them when email == login, because API users need to be able to unambiguously refer to users when making e.g. write calls. So the param does go away.

Gerv

Comment 77

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #76)
> but we need to keep returning them when email == login

I disagree. Currently, email == login and we don't return the full email address. This behavior must stay.

Comment 78

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #77)
> I disagree. Currently, email == login and we don't return the full email
> address. This behavior must stay.

How can I make a Bugzilla client using the API, if the only references I can get to people (so I can assign bugs, etc.) are non-unique and partial?

Gerv

Comment 79

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #78)
> How can I make a Bugzilla client using the API, if the only references I can
> get to people (so I can assign bugs, etc.) are non-unique and partial?

You must be logged in to get the full login name. If you are logged out, it's intentionally truncated.

Comment 80

[Dave Miller [:justdave]]

Sounds to me like maybe the User ID number (rather than just the login name) needs to be made available in the API, and suggest to use that for bug change purposes by clients if they cache usernames locally instead of looking them up at runtime.

Comment 81

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #79)
> You must be logged in to get the full login name. If you are logged out,
> it's intentionally truncated.

But now, the plan is to not reveal email addresses unless the person is in editusers. So users can't get login names by logging in. So we will need to update the API to use userIDs instead, as justdave suggests.

I propose we do that as a separate follow-up bug, and (if login and email are not split) keep the WebService returning full email addresses for logged in users for now. It means we don't quite reach our privacy aims totally until we do the second patch, but we do reach them for normal web access.

Gerv

Comment 82

[Gervase Markham [:gerv]]

Note to self: update date in Bugzilla/Install/DB.pm before checkin.

Gerv

Comment 83

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Here's an updated patch.

This eliminates webservice_email_filter, in favour of giving email addresses on the WebService if either a) email == login (as then they are everywhere anyway) or b) the user is logged in. Later, once we've added a userID param to the relevant APIs and got clients to update, we can stop giving out email addresses almost entirely.

Gerv

Comment 84

[Gervase Markham [:gerv]]

Attachment: Patch v.4

This is the latest version of the patch, which has been through the test plan.

It now features special handling for "@" and ":" prefixes in user matching. Typing "@gerv" or ":gerv" will return an exact match for the user with login name "gerv". 

Gerv

Comment 85

[Gervase Markham [:gerv]]

Hi glob,

Any news on when you might be able to get to this?

Gerv

Comment 87

[Aeed]

Oh wow, reported in 2003, it's 2014, 11 years later, and this STILL isn't fixed. Are there any admins on this site at all?

Comment 88

[Rainer Rillke]

> STILL isn't fixed
AFAIK there is a patch that is pending review. But there are a couple of other issues, so Wikimedia Foundation will most likely and hopefully use Phabricator in the near future. Phabricator has support for oAuth and other neat features and is written in PHP, not perl. And it's a lot more social.

Comment 89

[Andre Klapper]

Rainer: I have no idea how infrastructure changes of random third-party Bugzilla customers or even programming language preferences are related to this _specific_ upstream issue. Sounds offtopic.

Aeed: Age is mostly irrelevant for a ticket. If you want to get it fixed earlier, use your time travel machine and provide a patch earlier. To check "admin activity" (whatever an "admin" should do here instead of developers), Bugzilla provides a query interface for recent bug status changes for a project, for example.

Comment 90

[Gervase Markham [:gerv]]

<polite cough> glob? :-)

Gerv

Comment 91

[Aeed]

Andre Klapper: I disagree, age is VERY relevant to such a huge privacy issue such as this. It gives a bad impression to users as well, suggesting "privacy is unimportant" or "irrelevant" as none of the Bugzilla or Mozilla devs care enough to have it fixed by now. As for suggesting I "provide a patch", how do you suggest I do so? I'm not an admin or developer on this website, and nor would I have the knowledge of how to patch it.

Comment 92

[Gervase Markham [:gerv]]

Put the handbags down, gentlemen. This bug already has a patch. It just needs review :-)

Gerv

Comment 93

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #92)
> Put the handbags down, gentlemen. 

i'm not sure that response is appropriate :|

> This bug already has a patch. It just needs review :-)

yup; i haven't been ignoring it and have been slowing biting away at this patch.  i apologise that it's taking so long.

Comment 94

[Gervase Markham [:gerv]]

Just to be clear: my comment about handbags was a Britishism, as follows:
http://www.urbandictionary.com/define.php?term=handbags

No insult was intended to anyone :-)

Gerv

Comment 95

[Anje]

This is an old bug dating back some years and I'm not certain if I should create a new bug report.
I file Thunderbird bugs on bugzilla.
My email address is clearly visible to all who read any bug or comment I make.
REcently, some people were constantly adding derogatory chat to a bug due to their frustration.
After asking them to only post useful information, I received a personal email from one person to my email address, that was clearly designed to be annoying and offensive.

Please can non-developer email addresses not be exposed to other non-developers.
Please can rights privieges be created so that only developers see the current view and non-developers see the same except no email addresses of people who are not developers, assuming the developers agree to keep their email addresses visible.

Comment 96

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Unbitrotted patch against today's trunk. Hope that helps :-)

Gerv

Comment 97

[Gervase Markham [:gerv]]

glob: any progress here? I know this patch isn't small, but it's very useful function, and it's been many months now. It would be sad to see all the work done here fade away into bitrot for a second time.

Gerv

Comment 98

[Frédéric Buclin]

pfff, retarget the review request to me, if glob is really too busy. I will try to find some time to review it.

Comment 99

[Gervase Markham [:gerv]]

Comment on attachment 8526836 [details] [diff] [review]
Patch v.5

Thank you very much indeed :-)

Gerv

Comment 100

[Frédéric Buclin]

Comment on attachment 8526836 [details] [diff] [review]
Patch v.5

I cannot test this patch, because it's bitrotten. :(

Comment 101

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #100)
> I cannot test this patch, because it's bitrotten. :(

I've already unbitrotted it once... I don't suppose you could just roll back your install by git checkout-ing an earlier version to apply the patch to, just for your initial testing?

Once you've done a first round of review comments, I will of course have to unbitrot it then in order to fix them, but doing it this way at least reduces the number of times I have to load the large amount of mental context this patch requires back into my head.

Gerv

Comment 102

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #101)
> I don't suppose you could just roll back
> your install by git checkout-ing an earlier version to apply the patch to,
> just for your initial testing?

And I am supposed to guess when to come back in time? I don't even know how to do that with git. Maybe with a TARDIS...

Comment 103

[Matt McHenry]

'git log origin/master --until 2014-11-21 -1 --date-order' should show you the commit that was current on origin/master as of the date that the patch was attached.  Then you can get it your TARDIS with 'git checkout -b review-bug-218917 <that-commit-sha1>'.  :)

Comment 104

[Gervase Markham [:gerv]]

LpSolit: given the information in comment 103, are you able to give this patch its first review?

If you do want to require me to refresh it, can you give me some idea when you will have time for the review, so I can make sure the patch is ready at the right moment, and it's less likely to bitrot?

Gerv

Comment 105

[Matt McHenry]

In comment 103 I was assuming you'd made this patch off of master.  Looks like in fact you made it off of the 5.0 branch.  'git log --graph --oneline --all --date-order --until 2014-11-21 -20' gave me a set of candidate commits to try applying on top of, and tags/bugzilla-5.0rc1~34 (aka 1fc447ad3467f47d773a9ca6e9caf31e7867f77d) was the one that worked.  From there I found that tags/bugzilla-5.0rc1~29 (aka 8ebd893eb2be496c6afea66548073267b8f405c8) is the newest commit that I can rebase onto cleanly.  So the commands you want for an initial review are:

> git checkout -b review-bug-218917 1fc447ad3467f47d773a9ca6e9caf31e7867f77d
> git branch -u origin/5.0
> git apply <this-patch>
> git commit -am 'patch to review'
> git rebase --onto 8ebd893eb2be496c6afea66548073267b8f405c8

Comment 106

[Gervase Markham [:gerv]]

(In reply to Matt McHenry from comment #105)
> In comment 103 I was assuming you'd made this patch off of master.  Looks
> like in fact you made it off of the 5.0 branch. 

Did I? OK, I agree that's not ideal. I'll get my head back into it, and rebase it one more time :-)

Gerv

Comment 107

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Rebased patch onto today's trunk. Let me know if you need anything else for the review :-)

Gerv

Comment 108

[Frédéric Buclin]

Comment on attachment 8602072 [details] [diff] [review]
Patch v.6

>diff --git a/Bugzilla/API/1_0/Util.pm

> sub as_email    {
>     defined $_[0]
>-        ? ( Bugzilla->params->{webservice_email_filter} ? email_filter($_[0]) : $_[0] . '' )
>+        ? ( Bugzilla->params->{use_email_as_login} ? $_[0] . '' : email_filter($_[0]) )
>         : JSON::null
> }
> =head2 as_email
> 
>+Takes an email address as a parameter, but redacts it if C<use_email_as_login> is
>+disabled in the system settings (and so email addresses are supposed to be
>+private). If parameter is undefined, returns JSON::null.

I disagree with this. If we disclose a part of the email address, it's way too easy to guess the remaining part (start with @gmail.com, then try @outlook.com, etc...). If we pretend to guarantee email privacy, then email addresses shouldn't be disclosed at all, even partially. I think we agreed that email addresses were used to send emails, and only for that purpose. So this as_email method should die entirely, and either the admin or power user is allowed to see the full email address, or he isn't and in this case the email address shouldn't be disclosed at all.



>diff --git a/Bugzilla/Auth/Verify.pm

>+    my $email     = $params->{email};
>     my $username  = $params->{bz_username} || $params->{username};
>     my $password  = $params->{password} || '*';
>     my $real_name = $params->{realname} || '';

This is becoming confusing. bz_username and username should be renamed login, because it's way too easy to mix username and realname.


>+    if ($email && Bugzilla->params->{use_email_as_login}) {
>+        $username = $email;
>+    }

Err... if use_email_as_login = 1 and $email is not defined, we should return an error code, not silently ignore it.



>diff --git a/Bugzilla/Auth/Verify/RADIUS.pm

>+    $params->{extern_id}   = $username;

Why is extern_id set here, as it wasn't previously?



>diff --git a/Bugzilla/Config.pm

>+sub call_param_onchange_handlers {
>+    my ($changes) = @_;
>+
>+    _load_params unless %params;
>+
>+    foreach my $name (@$changes) {
>+        my $entry = $params{$name};

I think $param would be clearer than $entry.


>+        if (exists $entry->{'onchange'}) {
>+            $entry->{'onchange'}->(Bugzilla->params->{$name}, $entry);
>+        }

Is there a real need for the 2nd argument here? It seems you just cloned 'checker', but there is a real need for a 2nd argument for checkers. Also, for consistency, please also use 'onchange' for the duplicate_or_move_bug_status parameter and remove its corresponding code from editparams.cgi.


>+    # Set the value of use_email_as_login to off for new installs.
>+    # It has to default to on, because that's what the situation was before
>+    # the setting was invented.
>+    $param->{'use_email_as_login'} = 0 if $new_install;

The comment is useless. What this line does is pretty explicit.


>+  call_param_onchange_handlers(@changes);

This method takes an arrayref, not an array -> \@changes.


>+=item C<call_param_onchange_handlers(@changes)>

Same here.


>+Params:  C<@changes> (required) - A list of parameter names.

And here.



>diff --git a/Bugzilla/Config/Common.pm

>+# checker  - (optional) checking function for validating parameter entry

Nit: add a . at the end of sentences, for readability.


>+# onchange - (optional) handling function for parameter changes
>+#            It is called with the value of the param as the first arg
>+#            and a reference to the param's hash as the second argument

Same here. Also, as I said above, we don't need the 2nd argument.


>+=item C<change_use_email_as_login>
>+
>+Change handler for "use_email_as_login" preference - if pref is changed to

It's not a preference, it's a parameter.



>diff --git a/Bugzilla/DB.pm

>-sub bz_add_column {
>+sub _bz_add_column_raw {

I'm really not a fan to play with internals of DB code, especially for something which is used only once (has this code been tested with MySQL, PostgreSQL, SQLite *and* Oracle? I think you didn't). You don't need these changes. In Install/DB.pm, a "create new column" followed by a "fill the column" is enough. The UNIQUE index is created as a separate action anyway, so this is fine.



>diff --git a/Bugzilla/Hook.pm

>+The login of the new account. This will be the same as the email address if
>+you have set the "use_email_as_login" parameter, otherwise it may be different.

Who is "you" here? Because the one writing an extension may not be the admin who set the param.



>diff --git a/Bugzilla/Install.pm

>+    if (!$login || !$email || !$password || !$full_name) {
>         say "\n" . get_text('install_admin_setup') . "\n";
>     }

You shouldn't check $login if 'use_email_as_login' is on, because the admin won't be able to enter it.
Maybe this should be done in a separate bug, but I would also drop $full_name from the check. I think it's idiotic to *force* an admin to type his real name at this stage, and having to fill 4 fields to create an admin account is painful. He can enter it from the web UI once Bugzilla is set up.



>diff --git a/Bugzilla/Install/DB.pm

>+    $dbh->bz_add_duplicate_column('profiles', 'email',
>+                                  {TYPE => 'varchar(255)', NOTNULL => 1},

As I said above, simply call bz_add_column() as usual, and then call
  $dbh->do('UPDATE profiles SET email = login_name').

Also, move all this code into its own subroutine. This is not the right place to do this.



>diff --git a/Bugzilla/Token.pm

>+                AND ' . $dbh->sql_regexp('eventdata', '?', 0, $dbh->quote($regexp)) . '

Must $regexp really be quoted? Also, I don't understand why you don't pass $regexp directly instead of '?'.



>diff --git a/Bugzilla/User.pm

>sub update {

>-        # Delete all the tokens related to the userid
>-        $dbh->do('DELETE FROM tokens WHERE userid = ?', undef, $self->id)
>+        # If we changed the email, silently delete any tokens except the one
>+        # to restore the original email.
>+        $dbh->do(q{DELETE FROM tokens
>+                    WHERE tokentype != 'emailold'
>+                      AND userid = ?},
>+                 undef, $self->id)

Err... what's this change? Keeping old tokens has security implications, and this has nothing to do with this bug.


># This is public since createaccount.cgi needs to use it before issuing
># a token for account creation.

This comment should go away for both methods. It's not accurate as createaccount.cgi calls $user->check_and_send_account_creation_confirmation().


> sub check_login_name {
>+    $login = trim($login);

To prevent any abuse, I would rather call clean_text().


>+    # Any login name is valid; we use it in placeholders only
>+    trick_taint($login);

That is not true. For instance, "@" is not allowed in login names, to prevent confusion with email addresses.


>     # Check the name if it's a new user, or if we're changing the name.

s/name/login name/g for clarity (to me, name == realname).


>+    if (!ref($invocant) || lc($invocant->login) ne lc($login)) {
>+        is_available_username($login)

is_available_username() is a pretty useless method. You can directly call login_to_id(). This method does nothing else:

  ThrowUserError() if login_to_id();


>+sub check_email {

>+    validate_email_syntax($email)
>+        || ThrowUserError('illegal_email_address', { addr => $email });

This is exactly what check_email_syntax() does.


>+    if (!ref($invocant) || $invocant->email ne $email) {

The comparison must be lowercase, else you could create the same account several times using the same email address (PostgreSQL is case-sensitive).


>+        is_available_email($email)
>+            || ThrowUserError('account_exists', { email => $email });

That's not how the email change code works. The original code passes both the old and new email addresses on purpose.


>+sub set_email {
>+    my ($self, $email) = @_;
>+    $self->set('email', $email);
>+    $self->set_login($email) if Bugzilla->params->{'use_email_as_login'};

For this code to work, you must set VALIDATOR_DEPENDENCIES so that it's not possible for set_all() to override the login name if the param is turned on. Also, don't call set_login() itself, but call ->set('login_name') directly. See how set_disabledtext() does it.


> sub nick {

>+        if (Bugzilla->params->{use_email_as_login}) {
>+            $self->{nick} = (split(/@/, $self->email, 2))[0];
>+        }
>+        else {
>+            $self->{nick} = $self->login;
>+        }

Currently, "nick" returns the first part of [e-mail], i.e. "foo". But you can have login == email even when use_email_as_login is turned off, so nick would contain the full email address. In that case, I think we still want to return "foo" instead of "[e-mail]". I think you should ignore the use_email_as_login parameter and always set nick = (split(/@/, $self->login, 2))[0].

>-        $query .= "WHERE ("
>-            . $dbh->sql_istrcmp('login_name', '?', "LIKE") . " OR " .
>-              $dbh->sql_istrcmp('realname', '?', "LIKE") . ") ";
>+        $query .= "WHERE (" .
>+              $dbh->sql_istrcmp('login_name', '?', "LIKE") . " OR " .
>+              $dbh->sql_istrcmp('realname',   '?', "LIKE") . ") ";

Let's not change these lines as this brings no value. Your patch is already large enough, and when looking at the file history, these lines will look like you really edited them, which is confusing.


>+        # Special handling for "@" and ":" prefixes. We strip them off
>+        # before matching, so ":gerv" or "@gerv" exact-matches user "gerv"
>+        # and that user will be the only result returned.

No! First of all, this a bmo-specific hack. Secondly, @ is not allowed in login names, and : is perfectly valid character in login names.


> sub is_available_username {
>+    my ($username) = @_;
> 
>     if(login_to_id($username) != 0) {
>         return 0;
>     }
> 
>+    return 1;
>+}

As I said above, this method is useless. It can be summarized to:

  sub is_available_username { return !login_to_id($_[0]) }

So let's use login_to_id() directly where we need it. And "username" is confusing as it sounds like "realname". is_available_login() would have been a better name anyway.


>+sub is_available_email {

>+    if(email_to_id($email) != 0) {
>+        return 0;
>+    }

Let's do some cleanup, and write: return 0 if email_to_id($email);


> sub check_and_send_account_creation_confirmation {
>-    my ($self, $login) = @_;
>+    my ($self, $login, $email) = @_;

I don't think we should require a login at that point. The important part of the process is to make sure the email address belongs to the user. We should ask for a login at the same time as the password. Also, the UI should allow the user to not enter any login, in which case the email address will be used as the default login. This will also let you remove several of the changes you made here.


> sub login_to_id {

>     } elsif ($throw_error) {
>         ThrowUserError('invalid_username', { name => $login });

Please rename this error code to 'invalid_login_name' for clarity.


>+sub email_to_id {

It's mostly a clone of login_to_id(). It would be good to refactor the code a bit to avoid all these clones when possible.


>+    # No need to validate $email -- it will be used by the following SELECT
>+    # statement only, so it's safe to simply trick_taint.
>+    trick_taint($email);

The comment can go away. I always considered such comments as useless because the code is obvious enough. And if a developer cannot understand this without a comment, he shouldn't touch this code.


>+    elsif ($throw_error) {
>+        ThrowUserError('invalid_email', { addr => $email });

s/addr/email/.


> =item C<login>
> 
>-Returns the login name for this user.
>+Returns the login name (nick) for this user.

login != nick, see above. So please do not mention "nick" here. $user->nick and $user->login are not the same thing.


> =item C<nick>
>+Usually, this is an alias for C<login>.
>+If email addresses are used as logins, though, then this is the part of the
>+user's email address before the at sign (@).

Make it clear that nicks are not unique, as in the original code, and that it shouldn't be used to authenticate a user. This raises a big concern to me. Now that we cannot see other users email address anymore, we have no way to make sure that "gerv" is really gerv, or maybe it's ":gerv"? Or "gerv|mozilla"? Someone could easily try to abuse the system this way to be CC'ed to security bugs. And it's not an option to refuses someone else's login because it's a substring or a superstring (does this word exist?) of an existing login name. Being able to see the email address permits to authenticate a user for sure, because a user cannot impersonate/spoof someone else's email address, especially for corporate domain names (such as @mozilla.com). If I need to CC someone on a security bug, how do I distinguish who is who with realname + login name only? This question is a showstopper before getting any r+ from me.


> =item C<create>

> Params: login_name - B<Required> The login name for the new user.

The login name shouldn't be mandatory. If missing, it should fallback to the email address, which can always be used as login name, independently of the use_email_as_login parameter.


>-=item C<check_and_send_account_creation_confirmation($login)>
>+=item C<check_and_send_account_creation_confirmation($login, $email)>

$login not required at this point, see above.


> =item C<is_available_username>

Useless method, see above.



>diff --git a/Bugzilla/Util.pm

> sub check_email_syntax {
>     my ($addr) = @_;
> 
>     unless (validate_email_syntax(@_)) {
>-        my $email = $addr . Bugzilla->params->{'emailsuffix'};
>-        ThrowUserError('illegal_email_address', { addr => $email });
>+        ThrowUserError('illegal_email_address', { addr => $addr });
>     }
> }

For consistency with validate_email_syntax(), let's use $email instead of $addr.



>diff --git a/Bugzilla/WebService/Bug.pm

>-        creator       => $self->type('email', $comment->author->login),
>+        creator       => $self->type('login', $comment->author->login),

Err... 'login' is not a valid type for WS, right? It should be 'string' instead. There are many places where you did this change. On the other hand, all existing installations will use their email address as login when upgrading to Bugzilla 6.0, and it will take a long time before most users change their login to something else. This doesn't mean we should expose their email address to everone. So IMO, we should still filter login names to remove everything after @ if present in the login name and the caller hasn't enough privs to see the full login name (e.g. because he isn't logged in/authenticated).



>diff --git a/Bugzilla/WebService/Server/JSONRPC.pm

>+    elsif ($type eq 'email') {
>+        # If we use_email_as_login, we assume email (and login) exposure is
>+        # OK, because it's used in the UI. If we aren't, then we hide email
>+        # addresses completely unless you are logged in. Eventually, we will
>+        # hide them unless the user is in editusers, but that requires giving
>+        # API users another way to unambiguously refer to users.
>+        if (!Bugzilla->params->{'use_email_as_login'}
>+            && !Bugzilla->user->id)
>+        {
>+            $retval = "";
>+        }
>     }

Wow, this is totally confusing. Now either the user is powerful enough to be allowed to get the email address back, or he isn't and that's the job of the caller to not return this data. Maybe the 'email' type should be renamed 'login' or something similar, and real email fields do not need any special type anymore, because only admins should be allowed to get them, and in special methods only.



>diff --git a/Bugzilla/WebService/Server/XMLRPC.pm

>         elsif ($type eq 'email') {
>             $type = 'string';
>+            $value = email_filter($value);
>         }

Same confusion and comment here.



>diff --git a/Bugzilla/WebService/User.pm

>-    # Username and password params are required 
>+    # Username and password params are required
>     foreach my $param ("login", "password") {

s/Username/login/. I don't know why we use two distinct words to mean the same thing (username vs login).


> sub offer_account_by_email {
>     my $self = shift;
>     my ($params) = @_;
>     my $email = trim($params->{email})
>         || ThrowCodeError('param_required', { param => 'email' });
>+    my $login;
>+
>+    if (Bugzilla->params->{use_email_as_login}) {
>+        $login = $email;
>+    }
>+    else {
>+        $login = trim($params->{login})
>+            || ThrowCodeError('param_required', { param => 'login' });
>+    }

It's not the right time to ask for a login, as said above. It's much better to ask for it when choosing a password.


> sub create {

>+        $login = trim($params->{login})
>+            || ThrowCodeError('param_required', { param => 'login' });

Same here. It shouldn't be mandatory. Just fallback to the email address if missing.



>diff --git a/buglist.cgi

> my @bugowners = keys %$bugowners;
>+if (scalar(@bugowners) > 1
>+    && $user->in_group('editbugs')
>+    && Bugzilla->params->{'use_email_as_login'})
>+{
>     my $bugowners = join(",", @bugowners);
>     $vars->{'bugowners'} = $bugowners;
> }

Do we really want to keep this feature? Anyway, you could merge the last two lines into a single:

  $vars->{'bugowners'} = join(",", @bugowners);



>diff --git a/createaccount.cgi

> my $login = $cgi->param('login');
>+my $email = $cgi->param('email');
>+$login = $email if Bugzilla->params->{'use_email_as_login'};

Only ask for the email address as we do currently. The login can be chosen at the same time as the password.



>diff --git a/docs/en/rst/administering/users.rst

>+- *Login Name*:
>+  This is the user's login name, if it is different from their email
>+  address (i.e. if the "use_email_as_login" parameter is switched off).

It can be the same as the email address even when the parameter is off.



>diff --git a/editusers.cgi

>+    my $login_name = scalar $cgi->param('login');
>+    my $email      = scalar $cgi->param('email');

No need to prepend "scalar" in scalar context. This only makes sense in list context as in hashes.


>+    $login_name = $email if Bugzilla->params->{use_email_as_login};

There is no need to do this here. This is the job of validators.



>diff --git a/template/en/default/account/auth/login-small.html.tmpl

>-        type="email" placeholder="Email Address"
>-      [% END %]>
>+        placeholder="Login">

Keep type="email" if use_email_as_login is true.


>-        type="email" placeholder="Your Email Address"
>-      [% END %]>
>+           placeholder="Login">

Same here.



>diff --git a/template/en/default/account/create.html.tmpl


>+  [% IF !Param('use_email_as_login') %]
>+    a login name of your choice and
>+  [% END %]
>   a legitimate email address.

As discussed above, that's not the right time to ask for a login name.



>diff --git a/template/en/default/account/email/confirm-new.html.tmpl

>+    [% IF !Param('use_email_as_login') %]
>+      <tr>
>+        <th>Login:</th>
>+        <td>[% login FILTER html %]</td>
>+      </tr>
>+    [% END %]

Make this field editable so that the user can specify his login now.



>diff --git a/template/en/default/account/email/request-new.txt.tmpl

>+  # login: login name of the new account.

Can go away.



>diff --git a/template/en/default/account/prefs/account.html.tmpl

> [%# INTERFACE:
>+  # user:              Bugzilla::User object. The user viewing this page.

No need to mention this. 'user' is not passed by the caller but is set in the Template::Toolkit object itself.


>       <td colspan="3">
>+        Please enter your current password to confirm any changes in this section.
>       </td>

This table has two columns, not three (existing bug). Please write colspan="2".


>+        <span style="font-size: 80%">
>+          (Leave empty to keep current password)
>+        </span>

No hardcoded style, please. Use class="bz_info" instead, which does exactly that.


>+      <th align="right">Your login name (or &ldquo;nick&rdquo;):</th>

No align="foo"! This attribute is obsolete in HTML5. Labels are already right-aligned by default. You don't need to specify anything. Also, don't write "nick" here, as login != nick internally. No need to confuse admins/developers. And while you are on it, please use <label> as that's exactly what they are.


>+          <input size="35" name="new_login" value="[% user.login_name FILTER html %]">

Use user.login, not user.login_name ($user->login_name doesn't exist, and trying to access $user->{login_name} directly is bad).


>+        [% ELSE %]
>+          [% user.login_name FILTER html %]

Same here.


>+          <input size="35" name="new_email" value="[% user.email FILTER html %]">

Please restore type="email" if use_email_as_login = 1. Also, do not delete IDs defined in form fields. You are breaking all Selenium tests due to that. It also makes customization much harder when fields do not have an ID.

Does it make sense to display this field when authenticating with LDAP or RADIUS or ENV?


>+      <input size="35" name="realname" value="[% user.realname FILTER html %]">

Use user.name, not user.realname (same reason as above).



>diff --git a/template/en/default/account/request-new-password.html.tmpl

>     If you have an account, but have forgotten your password, enter your
>+    [% IF Param('use_email_as_login') %]
>       email address
>+    [% ELSE %]
>+      login name
>     [% END %]

We should make no difference between both cases. You should request the *email address* in all cases. What is the user supposed to do if he forgot his login name? While testing your patch, I changed my login name, but then I couldn't remember which login name I chose. I had to access MySQL directly to find it and to be able to log in again. What I can easily remember is my email address, not my login. So we should also let the user edit his login name in case he forgot it, not only his password.



>diff --git a/template/en/default/admin/params/auth.html.tmpl

>+  use_email_as_login => "If switched on, users will log in with the email " _
>+                        "address of the " _
>+                        "account. If switched off, they will log in with
>+                        a separate identifier.<br>" _
>+                        "<strong>Switching this from off to on results in " _
>+                        "the destructive act of the login " _
>+                        "name for all users being set to their email " _
>+                        "address!</strong>",

The length of these lines is pretty inconsistent. Please fix that.



>diff --git a/template/en/default/admin/params/ldap.html.tmpl

>   LDAPmailattribute => "The name of the attribute of a user in your " _
>                        "directory that contains the email address, to be " _
>-                       "used as Bugzilla username. If this parameter " _
>-                       "is empty, Bugzilla will use the LDAP username"_
>-                       " as the Bugzilla username. You may also want" _
>-                       " to set the \"emailsuffix\" parameter, in this case.",
>+                       "used to send the user bugmail.",

As I understand it, this parameter cannot be left empty, right? If true, then you must enforce this when validating this parameter.
Also, Bugzilla does more than just sending bugmails.



>diff --git a/template/en/default/admin/users/list.html.tmpl

>+   {name        => 'view_history_link',
>+    heading     => 'Account History'

>+   {name        => 'action_links',
>+    heading     => 'Action'

For consistency, the 2nd name should be 'action_link' (singular).



>diff --git a/template/en/default/admin/users/userdata.html.tmpl

>+[% UNLESS Param('use_email_as_login') %]
>+  <tr>
>+    <th><label for="login">Login:</label></th>
>+    <td>
>+      [% IF editusers %]
>+        <input size="64" maxlength="255" id="login" name="login"
>+               value="[% otheruser.login FILTER html %]"
>+               [%- " autofocus" UNLESS editform %] required>

You cannot have 2 autofocus in the same HTML page (there is already one for the email field). The web browser cannot give the focus to both fields at once.


>+  <th><label for="email">
>+    Email address
>+    [% ' (serves as login name, too)' IF Param('use_email_as_login') %]:

  Be less verbose: just "(login)", else the label wraps badly.


>+  </label></th>

Please fix the indentation correctly if you split the label on several lines, for readability:

  <th>
    <label for...>
      Text
    </label>
  </th>


I found several bugs here. The admin is allowed to inject @ in login names, even when the login name doesn't match the email address. This should be forbidden, even for admins. Whitespaces should be forbidden too. Either the validator is not called, or the validator is not doing all required checks correctly.



>diff --git a/template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "email_required" %]

>+  [% ELSIF error == "email_required" %]

Duplicated error code.



>diff --git a/template/en/default/global/user.html.tmpl

>+    [% IF Param("use_email_as_login") %]
>+        [% IF user.id %]
>+          <a class="email" href="mailto:[% who.login FILTER html %]"

What we really want here is the user email address, so use who.email (even if it's the same as who.login, it doesn't make sense and is confusing to still point to who.login here). Also, fix the indentation (2 characters in templates, not 4).



>diff --git a/template/en/default/reports/delete-series.html.tmpl

>+    This series has been created by
>+    [% series.creator.login FILTER html %]

Keep this on a single line, else no whitespace will be added after "by".



>diff --git a/token.cgi


You forgot to fix one place which still uses emailsuffix.


Also, you must fix sanitycheck.cgi to correctly look at the new email field. It currently throws errors about all user accounts because it thinks login_name must still be a valid email address.


>-    check_email_syntax($login_name);
>+    login_to_id($login_name, 'throw_error_if_not_exist');

Please revert that. As said above, it's safer to ask for the user email address than login name.


> sub cancel_create_account {
>-    my ($login_name, $token) = @_;
>+    my ($event_data, $token) = @_;
> 
>     $vars->{'message'} = 'account_creation_canceled';
>-    $vars->{'account'} = $login_name;
>+    ($vars->{'email'}, $vars->{'login'}) = split(':', $event_data);

Please revert these changes too.



>diff --git a/userprefs.cgi

>+    my $new_login = trim($cgi->param('new_login'));
>+    my $new_email = trim($cgi->param('new_email'));

Please call clean_text() instead of trim(), for security purposes.


>+    # This is used only if email and login are separate
>+    if ($user->authorizer->can_change_login
>+        && $new_login
>+        && $user->login ne $new_login)

This is not enough. One could easily bypass the use_email_as_login parameter.


>+        if ($new_login =~ /@/ &&
>+            $new_login ne $user->email)
>+        {
>+            ThrowUserError("login_at_sign_disallowed");
>+        }
>+
>+        if ($new_login =~ /\s/) {
>+            ThrowUserError("login_illegal_character");
>+        }

This is not the right place to do sanity checks. These checks must be in check_login().


>+        if (Bugzilla::Token::HasEmailChangeToken($user->id)) {
>+            ThrowUserError("login_change_during_email_change");
>+        }

No reason to depend on a pending email change, AFAIK.


>+        is_available_username($new_login)
>+                   || ThrowUserError("account_exists", {login => $new_login});

This check is already done by check_login(), which is called by set_login().


>+        # Before changing an email address, confirm one does not exist.
>+        check_email_syntax($new_email);
> 
>+        is_available_email($new_email)
>+                   || ThrowUserError("account_exists", {email => $new_email});

Call check_email(), which does exactly all that. In fact, you should call $user->set_name instead of all this.

Comment 109

[Gervase Markham [:gerv]]

Thanks for the review! I realise it must have taken some time. As did fixing the issues :-)

There's one outstanding issue (the use of "login" as a type name on the WebService) which I am still looking at. But I hope you will be able to respond to the following meantime :-)

(In reply to Frédéric Buclin from comment #108)
> Comment on attachment 8602072 [details] [diff] [review]
> 218917.diff
> 
> >diff --git a/Bugzilla/API/1_0/Util.pm
> 
> > sub as_email    {
> >     defined $_[0]
> >-        ? ( Bugzilla->params->{webservice_email_filter} ? email_filter($_[0]) : $_[0] . '' )
> >+        ? ( Bugzilla->params->{use_email_as_login} ? $_[0] . '' : email_filter($_[0]) )
> >         : JSON::null
> > }
> > =head2 as_email
> > 
> >+Takes an email address as a parameter, but redacts it if C<use_email_as_login> is
> >+disabled in the system settings (and so email addresses are supposed to be
> >+private). If parameter is undefined, returns JSON::null.
> 
> I disagree with this. If we disclose a part of the email address, it's way
> too easy to guess the remaining part (start with @gmail.com, then try
> @outlook.com, etc...). If we pretend to guarantee email privacy, then email
> addresses shouldn't be disclosed at all, even partially.

At the moment, localparts of email addresses are used as identifiers in the GUI when the accessor is not logged in, if a person in question has not set a real name. On the Web Service, they are provided in full if webservice_email_filter is not set, or in part if webservice_email_filter is set. The above code change is an analogous continuation of this practice. I believe the logic was that no-one is going to write a specific webservices client in order to get and spam Bugzilla email addresses. Regardless, I'd like to avoid getting a potential change in policy here tangled up with this change, which is already complex enough.

> >diff --git a/Bugzilla/Auth/Verify.pm
> 
> >+    my $email     = $params->{email};
> >     my $username  = $params->{bz_username} || $params->{username};
> >     my $password  = $params->{password} || '*';
> >     my $real_name = $params->{realname} || '';
> 
> This is becoming confusing. bz_username and username should be renamed
> login, because it's way too easy to mix username and realname.

I've renamed this within the file, although it makes the patch much more noisy. Changing the external interface would be still more intrusive; if you want that done, can you file a follow-up bug?

> >+    if ($email && Bugzilla->params->{use_email_as_login}) {
> >+        $username = $email;
> >+    }
> 
> Err... if use_email_as_login = 1 and $email is not defined, we should return
> an error code, not silently ignore it.

I think you are misreading this code. It says "If we are using email addresses as logins, then don't use the passed-in username as the account identifier, use the passed-in email address. It is not error-handling code.

> >diff --git a/Bugzilla/Install.pm
> 
> >+    if (!$login || !$email || !$password || !$full_name) {
> >         say "\n" . get_text('install_admin_setup') . "\n";
> >     }
> 
> You shouldn't check $login if 'use_email_as_login' is on, because the admin
> won't be able to enter it.

This is for the non-interactive install, so he can - it just goes in a file. However, you are right that it makes little sense for it to be required. So I've updated the code.

> Maybe this should be done in a separate bug, but I would also drop
> $full_name from the check. I think it's idiotic to *force* an admin to type
> his real name at this stage, and having to fill 4 fields to create an admin
> account is painful. He can enter it from the web UI once Bugzilla is set up.

This is only for the non-interactive install; it's not currently required for the interactive install. Nevertheless, I have removed the requirement for the non-interactive install also.

> >diff --git a/Bugzilla/Token.pm
> 
> >+                AND ' . $dbh->sql_regexp('eventdata', '?', 0, $dbh->quote($regexp)) . '
> 
> Must $regexp really be quoted? 

It seems so, looking at the examples in the MySQL manual:
https://dev.mysql.com/doc/refman/5.1/en/regexp.html

(If quoting were needed for some DBs and not others, I would expect the sql_regexp function in MySQL.pm to do the quoting.)

> Also, I don't understand why you don't pass
> $regexp directly instead of '?'.

I don't understand why sql_regexp has a "regexp" and a "real_regexp" parameter... Still, changed.

> >diff --git a/Bugzilla/User.pm
> 
> >sub update {
> 
> >-        # Delete all the tokens related to the userid
> >-        $dbh->do('DELETE FROM tokens WHERE userid = ?', undef, $self->id)
> >+        # If we changed the email, silently delete any tokens except the one
> >+        # to restore the original email.
> >+        $dbh->do(q{DELETE FROM tokens
> >+                    WHERE tokentype != 'emailold'
> >+                      AND userid = ?},
> >+                 undef, $self->id)
> 
> Err... what's this change? Keeping old tokens has security implications, and
> this has nothing to do with this bug.

It does have something to do with this bug. We used to delete all tokens if the login_name changed. Now, we have a split concept. People (unless it's administratively forbidden) can change their login_names at will, and there's no need to delete tokens. However, if people change their email address, we do need to delete tokens. But what's the point of having a token to restore the original email address ("If you didn't authorize this change, click here to undo it...") if it gets deleted as soon as the change is validated by someone clicking a link at the new email address?

> >+    if (!ref($invocant) || lc($invocant->login) ne lc($login)) {
> >+        is_available_username($login)
> 
> is_available_username() is a pretty useless method. You can directly call
> login_to_id(). This method does nothing else:

I think it's a useful customization point. If you want to restrict the available usernames, it's clear how and where to do it.

> >+    if (!ref($invocant) || $invocant->email ne $email) {
> 
> The comparison must be lowercase, else you could create the same account
> several times using the same email address (PostgreSQL is case-sensitive).

I'm not sure how the issue you mention could be caused by this comparison not being lower-case. The second half of this comparison checks to see if the user is changing their email; if we made it a lower-case comparison, then this check would _fail_ for a case-change, and the user could change the case of their email address at will. Currently, they can't do that. I have no strong preference as to which to allow, but neither option allows the creation of multiple accounts AIUI.

> >+sub set_email {
> >+    my ($self, $email) = @_;
> >+    $self->set('email', $email);
> >+    $self->set_login($email) if Bugzilla->params->{'use_email_as_login'};
> 
> For this code to work, you must set VALIDATOR_DEPENDENCIES so that it's not
> possible for set_all() to override the login name if the param is turned on.

Done. (Check I got it right.)

> Also, don't call set_login() itself, but call ->set('login_name') directly.
> See how set_disabledtext() does it.

But set_login() has other effects I want (it clears out $self->{nick}). Why is calling it this way actively bad?

> >+        # Special handling for "@" and ":" prefixes. We strip them off
> >+        # before matching, so ":gerv" or "@gerv" exact-matches user "gerv"
> >+        # and that user will be the only result returned.
> 
> No! First of all, this a bmo-specific hack. Secondly, @ is not allowed in
> login names, 

Indeed. Which is why referring to people as @LpSolit to get an exact match is both syntactically OK, and fits with how people now expect that character to be used. (I.e. I don't think using "@gerv" to search for an exact match for me is at all BMO-specific.)

> and : is perfectly valid character in login names.

Searching for ":gerv" to search for exactly me is arguably BMO-specific, due to our local convention (which emerged before @ became the standard). However, how common is it going to be for login names to _start_ with a colon? Still, I've updated the code to look for only @, and we can change BMO to also look for ":".

> So let's use login_to_id() directly where we need it. And "username" is
> confusing as it sounds like "realname". is_available_login() would have been
> a better name anyway.

Renamed.

> > sub check_and_send_account_creation_confirmation {
> >-    my ($self, $login) = @_;
> >+    my ($self, $login, $email) = @_;
> 
> I don't think we should require a login at that point. The important part of
> the process is to make sure the email address belongs to the user. We should
> ask for a login at the same time as the password. Also, the UI should allow
> the user to not enter any login, in which case the email address will be
> used as the default login. This will also let you remove several of the
> changes you made here.

Actually, I think this is a policy question. Some sites may want to require that users choose a login which is not their email address, to help them preserve their privacy. I think that if use_email_as_login is off, that's a good sign that emails are not desirable as logins. (Yes, when we migrate, we set them that way, but we don't expect things to stay that way - otherwise why would the admin have flipped the switch?)

offer_account_by_email() already sets $login to $email if use_email_as_login is set, so both will be filled in, in that case. So I think there is no change required here.

> > sub login_to_id {
> 
> >     } elsif ($throw_error) {
> >         ThrowUserError('invalid_username', { name => $login });
> 
> Please rename this error code to 'invalid_login_name' for clarity.

This error code is used in a number of places, including the example extension. I'm happy to make limited changes to remove the use of "username" terminology if you think it's confusing, but cleaning it up across the entire codebase will need an additional patch. (Which can mostly be done with search-and-replace.)

> >+sub email_to_id {
> 
> It's mostly a clone of login_to_id(). It would be good to refactor the code
> a bit to avoid all these clones when possible.

They don't look that similar to me; login_to_id() uses a cache, and they call different errors, and so on. I can't see a refactor which would be obviously clearer than what we have now. Suggestions?

> > =item C<nick>
> >+Usually, this is an alias for C<login>.
> >+If email addresses are used as logins, though, then this is the part of the
> >+user's email address before the at sign (@).
> 
> This raises a big concern to
> me. Now that we cannot see other users email address anymore, we have no way
> to make sure that "gerv" is really gerv, or maybe it's ":gerv"? Or
> "gerv|mozilla"? Someone could easily try to abuse the system this way to be
> CC'ed to security bugs. And it's not an option to refuses someone else's
> login because it's a substring or a superstring (does this word exist?) of
> an existing login name. Being able to see the email address permits to
> authenticate a user for sure, because a user cannot impersonate/spoof
> someone else's email address, especially for corporate domain names (such as
> @mozilla.com). If I need to CC someone on a security bug, how do I
> distinguish who is who with realname + login name only? This question is a
> showstopper before getting any r+ from me.

Let me make sure I understand. You are saying that there are potential security implications for sites which decide to make email != login (and hide email), because someone might create a login name which looks plausibly like a privileged user, and wait to get CCed on things they shouldn't see. Whereas if users can see other email addresses, they have additional validation on the user's identity because their email address reveals their organizational membership.

I'm not convinced that the danger is much greater than it is now. Many Mozille employees (to take an example Bugzilla-using group) do not use their @mozilla.com email addresses. Security guru Dan Veditz, for example, used to use [e-mail]. I could have registered cruzioo.com, created an email address, and spoofed Dan's Bugzilla account details exactly apart from one letter.

We could add documentation warning about this, but I don't think it's a showstopper. You need to know who you are CCing, email address or no email address. In fact, having fixed login names will _help_ here. Currently, anyone can put ":gerv" or ":dveditz" in their Real Name, and there's a risk that someone not paying attention will mis-CC that person. With login names and exact matching (which you object to :-), the risk of mistakenly CCing someone is reduced.


> > =item C<create>
> 
> > Params: login_name - B<Required> The login name for the new user.
> 
> The login name shouldn't be mandatory. If missing, it should fallback to the
> email address, which can always be used as login name, independently of the
> use_email_as_login parameter.

See above for my logic. login_name is mandatory; if use_email_as_login is set, calling code should pass the same value twice. If it's not, it should require a separate value.

> >diff --git a/Bugzilla/WebService/Server/JSONRPC.pm
> 
> >+    elsif ($type eq 'email') {
> >+        # If we use_email_as_login, we assume email (and login) exposure is
> >+        # OK, because it's used in the UI. If we aren't, then we hide email
> >+        # addresses completely unless you are logged in. Eventually, we will
> >+        # hide them unless the user is in editusers, but that requires giving
> >+        # API users another way to unambiguously refer to users.
> >+        if (!Bugzilla->params->{'use_email_as_login'}
> >+            && !Bugzilla->user->id)
> >+        {
> >+            $retval = "";
> >+        }
> >     }
> 
> Wow, this is totally confusing. Now either the user is powerful enough to be
> allowed to get the email address back, or he isn't and that's the job of the
> caller to not return this data. Maybe the 'email' type should be renamed
> 'login' or something similar, and real email fields do not need any special
> type anymore, because only admins should be allowed to get them, and in
> special methods only.

OK. I've changed it so that if use_email_as_login is off, we only expose email addresses to those with editusers.

> >diff --git a/Bugzilla/WebService/Server/XMLRPC.pm
> 
> >         elsif ($type eq 'email') {
> >             $type = 'string';
> >+            $value = email_filter($value);
> >         }
> 
> Same confusion and comment here.

It's not quite obvious to me what email_filter does in Bugzilla/Util.pm - does it reduce email addresses to the localpart?

Are you saying we want the same logic here as we have in JSONRPC.pm above?

> >diff --git a/Bugzilla/WebService/User.pm
> 
> >-    # Username and password params are required 
> >+    # Username and password params are required
> >     foreach my $param ("login", "password") {
> 
> s/Username/login/. I don't know why we use two distinct words to mean the
> same thing (username vs login).

I've changed it here, but there are a lot more, and as I've said, a global change is outside the scope of this bug.

> >+    $login_name = $email if Bugzilla->params->{use_email_as_login};
> 
> There is no need to do this here. This is the job of validators.

The validators in question would be check_email and check_login_name in Bugzilla/User.pm? Neither of them contain this logic... set_email() contains it. Is that enough, now we have VALIDATOR_DEPENDENCIES set?

> >+          <input size="35" name="new_email" value="[% user.email FILTER html %]">
> 
> Please restore type="email" if use_email_as_login = 1. 

Done.

> Also, do not delete
> IDs defined in form fields. You are breaking all Selenium tests due to that.

I think we do want to change the IDs, as the fields now have a different semantic meaning, and things like tests will need updating.

> It also makes customization much harder when fields do not have an ID.

I have added IDs.

> Does it make sense to display this field when authenticating with LDAP or
> RADIUS or ENV?

I've never used these authentication methods, and neither has anyone I know. Do they even use this form? Is there any documentation on how they work from a user perspective?

> >diff --git a/template/en/default/account/request-new-password.html.tmpl
> 
> >     If you have an account, but have forgotten your password, enter your
> >+    [% IF Param('use_email_as_login') %]
> >       email address
> >+    [% ELSE %]
> >+      login name
> >     [% END %]
> 
> We should make no difference between both cases. You should request the
> *email address* in all cases. 

OK. I've updated the code so all password resets require an email address, and only an email address.

> >diff --git a/template/en/default/admin/params/ldap.html.tmpl
> 
> >   LDAPmailattribute => "The name of the attribute of a user in your " _
> >                        "directory that contains the email address, to be " _
> >-                       "used as Bugzilla username. If this parameter " _
> >-                       "is empty, Bugzilla will use the LDAP username"_
> >-                       " as the Bugzilla username. You may also want" _
> >-                       " to set the \"emailsuffix\" parameter, in this case.",
> >+                       "used to send the user bugmail.",
> 
> As I understand it, this parameter cannot be left empty, right? 

According to the code in LDAP.pm, it can.

> I found several bugs here. The admin is allowed to inject @ in login names,
> even when the login name doesn't match the email address.

It seems I haven't yet put in code for rejecting login names with an @ sign in. :-| The obvious place to do this is check_login_name in User.pm. However, in the discussion above, it seems like we wanted to allow people to use a login name with an @ sign as long as it was equal to their email address (even when use_email_as_login is off). 

However, the validator doesn't have access to the user's current email address, does it? Or rather, it doesn't know if the login name it's validating is one which belongs to the currently logged-in user or another user. How do I solve that?

> This should be
> forbidden, even for admins. Whitespaces should be forbidden too.

Whitespace should now throw an error.

> > sub cancel_create_account {
> >-    my ($login_name, $token) = @_;
> >+    my ($event_data, $token) = @_;
> > 
> >     $vars->{'message'} = 'account_creation_canceled';
> >-    $vars->{'account'} = $login_name;
> >+    ($vars->{'email'}, $vars->{'login'}) = split(':', $event_data);
> 
> Please revert these changes too.

Why?
 
> >+    # This is used only if email and login are separate
> >+    if ($user->authorizer->can_change_login
> >+        && $new_login
> >+        && $user->login ne $new_login)
> 
> This is not enough. One could easily bypass the use_email_as_login parameter.

I'm not sure I follow you here...

> >+        if ($new_login =~ /@/ &&
> >+            $new_login ne $user->email)
> >+        {
> >+            ThrowUserError("login_at_sign_disallowed");
> >+        }
> >+
> >+        if ($new_login =~ /\s/) {
> >+            ThrowUserError("login_illegal_character");
> >+        }
> 
> This is not the right place to do sanity checks. These checks must be in
> check_login().

Done for the whitespace; see above for why this might be a problem for the @ sign.

> >+        if (Bugzilla::Token::HasEmailChangeToken($user->id)) {
> >+            ThrowUserError("login_change_during_email_change");
> >+        }
> 
> No reason to depend on a pending email change, AFAIK.

The aim of this code is to prevent login name changes while email changes are going on, for simplicity. Are you saying that's the wrong policy?

> In fact, you should call
> $user->set_name instead of all this.

Do you mean set_email? This code is not setting the email, it's sending out the email change token.

Phew!

Gerv

Comment 110

[Gervase Markham [:gerv]]

> >diff --git a/Bugzilla/WebService/Bug.pm
> 
> >-        creator       => $self->type('email', $comment->author->login),
> >+        creator       => $self->type('login', $comment->author->login),
> 
> Err... 'login' is not a valid type for WS, right? It should be 'string'
> instead. There are many places where you did this change. 

You are right; this is bogus. I can either change it back, or add special handling (in Bugzilla/WebService/Server/JSONRPC.pm?)

> On the other hand,
> all existing installations will use their email address as login when
> upgrading to Bugzilla 6.0, and it will take a long time before most users
> change their login to something else. This doesn't mean we should expose
> their email address to everone. So IMO, we should still filter login names
> to remove everything after @ if present in the login name and the caller
> hasn't enough privs to see the full login name (e.g. because he isn't logged
> in/authenticated).

At the moment, we have a param for this (webservice_email_filter), right? If it's on, we truncate logins (== email addresses) unless the user is logged in. BMO currently has it off. Am I right in saying that if it's on, a non-logged-in query has no way of disambiguating users?

We could retain the parameter and apply it to login names were use_email_as_login is true, or we could eliminate it, and always do either a) nothing, or b) non-logged-in filtering. What would you suggest?

Gerv

Comment 111

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #109)
> At the moment, localparts of email addresses are used as identifiers in the
> GUI when the accessor is not logged in, if a person in question has not set
> a real name.

But this bug is specifically to change that. So we must enforce this new policy: email adresses are private and are only used to send emails. So now is the right time to change the current policy.



> > Err... if use_email_as_login = 1 and $email is not defined, we should return
> > an error code, not silently ignore it.
> 
> I think you are misreading this code. It says "If we are using email
> addresses as logins, then don't use the passed-in username as the account
> identifier, use the passed-in email address. It is not error-handling code.

But still, it's not acceptable to have $email being undefined, so the first part of the test doesn't make sense.



> > Err... what's this change? Keeping old tokens has security implications, and
> > this has nothing to do with this bug.
> 
> [...] But what's the point of having a token
> to restore the original email address ("If you didn't authorize this change,
> click here to undo it...") if it gets deleted as soon as the change is
> validated by someone clicking a link at the new email address?

Currently, you can already cancel an email address change, and this code already exists. This code is not called when you want to change your email address in userprefs.cgi. Bugzilla::Token::IssueEmailChangeToken() is called instead. So please revert your change.



> > is_available_username() is a pretty useless method. You can directly call
> > login_to_id(). This method does nothing else:
> 
> I think it's a useful customization point. If you want to restrict the
> available usernames, it's clear how and where to do it.

This is not the right place for it, IMO. is_available_username() should not lie. So what you want to customize is its caller.



> > The comparison must be lowercase, else you could create the same account
> > several times using the same email address (PostgreSQL is case-sensitive).
> 
> I'm not sure how the issue you mention could be caused by this comparison
> not being lower-case.

If [e-mail] exists, then I could create [e-mail]. PostgreSQL would be happy to create it.



> > No! First of all, this a bmo-specific hack. Secondly, @ is not allowed in
> > login names, 
> 
> Indeed. Which is why referring to people as @LpSolit to get an exact match
> is both syntactically OK, and fits with how people now expect that character
> to be used. (I.e. I don't think using "@gerv" to search for an exact match
> for me is at all BMO-specific.)

Currently, @LpSolit in QuickSearch means "bugs assigned to me". I would have to think about this a bit more, but for now, I would prefer to see this in a separate bug.



> > I don't think we should require a login at that point.
> 
> Actually, I think this is a policy question. Some sites may want to require
> that users choose a login which is not their email address, to help them
> preserve their privacy.

I still think that's not the right time to ask for it. And this will make your code easier. I much prefer the email address to be validated first, and then ask for a login name, especially if the user keeps choosing a login name which already exists.


> Let me make sure I understand. You are saying that there are potential
> security implications for sites which decide to make email != login (and
> hide email), because someone might create a login name which looks plausibly
> like a privileged user, and wait to get CCed on things they shouldn't see.
> Whereas if users can see other email addresses, they have additional
> validation on the user's identity because their email address reveals their
> organizational membership.

Yes, exactly!


> I'm not convinced that the danger is much greater than it is now. Many
> Mozille employees (to take an example Bugzilla-using group) do not use their
> @mozilla.com email addresses. Security guru Dan Veditz, for example, used to
> use [e-mail]. I could have registered cruzioo.com, created an
> email address, and spoofed Dan's Bugzilla account details exactly apart from
> one letter.

I wouldn't have CC'ed dveditz on any security bug if I didn't know who he was exactly. So yes, seeing his @mozilla.com email addresss would have helped me to take this decision.


> We could add documentation warning about this, but I don't think it's a
> showstopper. You need to know who you are CCing, email address or no email
> address.

I disagree. Email addresses give a real information about who you are, especially if you don't know other users personally. One example: bug 957403 in which seeing a @adobe.com or @apple.com email address gives you a good idea of whom you are talking to.



> In fact, having fixed login names will _help_ here. Currently,
> anyone can put ":gerv" or ":dveditz" in their Real Name, and there's a risk
> that someone not paying attention will mis-CC that person. With login names
> and exact matching (which you object to :-), the risk of mistakenly CCing
> someone is reduced.

This only works if you know exactly who the other users are. When I'm involved in other Bugzilla installations, such as GCC (I'm the admin), I'm very happy to see their @gcc.gnu.org email addresses, because this helps me to know if such or such guy is in the GCC team or not. Their login alone wouldn't help me otherwise.



> > The login name shouldn't be mandatory. If missing, it should fallback to the
> > email address, which can always be used as login name, independently of the
> > use_email_as_login parameter.
> 
> See above for my logic. login_name is mandatory; if use_email_as_login is
> set, calling code should pass the same value twice. If it's not, it should
> require a separate value.

Except that it shouldn't be mandatory.



> > >         elsif ($type eq 'email') {
> > >             $type = 'string';
> > >+            $value = email_filter($value);
> > >         }
> > 
> > Same confusion and comment here.
> 
> It's not quite obvious to me what email_filter does in Bugzilla/Util.pm -
> does it reduce email addresses to the localpart?

Yes.


> Are you saying we want the same logic here as we have in JSONRPC.pm above?

Yes.



> > There is no need to do this here. This is the job of validators.
> 
> The validators in question would be check_email and check_login_name in
> Bugzilla/User.pm? Neither of them contain this logic... set_email() contains
> it. Is that enough, now we have VALIDATOR_DEPENDENCIES set?

It should.



> However, the validator doesn't have access to the user's current email
> address, does it? Or rather, it doesn't know if the login name it's
> validating is one which belongs to the currently logged-in user or another
> user. How do I solve that?

VALIDATOR_DEPENDENCIES.



> > > sub cancel_create_account {
> > >-    my ($login_name, $token) = @_;
> > >+    my ($event_data, $token) = @_;
> > > 
> > >     $vars->{'message'} = 'account_creation_canceled';
> > >-    $vars->{'account'} = $login_name;
> > >+    ($vars->{'email'}, $vars->{'login'}) = split(':', $event_data);
> > 
> > Please revert these changes too.
> 
> Why?

Because we shouldn't require a login name at this point.



> > >+    # This is used only if email and login are separate
> > >+    if ($user->authorizer->can_change_login
> > >+        && $new_login
> > >+        && $user->login ne $new_login)
> > 
> > This is not enough. One could easily bypass the use_email_as_login parameter.
> 
> I'm not sure I follow you here...

Where do you check that Bugzilla->params->{use_email_as_login} is false?



I answered all these questions without looking at the code again, so I hope I still remember it correctly. :)

Comment 112

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #111)
> But this bug is specifically to change that. So we must enforce this new
> policy: email adresses are private and are only used to send emails. So now
> is the right time to change the current policy.

If someone has not set a real name, and email_is_login is set, what identifier do you propose to use for that person? We only have one, and that's the email address. We can reduce it to a localpart to keep it somewhat private, but we can't just put "Anonymous Person" for everyone. Is that what you are proposing?

> I still think that's not the right time to ask for it. And this will make
> your code easier. I much prefer the email address to be validated first, and
> then ask for a login name, especially if the user keeps choosing a login
> name which already exists.

I am not going to make this change in this patch, because changing the way this works would require a lot of re-testing to make sure I've not broken anything. This bug is now 12 years old, and I've been working on it for 2 or 3 - it is not the time for a refactoring of this sort.

However, I am happy to file a follow-up bug about it.

> > > The login name shouldn't be mandatory. If missing, it should fallback to the
> > > email address, which can always be used as login name, independently of the
> > > use_email_as_login parameter.
> > 
> > See above for my logic. login_name is mandatory; if use_email_as_login is
> > set, calling code should pass the same value twice. If it's not, it should
> > require a separate value.
> 
> Except that it shouldn't be mandatory.

Is this an argument about the interface to the function? We are agreed that whatever happens, the login_name DB field needs to be populated, right? I'm saying that this function should be called with a login_name parameter (which may turn out to be the same as the email address); you are saying that the login_name parameter should not be mandatory, and if it's not present, it should be set to what the email address is. Which has exactly the same end effect.

Am I understanding your point correctly?

> I answered all these questions without looking at the code again, so I hope
> I still remember it correctly. :)

I think I've implemented the other changes you requested, but you'll need to check because it was a while ago that I did most of the work.

Gerv

Comment 113

[Gervase Markham [:gerv]]

Attachment: Patch v.7

If you have further non-massive code changes you want me to make, it would really really help if you made them in your copy and attached an updated patch, rather than described them. I often find it difficult to understand what change you are asking for when you describe it in prose.

Thanks :-)

Gerv

Comment 114

[Frédéric Buclin]

Comment on attachment 8667912 [details] [diff] [review]
Patch v.7

Your code doesn't compile:

# syntax error at Bugzilla/Token.pm line 114, near "$email:"

This is because you must write "$email:$login" (missing double quotes).


Also, it doesn't pass tests:

#   Failed test 'Bugzilla/User.pm contains tabs --WARNING'
#   at t/005whitespace.t line 37.

#   Failed test 'Bugzilla/User.pm POD coverage is 99%. Undocumented methods: is_available_login'
#   at t/011pod.t line 109.


With the compilation error fixed, I now get an error when a new user tries to create a new account:

An error occurred while performing a database operation:

DBD::mysql::db selectrow_array failed: called with 2 bind variables when 1 are needed [for Statement "SELECT COUNT(*)
           FROM tokens
          WHERE tokentype = ?
                AND eventdata REGEXP '^:'
                AND issuedate > NOW() - INTERVAL 10 MINUTE"] at Bugzilla/Token.pm line 103.
	Bugzilla::Token::issue_new_user_account_token("foo\@bar.com", "guest\@bugzilla.org") called at Bugzilla/User.pm line 2491
	Bugzilla::User::check_and_send_account_creation_confirmation(Bugzilla::User=HASH(0x8d03928), "foo\@bar.com", "guest\@bugzilla.org") called at /var/www/html/bugzilla-tmp/createaccount.cgi line 47


If I try to create the new account as an admin, I also get an error:

Can't use string ("Bugzilla::User") as a HASH ref while "strict refs" in use at Bugzilla/User.pm line 508.

Comment 115

[Neville Hillyer]

Perhaps this is rather late in the day and perhaps it has already been considered but here goes:

Why not automatically assign a random name to existing users and allow those who wish to change it later?

It may be be tedious getting the database to keep track of user history during the name changing process but it may be a simpler in the long run.

Comment 116

[Frédéric Buclin]

One more thing:

In Bugzilla/WebService/User.pm, you must also exclude the email address from being returned, or restrict it to users in the editusers group only.

Comment 117

[Frédéric Buclin]

Attachment: patch, v7.1

Unbitrotten patch. Half of the conflicts were due to whitespace removals in areas unrelated to the patch. I reverted them, because 1) they increase the risk to bitrot, 2) they appear in git blame, making it harder to track older changes, 3) they make the patch larger than necessary, making it harder to track related changes only. The patch is already big enough.

I also removed is_available_login from the Bugzilla::User::EXPORT list, because it was undefined and unused (thanks to 011pod.t for catching that).

Now I can work on it. :)

Comment 118

[Frédéric Buclin]

Attachment: patch, v8

Here we go! :)

Comment 119

[Frédéric Buclin]

Attachment: patch, v8.1

Remove the last 2 references to emailsuffix.

Comment 120

[Frédéric Buclin]

Attachment: patch, v8.2

Fix bitrot.

Comment 121

[Gervase Markham [:gerv]]

Comment on attachment 8741559 [details] [diff] [review]
patch, v8.2

Review of attachment 8741559 [details] [diff] [review]:
-----------------------------------------------------------------

Here are the comments from a visual code review. I'm now going to do some testing.

Gerv

::: Bugzilla/API/1_0/Resource/Bug.pm
@@ +1550,1 @@
>      }, $types, $prefix;

I can see from a privacy perspective why you might want to remove "email" here; but presumably this is a backwardly-incompatible change? Is it documented as such?

::: Bugzilla/API/1_0/Resource/User.pm
@@ +707,5 @@
>  
>  =item C<email> (string) - The email address for the new user.
> +If the installation has the C<use_email_as_login> parameter switched on, then
> +this parameter is ignored, and the value of the C<email> parameter will
> +be used as the login name for the new account.

There's something wrong here. "This parameter" in this context is the email parameter, which this text says is both "ignored" and "used as the login name".

Did you mean to attach this comment to the description of the "login" item just below, rather than here?

::: Bugzilla/Install/DB.pm
@@ +748,4 @@
>      # 2015-12-16 [e-mail] - Bug 1232578
>      _sanitize_audit_log_table();
>  
> +    # 2016-04-06 [e-mail] and [e-mail] - Bug 218917

Do add yourself to this auspicious list :-)

::: Bugzilla/User.pm
@@ +2673,5 @@
> +
> +Returns the sanitized login name if that login name is not already used by
> +another user account, and if it contains no forbidden characters, which means
> +no whitespaces, and no @ character unless the login name matches the email
> +address of the user account. Else an error is thrown.

This is a bit confusing as English. Try:

Returns the sanitized login name if:

1) it is not already used by another user account; and
2) it contains no forbidden characters, which means no whitespace characters; and
3) it contains no @ character (unless it exactly matches the email address of the account). 

Else an error is thrown.

Comment 122

[Gervase Markham [:gerv]]

I have run through my test plan and could find no clear problems. There was one instance where it seems like someone "fell out" of a group they were in because of the groups membership regexp, and I had to change the regexp to force a regenerate. But I don't know how it happened, and I don't know if it's connected to this patch. 

So r=gerv! Although we might want to wait for trunk to stabilise before committing.

Gerv

Comment 123

[Frédéric Buclin]

Attachment: patch, v8.3

I fixed all your review comments, and also updated the REST API doc to mention the login/email split. Carrying forward gerv's r+.

Comment 124

[Dylan Hardison [:dylan] (he/him)]

Please do not commit this yet to master. Given the limited resources (the resource being our release manager).

If we were to commit this, it would immediately end our ability to harmonize bugzilla and bmo. Basically we're blocked by bug 974387

Comment 125

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #124)
> If we were to commit this, it would immediately end our ability to harmonize
> bugzilla and bmo.

As I said on IRC, I see no reason for upstream to wait for bmo, especially when we know that the merge will not happen soon (probably several weeks or a few months, right?). Upstream also has very limited ressources, and its development is already slow enough. Letting the patch sit here is not acceptable, because it will bitrot very quickly due to its size (I already had to unbitrot gerv's patch once). Committing the patch to a separate branch, as suggested on IRC, and merging with master later this year won't help either, because nobody would use this branch to test this new feature, and so we would reach 6.0rc1 with almost no serious testing. Much better to commit such a large change now and have time to track and fix regressions. This is what a development cycle is for.

<LpSolit> people are testing master, not some x-y branch
<dylan> or I can take the other road and we have email-only (as a branch) and that is the current master
<dylan> and email-only is what continues to get merged into bmo
<LpSolit> ok, so if you create a branch for your bmo merge, I can commit it now
<dylan> so yes, do that. 


Thank you gerv for the original work. :)

To ssh://[e-mail]/bugzilla/bugzilla.git
   c44470a..3891b63  master -> master

Comment 126

[u602372]

Without reading previous comments (too many of them), or the commit code (too large): 
Does this bug fix should finally allow hiding the email address from other userson Bugzilla?

Comment 127

[Dylan Hardison [:dylan] (he/him)]

I'm not sure, but I have a different vision of what login_names vs. email addresses will be as documented in these bugs:

Bug 1372629
Bug 1372633
Bug 1372638
Bug 1372631

Comment 128

[Neville Hillyer]

I was unsure about this so have tried a few things and as far as I can tell:

1 - It hides the email address from the general public in cases where the user has supplied a name.

2 - It may not hide the email address if the user has not supplied a non-email name - perhaps others can confirm this.

3 - All email addresses appear to be available to logged in users.

4 - My name was not accepted for login purposes - perhaps I need to try this again.

5 - The login form does not make it clear if a user name is acceptable.

6 - The top of the page displays a form with my email address when I am logged in - I am not sure why and would have expected it to display my name.

7 - The password reset process is can be confusing and would be better changed to a more standard process - I am not sure if it deliberately changes to a tedious process after a few failed attempts.

Comment 129

[Neville Hillyer]

I can confirm that I am unable to use 'Neville Hillyer' to log in and I cannot see how to add a separate user name so perhaps the original request was never satisfied.

Comment 130

[Frédéric Buclin]

Did you turn off the "use_email_as_login" parameter? See editparams.cgi?section=auth#use_email_as_login_desc

When that's done, you must set your login name to something different from your email address, see userprefs.cgi?tab=account

Your login name is not your real name, see http://bugzilla.readthedocs.io/en/latest/administering/users.html#modifying-users

Comment 131

[Neville Hillyer]

I cannot see use_email_as_login switch in my profile.

Where is it and where are returning users told about it?

Comment 132

[Frédéric Buclin]

(In reply to Neville Hillyer from comment #131)
> I cannot see use_email_as_login switch in my profile.

Profile? It's a parameter, not a user preference, see the URL I pasted. To access it from the UI:

Administration > Parameters > User Authentication > use_email_as_login

Comment 133

[Neville Hillyer]

What user interface. I am using a browser on an old Mac.

Comment 134

[Neville Hillyer]

At the following URL:

https://bugzilla.mozilla.org/editparams.cgi?section=auth#use_email_as_login_desc

I get the following message:

"Sorry, you aren't a member of the 'tweakparams' group, and so you are not authorized to access parameters."

Comment 135

[Frédéric Buclin]

I was of course talking about your own Bugzilla installation. If you are talking about this Bugzilla installation (bugzilla.mozilla.org aka BMO), then this feature is not available as it's not running Bugzilla 5.1.

Further comments or help about this feature should go into the newsgroup or on IRC, see https://www.bugzilla.org/support/.

Comment 136

[Neville Hillyer]

(In reply to Meir Cohen from comment #126)

It relation to this version of Bugzilla.

It is very easy for anybody to harvest email addresses by:

1 - Without logging in and obtaining the large number of public email addresses.

2 - Obtaining an account and getting all the other addresses.

This may be acceptable as long as users are warned about it - preferably with a short note and link at the top of every page.

For this reason I am not prepared to use my normal email address here.

Comment 137

[Neville Hillyer]

(In reply to Frédéric Buclin from comment #135)

I was under the impression that this bug report was originally primarily about this Bugzilla, ie the installation at: https://bugzilla.mozilla.org

It is unfortunate that it is now being used for more general Bugzilla software development and that comments about this installation no longer appear to be acceptable.

As previously stated strictly the original bug was never corrected for this installation albeit much of the security concern was probably overcome for most users.

Comment 138

[Frédéric Buclin]

This bug has never been about this specific Bugzilla installation. Product = "Bugzilla" means the Bugzilla software, not bugzilla.mozilla.org. See dylan's comment 127 for BMO specifically.

Again, it's not the right place to complain that this feature is not available here.

Comment 139

[Neville Hillyer]

Where should I report bugs/difficulties with this installation?

Comment 140

[Dylan Hardison [:dylan] (he/him)]

(In reply to Neville Hillyer from comment #139)
> Where should I report bugs/difficulties with this installation?

in the bugzilla.mozilla.org product. 

Note in comment 37 I outlined (some) of the tasks I've scoped to actually support login names
in a way that will work for us (BMO).

As we start the next quarter, I'll engage with my team and see if we can prioritize the username work.

Comment 141

[Neville Hillyer]

I would be grateful if you could clarify: "in the bugzilla.mozilla.org product".

A URL would be useful.

Comment 142

[Emmanuel Seyman]

(In reply to Neville Hillyer from comment #141)
> I would be grateful if you could clarify: "in the bugzilla.mozilla.org
> product".

There's a product called "bugzilla.mozilla.org". This is the product you should file bugs that target this specific instance of bugzilla.
 
> A URL would be useful.

https://bugzilla.mozilla.org/enter_bug.cgi?product=bugzilla.mozilla.org

Comment 143

[Neville Hillyer]

OK - thanks - please see: https://bugzilla.mozilla.org/show_bug.cgi?id=1400827

Comment 146

[u591120]

In case anyone is as confused as I was, here's a link to the actual bug report for this forum's non-private email problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=1372631

Quite amazing that after 17 years this forum might actually be getting this feature soon (thanks to Kohei Yoshino).

Comment 148

[Twisted Code]

since bugs apparently get treated as dupes of this, even though this is marked as resolved and not actually implemented (hence how you can have a dupe of it)... for 6 years... can I ask why it was marked as resolved 6 years ago yet still not implemented?
I should also note that, if there's not an actual resolution soon, I'm probably just going to deactivate this account because I'm tired of my email getting leaked. From my perspective, there is no reason other than what seems to be the unstated reason: "privacy is not ACTUALLY a priority for Mozilla, at least when it comes to the people helping to identify bugs"... though I am hoping I'm wrong?

Comment 149

[Twisted Code]

(and if it wasn't for the fact the email on this account was ALREADY a junk email —just more active and needs to get checked more often—, I'd have already disabled this account. I've seen legislative committees move faster than this issue)

Comment 150

[Neville Hillyer]

Can you quantify the junk mail you have had from this address?

Comment 151

[Twisted Code]

(In reply to Neville Hillyer from comment #150)

Can you quantify the junk mail you have had from this address?

I can't think of a good metric for it. Admittedly, that inbox is already inundated with spam, most of which predates my presence on this tracker. However, that leak is also not the root of my frustration.
I'm far more disappointed and frustrated by the fact my email got exposed in the first place. I feel like you could've at least been clear about the scope either before or as part of the Oauth authentication workflow. Had I known this oauth scope really meant "we're going to look at your email address and include it, visible to other members, every time you interact with us"... I would have used a different github account or created my account directly.

This will be my last post via this account until I'm able to change the email SUCCESSFULLY. I tried unsuccessfully 2 times, but maybe the third time's a charm...
I may continue to follow up via my other account in the meantime, but I honestly don't feel like I'm getting anywhere here.

Comment 152

[Zarro Boogs]

(In reply to Neville Hillyer from comment #150)

Can you quantify the junk mail you have had from this address?

It shouldn't matter. You should close the barn door before the horse escapes, not after. I just discovered this tonight, and boy was I upset about it.

I don't want anyone giving my street address to strangers, either, even though nobody's ever (yet) come by my house and thrown eggs at it because they didn't like something I said in public. It's just common sense.

On a serious note, though, I've been cyberstalked by a psycho who managed to track down my real-life identity through obsessive dedication from scattered snippets on the internet and interfered with my IRL business, and it is NOT FUN. Internet detectives can connect tiny scraps of unwittingly leaked information about you to discover all kinds of things, and it only takes one single dedicated loony to make your meatspace life problematic for years. I'm old enough to remember when the internet was a trusting place run by academics, the environment Netscape was founded in, but we're not in Kansas anymore. I thought it was just established good netiquette for at least 10 or 15 years by now that revealing people's email addresses to strangers should not be the default behavior. PII should be kept private by default, unless the individual voluntarily opts-in to share it. That bell can't be unrung.

Obviously I've now changed this account to a disposable email address, but that's far from a good solution. It means that every time I use such an address's primary function and dispose of it, I will have to specifically remember to come back to bugzilla and manually update to the next one, which seems like an unnecessary pain. And if more sites I have accounts on were to start adopting the attitude of this one, keeping up with it would soon go from a pain to a downright impossibility.

Comment 153

[Neville Hillyer]

I agree with all you say but there are very few complaints about this - perhaps because most now have a name unrelated to their email address.

My view is that email addresses should never be published, even to closed groups, without the specific permission of the user.

It is a pity that my earlier suggestion was not adopted, ie assign an editable random user name to all those who did not have one.

At one point I requested better warnings about the issue.

This bug is now 'resolved' so it may be more fruitful to start a new bug if you wish to take this further.

Comment 154

[u701030]

FIX THIS! I CAN STILL GET ALL EMAIL ADDRESSES FROM EVERYBODY! YOU WONT LET ME DELETE MY POSTS SO I CANT REVERSE MY EMAIL BEING PUBLIC NOW! WTF MOZILLA! LET USER COMPLETELY HIDE THEIR EMAIL ADDRESSES! I WOULD HAVE NEVER USED THIS BUGTRACKER WHEN I WOULD HAVE KNEW ABOUT THIS CRAPPY BEHAVIOUR!

Comment 155

[Twisted Code]

(In reply to Neville Hillyer from comment #153)

I agree with all you say but there are very few complaints about this - perhaps because most now have a name unrelated to their email address.

My view is that email addresses should never be published, even to closed groups, without the specific permission of the user.

It is a pity that my earlier suggestion was not adopted, ie assign an editable random user name to all those who did not have one.

At one point I requested better warnings about the issue.

This bug is now 'resolved' so it may be more fruitful to start a new bug if you wish to take this further.

re: complaints
quite possibly, no one realizes how serious email privacy can be. Also what is your metric for "very few"?
re: "more fruitful to start a new bug"
I tried to start a new one before I even commented on this one. It got marked as a duplicate of this, despite the seemingly obvious contradiction of saying an unresolved problem is a duplicate of a resolved issue...
I wasn't going to un-mark it as a duplicate since I haven't been around very long and don't want to override someone else's call, but if you agree they were mistaken, I will.

(In reply to u701030 from comment #154)

FIX THIS! I CAN STILL GET ALL EMAIL ADDRESSES FROM EVERYBODY! YOU WONT LET ME DELETE MY POSTS SO I CANT REVERSE MY EMAIL BEING PUBLIC NOW! WTF MOZILLA! LET USER COMPLETELY HIDE THEIR EMAIL ADDRESSES! I WOULD HAVE NEVER USED THIS BUGTRACKER WHEN I WOULD HAVE KNEW ABOUT THIS CRAPPY BEHAVIOUR!

I agree. While I'm not sure the CAPS LOCKING is helping your point, and I think you are mistaken that you can't retroactively hide your email (just change to a throw-away email in your account settings. Hopefully™ that updates every reference to your email like it seems to for me), it would be a mistake to dismiss your anger simply because it's angry. Emotional people sometimes have very good points! in my experience thus far with the issue, the biggest take away is that if you are going to expose the user's email to anyone with an (easy to obtain) account and an HTTP client... you had better be quite sure the user is okay with this.

In my case, I named the Mozilla-relay email alias I'm using here "Bugzilla doesn't know the meaning of email privacy", so I'm still a bit peeved about the breach of trust

Comment 156

[Twisted Code]

also, I like the barn door and horse analogy. It's quite fitting, considering that's pretty much exactly how I felt when I realized that (regardless of any subsequent mitigation), there's at least a slight chance someone got a hold of my email between when I signed up and when I managed to change it. An avoidable error that, though I realize I had warning signs for it, I also at least partly blame this site's developers and administration for. seriously, get with the times.

Comment 157

[cda]

How is this still a problem after being reported NEARLY TWO DECADES ago?

This is a basic feature of every normal online forum, to be able to hide one's personal email address. Not only that, but it should be the default behavior that it is hidden, rather than shown. I might not have even noticed it was being revealed if I hadn't hovered over my username just to see what happened.

I'll change my email address to a throwaway now, but I really shouldn't have to do that just to protect my privacy.

Comment 158

[Twisted Code]

(In reply to cda from comment #157)

How is this still a problem after being reported NEARLY TWO DECADES ago?

This is a basic feature of every normal online forum, to be able to hide one's personal email address. Not only that, but it should be the default behavior that it is hidden, rather than shown. I might not have even noticed it was being revealed if I hadn't hovered over my username just to see what happened.

I'll change my email address to a throwaway now, but I really shouldn't have to do that just to protect my privacy.

why? Insufficient project governance, if I had to take a guess.

to quote Andre Klapper from comment #3 of the bug I opened

Bug 218917 will be fixed in upstream Bugzilla 6.0. Upstream Bugzilla 6.0 has not yet been released.
The website bugzilla.mozilla.org uses some heavily customized code version (before non-existing "6.0", obviously).

I looked up this mysterious "Bugzilla 6.0"[*] to see if there was any talk of when it might be released (so that the site can start using that). Looks like they've been talking about releasing it for at least three years, and it's just plain stuck in limbo or something?

Perhaps when I'm feeling up to it, I will start a new ticket asking what it will take to AT LEAST get this fix applied to the website in the absence of 6.0... seeing as the site is already using "some heavily customized code version"?

[*]By the way, question for the veterans around here: what is the best way to differentiate "Bugzilla" the software from "Bugzilla" the website using it? I'm new around here and initially conflated the two of them.

Comment 159

[Tamas Hubai [:htamas]]

(In reply to Twisted Code from comment #158)

[*]By the way, question for the veterans around here: what is the best way to differentiate "Bugzilla" the software from "Bugzilla" the website using it? I'm new around here and initially conflated the two of them.

"Bugzilla" refers to the software in general, "bugzilla.mozilla.org" or BMO means this particular installation. You can classify a bug by looking at its Product field in the header.

This bug concerns Bugzilla the software where It was fixed 6 years ago in the sense that an administrator of a website can allow its users to set a non-email username. The fix is included in Bugzilla 5.1.1 which is a development release on the 6.0 track.

I guess you are more interested in BMO. A couple of corresponding bugs were linked in the comments above, the most relevant of them being bug 1372631.

Comment 160

[Twisted Code]

(In reply to Tamas Hubai [:htamas] from comment #159)

(In reply to Twisted Code from comment #158)

[*]By the way, question for the veterans around here: what is the best way to differentiate "Bugzilla" the software from "Bugzilla" the website using it? I'm new around here and initially conflated the two of them.

"Bugzilla" refers to the software in general, "bugzilla.mozilla.org" or BMO means this particular installation. You can classify a bug by looking at its Product field in the header.

This bug concerns Bugzilla the software where It was fixed 6 years ago in the sense that an administrator of a website can allow its users to set a non-email username. The fix is included in Bugzilla 5.1.1 which is a development release on the 6.0 track.

I guess you are more interested in BMO. A couple of corresponding bugs were linked in the comments above, the most relevant of them being bug 1372631.

yes. Respectfully, I couldn't care less about Bugzilla itself right now. I want to be able to login to BMO using github Oauth without my github email being broadcast to every Tom Dave and Harry (Yes I modernized that expression. No one calls anyone D*** anymore unless they mean it in a rude way) that happens to have an account on here. Currently, if I set my BMO email to something other than my github email, the integration breaks, and I really, truly do not believe the email should be shown on one's profile in the first place. That is so nineteen nineties, this isn't a and I'm frankly insulted (in a "you breached my (implied) trust" sort of way) that I wasn't told about this outdated behavior when I was signing in with github for the first time. Even signing up for a mailing list these days (Python's dev lists are good example) will warn you about your email being publicized... good grief.

Comment 161

[Twisted Code]

to amend my previous message: I meant to say "That behavior is so nineteen nineties, this isn't a mailing list, and I'm frankly insulted (in a "you breached my (implied) trust" sort of way) that I wasn't told about the behavior when I was signing up with github for the first time."
apparently I typo'ed and left out the phrase "mailing list". Why is there no edit feature?.....

Comment 162

[Anje]

As I understand it:
originally, it would seem the email address was automatically visible, so spam bots could easily 'read' the auto displayed email address.
The current auto display shows a username which the user can set preventing spam bots from gleaning email address.
As no email address is auto displayed as a username, the bug fix in that respect was resolved.

It does not prevent eg: myself from clicking on a username to then reveal and email address, but this bug was apparently not about that particular problem.

So, in that sense, it would need to be a new bug worded in such a way that it means the username can be set to be not clickable and therefore does not reveal email address of person. Preferably, there should be an option in profile to disable/enable the 'clickability' setting, so users can choose whether to allow email address visibility for other signed in users upon clicking on username.