Add latest changes from gitlab-org/security/gitlab@15-7-stable-ee

11 files changed, 205 insertions, 13 deletions

diff --git a/app/controllers/projects/grafana_api_controller.rb b/app/controllers/projects/grafana_api_controller.rb
index d5099367873..9cd511f6a11 100644
--- a/app/controllers/projects/grafana_api_controller.rb
+++ b/app/controllers/projects/grafana_api_controller.rb
@@ -4,6 +4,8 @@ class Projects::GrafanaApiController < Projects::ApplicationController
   include RenderServiceResults
   include MetricsDashboard
 
+  before_action :authorize_read_grafana!, only: :proxy
+
   feature_category :metrics
   urgency :low
 
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 7f67e80e432..2a13fafa313 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -308,6 +308,8 @@ class ProjectPolicy < BasePolicy
   rule { guest & can?(:download_code) }.enable :build_download_code
   rule { guest & can?(:read_container_image) }.enable :build_read_container_image
 
+  rule { guest & ~public_project }.enable :read_grafana
+
   rule { can?(:reporter_access) }.policy do
     enable :admin_issue_board
     enable :download_code
@@ -340,6 +342,7 @@ class ProjectPolicy < BasePolicy
     enable :read_package
     enable :read_product_analytics
     enable :read_ci_cd_analytics
+    enable :read_grafana
   end
 
   # We define `:public_user_access` separately because there are cases in gitlab-ee
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index cb2711b6fee..96018db5974 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -31,6 +31,7 @@ module Users
 
       assign_identity
       build_canonical_email
+      reset_unconfirmed_email
 
       if @user.save(validate: validate) && update_status
         notify_success(user_exists)
@@ -64,6 +65,13 @@ module Users
       Users::UpdateCanonicalEmailService.new(user: @user).execute
     end
 
+    def reset_unconfirmed_email
+      return unless @user.persisted?
+      return unless @user.email_changed?
+
+      @user.update_column(:unconfirmed_email, nil)
+    end
+
     def update_status
       return true unless @status_params
 
diff --git a/app/views/devise/mailer/_confirmation_instructions_account.html.haml b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
index 9d469ff6e7b..c1655818770 100644
--- a/app/views/devise/mailer/_confirmation_instructions_account.html.haml
+++ b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
@@ -1,7 +1,7 @@
 - confirmation_link = confirmation_url(@resource, confirmation_token: @token)
 - if @resource.unconfirmed_email.present? || !@resource.created_recently?
   #content
-    = email_default_heading(@resource.unconfirmed_email || @resource.email)
+    = email_default_heading(@email)
     %p= _('Click the link below to confirm your email address.')
     #cta
       = link_to _('Confirm your email address'), confirmation_link
diff --git a/app/views/devise/mailer/_confirmation_instructions_account.text.erb b/app/views/devise/mailer/_confirmation_instructions_account.text.erb
index e6da78e3a3d..7e4f38885f6 100644
--- a/app/views/devise/mailer/_confirmation_instructions_account.text.erb
+++ b/app/views/devise/mailer/_confirmation_instructions_account.text.erb
@@ -1,5 +1,5 @@
 <% if @resource.unconfirmed_email.present? || !@resource.created_recently? %>
-<%= @resource.unconfirmed_email || @resource.email %>,
+<%= @email %>,
 <%= _('Use the link below to confirm your email address.') %>
 <% else %>
   <% if Gitlab.com? %>
diff --git a/doc/operations/metrics/embed_grafana.md b/doc/operations/metrics/embed_grafana.md
index b307aa5fa32..43a7447a978 100644
--- a/doc/operations/metrics/embed_grafana.md
+++ b/doc/operations/metrics/embed_grafana.md
@@ -55,9 +55,10 @@ To set up the Grafana API in Grafana:
 1. Select **Save Changes**.
 
 NOTE:
-If the Grafana integration is enabled, any user with read access to the GitLab
-project can query metrics from the Prometheus instance. All requests proxied
-through GitLab are authenticated with the same Grafana Administrator API token.
+If the Grafana integration is enabled, users with the Reporter role on public
+projects and the Guest role on non-public projects can query metrics from the
+Prometheus instance. All requests proxied through GitLab are authenticated with
+the same Grafana Administrator API token.
 
 ### Generate a link to a panel
 
diff --git a/spec/controllers/projects/grafana_api_controller_spec.rb b/spec/controllers/projects/grafana_api_controller_spec.rb
index 2e25b0271ce..90ab49f9467 100644
--- a/spec/controllers/projects/grafana_api_controller_spec.rb
+++ b/spec/controllers/projects/grafana_api_controller_spec.rb
@@ -2,13 +2,20 @@
 
 require 'spec_helper'
 
-RSpec.describe Projects::GrafanaApiController do
-  let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user) }
+RSpec.describe Projects::GrafanaApiController, feature_category: :metrics do
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+  let(:anonymous) { nil }
+  let(:user) { reporter }
+
+  before_all do
+    project.add_reporter(reporter)
+    project.add_guest(guest)
+  end
 
   before do
-    project.add_reporter(user)
-    sign_in(user)
+    sign_in(user) if user
   end
 
   describe 'GET #proxy' do
@@ -41,6 +48,39 @@ RSpec.describe Projects::GrafanaApiController do
       end
     end
 
+    shared_examples_for 'accessible' do
+      let(:service_result) { nil }
+
+      it 'returns non erroneous response' do
+        get :proxy, params: params
+
+        # We don't care about the specific code as long it's not an error.
+        expect(response).to have_gitlab_http_status(:no_content)
+      end
+    end
+
+    shared_examples_for 'not accessible' do
+      let(:service_result) { nil }
+
+      it 'returns 404 Not found' do
+        get :proxy, params: params
+
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(Grafana::ProxyService).not_to have_received(:new)
+      end
+    end
+
+    shared_examples_for 'login required' do
+      let(:service_result) { nil }
+
+      it 'redirects to login page' do
+        get :proxy, params: params
+
+        expect(response).to redirect_to(new_user_session_path)
+        expect(Grafana::ProxyService).not_to have_received(:new)
+      end
+    end
+
     context 'with a successful result' do
       let(:service_result) { { status: :success, body: '{}' } }
 
@@ -96,6 +136,38 @@ RSpec.describe Projects::GrafanaApiController do
         it_behaves_like 'error response', :bad_request
       end
     end
+
+    context 'as guest' do
+      let(:user) { guest }
+
+      it_behaves_like 'not accessible'
+    end
+
+    context 'as anonymous' do
+      let(:user) { anonymous }
+
+      it_behaves_like 'not accessible'
+    end
+
+    context 'on a private project' do
+      let_it_be(:project) { create(:project, :private) }
+
+      before_all do
+        project.add_guest(guest)
+      end
+
+      context 'as anonymous' do
+        let(:user) { anonymous }
+
+        it_behaves_like 'login required'
+      end
+
+      context 'as guest' do
+        let(:user) { guest }
+
+        it_behaves_like 'accessible'
+      end
+    end
   end
 
   describe 'GET #metrics_dashboard' do
diff --git a/spec/mailers/devise_mailer_spec.rb b/spec/mailers/devise_mailer_spec.rb
index 360eb827927..b8e768b7a5f 100644
--- a/spec/mailers/devise_mailer_spec.rb
+++ b/spec/mailers/devise_mailer_spec.rb
@@ -11,7 +11,7 @@ RSpec.describe DeviseMailer do
     subject { described_class.confirmation_instructions(user, 'faketoken', {}) }
 
     context "when confirming a new account" do
-      let(:user) { build(:user, created_at: 1.minute.ago, unconfirmed_email: nil) }
+      let(:user) { create(:user, created_at: 1.minute.ago) }
 
       it "shows the expected text" do
         expect(subject.body.encoded).to have_text "Welcome"
@@ -20,7 +20,13 @@ RSpec.describe DeviseMailer do
     end
 
     context "when confirming the unconfirmed_email" do
-      let(:user) { build(:user, unconfirmed_email: 'jdoe@example.com') }
+      subject { described_class.confirmation_instructions(user, user.confirmation_token, { to: user.unconfirmed_email }) }
+
+      let(:user) { create(:user) }
+
+      before do
+        user.update!(email: 'unconfirmed-email@example.com')
+      end
 
       it "shows the expected text" do
         expect(subject.body.encoded).not_to have_text "Welcome"
@@ -30,7 +36,7 @@ RSpec.describe DeviseMailer do
     end
 
     context "when re-confirming the primary email after a security issue" do
-      let(:user) { build(:user, created_at: 10.days.ago, unconfirmed_email: nil) }
+      let(:user) { create(:user, created_at: Devise.confirm_within.ago) }
 
       it "shows the expected text" do
         expect(subject.body.encoded).not_to have_text "Welcome"
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 4a66af4ddf1..4dbcc68af19 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -311,6 +311,34 @@ RSpec.describe User do
         end
       end
     end
+
+    describe 'confirmation instructions for unconfirmed email' do
+      let(:unconfirmed_email) { 'first-unconfirmed-email@example.com' }
+      let(:another_unconfirmed_email) { 'another-unconfirmed-email@example.com' }
+
+      context 'when email is changed to another before performing the job that sends confirmation instructions for previous email change request' do
+        it "mentions the recipient's email in the message body", :aggregate_failures do
+          same_user = User.find(user.id)
+          same_user.update!(email: unconfirmed_email)
+
+          user.update!(email: another_unconfirmed_email)
+
+          perform_enqueued_jobs
+
+          confirmation_instructions_for_unconfirmed_email = ActionMailer::Base.deliveries.find do |message|
+            message.subject == 'Confirmation instructions' && message.to.include?(unconfirmed_email)
+          end
+          expect(confirmation_instructions_for_unconfirmed_email.html_part.body.encoded).to match same_user.unconfirmed_email
+          expect(confirmation_instructions_for_unconfirmed_email.text_part.body.encoded).to match same_user.unconfirmed_email
+
+          confirmation_instructions_for_another_unconfirmed_email = ActionMailer::Base.deliveries.find do |message|
+            message.subject == 'Confirmation instructions' && message.to.include?(another_unconfirmed_email)
+          end
+          expect(confirmation_instructions_for_another_unconfirmed_email.html_part.body.encoded).to match user.unconfirmed_email
+          expect(confirmation_instructions_for_another_unconfirmed_email.text_part.body.encoded).to match user.unconfirmed_email
+        end
+      end
+    end
   end
 
   describe 'validations' do
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 9b2d10283f1..e370f536519 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -668,6 +668,35 @@ RSpec.describe ProjectPolicy do
     end
   end
 
+  describe 'read_grafana', feature_category: :metrics do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:policy) { :read_grafana }
+
+    where(:project_visibility, :role, :allowed) do
+      :public   | :anonymous | false
+      :public   | :guest     | false
+      :public   | :reporter  | true
+      :internal | :anonymous | false
+      :internal | :guest     | true
+      :internal | :reporter  | true
+      :private  | :anonymous | false
+      :private  | :guest     | true
+      :private  | :reporter  | true
+    end
+
+    with_them do
+      let(:current_user) { public_send(role) }
+      let(:project) { public_send("#{project_visibility}_project") }
+
+      if params[:allowed]
+        it { is_expected.to be_allowed(policy) }
+      else
+        it { is_expected.not_to be_allowed(policy) }
+      end
+    end
+  end
+
   describe 'update_max_artifacts_size' do
     context 'when no user' do
       let(:current_user) { anonymous }
diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb
index 411cd7316d8..f4ea757f81a 100644
--- a/spec/services/users/update_service_spec.rb
+++ b/spec/services/users/update_service_spec.rb
@@ -77,6 +77,34 @@ RSpec.describe Users::UpdateService do
           subject
         end
 
+        context 'when race condition' do
+          # See https://gitlab.com/gitlab-org/gitlab/-/issues/382957
+          it 'updates email for stale user', :aggregate_failures do
+            unconfirmed_email = 'unconfirmed-email-user-has-access-to@example.com'
+            forgery_email = 'forgery@example.com'
+
+            user.update!(email: unconfirmed_email)
+
+            stale_user = User.find(user.id)
+
+            service1 = described_class.new(stale_user, { email: unconfirmed_email }.merge(user: stale_user))
+
+            service2 = described_class.new(user, { email: forgery_email }.merge(user: user))
+
+            service2.execute
+            reloaded_user = User.find(user.id)
+            expect(reloaded_user.unconfirmed_email).to eq(forgery_email)
+            expect(stale_user.confirmation_token).not_to eq(user.confirmation_token)
+            expect(reloaded_user.confirmation_token).to eq(user.confirmation_token)
+
+            service1.execute
+            reloaded_user = User.find(user.id)
+            expect(reloaded_user.unconfirmed_email).to eq(unconfirmed_email)
+            expect(stale_user.confirmation_token).not_to eq(user.confirmation_token)
+            expect(reloaded_user.confirmation_token).to eq(stale_user.confirmation_token)
+          end
+        end
+
         context 'when check_password is true' do
           def update_user(user, opts)
             described_class.new(user, opts.merge(user: user)).execute(check_password: true)
@@ -139,9 +167,24 @@ RSpec.describe Users::UpdateService do
             update_user(user, job_title: 'supreme leader of the universe')
           end.not_to change { user.user_canonical_email }
         end
+
+        it 'does not reset unconfirmed email' do
+          unconfirmed_email = 'unconfirmed-email@example.com'
+          user.update!(email: unconfirmed_email)
+
+          expect do
+            update_user(user, job_title: 'supreme leader of the universe')
+          end.not_to change { user.unconfirmed_email }
+        end
       end
     end
 
+    it 'does not try to reset unconfirmed email for a new user' do
+      expect do
+        update_user(build(:user), job_title: 'supreme leader of the universe')
+      end.not_to raise_error
+    end
+
     def update_user(user, opts)
       described_class.new(user, opts.merge(user: user)).execute
     end