From f282c2595dd84bd5c16812b8d56d4991bb1c8cdf Mon Sep 17 00:00:00 2001 From: Nikita Bulai Date: Tue, 13 Aug 2024 15:49:23 +0300 Subject: [PATCH] Allow to set null secret value for Applications if it's public --- CHANGELOG.md | 1 + Gemfile | 2 +- .../orm/active_record/mixins/application.rb | 13 ++++++-- ...ns_secret_not_null_constraint_generator.rb | 33 +++++++++++++++++++ .../doorkeeper/templates/migration.rb.erb | 1 + ...ications_secret_not_null_constraint.rb.erb | 7 ++++ ...20151223192035_create_doorkeeper_tables.rb | 2 +- spec/dummy/db/schema.rb | 2 +- ...cret_not_null_constraint_generator_spec.rb | 25 ++++++++++++++ spec/models/doorkeeper/application_spec.rb | 6 ++++ 10 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 lib/generators/doorkeeper/remove_applications_secret_not_null_constraint_generator.rb create mode 100644 lib/generators/doorkeeper/templates/remove_applications_secret_not_null_constraint.rb.erb create mode 100644 spec/generators/remove_applications_secret_not_null_constraint_generator_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index e0454a863..206ea53c3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Gemfile b/Gemfile index f2cdb76b8..0caed60fc 100644 --- a/Gemfile +++ b/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" diff --git a/lib/doorkeeper/orm/active_record/mixins/application.rb b/lib/doorkeeper/orm/active_record/mixins/application.rb index 45916955d..9ea8b129f 100644 --- a/lib/doorkeeper/orm/active_record/mixins/application.rb +++ b/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). # diff --git a/lib/generators/doorkeeper/remove_applications_secret_not_null_constraint_generator.rb b/lib/generators/doorkeeper/remove_applications_secret_not_null_constraint_generator.rb new file mode 100644 index 000000000..f30c8071c --- /dev/null +++ b/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 diff --git a/lib/generators/doorkeeper/templates/migration.rb.erb b/lib/generators/doorkeeper/templates/migration.rb.erb index bf1798b23..c0afb149a 100644 --- a/lib/generators/doorkeeper/templates/migration.rb.erb +++ b/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 diff --git a/lib/generators/doorkeeper/templates/remove_applications_secret_not_null_constraint.rb.erb b/lib/generators/doorkeeper/templates/remove_applications_secret_not_null_constraint.rb.erb new file mode 100644 index 000000000..f42d99a82 --- /dev/null +++ b/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 diff --git a/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb b/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb index 3220b983e..94e361e21 100644 --- a/spec/dummy/db/migrate/20151223192035_create_doorkeeper_tables.rb +++ b/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 diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index 64160d50b..7ed67be14 100644 --- a/spec/dummy/db/schema.rb +++ b/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 diff --git a/spec/generators/remove_applications_secret_not_null_constraint_generator_spec.rb b/spec/generators/remove_applications_secret_not_null_constraint_generator_spec.rb new file mode 100644 index 000000000..d01bfaea2 --- /dev/null +++ b/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 diff --git a/spec/models/doorkeeper/application_spec.rb b/spec/models/doorkeeper/application_spec.rb index ed636a67e..d45e0c2b2 100644 --- a/spec/models/doorkeeper/application_spec.rb +++ b/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