842784 - Persona sign in automatically signs in user

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: Persona, defect)

Description

[Austin King [:ozten]]

At http://bugzilla.mozilla.org/ I'm seeing a weird bug where user's get the Persona dialog, but cannot choose which email address to use to log in.

Instead the most recently used email is automatically chosen and the dialog quickly closes.

We're not seeing this with other websites, so I'm filing here.

Steps to Reproduce
1) Visit https://bugzilla.mozilla.org
2) If you're logged in, log out
3) Click blue login button

If you're not currently logged into Persona... log in and log into bugzilla, then log out of bugzilla again, starting at step 2

Note: Dialog pops up, then disappears

Actual

You are "auto" logged in as last identity

Expected

You are able to choose an identity and have to click sign in button.

I've reproduced this in Aurora, Firefox 19, and Chrome. The trick is to log in and out a few times.

Comment 1

[Austin King [:ozten]]

So I see an issue with how the code is structured. Currently we register with the watch API after calling the request API.

Production:

    function persona_sign_in() {
      navigator.id.request({ siteName: 'Bugzilla\x40Mozilla' });
      navigator.id.watch({
        loggedInUser: null,
        onlogin: function(assertion) {
          var form = document.createElement('form');
          form.action = 'https:\/\/bugzilla.mozilla.org\/index.cgi';
          form.method = 'POST';
          form.style.display = 'none';

          createHidden('token', '1361314232-98adac6114612aa4e0b9025fe5a9c7db', form);
          createHidden('Bugzilla_remember', 'on', form);
          createHidden('persona_assertion', assertion, form);


          document.body.appendChild(form);
          form.submit();
          return true;
        },
        onlogout: function () {
          document.location = 'https://bugzilla.mozilla.org/index.cgi?logout=1';
          return true;
        }
      });
    }

I think what we want is

    navigator.id.watch({
        loggedInUser: null,
        onlogin: function(assertion) {
          var form = document.createElement('form');
          form.action = 'https:\/\/bugzilla.mozilla.org\/index.cgi';
          form.method = 'POST';
          form.style.display = 'none';

          createHidden('token', '1361314232-98adac6114612aa4e0b9025fe5a9c7db', form);
          createHidden('Bugzilla_remember', 'on', form);
          createHidden('persona_assertion', assertion, form);


          document.body.appendChild(form);
          form.submit();
          return true;
        },
        onlogout: function () {
          document.location = 'https://bugzilla.mozilla.org/index.cgi?logout=1';
          return true;
        }
      });

    function persona_sign_in() {
      navigator.id.request({ siteName: 'Bugzilla\x40Mozilla' });      
    }

Comment 2

[Austin King [:ozten]]

http://bzr.mozilla.org/bmo/4.0/revision/8489 shows the switch from .get to .watch API.

Another change we'd suggest:

The script block where you setup the watch API... that should be on every page. Even if the user is already authenticated.

Currently we only send down that javascript block to un-authenticated users. Once they are authenticated, this usage of navigator.id.watch isn't sent down. This breaks logout and future capabilities of Persona to respect user's wishes to sign out.

Comment 3

[:glob ✱]

Attachment: patch v1

this patch follows ozten's suggestions.

one issue i encountered is persona was calling our onlogin function automatically after you'd logged out, making it impossible to logout -- you'd logout, then be immediately logged back in again without the persona prompt.

i suspect this is related to the concept of 'signing in to the browser', however i don't think this behavour is suitable for bugzilla, so the onlogin handler ignores requests unless they were triggered by clicking on our login button.

i think later on we'll have to track the session's authentication method, and only include persona on pages where the user signed in via persona.

Comment 4

[Austin King [:ozten]]

I like that you've changed

    loggedInUser: null,

to

    loggedInUser: [% IF user.id %]'[% user.login FILTER js %]'[% ELSE %]null[% END %],

Basically, the watch API wants to synchronize the browser's concept of auth with your server side session. This can explain some weird callback behavior, and your patch is much better.

https://developer.mozilla.org/en-US/docs/DOM/navigator.id.watch#Parameters explains why extra callbacks are invoked, if loggedInUser is always set to null.

> i think later on we'll have to track the session's authentication method, and only include persona on pages where the user signed in via persona.

Fair enough, that makes sense.

BMO posts a form and does a full page reload, which is fine. Most apps integrating Persona are using Ajax techniques. This makes safety guards like persona_ignore_login less necessary.

Comment 5

[David Lawrence [:dkl]]

Sigh. Sorry to step on your feet on this one glob. I actually checked in a minor fix last night to fix this temporarily by moving navigator.id.request() after navigator.id.watch() in persona_sign_in which fixed the issue atm. Then I changed the summary of the bug to one that specified we need to refactor to pull out navigator.id.watch out of persona_sign_in, assigned the bug to myself and made a lengthy comment. And then forgot to push submit and fell asleep :(

I also had the same issue with the BMO UI automatically logging me back in when I had logged out when I had changed navigator.id.watch to be outside of persona_sign_in. So I was going to work on it today when I was more awake and I had seen some more documentation/examples.

Anyway thanks for looking at this.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Different variation of persona patch (dkl) (v1)

Your patch looks good and works for me which is much better than the results I was getting last night when I was trying to accomplish the same. The key I was missing was the persona_ignore_login = true obviously. Here is a variation of you patch which I think is smaller and looks cleaner to me. Let me know what you thing but I could be fine with just committing what you have.

dkl

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 715872 [details] [diff] [review]
patch v1

Review of attachment 715872 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 8

[:glob ✱]

thanks ozten and dkl!  this should be pushed live in about 12 hours time.

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/Persona/template/en/default/admin/params/persona.html.tmpl
modified extensions/Persona/template/en/default/hook/global/header-additional_header.html.tmpl
Committed revision 8501.

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/Persona/template/en/default/admin/params/persona.html.tmpl
modified extensions/Persona/template/en/default/hook/global/header-additional_header.html.tmpl
Committed revision 8580.

Comment 9

[:glob ✱]

reopening; on our staging server i'm seeing persona automatically logging me out after i login without using persona (sometimes).

i'll extend the 'ignore' trick to also ignore automatic logouts.

Comment 10

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #9)
> i'll extend the 'ignore' trick to also ignore automatic logouts.

this wasn't possible.. it looks like setting loggedInUser correctly confused persona when using cgi auth.

i've filed bug 843191 regarding only presenting the persona javascript to users logged in with persona, but until then i'm going to have to set loggedInUser to null.

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/Persona/template/en/default/hook/global/header-additional_header.html.tmpl
Committed revision 8502.

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/Persona/template/en/default/hook/global/header-additional_header.html.tmpl
Committed revision 8581.