Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/error_tracking/components/error_tracking_list.vue

@@ -38,6 +38,10 @@ export default {
       type: String,
       required: true,
     },
+    userCanEnableErrorTracking: {
+      type: Boolean,
+      required: true,
+    },
   },
   computed: {
     ...mapState(['errors', 'externalUrl', 'loading']),
@@ -111,7 +115,7 @@ export default {
         </gl-table>
       </div>
     </div>
-    <div v-else>
+    <div v-else-if="userCanEnableErrorTracking">
       <gl-empty-state
         :title="__('Get started with error tracking')"
         :description="__('Monitor your errors by integrating with Sentry')"
@@ -120,5 +124,17 @@ export default {
         :svg-path="illustrationPath"
       />
     </div>
+    <div v-else>
+      <gl-empty-state :title="__('Get started with error tracking')" :svg-path="illustrationPath">
+        <template #description>
+          <div>
+            <span>{{ __('Monitor your errors by integrating with Sentry.') }}</span>
+            <a href="/help/user/project/operations/error_tracking.html">
+              {{ __('More information') }}
+            </a>
+          </div>
+        </template>
+      </gl-empty-state>
+    </div>
   </div>
 </template>

app/assets/javascripts/error_tracking/index.js

@@ -14,9 +14,10 @@ export default () => {
     render(createElement) {
       const domEl = document.querySelector(this.$options.el);
       const { indexPath, enableErrorTrackingLink, illustrationPath } = domEl.dataset;
-      let { errorTrackingEnabled } = domEl.dataset;
+      let { errorTrackingEnabled, userCanEnableErrorTracking } = domEl.dataset;
 
       errorTrackingEnabled = parseBoolean(errorTrackingEnabled);
+      userCanEnableErrorTracking = parseBoolean(userCanEnableErrorTracking);
 
       return createElement('error-tracking-list', {
         props: {
@@ -24,6 +25,7 @@ export default () => {
           enableErrorTrackingLink,
           errorTrackingEnabled,
           illustrationPath,
+          userCanEnableErrorTracking,
         },
       });
     },

app/controllers/projects/settings/operations_controller.rb

@@ -3,7 +3,7 @@
 module Projects
   module Settings
     class OperationsController < Projects::ApplicationController
-      before_action :authorize_update_environment!
+      before_action :authorize_admin_operations!
 
       helper_method :error_tracking_setting
 

app/graphql/types/permission_types/project.rb

@@ -16,7 +16,7 @@ module Types
                 :create_deployment, :push_to_delete_protected_branch,
                 :admin_wiki, :admin_project, :update_pages,
                 :admin_remote_mirror, :create_label, :update_wiki, :destroy_wiki,
-                :create_pages, :destroy_pages, :read_pages_content
+                :create_pages, :destroy_pages, :read_pages_content, :admin_operations
     end
   end
 end

app/helpers/projects/error_tracking_helper.rb

 # frozen_string_literal: true
 
 module Projects::ErrorTrackingHelper
-  def error_tracking_data(project)
+  def error_tracking_data(current_user, project)
     error_tracking_enabled = !!project.error_tracking_setting&.enabled?
 
     {
       'index-path' => project_error_tracking_index_path(project,
                                                         format: :json),
+      'user-can-enable-error-tracking' => can?(current_user, :admin_operations, project).to_s,
       'enable-error-tracking-link' => project_settings_operations_path(project),
       'error-tracking-enabled' => error_tracking_enabled.to_s,
       'illustration-path' => image_path('illustrations/cluster_popover.svg')

app/policies/project_policy.rb

@@ -294,6 +294,7 @@ class ProjectPolicy < BasePolicy
     enable :destroy_release
     enable :destroy_artifacts
     enable :daily_statistics
+    enable :admin_operations
   end
 
   rule { (mirror_available & can?(:admin_project)) | admin }.enable :admin_remote_mirror

app/views/projects/error_tracking/index.html.haml

 - page_title _('Errors')
 
-#js-error_tracking{ data: error_tracking_data(@project) }
+#js-error_tracking{ data: error_tracking_data(@current_user, @project) }

app/views/projects/mirrors/_instructions.html.haml

@@ -3,6 +3,7 @@
     %li
       = _('The repository must be accessible over <code>http://</code>,
       <code>https://</code>, <code>ssh://</code> or <code>git://</code>.').html_safe
+    %li= _('When using the <code>http://</code> or <code>https://</code> protocols, please provide the exact URL to the repository. HTTP redirects will not be followed.').html_safe
     %li= _('Include the username in the URL if required: <code>https://username@gitlab.company.com/group/project.git</code>.').html_safe
     %li
       - minutes = Gitlab.config.gitlab_shell.git_timeout / 60

app/views/shared/_import_form.html.haml

@@ -27,6 +27,7 @@
       %ul
         %li
           = _('The repository must be accessible over <code>http://</code>, <code>https://</code> or <code>git://</code>.').html_safe
+        %li= _('When using the <code>http://</code> or <code>https://</code> protocols, please provide the exact URL to the repository. HTTP redirects will not be followed.').html_safe
         %li
           = _('If your HTTP repository is not publicly accessible, add your credentials.')
         %li

changelogs/unreleased/66467-enable-error-tracking-only-user-can-read-sentry-logs.yml

+---
+title: Display `more information` docs link on error tracking page when users do not have permissions to enable that feature
+merge_request: 32365
+author: Romain Maneschi
+type: fixed

spec/frontend/error_tracking/components/error_tracking_list_spec.js

@@ -11,19 +11,24 @@ describe('ErrorTrackingList', () => {
   let wrapper;
   let actions;
 
-  function mountComponent({ errorTrackingEnabled = true } = {}) {
+  function mountComponent({
+    errorTrackingEnabled = true,
+    userCanEnableErrorTracking = true,
+    stubs = {
+      'gl-link': GlLink,
+    },
+  } = {}) {
     wrapper = shallowMount(ErrorTrackingList, {
       localVue,
       store,
       propsData: {
         indexPath: '/path',
         enableErrorTrackingLink: '/link',
+        userCanEnableErrorTracking,
         errorTrackingEnabled,
         illustrationPath: 'illustration/path',
       },
-      stubs: {
-        'gl-link': GlLink,
-      },
+      stubs,
     });
   }
 
@@ -115,4 +120,23 @@ describe('ErrorTrackingList', () => {
       expect(wrapper.find(GlButton).exists()).toBeFalsy();
     });
   });
+
+  describe('When error tracking is disabled and user is not allowed to enable it', () => {
+    beforeEach(() => {
+      mountComponent({
+        errorTrackingEnabled: false,
+        userCanEnableErrorTracking: false,
+        stubs: {
+          'gl-link': GlLink,
+          'gl-empty-state': GlEmptyState,
+        },
+      });
+    });
+
+    it('shows empty state', () => {
+      expect(wrapper.find('a').attributes('href')).toBe(
+        '/help/user/project/operations/error_tracking.html',
+      );
+    });
+  });
 });

spec/helpers/projects/error_tracking_helper_spec.rb

@@ -6,21 +6,31 @@ describe Projects::ErrorTrackingHelper do
   include Gitlab::Routing.url_helpers
 
   set(:project) { create(:project) }
+  set(:current_user) { create(:user) }
 
   describe '#error_tracking_data' do
+    let(:can_enable_error_tracking) { true }
     let(:setting_path) { project_settings_operations_path(project) }
 
     let(:index_path) do
       project_error_tracking_index_path(project, format: :json)
     end
 
+    before do
+      allow(helper)
+        .to receive(:can?)
+        .with(current_user, :admin_operations, project)
+        .and_return(can_enable_error_tracking)
+    end
+
     context 'without error_tracking_setting' do
       it 'returns frontend configuration' do
-        expect(error_tracking_data(project)).to eq(
+        expect(helper.error_tracking_data(current_user, project)).to match(
           'index-path' => index_path,
+          'user-can-enable-error-tracking' => 'true',
           'enable-error-tracking-link' => setting_path,
           'error-tracking-enabled' => 'false',
-          "illustration-path" => "/images/illustrations/cluster_popover.svg"
+          'illustration-path' => match_asset_path('/assets/illustrations/cluster_popover.svg')
         )
       end
     end
@@ -36,7 +46,7 @@ describe Projects::ErrorTrackingHelper do
         end
 
         it 'show error tracking enabled' do
-          expect(error_tracking_data(project)).to include(
+          expect(helper.error_tracking_data(current_user, project)).to include(
             'error-tracking-enabled' => 'true'
           )
         end
@@ -48,11 +58,21 @@ describe Projects::ErrorTrackingHelper do
         end
 
         it 'show error tracking not enabled' do
-          expect(error_tracking_data(project)).to include(
+          expect(helper.error_tracking_data(current_user, project)).to include(
             'error-tracking-enabled' => 'false'
           )
         end
       end
     end
+
+    context 'when user is not maintainer' do
+      let(:can_enable_error_tracking) { false }
+
+      it 'shows error tracking enablement as disabled' do
+        expect(helper.error_tracking_data(current_user, project)).to include(
+          'user-can-enable-error-tracking' => 'false'
+        )
+      end
+    end
   end
 end

spec/requests/rack_attack_global_spec.rb

@@ -20,11 +20,6 @@ describe 'Rack Attack global throttles' do
   let(:period_in_seconds) { 10000 }
   let(:period) { period_in_seconds.seconds }
 
-  let(:url_that_does_not_require_authentication) { '/users/sign_in' }
-  let(:url_that_requires_authentication) { '/dashboard/snippets' }
-  let(:url_api_internal) { '/api/v4/internal/check' }
-  let(:api_partial_url) { '/todos' }
-
   around do |example|
     # Instead of test environment's :null_store so the throttles can increment
     Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
@@ -35,112 +30,10 @@ describe 'Rack Attack global throttles' do
     Rack::Attack.cache.store = Rails.cache
   end
 
-  # Requires let variables:
-  # * throttle_setting_prefix (e.g. "throttle_authenticated_api" or "throttle_authenticated_web")
-  # * get_args
-  # * other_user_get_args
-  shared_examples_for 'rate-limited token-authenticated requests' do
-    before do
-      # Set low limits
-      settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
-      settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds
-    end
-
-    context 'when the throttle is enabled' do
-      before do
-        settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true
-        stub_application_setting(settings_to_set)
-      end
-
-      it 'rejects requests over the rate limit' do
-        # At first, allow requests under the rate limit.
-        requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-
-        # the last straw
-        expect_rejection { get(*get_args) }
-      end
-
-      it 'allows requests after throttling and then waiting for the next period' do
-        requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-
-        expect_rejection { get(*get_args) }
-
-        Timecop.travel(period.from_now) do
-          requests_per_period.times do
-            get(*get_args)
-            expect(response).to have_http_status 200
-          end
-
-          expect_rejection { get(*get_args) }
-        end
-      end
-
-      it 'counts requests from different users separately, even from the same IP' do
-        requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-
-        # would be over the limit if this wasn't a different user
-        get(*other_user_get_args)
-        expect(response).to have_http_status 200
-      end
-
-      it 'counts all requests from the same user, even via different IPs' do
-        requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-
-        expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
-
-        expect_rejection { get(*get_args) }
-      end
-
-      it 'logs RackAttack info into structured logs' do
-        requests_per_period.times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-
-        arguments = {
-          message: 'Rack_Attack',
-          env: :throttle,
-          remote_ip: '127.0.0.1',
-          request_method: 'GET',
-          path: get_args.first,
-          user_id: user.id,
-          username: user.username
-        }
-
-        expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
-
-        expect_rejection { get(*get_args) }
-      end
-    end
-
-    context 'when the throttle is disabled' do
-      before do
-        settings_to_set[:"#{throttle_setting_prefix}_enabled"] = false
-        stub_application_setting(settings_to_set)
-      end
-
-      it 'allows requests over the rate limit' do
-        (1 + requests_per_period).times do
-          get(*get_args)
-          expect(response).to have_http_status 200
-        end
-      end
-    end
-  end
-
   describe 'unauthenticated requests' do
+    let(:url_that_does_not_require_authentication) { '/users/sign_in' }
+    let(:url_api_internal) { '/api/v4/internal/check' }
+
     before do
       # Set low limits
       settings_to_set[:throttle_unauthenticated_requests_per_period] = requests_per_period
@@ -245,6 +138,7 @@ describe 'Rack Attack global throttles' do
     let(:other_user) { create(:user) }
     let(:other_user_token) { create(:personal_access_token, user: other_user) }
     let(:throttle_setting_prefix) { 'throttle_authenticated_api' }
+    let(:api_partial_url) { '/todos' }
 
     context 'with the token in the query string' do
       let(:get_args) { [api(api_partial_url, personal_access_token: token)] }
@@ -265,10 +159,13 @@ describe 'Rack Attack global throttles' do
     let(:user) { create(:user) }
     let(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
     let(:token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api") }
+
     let(:other_user) { create(:user) }
     let(:other_user_application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: other_user) }
     let(:other_user_token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: other_user.id, scopes: "api") }
+
     let(:throttle_setting_prefix) { 'throttle_authenticated_api' }
+    let(:api_partial_url) { '/todos' }
 
     context 'with the token in the query string' do
       let(:get_args) { [api(api_partial_url, oauth_access_token: token)] }
@@ -299,110 +196,11 @@ describe 'Rack Attack global throttles' do
   end
 
   describe 'web requests authenticated with regular login' do
+    let(:throttle_setting_prefix) { 'throttle_authenticated_web' }
     let(:user) { create(:user) }
+    let(:url_that_requires_authentication) { '/dashboard/snippets' }
 
-    before do
-      login_as(user)
-
-      # Set low limits
-      settings_to_set[:throttle_authenticated_web_requests_per_period] = requests_per_period
-      settings_to_set[:throttle_authenticated_web_period_in_seconds] = period_in_seconds
-    end
-
-    context 'when the throttle is enabled' do
-      before do
-        settings_to_set[:throttle_authenticated_web_enabled] = true
-        stub_application_setting(settings_to_set)
-      end
-
-      it 'rejects requests over the rate limit' do
-        # At first, allow requests under the rate limit.
-        requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-
-        # the last straw
-        expect_rejection { get url_that_requires_authentication }
-      end
-
-      it 'allows requests after throttling and then waiting for the next period' do
-        requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-
-        expect_rejection { get url_that_requires_authentication }
-
-        Timecop.travel(period.from_now) do
-          requests_per_period.times do
-            get url_that_requires_authentication
-            expect(response).to have_http_status 200
-          end
-
-          expect_rejection { get url_that_requires_authentication }
-        end
-      end
-
-      it 'counts requests from different users separately, even from the same IP' do
-        requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-
-        # would be over the limit if this wasn't a different user
-        login_as(create(:user))
-
-        get url_that_requires_authentication
-        expect(response).to have_http_status 200
-      end
-
-      it 'counts all requests from the same user, even via different IPs' do
-        requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-
-        expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
-
-        expect_rejection { get url_that_requires_authentication }
-      end
-
-      it 'logs RackAttack info into structured logs' do
-        requests_per_period.times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-
-        arguments = {
-          message: 'Rack_Attack',
-          env: :throttle,
-          remote_ip: '127.0.0.1',
-          request_method: 'GET',
-          path: '/dashboard/snippets',
-          user_id: user.id,
-          username: user.username
-        }
-
-        expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
-
-        get url_that_requires_authentication
-      end
-    end
-
-    context 'when the throttle is disabled' do
-      before do
-        settings_to_set[:throttle_authenticated_web_enabled] = false
-        stub_application_setting(settings_to_set)
-      end
-
-      it 'allows requests over the rate limit' do
-        (1 + requests_per_period).times do
-          get url_that_requires_authentication
-          expect(response).to have_http_status 200
-        end
-      end
-    end
+    it_behaves_like 'rate-limited web authenticated requests'
   end
 
   def api_get_args_with_token_headers(partial_url, token_headers)

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

+# frozen_string_literal: true
+#
+# Requires let variables:
+# * throttle_setting_prefix: "throttle_authenticated_api", "throttle_authenticated_web", "throttle_protected_paths"
+# * get_args
+# * other_user_get_args
+# * requests_per_period
+# * period_in_seconds
+# * period
+shared_examples_for 'rate-limited token-authenticated requests' do
+  before do
+    # Set low limits
+    settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
+    settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds
+  end
+
+  context 'when the throttle is enabled' do
+    before do
+      settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true
+      stub_application_setting(settings_to_set)
+    end
+
+    it 'rejects requests over the rate limit' do
+      # At first, allow requests under the rate limit.
+      requests_per_period.times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+
+      # the last straw
+      expect_rejection { get(*get_args) }
+    end
+
+    it 'allows requests after throttling and then waiting for the next period' do
+      requests_per_period.times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+
+      expect_rejection { get(*get_args) }
+
+      Timecop.travel(period.from_now) do
+        requests_per_period.times do
+          get(*get_args)
+          expect(response).to have_http_status 200
+        end
+
+        expect_rejection { get(*get_args) }
+      end
+    end
+
+    it 'counts requests from different users separately, even from the same IP' do
+      requests_per_period.times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+
+      # would be over the limit if this wasn't a different user
+      get(*other_user_get_args)
+      expect(response).to have_http_status 200
+    end
+
+    it 'counts all requests from the same user, even via different IPs' do
+      requests_per_period.times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+
+      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
+
+      expect_rejection { get(*get_args) }
+    end
+
+    it 'logs RackAttack info into structured logs' do
+      requests_per_period.times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+
+      arguments = {
+        message: 'Rack_Attack',
+        env: :throttle,
+        remote_ip: '127.0.0.1',
+        request_method: 'GET',
+        path: get_args.first,
+        user_id: user.id,
+        username: user.username
+      }
+
+      expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
+
+      expect_rejection { get(*get_args) }
+    end
+  end
+
+  context 'when the throttle is disabled' do
+    before do
+      settings_to_set[:"#{throttle_setting_prefix}_enabled"] = false
+      stub_application_setting(settings_to_set)
+    end
+
+    it 'allows requests over the rate limit' do
+      (1 + requests_per_period).times do
+        get(*get_args)
+        expect(response).to have_http_status 200
+      end
+    end
+  end
+end
+
+# Requires let variables:
+# * throttle_setting_prefix: "throttle_authenticated_web" or "throttle_protected_paths"
+# * user
+# * url_that_requires_authentication
+# * requests_per_period
+# * period_in_seconds
+# * period
+shared_examples_for 'rate-limited web authenticated requests' do
+  before do
+    login_as(user)
+
+    # Set low limits
+    settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period
+    settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds
+  end
+
+  context 'when the throttle is enabled' do
+    before do
+      settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true
+      stub_application_setting(settings_to_set)
+    end
+
+    it 'rejects requests over the rate limit' do
+      # At first, allow requests under the rate limit.
+      requests_per_period.times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+
+      # the last straw
+      expect_rejection { get url_that_requires_authentication }
+    end
+
+    it 'allows requests after throttling and then waiting for the next period' do
+      requests_per_period.times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+
+      expect_rejection { get url_that_requires_authentication }
+
+      Timecop.travel(period.from_now) do
+        requests_per_period.times do
+          get url_that_requires_authentication
+          expect(response).to have_http_status 200
+        end
+
+        expect_rejection { get url_that_requires_authentication }
+      end
+    end
+
+    it 'counts requests from different users separately, even from the same IP' do
+      requests_per_period.times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+
+      # would be over the limit if this wasn't a different user
+      login_as(create(:user))
+
+      get url_that_requires_authentication
+      expect(response).to have_http_status 200
+    end
+
+    it 'counts all requests from the same user, even via different IPs' do
+      requests_per_period.times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+
+      expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4')
+
+      expect_rejection { get url_that_requires_authentication }
+    end
+
+    it 'logs RackAttack info into structured logs' do
+      requests_per_period.times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+
+      arguments = {
+        message: 'Rack_Attack',
+        env: :throttle,
+        remote_ip: '127.0.0.1',
+        request_method: 'GET',
+        path: '/dashboard/snippets',
+        user_id: user.id,
+        username: user.username
+      }
+
+      expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once
+
+      get url_that_requires_authentication
+    end
+  end
+
+  context 'when the throttle is disabled' do
+    before do
+      settings_to_set[:"#{throttle_setting_prefix}_enabled"] = false
+      stub_application_setting(settings_to_set)
+    end
+
+    it 'allows requests over the rate limit' do
+      (1 + requests_per_period).times do
+        get url_that_requires_authentication
+        expect(response).to have_http_status 200
+      end
+    end
+  end
+end