From 213ce33af25ae98ee8e2d406f410d0a1532a15d2 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 26 Aug 2013 11:04:16 +1000 Subject: [PATCH] Fixed all broken specs Moved middleware config into authenticators --- .../users/omniauth_callbacks_controller.rb | 10 +- config/application.rb | 1 + config/initializers/09-omniauth.rb | 11 ++ config/initializers/omniauth.rb | 72 ------- lib/auth.rb | 9 + lib/auth/authenticator.rb | 12 +- lib/auth/cas_authenticator.rb | 5 + lib/auth/facebook_authenticator.rb | 12 ++ lib/auth/github_authenticator.rb | 11 ++ lib/auth/open_id_authenticator.rb | 20 +- lib/auth/persona_authenticator.rb | 7 + lib/auth/twitter_authenticator.rb | 9 + lib/discourse.rb | 10 + lib/plugin/auth_provider.rb | 3 +- lib/plugin/instance.rb | 14 +- spec/components/plugin/instance_spec.rb | 18 +- spec/components/plugin/metadata_spec.rb | 18 -- .../omniauth_callbacks_controller_spec.rb | 187 ------------------ spec/controllers/users_controller_spec.rb | 26 +-- .../complete.html.erb_spec.rb | 10 +- 20 files changed, 137 insertions(+), 328 deletions(-) create mode 100644 config/initializers/09-omniauth.rb delete mode 100644 config/initializers/omniauth.rb create mode 100644 lib/auth.rb diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 895927b711c..25e8a96b891 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -2,19 +2,13 @@ require_dependency 'email' require_dependency 'enum' require_dependency 'user_name_suggester' -require_dependency 'auth/authenticator' -require_dependency 'auth/facebook_authenticator' -require_dependency 'auth/open_id_authenticator' -require_dependency 'auth/github_authenticator' -require_dependency 'auth/twitter_authenticator' -require_dependency 'auth/persona_authenticator' class Users::OmniauthCallbacksController < ApplicationController BUILTIN_AUTH = [ Auth::FacebookAuthenticator.new, - Auth::OpenIdAuthenticator.new("google", trusted: true), - Auth::OpenIdAuthenticator.new("yahoo", trusted: true), + Auth::OpenIdAuthenticator.new("google", "https://www.google.com/accounts/o8/id", trusted: true), + Auth::OpenIdAuthenticator.new("yahoo", "https://me.yahoo.com", trusted: true), Auth::GithubAuthenticator.new, Auth::TwitterAuthenticator.new, Auth::PersonaAuthenticator.new diff --git a/config/application.rb b/config/application.rb index 5c9d53e3d58..6fb47f3698b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -121,6 +121,7 @@ module Discourse unless Rails.env.test? require 'plugin' + require_dependency 'auth' Discourse.activate_plugins! end diff --git a/config/initializers/09-omniauth.rb b/config/initializers/09-omniauth.rb new file mode 100644 index 00000000000..1632ba0250c --- /dev/null +++ b/config/initializers/09-omniauth.rb @@ -0,0 +1,11 @@ +require "openssl" +require "openid_redis_store" + +# if you need to test this and are having ssl issues see: +# http://stackoverflow.com/questions/6756460/openssl-error-using-omniauth-specified-ssl-path-but-didnt-work + +Rails.application.config.middleware.use OmniAuth::Builder do + Discourse.authenticators.each do |authenticator| + authenticator.register_middleware(self) + end +end diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb deleted file mode 100644 index 0d1eedc3d56..00000000000 --- a/config/initializers/omniauth.rb +++ /dev/null @@ -1,72 +0,0 @@ -require "openssl" -require "openid_redis_store" - -# if you need to test this and are having ssl issues see: -# http://stackoverflow.com/questions/6756460/openssl-error-using-omniauth-specified-ssl-path-but-didnt-work - -Rails.application.config.middleware.use OmniAuth::Builder do - - provider :open_id, - :store => OpenID::Store::Redis.new($redis), - :name => "google", - :identifier => "https://www.google.com/accounts/o8/id", - :require => "omniauth-openid" - - provider :open_id, - :store => OpenID::Store::Redis.new($redis), - :name => "yahoo", - :identifier => "https://me.yahoo.com", - :require => "omniauth-openid" - - Discourse.auth_providers.each do |p| - if p.type == :open_id - provider :open_id, { - :name => p.name, - :store => OpenID::Store::Redis.new($redis), - :require => "omniauth-openid" - }.merge(p.options) - elsif p.type == :oauth2 - provider :oauth2, - p.options[:client_id], - p.options[:client_secret], - { - :name => p.name, - :require => "omniauth-oauth2" - }.merge(p.options) - else - provider p.type, *p.options - end - end - - # lambda is required for proper multisite support, - # without it subdomains will not function correctly - provider :facebook, - :setup => lambda { |env| - strategy = env["omniauth.strategy"] - strategy.options[:client_id] = SiteSetting.facebook_app_id - strategy.options[:client_secret] = SiteSetting.facebook_app_secret - }, - :scope => "email" - - provider :twitter, - :setup => lambda { |env| - strategy = env["omniauth.strategy"] - strategy.options[:consumer_key] = SiteSetting.twitter_consumer_key - strategy.options[:consumer_secret] = SiteSetting.twitter_consumer_secret - } - - provider :github, - :setup => lambda { |env| - strategy = env["omniauth.strategy"] - strategy.options[:client_id] = SiteSetting.github_client_id - strategy.options[:client_secret] = SiteSetting.github_client_secret - }, - :scope => "user:email" - - provider :browser_id, - :name => "persona" - - provider :cas, - :host => SiteSetting.cas_hostname - -end diff --git a/lib/auth.rb b/lib/auth.rb new file mode 100644 index 00000000000..ead065a5a9e --- /dev/null +++ b/lib/auth.rb @@ -0,0 +1,9 @@ +module Auth; end + +require_dependency 'auth/result' +require_dependency 'auth/authenticator' +require_dependency 'auth/facebook_authenticator' +require_dependency 'auth/open_id_authenticator' +require_dependency 'auth/github_authenticator' +require_dependency 'auth/twitter_authenticator' +require_dependency 'auth/persona_authenticator' diff --git a/lib/auth/authenticator.rb b/lib/auth/authenticator.rb index 351d3591e53..f6a67f3f3e6 100644 --- a/lib/auth/authenticator.rb +++ b/lib/auth/authenticator.rb @@ -1,9 +1,5 @@ # this class is used by the user and omniauth controllers, it controls how -# an authentication system interacts with our database - -module Auth; end - -require 'auth/result' +# an authentication system interacts with our database and middleware class Auth::Authenticator def after_authenticate(auth_options) @@ -21,4 +17,10 @@ class Auth::Authenticator def lookup_user(user_info, email) user_info.try(:user) || User.where(email: email).first end + + # hook used for registering omniauth middleware, + # without this we can not authenticate + def register_middleware(omniauth) + raise NotImplementedError + end end diff --git a/lib/auth/cas_authenticator.rb b/lib/auth/cas_authenticator.rb index 9adf9ff540a..3c9fc4f46ec 100644 --- a/lib/auth/cas_authenticator.rb +++ b/lib/auth/cas_authenticator.rb @@ -39,4 +39,9 @@ class Auth::CasAuthenticator < Auth::Authenticator result end + + def register_middleware(omniauth) + omniauth.provider :cas, + :host => SiteSetting.cas_hostname + end end diff --git a/lib/auth/facebook_authenticator.rb b/lib/auth/facebook_authenticator.rb index f8b4e895cea..d1e16208c66 100644 --- a/lib/auth/facebook_authenticator.rb +++ b/lib/auth/facebook_authenticator.rb @@ -30,6 +30,16 @@ class Auth::FacebookAuthenticator < Auth::Authenticator FacebookUserInfo.create({user_id: user.id}.merge(data)) end + def register_middleware(omniauth) + omniauth.provider :facebook, + :setup => lambda { |env| + strategy = env["omniauth.strategy"] + strategy.options[:client_id] = SiteSetting.facebook_app_id + strategy.options[:client_secret] = SiteSetting.facebook_app_secret + }, + :scope => "email" + end + protected def parse_auth_token(auth_token) @@ -53,4 +63,6 @@ class Auth::FacebookAuthenticator < Auth::Authenticator } end + + end diff --git a/lib/auth/github_authenticator.rb b/lib/auth/github_authenticator.rb index c24f76322d1..a5c82e09825 100644 --- a/lib/auth/github_authenticator.rb +++ b/lib/auth/github_authenticator.rb @@ -45,4 +45,15 @@ class Auth::GithubAuthenticator < Auth::Authenticator github_user_id: data[:github_user_id] ) end + + + def register_middleware(omniauth) + omniauth.provider :github, + :setup => lambda { |env| + strategy = env["omniauth.strategy"] + strategy.options[:client_id] = SiteSetting.github_client_id + strategy.options[:client_secret] = SiteSetting.github_client_secret + }, + :scope => "user:email" + end end diff --git a/lib/auth/open_id_authenticator.rb b/lib/auth/open_id_authenticator.rb index 667a34fd54e..77dec7b7e46 100644 --- a/lib/auth/open_id_authenticator.rb +++ b/lib/auth/open_id_authenticator.rb @@ -1,12 +1,11 @@ class Auth::OpenIdAuthenticator < Auth::Authenticator - def initialize(name, opts = {}) - @name = name - @opts = opts - end + attr_reader :name, :identifier - def name - @name + def initialize(name, identifier, opts = {}) + @name = name + @identifier = identifier + @opts = opts end def after_authenticate(auth_token) @@ -47,4 +46,13 @@ class Auth::OpenIdAuthenticator < Auth::Authenticator active: true ) end + + + def register_middleware(omniauth) + omniauth.provider :open_id, + :store => OpenID::Store::Redis.new($redis), + :name => name, + :identifier => identifier, + :require => "omniauth-openid" + end end diff --git a/lib/auth/persona_authenticator.rb b/lib/auth/persona_authenticator.rb index 029c4ba7cf0..3aa89829270 100644 --- a/lib/auth/persona_authenticator.rb +++ b/lib/auth/persona_authenticator.rb @@ -15,4 +15,11 @@ class Auth::PersonaAuthenticator < Auth::Authenticator result.user = User.find_by_email(email) result end + + def register_middleware(omniauth) + omniauth.provider :browser_id, + :name => "persona" + end end + + diff --git a/lib/auth/twitter_authenticator.rb b/lib/auth/twitter_authenticator.rb index f0728c4824e..43578da201d 100644 --- a/lib/auth/twitter_authenticator.rb +++ b/lib/auth/twitter_authenticator.rb @@ -37,4 +37,13 @@ class Auth::TwitterAuthenticator < Auth::Authenticator ) end + def register_middleware(omniauth) + omniauth.provider :twitter, + :setup => lambda { |env| + strategy = env["omniauth.strategy"] + strategy.options[:consumer_key] = SiteSetting.twitter_consumer_key + strategy.options[:consumer_secret] = SiteSetting.twitter_consumer_secret + } + end + end diff --git a/lib/discourse.rb b/lib/discourse.rb index 77b4f5fcf99..c67f5c8a399 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -35,6 +35,16 @@ module Discourse @plugins end + def self.authenticators + # TODO: perhaps we don't need auth providers and authenticators maybe one object is enough + + # NOTE: this bypasses the site settings and gives a list of everything, we need to register every middleware + # for the cases of multisite + # In future we may change it so we don't include them all for cases where we are not a multisite, but we would + # require a restart after site settings change + Users::OmniauthCallbacksController::BUILTIN_AUTH + auth_providers.map(&:authenticator) + end + def self.auth_providers providers = [] if plugins diff --git a/lib/plugin/auth_provider.rb b/lib/plugin/auth_provider.rb index 91d1081ca90..a7c90a2de1e 100644 --- a/lib/plugin/auth_provider.rb +++ b/lib/plugin/auth_provider.rb @@ -1,4 +1,5 @@ class Plugin::AuthProvider attr_accessor :type, :glyph, :background_color, :name, :title, - :message, :frame_width, :frame_height, :options, :callback + :message, :frame_width, :frame_height, :authenticator + end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index b621c56be95..cc2e8f4f660 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -5,7 +5,8 @@ require_dependency 'plugin/auth_provider' class Plugin::Instance - attr_reader :auth_providers, :assets, :path + attr_reader :auth_providers, :assets + attr_accessor :path, :metadata def self.find_all(parent_path) [].tap { |plugins| @@ -17,12 +18,16 @@ class Plugin::Instance } end - def initialize(metadata, path) + def initialize(metadata=nil, path=nil) @metadata = metadata @path = path @assets = [] end + def name + metadata.name + end + # will make sure all the assets this plugin needs are registered def generate_automatic_assets! paths = [] @@ -152,13 +157,10 @@ class Plugin::Instance @auth_providers ||= [] provider = Plugin::AuthProvider.new provider.type = type - [:name, :glyph, :background_color, :title, :message, :frame_width, :frame_height, :callback].each do |sym| + [:name, :glyph, :background_color, :title, :message, :frame_width, :frame_height, :authenticator].each do |sym| provider.send "#{sym}=", opts.delete(sym) end provider.name ||= type.to_s - provider.options = opts[:middleware_options] || opts - # prepare for splatting - provider.options = [provider.options] if provider.options.is_a? Hash @auth_providers << provider end diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb index e7566ce8541..cfb387c3bdf 100644 --- a/spec/components/plugin/instance_spec.rb +++ b/spec/components/plugin/instance_spec.rb @@ -3,9 +3,25 @@ require_dependency 'plugin/instance' describe Plugin::Instance do + context "find_all" do + it "can find plugins correctly" do + plugins = Plugin::Instance.find_all("#{Rails.root}/spec/fixtures/plugins") + plugins.count.should == 1 + plugin =plugins[0] + + plugin.name.should == "plugin-name" + plugin.path.should == "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" + end + + it "does not blow up on missing directory" do + plugins = Plugin::Instance.find_all("#{Rails.root}/frank_zappa") + plugins.count.should == 0 + end + end + context "activate!" do it "can activate plugins correctly" do - plugin = Plugin.new + plugin = Plugin::Instance.new plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" junk_file = "#{plugin.auto_generated_path}/junk" diff --git a/spec/components/plugin/metadata_spec.rb b/spec/components/plugin/metadata_spec.rb index 433df189115..9004cf5752a 100644 --- a/spec/components/plugin/metadata_spec.rb +++ b/spec/components/plugin/metadata_spec.rb @@ -9,8 +9,6 @@ describe Plugin::Metadata do # about: about: my plugin # version: 0.1 # authors: Frank Zappa -# gem: some_gem -# gem: some_gem, "1" some_ruby TEXT @@ -19,23 +17,7 @@ TEXT metadata.about.should == "about: my plugin" metadata.version.should == "0.1" metadata.authors.should == "Frank Zappa" - metadata.gems.should == ["some_gem", 'some_gem, "1"'] end end - context "find_all" do - it "can find plugins correctly" do - metadatas = Plugin::Metadata.find_all("#{Rails.root}/spec/fixtures/plugins") - metadatas.count.should == 1 - metadata = metadata[0] - - metadata.name.should == "plugin-name" - metadata.path.should == "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" - end - - it "does not blow up on missing directory" do - metadatas = Plugin.find_all("#{Rails.root}/frank_zappa") - metadatas.count.should == 0 - end - end end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 5823dbe6800..def39e3d485 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -20,193 +20,6 @@ describe Users::OmniauthCallbacksController do SiteSetting.stubs("enable_twitter_logins?").returns(true) expect(Users::OmniauthCallbacksController.find_authenticator("twitter")).not_to eq(nil) end - end - - - - # let(:auth) { {info: {email: 'eviltrout@made.up.email', name: 'Robin Ward', uid: 123456789}, "extra" => {"raw_info" => {} } } } - # let(:cas_auth) { { 'uid' => 'casuser', extra: { user: 'casuser'} } } - - # shared_examples_for "an authenticaton provider" do |provider| - # context "when #{provider} logins are disabled" do - # before do - # SiteSetting.stubs("enable_#{provider}_logins?").returns(false) - # end - - # it "fails" do - # get :complete, provider: provider - # response.should_not be_success - # end - - # end - - # context "when #{provider} logins are enabled" do - # before do - # SiteSetting.stubs("enable_#{provider}_logins?").returns(true) - # end - - # it "succeeds" do - # get :complete, provider: provider - # response.should be_success - # end - - # context "and 'invite only' site setting is enabled" do - # before do - # SiteSetting.stubs(:invite_only?).returns(true) - # end - - # it "informs the user they are awaiting approval" do - # xhr :get, :complete, provider: provider, format: :json - - # expect( - # JSON.parse(response.body)['awaiting_approval'] - # ).to be_true - # end - # end - - # end - - # end - - # describe 'invalid provider' do - - # it "fails" do - # request.env["omniauth.auth"] = auth - # get :complete, provider: 'hackprovider' - # response.should_not be_success - # end - - # end - - # describe 'twitter' do - - # before do - # request.env["omniauth.auth"] = auth - # end - - # it_behaves_like "an authenticaton provider", 'twitter' - - # end - - # describe 'facebook' do - - # before do - # request.env["omniauth.auth"] = auth - # end - - # it_behaves_like "an authenticaton provider", 'facebook' - - # end - - # describe 'cas' do - - # before do - # request.env["omniauth.auth"] = cas_auth - # end - - # it_behaves_like "an authenticaton provider", 'cas' - - # describe "extracted user data" do - # before do - # SiteSetting.stubs(:enable_cas_logins?).returns(true) - # end - - # subject { - # xhr :get, :complete, provider: 'cas', format: :json - # OpenStruct.new(JSON.parse(response.body)) - # } - - # context "when no user infos are returned by cas" do - # its(:username) { should eq 'casuser' } - # its(:name) { should eq 'casuser' } - # its(:email) { should eq 'casuser' } # No cas_domainname configured! - - # context "when cas_domainname is configured" do - # before do - # SiteSetting.stubs(:cas_domainname).returns("example.com") - # end - - # its(:email) { should eq 'casuser@example.com' } - # end - # end - - # context "when user infos are returned by cas" do - # before do - # request.env["omniauth.auth"] = cas_auth.merge({ - # info: { - # name: 'Proper Name', - # email: 'public@example.com' - # } - # }) - # end - - # its(:username) { should eq 'casuser' } - # its(:name) { should eq 'Proper Name' } - # its(:email) { should eq 'public@example.com' } - # end - - # end - - # end - - - # describe 'open id handler' do - - # before do - # request.env["omniauth.auth"] = { info: {email: 'eviltrout@made.up.email'}, extra: {identity_url: 'http://eviltrout.com'}} - # end - - # describe "google" do - # it_behaves_like "an authenticaton provider", 'google' - # end - - # describe "yahoo" do - # it_behaves_like "an authenticaton provider", 'yahoo' - # end - - # end - - # describe 'github' do - - # before do - # request.env["omniauth.auth"] = auth - # end - - # it_behaves_like "an authenticaton provider", 'github' - - # end - - # describe 'persona' do - - # before do - # request.env["omniauth.auth"] = auth - # end - - # it_behaves_like "an authenticaton provider", 'persona' - - # end - - # describe 'oauth2' do - # before do - # Discourse.stubs(:auth_providers).returns([stub(name: 'my_oauth2_provider', type: :oauth2)]) - # request.env["omniauth.auth"] = { uid: 'my-uid', provider: 'my-oauth-provider-domain.net', info: {email: 'eviltrout@made.up.email', name: 'Chatanooga'}} - # end - - # describe "#create_or_sign_on_user_using_oauth2" do - # context "User already exists" do - # before do - # User.stubs(:find_by_email).returns(Fabricate(:user)) - # end - - # it "should create an OauthUserInfo" do - # expect { - # post :complete, provider: 'my_oauth2_provider' - # }.to change { Oauth2UserInfo.count }.by(1) - # end - # end - # end - # end - end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index b7aa04086ac..acfd9f068a6 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -374,32 +374,16 @@ describe UsersController do SiteSetting.expects(:must_approve_users).returns(true) end - it 'should create twitter user info if none exists' do + it 'should create twitter user info if required' do + SiteSetting.stubs(:enable_twitter_logins?).returns(true) twitter_auth = { twitter_user_id: 42, twitter_screen_name: "bruce" } - session[:authentication] = twitter_auth - TwitterUserInfo.expects(:find_by_twitter_user_id).returns(nil) + auth = session[:authentication] = {} + auth[:authenticator_name] = 'twitter' + auth[:extra_data] = twitter_auth TwitterUserInfo.expects(:create) post_user end - - it 'should create facebook user info if none exists' do - fb_auth = { facebook: { facebook_user_id: 42} } - session[:authentication] = fb_auth - FacebookUserInfo.expects(:find_by_facebook_user_id).returns(nil) - FacebookUserInfo.expects(:create!) - - post_user - end - - it 'should create github user info if none exists' do - gh_auth = { github_user_id: 2, github_screen_name: "bruce" } - session[:authentication] = gh_auth - GithubUserInfo.expects(:find_by_github_user_id).returns(nil) - GithubUserInfo.expects(:create) - - post_user - end end end diff --git a/spec/views/omniauth_callbacks/complete.html.erb_spec.rb b/spec/views/omniauth_callbacks/complete.html.erb_spec.rb index 5db9226d0d9..f5c4da73438 100644 --- a/spec/views/omniauth_callbacks/complete.html.erb_spec.rb +++ b/spec/views/omniauth_callbacks/complete.html.erb_spec.rb @@ -1,4 +1,6 @@ require "spec_helper" + +require "auth/authenticator" require_dependency "auth/result" describe "users/omniauth_callbacks/complete.html.erb" do @@ -24,14 +26,16 @@ describe "users/omniauth_callbacks/complete.html.erb" do result = Auth::Result.new result.email = "xxx@xxx.com" - result.auth_provider = "CAS" + result.authenticator_name = "CAS" assign(:data, result) render - rendered_data["email"].should result.email - rendered_data["auth_provider"].should eq("CAS") + rendered_data["email"].should eq(result.email) + # TODO this is a bit weird, the upcasing is confusing, + # clean it up throughout + rendered_data["auth_provider"].should eq("Cas") end end