Add openid connect support using doorkeeper-openid_connect gem

[milan-cvetkovic]

...as discussed in Issue 507 and described by @mmd-osm.

To activate, set the value of doorkeeper_signing_key to pem of the RSA private key.

Allows using openstreetmap as an identity provider.

Adds openid scope to OAuth2 authorizations, required to login to OSM.

Currently, the only claims returned are sub and preferred_username.

[tomhughes]

This is just an initial review of obvious things as I haven't looked at any of the details of the gem and how it works.

[milan-cvetkovic]

This is just an initial review of obvious things as I haven't looked at any of the details of the gem and how it works.

Thanks for the quick review! I will look into addressing your comments.

[tomhughes]

I reran the actions as they failed due to some github issue and the rubocop run has now picked up a bunch more things.

[tomhughes]

I think from https://github.com/doorkeeper-gem/doorkeeper-openid_connect#internationalization-i18n that they will actually be found in the gem - the installer just added a copy of them so they could be customised.

That said it may be worth adding them so they can be translated - the main doorkeeper ones don't need to be as the doorkeeper-i18n gem does it.

[tomhughes]

Some more review comments based on my initial reading of the specification and the gem documentation.

I think we need to understand exactly the interaction between claims and scopes and which scopes we need to enable the different claims and I still need to read up on discovery though I'm not sure if that matters to us.

We also need to think about whether we want to make this generally available or whether it should be privileged and just to support internal use cases like the wiki.

[milan-cvetkovic]

I added email claim to read_email privileged scope.

ID_TOKEN and userinfo response now contain sub, preferred_name, and email claims.

Please let me know if I should create separate profile claim, and what to put in it.

[tomhughes]

The profile claim is probably not important for us, at least for the wiki. If we were to do it then I imagine /user/display_name would be the place to point it.

[milan-cvetkovic]

EDIT: this modification has been reverted.

I added a separate profile scope with some additional claims - normally part of public information and easily accessible from users table. Moved preferred_username to profile scope.

ID_TOKEN contains:

• sub (OSM user id)
• preferred_username (OSM display name)
• email
• email_verified

/oauth2/userinfo endpoint returns:

• sub
• preferred_username
• email
• email_verififed
• profile - profile url
• description - from profile, if available
• contributor_terms_agreed (boolean)
• changesets_count
• traces_count

Only claims appropriate for the scope are returned:

• email and email_verified only for privileged users having read_email scope
• sub claimis the only field returned with openid scope, The remaining claims are in profile scope.

[milan-cvetkovic]

@tomhughes

I think we need to understand exactly the interaction between claims and scopes and which scopes we need to enable the different claims and I still need to read up on discovery though I'm not sure if that matters to us.

We also need to think about whether we want to make this generally available or whether it should be privileged and just to support internal use cases like the wiki.

The well-known endpoints (discovery) are required for openid connect to work, at least using doorkeeper-openid_connect plugin. In particular, the openid-configuration endpoint is queried for token_endpoint, jwksuri_endpoint and userinfo_endpont used in later phases.

Any 3rd party app using OSM as ID provider would require at least a persistent value correlated to userid, which would be returned as a sub field in id_token.

[tomhughes]

Sure I realise we need to provide an ID but I didn't realise discovery was essential since it seemed to be a separate component but if we need it then fine.

One thing we do need to think about is how to test this...

[milan-cvetkovic]

I tested most of the flow manually.

I did try to write automated tests, but was not successful. The most I got is to verify the existence of discovery endpoints.

I am not too familiar with the testing framework, but the problems started when initiating authorization, the test was not able to access the endpoints reported by openid-configuration, as if the routes were not present.

[milan-cvetkovic]

@tomhughes I have added an openid connect test to oauth2 integration test.

Are there more changes to this PR that you had in mind?

[tomhughes]

Are there more changes to this PR that you had in mind?

I've done a new review which is mostly about the test.

Other than that it's on the agenda for this weeks operations call to discuss whether we want to make this globally available or keep it for internal use. I had hoped to have that discussion on the last call but we ran out of time.

[milan-cvetkovic]

@tomhughes Any updates with this PR?

[tomhughes]

I made a few minor textual cleanups and merged this.

As it involves a migration I'm going to wait until the database backup which is currently running completes (which should be tomorrow) before deploying it.