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

[Tom Hughes]

@tomhughes requested changes on this pull request.

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.

> @@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+Doorkeeper::OpenidConnect.configure do
+  issuer do |_resource_owner, _application|
+    "https://www.openstreetmap.org/oauth2"
+  end
+
+  signing_key Settings.doorkeeper_signing_key
+
+  subject_types_supported [:public]
+
+  resource_owner_from_access_token do |access_token|
+    # Example implementation:

This is no longer an example so we should remove this comment.

> +
+Doorkeeper::OpenidConnect.configure do
+  issuer do |_resource_owner, _application|
+    "https://www.openstreetmap.org/oauth2"
+  end
+
+  signing_key Settings.doorkeeper_signing_key
+
+  subject_types_supported [:public]
+
+  resource_owner_from_access_token do |access_token|
+    # Example implementation:
+    User.find_by(:id => access_token.resource_owner_id)
+  end
+
+  auth_time_from_resource_owner do |resource_owner|

If we're not going to provide this do we actually need the empty block or can we just remove it? The documentation says it's optional but recomended which suggests we can remote it?

The same goes for the reauthentication and account selections blocks, though in theory we could possibly implement those ones while we can't currently do this one as we don't record login times.

> +  end
+
+  # Depending on your configuration, a DoubleRenderError could be raised
+  # if render/redirect_to is called at some point before this callback is executed.
+  # To avoid the DoubleRenderError, you could add these two lines at the beginning
+  #  of this callback: (Reference: https://github.com/rails/rails/issues/25106)
+  #   self.response_body = nil
+  #   @_response_body = nil
+  select_account_for_resource_owner do |resource_owner, return_to|
+    # Example implementation:
+    # store_location_for resource_owner, return_to
+    # redirect_to account_select_url
+  end
+
+  subject do |resource_owner, _application|
+    # Example implementation:

Another comment that should be removed, and I'd probably remove the second one with the alternate implementation as well.

> +    resource_owner.id
+
+    # or if you need pairwise subject identifier, implement like below:
+    # Digest::SHA256.hexdigest("#{resource_owner.id}#{URI.parse(application.redirect_uri).host}#{'your_secret_salt'}")
+  end
+
+  # Protocol to use when generating URIs for the discovery endpoint,
+  # for example if you also use HTTPS in development
+  # protocol do
+  #   :https
+  # end
+
+  # Expiration time on or after which the ID Token MUST NOT be accepted for processing. (default 120 seconds).
+  # expiration 600
+
+  # Example claims:

I'd remove all this given we have a claims block below.

> +    # store_location_for resource_owner, return_to
+    # redirect_to account_select_url
+  end
+
+  subject do |resource_owner, _application|
+    # Example implementation:
+    resource_owner.id
+
+    # or if you need pairwise subject identifier, implement like below:
+    # Digest::SHA256.hexdigest("#{resource_owner.id}#{URI.parse(application.redirect_uri).host}#{'your_secret_salt'}")
+  end
+
+  # Protocol to use when generating URIs for the discovery endpoint,
+  # for example if you also use HTTPS in development
+  # protocol do
+  #   :https

Not sure what to do here... I for one use https in dev but then again if it's only for discovery then that possibly doesn't matter.

> +
+  # Example claims:
+  # claims do
+  #   normal_claim :_foo_ do |resource_owner|
+  #     resource_owner.foo
+  #   end
+
+  #   normal_claim :_bar_ do |resource_owner|
+  #     resource_owner.bar
+  #   end
+  # end
+
+  claims do
+    claim :preferred_username, :scope => :openid do |resource_owner, _scopes, _access_token|
+      # Pass the resource_owner's preferred_username if the application has
+      # `profile` scope access. Otherwise, provide a more generic alternative.

I'd drop this comment, especially as it's untrue because we're not requiring the profile scope like the original example code did.

Though actually if I understand the documentation right then the `profile` scope is required for this claim by default so I'm not sure why they were checking it again? If so then we need to add that scope...

> +  #   normal_claim :_foo_ do |resource_owner|
+  #     resource_owner.foo
+  #   end
+
+  #   normal_claim :_bar_ do |resource_owner|
+  #     resource_owner.bar
+  #   end
+  # end
+
+  claims do
+    claim :preferred_username, :scope => :openid do |resource_owner, _scopes, _access_token|
+      # Pass the resource_owner's preferred_username if the application has
+      # `profile` scope access. Otherwise, provide a more generic alternative.
+      resource_owner.display_name
+    end
+  end

We should add `profile` and `email` claims - note that the email claim requires the email scope I think but we already have that as a privileged scope.

> @@ -14,8 +15,10 @@ def description
     end
   end
 
-  def self.scopes(privileged: false)
-    scopes = SCOPES
+  def self.scopes(oauth2: false, privileged: false)
+    scopes = []
+    scopes += OAUTH2_SCOPES if oauth2
+    scopes += SCOPES

I'd put `SCOPES` back first as in the old code, and move `OAUTH2_SCOPES` to the end for consistency with ordering in other places.

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

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