diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 960e005ee..11b2c8f27 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -26,7 +26,7 @@ def after_sign_in_path_for(user) session[:pending_invite_accept_url] elsif !user.complete? edit_profile_path - elsif (referer = request.referer).present? && (URI.parse(referer).host == request.host) && (referer != new_user_session_url) && !referer.start_with?(edit_password_url(current_user)) && (referer != user_developer_omniauth_authorize_url) + elsif (referer = request.referer).present? && (URI.parse(referer).host == request.host) && (referer != new_user_session_url) && !referer.start_with?(edit_password_url(current_user)) && (referer != omniauth_authorize_url(provider: :developer)) referer elsif session[:target] session.delete(:target) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index f61ec8d4d..6cc2b48fb 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,4 +1,4 @@ -class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController +class Users::OmniauthCallbacksController < ApplicationController class AuthHash < SimpleDelegator def account_name dig('info', 'nickname') || dig('extra', 'raw_info', 'screen_name') @@ -9,24 +9,30 @@ def provider_name end end - before_action :link_identity_to_current_user, only: [:twitter, :github, :developer], if: -> { current_user } - skip_forgery_protection only: :developer + before_action :link_identity_to_current_user, only: :callback, if: -> { current_user } + skip_forgery_protection only: :callback + skip_before_action :current_event - def twitter - authenticate_with_hash - end - - def github - authenticate_with_hash + def callback + case params[:provider] + when 'twitter' + authenticate_with_hash + when 'github' + authenticate_with_hash + when 'developer' + user = User.find_by(email: auth_hash.uid) + authenticate_with_hash(user) + else + failure + end end - def developer - user = User.find_by email: auth_hash.uid - authenticate_with_hash user + def passthru + render plain: 'Not Found', status: :not_found end def failure - redirect_to new_user_session_url, danger: "There was an error authenticating you. Please try again." + redirect_to new_user_session_path, flash: {danger: 'There was an error authenticating you. Please try again.'} end private @@ -59,7 +65,7 @@ def authenticate_with_hash(user = nil) @user = user || User.from_omniauth(auth_hash, session[:pending_invite_email]) if @user.persisted? - flash.now[:info] = "You have signed in with #{auth_hash.provider_name}." + flash[:info] = "You have signed in with #{auth_hash.provider_name}." logger.info "Signing in user #{@user.inspect}" @user.confirmed_at = Time.current @@ -67,7 +73,7 @@ def authenticate_with_hash(user = nil) redirect_to after_sign_in_path_for(@user) else - redirect_to new_user_session_url, danger: "There was an error authenticating via Auth provider: #{params[:provider]}." + redirect_to new_user_session_path, flash: {danger: "There was an error authenticating via Auth provider: #{params[:provider]}."} end end diff --git a/app/models/user.rb b/app/models/user.rb index 0dcf11d39..c0a3f59c3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2,8 +2,7 @@ class User < ApplicationRecord # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :trackable, :confirmable, #:validatable, - :omniauthable, omniauth_providers: [:twitter, :github, :developer] + :recoverable, :rememberable, :trackable, :confirmable #:validatable, has_many :identities, dependent: :destroy has_many :invitations, dependent: :destroy diff --git a/app/views/devise/shared/_links.html.erb b/app/views/devise/shared/_links.html.erb index 3fd574c67..4f79d4d80 100644 --- a/app/views/devise/shared/_links.html.erb +++ b/app/views/devise/shared/_links.html.erb @@ -21,16 +21,12 @@
- <%- if devise_mapping.omniauthable? %> - - <%= button_to user_twitter_omniauth_authorize_path, data: {turbo: false}, class: "btn btn-twitter", form: {style: "display: inline-block"} do %> - - Sign in with Twitter - <% end %> - <%= button_to user_github_omniauth_authorize_path, data: {turbo: false}, class: "btn btn-github", form: {style: "display: inline-block"} do %> - - Sign in with GitHub - <% end %> - + <%= button_to omniauth_authorize_path(provider: :twitter), data: {turbo: false}, class: "btn btn-twitter", form: {style: "display: inline-block"} do %> + + Sign in with Twitter + <% end %> + <%= button_to omniauth_authorize_path(provider: :github), data: {turbo: false}, class: "btn btn-github", form: {style: "display: inline-block"} do %> + + Sign in with GitHub <% end %> diff --git a/app/views/profiles/edit.html.haml b/app/views/profiles/edit.html.haml index cc5741975..b5fceb788 100644 --- a/app/views/profiles/edit.html.haml +++ b/app/views/profiles/edit.html.haml @@ -72,7 +72,7 @@ = "Connected via #{name}" - else .service.mb-2 - = button_to send("user_#{provider}_omniauth_authorize_path"), data: {turbo: false}, class: "btn btn-#{provider}", form: {style: 'display: inline-block'} do + = button_to omniauth_authorize_path(provider: provider), data: {turbo: false}, class: "btn btn-#{provider}", form: {style: 'display: inline-block'} do %i{class: "bi bi-#{provider}"} = "Connect #{name}" diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 6215eb0f9..1d902f443 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -241,9 +241,6 @@ # ==> OmniAuth # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. - config.omniauth :github, ENV['GITHUB_KEY'], ENV['GITHUB_SECRET'], scope: 'user:email' - config.omniauth :twitter, ENV['TWITTER_KEY'], ENV['TWITTER_SECRET'] - config.omniauth :developer unless Rails.env.production? # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb new file mode 100644 index 000000000..468a3239c --- /dev/null +++ b/config/initializers/omniauth.rb @@ -0,0 +1,8 @@ +Rails.application.config.middleware.use OmniAuth::Builder do + provider :github, ENV['GITHUB_KEY'], ENV['GITHUB_SECRET'], scope: 'user:email' + provider :twitter, ENV['TWITTER_KEY'], ENV['TWITTER_SECRET'] + provider :developer unless Rails.env.production? +end + +OmniAuth.config.path_prefix = '/users/auth' +OmniAuth.config.allowed_request_methods = [:post] diff --git a/config/routes.rb b/config/routes.rb index a08b35378..6a983e155 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,11 @@ Rails.application.routes.draw do + # OmniAuth + get '/users/auth/:provider/callback', to: 'users/omniauth_callbacks#callback', as: :omniauth_callback + post '/users/auth/:provider', to: 'users/omniauth_callbacks#passthru', as: :omniauth_authorize + get '/users/auth/failure', to: 'users/omniauth_callbacks#failure', as: :omniauth_failure + root 'home#show' - devise_for :users, controllers: { omniauth_callbacks: 'users/omniauth_callbacks' } + devise_for :users mount ActionCable.server => '/cable' resource :profile, only: [:show, :edit, :update] do diff --git a/spec/controllers/users/omniauth_callbacks_controller_spec.rb b/spec/controllers/users/omniauth_callbacks_controller_spec.rb index 7470dbd7b..024e9a63f 100644 --- a/spec/controllers/users/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/users/omniauth_callbacks_controller_spec.rb @@ -20,7 +20,7 @@ end it 'allows twitter login for new user' do - get :twitter + post :callback, params: {provider: 'twitter'} expect(response).to redirect_to(edit_profile_url) expect(User.last.identities.find_by(provider: 'twitter')).to be_present end @@ -37,7 +37,7 @@ let!(:identity) { user.identities.create!(provider: 'github', uid: github_auth_hash.uid) } it 'finds user via identity and signs in' do - expect { get :github }.not_to change { User.count } + expect { post :callback, params: {provider: 'github'} }.not_to change { User.count } expect(controller.current_user).to eq(user) end end @@ -46,7 +46,7 @@ let!(:user) { create(:user, provider: 'github', uid: github_auth_hash.uid) } it 'finds user via legacy lookup and signs in' do - expect { get :github }.not_to change { User.count } + expect { post :callback, params: {provider: 'github'} }.not_to change { User.count } expect(controller.current_user).to eq(user) end end @@ -63,7 +63,7 @@ user = create(:user) sign_in(user) - expect { get :github }.to change { user.identities.count }.by(1) + expect { post :callback, params: {provider: 'github'} }.to change { user.identities.count }.by(1) expect(response).to redirect_to(edit_profile_path) expect(flash[:info]).to eq('Successfully connected GitHub to your account.') end @@ -75,7 +75,7 @@ user.identities.create!(provider: 'github', uid: github_auth_hash.uid) sign_in(user) - expect { get :github }.not_to change { Identity.count } + expect { post :callback, params: {provider: 'github'} }.not_to change { Identity.count } expect(response).to redirect_to(edit_profile_path) expect(flash[:info]).to eq('GitHub is already connected to your account.') end @@ -89,7 +89,7 @@ user = create(:user) sign_in(user) - expect { get :github }.not_to change { Identity.count } + expect { post :callback, params: {provider: 'github'} }.not_to change { Identity.count } expect(response).to redirect_to(edit_profile_path) expect(flash[:danger]).to eq('This GitHub account is already connected to another user.') end @@ -101,7 +101,7 @@ user = create(:user) sign_in(user) - get :github + post :callback, params: {provider: 'github'} expect(response).to redirect_to(merge_profile_path) expect(session[:pending_merge_user_id]).to eq(legacy_user.id)