Mozilla's released a new session API for BrowserID, which is used by their BrowserID browser add-on for Firefox.

The Drupal BrowserID module should support these new extensions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samat Jain’s picture

Status: Active » Needs review
FileSize
3.78 KB

Patch posted.

My only qualms: seems like a bad idea to expose user's e-mail address via Javascript (required by the Firefox add-on). AFAIK the user's (Drupal) display name can also appear, but I've left it as the e-mail address for consistency with current use of BrowserID.

Samat Jain’s picture

Forgot to mention, I've this code running on my blog [blog.samat.org]. Please feel free to test there!

IceCreamYou’s picture

Status: Needs review » Needs work
  • This module should support both APIs -- I don't want to drop support for the older API that isn't dependent on browser add-ons.
  • The module needs to support sites with Clean URLs turned off; your patch assumes Clean URLs are enabled.
  • I don't know what the security implications are of exposing a user's email address in JS, but that does seem to be the way the new API is designed. I guess that as long as you don't have malicious JS running on your page, you're only exposing the user's email address to themselves anyway.
  • IMO the text Sending you to the conventional login page. is superfluous and should be removed.
  • I used window.location.href to redirect/refresh the page, and you used document.location.href. I don't have a strong preference, but it should be consistent.

Thanks for the patch!

Samat Jain’s picture

This module should support both APIs -- I don't want to drop support for the older API that isn't dependent on browser add-ons.

AFAIK is still does support both APIs.

There are no user-facing changes for the old API; the "Sign In" button is still on the user login block/page.

On the backend, the main change is that browserid.js (where most of the changes reside) is now sent with every page request.

IMO the text Sending you to the conventional login page. is superfluous and should be removed.

Perhaps, but the error message (which is a little cryptic) doesn't tell the user how to finish what they were doing. I've removed the message in the new patch (I agree my wording isn't very good), but still think there should be a better-worded explanation of what happened and how they should fix it.

* The module needs to support sites with Clean URLs turned off; your patch assumes Clean URLs are enabled.
* I used window.location.href to redirect/refresh the page, and you used document.location.href. I don't have a strong preference, but it should be consistent.

Fixed in new patch.

I also made a few other minor coding style changes.

IceCreamYou’s picture

AFAIK is still does support both APIs.

There are no user-facing changes for the old API; the "Sign In" button is still on the user login block/page.

On the backend, the main change is that browserid.js (where most of the changes reside) is now sent with every page request.

With your patch applied, browserid.js is only sent for authenticated users. It should also be sent for anonymous users at least when a BrowserID login button is on the page.

Additionally, Drupal's code style standards call for always using curly braces for control structures like if/else even when they're not needed.

Perhaps, but the error message (which is a little cryptic) doesn't tell the user how to finish what they were doing. I've removed the message in the new patch (I agree my wording isn't very good), but still think there should be a better-worded explanation of what happened and how they should fix it.

How about Click "Okay" to continue to the normal log-in page.?

Other than that, looks good, and I'll try it and commit it as soon as I can.

Samat Jain’s picture

With your patch applied, browserid.js is only sent for authenticated users. It should also be sent for anonymous users at least when a BrowserID login button is on the page.

I think you might be misreading things—browser.js (moved to browserid.info) will be included in every page, while a new Javascript variable Drupal.settings.browserid.email, set in browserid_init() (which is what I think you may be misreading), is only set for authenticated users (i.e. the browser displays they have logged in).

Additionally, Drupal's code style standards call for always using curly braces for control structures like if/else even when they're not needed.

Sorry, I missed that—did you want me to submit a new patch? I think it's only in one place…

How about Click "Okay" to continue to the normal log-in page.

AFAIK the error dialog will only have an "OK", so telling them to click it (which they have to, since it's modal and they need to dismiss it) is definitely extraneous. Maybe something like An error occurred attempting to use your BrowserID, please login conventionally. so users both know an error occurred and why they are redirected to the conventional login page.

IceCreamYou’s picture

Status: Needs work » Fixed

I think you might be misreading things

Oh, I didn't realize scripts specified in the .info file are always included.

Additionally, Drupal's code style standards call for always using curly braces for control structures like if/else even when they're not needed.

Sorry, I missed that—did you want me to submit a new patch? I think it's only in one place…

It's in two places:

+    if (navigator.id) {
+      if (Drupal.settings.browserid)
+        navigator.id.sessions = [{ email: Drupal.settings.browserid.email }];
+      else
+        navigator.id.sessions = [];

and

+  if (!user_is_logged_in())
+    return;
Maybe something like An error occurred attempting to use your BrowserID, please login conventionally. so users both know an error occurred and why they are redirected to the conventional login page.

I don't like the word "conventionally" because many users might not know what the convention is. How about After clicking "OK," you will be redirected to a page where you can log in without BrowserID or try logging in with BrowserID again.?

Anyway, committed the patch with the above changes. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.