Merge pull request #1727 from doorkeeper-gem/fixes/allow-null-app-secret

Allow to set null secret value for Applications if it's public

CHANGELOG.md

@@ -13,6 +13,7 @@ Add your entry here.
 - [#1714] Fix `Doorkeeper::AccessToken.find_or_create_for` with empty scopes which raises NoMethodError
 - [#1712] Add `Pragma: no-cache` to token response
 - [#1726] Refactor token introspection class.
+- [#1727] Allow to set null secret value for Applications if they are public.
 
 ## 5.7.1
 

Gemfile

@@ -23,7 +23,7 @@ gem "rubocop-rspec", require: false
 gem "bcrypt", "~> 3.1", require: false
 
 gem "activerecord-jdbcsqlite3-adapter", platform: :jruby
-gem "sqlite3", "~> 2.0", platform: %i[ruby mswin mingw x64_mingw]
+gem "sqlite3", "~> 1.4", platform: [:ruby, :mswin, :mingw, :x64_mingw]
 
 gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw]
 gem "timecop"

lib/doorkeeper/orm/active_record/mixins/application.rb

@@ -20,11 +20,13 @@ module Application
                dependent: :delete_all,
                class_name: Doorkeeper.config.access_token_class.to_s
 
-      validates :name, :secret, :uid, presence: true
+      validates :name, :uid, presence: true
+      validates :secret, presence: true, if: -> { secret_required? }
       validates :uid, uniqueness: { case_sensitive: true }
-      validates_with Doorkeeper::RedirectUriValidator, attributes: [:redirect_uri]
       validates :confidential, inclusion: { in: [true, false] }
 
+      validates_with Doorkeeper::RedirectUriValidator, attributes: [:redirect_uri]
+
       validate :scopes_match_configured, if: :enforce_scopes?
 
       before_validation :generate_uid, :generate_secret, on: :create
@@ -118,7 +120,7 @@ def generate_uid
       end
 
       def generate_secret
-        return if secret.present?
+        return if secret.present? || !secret_required?
 
         renew_secret
       end
@@ -136,6 +138,11 @@ def enforce_scopes?
         Doorkeeper.config.enforce_configured_scopes?
       end
 
+      def secret_required?
+        confidential? ||
+          !self.class.columns.detect { |column| column.name == "secret" }&.null
+      end
+
       # Helper method to extract collection of serializable attribute names
       # considering serialization options (like `only`, `except` and so on).
       #

lib/generators/doorkeeper/remove_applications_secret_not_null_constraint_generator.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require "rails/generators"
+require "rails/generators/active_record"
+
+module Doorkeeper
+  # Generates migration with which drops NOT NULL constraint and allows not
+  # to bloat the database with redundant secret value.
+  #
+  class RemoveApplicationSecretNotNullConstraint < ::Rails::Generators::Base
+    include ::Rails::Generators::Migration
+    source_root File.expand_path("templates", __dir__)
+    desc "Removes NOT NULL constraint for OAuth2 applications."
+
+    def enable_polymorphic_resource_owner
+      migration_template(
+        "remove_applications_secret_not_null_constraint.rb.erb",
+        "db/migrate/remove_applications_secret_not_null_constraint.rb",
+        migration_version: migration_version,
+      )
+    end
+
+    def self.next_migration_number(dirname)
+      ActiveRecord::Generators::Base.next_migration_number(dirname)
+    end
+
+    private
+
+    def migration_version
+      "[#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}]"
+    end
+  end
+end

lib/generators/doorkeeper/templates/migration.rb.erb

@@ -5,6 +5,7 @@ class CreateDoorkeeperTables < ActiveRecord::Migration<%= migration_version %>
     create_table :oauth_applications do |t|
       t.string  :name,    null: false
       t.string  :uid,     null: false
+      # Remove `null: false` or use conditional constraint if you are planning to use public clients.
       t.string  :secret,  null: false
 
       # Remove `null: false` if you are planning to use grant flows

lib/generators/doorkeeper/templates/remove_applications_secret_not_null_constraint.rb.erb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class RemoveApplicationsSecretNotNullConstraint < ActiveRecord::Migration<%= migration_version %>
+  def change
+    change_column_null :oauth_applications, :secret, true
+  end
+end

spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb

@@ -5,7 +5,7 @@ def change
     create_table :oauth_applications do |t|
       t.string  :name,    null: false
       t.string  :uid,     null: false
-      t.string  :secret,  null: false
+      t.string  :secret
 
       # Remove `null: false` if you are planning to use grant flows
       # that doesn't require redirect URI to be used during authorization

spec/dummy/db/schema.rb

@@ -50,7 +50,7 @@
   create_table "oauth_applications", force: :cascade do |t|
     t.string "name", null: false
     t.string "uid", null: false
-    t.string "secret", null: false
+    t.string "secret"
     t.text "redirect_uri"
     t.string "scopes", default: "", null: false
     t.datetime "created_at", null: false

spec/generators/remove_applications_secret_not_null_constraint_generator_spec.rb

@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+require "generators/doorkeeper/remove_applications_secret_not_null_constraint_generator"
+
+RSpec.describe Doorkeeper::RemoveApplicationSecretNotNullConstraint do
+  include GeneratorSpec::TestCase
+
+  tests described_class
+  destination ::File.expand_path('tmp/dummy', __dir__)
+
+  describe "after running the generator" do
+    before do
+      prepare_destination
+    end
+
+    it "creates a migration with a version specifier" do
+      run_generator
+
+      assert_migration "db/migrate/remove_applications_secret_not_null_constraint.rb" do |migration|
+        assert migration.include?("change_column_null :oauth_applications, :secret")
+      end
+    end
+  end
+end

spec/models/doorkeeper/application_spec.rb

@@ -82,6 +82,12 @@
     expect(new_application).not_to be_valid
   end
 
+  it "is valid without secret if client is public" do
+    new_application.confidential = false
+    new_application.secret = nil
+    expect(new_application).to be_valid
+  end
+
   it "generates a secret using a custom object" do
     module CustomGeneratorArgs
       def self.generate