People who login with OpenID generally always login with OpenID. And furthermore, some sites like intranets only allow this sort of login. This patch logs a user's openid-identifier in his visitor cookie after a successful openid login. When this visitor arrives at the login form again, we switch to openid presentation and pre-fill the identifier. This is all done in javascript so that we are compatible with varnish and anon page cache.

When a user explicitly logs out, this persistent cookie is deleted. This is the recommended implementation as per http://www.plaxo.com/api/openid_recipe

This patch also updates the openid altered login for m to use #attached and also implements clearing of $_SESSION['openid'] after successful login. We already do that after successful openid registration.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

FileSize
4.18 KB

Bah - damn attachment+preview bug

Status: Needs review » Needs work

The last submitted patch, openid.patch, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

fix test failures.

Status: Needs review » Needs work
Issue tags: -Usability

The last submitted patch, openid.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
Issue tags: +Usability

#3: openid.patch queued for re-testing.

yoroy’s picture

Sounds like desired behavior indeed so +1 on the ux improvement. Somebody test/review the code?

moshe weitzman’s picture

FileSize
3.58 KB

Remove unneeded code comment.

Added user_cookie_delete() for cleaner deletion of visitor cookie.

Removed unwanted blank lines. Better doxygen formatting.

moshe weitzman’s picture

FileSize
3.57 KB

Mention 'homepage' as example cookie for deletion.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Looks like a very nice improvement, thanks for the multiple little fixes.

walkah’s picture

Agreed with Damien, this is a great improvement. +1 :-)

Dries’s picture

+++ modules/user/user.module
@@ -3498,6 +3498,16 @@ function user_cookie_save(array $values, array $fields = array('name', 'mail', '
+function user_cookie_delete($cookie_name) {
+  setrawcookie('Drupal.visitor.' . $cookie_name, '', REQUEST_TIME - 3600, '/');
+}

This is minor but user_cookie_save() takes an array of values, whereas user_cookie_delete() takes a singular value. This might be a minor API inbalance.
Thoughts?

+++ modules/openid/openid.module
@@ -76,6 +76,30 @@ function openid_user_insert(&$edit, $account, $category) {
+    user_cookie_save($_SESSION['openid']['user_login_values'], array('openid_identifier'));

Shouldn't the first parameter be an array?

moshe weitzman’s picture

1. I don't think of it as an imbalance. In both cases, there is only one cookie. In the save case, you save multiple *name/value pairs* to one cookie.

2. It actually is an array. Thats how openid stores its values in $_SESSION.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

c960657’s picture

Status: Fixed » Needs work

Nice improvement.

1. I don't think of it as an imbalance. In both cases, there is only one cookie. In the save case, you save multiple *name/value pairs* to one cookie.

Huh? AFAICT user_cookie_save($values, array('name', 'mail', 'homepage')) sets three individual cookies. If that is the case, I agree with Dries' comment. Also, the variable/argument names in user_cookie_save/user_cookie_delete() should be consistent (i.e. use either $cookie_name or $field, not both).

+    if (!$('#edit-openid-identifier.openid-processed').size()) {

I know this was already there, but it would be nice if you could update this to use the new once() pattern (see #444344: jQuery .once() method for adding behaviors once).

How about storing the username in a cookie as well?

For consistency I think we should also destroy the other variables (name, mail, homepage) on an explicit logout? This is outside the scope of this issue, though.

For filling out the form, could we hook into (and expand) Drupal.behaviors.fillUserInfoFromCookie() in misc/form.js?

(I hope we can make user_cookie_save() use window.localStorage instead of cookies in newer browsers to avoid the HTTP overhead and caching issues related to cookies. This definitely belongs in a separate issue - just thought I'd mention it here)

c960657’s picture

Looks like I cross-posted with Dries's commit message. I'm leaving the issue as "needs work" so my comment can be discussed. Most of the points I raised are nice-to-have's and further improvements, though the argument thing needs to be changed IMO.

moshe weitzman’s picture

IMO, there should only be one Drupal.visitor cookie. I suggest fixing the arg imbalance by changing user_cookie_save() to add new items to the one cookie and user_cookie_delete should delete items from the cookie. Will probably require some js changes as wel. Its not a simple fix, but is ideal IMO.

RobLoach’s picture

This is also a pretty popular widget around as far as an OpenID UI goes:
http://drupal.org/project/openid_selector

  • Dries committed 9b4274e on 8.3.x
    - Patch #742366 by moshe weitzman: better UX for OpenID users.
    
    

  • Dries committed 9b4274e on 8.3.x
    - Patch #742366 by moshe weitzman: better UX for OpenID users.
    
    

  • Dries committed 9b4274e on 8.4.x
    - Patch #742366 by moshe weitzman: better UX for OpenID users.
    
    

  • Dries committed 9b4274e on 8.4.x
    - Patch #742366 by moshe weitzman: better UX for OpenID users.
    
    

  • Dries committed 9b4274e on 9.1.x
    - Patch #742366 by moshe weitzman: better UX for OpenID users.