From 5919618a87675cfcba9a62621c8b0eb07587ee96 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 7 Feb 2020 17:32:35 +0000 Subject: [PATCH] DEV: Drop legacy OpenID 2.0 support (#8894) This is not used in core or official plugins, and has been printing a deprecation notice since v2.3.0beta4. All OpenID 2.0 code and dependencies have been dropped. The user_open_ids table remains for now, in case anyone has missed the deprecation notice, and needs to migrate their data. Context at https://meta.discourse.org/t/-/113249 --- Gemfile | 2 - Gemfile.lock | 12 -- app/models/user.rb | 1 - app/models/user_open_id.rb | 9 ++ app/services/user_anonymizer.rb | 1 - config/application.rb | 3 - config/initializers/009-omniauth.rb | 1 - lib/auth.rb | 1 - lib/auth/open_id_authenticator.rb | 109 ------------------ script/bulk_import/discourse_merger.rb | 2 +- .../auth/open_id_authenticator_spec.rb | 56 --------- spec/components/plugin/instance_spec.rb | 2 +- spec/fixtures/plugins/my_plugin/plugin.rb | 2 +- spec/models/user_open_id_spec.rb | 10 -- spec/services/user_anonymizer_spec.rb | 2 - spec/services/user_merger_spec.rb | 2 - 16 files changed, 12 insertions(+), 203 deletions(-) delete mode 100644 lib/auth/open_id_authenticator.rb delete mode 100644 spec/components/auth/open_id_authenticator_spec.rb delete mode 100644 spec/models/user_open_id_spec.rb diff --git a/Gemfile b/Gemfile index e6df12d56ba..82fbc701194 100644 --- a/Gemfile +++ b/Gemfile @@ -97,8 +97,6 @@ gem 'nokogiri' gem 'css_parser', require: false gem 'omniauth' -gem 'omniauth-openid' -gem 'openid-redis-store' gem 'omniauth-facebook' gem 'omniauth-twitter' gem 'omniauth-instagram' diff --git a/Gemfile.lock b/Gemfile.lock index 82114deca2d..aeff1353015 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -237,9 +237,6 @@ GEM omniauth-oauth2 (1.6.0) oauth2 (~> 1.1) omniauth (~> 1.9) - omniauth-openid (1.0.1) - omniauth (~> 1.0) - rack-openid (~> 1.3.1) omniauth-twitter (1.4.0) omniauth-oauth (~> 1.1) rack @@ -250,9 +247,6 @@ GEM mustache nokogiri (~> 1.7) sanitize - openid-redis-store (0.0.2) - redis - ruby-openid openssl-signature_algorithm (0.3.0) optimist (3.0.0) parallel (1.19.1) @@ -276,9 +270,6 @@ GEM rack (2.0.8) rack-mini-profiler (1.1.6) rack (>= 1.2.0) - rack-openid (1.3.1) - rack (>= 1.1.0) - ruby-openid (>= 2.1.8) rack-protection (2.0.8.1) rack rack-test (1.1.0) @@ -356,7 +347,6 @@ GEM unicode-display_width (>= 1.4.0, < 1.7) rubocop-discourse (1.0.2) rubocop (>= 0.69.0) - ruby-openid (2.9.2) ruby-prof (1.2.0) ruby-progressbar (1.10.1) ruby-readability (0.7.0) @@ -502,10 +492,8 @@ DEPENDENCIES omniauth-google-oauth2 omniauth-instagram omniauth-oauth2 - omniauth-openid omniauth-twitter onebox - openid-redis-store parallel_tests pg pry-nav diff --git a/app/models/user.rb b/app/models/user.rb index e3d83110da7..c4aa017b8da 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -14,7 +14,6 @@ class User < ActiveRecord::Base has_many :tag_users, dependent: :destroy has_many :user_api_keys, dependent: :destroy has_many :topics - has_many :user_open_ids, dependent: :destroy # dependent deleting handled via before_destroy has_many :user_actions diff --git a/app/models/user_open_id.rb b/app/models/user_open_id.rb index 934b0ef48cc..3c5b1d457f6 100644 --- a/app/models/user_open_id.rb +++ b/app/models/user_open_id.rb @@ -1,10 +1,19 @@ # frozen_string_literal: true +# This table is no longer used in core, but may be used by unofficial plugins class UserOpenId < ActiveRecord::Base + after_initialize :raise_deprecation_error + belongs_to :user validates_presence_of :email validates_presence_of :url + + private + + def raise_deprecation_error + raise "The user_open_ids table has been deprecated, and will be dropped in v2.5. See https://meta.discourse.org/t/-/113249" + end end # == Schema Information diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index bb5bd3f7054..2760e3e387e 100644 --- a/app/services/user_anonymizer.rb +++ b/app/services/user_anonymizer.rb @@ -64,7 +64,6 @@ class UserAnonymizer @user.single_sign_on_record.try(:destroy) @user.oauth2_user_infos.try(:destroy_all) @user.user_associated_accounts.try(:destroy_all) - @user.user_open_ids.find_each { |x| x.destroy } @user.api_keys.find_each { |x| x.try(:destroy) } @user.user_emails.secondary.destroy_all diff --git a/config/application.rb b/config/application.rb index e3bc620c5f9..b0d334bb0d5 100644 --- a/config/application.rb +++ b/config/application.rb @@ -299,9 +299,6 @@ module Discourse # Ensure that Discourse event triggers for web hooks are loaded require_dependency 'web_hook' - # So open id logs somewhere sane - OpenID::Util.logger = Rails.logger - # Load plugins plugin_initialization_guard do Discourse.plugins.each(&:notify_after_initialize) diff --git a/config/initializers/009-omniauth.rb b/config/initializers/009-omniauth.rb index 6acc2856429..c8bfbfafb7d 100644 --- a/config/initializers/009-omniauth.rb +++ b/config/initializers/009-omniauth.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "openssl" -require "openid_redis_store" require "middleware/omniauth_bypass_middleware" Rails.application.config.middleware.use Middleware::OmniauthBypassMiddleware diff --git a/lib/auth.rb b/lib/auth.rb index b48a22d6c45..5febb7aedc1 100644 --- a/lib/auth.rb +++ b/lib/auth.rb @@ -7,7 +7,6 @@ require 'auth/result' require 'auth/authenticator' require 'auth/managed_authenticator' require 'auth/facebook_authenticator' -require 'auth/open_id_authenticator' require 'auth/github_authenticator' require 'auth/twitter_authenticator' require 'auth/google_oauth2_authenticator' diff --git a/lib/auth/open_id_authenticator.rb b/lib/auth/open_id_authenticator.rb deleted file mode 100644 index 3dc60b3d65b..00000000000 --- a/lib/auth/open_id_authenticator.rb +++ /dev/null @@ -1,109 +0,0 @@ -# frozen_string_literal: true - -class Auth::OpenIdAuthenticator < Auth::Authenticator - - attr_reader :name, :identifier - - def initialize(name, identifier, enabled_site_setting, opts = {}) - @name = name - @identifier = identifier - @enabled_site_setting = enabled_site_setting - @opts = opts - end - - def enabled? - SiteSetting.get(@enabled_site_setting) - end - - def description_for_user(user) - info = UserOpenId.where("url LIKE ?", "#{@identifier}%").find_by(user_id: user.id) - info&.email || "" - end - - def can_revoke? - true - end - - def revoke(user, skip_remote: false) - info = UserOpenId.where("url LIKE ?", "#{@identifier}%").find_by(user_id: user.id) - raise Discourse::NotFound if info.nil? - - info.destroy! - true - end - - def can_connect_existing_user? - true - end - - def after_authenticate(auth_token, existing_account: nil) - Discourse.deprecate("OpenID Authentication has been deprecated, please migrate to OAuth2 or OpenID Connect", since: "2.3.0beta4", drop_from: "2.4") - result = Auth::Result.new - - data = auth_token[:info] - identity_url = auth_token[:extra][:response].identity_url - result.email = email = data[:email] - - raise Discourse::InvalidParameters.new(:email) if email.blank? - - # If the auth supplies a name / username, use those. Otherwise start with email. - result.name = data[:name] || data[:email] - result.username = data[:nickname] || data[:email] - - user_open_id = UserOpenId.find_by_url(identity_url) - - if existing_account && (user_open_id.nil? || existing_account.id != user_open_id.user_id) - user_open_id.destroy! if user_open_id - user_open_id = UserOpenId.create!(url: identity_url , user_id: existing_account.id, email: email, active: true) - end - - if !user_open_id && @opts[:trusted] && user = User.find_by_email(email) - user_open_id = UserOpenId.create(url: identity_url , user_id: user.id, email: email, active: true) - end - - result.user = user_open_id.try(:user) - result.extra_data = { - openid_url: identity_url, - # note email may change by the time after_create_account runs - email: email - } - - result.email_valid = @opts[:trusted] - - result - end - - def after_create_account(user, auth) - data = auth[:extra_data] - UserOpenId.create( - user_id: user.id, - url: data[:openid_url], - email: data[:email], - active: true - ) - end - - def register_middleware(omniauth) - omniauth.provider :open_id, - setup: lambda { |env| - strategy = env["omniauth.strategy"] - strategy.options[:store] = OpenID::Store::Redis.new(Discourse.redis) - - # Add CSRF protection in addition to OpenID Specification - def strategy.query_string - session["omniauth.state"] = state = SecureRandom.hex(24) - "?state=#{state}" - end - - def strategy.callback_phase - stored_state = session.delete("omniauth.state") - provided_state = request.params["state"] - return fail!(:invalid_credentials) unless provided_state == stored_state - super - end - }, - name: name, - identifier: identifier, - require: "omniauth-openid" - end -end diff --git a/script/bulk_import/discourse_merger.rb b/script/bulk_import/discourse_merger.rb index abe45f492d4..9029bdb28a8 100644 --- a/script/bulk_import/discourse_merger.rb +++ b/script/bulk_import/discourse_merger.rb @@ -148,7 +148,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base [ UserEmail, UserStat, UserOption, UserProfile, - UserVisit, UserSearchData, GivenDailyLike, UserSecondFactor, UserOpenId + UserVisit, UserSearchData, GivenDailyLike, UserSecondFactor ].each do |c| copy_model(c, skip_if_merged: true, is_a_user_model: true, skip_processing: true) end diff --git a/spec/components/auth/open_id_authenticator_spec.rb b/spec/components/auth/open_id_authenticator_spec.rb deleted file mode 100644 index 2ebf7e9ee64..00000000000 --- a/spec/components/auth/open_id_authenticator_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe Auth::OpenIdAuthenticator do - - it "can lookup pre-existing user if trusted" do - auth = Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) - - user = Fabricate(:user) - response = OpenStruct.new(identity_url: 'abc') - result = auth.after_authenticate(info: { email: user.email }, extra: { response: response }) - expect(result.user).to eq(user) - end - - it "raises an exception when email is missing" do - auth = Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) - response = OpenStruct.new(identity_url: 'abc') - expect { auth.after_authenticate(info: {}, extra: { response: response }) }.to raise_error(Discourse::InvalidParameters) - end - - it 'can connect to a different existing user account' do - authenticator = Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) - user1 = Fabricate(:user) - user2 = Fabricate(:user) - - UserOpenId.create!(url: "id/123" , user_id: user1.id, email: "bob@example.com", active: true) - - hash = { - info: { email: user1.email }, extra: { response: OpenStruct.new(identity_url: 'id/123') } - } - - result = authenticator.after_authenticate(hash, existing_account: user2) - - expect(result.user.id).to eq(user2.id) - expect(UserOpenId.exists?(user_id: user1.id)).to eq(false) - expect(UserOpenId.exists?(user_id: user2.id)).to eq(true) - end - - context 'revoke' do - fab!(:user) { Fabricate(:user) } - let(:authenticator) { Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) } - - it 'raises exception if no entry for user' do - expect { authenticator.revoke(user) }.to raise_error(Discourse::NotFound) - end - - it 'revokes correctly' do - UserOpenId.create!(url: "id/123" , user_id: user.id, email: "bob@example.com", active: true) - expect(authenticator.can_revoke?).to eq(true) - expect(authenticator.revoke(user)).to eq(true) - expect(authenticator.description_for_user(user)).to eq("") - end - - end -end diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb index 50064c3ad84..bd33bb4847b 100644 --- a/spec/components/plugin/instance_spec.rb +++ b/spec/components/plugin/instance_spec.rb @@ -245,7 +245,7 @@ describe Plugin::Instance do plugin.notify_before_auth expect(DiscoursePluginRegistry.auth_providers.count).to eq(1) auth_provider = DiscoursePluginRegistry.auth_providers.to_a[0] - expect(auth_provider.authenticator.name).to eq('ubuntu') + expect(auth_provider.authenticator.name).to eq('facebook') end it "finds all the custom assets" do diff --git a/spec/fixtures/plugins/my_plugin/plugin.rb b/spec/fixtures/plugins/my_plugin/plugin.rb index 501786da7d6..d55f50b08a6 100644 --- a/spec/fixtures/plugins/my_plugin/plugin.rb +++ b/spec/fixtures/plugins/my_plugin/plugin.rb @@ -6,7 +6,7 @@ # authors: Frank Zappa auth_provider title: 'with Ubuntu', - authenticator: Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', 'enable_badges', trusted: true), + authenticator: Auth::FacebookAuthenticator.new, message: 'Authenticating with Ubuntu (make sure pop up blockers are not enbaled)', frame_width: 1000, # the frame size used for the pop up window, overrides default frame_height: 800 diff --git a/spec/models/user_open_id_spec.rb b/spec/models/user_open_id_spec.rb deleted file mode 100644 index e6ce156d737..00000000000 --- a/spec/models/user_open_id_spec.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe UserOpenId do - - it { is_expected.to belong_to :user } - it { is_expected.to validate_presence_of :email } - it { is_expected.to validate_presence_of :url } -end diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 36ec4f0b779..5e9a64b7fca 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -203,14 +203,12 @@ describe UserAnonymizer do user.user_associated_accounts = [UserAssociatedAccount.create(user_id: user.id, provider_uid: "example", provider_name: "facebook")] user.single_sign_on_record = SingleSignOnRecord.create(user_id: user.id, external_id: "example", last_payload: "looks good") user.oauth2_user_infos = [Oauth2UserInfo.create(user_id: user.id, uid: "example", provider: "example")] - UserOpenId.create(user_id: user.id, email: user.email, url: "http://example.com/openid", active: true) make_anonymous user.reload expect(user.github_user_info).to eq(nil) expect(user.user_associated_accounts).to be_empty expect(user.single_sign_on_record).to eq(nil) expect(user.oauth2_user_infos).to be_empty - expect(user.user_open_ids.count).to eq(0) end it "removes api key" do diff --git a/spec/services/user_merger_spec.rb b/spec/services/user_merger_spec.rb index d95e8ca3101..a239d2c2b62 100644 --- a/spec/services/user_merger_spec.rb +++ b/spec/services/user_merger_spec.rb @@ -994,7 +994,6 @@ describe UserMerger do GithubUserInfo.create(user_id: source_user.id, screen_name: "example", github_user_id: "examplel123123") Oauth2UserInfo.create(user_id: source_user.id, uid: "example", provider: "example") SingleSignOnRecord.create(user_id: source_user.id, external_id: "example", last_payload: "looks good") - UserOpenId.create(user_id: source_user.id, email: source_user.email, url: "http://example.com/openid", active: true) merge_users! @@ -1002,7 +1001,6 @@ describe UserMerger do expect(GithubUserInfo.where(user_id: source_user.id).count).to eq(0) expect(Oauth2UserInfo.where(user_id: source_user.id).count).to eq(0) expect(SingleSignOnRecord.where(user_id: source_user.id).count).to eq(0) - expect(UserOpenId.where(user_id: source_user.id).count).to eq(0) end it "deletes auth tokens" do