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

[Tom Hughes]

@tomhughes commented on this pull request.

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.

> @@ -0,0 +1,5 @@
+class ValidateCreateDoorkeeperOpenidConnectTables < ActiveRecord::Migration[7.0]

Why is this a separate migration? Can we not just include it in the previous migration that adds the tables?

> @@ -0,0 +1,11 @@
+class AddOpenidScope < ActiveRecord::Migration[7.0]
+  def self.up
+    add_column :oauth_tokens, :allow_openid, :boolean, :null => false, :default => false
+    add_column :client_applications, :allow_openid, :boolean, :null => false, :default => false

Why are we adding openid connect support to OAuth 1? That's not even a thing is it?

> @@ -0,0 +1,23 @@
+en:

You can just add extra local files like this - if the gem doesn't make these available automatically then you'll have to add them to the main `en.yml` file.

> @@ -78,6 +78,7 @@ gem "omniauth-rails_csrf_protection", "~> 1.0"
 
 # Doorkeeper for OAuth2
 gem "doorkeeper"
+gem "doorkeeper-openid_connect"

Pretty sure rubocop is going to reject this as not correctly sorted - it will need to be moved after `doorkeeper-i18n`.

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

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