This repository has been archived by the owner. It is now read-only.

onlogout called immediately after onlogin if 3rd party cookies are disabled #2999

Closed
shane-tomlinson opened this Issue Feb 8, 2013 · 27 comments

Comments

Projects
None yet
8 participants
@shane-tomlinson
Member

shane-tomlinson commented Feb 8, 2013

A user reported an oddity today that may affect many Chrome users.

If 3rd party cookies are disabled, onlogout is called immediately after onlogin.

The following test page shows the error.

<html>
  <script src="https://login.persona.org/include.js"></script>
<body>
<fieldset>
    <legend>Persona Login</legend>
    <button onclick="login_persona()">Login</button>
</fieldset>
<script>
window.login_persona = (function() {
  navigator.id.watch({
    loggedInUser: null,
    onlogin: function (assertion) {
      console.info ("Checking an assertion from Persona.");
    },
    onlogout: function () {
      console.info ("Persona thinks I am or should be logged out.");
    }
  });

  return function() {
    console.info ("User requested login");
    navigator.id.request();
  }
}());
</script>
</body>
</html>

To both users and developers, it appears that Persona is operating as it should until the onlogout function is invoked with no clear indication why.

Instead of leaving users clueless, we should show them how to whitelist the Persona domain.

Relate to:
#982, #989

@unprolix

This comment has been minimized.

Show comment Hide comment
@unprolix

unprolix Feb 8, 2013

I spent quite some time trying to track this down!

unprolix commented Feb 8, 2013

I spent quite some time trying to track this down!

@ghost ghost assigned 6a68 Feb 14, 2013

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 14, 2013

Member

@seanmonstar @shane-tomlinson here's an idea for a fix: can we try to set and read back a cookie value, then bail intelligently if the cookie didn't actually get set. sound like a good approach?

Member

6a68 commented Feb 14, 2013

@seanmonstar @shane-tomlinson here's an idea for a fix: can we try to set and read back a cookie value, then bail intelligently if the cookie didn't actually get set. sound like a good approach?

@seanmonstar

This comment has been minimized.

Show comment Hide comment
@seanmonstar

seanmonstar Feb 14, 2013

Member
Member

seanmonstar commented Feb 14, 2013

@stenington stenington referenced this issue in mozilla/openbadges-backpack Feb 21, 2013

Open

Change backpack Persona usage to the Observer API #511

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 21, 2013

Member

Chrome's third-party cookie behavior is a bit more nuanced than Safari's.

Chrome behaves as expected if you tell it to allow third-party cookies from any Persona subdomain, making an exception for [*.]persona.org, not just persona.org.

Looking more closely at what cookies we set vs whether we test for subdomains or just parent domain.

Member

6a68 commented Feb 21, 2013

Chrome's third-party cookie behavior is a bit more nuanced than Safari's.

Chrome behaves as expected if you tell it to allow third-party cookies from any Persona subdomain, making an exception for [*.]persona.org, not just persona.org.

Looking more closely at what cookies we set vs whether we test for subdomains or just parent domain.

@toolness

This comment has been minimized.

Show comment Hide comment
@toolness

toolness Feb 21, 2013

Instead of leaving users clueless, we should show them how to whitelist the Persona domain.

Wait, this doesn't actually seem like an ideal solution... Persona works just fine with the callback API, where the client website is responsible for storing the user's login state, so this seems like a regression, and an inconvenience for users who disable third-party cookies.

Why does Persona need to manage the user's login state? Can the callback API be made to support some of the options that the observer API uses, or is there a way for clients of the Observer API to intentionally opt-out of Persona's management of login state? The only reason mozilla/openbadges#511 needs to support it is so we can direct users from their email to our website instead of persona, which seemingly has nothing to do with third-party cookies.

Another reason I'm concerned about this solution is because it might be the case that certain locked-down systems, such as public library computers, may not even allow users to whitelist domains. (If they disable View Source, they might as well disable third-party cookies too, but maybe I'm wrong.)

Instead of leaving users clueless, we should show them how to whitelist the Persona domain.

Wait, this doesn't actually seem like an ideal solution... Persona works just fine with the callback API, where the client website is responsible for storing the user's login state, so this seems like a regression, and an inconvenience for users who disable third-party cookies.

Why does Persona need to manage the user's login state? Can the callback API be made to support some of the options that the observer API uses, or is there a way for clients of the Observer API to intentionally opt-out of Persona's management of login state? The only reason mozilla/openbadges#511 needs to support it is so we can direct users from their email to our website instead of persona, which seemingly has nothing to do with third-party cookies.

Another reason I'm concerned about this solution is because it might be the case that certain locked-down systems, such as public library computers, may not even allow users to whitelist domains. (If they disable View Source, they might as well disable third-party cookies too, but maybe I'm wrong.)

@toolness

This comment has been minimized.

Show comment Hide comment
@toolness

toolness Feb 21, 2013

Also, I'm willing and able to help resolve this bug by contributing to persona, so let me know if there's anything I can do to help!

Also, I'm willing and able to help resolve this bug by contributing to persona, so let me know if there's anything I can do to help!

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 21, 2013

Member

@toolness ordinarily persona does work with third-party cookies disabled. shane's repro page, if loaded on localhost with safari, logs you in without problems. chrome's unique handling of third-party cookies might be an issue here; it's only a guess, and I'm actively investigating. (sidestepping the question of why persona wants to hook into login/logout; we can take that to the dev-identity list or IRC if you want to talk about get vs watch APIs)

Member

6a68 commented Feb 21, 2013

@toolness ordinarily persona does work with third-party cookies disabled. shane's repro page, if loaded on localhost with safari, logs you in without problems. chrome's unique handling of third-party cookies might be an issue here; it's only a guess, and I'm actively investigating. (sidestepping the question of why persona wants to hook into login/logout; we can take that to the dev-identity list or IRC if you want to talk about get vs watch APIs)

@stenington

This comment has been minimized.

Show comment Hide comment
@stenington

stenington Feb 21, 2013

I was running into this onlogout behavior with Firefox 18.

I was running into this onlogout behavior with Firefox 18.

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 21, 2013

Member

hmm. FF18 + chrome don't allow the communication_iframe to touch its localStorage if third-party cookies are disabled.

continuing to dig :-)

Member

6a68 commented Feb 21, 2013

hmm. FF18 + chrome don't allow the communication_iframe to touch its localStorage if third-party cookies are disabled.

continuing to dig :-)

@toolness

This comment has been minimized.

Show comment Hide comment
@toolness

toolness Feb 21, 2013

Is it possible for the communication_iframe to pass the token back to the parent window for it to store in its localStorage instead? That could be done transparently by the include.js shim... Not sure how awesome it is security-wise.

Is it possible for the communication_iframe to pass the token back to the parent window for it to store in its localStorage instead? That could be done transparently by the include.js shim... Not sure how awesome it is security-wise.

@seanmonstar

This comment has been minimized.

Show comment Hide comment
@seanmonstar

seanmonstar Feb 21, 2013

Member

The user's private key is stored in localStorage on persona.org domain, so 1) we don't want to store that in the localStorage of the RP, because then they have our user's keys, and 2) the communication_iframe wouldn't be able to access the key anyways, if localStorage was off.

Member

seanmonstar commented Feb 21, 2013

The user's private key is stored in localStorage on persona.org domain, so 1) we don't want to store that in the localStorage of the RP, because then they have our user's keys, and 2) the communication_iframe wouldn't be able to access the key anyways, if localStorage was off.

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 21, 2013

Member

By having persona throw a popup on user click, i.e. in direct response to user action, we avoid third-party cookie restrictions, and safari (and FF nightly, according to @seanmonstar) intelligently detects that the user knows about this domain, and so relaxes the cannot-touch-localStorage rule. FF18 and Chrome are not so clever about distinguishing good and evil third parties.

It looks like we explicitly decided, a while ago, not to check for storage in the communication_iframe, for the sake of IE compatibility. I suppose we may have to revisit that now, unless I find a simpler fix (maybe our localStorage stub is missing a method that triggers an uncaught exception).

Member

6a68 commented Feb 21, 2013

By having persona throw a popup on user click, i.e. in direct response to user action, we avoid third-party cookie restrictions, and safari (and FF nightly, according to @seanmonstar) intelligently detects that the user knows about this domain, and so relaxes the cannot-touch-localStorage rule. FF18 and Chrome are not so clever about distinguishing good and evil third parties.

It looks like we explicitly decided, a while ago, not to check for storage in the communication_iframe, for the sake of IE compatibility. I suppose we may have to revisit that now, unless I find a simpler fix (maybe our localStorage stub is missing a method that triggers an uncaught exception).

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 22, 2013

Member

I think what's happening is, the dialog does an explicit cookie check, but only on its own domain (checking for localstorage/cookies disabled globally).

This check succeeds.

Later, the communication_iframe does a cookie check deep in the stack, after the user has done the dialog login flow and the dialog is dismissed:

// on dialog_complete:
checkAndEmit ({
  getSilentAssertion ({
    checkAuthenticationAndSync ({
      checkAuthentication ({
        network.cookiesEnabled // which fails & returns false

I'm currently looking at a couple different ways of adding this check so that the user gets timely warning info in the dialog & expect to have a PR out today. Simplest thing I can think of is to have communication_iframe do a check on startup, right after opening the channel to the parent window, and firing the bad-news signal on page load, before the dialog is opened.

Member

6a68 commented Feb 22, 2013

I think what's happening is, the dialog does an explicit cookie check, but only on its own domain (checking for localstorage/cookies disabled globally).

This check succeeds.

Later, the communication_iframe does a cookie check deep in the stack, after the user has done the dialog login flow and the dialog is dismissed:

// on dialog_complete:
checkAndEmit ({
  getSilentAssertion ({
    checkAuthenticationAndSync ({
      checkAuthentication ({
        network.cookiesEnabled // which fails & returns false

I'm currently looking at a couple different ways of adding this check so that the user gets timely warning info in the dialog & expect to have a PR out today. Simplest thing I can think of is to have communication_iframe do a check on startup, right after opening the channel to the parent window, and firing the bad-news signal on page load, before the dialog is opened.

6a68 added a commit to 6a68/browserid that referenced this issue Feb 22, 2013

@toolness

This comment has been minimized.

Show comment Hide comment
@toolness

toolness Feb 22, 2013

Awesome, that's great--thanks @6a68 !

Awesome, that's great--thanks @6a68 !

6a68 added a commit to 6a68/browserid that referenced this issue Feb 22, 2013

Bail if 3rd party cookies disabled. Closes #2999.
  Chrome and FF18 currently will not allow any third-party to access localStorage
  or cookies in a third-party iframe. Safari is smarter and allows this for third-
  parties after the user interacts with their site (via throwing the popup).

  Added an exception for IE8, as it disallows cookie setting but will actually
  allow network requests to go through. Not thrilled about UA sniffing, though.

  Verified does the right thing on Chrome, FF18, IE8, and android 2.2 stock handset.

6a68 added a commit to 6a68/browserid that referenced this issue Feb 22, 2013

Bail if 3rd party cookies disabled. Closes #2999.
  Chrome and FF18 currently will not allow any third-party to access localStorage
  or cookies in a third-party iframe. Safari is smarter and allows this for third-
  parties after the user interacts with their site (via throwing the popup).

  Added an exception for IE8, as it disallows cookie setting but will actually
  allow network requests to go through. Not thrilled about UA sniffing, though.

  Verified does the right thing on Chrome, FF18, IE8, and android 2.2 stock handset.
@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 22, 2013

Member

@toolness think this should be fixed by my PR, expect it'll get attention Monday. arguably we should modify the cookies_disabled screen to warn the user that they have 3rd party cookies disabled

Member

6a68 commented Feb 22, 2013

@toolness think this should be fixed by my PR, expect it'll get attention Monday. arguably we should modify the cookies_disabled screen to warn the user that they have 3rd party cookies disabled

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Feb 27, 2013

Member

@toolness Just to keep you up to speed--this fix requires cross-domain messaging changes that've taken longer than expected and tend to need lots of QA. This is at risk to ship with the next train, but that doesn't rule out a hotfix.

I'll keep this ticket posted as dev continues. Lower-level details are in the PR 3043.

Member

6a68 commented Feb 27, 2013

@toolness Just to keep you up to speed--this fix requires cross-domain messaging changes that've taken longer than expected and tend to need lots of QA. This is at risk to ship with the next train, but that doesn't rule out a hotfix.

I'll keep this ticket posted as dev continues. Lower-level details are in the PR 3043.

@cmcavoy

This comment has been minimized.

Show comment Hide comment
@cmcavoy

cmcavoy Mar 1, 2013

Just checking in, any chance this can be a hotfix?

cmcavoy commented Mar 1, 2013

Just checking in, any chance this can be a hotfix?

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Mar 5, 2013

Member

@cmcavoy I don't think it's going to happen in the next few weeks. This might change, but it's doubtful--we are kinda overcommitted on Q1 goals. I'm bummed; I was hoping to get this done for you guys.

The good news here is that FF users won't have problems, but Chrome users will need to whitelist [*.]persona.org.

And we will finish this off, just not in the timeframe you were hoping for. Owe you a 🍺

Member

6a68 commented Mar 5, 2013

@cmcavoy I don't think it's going to happen in the next few weeks. This might change, but it's doubtful--we are kinda overcommitted on Q1 goals. I'm bummed; I was hoping to get this done for you guys.

The good news here is that FF users won't have problems, but Chrome users will need to whitelist [*.]persona.org.

And we will finish this off, just not in the timeframe you were hoping for. Owe you a 🍺

@cmcavoy cmcavoy referenced this issue in mozilla/openbadges-backpack Mar 8, 2013

Closed

Customize Persona popup #601

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Mar 12, 2013

Member

Update! @lloyd and I went over this bug again today.

Rather than fail the login if third-party cookies are disabled, we can instead fall back gracefully, and simply not manage the user's session. This means that the RP will have to do its own session management for those users; the iframe will always assert the user's logged in.

Working on a patch with this in mind, aim to have a pull request out tomorrow. Should be relatively much simpler.

Related quote from IRC:

lloyd|afk       _6a68: just to close, I'm thinking telling the user they could log in less
frequently by adding an exception might be higher value than notifying the RP.  as far as
notifying the RP the best thing I can think of is a short suggested session duration in
assertion.          4:11
lloyd|afk       _6a68: given that this is a privacy concious user, the assumption is
that more frequent logins is a price they're willing to pay.            4:12
Member

6a68 commented Mar 12, 2013

Update! @lloyd and I went over this bug again today.

Rather than fail the login if third-party cookies are disabled, we can instead fall back gracefully, and simply not manage the user's session. This means that the RP will have to do its own session management for those users; the iframe will always assert the user's logged in.

Working on a patch with this in mind, aim to have a pull request out tomorrow. Should be relatively much simpler.

Related quote from IRC:

lloyd|afk       _6a68: just to close, I'm thinking telling the user they could log in less
frequently by adding an exception might be higher value than notifying the RP.  as far as
notifying the RP the best thing I can think of is a short suggested session duration in
assertion.          4:11
lloyd|afk       _6a68: given that this is a privacy concious user, the assumption is
that more frequent logins is a price they're willing to pay.            4:12
@toolness

This comment has been minimized.

Show comment Hide comment
@toolness

toolness Mar 14, 2013

Awesome, @6a68! Thanks for your work on this.

Awesome, @6a68! Thanks for your work on this.

@shane-tomlinson

This comment has been minimized.

Show comment Hide comment
@shane-tomlinson

shane-tomlinson Mar 14, 2013

Member

This means that the RP will have to do its own session management for those users; the iframe will always assert the user's logged in.

@6a68 - do you mean to say the iframe will always assert the user is logged in or logged out? If logged in, what assertion will it pass back to the RP?

Member

shane-tomlinson commented Mar 14, 2013

This means that the RP will have to do its own session management for those users; the iframe will always assert the user's logged in.

@6a68 - do you mean to say the iframe will always assert the user is logged in or logged out? If logged in, what assertion will it pass back to the RP?

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Mar 14, 2013

Member

@shane-tomlinson I'm thinking we just fire onmatch no matter what

Member

6a68 commented Mar 14, 2013

@shane-tomlinson I'm thinking we just fire onmatch no matter what

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Mar 14, 2013

Member

@shane-tomlinson maybe something like this? mozilla#3101

You know, it's really a bit of a bummer that navigator.cookieEnabled doesn't tell the truth in the third-party frame context in FF (returns true even if cookies can't be set).

Member

6a68 commented Mar 14, 2013

@shane-tomlinson maybe something like this? mozilla#3101

You know, it's really a bit of a bummer that navigator.cookieEnabled doesn't tell the truth in the third-party frame context in FF (returns true even if cookies can't be set).

@shane-tomlinson shane-tomlinson referenced this issue in mozilla/badges.mozilla.org Mar 21, 2013

Closed

Keeps logging me out again after logging in #29

shane-tomlinson added a commit that referenced this issue Mar 21, 2013

Call .onready if 3rd party cookies are disabled.
Prevents users from being immediately logged out if 3rd party cookies are disabled.

Set a cookie in the client before /wsapi/session_context is requested. session_context checks for the existence of this cookie and returns a new field "cookies" which is true if cookies are enabled, false otw. Checking if the cookie can be read on the server avoid the Android 3.2 and 4.0 where cookies can be read by the browser even if they are disabled.

fixes #2308
fixes #2999

@callahad callahad referenced this issue in mozilla/django-browserid Mar 22, 2013

Closed

Redirected to logout immediately after login #143

shane-tomlinson added a commit that referenced this issue Mar 22, 2013

Call .onready if 3rd party cookies are disabled.
Prevents users from being immediately logged out if 3rd party cookies are disabled.

Set a cookie in the client before /wsapi/session_context is requested. session_context checks for the existence of this cookie and returns a new field "cookies" which is true if cookies are enabled, false otw. Checking if the cookie can be read on the server avoid the Android 3.2 and 4.0 where cookies can be read by the browser even if they are disabled.

fixes #2308
fixes #2999
@shane-tomlinson

This comment has been minimized.

Show comment Hide comment
@shane-tomlinson

shane-tomlinson Apr 2, 2013

Member

Link to #1903, #1835, #982, #989, #2575

Member

shane-tomlinson commented Apr 2, 2013

Link to #1903, #1835, #982, #989, #2575

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Apr 4, 2013

Member

link to #3169

Member

6a68 commented Apr 4, 2013

link to #3169

@callahad

This comment has been minimized.

Show comment Hide comment
@callahad

callahad Apr 4, 2013

Member

Workaround for users: Add a third party cookie exception for [*.]persona.org

Member

callahad commented Apr 4, 2013

Workaround for users: Add a third party cookie exception for [*.]persona.org

@6a68

This comment has been minimized.

Show comment Hide comment
@6a68

6a68 Apr 4, 2013

Member

Actually, depends on the browser. [*.]persona.org for chrome, but I think
persona.org is good enough for FF.

On Thu, Apr 4, 2013 at 1:19 PM, Dan Callahan notifications@github.comwrote:

Workaround for users: Add a third party cookie exception for [*.]
persona.org


Reply to this email directly or view it on GitHubhttps://github.com/mozilla/browserid/issues/2999#issuecomment-15921209
.

Member

6a68 commented Apr 4, 2013

Actually, depends on the browser. [*.]persona.org for chrome, but I think
persona.org is good enough for FF.

On Thu, Apr 4, 2013 at 1:19 PM, Dan Callahan notifications@github.comwrote:

Workaround for users: Add a third party cookie exception for [*.]
persona.org


Reply to this email directly or view it on GitHubhttps://github.com/mozilla/browserid/issues/2999#issuecomment-15921209
.

shane-tomlinson added a commit that referenced this issue Apr 10, 2013

Call .onready if 3rd party cookies are disabled.
Prevents users from being immediately logged out if 3rd party cookies are disabled.

Set a cookie in the client before /wsapi/session_context is requested. session_context checks for the existence of this cookie and returns a new field "cookies" which is true if cookies are enabled, false otw. Checking if the cookie can be read on the server avoid the Android 3.2 and 4.0 where cookies can be read by the browser even if they are disabled.

fixes #2308
fixes #2999

shane-tomlinson added a commit that referenced this issue Apr 11, 2013

Call .onready if 3rd party cookies are disabled.
Prevents users from being immediately logged out if 3rd party cookies are disabled.

Set a cookie in the client before /wsapi/session_context is requested. session_context checks for the existence of this cookie and returns a new field "cookies" which is true if cookies are enabled, false otw. Checking if the cookie can be read on the server avoid the Android 3.2 and 4.0 where cookies can be read by the browser even if they are disabled.

fixes #2308
fixes #2999

@callahad callahad referenced this issue in mozilla/django-browserid Apr 15, 2013

Closed

Don't require loading of include.js on every pageview? #152

@shane-tomlinson shane-tomlinson referenced this issue in mozilla/fxa-content-server Apr 14, 2014

Merged

feature(client): Add a `/cookies_disabled` page. #884

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.