diff options
-rw-r--r-- | app/controllers/jira_connect/app_descriptor_controller.rb | 12 | ||||
-rw-r--r-- | app/controllers/jira_connect/application_controller.rb | 24 | ||||
-rw-r--r-- | app/controllers/jira_connect/subscriptions_controller.rb | 6 | ||||
-rw-r--r-- | app/models/jira_connect_installation.rb | 4 | ||||
-rw-r--r-- | app/services/jira_connect_subscriptions/create_service.rb | 7 | ||||
-rw-r--r-- | doc/integration/jira/connect-app.md | 7 | ||||
-rw-r--r-- | lib/atlassian/jira_connect/client.rb | 20 | ||||
-rw-r--r-- | lib/atlassian/jira_connect/jira_user.rb | 18 | ||||
-rw-r--r-- | spec/controllers/jira_connect/subscriptions_controller_spec.rb | 37 | ||||
-rw-r--r-- | spec/services/jira_connect_subscriptions/create_service_spec.rb | 28 |
10 files changed, 147 insertions, 16 deletions
diff --git a/app/controllers/jira_connect/app_descriptor_controller.rb b/app/controllers/jira_connect/app_descriptor_controller.rb index 0de42ad2452..ca2d502d2fe 100644 --- a/app/controllers/jira_connect/app_descriptor_controller.rb +++ b/app/controllers/jira_connect/app_descriptor_controller.rb @@ -58,10 +58,14 @@ class JiraConnect::AppDescriptorController < JiraConnect::ApplicationController }, postInstallPage: { key: 'gitlab-configuration', - name: { - value: 'GitLab Configuration' - }, - url: relative_to_base_path(jira_connect_subscriptions_path) + name: { value: 'GitLab Configuration' }, + url: relative_to_base_path(jira_connect_subscriptions_path), + conditions: [ + { + condition: 'user_is_admin', + invert: false + } + ] } } diff --git a/app/controllers/jira_connect/application_controller.rb b/app/controllers/jira_connect/application_controller.rb index a6529ecb4ce..352e78d6255 100644 --- a/app/controllers/jira_connect/application_controller.rb +++ b/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 diff --git a/app/controllers/jira_connect/subscriptions_controller.rb b/app/controllers/jira_connect/subscriptions_controller.rb index 3ff12f29f10..ef60d387988 100644 --- a/app/controllers/jira_connect/subscriptions_controller.rb +++ b/app/controllers/jira_connect/subscriptions_controller.rb @@ -37,7 +37,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 @@ -47,7 +49,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 diff --git a/app/models/jira_connect_installation.rb b/app/models/jira_connect_installation.rb index 7480800abc3..a7e10a5c050 100644 --- a/app/models/jira_connect_installation.rb +++ b/app/models/jira_connect_installation.rb @@ -19,4 +19,8 @@ class JiraConnectInstallation < ApplicationRecord id: JiraConnectSubscription.for_project(project) }) } + + def client + Atlassian::JiraConnect::Client.new(base_url, shared_secret) + end end diff --git a/app/services/jira_connect_subscriptions/create_service.rb b/app/services/jira_connect_subscriptions/create_service.rb index 38e5fe7e690..2f31a3c8d4e 100644 --- a/app/services/jira_connect_subscriptions/create_service.rb +++ b/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) diff --git a/doc/integration/jira/connect-app.md b/doc/integration/jira/connect-app.md index fcee3f7a637..651f8e7d657 100644 --- a/doc/integration/jira/connect-app.md +++ b/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) diff --git a/lib/atlassian/jira_connect/client.rb b/lib/atlassian/jira_connect/client.rb index ea83076c49b..f1ba6437b19 100644 --- a/lib/atlassian/jira_connect/client.rb +++ b/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 diff --git a/lib/atlassian/jira_connect/jira_user.rb b/lib/atlassian/jira_connect/jira_user.rb new file mode 100644 index 00000000000..57ceb8fdf13 --- /dev/null +++ b/lib/atlassian/jira_connect/jira_user.rb @@ -0,0 +1,18 @@ +# 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 diff --git a/spec/controllers/jira_connect/subscriptions_controller_spec.rb b/spec/controllers/jira_connect/subscriptions_controller_spec.rb index 95b359a989a..60729f94288 100644 --- a/spec/controllers/jira_connect/subscriptions_controller_spec.rb +++ b/spec/controllers/jira_connect/subscriptions_controller_spec.rb @@ -56,11 +56,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 @@ -74,6 +80,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 @@ -88,8 +104,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 @@ -102,12 +124,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 diff --git a/spec/services/jira_connect_subscriptions/create_service_spec.rb b/spec/services/jira_connect_subscriptions/create_service_spec.rb index 5f467a07a78..cde4753cde7 100644 --- a/spec/services/jira_connect_subscriptions/create_service_spec.rb +++ b/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) |