summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/application_controller.rb10
-rw-r--r--app/controllers/concerns/confirm_email_warning.rb9
-rw-r--r--app/controllers/concerns/uploads_actions.rb17
-rw-r--r--app/controllers/uploads_controller.rb2
-rw-r--r--app/models/environment.rb1
-rw-r--r--app/models/project.rb1
-rw-r--r--app/policies/project_policy.rb15
-rw-r--r--changelogs/unreleased/nicolasdular-confirm-email-before-ci-usage.yml5
-rw-r--r--doc/development/feature_flags/controls.md8
-rw-r--r--locale/gitlab.pot2
-rw-r--r--spec/controllers/application_controller_spec.rb14
-rw-r--r--spec/controllers/concerns/confirm_email_warning_spec.rb2
-rw-r--r--spec/controllers/uploads_controller_spec.rb24
-rw-r--r--spec/features/users/login_spec.rb2
-rw-r--r--spec/features/users/signup_spec.rb2
-rw-r--r--spec/lib/gitlab/import_export/all_models.yml1
-rw-r--r--spec/policies/project_policy_spec.rb25
-rw-r--r--spec/requests/user_avatar_spec.rb36
18 files changed, 85 insertions, 91 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 1311c745da3..30882f5a9f7 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!
before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket!
- before_action :check_password_expiration, if: :html_request?
+ before_action :check_password_expiration
before_action :ldap_security_check
before_action :sentry_context
before_action :default_headers
- before_action :add_gon_variables, if: :html_request?
+ before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
before_action :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller?
@@ -449,8 +449,8 @@ class ApplicationController < ActionController::Base
response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end
- def html_request?
- request.format.html?
+ def peek_request?
+ request.path.start_with?('/-/peek')
end
def json_request?
@@ -460,7 +460,7 @@ class ApplicationController < ActionController::Base
def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
- html_request? && !devise_controller?
+ !(peek_request? || devise_controller?)
end
def set_usage_stats_consent_flag
diff --git a/app/controllers/concerns/confirm_email_warning.rb b/app/controllers/concerns/confirm_email_warning.rb
index 874deeb4702..86df0010665 100644
--- a/app/controllers/concerns/confirm_email_warning.rb
+++ b/app/controllers/concerns/confirm_email_warning.rb
@@ -4,22 +4,19 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern
included do
- before_action :set_confirm_warning, if: :show_confirm_warning?
+ before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
end
protected
- def show_confirm_warning?
- html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
- end
-
def set_confirm_warning
return unless current_user
return if current_user.confirmed?
+ return if peek_request? || json_request? || !request.get?
email = current_user.unconfirmed_email || current_user.email
- flash.now[:warning] = _("Please check your email (%{email}) to verify that you own this address. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}.").html_safe % {
+ flash.now[:warning] = _("Please check your email (%{email}) to verify that you own this address and unlock the power of CI/CD. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}.").html_safe % {
email: email,
resend_link: view_context.link_to(_('Resend it'), user_confirmation_path(user: { email: email }), method: :post),
update_link: view_context.link_to(_('Update it'), profile_path)
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index 023c41821da..b87779c22d3 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -1,16 +1,11 @@
# frozen_string_literal: true
module UploadsActions
- extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
include SendFileUpload
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
- included do
- prepend_before_action :set_request_format_from_path_extension
- end
-
def create
uploader = UploadService.new(model, params[:file], uploader_class).execute
@@ -69,18 +64,6 @@ module UploadsActions
private
- # From ActionDispatch::Http::MimeNegotiation. We have an initializer that
- # monkey-patches this method out (so that repository paths don't guess a
- # format based on extension), but we do want this behaviour when serving
- # uploads.
- def set_request_format_from_path_extension
- path = request.headers['action_dispatch.original_path'] || request.headers['PATH_INFO']
-
- if match = path&.match(/\.(\w+)\z/)
- request.format = match.captures.first
- end
- end
-
def uploader_class
raise NotImplementedError
end
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index f39a2b81b54..635db386792 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -20,7 +20,7 @@ class UploadsController < ApplicationController
skip_before_action :authenticate_user!
before_action :upload_mount_satisfied?
- before_action :model
+ before_action :find_model
before_action :authorize_access!, only: [:show]
before_action :authorize_create_access!, only: [:create, :authorize]
before_action :verify_workhorse_api!, only: [:authorize]
diff --git a/app/models/environment.rb b/app/models/environment.rb
index aace408b5ed..6bc244b1df4 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -65,6 +65,7 @@ class Environment < ApplicationRecord
scope :for_project, -> (project) { where(project_id: project) }
scope :with_deployment, -> (sha) { where('EXISTS (?)', Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)) }
+ scope :unfoldered, -> { where(environment_type: nil) }
state_machine :state, initial: :available do
event :start do
diff --git a/app/models/project.rb b/app/models/project.rb
index 2cb5af382b6..b44c8d1a655 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -286,6 +286,7 @@ class Project < ApplicationRecord
has_many :variables, class_name: 'Ci::Variable'
has_many :triggers, class_name: 'Ci::Trigger'
has_many :environments
+ has_many :unfoldered_environments, -> { unfoldered.available }, class_name: 'Environment'
has_many :deployments
has_many :pipeline_schedules, class_name: 'Ci::PipelineSchedule'
has_many :project_deploy_tokens
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index ea2be37d7e6..ff70c6e6aeb 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -117,6 +117,10 @@ class ProjectPolicy < BasePolicy
!@subject.builds_enabled?
end
+ condition(:user_confirmed?) do
+ @user && @user.confirmed?
+ end
+
features = %w[
merge_requests
issues
@@ -249,10 +253,7 @@ class ProjectPolicy < BasePolicy
enable :update_commit_status
enable :create_build
enable :update_build
- enable :create_pipeline
- enable :update_pipeline
enable :read_pipeline_schedule
- enable :create_pipeline_schedule
enable :create_merge_request_from
enable :create_wiki
enable :push_code
@@ -267,6 +268,12 @@ class ProjectPolicy < BasePolicy
enable :update_release
end
+ rule { can?(:developer_access) & user_confirmed? }.policy do
+ enable :create_pipeline
+ enable :update_pipeline
+ enable :create_pipeline_schedule
+ end
+
rule { can?(:maintainer_access) }.policy do
enable :admin_board
enable :push_to_delete_protected_branch
@@ -418,7 +425,7 @@ class ProjectPolicy < BasePolicy
# These rules are included to allow maintainers of projects to push to certain
# to run pipelines for the branches they have access to.
- rule { can?(:public_access) & has_merge_requests_allowing_pushes }.policy do
+ rule { can?(:public_access) & has_merge_requests_allowing_pushes & user_confirmed? }.policy do
enable :create_build
enable :create_pipeline
end
diff --git a/changelogs/unreleased/nicolasdular-confirm-email-before-ci-usage.yml b/changelogs/unreleased/nicolasdular-confirm-email-before-ci-usage.yml
new file mode 100644
index 00000000000..f24ffb6c368
--- /dev/null
+++ b/changelogs/unreleased/nicolasdular-confirm-email-before-ci-usage.yml
@@ -0,0 +1,5 @@
+---
+title: Only allow confirmed users to run pipelines
+merge_request:
+author:
+type: fixed
diff --git a/doc/development/feature_flags/controls.md b/doc/development/feature_flags/controls.md
index f22c3bb1e37..cbbbe1cd5ea 100644
--- a/doc/development/feature_flags/controls.md
+++ b/doc/development/feature_flags/controls.md
@@ -29,6 +29,14 @@ Monitor stage, Health group.
For all production environment Chatops commands, use the `#production` channel.
+Regardless of the channel in which the Chatops command is ran, any feature flag change that affects GitLab.com will automatically be logged in an issue.
+
+The issue is created in the [gl-infra/feature-flag-log](https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/issues?scope=all&utf8=%E2%9C%93&state=closed) project, and it will at minimum log the Slack handle of person enabling a feature flag, the time, and the name of the flag being changed.
+
+The issue is then also posted to GitLab Inc. internal [Grafana dashboard](https://dashboards.gitlab.net/) as an annotation marker to make the change even more visible.
+
+Changes to the issue format can be submitted in the [Chatops project](https://gitlab.com/gitlab-com/chatops).
+
## Rolling out changes
When the changes are deployed to the environments it is time to start
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index c246741655e..f48df8b2b60 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -12205,7 +12205,7 @@ msgstr ""
msgid "Please check the configuration file to ensure that it is available and the YAML is valid"
msgstr ""
-msgid "Please check your email (%{email}) to verify that you own this address. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}."
+msgid "Please check your email (%{email}) to verify that you own this address and unlock the power of CI/CD. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}."
msgstr ""
msgid "Please choose a group URL with no special characters."
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 481883c50fa..94afe741057 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -90,16 +90,18 @@ describe ApplicationController do
let(:format) { :html }
it_behaves_like 'setting gon variables'
- end
- context 'with json format' do
- let(:format) { :json }
+ context 'for peek requests' do
+ before do
+ request.path = '/-/peek'
+ end
- it_behaves_like 'not setting gon variables'
+ it_behaves_like 'not setting gon variables'
+ end
end
- context 'with atom format' do
- let(:format) { :atom }
+ context 'with json format' do
+ let(:format) { :json }
it_behaves_like 'not setting gon variables'
end
diff --git a/spec/controllers/concerns/confirm_email_warning_spec.rb b/spec/controllers/concerns/confirm_email_warning_spec.rb
index 0c598a360af..25429cdd149 100644
--- a/spec/controllers/concerns/confirm_email_warning_spec.rb
+++ b/spec/controllers/concerns/confirm_email_warning_spec.rb
@@ -19,7 +19,7 @@ describe ConfirmEmailWarning do
RSpec::Matchers.define :set_confirm_warning_for do |email|
match do |response|
- expect(response).to set_flash.now[:warning].to include("Please check your email (#{email}) to verify that you own this address.")
+ expect(response).to set_flash.now[:warning].to include("Please check your email (#{email}) to verify that you own this address and unlock the power of CI/CD.")
end
end
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index f35babc1b56..1bcf3bb106b 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -228,10 +228,10 @@ describe UploadsController do
user.block
end
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
@@ -320,10 +320,10 @@ describe UploadsController do
end
context "when not signed in" do
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
@@ -343,10 +343,10 @@ describe UploadsController do
project.add_maintainer(user)
end
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
@@ -439,10 +439,10 @@ describe UploadsController do
user.block
end
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
@@ -526,10 +526,10 @@ describe UploadsController do
end
context "when not signed in" do
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
@@ -549,10 +549,10 @@ describe UploadsController do
project.add_maintainer(user)
end
- it "responds with status 401" do
+ it "redirects to the sign in page" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to have_gitlab_http_status(401)
+ expect(response).to redirect_to(new_user_session_path)
end
end
diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb
index d1f3b3f4076..b7c54bb6de8 100644
--- a/spec/features/users/login_spec.rb
+++ b/spec/features/users/login_spec.rb
@@ -806,7 +806,7 @@ describe 'Login' do
gitlab_sign_in(user)
expect(current_path).to eq root_path
- expect(page).to have_content("Please check your email (#{user.email}) to verify that you own this address.")
+ expect(page).to have_content("Please check your email (#{user.email}) to verify that you own this address and unlock the power of CI/CD.")
end
context "when not having confirmed within Devise's allow_unconfirmed_access_for time" do
diff --git a/spec/features/users/signup_spec.rb b/spec/features/users/signup_spec.rb
index 8a82d3f3bd0..5d4c30b6e8e 100644
--- a/spec/features/users/signup_spec.rb
+++ b/spec/features/users/signup_spec.rb
@@ -222,7 +222,7 @@ shared_examples 'Signup' do
expect(current_path).to eq users_sign_up_welcome_path
else
expect(current_path).to eq dashboard_projects_path
- expect(page).to have_content("Please check your email (#{new_user.email}) to verify that you own this address.")
+ expect(page).to have_content("Please check your email (#{new_user.email}) to verify that you own this address and unlock the power of CI/CD.")
end
end
end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index d0e4b92410d..8bad40c629b 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -339,6 +339,7 @@ project:
- triggers
- pipeline_schedules
- environments
+- unfoldered_environments
- deployments
- project_feature
- auto_devops
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index d0788452a20..ab54d97f2a2 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -315,6 +315,31 @@ describe ProjectPolicy do
end
end
+ context 'pipeline feature' do
+ let(:project) { create(:project) }
+
+ describe 'for unconfirmed user' do
+ let(:unconfirmed_user) { create(:user, confirmed_at: nil) }
+ subject { described_class.new(unconfirmed_user, project) }
+
+ it 'disallows to modify pipelines' do
+ expect_disallowed(:create_pipeline)
+ expect_disallowed(:update_pipeline)
+ expect_disallowed(:create_pipeline_schedule)
+ end
+ end
+
+ describe 'for confirmed user' do
+ subject { described_class.new(developer, project) }
+
+ it 'allows modify pipelines' do
+ expect_allowed(:create_pipeline)
+ expect_allowed(:update_pipeline)
+ expect_allowed(:create_pipeline_schedule)
+ end
+ end
+ end
+
context 'builds feature' do
context 'when builds are disabled' do
subject { described_class.new(owner, project) }
diff --git a/spec/requests/user_avatar_spec.rb b/spec/requests/user_avatar_spec.rb
deleted file mode 100644
index 9451674161c..00000000000
--- a/spec/requests/user_avatar_spec.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe 'Loading a user avatar' do
- let(:user) { create(:user, :with_avatar) }
-
- context 'when logged in' do
- # The exact query count will vary depending on the 2FA settings of the
- # instance, group, and user. Removing those extra 2FA queries in this case
- # may not be a good idea, so we just set up the ideal case.
- before do
- stub_application_setting(require_two_factor_authentication: true)
-
- login_as(create(:user, :two_factor))
- end
-
- # One each for: current user, avatar user, and upload record
- it 'only performs three SQL queries' do
- get user.avatar_url # Skip queries on first application load
-
- expect(response).to have_gitlab_http_status(200)
- expect { get user.avatar_url }.not_to exceed_query_limit(3)
- end
- end
-
- context 'when logged out' do
- # One each for avatar user and upload record
- it 'only performs two SQL queries' do
- get user.avatar_url # Skip queries on first application load
-
- expect(response).to have_gitlab_http_status(200)
- expect { get user.avatar_url }.not_to exceed_query_limit(2)
- end
- end
-end
View Lorry Controller Status