Ticket #2500 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

Openstreetmap as a relyingparty for OpenID

Reported by: amm Owned by: tom@…
Priority: minor Milestone:
Component: website Version:
Keywords: Cc:

Description

Having to remember a new username and password for each new website is annoying and error prone and so being able to have a single sign on solution such as OpenID for OpenStreetMap would be nice and potentially even reduce the barrier of entry.

Attached is an initial version for implementing OpenID (as a relying party) on the main site of OpenStreetMap.

In this implementation user accounts continue to exist exactly as before including the need for email verification, however in addition to the possibility to login via username and password, with this patch it is now also possible to associate an OpenID to an existing account and login via your OpenID.

There are a couple of caveats with this though.

For one, this patch only implements OpenID login for the main website, however the OSM account credentials are needed at several other places too. E.g. talking to the API, logging in to the forum or trac, which means it is still necessary to have a password if one wants to use these services. For the API, OAuth provides an alternative and at least if you only use OAuth enabled clients, it is not necessary to have a password anymore. The wiki uses a separate user account anyway and with the mediawiki OpenID plugin this could benefit too. So I think this is a useful first step with other services to hopefully follow to make the experience even smoother.

The second caveat is that this is my first patch to the OpenStreetMap code base and indeed by first encounter of ruby or rails. So I suspect there are some problems with coding style and potentially bugs. It is a fairly short patch though so I should be able to fix up any comments fairly easily.

This patch contains one migration and in addition needs the open_id_authentication plugin (  http://github.com/rails/open_id_authentication )

Attachments

openid.patch Download (15.0 KB) - added by amm 4 years ago.
Initial version of a patch that implements login via OpenID
openidV2.patch Download (18.2 KB) - added by amm 4 years ago.
OpenID patch V2 (without plugin)
openidV2-withplugin.patch Download (82.3 KB) - added by amm 4 years ago.
OpenID patch V2 (with plugin included)

Change History

Changed 4 years ago by amm

Initial version of a patch that implements login via OpenID

comment:1 follow-up: ↓ 2 Changed 4 years ago by tom@…

It would probably have been better to talk to us first before you spent a lot of time on this, but thanks for your work and I will review it fully as soon as possible.

One obvious question is what other openid plugins did you consider, if any (there are a number I believe) and what made you pick that one over any others?

As far as the code goes, from a quick scan of the patch I'm guessing that you should probably be adding an index on the new column that you've added to users? You should also consider what indexes and foreign key constraints are appropriate on the two new tables that you've added.

I think I would also prefer openid_url to identity_url for the new field on the user table.

comment:2 in reply to: ↑ 1 Changed 4 years ago by amm

Replying to tom@compton.nu:

It would probably have been better to talk to us first before you spent a lot of time on this, but thanks for your work and I will review it fully as soon as possible.

Thanks for reviewing it. I haven't spent too much time on it yet and it was really just a proof of concept to see how much work would be involved to implement it and it turned out it was less than I had thought. So treat this as an initial question if OpenID is something that is potentially acceptable for the main site together with a patch to indicate that I would be willing to help implement it.

One obvious question is what other openid plugins did you consider, if any (there are a number I believe) and what made you pick that one over any others?

Yes, I think I came accross three different ones, although to be honest I can't remember which ones I looked at. I think I chose this one, as it was the simplest one to work with and seemed to do all that is required, i.e. one can give it an OpenID url and it tells you if the user can authenticate the url. The other ones tied in deeper into the account system and given we don't want to replace the existing accounts, they were more problematic. But I'll need to check again to make sure I remember it correctly.

As far as the code goes, from a quick scan of the patch I'm guessing that you should probably be adding an index on the new column that you've added to users? You should also consider what indexes and foreign key constraints are appropriate on the two new tables that you've added.

Good point, yes the user column should have an index. The other tables were created by the plugin it self. So I assumed it was in the way it needed it, but I'll have a close look to see what those actually are needed for.

I think I would also prefer openid_url to identity_url for the new field on the user table.

That at least is an easy change.

comment:3 Changed 4 years ago by amm

I have finally got around to incorporating some of the feedback received here in the ticket and via IRC. It is still the same flavor as before, so it doesn't integrate any tighter into the rest of the user authentication code as suggested by zerebubuth yet, but at least it should fix some of the bugs and I think it would be an acceptable interims solution, but that is obviously up to you to decide.

Changes from previouse version: 1) Renamed identity_url to openid_url 2) Added a unique index on users.openid_url and normal indexes on open_id_authentication_associations.server_url and open_id_authentication_nonces.timestamp, which covers all of the lookups performed during authentication. Neither open_id_authentication_associations nor open_id_authentication_nonces should be particularly large though. The associations table only has one entry per OpenID-provider and the nonce has one entry per (successful) login attempt, but gets cleared every 5 hours. (server time skew window) 3) It now includes the dependency on the ruby-openid gem in environment.rb 4) Fixed the incorrect referer_url in the openid exchange if the server was run on a non standard port e.g. 3000 5) Fixed a bug in one of the fallback paths

I have attached two versions of the patch. The first patch is the actual code written and is smaller, whereas the second one includes the entire open_id_authentication plugin in addition and should be ready to apply and try out.

Further comments are welcome

Changed 4 years ago by amm

OpenID patch V2 (without plugin)

Changed 4 years ago by amm

OpenID patch V2 (with plugin included)

comment:4 Changed 4 years ago by amm

As a status update:

There is now a branch in svn for the OpenID work at  http://trac.openstreetmap.org/browser/sites/rails_port_branches/openID

And also a test instance running on the dev server if anyone wants to try it out:  http://openid.dev.openstreetmap.org/

I guess any comments and improvement suggestions would still best be added to this ticket.

comment:5 Changed 4 years ago by JasperWallace

Tested with  phpMyID 0.9 and works fine.

The only issue is that it's not clear that you don't need a password, it would probably be worth adding some text explaining this:

"You can leave the password field blank when logging in with OpenID, however if you want to access other site in the Openstreetmap project that use openstreetmap.org account info you will need to set a password."

comment:6 Changed 3 years ago by amm

  • Status changed from new to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.