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.
Comment | File | Size | Author |
---|---|---|---|
#8 | openid.patch | 3.57 KB | moshe weitzman |
#7 | openid.patch | 3.58 KB | moshe weitzman |
#3 | openid.patch | 3.04 KB | moshe weitzman |
#1 | openid.patch | 4.18 KB | moshe weitzman |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedBah - damn attachment+preview bug
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedfix test failures.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commented#3: openid.patch queued for re-testing.
Comment #6
yoroy CreditAttribution: yoroy commentedSounds like desired behavior indeed so +1 on the ux improvement. Somebody test/review the code?
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedRemove unneeded code comment.
Added user_cookie_delete() for cleaner deletion of visitor cookie.
Removed unwanted blank lines. Better doxygen formatting.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedMention 'homepage' as example cookie for deletion.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks like a very nice improvement, thanks for the multiple little fixes.
Comment #10
walkah CreditAttribution: walkah commentedAgreed with Damien, this is a great improvement. +1 :-)
Comment #11
Dries CreditAttribution: Dries commentedThis 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?
Shouldn't the first parameter be an array?
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commented1. 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.
Comment #13
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #14
c960657 CreditAttribution: c960657 commentedNice improvement.
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).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)
Comment #15
c960657 CreditAttribution: c960657 commentedLooks 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.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, 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.
Comment #17
RobLoachThis is also a pretty popular widget around as far as an OpenID UI goes:
http://drupal.org/project/openid_selector