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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: emorley, Assigned: glob)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
dylan
:
review+
|
Details | Diff | Splinter Review |
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? :-)
Reporter | ||
Comment 1•9 years ago
|
||
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?
Reporter | ||
Comment 2•9 years ago
|
||
s/the 2015-09-06/the 2015-09-06 Firefox Nightly/
Comment 3•9 years ago
|
||
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.
Reporter | ||
Comment 4•9 years ago
|
||
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'.
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.
- if $input_params->{Bugzilla_remember} is missing fall back to default
Attachment #8658101 -
Flags: review?(dylan)
Reporter | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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).
Assignee | ||
Comment 12•9 years ago
|
||
(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 13•9 years ago
|
||
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+
Assignee | ||
Comment 14•9 years ago
|
||
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
Reporter | ||
Comment 15•9 years ago
|
||
Now that bug 1199087 is fixed, I tried enabling 2FA again, but still got signed out.
I've had to disable MFA again.
Assignee | ||
Comment 16•9 years ago
|
||
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?)
Reporter | ||
Comment 17•9 years ago
|
||
Definitely header, but pretty sure both.
And on both desktop Nightly and android beta.
Comment 18•9 years ago
|
||
dougt has also been seeing this a lot.
Comment 19•9 years ago
|
||
This just started happening to me today. It'd been working fine for a couple weeks.
Comment 20•9 years ago
|
||
I just saw this after updating my beta mac desktop browser.
Assignee | ||
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
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
Assignee | ||
Comment 24•9 years ago
|
||
this has been pushed. ed - want to give it another shot?
Reporter | ||
Comment 25•9 years ago
|
||
Seems to work now, thank you.
Comment 26•9 years ago
|
||
Working fine for me as well. Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•