<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>

<p dir="auto">Some more review comments based on my initial reading of the specification and the gem documentation.</p>
<p dir="auto">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.</p>
<p dir="auto">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.</p><hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316434530">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> @@ -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:
</pre>
<p dir="auto">This is no longer an example so we should remove this comment.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316435481">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +
+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|
</pre>
<p dir="auto">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?</p>
<p dir="auto">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.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316437328">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +  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:
</pre>
<p dir="auto">Another comment that should be removed, and I'd probably remove the second one with the alternate implementation as well.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316439100">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +    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:
</pre>
<p dir="auto">I'd remove all this given we have a claims block below.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316439835">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +    # 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
</pre>
<p dir="auto">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.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316442346">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +
+  # 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.
</pre>
<p dir="auto">I'd drop this comment, especially as it's untrue because we're not requiring the profile scope like the original example code did.</p>
<p dir="auto">Though actually if I understand the documentation right then the <code class="notranslate">profile</code> 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...</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316444149">config/initializers/doorkeeper_openid_connect.rb</a>:</p>
<pre style='color:#555'>> +  #   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
</pre>
<p dir="auto">We should add <code class="notranslate">profile</code> and <code class="notranslate">email</code> claims - note that the email claim requires the email scope I think but we already have that as a privileged scope.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#discussion_r1316446702">lib/oauth.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">I'd put <code class="notranslate">SCOPES</code> back first as in the old code, and move <code class="notranslate">OAUTH2_SCOPES</code> to the end for consistency with ordering in other places.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4226#pullrequestreview-1611984829">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLLQNLDYPKW2N3X5AM3XY6M7ZANCNFSM6AAAAAA4KPKL4I">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLLAO7MUIFIER4QNMDTXY6M7ZA5CNFSM6AAAAAA4KPKL4KWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTACTX32.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4226/review/1611984829</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4226#pullrequestreview-1611984829",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4226#pullrequestreview-1611984829",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>