Merge branch '350336-confidential-issue' into 'master'

Add state parameter on bitbucket import project oauth

See merge request gitlab-org/gitlab!83598

app/controllers/import/bitbucket_controller.rb

@@ -12,14 +12,21 @@ class Import::BitbucketController < Import::BaseController
   rescue_from Bitbucket::Error::Unauthorized, with: :bitbucket_unauthorized
 
   def callback
-    response = oauth_client.auth_code.get_token(params[:code], redirect_uri: users_import_bitbucket_callback_url)
+    auth_state = session[:bitbucket_auth_state]
+    session[:bitbucket_auth_state] = nil
 
-    session[:bitbucket_token]         = response.token
-    session[:bitbucket_expires_at]    = response.expires_at
-    session[:bitbucket_expires_in]    = response.expires_in
-    session[:bitbucket_refresh_token] = response.refresh_token
+    if auth_state.blank? || !ActiveSupport::SecurityUtils.secure_compare(auth_state, params[:state])
+      go_to_bitbucket_for_permissions
+    else
+      response = oauth_client.auth_code.get_token(params[:code], redirect_uri: users_import_bitbucket_callback_url)
+
+      session[:bitbucket_token]         = response.token
+      session[:bitbucket_expires_at]    = response.expires_at
+      session[:bitbucket_expires_in]    = response.expires_in
+      session[:bitbucket_refresh_token] = response.refresh_token
 
-    redirect_to status_import_bitbucket_url
+      redirect_to status_import_bitbucket_url
+    end
   end
 
   def status
@@ -113,7 +120,9 @@ class Import::BitbucketController < Import::BaseController
   end
 
   def go_to_bitbucket_for_permissions
-    redirect_to oauth_client.auth_code.authorize_url(redirect_uri: users_import_bitbucket_callback_url)
+    state = SecureRandom.base64(64)
+    session[:bitbucket_auth_state] = state
+    redirect_to oauth_client.auth_code.authorize_url(redirect_uri: users_import_bitbucket_callback_url, state: state)
   end
 
   def bitbucket_unauthorized(exception)

spec/controllers/import/bitbucket_controller_spec.rb

@@ -26,31 +26,55 @@ RSpec.describe Import::BitbucketController do
       session[:oauth_request_token] = {}
     end
 
-    it "updates access token" do
-      expires_at = Time.current + 1.day
-      expires_in = 1.day
-      access_token = double(token: token,
-                            secret: secret,
-                            expires_at: expires_at,
-                            expires_in: expires_in,
-                            refresh_token: refresh_token)
-      allow_any_instance_of(OAuth2::Client)
-        .to receive(:get_token)
-        .with(hash_including(
-                'grant_type' => 'authorization_code',
-                'code' => code,
-                redirect_uri: users_import_bitbucket_callback_url),
-              {})
-        .and_return(access_token)
-      stub_omniauth_provider('bitbucket')
-
-      get :callback, params: { code: code }
-
-      expect(session[:bitbucket_token]).to eq(token)
-      expect(session[:bitbucket_refresh_token]).to eq(refresh_token)
-      expect(session[:bitbucket_expires_at]).to eq(expires_at)
-      expect(session[:bitbucket_expires_in]).to eq(expires_in)
-      expect(controller).to redirect_to(status_import_bitbucket_url)
+    context "when auth state param is invalid" do
+      let(:random_key) { "pure_random"  }
+      let(:external_bitbucket_auth_url) { "http://fake.bitbucket.host/url" }
+
+      it "redirects to external auth url" do
+        allow(SecureRandom).to receive(:base64).and_return(random_key)
+        allow_next_instance_of(OAuth2::Client) do |client|
+          allow(client).to receive_message_chain(:auth_code, :authorize_url)
+            .with(redirect_uri: users_import_bitbucket_callback_url, state: random_key)
+            .and_return(external_bitbucket_auth_url)
+        end
+
+        get :callback, params: { code: code, state: "invalid-token" }
+
+        expect(controller).to redirect_to(external_bitbucket_auth_url)
+      end
+    end
+
+    context "when auth state param is valid" do
+      before do
+        session[:bitbucket_auth_state] = 'state'
+      end
+
+      it "updates access token" do
+        expires_at = Time.current + 1.day
+        expires_in = 1.day
+        access_token = double(token: token,
+                              secret: secret,
+                              expires_at: expires_at,
+                              expires_in: expires_in,
+                              refresh_token: refresh_token)
+        allow_any_instance_of(OAuth2::Client)
+          .to receive(:get_token)
+          .with(hash_including(
+                  'grant_type' => 'authorization_code',
+                  'code' => code,
+                  redirect_uri: users_import_bitbucket_callback_url),
+                {})
+          .and_return(access_token)
+        stub_omniauth_provider('bitbucket')
+
+        get :callback, params: { code: code, state: 'state' }
+
+        expect(session[:bitbucket_token]).to eq(token)
+        expect(session[:bitbucket_refresh_token]).to eq(refresh_token)
+        expect(session[:bitbucket_expires_at]).to eq(expires_at)
+        expect(session[:bitbucket_expires_in]).to eq(expires_in)
+        expect(controller).to redirect_to(status_import_bitbucket_url)
+      end
     end
   end
 
@@ -59,46 +83,68 @@ RSpec.describe Import::BitbucketController do
       @repo = double(name: 'vim', slug: 'vim', owner: 'asd', full_name: 'asd/vim', clone_url: 'http://test.host/demo/url.git', 'valid?' => true)
       @invalid_repo = double(name: 'mercurialrepo', slug: 'mercurialrepo', owner: 'asd', full_name: 'asd/mercurialrepo', clone_url: 'http://test.host/demo/mercurialrepo.git', 'valid?' => false)
       allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org')
+    end
 
-      assign_session_tokens
+    context "when token does not exists" do
+      let(:random_key) { "pure_random"  }
+      let(:external_bitbucket_auth_url) { "http://fake.bitbucket.host/url" }
+
+      it 'redirects to authorize url with state included' do
+        allow(SecureRandom).to receive(:base64).and_return(random_key)
+        allow_next_instance_of(OAuth2::Client) do |client|
+          allow(client).to receive_message_chain(:auth_code, :authorize_url)
+            .with(redirect_uri: users_import_bitbucket_callback_url, state: random_key)
+            .and_return(external_bitbucket_auth_url)
+        end
+
+        get :status, format: :json
+
+        expect(controller).to redirect_to(external_bitbucket_auth_url)
+      end
     end
 
-    it_behaves_like 'import controller status' do
+    context "when token is valid" do
       before do
-        allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org')
+        assign_session_tokens
       end
 
-      let(:repo) { @repo }
-      let(:repo_id) { @repo.full_name }
-      let(:import_source) { @repo.full_name }
-      let(:provider_name) { 'bitbucket' }
-      let(:client_repos_field) { :repos }
-    end
+      it_behaves_like 'import controller status' do
+        before do
+          allow(controller).to receive(:provider_url).and_return('http://demobitbucket.org')
+        end
 
-    it 'returns invalid repos' do
-      allow_any_instance_of(Bitbucket::Client).to receive(:repos).and_return([@repo, @invalid_repo])
+        let(:repo) { @repo }
+        let(:repo_id) { @repo.full_name }
+        let(:import_source) { @repo.full_name }
+        let(:provider_name) { 'bitbucket' }
+        let(:client_repos_field) { :repos }
+      end
 
-      get :status, format: :json
+      it 'returns invalid repos' do
+        allow_any_instance_of(Bitbucket::Client).to receive(:repos).and_return([@repo, @invalid_repo])
 
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response['incompatible_repos'].length).to eq(1)
-      expect(json_response.dig("incompatible_repos", 0, "id")).to eq(@invalid_repo.full_name)
-      expect(json_response['provider_repos'].length).to eq(1)
-      expect(json_response.dig("provider_repos", 0, "id")).to eq(@repo.full_name)
-    end
+        get :status, format: :json
 
-    context 'when filtering' do
-      let(:filter) { '<html>test</html>' }
-      let(:expected_filter) { 'test' }
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['incompatible_repos'].length).to eq(1)
+        expect(json_response.dig("incompatible_repos", 0, "id")).to eq(@invalid_repo.full_name)
+        expect(json_response['provider_repos'].length).to eq(1)
+        expect(json_response.dig("provider_repos", 0, "id")).to eq(@repo.full_name)
+      end
 
-      subject { get :status, params: { filter: filter }, as: :json }
+      context 'when filtering' do
+        let(:filter) { '<html>test</html>' }
+        let(:expected_filter) { 'test' }
 
-      it 'passes sanitized filter param to bitbucket client' do
-        expect_next_instance_of(Bitbucket::Client) do |client|
-          expect(client).to receive(:repos).with(filter: expected_filter).and_return([@repo])
-        end
+        subject { get :status, params: { filter: filter }, as: :json }
 
-        subject
+        it 'passes sanitized filter param to bitbucket client' do
+          expect_next_instance_of(Bitbucket::Client) do |client|
+            expect(client).to receive(:repos).with(filter: expected_filter).and_return([@repo])
+          end
+
+          subject
+        end
       end
     end
   end