diff --git a/app/controllers/github/callbacks_controller.rb b/app/controllers/github/callbacks_controller.rb index 2bb2017a2..7f6cf9fb9 100644 --- a/app/controllers/github/callbacks_controller.rb +++ b/app/controllers/github/callbacks_controller.rb @@ -2,6 +2,9 @@ class Github::CallbacksController < ApplicationController include Github::IntegrationHelper def show + # Validate account context early for all flows that require it + account if params[:code].present? + if params[:installation_id].present? && params[:code].present? # Both installation and OAuth code present - handle both handle_installation_with_oauth @@ -78,36 +81,38 @@ class Github::CallbacksController < ApplicationController end def handle_response(installation_id = nil) + settings = build_hook_settings(installation_id) + hook = create_integration_hook(settings) + hook.save! + + cleanup_session_data + redirect_to github_redirect_uri + rescue StandardError => e + Rails.logger.error("Github callback error: #{e.message}") + redirect_to fallback_redirect_uri + end + + def build_hook_settings(installation_id) settings = { token_type: parsed_body['token_type'], scope: parsed_body['scope'] } - # Add installation_id from parameter or session - if installation_id - settings[:installation_id] = installation_id - elsif session[:github_installation_id] - settings[:installation_id] = session[:github_installation_id] - end + settings[:installation_id] = installation_id || session[:github_installation_id] + settings.compact + end - # Use account from state parameter - this should work for both flows now - target_account = account - - hook = target_account.hooks.new( + def create_integration_hook(settings) + account.hooks.new( access_token: parsed_body['access_token'], status: 'enabled', app_id: 'github', settings: settings ) - hook.save! + end - # Clear session data + def cleanup_session_data session.delete(:github_installation_id) - - redirect_to github_redirect_uri - rescue StandardError => e - Rails.logger.error("Github callback error: #{e.message}") - redirect_to fallback_redirect_uri end def account diff --git a/app/models/integrations/app.rb b/app/models/integrations/app.rb index f5d870836..ead75ce80 100644 --- a/app/models/integrations/app.rb +++ b/app/models/integrations/app.rb @@ -63,7 +63,7 @@ class Integrations::App when 'linear' GlobalConfigService.load('LINEAR_CLIENT_ID', nil).present? when 'github' - GlobalConfigService.load('GITHUB_CLIENT_ID', nil).present? && GlobalConfigService.load('GITHUB_CLIENT_SECRET', nil).present? + github_enabled? when 'shopify' shopify_enabled?(account) when 'leadsquared' @@ -143,6 +143,10 @@ class Integrations::App private + def github_enabled? + GlobalConfigService.load('GITHUB_CLIENT_ID', nil).present? && GlobalConfigService.load('GITHUB_CLIENT_SECRET', nil).present? + end + def shopify_enabled?(account) account.feature_enabled?('shopify_integration') && GlobalConfigService.load('SHOPIFY_CLIENT_ID', nil).present? end diff --git a/spec/controllers/github/callbacks_controller_spec.rb b/spec/controllers/github/callbacks_controller_spec.rb new file mode 100644 index 000000000..2c1f52f2d --- /dev/null +++ b/spec/controllers/github/callbacks_controller_spec.rb @@ -0,0 +1,204 @@ +require 'rails_helper' + +RSpec.describe Github::CallbacksController do + let(:account) { create(:account) } + let(:signed_account_id) { account.to_signed_global_id(expires_in: 1.hour) } + + before do + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return('test_client_id') + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_SECRET', nil).and_return('test_client_secret') + + # Configure test environment + @original_frontend_url = ENV.fetch('FRONTEND_URL', nil) + ENV['FRONTEND_URL'] = 'http://localhost:3000' + + # Configure request.host to match our test URL + allow_any_instance_of(ActionDispatch::Request).to receive(:host).and_return('localhost') + allow_any_instance_of(ActionDispatch::Request).to receive(:port).and_return(3000) + allow_any_instance_of(ActionDispatch::Request).to receive(:protocol).and_return('http://') + + # Mock all GitHub OAuth API calls to prevent WebMock errors + stub_request(:post, 'https://github.com/login/oauth/access_token') + .to_return(status: 200, body: '', headers: {}) + end + + after do + ENV['FRONTEND_URL'] = @original_frontend_url + end + + describe 'GET #show' do + context 'when handling installation with OAuth (both installation_id and code present)' do + let(:oauth_response) do + double('OAuth2::Response', response: double(parsed: { + 'access_token' => 'test_token', + 'token_type' => 'bearer', + 'scope' => 'repo,read:org' + })) + end + + before do + oauth_client = double('OAuth2::Client') + auth_code = double('OAuth2::Strategy::AuthCode') + allow(controller).to receive(:oauth_client).and_return(oauth_client) + allow(oauth_client).to receive(:auth_code).and_return(auth_code) + allow(auth_code).to receive(:get_token).and_return(oauth_response) + end + + it 'creates integration hook with installation_id and redirects' do + get :show, params: { + code: 'test_code', + installation_id: '12345', + setup_action: 'install', + state: signed_account_id + } + + hook = account.hooks.find_by(app_id: 'github') + expect(hook).to be_present + expect(hook.access_token).to eq('test_token') + expect(hook.settings['installation_id']).to eq('12345') + expect(hook.settings['token_type']).to eq('bearer') + expect(hook.settings['scope']).to eq('repo,read:org') + expect(response).to redirect_to(a_string_including('/settings/integrations/github')) + end + end + + context 'when handling installation only (only installation_id present)' do + it 'redirects to OAuth URL' do + get :show, params: { + installation_id: '12345', + setup_action: 'install', + state: signed_account_id + } + + expect(response).to be_redirect + expect(response.location).to include('github?setup_action=install&installation_id=12345') + end + end + + context 'when handling authorization only (only code present)' do + let(:oauth_response) do + double('OAuth2::Response', response: double(parsed: { + 'access_token' => 'test_token', + 'token_type' => 'bearer', + 'scope' => 'repo,read:org' + })) + end + + before do + oauth_client = double('OAuth2::Client') + auth_code = double('OAuth2::Strategy::AuthCode') + allow(controller).to receive(:oauth_client).and_return(oauth_client) + allow(oauth_client).to receive(:auth_code).and_return(auth_code) + allow(auth_code).to receive(:get_token).and_return(oauth_response) + end + + it 'creates integration hook without installation_id and redirects' do + get :show, params: { + code: 'test_code', + state: signed_account_id + } + + hook = account.hooks.find_by(app_id: 'github') + expect(hook).to be_present + expect(hook.access_token).to eq('test_token') + expect(hook.settings['installation_id']).to be_nil + expect(response).to redirect_to(a_string_including('/settings/integrations/github')) + end + end + + context 'when state parameter is missing' do + it 'handles error gracefully and redirects to fallback URI' do + get :show, params: { + code: 'test_code', + installation_id: '12345', + setup_action: 'install' + } + + expect(response).to be_redirect + end + end + + context 'when state parameter is invalid' do + it 'handles error gracefully and redirects to fallback URI' do + get :show, params: { + code: 'test_code', + installation_id: '12345', + setup_action: 'install', + state: 'invalid_state' + } + + expect(response).to be_redirect + end + end + + context 'when OAuth fails' do + before do + oauth_client = double('OAuth2::Client') + auth_code = double('OAuth2::Strategy::AuthCode') + allow(controller).to receive(:oauth_client).and_return(oauth_client) + allow(oauth_client).to receive(:auth_code).and_return(auth_code) + allow(auth_code).to receive(:get_token).and_raise(StandardError, 'OAuth failed') + end + + it 'logs error and redirects to fallback URI' do + expect(Rails.logger).to receive(:error).with('Github callback error: OAuth failed') + + get :show, params: { + code: 'test_code', + installation_id: '12345', + setup_action: 'install', + state: signed_account_id + } + + expect(response).to be_redirect + end + end + end + + describe 'private methods' do + let(:controller_instance) { described_class.new } + + before do + allow(controller_instance).to receive(:account).and_return(account) + allow(controller_instance).to receive(:parsed_body).and_return({ + 'access_token' => 'test_token', + 'token_type' => 'bearer', + 'scope' => 'repo,read:org' + }) + # Mock session for private method tests + allow(controller_instance).to receive(:session).and_return({}) + end + + describe '#build_hook_settings' do + it 'builds settings with installation_id parameter' do + settings = controller_instance.send(:build_hook_settings, '12345') + + expect(settings[:token_type]).to eq('bearer') + expect(settings[:scope]).to eq('repo,read:org') + expect(settings[:installation_id]).to eq('12345') + end + + it 'builds settings without installation_id when nil' do + settings = controller_instance.send(:build_hook_settings, nil) + + expect(settings[:token_type]).to eq('bearer') + expect(settings[:scope]).to eq('repo,read:org') + expect(settings.key?(:installation_id)).to be false + end + end + + describe '#create_integration_hook' do + it 'creates a new hook with correct attributes' do + settings = { token_type: 'bearer', scope: 'repo,read:org', installation_id: '12345' } + hook = controller_instance.send(:create_integration_hook, settings) + + expect(hook.access_token).to eq('test_token') + expect(hook.status).to eq('enabled') + expect(hook.app_id).to eq('github') + # Convert expected settings to string keys to match actual storage format + expect(hook.settings).to eq(settings.stringify_keys) + expect(hook.account).to eq(account) + end + end + end +end \ No newline at end of file diff --git a/spec/models/integrations/app_spec.rb b/spec/models/integrations/app_spec.rb index fc64e1cc6..3ebf9dfb1 100644 --- a/spec/models/integrations/app_spec.rb +++ b/spec/models/integrations/app_spec.rb @@ -38,6 +38,24 @@ RSpec.describe Integrations::App do end end end + + context 'when the app is github' do + let(:app_name) { 'github' } + + it 'returns the GitHub App installation URL with state parameter' do + with_modified_env GITHUB_CLIENT_ID: 'dummy_client_id', GITHUB_APP_NAME: 'test-app' do + expect(app.action).to include('https://github.com/apps/test-app/installations/new') + expect(app.action).to include('state=') + end + end + + it 'uses default app name when GITHUB_APP_NAME is not set' do + # Mock GlobalConfigService to ensure consistent behavior + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return('dummy_client_id') + allow(GlobalConfigService).to receive(:load).with('GITHUB_APP_NAME', 'chatwoot-qa').and_return('chatwoot-qa') + expect(app.action).to include('https://github.com/apps/chatwoot-qa/installations/new') + end + end end describe '#active?' do @@ -87,6 +105,34 @@ RSpec.describe Integrations::App do end end + context 'when the app is github' do + let(:app_name) { 'github' } + + it 'returns false if github credentials are not present' do + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return(nil) + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_SECRET', nil).and_return(nil) + expect(app.active?(account)).to be false + end + + it 'returns false if only client_id is present' do + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return('client_id') + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_SECRET', nil).and_return(nil) + expect(app.active?(account)).to be false + end + + it 'returns false if only client_secret is present' do + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return(nil) + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_SECRET', nil).and_return('client_secret') + expect(app.active?(account)).to be false + end + + it 'returns true if both client_id and client_secret are present' do + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_ID', nil).and_return('client_id') + allow(GlobalConfigService).to receive(:load).with('GITHUB_CLIENT_SECRET', nil).and_return('client_secret') + expect(app.active?(account)).to be true + end + end + context 'when other apps are queried' do let(:app_name) { 'webhook' }