[openstreetmap/openstreetmap-website] Allow registration of OAuth 1.0 applications to be disabled (PR #4500)

[Andy Allan]

@gravitystorm requested changes on this pull request.

Other than the inline comments, looks good to me.

> @@ -19,7 +19,11 @@ def show
   end
 
   def new
-    @client_application = ClientApplication.new
+    if Settings.oauth_10_registration
+      @client_application = ClientApplication.new
+    else
+      redirect_to :action => "index"

I think this needs a flash message. If we're doing more than just removing the link from the index page, then I guess we're trying to handle external links to `:new` e.g. from a tutorial somewhere. A flash message would explain why the link to `:new` no longer works.

> @@ -97,6 +97,7 @@ attachments_dir: ":rails_root/public/attachments"
 basic_auth_support: true
 # Enable legacy OAuth 1.0 support
 oauth_10_support: true
+oauth_10_registration: true

I think just a simple test for the `:new -> :index` redirect would be needed and is also worthwhile.

> @@ -19,7 +19,11 @@ def show
   end
 
   def new
-    @client_application = ClientApplication.new
+    if Settings.oauth_10_registration
+      @client_application = ClientApplication.new
+    else
+      redirect_to :action => "index"

Does `:create` need a similar redirect? Or are we happy that if the form is removed, then anyone going to the lengths of making a direct `:post` with the correct CSRF token is only causing problems for themselves?

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

Message ID: <openstreetmap/openstreetmap-website/pull/4500/review/1853862362 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240131/0d4e1170/attachment-0001.htm>