Better support for native apps

CHANGELOG.md

@@ -11,6 +11,7 @@ User-visible changes worth mentioning.
 - [#1249]: Specify case sensitive uniqueness to remove Rails 6 deprecation message
 - [#1248] Display the Application Secret in HTML after creating a new application even when `hash_application_secrets` is used.
 - [#1248] Return the unhashed Application Secret in the JSON response after creating new application even when `hash_application_secrets` is used.
+- [#1238] Better support for native app with support for custom scheme and localhost redirection.
 
 ## 5.1.0
 

app/validators/redirect_uri_validator.rb

@@ -6,23 +6,20 @@
 # to OAuth standards and Doorkeeper configuration.
 #
 class RedirectUriValidator < ActiveModel::EachValidator
-  def self.native_redirect_uri
-    Doorkeeper.configuration.native_redirect_uri
-  end
-
   def validate_each(record, attribute, value)
     if value.blank?
       return if Doorkeeper.configuration.allow_blank_redirect_uri?(record)
 
       record.errors.add(attribute, :blank)
     else
       value.split.each do |val|
-        uri = ::URI.parse(val)
-        next if native_redirect_uri?(uri)
+        next if oob_redirect_uri?(val)
 
+        uri = ::URI.parse(val)
         record.errors.add(attribute, :forbidden_uri) if forbidden_uri?(uri)
         record.errors.add(attribute, :fragment_present) unless uri.fragment.nil?
-        record.errors.add(attribute, :relative_uri) if uri.scheme.nil? || uri.host.nil?
+        record.errors.add(attribute, :unspecified_scheme) if unspecified_scheme?(uri)
+        record.errors.add(attribute, :relative_uri) if relative_uri?(uri)
         record.errors.add(attribute, :secured_uri) if invalid_ssl_uri?(uri)
       end
     end
@@ -32,14 +29,24 @@ def validate_each(record, attribute, value)
 
   private
 
-  def native_redirect_uri?(uri)
-    self.class.native_redirect_uri.present? && uri.to_s == self.class.native_redirect_uri.to_s
+  def oob_redirect_uri?(uri)
+    Doorkeeper::OAuth::NonStandard::IETF_WG_OAUTH2_OOB_METHODS.include?(uri)
   end
 
   def forbidden_uri?(uri)
     Doorkeeper.configuration.forbid_redirect_uri.call(uri)
   end
 
+  def unspecified_scheme?(uri)
+    return true if uri.opaque.present?
+
+    %w[localhost].include?(uri.try(:scheme))
+  end
+
+  def relative_uri?(uri)
+    uri.scheme.nil? && uri.host.nil?
+  end
+
   def invalid_ssl_uri?(uri)
     forces_ssl = Doorkeeper.configuration.force_ssl_in_redirect_uri
     non_https = uri.try(:scheme) == "http"

app/views/doorkeeper/applications/_form.html.erb

@@ -20,12 +20,6 @@
         <%= t('doorkeeper.applications.help.redirect_uri') %>
       </span>
 
-      <% if Doorkeeper.configuration.native_redirect_uri %>
-          <span class="form-text text-secondary">
-            <%= raw t('doorkeeper.applications.help.native_redirect_uri', native_redirect_uri: content_tag(:code, class: 'bg-light') { Doorkeeper.configuration.native_redirect_uri }) %>
-          </span>
-      <% end %>
-
       <% if Doorkeeper.configuration.allow_blank_redirect_uri?(application) %>
         <span class="form-text text-secondary">
           <%= t('doorkeeper.applications.help.blank_redirect_uri') %>

config/locales/en.yml

@@ -11,6 +11,7 @@ en:
             redirect_uri:
               fragment_present: 'cannot contain a fragment.'
               invalid_uri: 'must be a valid URI.'
+              unspecified_scheme: 'must specify a scheme.'
               relative_uri: 'must be an absolute URI.'
               secured_uri: 'must be an HTTPS/SSL URI.'
               forbidden_uri: 'is forbidden by the server.'
@@ -33,7 +34,6 @@ en:
         confidential: 'Application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.'
         redirect_uri: 'Use one line per URI'
         blank_redirect_uri: "Leave it blank if you configured your provider to use Client Credentials, Resource Owner Password Credentials or any other grant type that doesn't require redirect URI."
-        native_redirect_uri: 'Use %{native_redirect_uri} if you want to add localhost URIs for development purposes'
         scopes: 'Separate scopes with spaces. Leave blank to use the default scopes.'
       edit:
         title: 'Edit application'

lib/doorkeeper.rb

@@ -52,6 +52,7 @@
 require "doorkeeper/oauth/token_introspection"
 require "doorkeeper/oauth/invalid_token_response"
 require "doorkeeper/oauth/forbidden_token_response"
+require "doorkeeper/oauth/nonstandard"
 
 require "doorkeeper/secret_storing/base"
 require "doorkeeper/secret_storing/plain"

lib/doorkeeper/config.rb

@@ -254,7 +254,7 @@ def configure_secrets_for(type, using:, fallback:)
     option :custom_access_token_expires_in, default: ->(_context) { nil }
     option :authorization_code_expires_in,  default: 600
     option :orm,                            default: :active_record
-    option :native_redirect_uri,            default: "urn:ietf:wg:oauth:2.0:oob"
+    option :native_redirect_uri,            default: "urn:ietf:wg:oauth:2.0:oob", deprecated: true
     option :active_record_options,          default: {}
     option :grant_flows,                    default: %w[authorization_code client_credentials]
     option :handle_auth_errors,             default: :render

lib/doorkeeper/config/option.rb

@@ -38,14 +38,20 @@ def option(name, options = {})
 
         Builder.instance_eval do
           remove_method name if method_defined?(name)
-          define_method name do |*args, &block|
-            value = if attribute_builder
-                      attribute_builder.new(&block).build
-                    else
-                      block || args.first
-                    end
+          if options[:deprecated]
+            define_method name do |*_, &_|
+              Kernel.warn "[DOORKEEPER] #{name} has been deprecated and will soon be removed"
+            end
+          else
+            define_method name do |*args, &block|
+              value = if attribute_builder
+                        attribute_builder.new(&block).build
+                      else
+                        block || args.first
+                      end
 
-            @config.instance_variable_set(:"@#{attribute}", value)
+              @config.instance_variable_set(:"@#{attribute}", value)
+            end
           end
         end
 

lib/doorkeeper/oauth/authorization/code.rb

@@ -15,7 +15,7 @@ def issue_token
           @token ||= AccessGrant.create!(access_grant_attributes)
         end
 
-        def native_redirect
+        def oob_redirect
           { action: :show, code: token.plaintext_token }
         end
 

lib/doorkeeper/oauth/authorization/token.rb

@@ -63,7 +63,7 @@ def issue_token
           )
         end
 
-        def native_redirect
+        def oob_redirect
           {
             controller: controller,
             action: :show,

lib/doorkeeper/oauth/code_response.rb

@@ -18,8 +18,8 @@ def redirectable?
       end
 
       def redirect_uri
-        if URIChecker.native_uri? pre_auth.redirect_uri
-          auth.native_redirect
+        if URIChecker.oob_uri? pre_auth.redirect_uri
+          auth.oob_redirect
         elsif response_on_fragment
           Authorization::URIBuilder.uri_with_fragment(
             pre_auth.redirect_uri,

lib/doorkeeper/oauth/error_response.rb

@@ -41,7 +41,7 @@ def status
 
       def redirectable?
         name != :invalid_redirect_uri && name != :invalid_client &&
-          !URIChecker.native_uri?(@redirect_uri)
+          !URIChecker.oob_uri?(@redirect_uri)
       end
 
       def redirect_uri

lib/doorkeeper/oauth/helpers/uri_checker.rb

@@ -25,10 +25,10 @@ module OAuth
     module Helpers
       module URIChecker
         def self.valid?(url)
-          return true if native_uri?(url)
+          return true if oob_uri?(url)
 
           uri = as_uri(url)
-          uri.fragment.nil? && !uri.host.nil? && !uri.scheme.nil?
+          valid_scheme?(uri) && iff_host?(uri) && uri.fragment.nil? && uri.opaque.nil?
         rescue URI::InvalidURIError
           false
         end
@@ -78,8 +78,22 @@ def self.query_matches?(query, client_query)
           client_query.split("&").sort == query.split("&").sort
         end
 
-        def self.native_uri?(url)
-          url == Doorkeeper.configuration.native_redirect_uri
+        def self.valid_scheme?(uri)
+          return false if uri.scheme.nil?
+
+          %w[localhost].include?(uri.scheme) == false
+        end
+
+        def self.hypertext_scheme?(uri)
+          %w[http https].include?(uri.scheme)
+        end
+
+        def self.iff_host?(uri)
+          !(hypertext_scheme?(uri) && uri.host.nil?)
+        end
+
+        def self.oob_uri?(uri)
+          NonStandard::IETF_WG_OAUTH2_OOB_METHODS.include?(uri)
         end
       end
     end

lib/doorkeeper/oauth/nonstandard.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+module Doorkeeper
+  module OAuth
+    class NonStandard
+      # These are not part of the OAuth 2 specification but are still in use by Google
+      # and in some other implementations. Native applications should use one of the
+      # approaches discussed in RFC8252. OOB is 'Out of Band'
+
+      # This value signals to the Google Authorization Server that the authorization
+      # code should be returned in the title bar of the browser, with the page text
+      # prompting the user to copy the code and paste it in the application.
+      # This is useful when the client (such as a Windows application) cannot listen
+      # on an HTTP port without significant client configuration.
+
+      # When you use this value, your application can then detect that the page has loaded, and can
+      # read the title of the HTML page to obtain the authorization code. It is then up to your
+      # application to close the browser window if you want to ensure that the user never sees the
+      # page that contains the authorization code. The mechanism for doing this varies from platform
+      # to platform.
+      #
+      # If your platform doesn't allow you to detect that the page has loaded or read the title of
+      # the page, you can have the user paste the code back to your application, as prompted by the
+      # text in the confirmation page that the OAuth 2.0 server generates.
+      IETF_WG_OAUTH2_OOB = "urn:ietf:wg:oauth:2.0:oob"
+
+      # This is identical to urn:ietf:wg:oauth:2.0:oob, but the text in the confirmation page that
+      # the OAuth 2.0 server generates won't instruct the user to copy the authorization code, but
+      # instead will simply ask the user to close the window.
+      #
+      # This is useful when your application reads the title of the HTML page (by checking window
+      # titles on the desktop, for example) to obtain the authorization code, but can't close the
+      # page on its own.
+      IETF_WG_OAUTH2_OOB_AUTO = "urn:ietf:wg:oauth:2.0:oob:auto"
+
+      IETF_WG_OAUTH2_OOB_METHODS = [IETF_WG_OAUTH2_OOB, IETF_WG_OAUTH2_OOB_AUTO].freeze
+    end
+  end
+end

lib/generators/doorkeeper/templates/initializer.rb

@@ -196,15 +196,6 @@
   #
   # access_token_methods :from_bearer_authorization, :from_access_token_param, :from_bearer_param
 
-  # Change the native redirect uri for client apps
-  # When clients register with the following redirect uri, they won't be redirected to
-  # any server and the authorizationcode will be displayed within the provider
-  # The value can be any string. Use nil to disable this feature. When disabled, clients
-  # must providea valid URL
-  # (Similar behaviour: https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi)
-  #
-  # native_redirect_uri 'urn:ietf:wg:oauth:2.0:oob'
-
   # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled
   # by default in non-development environments). OAuth2 delegates security in
   # communication to the HTTPS protocol so it is wise to keep this enabled.

spec/controllers/applications_controller_spec.rb

@@ -48,6 +48,21 @@ module Doorkeeper
         expect(json_response).to include("errors")
       end
 
+      it "returns validations on wrong create params (unspecified scheme)" do
+        expect do
+          post :create, params: {
+            doorkeeper_application: {
+              name: "Example",
+              redirect_uri: "app.com:80",
+            }, format: :json,
+          }
+        end.not_to(change { Doorkeeper::Application.count })
+
+        expect(response).to have_http_status(422)
+
+        expect(json_response).to include("errors")
+      end
+
       it "returns application info" do
         application = FactoryBot.create(:application, name: "Change me")
 

spec/dummy/config/initializers/doorkeeper.rb

@@ -65,15 +65,6 @@
   # Check out the wiki for more information on customization
   # access_token_methods :from_bearer_authorization, :from_access_token_param, :from_bearer_param
 
-  # Change the native redirect uri for client apps
-  # When clients register with the following redirect uri, they won't be redirected to any server and
-  # the authorization code will be displayed within the provider
-  # The value can be any string. Use nil to disable this feature.
-  # When disabled, clients must provide a valid URL
-  # (Similar behaviour: https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi)
-  #
-  # native_redirect_uri 'urn:ietf:wg:oauth:2.0:oob'
-
   # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled
   # by default in non-development environments). OAuth2 delegates security in
   # communication to the HTTPS protocol so it is wise to keep this enabled.

spec/lib/config_spec.rb

@@ -694,4 +694,15 @@
       end
     end
   end
+
+  describe "options deprecation" do
+    it "prints a warning message when an option is deprecated" do
+      expect(Kernel).to receive(:warn).with(
+        "[DOORKEEPER] native_redirect_uri has been deprecated and will soon be removed"
+      )
+      Doorkeeper.configure do
+        native_redirect_uri "urn:ietf:wg:oauth:2.0:oob"
+      end
+    end
+  end
 end

spec/lib/oauth/helpers/uri_checker_spec.rb

@@ -40,13 +40,28 @@ module Doorkeeper::OAuth::Helpers
         expect(URIChecker.valid?(uri)).to be_falsey
       end
 
+      it "is invalid if localhost is resolved as as scheme (no scheme specified)" do
+        uri = "localhost:8080"
+        expect(URIChecker.valid?(uri)).to be_falsey
+      end
+
+      it "is invalid if scheme is missing #2" do
+        uri = "app.co:80"
+        expect(URIChecker.valid?(uri)).to be_falsey
+      end
+
       it "is invalid if is not an uri" do
         uri = "   "
         expect(URIChecker.valid?(uri)).to be_falsey
       end
 
-      it "is valid for native uris" do
-        uri = "urn:ietf:wg:oauth:2.0:oob"
+      it "is valid for custom schemes" do
+        uri = "com.example.app:/test"
+        expect(URIChecker.valid?(uri)).to be_truthy
+      end
+
+      it "is valid for custom schemes with authority marker (common misconfiguration)" do
+        uri = "com.example.app://test"
         expect(URIChecker.valid?(uri)).to be_truthy
       end
     end

spec/lib/oauth/pre_authorization_spec.rb

@@ -149,21 +149,6 @@ module Doorkeeper::OAuth
       expect(subject.scopes).to eq(Scopes.from_string("default"))
     end
 
-    context "with native redirect uri" do
-      let(:native_redirect_uri) { "urn:ietf:wg:oauth:2.0:oob" }
-
-      it "accepts redirect_uri when it matches with the client" do
-        subject.redirect_uri = native_redirect_uri
-        allow(subject.client).to receive(:redirect_uri) { native_redirect_uri }
-        expect(subject).to be_authorizable
-      end
-
-      it "invalidates redirect_uri when it does'n match with the client" do
-        subject.redirect_uri = "urn:ietf:wg:oauth:2.0:oob"
-        expect(subject).not_to be_authorizable
-      end
-    end
-
     it "matches the redirect uri against client's one" do
       subject.redirect_uri = "http://nothesame.com"
       expect(subject).not_to be_authorizable