Skip to content
This repository has been archived by the owner on Jul 24, 2021. It is now read-only.

Openstreetmap as a relyingparty for OpenID #2500

Closed
openstreetmap-trac opened this issue Jul 23, 2021 · 5 comments
Closed

Openstreetmap as a relyingparty for OpenID #2500

openstreetmap-trac opened this issue Jul 23, 2021 · 5 comments

Comments

@openstreetmap-trac
Copy link

Reporter: amm
[Submitted to the original trac issue database at 1.33pm, Tuesday, 24th November 2009]

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 )

@openstreetmap-trac
Copy link
Author

Author: tom[at]compton.nu
[Added to the original trac issue at 12.28am, Wednesday, 25th November 2009]

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.

@openstreetmap-trac
Copy link
Author

Author: amm
[Added to the original trac issue at 12.59am, Wednesday, 25th November 2009]

Replying to [comment:1 tom[at]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.

@openstreetmap-trac
Copy link
Author

Author: amm
[Added to the original trac issue at 3.52pm, Monday, 28th December 2009]

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

@openstreetmap-trac
Copy link
Author

Author: amm
[Added to the original trac issue at 9.40am, Sunday, 17th January 2010]

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.

@openstreetmap-trac
Copy link
Author

Author: JasperWallace
[Added to the original trac issue at 4.49pm, Saturday, 1st May 2010]

Tested with [http://siege.org/projects/phpMyID/ 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."

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant