Merge branch 'security-327062-jira-ns-perms' into 'master'

Enforce Jira namespace permissions

See merge request gitlab-org/security/gitlab!1532

app/controllers/jira_connect/application_controller.rb

@@ -38,12 +38,30 @@ class JiraConnect::ApplicationController < ApplicationController
   end
 
   def installation_from_jwt
-    return unless auth_token
-
     strong_memoize(:installation_from_jwt) do
+      next unless claims['iss']
+
+      JiraConnectInstallation.find_by_client_key(claims['iss'])
+    end
+  end
+
+  def claims
+    strong_memoize(:claims) do
+      next {} unless auth_token
+
       # Decode without verification to get `client_key` in `iss`
       payload, _ = Atlassian::Jwt.decode(auth_token, nil, false)
-      JiraConnectInstallation.find_by_client_key(payload['iss'])
+      payload
+    end
+  end
+
+  def jira_user
+    strong_memoize(:jira_user) do
+      next unless installation_from_jwt
+      next unless claims['sub']
+
+      # This only works for Jira Cloud installations.
+      installation_from_jwt.client.user_info(claims['sub'])
     end
   end
 

app/controllers/jira_connect/subscriptions_controller.rb

@@ -44,7 +44,9 @@ class JiraConnect::SubscriptionsController < JiraConnect::ApplicationController
   def destroy
     subscription = current_jira_installation.subscriptions.find(params[:id])
 
-    if subscription.destroy
+    if !jira_user&.site_admin?
+      render json: { error: 'forbidden' }, status: :forbidden
+    elsif subscription.destroy
       render json: { success: true }
     else
       render json: { error: subscription.errors.full_messages.join(', ') }, status: :unprocessable_entity
@@ -54,7 +56,7 @@ class JiraConnect::SubscriptionsController < JiraConnect::ApplicationController
   private
 
   def create_service
-    JiraConnectSubscriptions::CreateService.new(current_jira_installation, current_user, namespace_path: params['namespace_path'])
+    JiraConnectSubscriptions::CreateService.new(current_jira_installation, current_user, namespace_path: params['namespace_path'], jira_user: jira_user)
   end
 
   def allow_rendering_in_iframe

app/models/jira_connect_installation.rb

@@ -20,4 +20,8 @@ class JiraConnectInstallation < ApplicationRecord
         id: JiraConnectSubscription.for_project(project)
       })
   }
+
+  def client
+    Atlassian::JiraConnect::Client.new(base_url, shared_secret)
+  end
 end

app/services/jira_connect_subscriptions/create_service.rb

@@ -5,8 +5,11 @@ module JiraConnectSubscriptions
     include Gitlab::Utils::StrongMemoize
     MERGE_REQUEST_SYNC_BATCH_SIZE = 20
     MERGE_REQUEST_SYNC_BATCH_DELAY = 1.minute.freeze
+    NOT_SITE_ADMIN = 'The Jira user is not a site administrator.'
 
     def execute
+      return error(NOT_SITE_ADMIN, 403) unless can_administer_jira?
+
       unless namespace && can?(current_user, :create_jira_connect_subscription, namespace)
         return error('Invalid namespace. Please make sure you have sufficient permissions', 401)
       end
@@ -16,6 +19,10 @@ module JiraConnectSubscriptions
 
     private
 
+    def can_administer_jira?
+      @params[:jira_user]&.site_admin?
+    end
+
     def create_subscription
       subscription = JiraConnectSubscription.new(installation: jira_connect_installation, namespace: namespace)
 

doc/integration/jira/connect-app.md

@@ -6,6 +6,10 @@ info: To determine the technical writer assigned to the Stage/Group associated w
 
 # GitLab.com for Jira Cloud app **(FREE)**
 
+NOTE:
+Only Jira users with administrator level access are able to install or configure
+the GitLab app for Jira Cloud.
+
 ## GitLab.com for Jira Cloud app **(FREE SAAS)**
 
 You can integrate GitLab.com and Jira Cloud using the
@@ -39,7 +43,8 @@ For a walkthrough of the integration with GitLab.com for Jira Cloud app, watch
    ![Sign in to GitLab.com in GitLab.com for Jira Cloud app](img/jira_dev_panel_setup_com_3_v13_9.png)
 1. Select **Add namespace** to open the list of available namespaces.
 
-1. Identify the namespace you want to link, and select **Link**.
+1. Identify the namespace you want to link, and select **Link**. Only Jira site
+   administrators are permitted to add or remove namespaces for an installation.
 
    ![Link namespace in GitLab.com for Jira Cloud app](img/jira_dev_panel_setup_com_4_v13_9.png)
 

lib/atlassian/jira_connect/client.rb

@@ -30,8 +30,21 @@ module Atlassian
         responses.compact
       end
 
+      def user_info(account_id)
+        r = get('/rest/api/3/user', { accountId: account_id, expand: 'groups' })
+
+        JiraUser.new(r.parsed_response) if r.code == 200
+      end
+
       private
 
+      def get(path, query_params)
+        uri = URI.join(@base_uri, path)
+        uri.query = URI.encode_www_form(query_params)
+
+        self.class.get(uri, headers: headers(uri, 'GET'))
+      end
+
       def store_ff_info(project:, feature_flags:, **opts)
         items = feature_flags.map { |flag| ::Atlassian::JiraConnect::Serializers::FeatureFlagEntity.represent(flag, opts) }
         items.reject! { |item| item.issue_keys.empty? }
@@ -99,10 +112,11 @@ module Atlassian
         self.class.post(uri, headers: headers(uri), body: metadata.merge(payload).to_json)
       end
 
-      def headers(uri)
+      def headers(uri, http_method = 'POST')
         {
-          'Authorization' => "JWT #{jwt_token('POST', uri)}",
-          'Content-Type' => 'application/json'
+          'Authorization' => "JWT #{jwt_token(http_method, uri)}",
+          'Content-Type' => 'application/json',
+          'Accept' => 'application/json'
         }
       end
 

lib/atlassian/jira_connect/jira_user.rb

+# frozen_string_literal: true
+
+module Atlassian
+  module JiraConnect
+    class JiraUser
+      def initialize(data)
+        @data = data
+      end
+
+      def site_admin?
+        groups = @data.dig('groups', 'items')
+        return false unless groups
+
+        groups.any? { |g| g['name'] == 'site-admins' }
+      end
+    end
+  end
+end

spec/controllers/jira_connect/subscriptions_controller_spec.rb

@@ -102,11 +102,17 @@ RSpec.describe JiraConnect::SubscriptionsController do
     end
 
     context 'with valid JWT' do
-      let(:jwt) { Atlassian::Jwt.encode({ iss: installation.client_key }, installation.shared_secret) }
+      let(:claims) { { iss: installation.client_key, sub: 1234 } }
+      let(:jwt) { Atlassian::Jwt.encode(claims, installation.shared_secret) }
+      let(:jira_user) { { 'groups' => { 'items' => [{ 'name' => jira_group_name }] } } }
+      let(:jira_group_name) { 'site-admins' }
 
       context 'signed in to GitLab' do
         before do
           sign_in(user)
+          WebMock
+            .stub_request(:get, "#{installation.base_url}/rest/api/3/user?accountId=1234&expand=groups")
+            .to_return(body: jira_user.to_json, status: 200, headers: { 'Content-Type' => 'application/json' })
         end
 
         context 'dev panel integration is available' do
@@ -120,6 +126,16 @@ RSpec.describe JiraConnect::SubscriptionsController do
             expect(response).to have_gitlab_http_status(:ok)
           end
         end
+
+        context 'when the Jira user is not a site-admin' do
+          let(:jira_group_name) { 'some-other-group' }
+
+          it 'returns forbidden' do
+            subject
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
+        end
       end
 
       context 'not signed in to GitLab' do
@@ -134,8 +150,14 @@ RSpec.describe JiraConnect::SubscriptionsController do
 
   describe '#destroy' do
     let(:subscription) { create(:jira_connect_subscription, installation: installation) }
+    let(:jira_user) { { 'groups' => { 'items' => [{ 'name' => jira_group_name }] } } }
+    let(:jira_group_name) { 'site-admins' }
 
     before do
+      WebMock
+        .stub_request(:get, "#{installation.base_url}/rest/api/3/user?accountId=1234&expand=groups")
+        .to_return(body: jira_user.to_json, status: 200, headers: { 'Content-Type' => 'application/json' })
+
       delete :destroy, params: { jwt: jwt, id: subscription.id }
     end
 
@@ -148,12 +170,23 @@ RSpec.describe JiraConnect::SubscriptionsController do
     end
 
     context 'with valid JWT' do
-      let(:jwt) { Atlassian::Jwt.encode({ iss: installation.client_key }, installation.shared_secret) }
+      let(:claims) { { iss: installation.client_key, sub: 1234 } }
+      let(:jwt) { Atlassian::Jwt.encode(claims, installation.shared_secret) }
 
       it 'deletes the subscription' do
         expect { subscription.reload }.to raise_error ActiveRecord::RecordNotFound
         expect(response).to have_gitlab_http_status(:ok)
       end
+
+      context 'when the Jira user is not a site admin' do
+        let(:jira_group_name) { 'some-other-group' }
+
+        it 'does not delete the subscription' do
+          expect(response).to have_gitlab_http_status(:forbidden)
+
+          expect { subscription.reload }.not_to raise_error
+        end
+      end
     end
   end
 end

spec/services/jira_connect_subscriptions/create_service_spec.rb

@@ -7,8 +7,10 @@ RSpec.describe JiraConnectSubscriptions::CreateService do
   let(:current_user) { create(:user) }
   let(:group) { create(:group) }
   let(:path) { group.full_path }
+  let(:params) { { namespace_path: path, jira_user: jira_user } }
+  let(:jira_user) { double(:JiraUser, site_admin?: true) }
 
-  subject { described_class.new(installation, current_user, namespace_path: path).execute }
+  subject { described_class.new(installation, current_user, params).execute }
 
   before do
     group.add_maintainer(current_user)
@@ -24,6 +26,30 @@ RSpec.describe JiraConnectSubscriptions::CreateService do
     end
   end
 
+  context 'remote user does not have access' do
+    let(:jira_user) { double(site_admin?: false) }
+
+    it 'does not create a subscription' do
+      expect { subject }.not_to change { installation.subscriptions.count }
+    end
+
+    it 'returns error' do
+      expect(subject[:status]).to eq(:error)
+    end
+  end
+
+  context 'remote user cannot be retrieved' do
+    let(:jira_user) { nil }
+
+    it 'does not create a subscription' do
+      expect { subject }.not_to change { installation.subscriptions.count }
+    end
+
+    it 'returns error' do
+      expect(subject[:status]).to eq(:error)
+    end
+  end
+
   context 'when user does have access' do
     it 'creates a subscription' do
       expect { subject }.to change { installation.subscriptions.count }.from(0).to(1)