Closed Bug 23067 Opened 25 years ago Closed 22 years ago

Should have some way to change the email account

Categories

(Bugzilla :: User Accounts, defect, P1)

Product:
Bugzilla
Component:
User Accounts
Version:
2.10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: hamfastgamgee, Assigned: jayvdb)

References

Details

(Whiteboard: Mail gerv@mozilla.org to change email accounts in the meantime)

Attachments

(2 files, 12 obsolete files)

Review for patch v.10 (needs-work) - Moz can't do big POSTs at the moment
8.89 KB, text/plain
Details
v12: complete patch, includes v11 + v11fixes with no other changes
31.32 KB, patch
justdave
: review+
justdave
: review+
Details | Diff | Splinter Review 
For those of us who switch email accounts on at least a semi-regular basis
(like, from home to school, from work to home, etc.), some way to change the
email address associated with a particular Bugzilla account would be exceedingly
helpful.  (Or, even more ideally, allow for generic user accounts from which a
person could add an arbitrary amount of email addresses as aliases to the one
user, but I'd love to at least have the ability to switch the associated email
address for an account.)
*** Bug 23065 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Priority: P3 → P2
The voting system shows that there is at least some sort of user id concept in
Bugzilla, so it seems to be moving in this direction.
It's not just the semi-regular e-mail address changes (You could, and should, 
conceivably have forward-files to send everything to your active account) but 
sometimes old addresses just come infeasible - change of job place, change of 
ISP etc. As it stands, if the e-mail address is reused as sometimes happens, the 
new owner will get spammed by change notifications (Unless they're turned off, 
but even then somebody could try to contact them based on their entries in 
Bugzilla). Luckily this hasn't happened to me, but I do have bugs under at least 
three different accounts on Bugzilla, two of which I'm forced to use actively 
due to firewalls. This makes "My Bugs" rather useless, and requires me to do 
three separate queries to find bugs I'm involved in. So yes, some sort of 
multi-email support (Deliver copies to multiple e-mails and/or search for 
entries under multiple e-mails) and hopefully ability to consolidate existing 
accounts into one or terminate/close them would be nice.
This is mostly do-able, with one big problem: the old email-notification system.

Right now, if I go in and tweak the database to change someone's email address,
then everyone using the old email-notification system will get lots of confusing
and pointless spam, as it helpfully notifies them that your address has changed
in every comment you've ever added to any bug.

Once I finish the new email-notification system, and get rid of the old one,
then I can finally do this kind of thing.
I don't necessarily think it's a problem - it could be useful to know someone
has changed their address.

The problem is that there could be many notifications that contain it, and
preventing this is discussed in bug #26943.
tara@tequilarista.org is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)
Assignee: terry → tara
Status: ASSIGNED → NEW
*Sigh* Another bug terry supposedly had a fix or an idea for a fix for.  yay.
Status: NEW → ASSIGNED
Any progress on this? My email address is going to die in a month or two.
Nathan, for now, you have to ask staff@mozilla.org (or post to .webtools), and
they'll make the change for you.
*** Bug 53930 has been marked as a duplicate of this bug. ***
I think you can go ahead and convert all email addresses to bugzilla account
names (in bug CC's, Reporter's, Assign To's, etc and implement accounts which
can have one or more email addresses and write a program to do a one-time
conversion in the database.  The conversion program would turn notifications off
during the conversion, then send an email to all users indicating the conversion
took place and how the new system works.
The account name could be <firstname>.<lastname> (case-insensitive).
There's really no reason for email addresses to be public.  Any communication
about bugs should be made in the bugzilla system so everyone has access to all
the discussion about a bug.  If there is a need for non-public comments, then
another enhancement could be made so that we can make "non-public" comments
which could only be viewed by priviledged accounts or "private" comments which
could be viewed only by the specified account name.

This should be on the 3.0-consideration radar. If we are dumping old email 
tech, it shouldn't be hard to have IDs implemented as integers, with email 
address as just another pref. 

Gerv
*** Bug 62483 has been marked as a duplicate of this bug. ***
*** Bug 67551 has been marked as a duplicate of this bug. ***
This should be possible for shortly after 2.12 comes out.  I think bug 66876 is 
the last thing to use e-mail addresses instead of DBID's.
Depends on: 66876
Whiteboard: 2.14
Whiteboard: 2.14 → 2.16
Whiteboard: 2.16 → 2.16, oldmail
Whiteboard: 2.16, oldmail → 2.16
Well, in 2.12 the admin can change this, we just need to expose it to the user.
Severity: normal → enhancement
OS: Windows 98 → All
QA Contact: matty
Hardware: PC → All
We just need something under "account preferences", under "your real name", 
right?

Thing is, if this can be changed, someone who finds your Bugzilla password can 
totally hijack your account...

Gerv
Don't give your password out :-)... (OK, hard, if the installation doesn't
support SSL. Bad mozilla.org :). )

Seriously, what do you suggest? The old account may be accessible.
I suggest something like this:
if the user requests an email change, the system should send an email to the old
email address confirming the change (and go ahead with the change).  If the
account was hijacked, the real user can reply to the email with "reset" command
and the system can automatically reset the password and restore the old email
address.
If a bounce is returned, the new email address is automatically confirmed.  If a
pre-determined expiration date (say, 90 days) has passed, the new email address
is automatically confirmed.  Same if the user at the old email address replied
with "confirm" command.
The system would have to keep all previous email addresses for 30 days (unless
the change is confirmed as described above) to prevent the hijacker from
switching email addresses a couple of times to avoid the real user from issuing
"reset" command.  The "reset" command would reset the email address to the
sender matching any of the previous emails.  When reset is done, all email
addresses set after that one are erased.  A reset can still be done from a
previous email address until the expiration date.
Does this make sense?

> Does this make sense?

Sure.  Is it easy to do?  Not really.  The bit about sending off an e-mail
reporting the change is trivial... It's all the other stuff (replying to the
message to reset it, etc.) that's difficult.  AFAIK, bugzilla currently doesn't
process incoming e-mails (there's stuff in the contrib directory for it, but
it's in not set up by default).
Having floated the worry, I'm now going to say that we should just live with
the possibility, and people can complain to the admin if someone nicks their 
account, and it can be nicked back.

I can't see why anyone would want to do so - unless that account had special 
privileges. Hmm.

Gerv
Bugzilla can do web inquiries though.  The email can have a link to click instead 
of replying to it.

This is what AOL does for free AOL Instant Messenger accounts:

Sends email to BOTH the new address and the old address with confirmation.  The 
email is not immediately changed.  Both addresses are given 72 hours to reply.  
The new address has to reply to it to say "yes I want to be receiving email at 
this address".  The old address has the option to reply and say "I didn't 
authorize this, abort it".

In order for the change to happen, the new email address has to reply to it, and 
no replies must have been received from the old email address by the time that 72 
hour window expires.  If neither address replies, the change aborts.  If the old 
address replies, the change aborts.  If the new address DOESN'T reply, the change 
aborts.

I think it would be relatively easy for Bugzilla to implement something like this 
if the users were given token numbers with links to click in the email to reply 
to it instead of sending physical replies.  The token numbers would be randomly 
generated and stored similar to how the login cookies are stored currently, along 
with the data that was requested to be changed.  The old address and the new 
address would get different token numbers, to prevent them from being able to 
easily masquerade as the opposite address when clicking the link.  They would 
obviously be tied by the userID in the token records.  Instead of installing Yet 
Another Cron Job to check when the end of the 72 hours has ocurred, the check 
could be hacked to run any time the versioncache file is updated (which ocurrs 
once and hour or once per Bugzilla access, whichever is longer).

I don't consider it a major problem anyway.  We need the feature to change our
email address more than we need to verify the change in case someone gets your
bugzilla password.  If a user had a problem, they can always get another account
and report the problem to get their old account back.  I will need this feature
real soon, so let's get it in!

moving to real milestones...
Target Milestone: --- → Bugzilla 2.16
*** Bug 72169 has been marked as a duplicate of this bug. ***
Depends on: 77473
there's been a lot of discussion (and set up) for this in bug 77473.
Priority: P2 → P1
Whiteboard: 2.16
*** Bug 89215 has been marked as a duplicate of this bug. ***
*** Bug 93912 has been marked as a duplicate of this bug. ***
Component: Bugzilla → User Accounts
Product: Webtools → Bugzilla
Version: other → 2.10
Dave Miller's AIM-like proposal looks good. Don Beusee's proposal, however, has 
problems--if somebody's email account was deactivated (like my old one was--
thanks to Asa for changing my bugzilla acct. to my new email!) then they 
wouldn't be able to change the email for their bugzilla account.
*** Bug 102400 has been marked as a duplicate of this bug. ***
Severity: enhancement → blocker
This is going to be a blocker for the 2.16 release
*** Bug 108643 has been marked as a duplicate of this bug. ***
taking...
Assignee: tara → justdave
Status: ASSIGNED → NEW
I think we should do this as follows:
If you are logged in, you can change the email name. The change is effective
immediately. It's as simple as that. Email is sent to both accounts as a quick
safety measure.

Why does it have to be more complicated? If someone gets your Bugzilla password,
then they can do all sorts of things other than changing the login. For example,
they can change the password on you, or (if you have admin privs.) alter the
params etc. Anyway, if they make the change, you'll get email about it and you
can complain to the admins.

This scheme is simple and as secure as needs be. We don't need to mess with
tokens and stuff.

It can be a single edit box in the user preferences.

Gerv
They get an email saying their email was changed.  They're out of town for a
couple days at the time.  They get back and find this email.  They complain, it
gets fixed.

Meanwhile, they've lost three days worth of bugmail.
Right. This situation assumes that: a) someone has stolen their Bugzilla
password, and b) they aren't around to notice, and the loss is 3 days worth of
bugmail. Hardly a likely scenario (have you _ever_ heard of any situation where
a Bugzilla password has been stolen?) and hardly a big loss.

Gerv
*** Bug 111588 has been marked as a duplicate of this bug. ***
Some additional reasons for changing email:

My employer changed from initials to first.last-name, but fortunately still
accepts the initials.  That might go sometime, leaving the account stranded.

My employer has introduced a condition of service that means that every comment
I make really ought to be followed by about six lines of signature and 15 lines
of legal notice, to which one would have to add a disclaimer for the disclaimer,
to say that the material wasn't really confidential and for a named individual
only.  Especially as most feedback on Mozilla is probably not strictly work
related, it would be easier to switch to my home email address.  (I think I will
create a new  account under my home address, but that may introduce a problem of
merging if email changing does become possible.)

There has also been talk recently of a major cable internet company going out of
business and stranding large numbers of email addresses.
David: it's already possible to change email by mailing the admins; this bug is
merely about making the process easier and quicker for users.

Gerv
Comment on attachment 61881 [details] [diff] [review]
Adds email address input to userprefs; emails both addresses on valid change

Very good patch, thanks for submitting it. However, the problem with this
design is if someone does it maliciously to hijack an account, the original
owner effectively gets locked out until they can get ahold of the
administrator, and the new user now has free reign of the original person's
privs without any capability of the original user to change the password to
stop them if they also changed the password at the same time.
Attachment #61881 - Flags: review-
The real reason for requiring a token to be mailed here, besides verifying the
user really wants the change, is it verifies that the email address actually
works.  We already have this mechanism in place for creating accounts, since the
password is mailed to you when you create it.  Thus if you put in a bad email
address, you're never able to log in with it, because you never get the password.
The way I currently authenticate this, when people ask me to do it, is to email
both accounts a notification that I've changed it. This seems to work fine in
practice - why do we need higher security than that already in place?

Gerv
Just don't get too crafty on trying to create this. Some people just wants to do
this simply because their isp has changed. I don't see what is wrong with
placing this in the userprefs in bugzilla as theres not much security detail on
getting it changed anyways.

When I was trying to get my email address changed, all they did was ask from
which to which and it was over with. If anyone wanted to be malicious, it would
have been there. At least in userprefs, a password is required to get in.
Comment on attachment 61881 [details] [diff] [review]
Adds email address input to userprefs; emails both addresses on valid change

revoking the needs-work.  If this works, and since b.m.o is obviously the place
that needs it the worst, and Gerv is willing to live with it, I suppose this
will at least be a good temporary solution.  We can always file a new bug to
make it more secure later if we need to.
Attachment #61881 - Flags: review-
Sorry, but I cannot in good faith give this a positive review.

We need to authenticate *at the very least* the new e-mail address.  Currently,
our CheckEmailSyntax only checks for illegal characters.  There are many e-mail
addresses that are invalid for our purposes.  I surely would not want someone to
think they are funny and change their e-mail as a prank to
'president@whitehouse.gov' or 'root' or even someone they know for example. 
This allows abuse of the mail system and corruption of the database with bad
e-mail addresses (I am not saying it will happen, just allowing for it).

We do desparately need this for Bugzilla, but not this desparately.  Let's do
this The Right Way(tm).  If we let this patch slip in, we render our
registration emailing moot.  For then someone can register and swap addresses
unhindered.
Comment on attachment 61881 [details] [diff] [review]
Adds email address input to userprefs; emails both addresses on valid change

I agree with caillon. given bugzilla.mozilla.org user tendencies this is quite
likely to be abused. It looks like i could become timeless@mozilla.org with
this mechanism.

And, given that i don't really read the email sent to timeless@bemail.org, i
might be quite likely to make this change.

otoh, perhaps there's nothing wrong with bugzilla addresses not being live
email addresses.  Netbeans's Issuezilla in conjunction with collabnet
sourcecast creates special usernames which turn out to be real email addresses
but which aren't as bound to the user's email address. (I think i'm timelessmac
or something strange there).

If we intend to make a change that would allow me to capture
timeless@mozilla.org, then the bug summary needs to clearly indicate this.
Attachment #61881 - Flags: review-
Well if authentification is what you guys want, why you don't you adopt a little
coding idea DALnet IRC Networks uses. For new accounts, you would be emailed an
authorization ID you must imput to their services so that your email address is
verified.

This authorization ID is just a huge random number selection that I know of
using UNIXTIME * randomnumber + randomnumber. Its easy to copy and paste these
things now a days, or even have some type of hyperlink thing so it'll do it for
you, you just have to enter your email address and password.

If this authentication is done in 3 days or so, the account is dropped.

You can adopt this idea for email changes too, using the exact same idea, except
just revert to the old email address if it has not been authenticated after the
3 days.

There is also an implementation so that the same email address doesn't get
authentications notice within a 24hour period. 

Ideas? Suggestions?
i was going to suggest borrowing a mechanism from the new reset password alg. i 
don't actually know how it works, but...

The requirement would be that you'd have to have your login token and provide 
the rename token.

well, alternatively i'm perfectly happy with the patch applied to 
bugzilla.mozilla.org so long as i can get timeless@mozilla.org, but i'm pretty 
sure most people would object to that.
Timeless ... is the new reset password algorithm in the tree, or could you give
a bug number where the patch is?

I dont like the idea of having to wait three days for a user to active the new
address (via email token), as a user might have mispelt the new email address,
in which case they would want to continue using the old email address for long
enough to change it again.  It also complicates the solution.  If we leave the
old email address as active until they use the token, this allows the solution
to be an email with token, and an atomic change on use of the token.  If we have
de-activated users anywhere during this process, a 'account [de-]activation' GUI
for admins will need to be created to fix any problems.

To prevent hi-jacking accounts would require that the old email account has a
greater opportunity to respond than that of the new email account, in order to
be fair.  This could be achieved by not accepting the token for a specified
period of time, however this would then mean that in the legitimate case, the
user would have to continue using their old email address for that period (and
possibly not be able to recieve mail).

If we are going to have Bugzilla accept a token, should a new .cgi be used, or
can we have it go to userprefs.cgi.
You all should go back and read comment 22.  The infrastructure is in place with
the current password reset tokens to implement exactly that (except the response
if via clicking a link, since Bugzilla can't handle incoming email well at the
moment).  Some of your suggestions in the last few comments (all of you) are
duplicating things we already planned on, and were just waiting to find someone
with the time to actually implement.

The password reset token structure was committed with the patch on bug 77473.
Thanks for pointer Dave.
i don't think that's satisfactory.

the new account should be required to login. i'm not 100% certain your code 
doesn't but a quick scan doesn't indicate that it does.

the reason for this stipulation should be pretty simple.  as a user w/ a 
bugzilla token, i shouldn't be concerned that i could visit a random page which 
redirects me to the bugzilla change account page to some email address i don't 
control which would then automatically connect (not knowning the 
account password) and then run a password change.
This patch still does not require the user to login, however it does require
that the user is able to provide the old/new address depending on whether the
token was sent to the new/old (respectively) addresses.

I have tried to stay in line with:
http://bugzilla.mozilla.org/show_bug.cgi?id=77473#c23 & c24
I should point out that the hijacker knowing the old email address is irrelavent
as he can get the email addresses through bugzilla anyways. All one has to do is
put the pointer over someones name and you can see the mailto: address.
Something has to go, the mailto: links or the knowing the old email address.
The mailto: links should go :)

Irrespective of this, when the email address change is requested from userprefs,
the real user is required to specify the new email address, enter their old
passwd, and a new password twice.  The hi-jacker, after finding out old email
address via mailto:'s, can only confirm that the new address specified by the
real user should be committed.  The real user still has three days to revert the
change with the token sent to the old address (in the case they mistyped the new
email address).

Requiring the holder of new token to enter the old email address is currently
only a precautionary measure.  However to solve the problem here, the new email
address could be committed immediately, and a cron job automaticually reverting
the changes after a period of no confirmation.  I had hoped to avoid cron jobs.

Here are the relavent scenarios from bug 77473:

3. A user wants to change email addresses and still has access to her old
address.  She goes to her user preferences, enters her new address, and submits
the form.  Her record in the "profiles" table is NOT updated with her new
address, two token records are created in the "tokens" table, and emails are
sent to both her old and new addresses, each email containing one of the tokens
along with links for confirming and cancelling the request.  She receives the
emails and clicks the links to confirm the request.  The email address is
updated and the both token records (current token + 'emailnew') are deleted from
the "tokens" table.

3. A user wants to change email addresses and no longer has access to her old
address.  She goes to her user preferences, enters her new address, and submits
the form.  Her record in the "profiles" table is NOT updated with her new
address, two token records are created in the "tokens" table, and emails are
sent to both her old and new addresses, each email containing one of the tokens
along with links for confirming and cancelling the request.  She receives the
email at her new address and clicks the link to confirm the request.  The
corresponding token record (current token = 'emailnew') is deleted from the
"tokens" table.  After a specified period of time, subsequent use of tokens.cgi
removes the 'emailold' token record from the "tokens" table.

5. A malicious hacker gains access to a user's account and changes her email
address to his own address.  The user's record in the "profiles" table is NOT
updated with the new address, two token records are created in the "tokens"
table, and emails are sent to both the user's and the hacker's address, each
email containing one of the tokens along with links for confirming and
cancelling the request.  The hacker receives an email and clicks the link to
confirm the request.  The email address is changed in the profiles table, and
corresponding token record (current token = 'emailnew') is deleted from the
"tokens" table.  The user receives the email and clicks the link to cancel the
request.  Her email address reverts to her old address (embedded into the
eventdata), and the corresponding token record is deleted from the "tokens"
table, and the system administrator is NOT notified (oops...will add once
everyone is happy with the rest).

4. A real user wants to change email addresses and no longer has access to her
old address, however they click Submit as they realise they have mispelt the new
address.  The user's record in the "profiles" table is NOT updated with the new
address, two token records are created in the "tokens" table, and emails are
sent to both the user's and the hacker's address, each email containing one of
the tokens along with links for confirming and cancelling the request.  As
neither will be confirmed or cancelled, both tokens will expire (on subsequent
use of the tokens.cgi), and the user continues to use the same email address as
before.

If the new email address was not confirmed before changed, the user would be
required to use the new email address to log on until the tokens expired, after
which they would have to revert to using the old email address, with no access
to the emails which would inform them of the login name having been reverted. 
Dumb users = Abused admins.


any mechanism that allows me to become timeless@mozilla.org is probably 
unacceptable to bugzilla.mozilla.org. And any mechanism that allows someone 
else to become me w/o providing my password is unacceptable to me.
I think if the user at the new address confirms, and the user at the old address
hasn't responded yet, the address should NOT change until a period of time has
passed (24 hours?  72 hours? whatever).  With the system as given above, the new
email address would still get email from the account temporarily before the old
user could revert it.  If you no longer have access to the old address, the
delay is an unfortunate side effect (and enforced on many other services, so
this isn't a new concept for most people).
so a hacker could victimize people who don't check bugmail frequently?  i see 
no reason not to require users to have the old account info 
(username+password).
I dunno guys. Theres no security now when it comes to changing email addresses
in bugzilla as it is. I like the idea of being able to cancel changes within x
hours, but lets not go overboard with this hijacking thing.

As it is, anyone can hijack anything in bugzilla as there is no security
encryption for the userprefs as is. If we are going to worry about hijackers
then can we just file other bugs for that. This one is just to get email changes
in bugzilla, instead of me being able to change them through irc without any
questions asked.

I understand that there is a need for security, but why have security when we
are sending things around in raw text. Besides, I think a hijacker would spend
his time trying to create the latest and greatest virus of all times than trying
to disrupt 200 bugzilla accounts to attempt to spam 200 unknown people around
the world.

*watches bugzilla filter this post among 100+ cc'ers*

*points out how funny timeless@mac.com is really timeless@bemail.org*
To ensure that noone can become timeless@mozilla.org, the old email address
should not be able to not confirm, which is closer to how the new accounts work.

"And any mechanism that allows someone 
else to become me w/o providing my password is unacceptable to me."

There are two ways this is possible.
1) Assume that Mr. Malicious has been able to login to your account, used
userprefs to select their email address, having to change the pasword in the
process, and blocked your email address.  
2) Dumb user logs into their account and changes their email address to one that
is used by Mr Malicious.

In both cases the person performing the change already has the password, or
worse: has changed the password.  So it does not matter if a password is used or
not; the contention is whether it should be possible to change an email address
over an insecure network.  However Bugzilla currently allows this, as any
account which has priveledge 'editusers' could be hijacked using the same
assumptions, and used to change email addresses.  

Besides, over SSL this is not a problem, so perhaps a Release Note would be more
appropriate, informing non SSL installations of the implications on enabling
this.  And perhaps another bug created to direct userprefs, editusers and
editparams to use a SSL connection.  This would allow a smooth path for non-SSL
installations to migrate to SSL.
I think the easist solution is to have an email to both new and old accounts, as
many people pointed out.

New email:

Some type of confimation email. "You have sucessfully change your email from
blah@mozilla.org to blah@somecrappyisp.com. If you believe this email doesn't
belong to you click here blah blha blah blah blah."

Old email:

Some type of notification email. "Please be aware that email forwarding for
bugzilla has been changed from blah@mozilla.org to blah@somecrappyisp.com. If
this was unintented change (typo etc) then click here. If you believe someone
has somehow gained access to your account, email
somereallysmartadmin@mozilla.org to keep track of your account blah blah blah blah."

But I'm saying this, you are begging for hijackers if there is no SSL
implementation and if you don't remove those mailto links. To me, without this
implementation, its like posting my visa # and credentials to a mailing list.

But enough about this, can we just please fix our summary problem and then file
security ideas for this bug as new ones? Either that, or elaborate the summary
so it includes security as a focus. Because right now we are just beating around
the bush with this fix.

ok, i just noticed (bad me to not try examining the output of the patch) that 
you're changing the first panel, which indeed does require the user to enter 
the password.
Changes:
1) Old address can not confirm (default email change and restricted thru code).

2) Uses email templates in template/default/email, including emailchangeold
(sent to old address), emailchangenew (sent to new address), and tokencancel
(sent when Token::Cancel is called).  There is still one hard coded email
within Token.pm, which is sent from MailPasswordToken.
3) Param to control this behaviour renamed from 'allowuserchange' to
'allowemailchange'

This patch purposely leaves out emailing the administrator itself, instead
using Token::Cancel which should probably be altered further to email the
administrator.
(http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/Token.pm#124)
I know this sounds like an RFE, but I think we should kill it in this bug.  Can
we add an |X-Priority: 2| header (or 1) to the message sent to the old address?
 For users who actually use the priority (me), I'd rather it have a higher
priority so I can read it more quickly in the event someone is trying to hijack
my account.  Also, I'm less likely to delete without reading a high priority
mail unless it is obvious spam.  I might just mass delete a few thousand
bugzilla mails without reading them.
The email sent to the old address is now heavy-laden with priority flags to
appease a wide range of email clients.
Email templates are now configured with TRIM=1 rather than POST_CHOMP=1, which
required additional linefeeds after variables in the template.
That sounds good to me! (I just realized I'm changing ISPs and need to change my
address) :-)

adding patch and review keywords
Keywords: patch, review
Just a note: we've been using PRE_CHOMP in the HTML templates, which seems to
work quite well.

Gerv
Why did you use $::tokentype instead of $tokentype? We're trying to move away
from global variable names, sort of (for eventual mod_perl support).

I'll test this out later.
I was following the current coding style, and keeping the number of queries
down, without cluttering the code with lots of argument passing.  will fix with
other review points after you have tested it.
I'd prefer the argument passing....

Anyway, apart from that it looks fine. Maybe call the sub in Token.pm
IssueEmailChangeToken, instead?

From the implementation point of view, I'd prefer that both emails acknowledge
the change. majordoomo requires this. In the case where the old email addr is no
longer accessible, they'll have to mail the admin - some text should be added
somewhere to confirm that. Most people tend to know about these things in
advance, though. That way you can drop the priority headers. Maybe this sort of
mail should come from a different address than bugzilla-daemon@mozilla.org, so
that it isn't sorted automatically?
Just to point out, if you do an address change from the web interface in
Majordomo 2, only the new address has to confirm.  The old address is considered
confirmed already at that point, since they had to enter a password to log into
the web interface.  Majordomo 2 only sends a confirmation to the old address if
the address change request was received via email (since email can be forged
easily).  Since we're putting the interface for this on the prefs page, which
they have to be logged in to see, them entering the password would be sufficient
to confirm the old address I'd think.  In the case of someone's computer being
left alone (since the cookies are persistant) it should probably be on the
password page so they have to enter the password on that page whether they're
logged in or not in order to do it.
Ah, OK. I missed that. You can ignroe that point, then ;)
Comment on attachment 63087 [details] [diff] [review]
v5: adds email priority, cleaner templates

In general, try to break long lines (greater than 80 characters in length)
into multi-line statements.


>+    $template->process("email/emailchangeold.tmpl", $vars, "/tmp/bugmail.$$")
>+      || &::DisplayError("Template process failed: " . $template->error())
>+      && exit;
>+
>+    system("/usr/lib/sendmail -t < /tmp/bugmail.$$");
>+    unlink "/tmp/bugmail.$$";

Avoid using a temporary file in these situations (creating an additional
point of failure, a Unix filesystem dependency, and mod_perl session ID
problems) by doing this instead:

    my $message;
    $template->process("email/emailchangeold.tmpl", $vars, \$message)
      || &::DisplayError("Template process failed: " . $template->error())
      && exit;

    open SENDMAIL, "|/usr/lib/sendmail -t";
    print SENDMAIL $message;
    close SENDMAIL;


>   # Make sure the token exists in the database.
>   SendSQL( "SELECT tokentype FROM tokens WHERE token = $::quotedtoken" );
>-  (my $tokentype = FetchSQLData())
>-    || DisplayError("The token you submitted does not exist.")
>+  $::tokentype = FetchSQLData()
>+    || DisplayError("The token you submitted does not exist or has expired.")
>     && exit;
> 
>   # Make sure the token is the correct type for the action being taken.
>-  if ( grep($::action eq $_ , qw(cfmpw cxlpw chgpw)) && $tokentype ne 'password' ) {
>+  if ( grep($::action eq $_ , qw(cfmpw cxlpw chgpw)) && $::tokentype ne 'password' ) {

I second bbaetz's comment: the $tokentype variable should be local.  
Also, this code should be updated with the new actions you added: 
cfmem, cxlem, and chgem.


>+sub confirmChangeEmail {
>+    # Return HTTP response headers.
>+    if($::tokentype eq "emailold") {
>+        DisplayError("Please use the token sent to the new email address");
>+        exit;
>+    }
> 
>+    print "Content-Type: text/html\n\n";

Move the comment "#Return HTTP response headers." below the error checking
code, since it is intended to explain the 'print "Content-Type...' statement.


>+    PutHeader("Confirm Email Change");
>+    print qq|
>+      <p>
>+      To change your email address, please enter the old email address:
>+      </p>
>+      <form method="post" action="token.cgi">
>+        <input type="hidden" name="t" value="$::token">
>+        <input type="hidden" name="a" value="chgem">
>+        <table>
>+          <tr>
>+            <th align="right">Old Email Address:</th>
>+            <td><input type="input" name="email" size="36"></td>
>+          </tr>
>+          <tr>
>+            <th align="right">&nbsp;</th>
>+            <td><input type="submit" value="Submit"></td>
>+          </tr>
>+        </table>
>+      </form>
>+   |;
>+   PutFooter();

New display code should be templatized.


>+    if($::tokentype eq "emailold") {
>+        PutHeader("Confirm Email Change");
>+        DisplayError("Please use the token sent to the new email address.");

Stray PutHeader call?  PutHeader isn't needed before DisplayError.


>+    # Update the user's password in the profiles table and delete the token
>+    # from the tokens table.

password -> login name


>+    # Return HTTP response headers.
>+    print "Content-Type: text/html\n\n";
>+
>+    # Let the user know their email address has been changed.
>+    PutHeader("Email Address Changed");
>+    print qq|
>+      <p>
>+      Your email address has been changed.
>+      </p>
>+    |;
>+    PutFooter();

Look in the patch for bug 103778 for the "message.html.tmpl" template,
which could be used to display messages like this one.


>+sub cancelChangeEmail {
>+    # Get the user's ID from the tokens table.
>+    SendSQL("SELECT userid, eventdata FROM tokens WHERE token = $::quotedtoken");
>+    my ($userid, $eventdata) = FetchSQLData();
>+    my ($old_email, $new_email) = split(/:/,$eventdata);
>+    
>+    # Return HTTP response headers.
>+    print "Content-Type: text/html\n\n";
>+
>+    PutHeader("Cancel Request to Change Email Address");

Return HTTP response headers and the results of the request (including
the PutHeader section, which can go away in the templatization process) 
after processing is completed.


>+            if (DBname_to_id($old_email) != 0) {
>+                DisplayError("Account $old_email already exists");
>+                Token::Cancel($::token, "old email address requested email change cancellation - old email address unavailable");

Seems like it would make sense to refuse to let a new account be created
at the old address until the three day period was over.


> sub ShowAccount {
>+    if(Param("allowemailchange")) {
>+        SendSQL("SELECT login_name FROM profiles WHERE userid = $userid");
>+        my ($login_name) = (FetchSQLData());

Because userprefs.cgi calls confirm_login(), which calls quietly_check_login(),
which sets $::COOKIE{"Bugzilla_login"} to the user's login name (admittedly
a poor name for that variable), you don't need to execute this SQL statement
to get the user's login_name.  Just use $::COOKIE{"Bugzilla_login"} instead.

Overall, looks good and will be a welcome addition to Bugzilla!  Thanks for
all your hard work on this; I look forward to reviewing the next version.
Attachment #63087 - Flags: review-
*** Bug 122568 has been marked as a duplicate of this bug. ***
Attached patch v6: review fixes (obsolete) — — Splinter Review 
Sorry for taking so long submitting this update.
As well as the review points:
1) fixes an assumption in Token:: HasPasswordToken which resulted in reverting
an account not working if the new address logged in successfully after
confirming the change.	
2) Renamed templates to align with new naming convention of cgi/*.format.tmpl.
3) Moved validation of a new account's email into a function, as the logic now
needs to include a check in the token table (as per myk's comments), and is
needed in three separate files to ensure an old address is not taken, thus
being able to be reverted.
*** Bug 123199 has been marked as a duplicate of this bug. ***
*** Bug 123450 has been marked as a duplicate of this bug. ***
Whiteboard: Mail gerv@mozilla.org to change email accounts in the meantime
Comment on attachment 67383 [details] [diff] [review]
v6: review fixes

You shouldn't create a new template object - use use vars instead.

Should the param default to on? And in the SQL in ValidateNewUser can you use
SQL to do the partial match? That may not be supported in the versions of mysql
we have to support, though.
Attachment #67383 - Flags: review-
>You shouldn't create a new template object - use use vars instead.

Just to clarify: I should use a global variable for 'template' ?  (I ask due to
comment #69)

Given that the current 'email change' functionality requires the administrators
approval, I think the administrator should have to explicitly turn this on to
ensure it actually improves their process and meets their user security
requirements.

Partial matching is already in the code base I think (see userpref.cgi:302).
No longer blocks: 98818
Could someone familiar with the template work please comment on what bbaetz
meant in comment 79?  The patch author is confused and so am I. :-)

See my patch on bug 100094 for examples of what I mean. Basically, instead of doing:

my $template = new Template...

my $vars = { ... };

Do:

use vars qw($template $vars); which will bring the global vars into the local
namespace.

Then just use $template and $vars as if you'd already created them. The default
$vars includes the two functions you're using here, so you don't need to add
them manually.

The advantage is that you move all the init code into one common place so that
it can be easily changed, and you avoid the extra template creation overhead
when using mod_perl (yes, I know we don't support that yet)
I imagine this is going to conflict with bug #117060.
Attached patch v7: review fixes (obsolete) — — Splinter Review 
Fixes:
1) updates sendmail usage
2) updates ValidateNewUser to use partial matching, and verify new email
address is not taken by new user before token is used.
3) userpref change email submission now checks for competing changes of email
address using ValidateNewUser
4) added restriction of one outstanding change email request to prevent the
Tokens table from filling up, thus slowing down logins.
5) editusers uses ValidateNewUser.
6) param defaults to on
7) uses global vars for template/vars
From attachment 69169 [details] [diff] [review] (v7: review fixes)
> +    open SENDMAIL, "|/usr/lib/sendmail -ti";
This clashes with Exim. Use -t -i instead. See bug 125516.
Comment on attachment 69169 [details] [diff] [review]
v7: review fixes

marking needs-work on behalf of Tobias, I agree.
Attachment #69169 - Flags: review-
Attached patch v8: update (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 70277 [details] [diff] [review]
v8: update

Tried this out, and it appears that I need to change my password at the same
time or it fails.  I know there's an exception to the password change required
if you change your real name... the same exception should be applied to the
email change as well.

As long as we're in there, it should probably state on the page that entering
your old password is required to change any of the fields on that page.

        Attachment #70277 -
        Flags: review-

    
    

    
  
That restriction is removed in the templatised version of userprefs.cgi (bug
117060). Perhaps reviewing that and checking it in ASAP would be helpful in
progressing this bug.

Gerv
Keywords: patch, 
          
            review

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v9: update + review fixes (obsolete)
        — 
          — Splinter Review
      

    


  
This patch addresses the need to change password when changing other account
settings, and UI clearly states the old password is required.  To assist the
user thru the process, userprefs.cgi now displays the current status of an
email change in progress.

    
    

    
  
Comment on attachment 72195 [details] [diff] [review]
v9: update + review fixes

Tested and works for the most part except that isn't the new address 
supposed to be able to confirm the request?  Also, when I cancelled 
the request from the new address Bugzilla told me that the old address 
had cancelled the request.

General Remarks:

1. Use "txt" as the file extension for email message templates to accommodate
future work implementing HTML versions of email messages (which will
probably use some variation of the formatting code in the patch on bug 103778
along with factoring out the message headers into a separate template
in the globals/ directory that can generate message headers for multiple
email templates).


2. If the user enters their password incorrectly while attempting to change
their email address, Bugzilla displays a misleading message about the user's
"username or password" being incorrect.  It should only display a message
about their password being incorrect.


3. "use vars { $template $vars }" makes those two variables available
everywhere in the file, so the following code is redundant:

+    my $template = $::template;
+    my $vars = $::vars;


4. Some of the messages displayed by the code aren't clear.  I realize you
modeled them after existing messages for which I am responsible, and I
apologize for providing such poor role models.	In any case, here are some
better alternatives:

Message sent to old email address: 
  Bugzilla has received a request to change the email address 
  for your account to [new email address].

Message sent to new email address: 
  Bugzilla has received a request to change the email address 
  for the [old email address] account to your address.

Message displayed when request is cancelled by old address: 
  The request to change the email address for your account 
  to [new email address] has been cancelled.

Message displayed when request is cancelled by new address: 
  The request to change the email address for the [old email address] 
  account to [new email address] has been cancelled.


Code-Specific Remarks:

>+++ bugzilla-email/CGI.pl	Sat Mar  2 12:08:18 2002

>@@ -803,6 +803,13 @@

>+                print "Content-Type: text/html\n\n";
>+                DisplayError("Account Exists");

DisplayError generates its own "Content-Type" header, so the one
printed here is redundant.


>+++ bugzilla-email/Token.pm	Sat Mar  2 13:38:14 2002

>@@ -175,10 +246,27 @@

>+sub HasEmailChangeToken {
>+    # Returns an email change token if the user has one. 
>+    # Otherwise returns 0 (false).

Nit: this function returns the undefined value if the user
does not have an email change token.


>+++ bugzilla-email/globals.pl	Sat Mar  2 13:24:44 2002

>@@ -668,6 +670,31 @@
> }
> 
> 
>+# Validates a given username as a new username
>+# returns 1 if valid, 0 if in-valid

Nit: "invalid" is not hyphenated.


>+sub ValidateNewUser {
>+    my ($username, $old_username) = (@_);

Nit: list context is determined by parentheses to the left of the
equivalence operator (=), so the parentheses around @_ are redundant.


>+++ bugzilla-email/defparams.pl	Thu Feb 28 11:07:30 2002

>@@ -593,6 +593,12 @@

>+DefParam("allowemailchange",
>+         q{Users can change their own email address through the preferences.},
>+         "b",
>+         1);

Turn this parameter off by default per comment 79 and comment 80.


>+++ bugzilla-email/token.cgi	Sat Mar  2 14:10:11 2002

>@@ -242,6 +272,122 @@

>+sub changeEmail {
>+
>+    # Quote the password and token for inclusion into SQL statements.
>+    my $email = $::FORM{'email'};

Wrong comment.


>+++ bugzilla-email/userprefs.cgi	Sat Mar  2 13:47:34 2002

>@@ -89,23 +104,65 @@

>+        if ($::FORM{'new_password1'} ne "" || $::FORM{'new_password2'} ne "")
>+        {
>+            my $pwd1 = SqlQuote($::FORM{'new_password1'});
>+            my $pwd2 = SqlQuote($::FORM{'new_password2'});
>+            if ($pwd1 ne $pwd2) {
>+                DisplayError("The two passwords you entered did not match.");
>+                exit;
>+            }

Escaping the two passwords for use in an SQL query is unnecessary here,
since neither escaped value is ever used in a query (in fact the only 
place the values are used is in the following comparison, which would work
equally well without the SQL escaping).  Don't bother escaping the strings,
just compare them to each other.

        Attachment #72195 -
        Flags: review-

    
    

    
  
> 2. If the user enters their password incorrectly while attempting to change
> their email address, Bugzilla displays a misleading message about the user's
> "username or password" being incorrect.  It should only display a message
> about their password being incorrect.

This is printed by the standard confirm_login code, since thats the bit which
checks the username/password match. See bug 45918.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v10: review fixes (obsolete)
        — 
          — Splinter Review
      

    


  
> Tested and works for the most part except that isn't the new address 
> supposed to be able to confirm the request?

That is what is supposed to happen, however the old address is not able to
confirm the change.

    
    

    
  
>This is printed by the standard confirm_login code, since thats the bit which
>checks the username/password match. See bug 45918.

Ok, but it's still a bug, since the message is misleading.  Nevertheless, it was
the fix for bug 45918 that introduced it, not this fix, so I filed bug 129254 on
it, and you can ignore my review comment about this.


>> Tested and works for the most part except that isn't the new address 
>> supposed to be able to confirm the request?
>
>That is what is supposed to happen, however the old address is not able to
>confirm the change.

My mistake.  As I see now, this works correctly.

The code looks good; the only things left are a few more UI issues:

1. When a user requests an email address change, they should not see the message
"The changes to your account settings have been saved," since it is misleading.

2. When a user requests an email address change, they should be told how to use
the emails sent to their addresses to confirm or cancel the change.  Something
like this would work (bold text enclosed within asterisks):

Messages have been sent to your old and new email addresses requesting
confirmation for this change.  To confirm the change, visit the "confirm" URL
included in the message sent to your *new* address.  If you made a mistake and
would like to cancel the change, visit the "cancel" URL included in either message.

3. After a user confirms the change, it isn't clear what the "completion date"
field on the "account settings" page means.  There needs to be some explanatory
information on that page so users know what the field is there for, something
like this:

Your email address change has been completed, and Bugzilla now uses the new
address in place of the old one.  In order to deter account hijacking, however,
the old address will continue to have the option to reverse the change (via the
"cancel" URL in the message sent to that address) until the time shown.

    
    

    
  
> 1. When a user requests an email address change, they should not see the message
> "The changes to your account settings have been saved," since it is misleading.

The user may have also changed their password and/or real name, in which case
not having this message would also be misleading, and definately inconsistent.

Rather than placing the email change details in the table (as in v10), the user
messages you requested in points 2 & 3 could be placed in a section labeled
"Email change status" after the table?
The message "The changes to your account settings have been saved" could then be
retained as it would be only to ensure the user is confident something did
occur, after which perusal of the page would provide further directions/more
information.

    
    

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

    
    

    
  
Comment on attachment 72766 [details] [diff] [review]
v10: review fixes

>The user may have also changed their password and/or real name, in which case
>not having this message would also be misleading, and definately inconsistent.

Right, so both messages should be shown if the user changes their password or
real name and their email address at the same time.  If, however, the user
changes only one of these, only the message appropriate to their change should
be shown.


>Rather than placing the email change details in the table (as in v10), the user
>messages you requested in points 2 & 3 could be placed in a section labeled
>"Email change status" after the table?

Sounds reasonable to me.

--

This patch addresses all my concerns except for these polish issues, and it
doesn't make sense to hold up this patch until those are resolved, so r=myk,
but file a separate bug that addresses these issues.

        Attachment #72766 -
        Flags: review+

    
    

    
  
Comment on attachment 72766 [details] [diff] [review]
v10: review fixes

>+use vars qw(
>+    $template
>+    $vars
>+);
>+
>+# globals.pl is required otherwise $template and $vars are undefined.
>+require "globals.pl";
>+
> # Bundle the functions in this file together into the "Token" package.
> package Token;
> 
>+my $template = $::template;
>+my $vars = $::vars;
>+

The correct way to do the template variables is:

>+require "globals.pl";
>+
>+use vars qw($template $vars);

That's all you need.

>+sub IssueEmailChangeToken {
>+    my ($userid, $old_email, $new_email) = @_;
>+
>+    # Generate a unique token and insert it into the tokens table.
>+    # We have to lock the tokens table before generating the token, 
>+    # since the database must be queried for token uniqueness.
>+    &::SendSQL("LOCK TABLES tokens WRITE");
>+    my $token = GenerateUniqueToken();
>+    my $quotedtoken = &::SqlQuote($token);

We don't normally use the &:: - it's not our style. If you could take these out
(simple search and replace) that would be good.

>+    #my $quotedipaddr = &::SqlQuote($::ENV{'REMOTE_ADDR'});

If you're not using this, please remove it.

>+    # Mail the user the token along with instructions for using it.
>+
>+    $vars->{'oldemailaddress'} = $old_email . &::Param('emailsuffix');
>+    $vars->{'newemailaddress'} = $new_email . &::Param('emailsuffix');
>+
>+    $vars->{'token'} = &::url_quote($token);
>+    $vars->{'emailaddress'} = $old_email . &::Param('emailsuffix');
>+
>+    my $message;
>+    $template->process("token/emailchangeold.txt.tmpl", $vars, \$message)
>+      || &::DisplayError("Template process failed: " . $template->error())
>+      && exit;
>+
>+    open SENDMAIL, "|/usr/lib/sendmail -t -i";
>+    print SENDMAIL $message;
>+    close SENDMAIL;
>+
>+    $vars->{'token'} = &::url_quote($newtoken);
>+    $vars->{'emailaddress'} = $new_email . &::Param('emailsuffix');
>+
>+    $message = "";
>+    $template->process("token/emailchangenew.txt.tmpl", $vars, \$message)
>+      || &::DisplayError("Template process failed: " . $template->error())
>+      && exit;
>+
>+    open SENDMAIL, "|/usr/lib/sendmail -t -i";
>+    print SENDMAIL $message;
>+    close SENDMAIL;
>+
>+}

Use a subroutine to factor out the common code here? token and email address
are parameters.

>diff -Nur --exclude .htaccess --exclude CVS --exclude data bugzilla-trunk/defparams.pl bugzilla-email/defparams.pl
>--- bugzilla-trunk/defparams.pl	Thu Feb 28 11:05:51 2002
>+++ bugzilla-email/defparams.pl	Wed Mar  6 10:14:31 2002
>@@ -593,6 +593,12 @@
>          0);
> 
> 
>+DefParam("allowemailchange",
>+         q{Users can change their own email address through the preferences.},
>+         "b",
>+         0);
>+
>+

"Note that the change is validated by emailing both addresses, so switching
this option on will not let users use an invalid address."

>diff -Nur --exclude .htaccess --exclude CVS --exclude data bugzilla-trunk/template/default/prefs/account.tmpl bugzilla-email/template/default/prefs/account.tmpl
>--- bugzilla-trunk/template/default/prefs/account.tmpl	Thu Feb 28 11:13:56 2002
>+++ bugzilla-email/template/default/prefs/account.tmpl	Sat Mar  2 13:25:54 2002
>@@ -21,17 +21,27 @@
> [%# INTERFACE:
>   # realname: string. The user's real name, if any.
>   # login:    string. The user's Bugzilla login email address.
>+  # login_change_date: string. The date the email change will be complete.
>+  # new_login_name:    string. The user's new Bugzilla login whilst not confirmed.

Please add that they can be empty/undefined (if they can be.)

>+          <th align="right">Token cancellation date:</th>
>+          <td>[% login_change_date %]</td>

Should we be exposing the internal term "token" to the user? I don't see a need
for it.

"Change request expires:"?

>+  # changes_saved: boolean/string. True if the CGI processed form data before 
>+  #                displaying anything.  Contains custom message if required.
>   #%]

This could be clearer. " '1' if the CGI... or can contain a custom message if
required."

> <h3>[% current_tab.description %]</h3>
>diff -Nur --exclude .htaccess --exclude CVS --exclude data bugzilla-trunk/template/default/token/confirmemail.html.tmpl bugzilla-email/template/default/token/confirmemail.html.tmpl
>--- bugzilla-trunk/template/default/token/confirmemail.html.tmpl	Thu Jan  1 10:00:00 1970
>+++ bugzilla-email/template/default/token/confirmemail.html.tmpl	Wed Feb 13 10:54:43 2002

>+[% INCLUDE global/header title=title %]

You don't need title=title, and we don't use it elsewhere.

>+From: bugzilla-admin-daemon

Is this email address agreed? It should certainly have a domain appended; many
people have accounts on multiple Bugzillas.

>+[% Param('urlbase') %]token.cgi?a=cfmem&t=[% token %]

Why choose a and t as the param names? This seems needlessly cryptic. Are there
backwards compatibility issues, or can you change them?

>+
>+If you are not the person who made this request, or you wish to cancel
>+this request, visit the following link:
>+
>+[% Param('urlbase') %]token.cgi?a=cxlem&t=[% token %]
>+

"You may want to change your Bugzilla password"?

>+If you are not the person who made this request, or you wish to cancel
>+this request, visit the following link:
>+
>+[% Param('urlbase') %]token.cgi?a=cxlem&t=[% token %]

As I understand it, currently we have to wait for this person to _not_ cancel
before
we can make the change, right? Why not give them a confirm link also; then the
legitimate
person can click both links and make the change immediately.

>+Subject: [% tokentype %] token cancelled
>+
>+A token was cancelled from [% remoteaddress %].  
>+If you did not request this, it could be either an honest 
>+mistake or the result of a malicious hack attempt.  

This is both cryptic and scary; we shouldn't be talking about tokens,
(I don't know if I requested a 'token cancellation' - I'm just trying to change
my email address)
and we shouldn't be talking about "malicious hack attempts" without
qualifying it. e.g. "someone breaking into your Bugzilla account"
is better.

>+Take a look at the information below and forward this email 
>+to [% maintainer %] if you suspect foul play.
>+
>+            Token: [% token %]
>+       Token Type: [% tokentype %]
>+             User: [% emailaddress %]
>+       Issue Date: [% issuedate %]
>+       Event Data: [% eventdata %]
>+Cancelled Because: [% cancelaction %]

We can keep token refs here because this is debugging data.

>   # Make sure the token exists in the database.
>   SendSQL( "SELECT tokentype FROM tokens WHERE token = $::quotedtoken" );
>   (my $tokentype = FetchSQLData())
>-    || DisplayError("The token you submitted does not exist.")
>+    || DisplayError("The token you submitted does not exist or has expired.")

That reminds me. The emails should contain the date by which you have to reply
to
confirm the change.

>+  if ( ($::action eq 'cxlem') 

"cxlem"? Can we not choose something less cryptic? I can't begin to think what
that stands for.

>+    # Check the user entered the correct old email address
>+    if($::FORM{'email'} ne $old_email) {
>+        DisplayError("Email Address confirmation failed");

Nit: small A.

>+    if (! ValidateNewUser($new_email,$old_email)) {

Nit: our style is (!ValidateNewUser($new_email, $old_email)

>+    $vars->{'title'} = "Email Address Changed";
>+    $vars->{'message'} = "Your email address has been changed.";

That's very misleading. :-) Their email address is still the same as it was.

"Your Bugzilla account login has been changed."

>diff -Nur --exclude .htaccess --exclude CVS --exclude data bugzilla-trunk/userprefs.cgi bugzilla-email/userprefs.cgi
>--- bugzilla-trunk/userprefs.cgi	Tue Mar  5 11:27:21 2002
>+++ bugzilla-email/userprefs.cgi	Wed Mar  6 10:23:28 2002
>@@ -65,16 +65,33 @@
> sub DoAccount {
>     SendSQL("SELECT realname FROM profiles WHERE userid = $userid");
>     $vars->{'realname'} = FetchSQLData();
>+
>+    if(Param('allowemailchange')) {
>+        SendSQL("SELECT tokentype, issuedate + INTERVAL 3 DAY, eventdata 
>+                    FROM tokens
>+                    WHERE userid = $userid
>+                    AND tokentype LIKE 'email%' 
>+                    ORDER BY tokentype ASC LIMIT 1");
>+        if(MoreSQLData()) {
>+            my ($tokentype, $change_date, $e

        Attachment #72766 -
        Flags: review-

    
    

    
  


  
Oh dear. No big POSTS for me. Review attached.

Gerv

        Attachment #61881 -
        Attachment is obsolete: true

        Attachment #61941 -
        Attachment is obsolete: true

        Attachment #61955 -
        Attachment is obsolete: true

        Attachment #62807 -
        Attachment is obsolete: true

        Attachment #63087 -
        Attachment is obsolete: true

        Attachment #67383 -
        Attachment is obsolete: true

        Attachment #69169 -
        Attachment is obsolete: true

        Attachment #70277 -
        Attachment is obsolete: true

        Attachment #72195 -
        Attachment is obsolete: true

    
    

    
  
Gerv, this is inside the Token.pm module where you see the &::, correct?  How do
you propose to remove those then?  You have to use that syntax to specify a
routine which is not in your own module.

    
    

    
  
Oops, sorry, my mistake. Ignore that bit.

Gerv

    
    

    
  
reassign to patch author
Assignee: justdave → zeroJ

    
    

    
  
Gerv: most of your complaints about the token code (as I'm going through them
trying to make your changes to a new patch) are complaints about Myk's original
token code which John merely copied and made changes to.  I'm going to hold off
on making those changes and we'll file new bugs for those.

The "cryptic" URLs generated for the token emails for example.  I think the
primary idea was to keep them as short as possible so they didn't wrap in the
email.  The a= and t= are the same codes used by the existing password reset
tokens.  "cnlem" is "cancel email" and follows the same format as the cancel
token action for the password reset token.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        v11:  (obsolete)
        — 
          — Splinter Review
      

    


  
Fixes most of Gerv's review comments except for the ones I previously posted
about.

I was unable to get use vars qw( $vars $template ) to work in Token.pm.  I
think it's a namespace issue because we're importing them from main and not
Token, so I had to leave the my $template = $::template, etc.  I couldn't find
any other way to make it work (and I did try).

I'll make an r= justdave for John's previous work, I've tested this out and it
works good on my local install and doesn't seem to break much else.  I'll voice
an r=justdave on John's previous work, but I'll let someone else mark it since
I've made changes.  I'll add Myk's previous r= back on here since the issues
Gerv brought up were mostly cosmetic things.

        Attachment #72766 -
        Attachment is obsolete: true

        Attachment #76959 -
        Flags: review+
Keywords: patch, 
          
            review

    
    

    
  
> I was unable to get use vars qw( $vars $template ) to work in Token.pm.  I
> think it's a namespace issue because we're importing them from main and not
> Token, so I had to leave the my $template = $::template, etc.  I couldn't find
> any other way to make it work (and I did try).

As we are returning to using $::template syntax, should we also remove "require
globals.pl" and move the 'my $template = $::template' down into the sub-routines
in order that all tests pass again?
Keywords: patch, 
          
            review

    
    

    
  
Comment on attachment 76959 [details] [diff] [review]
v11: 

marking needs-work on behalf of John...

Yes, go ahead and do that so the tests pass.  I'll let you do it now that
you're around because then I can review and check it in.

        Attachment #76959 -
        Flags: review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v11 fixes (obsolete)
        — 
          — Splinter Review
      

    


  
Patch against v11 to simplify review and merging.

I have the rest of Gerv's requests ready for review, however they required
major changes to the patch and, as has been said, are probably best left to
another bug now that everyone is happy.

    
    

    
  


  

        Attachment #76959 -
        Attachment is obsolete: true

        Attachment #76978 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 76982 [details] [diff] [review]
v12: complete patch, includes v11 + v11fixes with no other changes

carrying over r=myk from previous review.  Adding r=justdave for the current
updates.  It still works.  (I thought I'd tried it that way before and Perl
kept core-dumping on me when I did that, but you have it in a slightly
different place than I did, and it works. :-)

        Attachment #76982 -
        Flags: review+

    
    

    
  
Checking in default/prefs/account.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/prefs/account.tmpl,v  <-- 
account.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in default/prefs/userprefs.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/prefs/userprefs.tmpl,v  <--
 userprefs.tmpl
new revision: 1.3; previous revision: 1.2
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/token/confirmemail.html.tmpl,v
done
Checking in default/token/confirmemail.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/token/confirmemail.html.tmpl,v
 <--  confirmemail.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/token/emailchangenew.txt.tmpl,v
done
Checking in default/token/emailchangenew.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/token/emailchangenew.txt.tmpl,v
 <--  emailchangenew.txt.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/token/emailchangeold.txt.tmpl,v
done
Checking in default/token/emailchangeold.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/token/emailchangeold.txt.tmpl,v
 <--  emailchangeold.txt.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/token/tokencancel.txt.tmpl,v
done
Checking in default/token/tokencancel.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/token/tokencancel.txt.tmpl,v
 <--  tokencancel.txt.tmpl
initial revision: 1.1
done

Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
Any remaining issues with this please file new bugs, don't reopen this one.

    
    

    
  
John - thanks for making the changes I requested. I didn't realise these were
problems in the old code. If you'd like to file a new bug and attach your patch
for the other stuff, that would be great :-)

One less bug to go to 2.16...

Gerv

    
    

    
  
oops, half the patch didn't get checked in.  Here's the other half.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.139; previous revision: 1.138
done
Checking in Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.7; previous revision: 1.6
done
Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.18; previous revision: 1.17
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.68; previous revision: 1.67
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.32; previous revision: 1.31
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.152; previous revision: 1.151
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.6; previous revision: 1.5
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.32; previous revision: 1.31
done


    
    

    
  
Is this supposed to be live on b.m.o?

    
    

    
  
No, not until the next time bmo updates.

    
    

    
  
No.  It is (for now) just in CVS.  The next time b.m.o upgrades it will pick up
the change.

    
    

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

    
    

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

    
    

    
  
Verified fixed
Status: RESOLVED → VERIFIED
QA Contact: matty_is_a_geek → default-qa

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: