[openstreetmap/openstreetmap-website] Add openid connect support using doorkeeper-openid_connect gem (PR #4226)

[Tom Hughes]

@tomhughes requested changes on this pull request.



> @@ -150,3 +150,7 @@ smtp_password: null
 #signup_ip_max_burst:
 #signup_email_per_day:
 #signup_email_max_burst:
+#doorkeeper_signing_key: |

I know it's kind of self describing but every other option or group of options has a comment before it so can we add one here.

> @@ -22,3 +22,33 @@ trace_icon_storage: "test"
 # Lower some rate limits for testing
 max_changeset_comments_per_hour: 30
 moderator_changeset_comments_per_hour: 60
+

We don't have any other blank lines in this file, rather each group of options is preceded by a comment.

> @@ -107,6 +114,7 @@ CREATE TYPE public.user_status_enum AS ENUM (
     'deleted'
 );
 
+

This extra blank line looks suspicious?

> @@ -3448,3 +3518,5 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('7'),
 ('8'),
 ('9');
+
+

These two extra blank lines at the end also look suspicious.

>    private
 
   def authorize_client(user, client, options = {})
     options = options.merge(:client_id => client.uid,
                             :redirect_uri => client.redirect_uri,
                             :response_type => "code",
-                            :scope => "read_prefs")
+                            :scope => "read_prefs") { |_key, oldval, _newval| oldval }

If you want to reverse the priority then just reverse the hashes rather than using a lamda like that, so something like:

```ruby
options = {
  :client_id => client.uid,
  :redirect_uri => client.redirect_uri,
  :response_type => "code",
  :scope => "read_prefs"
}.merge(options)
```

> +    state = SecureRandom.urlsafe_base64(16)
+    verifier = SecureRandom.urlsafe_base64(48)
+    challenge = Base64.urlsafe_encode64(Digest::SHA256.digest(verifier), :padding => false)
+
+    authorize_client(user, client, :state => state, :code_challenge => challenge, :code_challenge_method => "S256", :scope => "openid read_prefs")
+    assert_response :redirect
+    code = validate_redirect(client, state)
+
+    tokens = request_tokens(client, code, verifier)
+    id_token = tokens[:id_token]
+    access_token = tokens[:access_token]
+
+    assert_not_nil(id_token)
+
+    data, _headers = JWT.decode id_token, Doorkeeper::OpenidConnect.signing_key.keypair, true, {
+      :algorithm => [Doorkeeper::OpenidConnect.signing_algorithm.to_s],

We should probably get the key and algorithm the same way a real client will, by fetching `/oauth2/discovery/keys`.

In fact we should probably fetch `/.well-known/openid-configuration` and do some validation on it, then follow the `jwks_uri` entry to get the keys.

> @@ -116,6 +161,11 @@ def validate_redirect(client, state)
   end
 
   def request_token(client, code, verifier = nil)
+    tokens = request_tokens(client, code, verifier)
+    tokens[:access_token]
+  end
+
+  def request_tokens(client, code, verifier = nil)

I think we should leave this called `request_token` and get rid of the other routine, then have this just return `token` and have the callers pick out the bits they want.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4226#pullrequestreview-1629079368
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4226/review/1629079368 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230915/5d6839f5/attachment.htm>