Closed Bug 23067 Opened 25 years ago Closed 23 years ago

Should have some way to change the email account

Categories

(Bugzilla :: User Accounts, defect, P1)

2.10
defect

Tracking

()

VERIFIED FIXED
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)

(deleted), text/plain
Details
(deleted), 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.
Attached patch v5: adds email priority, cleaner templates (obsolete) (deleted) — Splinter Review
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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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) (deleted) — 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: 23 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

Created:
Updated:
Size: