From e4a92d342784ccbb929e7d2b1faa42d6c2f591a3 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 6 Jan 2023 22:28:28 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-7-stable-ee --- app/controllers/projects/grafana_api_controller.rb | 2 + app/policies/project_policy.rb | 3 + app/services/users/update_service.rb | 8 +++ .../_confirmation_instructions_account.html.haml | 2 +- .../_confirmation_instructions_account.text.erb | 2 +- doc/operations/metrics/embed_grafana.md | 7 +- .../projects/grafana_api_controller_spec.rb | 82 ++++++++++++++++++++-- spec/mailers/devise_mailer_spec.rb | 12 +++- spec/models/user_spec.rb | 28 ++++++++ spec/policies/project_policy_spec.rb | 29 ++++++++ spec/services/users/update_service_spec.rb | 43 ++++++++++++ 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 -- cgit v1.2.3