Closed Bug 313679 Opened 19 years ago Closed 19 years ago

Changing email address in sudo mode logs user in as impersonated user

Categories

(Bugzilla :: Bugzilla-General, defect)

2.21
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: karl, Assigned: karl)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax) When a user, being impersonated in sudo mode, has there email address changed, and the change is made through the session, the person doing the session loses their login and appears as if they were directly logged in as the impersonated user (instead of impersonating the user through sudo mode). Reproducible: Always Steps to Reproduce: 1. Begin an sudo session, impersonating user X 2. Go to user X's prefs 3. Enter X's password and a new email address Actual Results: The confirmation page, and all future pages, appear as if you are directly logged in as user X. Your actual login, and therefore your sudo session, is lost. Expected Results: The email address should have changed, and that's it.
Assignee: general → karl
Target Milestone: --- → Bugzilla 2.22
Version: unspecified → 2.21
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
This was an interesting one. Because the user's login & password were sent in by CGI parameter, the login method went to those instead of looking at the cookies we already have. This means that (1) you are suddenly and directly logged in as the person you are impersonating, and (2) your browser just received the cookies needed to make this new login semi-permanent. That means we have to (a) make the current session forget about the messed up login from (1), and (b) get rid of any new cookies that were sent out. (a) is a pretty simple solution. I check to see if there is an 'sudo' cookie being passed in. If so, I invalidate the current login session, delete the Bugzilla_login and Bugzilla_password CGI parameters, and log in again, which goes straight to the cookies and resumes the sudo session for this page. (b) is a bit harder. The 'Bugzilla_login' and 'Bugzilla_logincookie' cookies, if generated, are in an array ref in Bugzilla::CGI waiting to be sent out. To cope with this I have to make a new method in Bugzilla::CGI. This method takes the name of a cookie, searches through the list, and removes it if found. Since the cookie is already in proper form for transmission to the client as a header, I have to use a regex to hunt it down. Requesting review from glob, as he's listed for Bugzilla::CGI. If needed, LpSolit or joel (who also know about the sudo feature) could maybe be asked.
Attachment #202771 - Flags: review?(bugzilla)
Comment on attachment 202771 [details] [diff] [review] Patch v1 r=glob by inspection on cgi stuff only. asking lpsolit for additional review for the sudo stuff. >+ next if $cookie =~ m/^$cookiename=/; nit: would prefer this is be =~ m/^\Q$cookiename\E=/
Attachment #202771 - Flags: review?(bugzilla)
Attachment #202771 - Flags: review?(LpSolit)
Attachment #202771 - Flags: review+
Comment on attachment 202771 [details] [diff] [review] Patch v1 Marc knows cookies better than me.
Attachment #202771 - Flags: review?(LpSolit) → review?(wurblzap)
Comment on attachment 202771 [details] [diff] [review] Patch v1 This is the first time I look Bugzilla sudo in the eye, so I'd like to understand this a little better... I may be shortsighted, but I'd have hoped we could avoid plucking cookies back out of the jar. Isn't it possible to bypass the process of logging in if there is a sudo cookie present? For example, in login() in Bugzilla/Auth/Login/WWW.pm or in authenticate() in Bugzilla/Auth.pm or somesuch?
(In reply to comment #2) > > nit: would prefer this is be =~ m/^\Q$cookiename\E=/ > Agreed. I can either make the change and get a re-review, or make the change on checkin.
(In reply to comment #4) > (From update of attachment 202771 [details] [diff] [review] [edit]) > This is the first time I look Bugzilla sudo in the eye, so I'd like to > understand this a little better... I may be shortsighted, but I'd have hoped we > could avoid plucking cookies back out of the jar. Isn't it possible to bypass > the process of logging in if there is a sudo cookie present? For example, in > login() in Bugzilla/Auth/Login/WWW.pm or in authenticate() in Bugzilla/Auth.pm > or somesuch? I think I see what you mean. Are you asking if the login process, which normally prefers authenticating with CGI params instead of cookies, should bypass the CGI params when an sudo cookie is present? I did not really want to bypass the login process when I implemented sudo functionality. I left most of the login process alone because (a) I still need to obtain the identity of the real person who wants to start the session (to ensure that they have the rights to do so), and (b) it would probably mean much more modification & testing to make changes to the login code.
(In reply to comment #5) > > nit: would prefer this is be =~ m/^\Q$cookiename\E=/ > > Agreed. I can either make the change and get a re-review, or make the change > on checkin. New patch please.
Comment on attachment 202771 [details] [diff] [review] Patch v1 r- per discussion on IRC. It looks like this can be done more cleanly and easily by not passing the CGI variables for login and password on to Bugzilla->login when there is a sudo cookie present.
Attachment #202771 - Flags: review?(wurblzap) → review-
Attached patch Patch v1.1Splinter Review
Modification of attachment 202771 [details] [diff] [review] with respect to comment 8: It seems that this bug can be fixed by just deleting the CGI paramers, but saving them to local variables, before Bugzilla->login is called. The password entered on the userprefs page is checked against the in-DB password, so we seem fine there!
Attachment #202771 - Attachment is obsolete: true
Attachment #205411 - Flags: review?(wurblzap)
Comment on attachment 205411 [details] [diff] [review] Patch v1.1 Yup. Works well, r=wurblzap.
Attachment #205411 - Flags: review?(wurblzap) → review+
Flags: approval?
Flags: approval? → approval+
Checking in userprefs.cgi; /cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v <-- userprefs.cgi new revision: 1.93; previous revision: 1.92 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: