Closed Bug 1202281 Opened 9 years ago Closed 9 years ago

BMO login being reset for every browser session

Categories

(bugzilla.mozilla.org :: General, defect)

Production
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: emorley, Assigned: glob)

References

Details

Attachments

(1 file, 1 obsolete file)

Every time I restart Nightly I'm having to re-login to BMO, including 2FA (same on Firefox for Android). This is starting to get a little tedious. I'm presuming this isn't intentional? :-)
For desktop: 1) Use the 2015-09-06 on Windows 8.1 x64 2) Clear all login related BMO cookies 3) Set the cookie expiration setting set to "expire at end of session" 4) Add both https://mozilla.org and https://bugzilla.mozilla.org as "always allow" whitelist exceptions 5) Restart the browser 6) Go to BMO and log in using the site root header login form 7) Close the BMO tab 8) Restart the browser 9) Navigate to BMO Expected: Still logged into BMO Actual: Logged out of BMO. - Changing the "keep until" cookie expiration setting to "they expire" stops the issue. Going by just this I would have said it was definitely a Nightly regression (since I've used the settings above fine for months) - however there are a few strange things: 1) When inspecting the BMO login cookies, the expire time is shown as "at end of session", even with the "keep until" cookie setting set to "they expire". Has the BMO cookie "expires" time been changed at all? 2) I also get signed out on Firefox for Android (official release version on the google play store), where they don't have any such cookie settings (other than "disallow third party cookies", which I have selected on both desktop and mobile) - plus it's the release build, so can't be a recent Nightly regression. 3) None of my other sites are getting logged out, on desktop or mobile. Could this perhaps be some combination of: BMO cookies now having the wrong expires time + Firefox desktop nightly having some pre-existing quirk where it doesn't delete cookies, even if they are marked as "until end of session", iff the user has the default cookie setting - but if the user chooses the option I choose, it then does delete them?
s/the 2015-09-06/the 2015-09-06 Firefox Nightly/
Something to note: Apparently session restore maintains session cookies across restarts, this could be confusing some of the testing here: http://stackoverflow.com/questions/777767/firefox-session-cookies I think Bugzilla has changed, as git-bz-moz can no longer get the cookie out of the sqlite file because of the switch to being a session cookie.
Ah that's helpful, thank you. So yeah does seem like a BMO regression then. Bugzilla/Auth/Persist/Cookie.pm hasn't changed since August 25th, and that change hardly seems relevant. From that file: # Remember cookie only if admin has told so # or admin didn't forbid it and user told to remember. if ( Bugzilla->params->{'rememberlogin'} eq 'on' || (Bugzilla->params->{'rememberlogin'} ne 'off' && $input_params->{'Bugzilla_remember'} && $input_params->{'Bugzilla_remember'} eq 'on') ) { # Not a session cookie, so set an infinite expiry $cookieargs{'-expires'} = 'Fri, 01-Jan-2038 00:00:00 GMT'; } So perhaps a BMO admin has changed the 'rememberlogin' site param?
'rememberlogin' hasn't changed, but there have been plenty of auth system changes to stuff 2fa into the mix. using the steps in comment 1 i'm unable to reproduce this issue. i can only replicate it when i set cookies to 'expire at end of session' without whitelisting bmo. ed, can you look at the raw headers that bmo is returning when you see cookies set to expire at the end of the session?
Flags: needinfo?(emorley)
i hit this after updating devedition, and captured some info along the way. after updating i was logged out of bugzilla. there was a cookie starting with mG6y in my browser which didn't exist in bmo's logincookies table. i logged in again, which as expected created a new cookie with a corresponding row in the logincookies table. then i had to immediately update devedition again. a non-public bug was restored without issue, but when i refreshed the tab it showed that i was logged out of bugzilla. when i looked at my stored cookie it showed the old mG6y cookie, not the new one (which still existed in the database).
> So perhaps a BMO admin has changed the 'rememberlogin' site param? this param hasn't been changed, and is 'defaulton'.
Assignee: nobody → glob
i'm seeing conflicting data, but there's something afoot with 2fa's lack of carrying the Bugzilla_remember param through the auth step (i'll fix that as part of bug 1199087). the weird thing is i see issues on accounts that don't have 2fa enabled. taking this one step at a time i'll adjust the logic in cookie.pm to treat 'on' and 'defaulton' as the same, and we can see if that helps.
Attached patch 1202281_1.patch (obsolete) (deleted) — Splinter Review
- if $input_params->{Bugzilla_remember} is missing fall back to default
Attachment #8658101 - Flags: review?(dylan)
Cancelling needinfo since it seems like the issues found in comment 8 / comment 9 will help. Happy to test once they are in production :-)
Flags: needinfo?(emorley)
Comment on attachment 8658101 [details] [diff] [review] 1202281_1.patch Is this going to work for the off case if the 2fa page doesn't pass through the Bugzilla_remember parameter? (which it doesn't afaict).
(In reply to Mark Banner (:standard8) from comment #11) > Is this going to work for the off case if the 2fa page doesn't pass through > the Bugzilla_remember parameter? (which it doesn't afaict). that particular issue is the only thing the patch is addressing.
Comment on attachment 8658101 [details] [diff] [review] 1202281_1.patch Review of attachment 8658101 [details] [diff] [review]: ----------------------------------------------------------------- r=dylan note: updated my bz-dev environment to use "defaulton", as this bug did not manifest with rememberlogin set to "on"
Attachment #8658101 - Flags: review?(dylan) → review+
as part of my pre-commit checks i realised this patch won't exactly work as is - Bugzilla_remember is a checkbox so when it's unchecked the value is missing from $input_params. this makes things tricky -- if someone unchecks Bugzilla_remember there's no way for persist::cookie to know if it's missing because it was unchecked, or if it's missing because 2fa dropped it. the current patch will result in login cookies being set to never-expire for 2fa logins even if someone explicitly told bugzilla to expire cookies at the end of the session. this will all be fixed by bug 1199087, which extends 2fa to carry through options, including Bugzilla_remember. given the risks of setting cookies to never-expire in situations where we shouldn't, i think it's best to wait for bug 1199087 to fix this issue correctly. unfortunately this means we'll have to deal with sessions being dropped until then.
Depends on: 1199087
Attachment #8658101 - Attachment is obsolete: true
Now that bug 1199087 is fixed, I tried enabling 2FA again, but still got signed out. I've had to disable MFA again.
thanks for the feedback. i was seeing this occasionally, but i haven't seen it since bug 1199087 was fixed. how are you logging into to bugzilla? (ie. via the header, or only when prompted to view restricted content?)
Definitely header, but pretty sure both. And on both desktop Nightly and android beta.
dougt has also been seeing this a lot.
This just started happening to me today. It'd been working fine for a couple weeks.
I just saw this after updating my beta mac desktop browser.
Attached patch 1202281_2.patch (deleted) — Splinter Review
we shouldn't be casting checkboxes to a boolean. they should be one of: - undefined (input element missing from form_ - empty string (unchecked) - "on" (or whatever the checkbox's value is)
Attachment #8670627 - Flags: review?(dylan)
Comment on attachment 8670627 [details] [diff] [review] 1202281_2.patch r=dylan Right! this works because we check specifically if Bugzilla_remember eq "on". Ugh.
Attachment #8670627 - Flags: review?(dylan) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git d6f47aa..bcb6edf master -> master looking forward to pushing this into production early next week.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
this has been pushed. ed - want to give it another shot?
Seems to work now, thank you.
Working fine for me as well. Thanks!
Depends on: 1221480
Blocks: 1221480
No longer depends on: 1221480
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: