672841 - Make BMO support BrowserID

Status: RESOLVED FIXED

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

Description

[Gervase Markham [:gerv]]

I plan to use Bugzilla's pluggable auth to implement BrowserID support for BMO.

Gerv

Comment 1

[Reed Loden [:reed]]

Note I'm strongly against any type of external auth used for privileged accounts and will veto any such implementation that doesn't exempt such accounts.

Besides that, is BMO really an appropriate place for a Labs project that has very little traction and little-to-no documented security architecture review?

Comment 2

[David Lawrence [:dkl]]

As much as I would like to see this happen due to the coolness factor of it and the dog food aspects (demonstrating Mozilla work in critical systems), I would have to concur with Reed. I do not want to see this type of feature put in place without full approval from Mozilla Security after thorough review and we should definitely have a way to make this an option for certain accounts only.

dkl

Comment 3

[Gervase Markham [:gerv]]

Good points.

- We can make BrowserID accounts a separate account type. I will mark BrowserID accounts in the DB, and only allow this sort of login for accounts originally created this way. In other words, it will not be possible to log in to an existing account this way.

- If people want to ban BrowserID-backed accounts from having privileges beyond editbugs, until it's a more proven concept, I'm cool with that.

- That means that BrowserID becomes a simpler way of creating a Bugzilla account, rather than doing our own email challenge-response.

- The idea is that a user clicks the "Login with BrowserID" button, and we do like we do with LDAP - find the user's email address using the BrowserID mechanism and, if an account doesn't exist for that address, we create one. 

Does that help?

Gerv

Comment 4

[David Lawrence [:dkl]]

(In reply to comment #3)
> Good points.
> 
> - We can make BrowserID accounts a separate account type. I will mark
> BrowserID accounts in the DB, and only allow this sort of login for accounts
> originally created this way. In other words, it will not be possible to log
> in to an existing account this way.

I am not sure we need to go so far as not allow existing accounts. I can definitely imagine some current users of BMO preferring the browserID method and would not want to create separate accounts just to do so.

Preferably, I would like to see it integrated in such a way as to allow the user to choose which method they would like to authenticate with. Some sites I visit allow for their own native login or using say a Google account. It would be nice to have the same choice. Of course this applies to those where browser id is permitted. Otherwise they would not see the choice.

> - If people want to ban BrowserID-backed accounts from having privileges
> beyond editbugs, until it's a more proven concept, I'm cool with that.

I would also be fine with that as well. Obviously if they have editbugs and more than that such as any security group, etc. they would be banned.

dkl

Comment 5

[Gervase Markham [:gerv]]

Attachment: BrowserID extension v.1 (dummy attachment for review)

OK. The first version of a BrowserID extension is here:
http://bzr.mozilla.org/bugzilla/extensions/browserid/trunk/files

It's fairly simple - basically only one class, with a hundred lines of code.

Accounts with privileges higher than editbugs will not be able to log in this way (although this feature is disabled for testing purposes). Note: this means anyone who works for Mozilla will automatically be disallowed, as they all have either mozilla-corporation-confidential or mozilla-foundation-confidential.

Let's get the code reviewed from a BMO perspective, and then see what sort of security review we might need from infrasec.

dkl: can you look it over, please?

Gerv

Comment 6

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #5)
> Created attachment 569944 [details]
> BrowserID extension v.1 (dummy attachment for review)

I have had a chance to play around with this and I am impressed on how little is needed to make it work. I had to make a few changes to allow it to work with my devel environment which I will outline below.

Comments:
1. I had to add the required Hooks to account/auth/login*.html.tmpl as they are not present by default on bmo/4.0 or bugzilla/trunk. We will need to file bugs upstream to add those.

+[% Hook.process('additional_methods') %]

In the small login template, I wrapped it all in <li></li> tags and added the hook so that it would just be another choice in the header.

2. When I reenabled the group checking, it did fail to login for an empowered user properly, but if the user clicks once more to login via BrowserID for whatever reason, it throws the following error to the Bugzilla page:

malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "Internal Server Erro...") at /loader/0x4225768/Bugzilla/Extension/BrowserID/Login.pm line 53.

This is because it is trying to repass the old browserid_assertion and the browserid.org site is generating the internal server error which JSON cannot decode.

So we need to wrap all of that in an eval of some sort.

3. I had to make the following changes to the extension templates:

extensions/BrowserID/template/en/default/hook/account/auth/login-additional_methods.html.tmpl:
-            window.location.href = "http://" + window.location.host + "?browserid_assertion=" + assertion;
+            window.location.href = "[% urlbase FILTER none %]index.cgi?browserid_assertion=" + assertion;

extensions/BrowserID/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl:
-            window.location.href = "http://" + window.location.host + "?browserid_assertion=" + assertion;
+            window.location.href = "[% login_target FILTER none %]?browserid_assertion=" + assertion;

Otherwise once the BrowserID login was successful, it would redirect to the wrong URL for my installation.

4. When a BrowserID login fails, or the user is an empowered user, it would be nicer if it would throw up a login error instead of just going back to the same page before the login. This is what happens if they do a CGI login where a bad username/password is provided. This would also possibly alleviate comment #2.

5. Make the 120 secs in Login.pm a constant in extensions/BrowserID/lib/Constants.pm unless that will not be needed in the future.

dkl

Comment 7

[Gervase Markham [:gerv]]

Hi dkl,

Thanks for the review :-)

(In reply to David Lawrence [:dkl] from comment #6)
> 1. I had to add the required Hooks to account/auth/login*.html.tmpl as they
> are not present by default on bmo/4.0 or bugzilla/trunk. We will need to
> file bugs upstream to add those.

Oops, sorry about that - I should have included a patch.

Bug filed: bug 698418.

> +[% Hook.process('additional_methods') %]
> 
> In the small login template, I wrapped it all in <li></li> tags and added
> the hook so that it would just be another choice in the header.

I changed it so the BrowserID button was also hidden inside the "Login" expansion section. I think that's probably the right place for it.

> 2. When I reenabled the group checking, it did fail to login for an
> empowered user properly, but if the user clicks once more to login via
> BrowserID for whatever reason, it throws the following error to the Bugzilla
> page:

This isn't what happens for me. Can you provide more detailed exact STRs ("click here, then click here...")?

> 3. I had to make the following changes to the extension templates:
> 
> extensions/BrowserID/template/en/default/hook/account/auth/login-
> additional_methods.html.tmpl:
> -            window.location.href = "http://" + window.location.host +
> "?browserid_assertion=" + assertion;
> +            window.location.href = "[% urlbase FILTER none
> %]index.cgi?browserid_assertion=" + assertion;
> 
> extensions/BrowserID/template/en/default/hook/account/auth/login-small-
> additional_methods.html.tmpl:
> -            window.location.href = "http://" + window.location.host +
> "?browserid_assertion=" + assertion;
> +            window.location.href = "[% login_target FILTER none
> %]?browserid_assertion=" + assertion;
> 
> Otherwise once the BrowserID login was successful, it would redirect to the
> wrong URL for my installation.

Why does one of your modifications use urlbase and one login_target? I would expect the main login form to use login_target, as that's the one which gets put up as an interstitial whenever you try and do something but are not sufficiently empowered...

> 4. When a BrowserID login fails, or the user is an empowered user, it would
> be nicer if it would throw up a login error instead of just going back to
> the same page before the login. This is what happens if they do a CGI login
> where a bad username/password is provided. This would also possibly
> alleviate comment #2.

It would, but unfortunately there's a bug in Bugzilla which prevents this - bug 698423, which I just filed.

> 5. Make the 120 secs in Login.pm a constant in
> extensions/BrowserID/lib/Constants.pm unless that will not be needed in the
> future.

It should not be needed in the future. It's an acknowledged bug in the current BrowserID server:
https://groups.google.com/group/mozilla.dev.identity/browse_thread/thread/11c6e35d978967af

Gerv

Comment 8

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #7)
> I changed it so the BrowserID button was also hidden inside the "Login"
> expansion section. I think that's probably the right place for it.

Good idea. Definitely will look cleaner. I will try it when you have it updated.

> This isn't what happens for me. Can you provide more detailed exact STRs
> ("click here, then click here...")?

1. Logout whatever you are logged in as
2. Click the BrowserID Sign In image to bring up a browser id window.
3. Choose a user you know has editbugs or higher.
4. Bugzilla page reloads but according to header, user is not logged in (as planned since
user was empowered).
URL for me at this point is similar to: http://fedora/672841/index.cgi?browserid_assertion=eyJjZXJ0aWZpY2F0ZXMiOlsiZXlKaGJHY2lPaUpTVXpFeU9...
5. Immediately click on the BrowserID Sign In image again which brings up a similar window as before. Choose the same empowered user address as before.
6. Again Bugzilla reloads the page to try and login, except this time I get the following server error page:
malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "Internal Server Erro...") at /loader/0x339e768/Bugzilla/Extension/BrowserID/Login.pm line 53.
> > 3. I had to make the following changes to the e

Seems to me that it is trying to use the previously obtained browserid_assertion again instead of trying the login again without one, which the BrowserID server doesn't like 
and throws the error which Bugzilla cannot deal with.

> Why does one of your modifications use urlbase and one login_target? I would
> expect the main login form to use login_target, as that's the one which gets
> put up as an interstitial whenever you try and do something but are not
> sufficiently empowered...

If you look at the <form> tags login.html.tmpl and login-small.html.tmpl, one uses 'target' and one uses 'login_target'. Looking back at my changes, a better way I should have done
was for login-additional_methods.html.tmpl to use 'target' and not urlbase since it is in fact set in Bugzilla/Auth/Login/CGI.pm.

For login-small-additional_methods.html.tmpl, 'login_target' is the proper thing to use as it gets set to cgi.url regardless of uslbase.

dkl

Comment 9

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #8)
> (In reply to Gervase Markham [:gerv] from comment #7)
> > I changed it so the BrowserID button was also hidden inside the "Login"
> > expansion section. I think that's probably the right place for it.
> 
> Good idea. Definitely will look cleaner. I will try it when you have it
> updated.

Oops, sorry if I wasn't clear. The patch attached to bug 698418 implements this.

> > This isn't what happens for me. Can you provide more detailed exact STRs
> > ("click here, then click here...")?
> 
> 1. Logout whatever you are logged in as
<snip>

Ah, got it, thanks :-)
https://github.com/mozilla/browserid/issues/506 filed on the fact that the server returns a 500 ISE.

However, I think the problem is that the login_target or target is including the current URL, and therefore the previous assertion - so when we append another assertion, the result is gibberish.

Perhaps you could advise as to the best way we should be doing this. Should we be giving the assertion back to the current page as a POST or something? Or do we need to hack around the login_target to remove the old assertion?

Gerv

Comment 10

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #9) 
> Oops, sorry if I wasn't clear. The patch attached to bug 698418 implements
> this.

Tried the patch on my test instance and looks good.
 
> However, I think the problem is that the login_target or target is including
> the current URL, and therefore the previous assertion - so when we append
> another assertion, the result is gibberish.

Just add $cgi->delete('browserid_assertion') in extensions/BrowserID/lib/Login.pm so it will not be in cgi.url in the templates.

+    my $cgi = Bugzilla->cgi;

     my $assertion = $cgi->param("browserid_assertion");
+    $cgi->delete('browserid_assertion');

dkl

Comment 11

[Gervase Markham [:gerv]]

OK, that fixes the error. Excellent. I've checked in the review fixes - that one, the eval {}, and the change of "urlbase".

Gerv

Comment 12

[Gervase Markham [:gerv]]

mcoates: can we get this on some sort of infrasec review train? :-)

Summary: this is a Bugzilla extension which provides support for logging in (to existing or newly-created accounts) using BrowserID. We'd like to deploy it on b.m.o., because we want to support BrowserID and eat our own dogfood.

At the moment it only works for accounts whose max privilege is "editbugs". We'd like to loosen that restriction at some point, and would be interested in infrasec's views on that too.

Gerv

Comment 13

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to Gervase Markham [:gerv] from comment #12)
> mcoates: can we get this on some sort of infrasec review train? :-)

Yes, please file a security review request. We'd be happy too to review this.

https://wiki.mozilla.org/WebAppSec/Security_Review_Request

> 
> Summary: this is a Bugzilla extension which provides support for logging in
> (to existing or newly-created accounts) using BrowserID. We'd like to deploy
> it on b.m.o., because we want to support BrowserID and eat our own dogfood.
> 
> At the moment it only works for accounts whose max privilege is "editbugs".
> We'd like to loosen that restriction at some point, and would be interested
> in infrasec's views on that too.

I agree with this approach.  We're doing the same thing with other apps. Roll it out to lower privilege accounts at first, work out any concerns, and then consider rolling it out for all users.

Comment 14

[Gervase Markham [:gerv]]

Bug 698808 filed for security review. 
Bug 698798 filed for getting the extension up on bugzilla-stage-tip.

Gerv

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 569944 [details]
BrowserID extension v.1 (dummy attachment for review)

r+ for this but I would like to see the bug fix eventually implemented that fixes the failover for pluggable modules (bug 698423). For now if a BrowserID login fails, the user will just need to know to click on the Bugzilla login link instead.

dkl

Comment 17

[Gervase Markham [:gerv]]

OK, so the security review is completed.

Can we get this up on b.m.o., along with the latest hook patch from bug 698418?

Remember, it will start disabled, and can be re-disabled at any time by changing the available login methods in the admin screen.

Gerv

Comment 18

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #17)
> OK, so the security review is completed.
> 
> Can we get this up on b.m.o., along with the latest hook patch from bug
> 698418?
> 
> Remember, it will start disabled, and can be re-disabled at any time by
> changing the available login methods in the admin screen.
> 
> Gerv

We just finished our normal weekly code push. We are trying to get another push over the weekend that requires downtime due to schema changes. We could possibly push the code at that time, otherwise it will be thursday of next week.

Dkl

Comment 19

[Gervase Markham [:gerv]]

dkl: either of those is fine. If there's no reason not to take it at the weekend, let's do that.

Gerv

Comment 20

[David Lawrence [:dkl]]

We'll commit this right before the outage.

dkl

Comment 21

[David Lawrence [:dkl]]

I apologize for the lack of communication on this. Glob and I discussed right before last weeks outage about this and felt best to push this off to another normal update due to the other large changes that were occurring during the schema related outage. I have committed this now and will be in the next code update (hopefully tomorrow). Closing as well. Any new changes/bugs should be filed as new bugs under the Extensions: BrowserID component.

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added extensions/BrowserID
added extensions/BrowserID/Config.pm
added extensions/BrowserID/Extension.pm
added extensions/BrowserID/TODO
added extensions/BrowserID/lib
added extensions/BrowserID/template
added extensions/BrowserID/web
added extensions/BrowserID/lib/Login.pm
added extensions/BrowserID/template/en
added extensions/BrowserID/template/en/default
added extensions/BrowserID/template/en/default/hook
added extensions/BrowserID/template/en/default/hook/account
added extensions/BrowserID/template/en/default/hook/account/auth
added extensions/BrowserID/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
added extensions/BrowserID/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
added extensions/BrowserID/web/sign_in_orange.png
modified template/en/default/account/auth/login-small.html.tmpl
modified template/en/default/account/auth/login.html.tmpl
Committed revision 8116 

Committing to: bzr+ssh://[e-mail]/bmo/4.2
added extensions/BrowserID
added extensions/BrowserID/Config.pm
added extensions/BrowserID/Extension.pm
added extensions/BrowserID/TODO
added extensions/BrowserID/lib
added extensions/BrowserID/template
added extensions/BrowserID/lib/Login.pm
added extensions/BrowserID/template/en
added extensions/BrowserID/template/en/default
added extensions/BrowserID/template/en/default/hook
added extensions/BrowserID/template/en/default/hook/account
added extensions/BrowserID/template/en/default/hook/account/auth
added extensions/BrowserID/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
added extensions/BrowserID/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
missing extensions/ComponentWatching/Extension.pm.orig
modified extensions/ComponentWatching/Extension.pm.orig
modified template/en/default/account/auth/login-small.html.tmpl
modified template/en/default/account/auth/login.html.tmpl
Committed revision 8103

dkl