From 5332995ca419bfaa8a84708914fb60c461434c9d Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 25 Oct 2018 13:19:56 +0200 Subject: Resolve reflected XSS in Ouath authorize window --- app/controllers/oauth/applications_controller.rb | 2 +- .../security-fix-uri-xss-applications.yml | 5 +++ config/initializers/doorkeeper.rb | 7 ++++ ...181026091631_migrate_forbidden_redirect_uris.rb | 32 +++++++++++++++ db/schema.rb | 2 +- .../oauth/applications_controller_spec.rb | 17 ++++++++ .../migrate_forbidden_redirect_uris_spec.rb | 48 ++++++++++++++++++++++ spec/requests/api/applications_spec.rb | 12 +++++- 8 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-fix-uri-xss-applications.yml create mode 100644 db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb create mode 100644 spec/migrations/migrate_forbidden_redirect_uris_spec.rb diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index b50f140dc80..ab4ca56bb49 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -9,7 +9,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController before_action :verify_user_oauth_applications_enabled, except: :index before_action :authenticate_user! before_action :add_gon_variables - before_action :load_scopes, only: [:index, :create, :edit] + before_action :load_scopes, only: [:index, :create, :edit, :update] helper_method :can? diff --git a/changelogs/unreleased/security-fix-uri-xss-applications.yml b/changelogs/unreleased/security-fix-uri-xss-applications.yml new file mode 100644 index 00000000000..0eaa1b1c4a3 --- /dev/null +++ b/changelogs/unreleased/security-fix-uri-xss-applications.yml @@ -0,0 +1,5 @@ +--- +title: Resolve reflected XSS in Ouath authorize window +merge_request: +author: +type: security diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index f321b4ea763..6be5c00daaa 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -48,6 +48,13 @@ Doorkeeper.configure do # force_ssl_in_redirect_uri false + # Specify what redirect URI's you want to block during Application creation. + # Any redirect URI is whitelisted by default. + # + # You can use this option in order to forbid URI's with 'javascript' scheme + # for example. + forbid_redirect_uri { |uri| %w[data vbscript javascript].include?(uri.scheme.to_s.downcase) } + # Provide support for an owner to be assigned to each registered application (disabled by default) # Optional parameter confirmation: true (default false) if you want to enforce ownership of # a registered application diff --git a/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb b/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb new file mode 100644 index 00000000000..ff5510e8eb7 --- /dev/null +++ b/db/post_migrate/20181026091631_migrate_forbidden_redirect_uris.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class MigrateForbiddenRedirectUris < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + FORBIDDEN_SCHEMES = %w[data:// vbscript:// javascript://] + NEW_URI = 'http://forbidden-scheme-has-been-overwritten' + + disable_ddl_transaction! + + def up + update_forbidden_uris(:oauth_applications) + update_forbidden_uris(:oauth_access_grants) + end + + def down + # noop + end + + private + + def update_forbidden_uris(table_name) + update_column_in_batches(table_name, :redirect_uri, NEW_URI) do |table, query| + where_clause = FORBIDDEN_SCHEMES.map do |scheme| + table[:redirect_uri].matches("#{scheme}%") + end.inject(&:or) + + query.where(where_clause) + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 50989960aa9..14c036e7651 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20181013005024) do +ActiveRecord::Schema.define(version: 20181026091631) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index ace8a954e92..b4219856fc0 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -40,6 +40,23 @@ describe Oauth::ApplicationsController do expect(response).to have_gitlab_http_status(302) expect(response).to redirect_to(profile_path) end + + context 'redirect_uri' do + render_views + + it 'shows an error for a forbidden URI' do + invalid_uri_params = { + doorkeeper_application: { + name: 'foo', + redirect_uri: 'javascript://alert()' + } + } + + post :create, invalid_uri_params + + expect(response.body).to include 'Redirect URI is forbidden by the server' + end + end end end diff --git a/spec/migrations/migrate_forbidden_redirect_uris_spec.rb b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb new file mode 100644 index 00000000000..0bc13a3974a --- /dev/null +++ b/spec/migrations/migrate_forbidden_redirect_uris_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20181026091631_migrate_forbidden_redirect_uris.rb') + +describe MigrateForbiddenRedirectUris, :migration do + let(:oauth_application) { table(:oauth_applications) } + let(:oauth_access_grant) { table(:oauth_access_grants) } + + let!(:control_app) { oauth_application.create(random_params) } + let!(:control_access_grant) { oauth_application.create(random_params) } + let!(:forbidden_js_app) { oauth_application.create(random_params.merge(redirect_uri: 'javascript://alert()')) } + let!(:forbidden_vb_app) { oauth_application.create(random_params.merge(redirect_uri: 'VBSCRIPT://alert()')) } + let!(:forbidden_access_grant) { oauth_application.create(random_params.merge(redirect_uri: 'vbscript://alert()')) } + + context 'oauth application' do + it 'migrates forbidden javascript URI' do + expect { migrate! }.to change { forbidden_js_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten') + end + + it 'migrates forbidden VBScript URI' do + expect { migrate! }.to change { forbidden_vb_app.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten') + end + + it 'does not migrate a valid URI' do + expect { migrate! }.not_to change { control_app.reload.redirect_uri } + end + end + + context 'access grant' do + it 'migrates forbidden VBScript URI' do + expect { migrate! }.to change { forbidden_access_grant.reload.redirect_uri }.to('http://forbidden-scheme-has-been-overwritten') + end + + it 'does not migrate a valid URI' do + expect { migrate! }.not_to change { control_access_grant.reload.redirect_uri } + end + end + + def random_params + { + name: 'test', + secret: 'test', + uid: Doorkeeper::OAuth::Helpers::UniqueToken.generate, + redirect_uri: 'http://valid.com' + } + end +end diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb index 270e12bf201..6154be5c425 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -25,7 +25,7 @@ describe API::Applications, :api do it 'does not allow creating an application with the wrong redirect_uri format' do expect do - post api('/applications', admin_user), name: 'application_name', redirect_uri: 'wrong_url_format', scopes: '' + post api('/applications', admin_user), name: 'application_name', redirect_uri: 'http://', scopes: '' end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(400) @@ -33,6 +33,16 @@ describe API::Applications, :api do expect(json_response['message']['redirect_uri'][0]).to eq('must be an absolute URI.') end + it 'does not allow creating an application with a forbidden URI format' do + expect do + post api('/applications', admin_user), name: 'application_name', redirect_uri: 'javascript://alert()', scopes: '' + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(400) + expect(json_response).to be_a Hash + expect(json_response['message']['redirect_uri'][0]).to eq('is forbidden by the server.') + end + it 'does not allow creating an application without a name' do expect do post api('/applications', admin_user), redirect_uri: 'http://application.url', scopes: '' -- cgit v1.2.1 From dd88eb0c5c22dcad3281cc8a3fcb3321e6b47dce Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 6 Nov 2018 15:46:19 +0100 Subject: Provide email notification on email updates --- app/views/devise/mailer/email_changed.html.haml | 12 ++++++++++++ app/views/devise/mailer/email_changed.text.erb | 10 ++++++++++ changelogs/unreleased/security-email-change-notification.yml | 5 +++++ config/initializers/devise.rb | 3 +++ doc/workflow/notifications.md | 2 ++ 5 files changed, 32 insertions(+) create mode 100644 app/views/devise/mailer/email_changed.html.haml create mode 100644 app/views/devise/mailer/email_changed.text.erb create mode 100644 changelogs/unreleased/security-email-change-notification.yml diff --git a/app/views/devise/mailer/email_changed.html.haml b/app/views/devise/mailer/email_changed.html.haml new file mode 100644 index 00000000000..5398430fdfd --- /dev/null +++ b/app/views/devise/mailer/email_changed.html.haml @@ -0,0 +1,12 @@ += email_default_heading("Hello, #{@resource.name}!") + +- if @resource.try(:unconfirmed_email?) + %p + We're contacting you to notify you that your email is being changed to #{@resource.reload.unconfirmed_email}. +- else + %p + We're contacting you to notify you that your email has been changed to #{@resource.email}. + +%p + If you did not initiate this change, please contact your administrator + immediately. diff --git a/app/views/devise/mailer/email_changed.text.erb b/app/views/devise/mailer/email_changed.text.erb new file mode 100644 index 00000000000..18137389e7b --- /dev/null +++ b/app/views/devise/mailer/email_changed.text.erb @@ -0,0 +1,10 @@ +Hello, <%= @resource.name %>! + +<% if @resource.try(:unconfirmed_email?) %> +We're contacting you to notify you that your email is being changed to <%= @resource.reload.unconfirmed_email %>. +<% else %> +We're contacting you to notify you that your email has been changed to <%= @resource.email %>. +<% end %> + +If you did not initiate this change, please contact your administrator +immediately. diff --git a/changelogs/unreleased/security-email-change-notification.yml b/changelogs/unreleased/security-email-change-notification.yml new file mode 100644 index 00000000000..45075ff20bb --- /dev/null +++ b/changelogs/unreleased/security-email-change-notification.yml @@ -0,0 +1,5 @@ +--- +title: Provide email notification when a user changes their email address +merge_request: +author: +type: security diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 179e00cdbd0..67eabb0b4fc 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -103,6 +103,9 @@ Devise.setup do |config| # Send a notification email when the user's password is changed config.send_password_change_notification = true + # Send a notification email when the user's email is changed + config.send_email_changed_notification = true + # ==> Configuration for :validatable # Range for password length. Default is 6..128. config.password_length = 8..128 diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index c590ac4b0ba..020aa73f809 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -64,6 +64,8 @@ Below is the table of events users can be notified of: |------------------------------|-------------------------------------------------------------------|------------------------------| | New SSH key added | User | Security email, always sent. | | New email added | User | Security email, always sent. | +| Email changed | User | Security email, always sent. | +| Password changed | User | Security email, always sent. | | New user created | User | Sent on user creation, except for omniauth (LDAP)| | User added to project | User | Sent when user is added to project | | Project access level changed | User | Sent when user project access level is changed | -- cgit v1.2.1 From 11ef7a0f1aa249fdb480a5e9fc4d6b9ef85b1d2c Mon Sep 17 00:00:00 2001 From: Chantal Rollison Date: Tue, 6 Nov 2018 08:43:04 -0800 Subject: Fixed read private group names --- app/controllers/dashboard/todos_controller.rb | 10 ++++++++++ changelogs/unreleased/security-private-group.yml | 6 ++++++ spec/controllers/dashboard/todos_controller_spec.rb | 10 ++++++++++ 3 files changed, 26 insertions(+) create mode 100644 changelogs/unreleased/security-private-group.yml diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index b82caf30a91..3fa582cf25b 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -4,6 +4,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController include ActionView::Helpers::NumberHelper before_action :authorize_read_project!, only: :index + before_action :authorize_read_group!, only: :index before_action :find_todos, only: [:index, :destroy_all] def index @@ -60,6 +61,15 @@ class Dashboard::TodosController < Dashboard::ApplicationController end end + def authorize_read_group! + group_id = params[:group_id] + + if group_id.present? + group = Group.find(group_id) + render_404 unless can?(current_user, :read_group, group) + end + end + def find_todos @todos ||= TodosFinder.new(current_user, todo_params).execute end diff --git a/changelogs/unreleased/security-private-group.yml b/changelogs/unreleased/security-private-group.yml new file mode 100644 index 00000000000..dbb7794dfed --- /dev/null +++ b/changelogs/unreleased/security-private-group.yml @@ -0,0 +1,6 @@ +--- +title: Removed ability to see private group names when the group id is entered in + the url. +merge_request: +author: +type: security diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index b4a731fd3a3..e2c799f5205 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -42,6 +42,16 @@ describe Dashboard::TodosController do end end + context 'group authorization' do + it 'renders 404 when user does not have read access on given group' do + unauthorized_group = create(:group, :private) + + get :index, group_id: unauthorized_group.id + + expect(response).to have_gitlab_http_status(404) + end + end + context 'when using pagination' do let(:last_page) { user.todos.page.total_pages } let!(:issues) { create_list(:issue, 3, project: project, assignees: [user]) } -- cgit v1.2.1 From 5a0a9dabd72e2d4dd79161072dd789ec0150c0c5 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 13 Nov 2018 15:15:58 +0530 Subject: No redirects in prometheus service Do not allow redirects in the prometheus service to prevent SSRFs. --- app/models/project_services/prometheus_service.rb | 2 +- spec/models/project_services/prometheus_service_spec.rb | 17 +++++++++++++++++ spec/support/helpers/prometheus_helpers.rb | 4 ++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 211e5c3fcbf..60cb2d380d5 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -71,7 +71,7 @@ class PrometheusService < MonitoringService end def prometheus_client - RestClient::Resource.new(api_url) if api_url && manual_configuration? && active? + RestClient::Resource.new(api_url, max_redirects: 0) if api_url && manual_configuration? && active? end def prometheus_available? diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index f2cb927df37..279b40e6bd7 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -13,6 +13,23 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do it { is_expected.to belong_to :project } end + context 'redirects' do + it 'does not follow redirects' do + redirect_to = 'https://redirected.example.com' + redirect_req_stub = stub_prometheus_request(prometheus_query_url('1'), status: 302, headers: {location: redirect_to}) + redirected_req_stub = stub_prometheus_request(redirect_to, body: {'status': 'success'}) + + result = service.test + + # result = { success: false, result: error } + expect(result[:success]).to be_falsy + expect(result[:result]).to be_instance_of(Gitlab::PrometheusClient::Error) + + expect(redirect_req_stub).to have_been_requested + expect(redirected_req_stub).not_to have_been_requested + end + end + describe 'Validations' do context 'when manual_configuration is enabled' do before do diff --git a/spec/support/helpers/prometheus_helpers.rb b/spec/support/helpers/prometheus_helpers.rb index 4212be2cc88..ce1f9fce10d 100644 --- a/spec/support/helpers/prometheus_helpers.rb +++ b/spec/support/helpers/prometheus_helpers.rb @@ -49,11 +49,11 @@ module PrometheusHelpers "https://prometheus.example.com/api/v1/series?#{query}" end - def stub_prometheus_request(url, body: {}, status: 200) + def stub_prometheus_request(url, body: {}, status: 200, headers: {}) WebMock.stub_request(:get, url) .to_return({ status: status, - headers: { 'Content-Type' => 'application/json' }, + headers: { 'Content-Type' => 'application/json' }.merge(headers), body: body.to_json }) end -- cgit v1.2.1 From cdcf4c3bebcfc130d248a9dc9618a075f4f34821 Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 13 Nov 2018 15:24:10 +0530 Subject: Fix rubocop missing whitespace warnings --- spec/models/project_services/prometheus_service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 279b40e6bd7..b6cf4c72450 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -16,8 +16,8 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'redirects' do it 'does not follow redirects' do redirect_to = 'https://redirected.example.com' - redirect_req_stub = stub_prometheus_request(prometheus_query_url('1'), status: 302, headers: {location: redirect_to}) - redirected_req_stub = stub_prometheus_request(redirect_to, body: {'status': 'success'}) + redirect_req_stub = stub_prometheus_request(prometheus_query_url('1'), status: 302, headers: { location: redirect_to }) + redirected_req_stub = stub_prometheus_request(redirect_to, body: { 'status': 'success' }) result = service.test -- cgit v1.2.1 From d6ea557bfd4c78a35d5bb57778fe5588e8a60c4c Mon Sep 17 00:00:00 2001 From: rpereira2 Date: Tue, 13 Nov 2018 15:32:20 +0530 Subject: Changelog entry for prometheus ssrf fix --- changelogs/unreleased/security-2736-prometheus-ssrf.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-2736-prometheus-ssrf.yml diff --git a/changelogs/unreleased/security-2736-prometheus-ssrf.yml b/changelogs/unreleased/security-2736-prometheus-ssrf.yml new file mode 100644 index 00000000000..9d0dda8a75f --- /dev/null +++ b/changelogs/unreleased/security-2736-prometheus-ssrf.yml @@ -0,0 +1,5 @@ +--- +title: Do not follow redirects in Prometheus service when making http requests to the configured api url +merge_request: +author: +type: security -- cgit v1.2.1 From 6dcfae5488acf2cb2dc631e47e5fc71b16cfea54 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 9 Nov 2018 13:58:57 -0200 Subject: Fix milestone promotion authorization Promoting milestone was missing an authorization check, guest users were being able to promote project milestones to group milestones. --- app/controllers/projects/milestones_controller.rb | 21 +++++++++++--- app/helpers/milestones_helper.rb | 13 +++++++++ app/views/shared/milestones/_milestone.html.haml | 4 +-- changelogs/unreleased/security-issue_51301.yml | 5 ++++ .../projects/milestones_controller_spec.rb | 33 ++++++++++++++++++---- .../milestones/user_promotes_milestone_spec.rb | 32 +++++++++++++++++++++ 6 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/security-issue_51301.yml create mode 100644 spec/features/milestones/user_promotes_milestone_spec.rb diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 20998c97730..8e68014a30d 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -11,7 +11,10 @@ class Projects::MilestonesController < Projects::ApplicationController before_action :authorize_read_milestone! # Allow admin milestone - before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels, :promote] + before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels] + + # Allow to promote milestone + before_action :authorize_promote_milestone!, only: :promote respond_to :html @@ -78,7 +81,7 @@ class Projects::MilestonesController < Projects::ApplicationController def promote promoted_milestone = Milestones::PromoteService.new(project, current_user).execute(milestone) - flash[:notice] = flash_notice_for(promoted_milestone, project.group) + flash[:notice] = flash_notice_for(promoted_milestone, project_group) respond_to do |format| format.html do @@ -109,6 +112,12 @@ class Projects::MilestonesController < Projects::ApplicationController protected + def project_group + strong_memoize(:project_group) do + project.group + end + end + def milestones strong_memoize(:milestones) do MilestonesFinder.new(search_params).execute @@ -125,13 +134,17 @@ class Projects::MilestonesController < Projects::ApplicationController return render_404 unless can?(current_user, :admin_milestone, @project) end + def authorize_promote_milestone! + return render_404 unless can?(current_user, :admin_milestone, project_group) + end + def milestone_params params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event) end def search_params - if request.format.json? && @project.group && can?(current_user, :read_group, @project.group) - groups = @project.group.self_and_ancestors_ids + if request.format.json? && project_group && can?(current_user, :read_group, project_group) + groups = project_group.self_and_ancestors_ids end params.permit(:state).merge(project_ids: @project.id, group_ids: groups) diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index 94a030d9d57..9666080092b 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -2,6 +2,7 @@ module MilestonesHelper include EntityDateHelper + include Gitlab::Utils::StrongMemoize def milestones_filter_path(opts = {}) if @project @@ -243,4 +244,16 @@ module MilestonesHelper dashboard_milestone_path(milestone.safe_title, title: milestone.title) end end + + def can_admin_project_milestones? + strong_memoize(:can_admin_project_milestones) do + can?(current_user, :admin_milestone, @project) + end + end + + def can_admin_group_milestones? + strong_memoize(:can_admin_group_milestones) do + can?(current_user, :admin_milestone, @project.group) + end + end end diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index 3dd2842be4f..ed7fefba56d 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -35,8 +35,8 @@ .col-sm-2 .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end - if @project - - if can?(current_user, :admin_milestone, milestone.project) and milestone.active? - - if @project.group + - if can_admin_project_milestones? and milestone.active? + - if can_admin_group_milestones? %button.js-promote-project-milestone-button.btn.btn-blank.btn-sm.btn-grouped.has-tooltip{ title: _('Promote to Group Milestone'), disabled: true, type: 'button', diff --git a/changelogs/unreleased/security-issue_51301.yml b/changelogs/unreleased/security-issue_51301.yml new file mode 100644 index 00000000000..cf8ebb54b1c --- /dev/null +++ b/changelogs/unreleased/security-issue_51301.yml @@ -0,0 +1,5 @@ +--- +title: Fix milestone promotion authorization check +merge_request: +author: +type: security diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index ccd4fc4db3a..658aa2a6738 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -143,11 +143,27 @@ describe Projects::MilestonesController do end describe '#promote' do + let(:group) { create(:group) } + + before do + project.update(namespace: group) + end + + context 'when user does not have permission to promote milestone' do + before do + group.add_guest(user) + end + + it 'renders 404' do + post :promote, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid + + expect(response).to have_gitlab_http_status(404) + end + end + context 'promotion succeeds' do before do - group = create(:group) group.add_developer(user) - milestone.project.update(namespace: group) end it 'shows group milestone' do @@ -166,12 +182,17 @@ describe Projects::MilestonesController do end end - context 'promotion fails' do - it 'shows project milestone' do + context 'when user cannot admin group milestones' do + before do + project.add_developer(user) + end + + it 'renders 404' do + project.update(namespace: user.namespace) + post :promote, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid - expect(response).to redirect_to(project_milestone_path(project, milestone)) - expect(flash[:alert]).to eq('Promotion failed - Project does not belong to a group.') + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/features/milestones/user_promotes_milestone_spec.rb b/spec/features/milestones/user_promotes_milestone_spec.rb new file mode 100644 index 00000000000..df1bc502134 --- /dev/null +++ b/spec/features/milestones/user_promotes_milestone_spec.rb @@ -0,0 +1,32 @@ +require 'rails_helper' + +describe 'User promotes milestone' do + set(:group) { create(:group) } + set(:user) { create(:user) } + set(:project) { create(:project, namespace: group) } + set(:milestone) { create(:milestone, project: project) } + + context 'when user can admin group milestones' do + before do + group.add_developer(user) + sign_in(user) + visit(project_milestones_path(project)) + end + + it "shows milestone promote button" do + expect(page).to have_selector('.js-promote-project-milestone-button') + end + end + + context 'when user cannot admin group milestones' do + before do + project.add_developer(user) + sign_in(user) + visit(project_milestones_path(project)) + end + + it "does not show milestone promote button" do + expect(page).not_to have_selector('.js-promote-project-milestone-button') + end + end +end -- cgit v1.2.1 From b82aba0e4808e7cb53ac2aee9cebb2fdda95abbc Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Fri, 9 Nov 2018 11:07:33 -0600 Subject: Sanitize output of SpacedLinkFilter --- app/models/concerns/cache_markdown_field.rb | 2 +- ...y-xss-in-markdown-following-unrecognized-html-element.yml | 5 +++++ lib/banzai/filter/spaced_link_filter.rb | 3 +++ lib/banzai/pipeline/gfm_pipeline.rb | 5 ++++- spec/lib/banzai/pipeline/gfm_pipeline_spec.rb | 12 ++++++++++++ 5 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 6e2adc76ec6..a8c9e54f00c 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -15,7 +15,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_REDCARPET_VERSION = 3 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 11 + CACHE_COMMONMARK_VERSION = 12 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze diff --git a/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml b/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml new file mode 100644 index 00000000000..3bd8123a346 --- /dev/null +++ b/changelogs/unreleased/security-xss-in-markdown-following-unrecognized-html-element.yml @@ -0,0 +1,5 @@ +--- +title: Fix possible XSS attack in Markdown urls with spaces +merge_request: 2599 +author: +type: security diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb index a27f1d46863..c6a3a763c23 100644 --- a/lib/banzai/filter/spaced_link_filter.rb +++ b/lib/banzai/filter/spaced_link_filter.rb @@ -17,6 +17,9 @@ module Banzai # This is a small extension to the CommonMark spec. If they start allowing # spaces in urls, we could then remove this filter. # + # Note: Filter::SanitizationFilter should always be run sometime after this filter + # to prevent XSS attacks + # class SpacedLinkFilter < HTML::Pipeline::Filter include ActionView::Helpers::TagHelper diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index be75e34a673..96bea7ca935 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -12,13 +12,16 @@ module Banzai def self.filters @filters ||= FilterArray[ Filter::PlantumlFilter, + + # Must always be before the SanitizationFilter to prevent XSS attacks + Filter::SpacedLinkFilter, + Filter::SanitizationFilter, Filter::SyntaxHighlightFilter, Filter::MathFilter, Filter::ColorFilter, Filter::MermaidFilter, - Filter::SpacedLinkFilter, Filter::VideoLinkFilter, Filter::ImageLazyLoadFilter, Filter::ImageLinkFilter, diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index df24cef0b8b..91b0499375d 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -104,5 +104,17 @@ describe Banzai::Pipeline::GfmPipeline do expect(output).to include("src=\"test%20image.png\"") end + + it 'sanitizes the fixed link' do + markdown_xss = "[xss](javascript: alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + + markdown_xss = "\n[xss](javascript:alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + end end end -- cgit v1.2.1 From e0fb6fcf8218a7d878a2e0d1a02dbd31734f18de Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 9 Nov 2018 13:58:50 +0100 Subject: Don't use fragment cache on commit page This makes sure the user viewing the commit does not get to see anything they're not allowed to see --- app/views/projects/commits/_commit.html.haml | 94 ++++++++++------------ .../security-bvl-exposure-in-commits-list.yml | 5 ++ .../projects/commits/user_browses_commits_spec.rb | 23 +++++- 3 files changed, 67 insertions(+), 55 deletions(-) create mode 100644 changelogs/unreleased/security-bvl-exposure-in-commits-list.yml diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index c6789e32dbe..1a74b120c26 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -8,62 +8,50 @@ - ref = local_assigns.fetch(:ref) { merge_request&.source_branch } - link = commit_path(project, commit, merge_request: merge_request) -- cache_key = [project.full_path, - ref, - commit.id, - Gitlab::CurrentSettings.current_application_settings, - @path.presence, - current_controller?(:commits), - merge_request&.iid, - view_details, - commit.status(ref), - I18n.locale].compact - -= cache(cache_key, expires_in: 1.day) do - %li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" } - - .avatar-cell.d-none.d-sm-block - = author_avatar(commit, size: 36, has_tooltip: false) - - .commit-detail.flex-list - .commit-content.qa-commit-content - - if view_details && merge_request - = link_to commit.title, project_commit_path(project, commit.id, merge_request_iid: merge_request.iid), class: "commit-row-message item-title" - - else - = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title") - %span.commit-row-message.d-block.d-sm-none - · - = commit.short_id - - if commit.status(ref) - .d-block.d-sm-none - = render_commit_status(commit, ref: ref) - - if commit.description? - %button.text-expander.js-toggle-button - = sprite_icon('ellipsis_h', size: 12) +%li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" } + + .avatar-cell.d-none.d-sm-block + = author_avatar(commit, size: 36, has_tooltip: false) + + .commit-detail.flex-list + .commit-content.qa-commit-content + - if view_details && merge_request + = link_to commit.title, project_commit_path(project, commit.id, merge_request_iid: merge_request.iid), class: "commit-row-message item-title" + - else + = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title") + %span.commit-row-message.d-block.d-sm-none + · + = commit.short_id + - if commit.status(ref) + .d-block.d-sm-none + = render_commit_status(commit, ref: ref) + - if commit.description? + %button.text-expander.js-toggle-button + = sprite_icon('ellipsis_h', size: 12) - .committer - - commit_author_link = commit_author_link(commit, avatar: false, size: 24) - - commit_timeago = time_ago_with_tooltip(commit.authored_date, placement: 'bottom') - - commit_text = _('%{commit_author_link} authored %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago } - #{ commit_text.html_safe } + .committer + - commit_author_link = commit_author_link(commit, avatar: false, size: 24) + - commit_timeago = time_ago_with_tooltip(commit.authored_date, placement: 'bottom') + - commit_text = _('%{commit_author_link} authored %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago } + #{ commit_text.html_safe } - - if commit.description? - %pre.commit-row-description.js-toggle-content.append-bottom-8 - = preserve(markdown_field(commit, :description)) + - if commit.description? + %pre.commit-row-description.js-toggle-content.append-bottom-8 + = preserve(markdown_field(commit, :description)) - .commit-actions.flex-row.d-none.d-sm-flex - - if request.xhr? - = render partial: 'projects/commit/signature', object: commit.signature - - else - = render partial: 'projects/commit/ajax_signature', locals: { commit: commit } + .commit-actions.flex-row.d-none.d-sm-flex + - if request.xhr? + = render partial: 'projects/commit/signature', object: commit.signature + - else + = render partial: 'projects/commit/ajax_signature', locals: { commit: commit } - - if commit.status(ref) - = render_commit_status(commit, ref: ref) + - if commit.status(ref) + = render_commit_status(commit, ref: ref) - .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } } + .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } } - .commit-sha-group - .label.label-monospace - = commit.short_id - = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"), class: "btn btn-default", container: "body") - = link_to_browse_code(project, commit) + .commit-sha-group + .label.label-monospace + = commit.short_id + = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"), class: "btn btn-default", container: "body") + = link_to_browse_code(project, commit) diff --git a/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml b/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml new file mode 100644 index 00000000000..0361fb0c041 --- /dev/null +++ b/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml @@ -0,0 +1,5 @@ +--- +title: Don't expose confidential information in commit message list +merge_request: +author: +type: security diff --git a/spec/features/projects/commits/user_browses_commits_spec.rb b/spec/features/projects/commits/user_browses_commits_spec.rb index 534cfe1eb12..2159adf49fc 100644 --- a/spec/features/projects/commits/user_browses_commits_spec.rb +++ b/spec/features/projects/commits/user_browses_commits_spec.rb @@ -4,10 +4,9 @@ describe 'User browses commits' do include RepoHelpers let(:user) { create(:user) } - let(:project) { create(:project, :repository, namespace: user.namespace) } + let(:project) { create(:project, :public, :repository, namespace: user.namespace) } before do - project.add_maintainer(user) sign_in(user) end @@ -127,6 +126,26 @@ describe 'User browses commits' do .and have_selector('entry summary', text: commit.description[0..10].delete("\r\n")) end + context 'when a commit links to a confidential issue' do + let(:confidential_issue) { create(:issue, confidential: true, title: 'Secret issue!', project: project) } + + before do + project.repository.create_file(user, 'dummy-file', 'dummy content', + branch_name: 'feature', + message: "Linking #{confidential_issue.to_reference}") + end + + context 'when the user cannot see confidential issues but was cached with a link', :use_clean_rails_memory_store_fragment_caching do + it 'does not render the confidential issue' do + visit project_commits_path(project, 'feature') + sign_in(create(:user)) + visit project_commits_path(project, 'feature') + + expect(page).not_to have_link(href: project_issue_path(project, confidential_issue)) + end + end + end + context 'master branch' do before do visit_commits_page -- cgit v1.2.1 From bfe02a556823235e4a7b0eebf7171f34d7a3d475 Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Mon, 19 Nov 2018 12:23:52 +0100 Subject: Upgrade GitLab Pages to v1.3.1 --- GITLAB_PAGES_VERSION | 2 +- changelogs/unreleased/security-pages-toctou-race.yml | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-pages-toctou-race.yml diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index f0bb29e7638..3a3cd8cc8b0 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -1.3.0 +1.3.1 diff --git a/changelogs/unreleased/security-pages-toctou-race.yml b/changelogs/unreleased/security-pages-toctou-race.yml new file mode 100644 index 00000000000..1c055f6087f --- /dev/null +++ b/changelogs/unreleased/security-pages-toctou-race.yml @@ -0,0 +1,6 @@ +--- +title: Fix a possible symlink time of check to time of use race condition in GitLab + Pages +merge_request: +author: +type: security -- cgit v1.2.1 From 759c52961bf8d05ce3f651c81a6b8c3009932885 Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Thu, 8 Nov 2018 09:06:07 +0000 Subject: Validate URI scheme also for internal URI Gitlab::UrlBlocker ignores scheme when validating URI matching either config.gitlab or config.gitlab_shell This patch enforces matching config.gitlab.protocol for internal web and ssh for internal shell. --- .../unreleased/security-stored-xss-for-environments.yml | 5 +++++ lib/gitlab/url_blocker.rb | 6 ++++-- spec/factories/ci/builds.rb | 2 +- spec/lib/gitlab/url_blocker_spec.rb | 12 ++++++++++-- 4 files changed, 20 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/security-stored-xss-for-environments.yml diff --git a/changelogs/unreleased/security-stored-xss-for-environments.yml b/changelogs/unreleased/security-stored-xss-for-environments.yml new file mode 100644 index 00000000000..5d78ca00942 --- /dev/null +++ b/changelogs/unreleased/security-stored-xss-for-environments.yml @@ -0,0 +1,5 @@ +--- +title: Fix stored XSS for Environments +merge_request: +author: +type: security diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 86efe8ad114..4b1b58d68d8 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -111,12 +111,14 @@ module Gitlab end def internal_web?(uri) - uri.hostname == config.gitlab.host && + uri.scheme == config.gitlab.protocol && + uri.hostname == config.gitlab.host && (uri.port.blank? || uri.port == config.gitlab.port) end def internal_shell?(uri) - uri.hostname == config.gitlab_shell.ssh_host && + uri.scheme == 'ssh' && + uri.hostname == config.gitlab_shell.ssh_host && (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port) end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 90754319f05..07c1fc31152 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -308,7 +308,7 @@ FactoryBot.define do trait :with_runner_session do after(:build) do |build| - build.build_runner_session(url: 'ws://localhost') + build.build_runner_session(url: 'https://localhost') end end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 8df0facdab3..35b550283b5 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -10,8 +10,8 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?(import_url)).to be false end - it 'allows imports from configured SSH host and port' do - import_url = "http://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git" + it 'allows mirroring from configured SSH host and port' do + import_url = "ssh://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git" expect(described_class.blocked_url?(import_url)).to be false end @@ -29,6 +29,14 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true end + it 'returns true for bad protocol on configured web/SSH host and ports' do + web_url = "javascript://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git%0aalert(1)" + expect(described_class.blocked_url?(web_url)).to be true + + ssh_url = "javascript://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git%0aalert(1)" + expect(described_class.blocked_url?(ssh_url)).to be true + end + it 'returns true for localhost IPs' do expect(described_class.blocked_url?('https://0.0.0.0/foo/foo.git')).to be true expect(described_class.blocked_url?('https://[::1]/foo/foo.git')).to be true -- cgit v1.2.1 From 2f89c77e238f017a9b05e7b0f118928566757cef Mon Sep 17 00:00:00 2001 From: Alessio Caiazza Date: Thu, 8 Nov 2018 10:12:26 +0000 Subject: Cleanup stored XSS from environments table --- ...1108091549_cleanup_environments_external_url.rb | 18 ++++++++++++++ .../cleanup_environments_external_url_spec.rb | 28 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 db/migrate/20181108091549_cleanup_environments_external_url.rb create mode 100644 spec/migrations/cleanup_environments_external_url_spec.rb diff --git a/db/migrate/20181108091549_cleanup_environments_external_url.rb b/db/migrate/20181108091549_cleanup_environments_external_url.rb new file mode 100644 index 00000000000..8d6c20a4b15 --- /dev/null +++ b/db/migrate/20181108091549_cleanup_environments_external_url.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class CleanupEnvironmentsExternalUrl < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + update_column_in_batches(:environments, :external_url, nil) do |table, query| + query.where(table[:external_url].matches('javascript://%')) + end + end + + def down + end +end diff --git a/spec/migrations/cleanup_environments_external_url_spec.rb b/spec/migrations/cleanup_environments_external_url_spec.rb new file mode 100644 index 00000000000..07ddaf3d38f --- /dev/null +++ b/spec/migrations/cleanup_environments_external_url_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20181108091549_cleanup_environments_external_url.rb') + +describe CleanupEnvironmentsExternalUrl, :migration do + let(:environments) { table(:environments) } + let(:invalid_entries) { environments.where(environments.arel_table[:external_url].matches('javascript://%')) } + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + + before do + namespace = namespaces.create(name: 'foo', path: 'foo') + project = projects.create!(namespace_id: namespace.id) + + environments.create!(id: 1, project_id: project.id, name: 'poisoned', slug: 'poisoned', external_url: 'javascript://alert("1")') + end + + it 'clears every environment with a javascript external_url' do + expect do + subject.up + end.to change { invalid_entries.count }.from(1).to(0) + end + + it 'do not removes environments' do + expect do + subject.up + end.not_to change { environments.count } + end +end -- cgit v1.2.1 From bd0bfdd1476ca8b44ca41a1d6c4f8c830dc59503 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 31 Oct 2018 11:28:33 +0800 Subject: Redact sensitive information on workhorse log --- GITLAB_WORKHORSE_VERSION | 2 +- changelogs/unreleased/security-182-update-workhorse.yml | 5 +++++ config/application.rb | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-182-update-workhorse.yml diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 21c8c7b46b8..1996c504476 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -7.1.1 +7.1.3 diff --git a/changelogs/unreleased/security-182-update-workhorse.yml b/changelogs/unreleased/security-182-update-workhorse.yml new file mode 100644 index 00000000000..76850901b68 --- /dev/null +++ b/changelogs/unreleased/security-182-update-workhorse.yml @@ -0,0 +1,5 @@ +--- +title: Redact sensitive information on gitlab-workhorse log +merge_request: +author: +type: security diff --git a/config/application.rb b/config/application.rb index 796aa85e113..cdc50372011 100644 --- a/config/application.rb +++ b/config/application.rb @@ -97,6 +97,9 @@ module Gitlab # - Webhook URLs (:hook) # - Sentry DSN (:sentry_dsn) # - File content from Web Editor (:content) + # + # NOTE: It is **IMPORTANT** to also update gitlab-workhorse's filter when adding parameters here to not + # introduce another security vulnerability: https://gitlab.com/gitlab-org/gitlab-workhorse/issues/182 config.filter_parameters += [/token$/, /password/, /secret/, /key$/] config.filter_parameters += %i( certificate -- cgit v1.2.1 From 01afaf852b11387829b4dc90dbd90ba1311b7a27 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 30 Oct 2018 12:56:34 +0100 Subject: Update code to use API scope on PAT auth --- app/controllers/application_controller.rb | 23 +--- .../concerns/sessionless_authentication.rb | 28 ++++ app/controllers/dashboard/projects_controller.rb | 1 + app/controllers/dashboard_controller.rb | 3 + app/controllers/graphql_controller.rb | 1 + app/controllers/groups_controller.rb | 3 + app/controllers/projects/commits_controller.rb | 1 + app/controllers/projects/issues_controller.rb | 17 +-- app/controllers/projects/tags_controller.rb | 2 + app/controllers/projects_controller.rb | 2 + app/controllers/users_controller.rb | 1 + .../unreleased/security-fix-pat-web-access.yml | 5 + config/initializers/rack_attack_global.rb | 10 +- lib/gitlab/auth/request_authenticator.rb | 14 +- lib/gitlab/auth/user_auth_finders.rb | 39 +++++- spec/controllers/application_controller_spec.rb | 151 --------------------- .../dashboard/projects_controller_spec.rb | 5 + spec/controllers/dashboard_controller_spec.rb | 31 +++-- spec/controllers/graphql_controller_spec.rb | 47 ++++++- spec/controllers/groups_controller_spec.rb | 20 +++ .../projects/commits_controller_spec.rb | 138 +++++++++++-------- .../controllers/projects/issues_controller_spec.rb | 36 +++++ spec/controllers/projects/tags_controller_spec.rb | 22 +++ spec/controllers/projects_controller_spec.rb | 22 +++ spec/controllers/users_controller_spec.rb | 8 ++ spec/lib/gitlab/auth/request_authenticator_spec.rb | 18 +-- spec/lib/gitlab/auth/user_auth_finders_spec.rb | 79 +++++++++-- .../sessionless_auth_controller_shared_examples.rb | 92 +++++++++++++ 28 files changed, 538 insertions(+), 281 deletions(-) create mode 100644 app/controllers/concerns/sessionless_authentication.rb create mode 100644 changelogs/unreleased/security-fix-pat-web-access.yml create mode 100644 spec/controllers/dashboard/projects_controller_spec.rb create mode 100644 spec/support/controllers/sessionless_auth_controller_shared_examples.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9b40ffb26a2..dbb22127e82 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -12,11 +12,11 @@ class ApplicationController < ActionController::Base include WorkhorseHelper include EnforcesTwoFactorAuthentication include WithPerformanceBar + include SessionlessAuthentication # this can be removed after switching to rails 5 # https://gitlab.com/gitlab-org/gitlab-ce/issues/51908 include InvalidUTF8ErrorHandler unless Gitlab.rails5? - before_action :authenticate_sessionless_user! before_action :authenticate_user! before_action :enforce_terms!, if: :should_enforce_terms? before_action :validate_user_service_ticket! @@ -153,13 +153,6 @@ class ApplicationController < ActionController::Base end end - # This filter handles personal access tokens, and atom requests with rss tokens - def authenticate_sessionless_user! - user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user - - sessionless_sign_in(user) if user - end - def log_exception(exception) Raven.capture_exception(exception) if sentry_enabled? @@ -426,25 +419,11 @@ class ApplicationController < ActionController::Base Gitlab::I18n.with_user_locale(current_user, &block) end - def sessionless_sign_in(user) - if user && can?(user, :log_in) - # Notice we are passing store false, so the user is not - # actually stored in the session and a token is needed - # for every request. If you want the token to work as a - # sign in token, you can simply remove store: false. - sign_in(user, store: false, message: :sessionless_sign_in) - end - end - def set_page_title_header # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8 response.headers['Page-Title'] = URI.escape(page_title('GitLab')) end - def sessionless_user? - current_user && !session.keys.include?('warden.user.user.key') - end - def peek_request? request.path.start_with?('/-/peek') end diff --git a/app/controllers/concerns/sessionless_authentication.rb b/app/controllers/concerns/sessionless_authentication.rb new file mode 100644 index 00000000000..590eefc6dab --- /dev/null +++ b/app/controllers/concerns/sessionless_authentication.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# == SessionlessAuthentication +# +# Controller concern to handle PAT and RSS token authentication methods +# +module SessionlessAuthentication + # This filter handles personal access tokens, and atom requests with rss tokens + def authenticate_sessionless_user!(request_format) + user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format) + + sessionless_sign_in(user) if user + end + + def sessionless_user? + current_user && !session.keys.include?('warden.user.user.key') + end + + def sessionless_sign_in(user) + if user && can?(user, :log_in) + # Notice we are passing store false, so the user is not + # actually stored in the session and a token is needed + # for every request. If you want the token to work as a + # sign in token, you can simply remove store: false. + sign_in(user, store: false, message: :sessionless_sign_in) + end + end +end diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index e9686ed8d06..57e612d89d3 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -4,6 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController include ParamsBackwardCompatibility include RendersMemberAccess + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } before_action :set_non_archived_param before_action :default_sorting skip_cross_project_access_check :index, :starred diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 4ce9be44403..be2d9512c01 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -4,6 +4,9 @@ class DashboardController < Dashboard::ApplicationController include IssuesAction include MergeRequestsAction + prepend_before_action(only: [:issues]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) } + before_action :event_filter, only: :activity before_action :projects, only: [:issues, :merge_requests] before_action :set_show_full_reference, only: [:issues, :merge_requests] diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index a1ec144410b..6ea4758ec32 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -3,6 +3,7 @@ class GraphqlController < ApplicationController # Unauthenticated users have access to the API for public data skip_before_action :authenticate_user! + prepend_before_action(only: [:execute]) { authenticate_sessionless_user!(:api) } before_action :check_graphql_feature_flag! diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 062c8c4e9e1..c5d8ac2ed77 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -9,6 +9,9 @@ class GroupsController < Groups::ApplicationController respond_to :html + prepend_before_action(only: [:show, :issues]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) } + before_action :authenticate_user!, only: [:new, :create] before_action :group, except: [:index, :new, :create] diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 84a2a461da7..8ba18aacc58 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -6,6 +6,7 @@ class Projects::CommitsController < Projects::ApplicationController include ExtractsPath include RendersCommits + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } before_action :whitelist_query_limiting, except: :commits_root before_action :require_non_empty_project before_action :assign_ref_vars, except: :commits_root diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 308f666394c..55d03826b7e 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -9,10 +9,6 @@ class Projects::IssuesController < Projects::ApplicationController include IssuesCalendar include SpammableActions - def self.authenticate_user_only_actions - %i[new] - end - def self.issue_except_actions %i[index calendar new create bulk_update] end @@ -21,7 +17,10 @@ class Projects::IssuesController < Projects::ApplicationController %i[index calendar] end - prepend_before_action :authenticate_user!, only: authenticate_user_only_actions + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } + prepend_before_action(only: [:calendar]) { authenticate_sessionless_user!(:ics) } + prepend_before_action :authenticate_new_issue!, only: [:new] + prepend_before_action :store_uri, only: [:new, :show] before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] before_action :check_issues_available! @@ -230,16 +229,18 @@ class Projects::IssuesController < Projects::ApplicationController ] + [{ label_ids: [], assignee_ids: [] }] end - def authenticate_user! + def authenticate_new_issue! return if current_user notice = "Please sign in to create the new issue." + redirect_to new_user_session_path, notice: notice + end + + def store_uri if request.get? && !request.xhr? store_location_for :user, request.fullpath end - - redirect_to new_user_session_path, notice: notice end def serializer diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index c8442ff3592..2b28670a49b 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -3,6 +3,8 @@ class Projects::TagsController < Projects::ApplicationController include SortingHelper + prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } + # Authorize before_action :require_non_empty_project before_action :authorize_download_code! diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 7f4a9f5151b..8bf93bfd68d 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -7,6 +7,8 @@ class ProjectsController < Projects::ApplicationController include PreviewMarkdown include SendFileUpload + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } + before_action :whitelist_query_limiting, only: [:create] before_action :authenticate_user!, except: [:index, :show, :activity, :refs] before_action :redirect_git_extension, only: [:show] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5b70c69d7f4..8b040dc080e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -14,6 +14,7 @@ class UsersController < ApplicationController calendar_activities: true skip_before_action :authenticate_user! + prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) } before_action :user, except: [:exists] before_action :authorize_read_user_profile!, only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :snippets] diff --git a/changelogs/unreleased/security-fix-pat-web-access.yml b/changelogs/unreleased/security-fix-pat-web-access.yml new file mode 100644 index 00000000000..62ffb908fe5 --- /dev/null +++ b/changelogs/unreleased/security-fix-pat-web-access.yml @@ -0,0 +1,5 @@ +--- +title: Restrict Personal Access Tokens to API scope on web requests +merge_request: +author: +type: security diff --git a/config/initializers/rack_attack_global.rb b/config/initializers/rack_attack_global.rb index 45963831c41..86cb930eca9 100644 --- a/config/initializers/rack_attack_global.rb +++ b/config/initializers/rack_attack_global.rb @@ -33,22 +33,22 @@ class Rack::Attack throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req| Gitlab::Throttle.settings.throttle_authenticated_api_enabled && req.api_request? && - req.authenticated_user_id + req.authenticated_user_id([:api]) end throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req| Gitlab::Throttle.settings.throttle_authenticated_web_enabled && req.web_request? && - req.authenticated_user_id + req.authenticated_user_id([:api, :rss, :ics]) end class Request def unauthenticated? - !authenticated_user_id + !authenticated_user_id([:api, :rss, :ics]) end - def authenticated_user_id - Gitlab::Auth::RequestAuthenticator.new(self).user&.id + def authenticated_user_id(request_formats) + Gitlab::Auth::RequestAuthenticator.new(self).user(request_formats)&.id end def api_request? diff --git a/lib/gitlab/auth/request_authenticator.rb b/lib/gitlab/auth/request_authenticator.rb index cb9f2582936..176766d1a8b 100644 --- a/lib/gitlab/auth/request_authenticator.rb +++ b/lib/gitlab/auth/request_authenticator.rb @@ -13,12 +13,18 @@ module Gitlab @request = request end - def user - find_sessionless_user || find_user_from_warden + def user(request_formats) + request_formats.each do |format| + user = find_sessionless_user(format) + + return user if user + end + + find_user_from_warden end - def find_sessionless_user - find_user_from_access_token || find_user_from_feed_token + def find_sessionless_user(request_format) + find_user_from_web_access_token(request_format) || find_user_from_feed_token(request_format) rescue Gitlab::Auth::AuthenticationError nil end diff --git a/lib/gitlab/auth/user_auth_finders.rb b/lib/gitlab/auth/user_auth_finders.rb index c304adc64db..adba9084845 100644 --- a/lib/gitlab/auth/user_auth_finders.rb +++ b/lib/gitlab/auth/user_auth_finders.rb @@ -27,8 +27,8 @@ module Gitlab current_request.env['warden']&.authenticate if verified_request? end - def find_user_from_feed_token - return unless rss_request? || ics_request? + def find_user_from_feed_token(request_format) + return unless valid_rss_format?(request_format) # NOTE: feed_token was renamed from rss_token but both needs to be supported because # users might have already added the feed to their RSS reader before the rename @@ -38,6 +38,17 @@ module Gitlab User.find_by_feed_token(token) || raise(UnauthorizedError) end + # We only allow Private Access Tokens with `api` scope to be used by web + # requests on RSS feeds or ICS files for backwards compatibility. + # It is also used by GraphQL/API requests. + def find_user_from_web_access_token(request_format) + return unless access_token && valid_web_access_format?(request_format) + + validate_access_token!(scopes: [:api]) + + access_token.user || raise(UnauthorizedError) + end + def find_user_from_access_token return unless access_token @@ -109,6 +120,26 @@ module Gitlab @current_request ||= ensure_action_dispatch_request(request) end + def valid_web_access_format?(request_format) + case request_format + when :rss + rss_request? + when :ics + ics_request? + when :api + api_request? + end + end + + def valid_rss_format?(request_format) + case request_format + when :rss + rss_request? + when :ics + ics_request? + end + end + def rss_request? current_request.path.ends_with?('.atom') || current_request.format.atom? end @@ -116,6 +147,10 @@ module Gitlab def ics_request? current_request.path.ends_with?('.ics') || current_request.format.ics? end + + def api_request? + current_request.path.starts_with?("/api/") + end end end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index efc3ce74627..1b585bcd4c6 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -107,59 +107,6 @@ describe ApplicationController do end end - describe "#authenticate_user_from_personal_access_token!" do - before do - stub_authentication_activity_metrics(debug: false) - end - - controller(described_class) do - def index - render text: 'authenticated' - end - end - - let(:personal_access_token) { create(:personal_access_token, user: user) } - - context "when the 'personal_access_token' param is populated with the personal access token" do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - get :index, private_token: personal_access_token.token - - expect(response).to have_gitlab_http_status(200) - expect(response.body).to eq('authenticated') - end - end - - context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - @request.headers["PRIVATE-TOKEN"] = personal_access_token.token - get :index - - expect(response).to have_gitlab_http_status(200) - expect(response.body).to eq('authenticated') - end - end - - it "doesn't log the user in otherwise" do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, private_token: "token" - - expect(response.status).not_to eq(200) - expect(response.body).not_to eq('authenticated') - end - end - describe 'session expiration' do controller(described_class) do # The anonymous controller will report 401 and fail to run any actions. @@ -224,74 +171,6 @@ describe ApplicationController do end end - describe '#authenticate_sessionless_user!' do - before do - stub_authentication_activity_metrics(debug: false) - end - - describe 'authenticating a user from a feed token' do - controller(described_class) do - def index - render text: 'authenticated' - end - end - - context "when the 'feed_token' param is populated with the feed token" do - context 'when the request format is atom' do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status 200 - expect(response.body).to eq 'authenticated' - end - end - - context 'when the request format is ics' do - it "logs the user in" do - expect(authentication_metrics) - .to increment(:user_authenticated_counter) - .and increment(:user_session_override_counter) - .and increment(:user_sessionless_authentication_counter) - - get :index, feed_token: user.feed_token, format: :ics - - expect(response).to have_gitlab_http_status 200 - expect(response.body).to eq 'authenticated' - end - end - - context 'when the request format is neither atom nor ics' do - it "doesn't log the user in" do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, feed_token: user.feed_token - - expect(response.status).not_to have_gitlab_http_status 200 - expect(response.body).not_to eq 'authenticated' - end - end - end - - context "when the 'feed_token' param is populated with an invalid feed token" do - it "doesn't log the user" do - expect(authentication_metrics) - .to increment(:user_unauthenticated_counter) - - get :index, feed_token: 'token', format: :atom - - expect(response.status).not_to eq 200 - expect(response.body).not_to eq 'authenticated' - end - end - end - end - describe '#route_not_found' do it 'renders 404 if authenticated' do allow(controller).to receive(:current_user).and_return(user) @@ -557,36 +436,6 @@ describe ApplicationController do expect(response).to have_gitlab_http_status(200) end - - context 'for sessionless users' do - render_views - - before do - sign_out user - end - - it 'renders a 403 when the sessionless user did not accept the terms' do - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status(403) - end - - it 'renders the error message when the format was html' do - get :index, - private_token: create(:personal_access_token, user: user).token, - format: :html - - expect(response.body).to have_content /accept the terms of service/i - end - - it 'renders a 200 when the sessionless user accepted the terms' do - accept_terms(user) - - get :index, feed_token: user.feed_token, format: :atom - - expect(response).to have_gitlab_http_status(200) - end - end end end diff --git a/spec/controllers/dashboard/projects_controller_spec.rb b/spec/controllers/dashboard/projects_controller_spec.rb new file mode 100644 index 00000000000..2975205e09c --- /dev/null +++ b/spec/controllers/dashboard/projects_controller_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Dashboard::ProjectsController do + it_behaves_like 'authenticates sessionless user', :index, :atom +end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index 187542ba30c..c857a78d5e8 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -1,21 +1,26 @@ require 'spec_helper' describe DashboardController do - let(:user) { create(:user) } - let(:project) { create(:project) } + context 'signed in' do + let(:user) { create(:user) } + let(:project) { create(:project) } - before do - project.add_maintainer(user) - sign_in(user) - end + before do + project.add_maintainer(user) + sign_in(user) + end - describe 'GET issues' do - it_behaves_like 'issuables list meta-data', :issue, :issues - it_behaves_like 'issuables requiring filter', :issues - end + describe 'GET issues' do + it_behaves_like 'issuables list meta-data', :issue, :issues + it_behaves_like 'issuables requiring filter', :issues + end - describe 'GET merge requests' do - it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests - it_behaves_like 'issuables requiring filter', :merge_requests + describe 'GET merge requests' do + it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests + it_behaves_like 'issuables requiring filter', :merge_requests + end end + + it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first + it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics end diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index 1449036e148..949ad532365 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -52,15 +52,58 @@ describe GraphqlController do end end + context 'token authentication' do + before do + stub_authentication_activity_metrics(debug: false) + end + + let(:user) { create(:user, username: 'Simon') } + let(:personal_access_token) { create(:personal_access_token, user: user) } + + context "when the 'personal_access_token' param is populated with the personal access token" do + it 'logs the user in' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + run_test_query!(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + expect(query_response).to eq('echo' => '"Simon" says: test success') + end + end + + context 'when the personal access token has no api scope' do + it 'does not log the user in' do + personal_access_token.update(scopes: [:read_user]) + + run_test_query!(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + + expect(query_response).to eq('echo' => 'nil says: test success') + end + end + + context 'without token' do + it 'shows public data' do + run_test_query! + + expect(query_response).to eq('echo' => 'nil says: test success') + end + end + end + # Chosen to exercise all the moving parts in GraphqlController#execute - def run_test_query!(variables: { 'text' => 'test success' }) + def run_test_query!(variables: { 'text' => 'test success' }, private_token: nil) query = <<~QUERY query Echo($text: String) { echo(text: $text) } QUERY - post :execute, query: query, operationName: 'Echo', variables: variables + post :execute, query: query, operationName: 'Echo', variables: variables, private_token: private_token end def query_response diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 4de61b65f71..f6c85102830 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -606,4 +606,24 @@ describe GroupsController do end end end + + context 'token authentication' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(id: group) + end + end + + it_behaves_like 'authenticates sessionless user', :issues, :atom, public: true do + before do + default_params.merge!(id: group, author_id: user.id) + end + end + + it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics, public: true do + before do + default_params.merge!(id: group) + end + end + end end diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb index a43bdd3ea80..5c72dab698c 100644 --- a/spec/controllers/projects/commits_controller_spec.rb +++ b/spec/controllers/projects/commits_controller_spec.rb @@ -5,87 +5,115 @@ describe Projects::CommitsController do let(:user) { create(:user) } before do - sign_in(user) project.add_maintainer(user) end - describe "GET commits_root" do - context "no ref is provided" do - it 'should redirect to the default branch of the project' do - get(:commits_root, - namespace_id: project.namespace, - project_id: project) + context 'signed in' do + before do + sign_in(user) + end + + describe "GET commits_root" do + context "no ref is provided" do + it 'should redirect to the default branch of the project' do + get(:commits_root, + namespace_id: project.namespace, + project_id: project) - expect(response).to redirect_to project_commits_path(project) + expect(response).to redirect_to project_commits_path(project) + end end end - end - describe "GET show" do - render_views + describe "GET show" do + render_views - context 'with file path' do - before do - get(:show, - namespace_id: project.namespace, - project_id: project, - id: id) - end + context 'with file path' do + before do + get(:show, + namespace_id: project.namespace, + project_id: project, + id: id) + end - context "valid branch, valid file" do - let(:id) { 'master/README.md' } + context "valid branch, valid file" do + let(:id) { 'master/README.md' } - it { is_expected.to respond_with(:success) } - end + it { is_expected.to respond_with(:success) } + end - context "valid branch, invalid file" do - let(:id) { 'master/invalid-path.rb' } + context "valid branch, invalid file" do + let(:id) { 'master/invalid-path.rb' } - it { is_expected.to respond_with(:not_found) } - end + it { is_expected.to respond_with(:not_found) } + end - context "invalid branch, valid file" do - let(:id) { 'invalid-branch/README.md' } + context "invalid branch, valid file" do + let(:id) { 'invalid-branch/README.md' } - it { is_expected.to respond_with(:not_found) } + it { is_expected.to respond_with(:not_found) } + end end - end - context "when the ref name ends in .atom" do - context "when the ref does not exist with the suffix" do - before do - get(:show, - namespace_id: project.namespace, - project_id: project, - id: "master.atom") + context "when the ref name ends in .atom" do + context "when the ref does not exist with the suffix" do + before do + get(:show, + namespace_id: project.namespace, + project_id: project, + id: "master.atom") + end + + it "renders as atom" do + expect(response).to be_success + expect(response.content_type).to eq('application/atom+xml') + end + + it 'renders summary with type=html' do + expect(response.body).to include('') + end end - it "renders as atom" do - expect(response).to be_success - expect(response.content_type).to eq('application/atom+xml') - end + context "when the ref exists with the suffix" do + before do + commit = project.repository.commit('master') - it 'renders summary with type=html' do - expect(response.body).to include('') + allow_any_instance_of(Repository).to receive(:commit).and_call_original + allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit) + + get(:show, + namespace_id: project.namespace, + project_id: project, + id: "master.atom") + end + + it "renders as HTML" do + expect(response).to be_success + expect(response.content_type).to eq('text/html') + end end end + end + end - context "when the ref exists with the suffix" do + context 'token authentication' do + context 'public project' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do before do - commit = project.repository.commit('master') + public_project = create(:project, :repository, :public) - allow_any_instance_of(Repository).to receive(:commit).and_call_original - allow_any_instance_of(Repository).to receive(:commit).with('master.atom').and_return(commit) - - get(:show, - namespace_id: project.namespace, - project_id: project, - id: "master.atom") + default_params.merge!(namespace_id: public_project.namespace, project_id: public_project, id: "master.atom") end + end + end + + context 'private project' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: false do + before do + private_project = create(:project, :repository, :private) + private_project.add_maintainer(user) - it "renders as HTML" do - expect(response).to be_success - expect(response.content_type).to eq('text/html') + default_params.merge!(namespace_id: private_project.namespace, project_id: private_project, id: "master.atom") end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 80138183c07..02930edbf72 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1068,4 +1068,40 @@ describe Projects::IssuesController do end end end + + context 'private project with token authentication' do + let(:private_project) { create(:project, :private) } + + it_behaves_like 'authenticates sessionless user', :index, :atom do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + + it_behaves_like 'authenticates sessionless user', :calendar, :ics do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :public) } + + it_behaves_like 'authenticates sessionless user', :index, :atom, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + + it_behaves_like 'authenticates sessionless user', :calendar, :ics, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + end end diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index c48f41ca12e..6fbf75d0259 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/spec/controllers/projects/tags_controller_spec.rb @@ -35,4 +35,26 @@ describe Projects::TagsController do it { is_expected.to respond_with(:not_found) } end end + + context 'private project with token authentication' do + let(:private_project) { create(:project, :repository, :private) } + + it_behaves_like 'authenticates sessionless user', :index, :atom do + before do + default_params.merge!(project_id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :repository, :public) } + + it_behaves_like 'authenticates sessionless user', :index, :atom, public: true do + before do + default_params.merge!(project_id: public_project, namespace_id: public_project.namespace) + end + end + end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 3bc9cbe64c5..7849bec4762 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -882,6 +882,28 @@ describe ProjectsController do end end + context 'private project with token authentication' do + let(:private_project) { create(:project, :private) } + + it_behaves_like 'authenticates sessionless user', :show, :atom do + before do + default_params.merge!(id: private_project, namespace_id: private_project.namespace) + + private_project.add_maintainer(user) + end + end + end + + context 'public project with token authentication' do + let(:public_project) { create(:project, :public) } + + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(id: public_project, namespace_id: public_project.namespace) + end + end + end + def project_moved_message(redirect_route, project) "Project '#{redirect_route.path}' was moved to '#{project.full_path}'. Please update any links and bookmarks that may still have the old path." end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 071f96a729e..fe438e71e9e 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -395,6 +395,14 @@ describe UsersController do end end + context 'token authentication' do + it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do + before do + default_params.merge!(username: user.username) + end + end + end + def user_moved_message(redirect_route, user) "User '#{redirect_route.path}' was moved to '#{user.full_path}'. Please update any links and bookmarks that may still have the old path." end diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb index 242ab4a91dd..3d979132880 100644 --- a/spec/lib/gitlab/auth/request_authenticator_spec.rb +++ b/spec/lib/gitlab/auth/request_authenticator_spec.rb @@ -19,17 +19,17 @@ describe Gitlab::Auth::RequestAuthenticator do allow_any_instance_of(described_class).to receive(:find_sessionless_user).and_return(sessionless_user) allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_return(session_user) - expect(subject.user).to eq sessionless_user + expect(subject.user([:api])).to eq sessionless_user end it 'returns session user if no sessionless user found' do allow_any_instance_of(described_class).to receive(:find_user_from_warden).and_return(session_user) - expect(subject.user).to eq session_user + expect(subject.user([:api])).to eq session_user end it 'returns nil if no user found' do - expect(subject.user).to be_blank + expect(subject.user([:api])).to be_blank end it 'bubbles up exceptions' do @@ -42,26 +42,26 @@ describe Gitlab::Auth::RequestAuthenticator do let!(:feed_token_user) { build(:user) } it 'returns access_token user first' do - allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_return(access_token_user) + allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_return(access_token_user) allow_any_instance_of(described_class).to receive(:find_user_from_feed_token).and_return(feed_token_user) - expect(subject.find_sessionless_user).to eq access_token_user + expect(subject.find_sessionless_user([:api])).to eq access_token_user end it 'returns feed_token user if no access_token user found' do allow_any_instance_of(described_class).to receive(:find_user_from_feed_token).and_return(feed_token_user) - expect(subject.find_sessionless_user).to eq feed_token_user + expect(subject.find_sessionless_user([:api])).to eq feed_token_user end it 'returns nil if no user found' do - expect(subject.find_sessionless_user).to be_blank + expect(subject.find_sessionless_user([:api])).to be_blank end it 'rescue Gitlab::Auth::AuthenticationError exceptions' do - allow_any_instance_of(described_class).to receive(:find_user_from_access_token).and_raise(Gitlab::Auth::UnauthorizedError) + allow_any_instance_of(described_class).to receive(:find_user_from_web_access_token).and_raise(Gitlab::Auth::UnauthorizedError) - expect(subject.find_sessionless_user).to be_blank + expect(subject.find_sessionless_user([:api])).to be_blank end end end diff --git a/spec/lib/gitlab/auth/user_auth_finders_spec.rb b/spec/lib/gitlab/auth/user_auth_finders_spec.rb index 454ad1589b9..5d3fbba264f 100644 --- a/spec/lib/gitlab/auth/user_auth_finders_spec.rb +++ b/spec/lib/gitlab/auth/user_auth_finders_spec.rb @@ -9,7 +9,7 @@ describe Gitlab::Auth::UserAuthFinders do 'rack.input' => '' } end - let(:request) { Rack::Request.new(env)} + let(:request) { Rack::Request.new(env) } def set_param(key, value) request.update_param(key, value) @@ -49,6 +49,7 @@ describe Gitlab::Auth::UserAuthFinders do describe '#find_user_from_feed_token' do context 'when the request format is atom' do before do + env['SCRIPT_NAME'] = 'url.atom' env['HTTP_ACCEPT'] = 'application/atom+xml' end @@ -56,17 +57,17 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns user if valid feed_token' do set_param(:feed_token, user.feed_token) - expect(find_user_from_feed_token).to eq user + expect(find_user_from_feed_token(:rss)).to eq user end it 'returns nil if feed_token is blank' do - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end it 'returns exception if invalid feed_token' do set_param(:feed_token, 'invalid_token') - expect { find_user_from_feed_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + expect { find_user_from_feed_token(:rss) }.to raise_error(Gitlab::Auth::UnauthorizedError) end end @@ -74,34 +75,38 @@ describe Gitlab::Auth::UserAuthFinders do it 'returns user if valid rssd_token' do set_param(:rss_token, user.feed_token) - expect(find_user_from_feed_token).to eq user + expect(find_user_from_feed_token(:rss)).to eq user end it 'returns nil if rss_token is blank' do - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end it 'returns exception if invalid rss_token' do set_param(:rss_token, 'invalid_token') - expect { find_user_from_feed_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + expect { find_user_from_feed_token(:rss) }.to raise_error(Gitlab::Auth::UnauthorizedError) end end end context 'when the request format is not atom' do it 'returns nil' do + env['SCRIPT_NAME'] = 'json' + set_param(:feed_token, user.feed_token) - expect(find_user_from_feed_token).to be_nil + expect(find_user_from_feed_token(:rss)).to be_nil end end context 'when the request format is empty' do it 'the method call does not modify the original value' do + env['SCRIPT_NAME'] = 'url.atom' + env.delete('action_dispatch.request.formats') - find_user_from_feed_token + find_user_from_feed_token(:rss) expect(env['action_dispatch.request.formats']).to be_nil end @@ -111,8 +116,12 @@ describe Gitlab::Auth::UserAuthFinders do describe '#find_user_from_access_token' do let(:personal_access_token) { create(:personal_access_token, user: user) } + before do + env['SCRIPT_NAME'] = 'url.atom' + end + it 'returns nil if no access_token present' do - expect(find_personal_access_token).to be_nil + expect(find_user_from_access_token).to be_nil end context 'when validate_access_token! returns valid' do @@ -131,9 +140,59 @@ describe Gitlab::Auth::UserAuthFinders do end end + describe '#find_user_from_web_access_token' do + let(:personal_access_token) { create(:personal_access_token, user: user) } + + before do + env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token + end + + it 'returns exception if token has no user' do + allow_any_instance_of(PersonalAccessToken).to receive(:user).and_return(nil) + + expect { find_user_from_access_token }.to raise_error(Gitlab::Auth::UnauthorizedError) + end + + context 'no feed or API requests' do + it 'returns nil if the request is not RSS' do + expect(find_user_from_web_access_token(:rss)).to be_nil + end + + it 'returns nil if the request is not ICS' do + expect(find_user_from_web_access_token(:ics)).to be_nil + end + + it 'returns nil if the request is not API' do + expect(find_user_from_web_access_token(:api)).to be_nil + end + end + + it 'returns the user for RSS requests' do + env['SCRIPT_NAME'] = 'url.atom' + + expect(find_user_from_web_access_token(:rss)).to eq(user) + end + + it 'returns the user for ICS requests' do + env['SCRIPT_NAME'] = 'url.ics' + + expect(find_user_from_web_access_token(:ics)).to eq(user) + end + + it 'returns the user for API requests' do + env['SCRIPT_NAME'] = '/api/endpoint' + + expect(find_user_from_web_access_token(:api)).to eq(user) + end + end + describe '#find_personal_access_token' do let(:personal_access_token) { create(:personal_access_token, user: user) } + before do + env['SCRIPT_NAME'] = 'url.atom' + end + context 'passed as header' do it 'returns token if valid personal_access_token' do env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token diff --git a/spec/support/controllers/sessionless_auth_controller_shared_examples.rb b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb new file mode 100644 index 00000000000..7e4958f177a --- /dev/null +++ b/spec/support/controllers/sessionless_auth_controller_shared_examples.rb @@ -0,0 +1,92 @@ +shared_examples 'authenticates sessionless user' do |path, format, params| + params ||= {} + + before do + stub_authentication_activity_metrics(debug: false) + end + + let(:user) { create(:user) } + let(:personal_access_token) { create(:personal_access_token, user: user) } + let(:default_params) { { format: format }.merge(params.except(:public) || {}) } + + context "when the 'personal_access_token' param is populated with the personal access token" do + it 'logs the user in' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + get path, default_params.merge(private_token: personal_access_token.token) + + expect(response).to have_gitlab_http_status(200) + expect(controller.current_user).to eq(user) + end + + it 'does not log the user in if page is public', if: params[:public] do + get path, default_params + + expect(response).to have_gitlab_http_status(200) + expect(controller.current_user).to be_nil + end + end + + context 'when the personal access token has no api scope', unless: params[:public] do + it 'does not log the user in' do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + personal_access_token.update(scopes: [:read_user]) + + get path, default_params.merge(private_token: personal_access_token.token) + + expect(response).not_to have_gitlab_http_status(200) + end + end + + context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do + it 'logs the user in' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + @request.headers['PRIVATE-TOKEN'] = personal_access_token.token + get path, default_params + + expect(response).to have_gitlab_http_status(200) + end + end + + context "when the 'feed_token' param is populated with the feed token", if: format == :rss do + it "logs the user in" do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + .and increment(:user_session_override_counter) + .and increment(:user_sessionless_authentication_counter) + + get path, default_params.merge(feed_token: user.feed_token) + + expect(response).to have_gitlab_http_status 200 + end + end + + context "when the 'feed_token' param is populated with an invalid feed token", if: format == :rss, unless: params[:public] do + it "logs the user" do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + get path, default_params.merge(feed_token: 'token') + + expect(response.status).not_to eq 200 + end + end + + it "doesn't log the user in otherwise", unless: params[:public] do + expect(authentication_metrics) + .to increment(:user_unauthenticated_counter) + + get path, default_params.merge(private_token: 'token') + + expect(response.status).not_to eq(200) + end +end -- cgit v1.2.1 From 412f62919340e5df7505749927202bf6ea8b3785 Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Mon, 26 Nov 2018 16:29:40 +0200 Subject: Externalize strings from `/app/views/snippets` --- app/views/snippets/_actions.html.haml | 26 +++++++++++----------- app/views/snippets/_snippets.html.haml | 2 +- app/views/snippets/_snippets_scope_menu.html.haml | 8 +++---- app/views/snippets/edit.html.haml | 5 +++-- app/views/snippets/index.html.haml | 6 ++--- app/views/snippets/new.html.haml | 2 +- app/views/snippets/notes/_actions.html.haml | 4 ++-- app/views/snippets/show.html.haml | 4 ++-- .../gt-externalize-app-views-snippets.yml | 5 +++++ locale/gitlab.pot | 21 +++++++++++++++++ 10 files changed, 55 insertions(+), 28 deletions(-) create mode 100644 changelogs/unreleased/gt-externalize-app-views-snippets.yml diff --git a/app/views/snippets/_actions.html.haml b/app/views/snippets/_actions.html.haml index 0ce13ee7a53..ef8664e6f47 100644 --- a/app/views/snippets/_actions.html.haml +++ b/app/views/snippets/_actions.html.haml @@ -3,31 +3,31 @@ .d-none.d-sm-block - if can?(current_user, :update_personal_snippet, @snippet) = link_to edit_snippet_path(@snippet), class: "btn btn-grouped" do - Edit + = _("Edit") - if can?(current_user, :admin_personal_snippet, @snippet) - = link_to snippet_path(@snippet), method: :delete, data: { confirm: "Are you sure?" }, class: "btn btn-grouped btn-inverted btn-remove", title: 'Delete Snippet' do - Delete - = link_to new_snippet_path, class: "btn btn-grouped btn-inverted btn-success", title: "New snippet" do - New snippet + = link_to snippet_path(@snippet), method: :delete, data: { confirm: _("Are you sure?") }, class: "btn btn-grouped btn-inverted btn-remove", title: _('Delete Snippet') do + = _("Delete") + = link_to new_snippet_path, class: "btn btn-grouped btn-inverted btn-create", title: _("New snippet") do + = _("New snippet") - if @snippet.submittable_as_spam_by?(current_user) - = link_to 'Submit as spam', mark_as_spam_snippet_path(@snippet), method: :post, class: 'btn btn-grouped btn-spam', title: 'Submit as spam' + = link_to _('Submit as spam'), mark_as_spam_snippet_path(@snippet), method: :post, class: 'btn btn-grouped btn-spam', title: _('Submit as spam') .d-block.d-sm-none.dropdown %button.btn.btn-default.btn-block.append-bottom-0.prepend-top-5{ data: { toggle: "dropdown" } } - Options + = _("Options") = icon('caret-down') .dropdown-menu.dropdown-menu-full-width %ul %li - = link_to new_snippet_path, title: "New snippet" do - New snippet + = link_to new_snippet_path, title: _("New snippet") do + = _("New snippet") - if can?(current_user, :admin_personal_snippet, @snippet) %li - = link_to snippet_path(@snippet), method: :delete, data: { confirm: "Are you sure?" }, title: 'Delete Snippet' do - Delete + = link_to snippet_path(@snippet), method: :delete, data: { confirm: _("Are you sure?") }, title: _('Delete Snippet') do + = _("Delete") - if can?(current_user, :update_personal_snippet, @snippet) %li = link_to edit_snippet_path(@snippet) do - Edit + = _("Edit") - if @snippet.submittable_as_spam_by?(current_user) %li - = link_to 'Submit as spam', mark_as_spam_snippet_path(@snippet), method: :post + = link_to _('Submit as spam'), mark_as_spam_snippet_path(@snippet), method: :post diff --git a/app/views/snippets/_snippets.html.haml b/app/views/snippets/_snippets.html.haml index dfea8b40bd8..69d41f8fe5e 100644 --- a/app/views/snippets/_snippets.html.haml +++ b/app/views/snippets/_snippets.html.haml @@ -5,6 +5,6 @@ = render partial: 'shared/snippets/snippet', collection: @snippets, locals: { link_project: link_project } - if @snippets.empty? %li - .nothing-here-block Nothing here. + .nothing-here-block= _("Nothing here.") = paginate @snippets, theme: 'gitlab' diff --git a/app/views/snippets/_snippets_scope_menu.html.haml b/app/views/snippets/_snippets_scope_menu.html.haml index dc4b0fd9ba0..c312226dd6c 100644 --- a/app/views/snippets/_snippets_scope_menu.html.haml +++ b/app/views/snippets/_snippets_scope_menu.html.haml @@ -4,7 +4,7 @@ .nav-links.snippet-scope-menu.mobile-separator.nav.nav-tabs %li{ class: active_when(params[:scope].nil?) } = link_to subject_snippets_path(subject) do - All + = _("All") %span.badge.badge-pill - if include_private = subject.snippets.count @@ -14,18 +14,18 @@ - if include_private %li{ class: active_when(params[:scope] == "are_private") } = link_to subject_snippets_path(subject, scope: 'are_private') do - Private + = _("Private") %span.badge.badge-pill = subject.snippets.are_private.count %li{ class: active_when(params[:scope] == "are_internal") } = link_to subject_snippets_path(subject, scope: 'are_internal') do - Internal + = _("Internal") %span.badge.badge-pill = subject.snippets.are_internal.count %li{ class: active_when(params[:scope] == "are_public") } = link_to subject_snippets_path(subject, scope: 'are_public') do - Public + = _("Public") %span.badge.badge-pill = subject.snippets.are_public.count diff --git a/app/views/snippets/edit.html.haml b/app/views/snippets/edit.html.haml index 18ebeb78f87..ebc6c0a2605 100644 --- a/app/views/snippets/edit.html.haml +++ b/app/views/snippets/edit.html.haml @@ -1,5 +1,6 @@ -- page_title "Edit", "#{@snippet.title} (#{@snippet.to_reference})", "Snippets" +- page_title _("Edit"), "#{@snippet.title} (#{@snippet.to_reference})", _("Snippets") + %h3.page-title - Edit Snippet + = _("Edit Snippet") %hr = render 'shared/snippets/form', url: snippet_path(@snippet) diff --git a/app/views/snippets/index.html.haml b/app/views/snippets/index.html.haml index 9b4a7dbe68d..4f418e2381f 100644 --- a/app/views/snippets/index.html.haml +++ b/app/views/snippets/index.html.haml @@ -1,13 +1,13 @@ -- page_title "By #{@user.name}", "Snippets" +- page_title _("By %{user_name}") % { user_name: @user.name }, _("Snippets") %ol.breadcrumb %li.breadcrumb-item = link_to snippets_path do - Snippets + = _("Snippets") %li.breadcrumb-item = @user.name .float-right.d-none.d-sm-block = link_to user_path(@user) do - #{@user.name} profile page + = _("%{user_name} profile page") % { user_name: @user.name } = render 'snippets' diff --git a/app/views/snippets/new.html.haml b/app/views/snippets/new.html.haml index 6bc748d346e..114c777bdc2 100644 --- a/app/views/snippets/new.html.haml +++ b/app/views/snippets/new.html.haml @@ -1,6 +1,6 @@ - @hide_top_links = true - @hide_breadcrumbs = true -- page_title "New Snippet" +- page_title _("New Snippet") .page-title-holder %h1.page-title= _('New Snippet') diff --git a/app/views/snippets/notes/_actions.html.haml b/app/views/snippets/notes/_actions.html.haml index 220ba2b49e6..01b95145937 100644 --- a/app/views/snippets/notes/_actions.html.haml +++ b/app/views/snippets/notes/_actions.html.haml @@ -1,7 +1,7 @@ - if current_user - if note.emoji_awardable? .note-actions-item - = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji has-tooltip", data: { position: 'right' } do + = link_to '#', title: _('Add reaction'), class: "note-action-button note-emoji-button js-add-award js-note-emoji has-tooltip", data: { position: 'right' } do = icon('spinner spin') %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face') %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley') @@ -9,7 +9,7 @@ - if note_editable .note-actions-item - = button_tag title: 'Edit comment', class: 'note-action-button js-note-edit has-tooltip btn btn-transparent', data: { container: 'body' } do + = button_tag title: _('Edit comment'), class: 'note-action-button js-note-edit has-tooltip btn btn-transparent', data: { container: 'body' } do %span.link-highlight = custom_icon('icon_pencil') diff --git a/app/views/snippets/show.html.haml b/app/views/snippets/show.html.haml index 578327883e5..36b4e00e8d5 100644 --- a/app/views/snippets/show.html.haml +++ b/app/views/snippets/show.html.haml @@ -1,8 +1,8 @@ - @hide_top_links = true - @content_class = "limit-container-width limited-inner-width-container" unless fluid_layout -- add_to_breadcrumbs "Snippets", dashboard_snippets_path +- add_to_breadcrumbs _("Snippets"), dashboard_snippets_path - breadcrumb_title @snippet.to_reference -- page_title "#{@snippet.title} (#{@snippet.to_reference})", "Snippets" +- page_title "#{@snippet.title} (#{@snippet.to_reference})", _("Snippets") = render 'shared/snippets/header' diff --git a/changelogs/unreleased/gt-externalize-app-views-snippets.yml b/changelogs/unreleased/gt-externalize-app-views-snippets.yml new file mode 100644 index 00000000000..633aa9f2534 --- /dev/null +++ b/changelogs/unreleased/gt-externalize-app-views-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Externalize strings from `/app/views/snippets` +merge_request: 23351 +author: Tao Wang +type: other diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c29c25c917e..fecf78dc82f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -158,6 +158,9 @@ msgstr "" msgid "%{usage_ping_link_start}Learn more%{usage_ping_link_end} about what information is shared with GitLab Inc." msgstr "" +msgid "%{user_name} profile page" +msgstr "" + msgid "+ %{count} more" msgstr "" @@ -1082,6 +1085,9 @@ msgstr "" msgid "Business metrics (Custom)" msgstr "" +msgid "By %{user_name}" +msgstr "" + msgid "ByAuthor|by" msgstr "" @@ -2558,6 +2564,9 @@ msgstr "" msgid "Edit application" msgstr "" +msgid "Edit comment" +msgstr "" + msgid "Edit environment" msgstr "" @@ -3496,6 +3505,9 @@ msgstr "" msgid "Interested parties can even contribute by pushing commits if they want to." msgstr "" +msgid "Internal" +msgstr "" + msgid "Internal - The group and any internal projects can be viewed by any logged in user." msgstr "" @@ -4321,6 +4333,9 @@ msgstr "" msgid "Notes|Show history only" msgstr "" +msgid "Nothing here." +msgstr "" + msgid "Notification events" msgstr "" @@ -4779,6 +4794,9 @@ msgstr "" msgid "Prioritized label" msgstr "" +msgid "Private" +msgstr "" + msgid "Private - Project access must be granted explicitly to each user." msgstr "" @@ -5205,6 +5223,9 @@ msgstr "" msgid "Provider" msgstr "" +msgid "Public" +msgstr "" + msgid "Public - The group and any public projects can be viewed without any authentication." msgstr "" -- cgit v1.2.1 From 927f54e0341e1710b94608589af0e747d7f89d7b Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 26 Nov 2018 22:35:39 +0000 Subject: Update CHANGELOG.md for 11.5.1 [ci skip] --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c65b584e3c1..9c0bacfea7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.5.1 (2018-11-26) + +### Security (17 changes) + +- Escape user fullname while rendering autocomplete template to prevent XSS. +- Fix CRLF vulnerability in Project hooks. +- Fix possible XSS attack in Markdown urls with spaces. +- Redact sensitive information on gitlab-workhorse log. +- Do not follow redirects in Prometheus service when making http requests to the configured api url. +- Don't expose confidential information in commit message list. +- Provide email notification when a user changes their email address. +- Restrict Personal Access Tokens to API scope on web requests. +- Resolve reflected XSS in Ouath authorize window. +- Fix SSRF in project integrations. +- Fixed ability to comment on locked/confidential issues. +- Fixed ability of guest users to edit/delete comments on locked or confidential issues. +- Fix milestone promotion authorization check. +- Configure mermaid to not render HTML content in diagrams. +- Fix a possible symlink time of check to time of use race condition in GitLab Pages. +- Removed ability to see private group names when the group id is entered in the url. +- Fix stored XSS for Environments. + + ## 11.5.0 (2018-11-22) ### Security (10 changes, 1 of them is from the community) -- cgit v1.2.1 From 1ca3a2d6341d75c61beee61580a31f531a7bf3ff Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 26 Nov 2018 23:51:32 +0000 Subject: Update CHANGELOG.md for 11.3.11 [ci skip] --- CHANGELOG.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c0bacfea7c..92cca794e31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -567,6 +567,45 @@ entry. - Check frozen string in style builds. (gfyoung) +## 11.3.11 (2018-11-26) + +### Security (33 changes) + +- Filter user sensitive data from discussions JSON. !2537 +- Escape entity title while autocomplete template rendering to prevent XSS. !2557 +- Restrict Personal Access Tokens to API scope on web requests. +- Fix XSS in merge request source branch name. +- Escape user fullname while rendering autocomplete template to prevent XSS. +- Fix CRLF vulnerability in Project hooks. +- Fix possible XSS attack in Markdown urls with spaces. +- Redact sensitive information on gitlab-workhorse log. +- Set timeout for syntax highlighting. +- Do not follow redirects in Prometheus service when making http requests to the configured api url. +- Persist only SHA digest of PersonalAccessToken#token. +- Sanitize JSON data properly to fix XSS on Issue details page. +- Don't expose confidential information in commit message list. +- Markdown API no longer displays confidential title references unless authorized. +- Provide email notification when a user changes their email address. +- Properly filter private references from system notes. +- Redact personal tokens in unsubscribe links. +- Resolve reflected XSS in Ouath authorize window. +- Fix SSRF in project integrations. +- Fix stored XSS in merge requests from imported repository. +- Fixed ability to comment on locked/confidential issues. +- Fixed ability of guest users to edit/delete comments on locked or confidential issues. +- Fix milestone promotion authorization check. +- Monkey kubeclient to not follow any redirects. +- Configure mermaid to not render HTML content in diagrams. +- Redact confidential events in the API. +- Fix xss vulnerability sourced from package.json. +- Fix a possible symlink time of check to time of use race condition in GitLab Pages. +- Removed ability to see private group names when the group id is entered in the url. +- Fix stored XSS for Environments. +- Block loopback addresses in UrlBlocker. +- Prevent SSRF attacks in HipChat integration. +- Validate Wiki attachments are valid temporary files. + + ## 11.3.10 (2018-11-18) ### Security (1 change) -- cgit v1.2.1 From 54b639195ddb2d11b5a5e8208d5f94df365041db Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 22 Nov 2018 21:13:36 +0100 Subject: Make add_reference cop accept a hash for :index It might happen you want to make the reference column have a unique value, or you want to create partial indexes. So instead of only accepting a `true` value, also accept a hash of options. --- rubocop/cop/migration/add_reference.rb | 9 +++++++-- spec/rubocop/cop/migration/add_reference_spec.rb | 12 ++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/rubocop/cop/migration/add_reference.rb b/rubocop/cop/migration/add_reference.rb index 4b67270c97a..1d471b9797e 100644 --- a/rubocop/cop/migration/add_reference.rb +++ b/rubocop/cop/migration/add_reference.rb @@ -8,7 +8,7 @@ module RuboCop class AddReference < RuboCop::Cop::Cop include MigrationHelpers - MSG = '`add_reference` requires `index: true`' + MSG = '`add_reference` requires `index: true` or `index: { options... }`' def on_send(node) return unless in_migration?(node) @@ -33,7 +33,12 @@ module RuboCop private def index_enabled?(pair) - hash_key_type(pair) == :sym && hash_key_name(pair) == :index && pair.children[1].true_type? + return unless hash_key_type(pair) == :sym + return unless hash_key_name(pair) == :index + + index = pair.children[1] + + index.true_type? || index.hash_type? end def hash_key_type(pair) diff --git a/spec/rubocop/cop/migration/add_reference_spec.rb b/spec/rubocop/cop/migration/add_reference_spec.rb index 8f795bb561e..c348fc0efac 100644 --- a/spec/rubocop/cop/migration/add_reference_spec.rb +++ b/spec/rubocop/cop/migration/add_reference_spec.rb @@ -29,7 +29,7 @@ describe RuboCop::Cop::Migration::AddReference do expect_offense(<<~RUBY) call do add_reference(:projects, :users) - ^^^^^^^^^^^^^ `add_reference` requires `index: true` + ^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }` end RUBY end @@ -38,7 +38,7 @@ describe RuboCop::Cop::Migration::AddReference do expect_offense(<<~RUBY) def up add_reference(:projects, :users, index: false) - ^^^^^^^^^^^^^ `add_reference` requires `index: true` + ^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }` end RUBY end @@ -50,5 +50,13 @@ describe RuboCop::Cop::Migration::AddReference do end RUBY end + + it 'does not register an offense when the index is unique' do + expect_no_offenses(<<~RUBY) + def up + add_reference(:projects, :users, index: { unique: true } ) + end + RUBY + end end end -- cgit v1.2.1 From 53eb28b5fed8b04c6609633064fb70ed7115dd11 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Tue, 27 Nov 2018 11:38:04 +0000 Subject: Update CHANGELOG.md for 11.4.8 [ci skip] --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92cca794e31..57e946befb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -287,6 +287,36 @@ entry. - Disables stop environment button while the deploy is in progress. +## 11.4.8 (2018-11-27) + +### Security (24 changes) + +- Escape entity title while autocomplete template rendering to prevent XSS. !2571 +- Resolve reflected XSS in Ouath authorize window. +- Fix XSS in merge request source branch name. +- Escape user fullname while rendering autocomplete template to prevent XSS. +- Fix CRLF vulnerability in Project hooks. +- Fix possible XSS attack in Markdown urls with spaces. +- Redact sensitive information on gitlab-workhorse log. +- Do not follow redirects in Prometheus service when making http requests to the configured api url. +- Persist only SHA digest of PersonalAccessToken#token. +- Don't expose confidential information in commit message list. +- Provide email notification when a user changes their email address. +- Restrict Personal Access Tokens to API scope on web requests. +- Redact personal tokens in unsubscribe links. +- Fix SSRF in project integrations. +- Fixed ability to comment on locked/confidential issues. +- Fixed ability of guest users to edit/delete comments on locked or confidential issues. +- Fix milestone promotion authorization check. +- Monkey kubeclient to not follow any redirects. +- Configure mermaid to not render HTML content in diagrams. +- Fix a possible symlink time of check to time of use race condition in GitLab Pages. +- Removed ability to see private group names when the group id is entered in the url. +- Fix stored XSS for Environments. +- Prevent SSRF attacks in HipChat integration. +- Validate Wiki attachments are valid temporary files. + + ## 11.4.7 (2018-11-20) - No changes. -- cgit v1.2.1 From fff7754186202cfcdeaa0962c28e5d43ddd705b7 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 20 Nov 2018 12:48:18 +0100 Subject: Rename the Repository table to PoolRepository To separate the different kinds of repositories we have at GitLab this table will be renamed to pool_repositories. A project can, for now at least, be member of none, or one of these. The table will get additional columns in a later merge request where more logic is implemented for the model. Further included is a small refactor of logic around hashing ids for the disk_path, mainly to ensure a previous implementation is reusable. The disk_path for the pool_repositories table no longer has a NOT NULL constraint, but given the hashing of the ID requires the DB to assign the record an ID, an after_create hook is used to update the value. A related MR is: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23143, adding tables for 'normal' repositories and wiki_repositories. --- app/models/pool_repository.rb | 19 +++++++++++------ app/models/storage/hashed_project.rb | 8 +++++--- ...082911_rename_repositories_pool_repositories.rb | 11 ++++++++++ ...ot_null_constraint_pool_repository_disk_path.rb | 9 ++++++++ db/schema.rb | 18 ++++++++-------- lib/tasks/gitlab/cleanup.rake | 4 ++-- spec/factories/pool_repositories.rb | 5 +++++ spec/factories/shards.rb | 5 +++++ spec/models/pool_repository_spec.rb | 24 ++++++++++++++++++++++ 9 files changed, 83 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20181120082911_rename_repositories_pool_repositories.rb create mode 100644 db/migrate/20181123135036_drop_not_null_constraint_pool_repository_disk_path.rb create mode 100644 spec/factories/pool_repositories.rb create mode 100644 spec/factories/shards.rb create mode 100644 spec/models/pool_repository_spec.rb diff --git a/app/models/pool_repository.rb b/app/models/pool_repository.rb index 8ef74539209..4b697d1991f 100644 --- a/app/models/pool_repository.rb +++ b/app/models/pool_repository.rb @@ -1,16 +1,12 @@ # frozen_string_literal: true class PoolRepository < ActiveRecord::Base - POOL_PREFIX = '@pools' - belongs_to :shard validates :shard, presence: true - # For now, only pool repositories are tracked in the database. However, we may - # want to add other repository types in the future - self.table_name = 'repositories' + has_many :member_projects, class_name: 'Project' - has_many :pool_member_projects, class_name: 'Project', foreign_key: :pool_repository_id + after_create :correct_disk_path def shard_name shard&.name @@ -19,4 +15,15 @@ class PoolRepository < ActiveRecord::Base def shard_name=(name) self.shard = Shard.by_name(name) end + + private + + def correct_disk_path + update(disk_path: storage.disk_path) + end + + def storage + Storage::HashedProject + .new(self, prefix: Storage::HashedProject::POOL_PATH_PREFIX) + end end diff --git a/app/models/storage/hashed_project.rb b/app/models/storage/hashed_project.rb index 90710f73fd3..911fb7e9ce9 100644 --- a/app/models/storage/hashed_project.rb +++ b/app/models/storage/hashed_project.rb @@ -5,17 +5,19 @@ module Storage attr_accessor :project delegate :gitlab_shell, :repository_storage, to: :project - ROOT_PATH_PREFIX = '@hashed'.freeze + REPOSITORY_PATH_PREFIX = '@hashed' + POOL_PATH_PREFIX = '@pools' - def initialize(project) + def initialize(project, prefix: REPOSITORY_PATH_PREFIX) @project = project + @prefix = prefix end # Base directory # # @return [String] directory where repository is stored def base_dir - "#{ROOT_PATH_PREFIX}/#{disk_hash[0..1]}/#{disk_hash[2..3]}" if disk_hash + "#{@prefix}/#{disk_hash[0..1]}/#{disk_hash[2..3]}" if disk_hash end # Disk path is used to build repository and project's wiki path on disk diff --git a/db/migrate/20181120082911_rename_repositories_pool_repositories.rb b/db/migrate/20181120082911_rename_repositories_pool_repositories.rb new file mode 100644 index 00000000000..165771c4775 --- /dev/null +++ b/db/migrate/20181120082911_rename_repositories_pool_repositories.rb @@ -0,0 +1,11 @@ +class RenameRepositoriesPoolRepositories < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # This change doesn't require downtime as the table is not in use, so we're + # free to change an empty table + DOWNTIME = false + + def change + rename_table :repositories, :pool_repositories + end +end diff --git a/db/migrate/20181123135036_drop_not_null_constraint_pool_repository_disk_path.rb b/db/migrate/20181123135036_drop_not_null_constraint_pool_repository_disk_path.rb new file mode 100644 index 00000000000..bcd969e91c5 --- /dev/null +++ b/db/migrate/20181123135036_drop_not_null_constraint_pool_repository_disk_path.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class DropNotNullConstraintPoolRepositoryDiskPath < ActiveRecord::Migration[5.0] + DOWNTIME = false + + def change + change_column_null :pool_repositories, :disk_path, true + end +end diff --git a/db/schema.rb b/db/schema.rb index e0ca571df5a..810afcdcfd5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1501,6 +1501,13 @@ ActiveRecord::Schema.define(version: 20181126153547) do t.index ["user_id"], name: "index_personal_access_tokens_on_user_id", using: :btree end + create_table "pool_repositories", id: :bigserial, force: :cascade do |t| + t.integer "shard_id", null: false + t.string "disk_path" + t.index ["disk_path"], name: "index_pool_repositories_on_disk_path", unique: true, using: :btree + t.index ["shard_id"], name: "index_pool_repositories_on_shard_id", using: :btree + end + create_table "programming_languages", force: :cascade do |t| t.string "name", null: false t.string "color", null: false @@ -1798,13 +1805,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do t.index ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree end - create_table "repositories", id: :bigserial, force: :cascade do |t| - t.integer "shard_id", null: false - t.string "disk_path", null: false - t.index ["disk_path"], name: "index_repositories_on_disk_path", unique: true, using: :btree - t.index ["shard_id"], name: "index_repositories_on_shard_id", using: :btree - end - create_table "repository_languages", id: false, force: :cascade do |t| t.integer "project_id", null: false t.integer "programming_language_id", null: false @@ -2377,6 +2377,7 @@ ActiveRecord::Schema.define(version: 20181126153547) do add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id" add_foreign_key "pages_domains", "projects", name: "fk_ea2f6dfc6f", on_delete: :cascade add_foreign_key "personal_access_tokens", "users" + add_foreign_key "pool_repositories", "shards", on_delete: :restrict add_foreign_key "project_authorizations", "projects", on_delete: :cascade add_foreign_key "project_authorizations", "users", on_delete: :cascade add_foreign_key "project_auto_devops", "projects", on_delete: :cascade @@ -2389,7 +2390,7 @@ ActiveRecord::Schema.define(version: 20181126153547) do add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade add_foreign_key "project_mirror_data", "projects", on_delete: :cascade add_foreign_key "project_statistics", "projects", on_delete: :cascade - add_foreign_key "projects", "repositories", column: "pool_repository_id", name: "fk_6e5c14658a", on_delete: :nullify + add_foreign_key "projects", "pool_repositories", name: "fk_6e5c14658a", on_delete: :nullify add_foreign_key "prometheus_metrics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches", name: "fk_8a3072ccb3", on_delete: :cascade add_foreign_key "protected_branch_push_access_levels", "protected_branches", name: "fk_9ffc86a3d9", on_delete: :cascade @@ -2401,7 +2402,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do add_foreign_key "push_event_payloads", "events", name: "fk_36c74129da", on_delete: :cascade add_foreign_key "releases", "projects", name: "fk_47fe2a0596", on_delete: :cascade add_foreign_key "remote_mirrors", "projects", on_delete: :cascade - add_foreign_key "repositories", "shards", on_delete: :restrict add_foreign_key "repository_languages", "projects", on_delete: :cascade add_foreign_key "resource_label_events", "issues", on_delete: :cascade add_foreign_key "resource_label_events", "labels", on_delete: :nullify diff --git a/lib/tasks/gitlab/cleanup.rake b/lib/tasks/gitlab/cleanup.rake index e8ae5dfa540..451ba651674 100644 --- a/lib/tasks/gitlab/cleanup.rake +++ b/lib/tasks/gitlab/cleanup.rake @@ -6,7 +6,7 @@ namespace :gitlab do desc "GitLab | Cleanup | Clean namespaces" task dirs: :gitlab_environment do namespaces = Set.new(Namespace.pluck(:path)) - namespaces << Storage::HashedProject::ROOT_PATH_PREFIX + namespaces << Storage::HashedProject::REPOSITORY_PATH_PREFIX Gitaly::Server.all.each do |server| all_dirs = Gitlab::GitalyClient::StorageService @@ -49,7 +49,7 @@ namespace :gitlab do # TODO ignoring hashed repositories for now. But revisit to fully support # possible orphaned hashed repos - next if repo_with_namespace.start_with?(Storage::HashedProject::ROOT_PATH_PREFIX) + next if repo_with_namespace.start_with?(Storage::HashedProject::REPOSITORY_PATH_PREFIX) next if Project.find_by_full_path(repo_with_namespace) new_path = path + move_suffix diff --git a/spec/factories/pool_repositories.rb b/spec/factories/pool_repositories.rb new file mode 100644 index 00000000000..2ed0844ed47 --- /dev/null +++ b/spec/factories/pool_repositories.rb @@ -0,0 +1,5 @@ +FactoryBot.define do + factory :pool_repository do + shard + end +end diff --git a/spec/factories/shards.rb b/spec/factories/shards.rb new file mode 100644 index 00000000000..c095fa5f0a0 --- /dev/null +++ b/spec/factories/shards.rb @@ -0,0 +1,5 @@ +FactoryBot.define do + factory :shard do + name "default" + end +end diff --git a/spec/models/pool_repository_spec.rb b/spec/models/pool_repository_spec.rb new file mode 100644 index 00000000000..6c904710fb5 --- /dev/null +++ b/spec/models/pool_repository_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe PoolRepository do + describe 'associations' do + it { is_expected.to belong_to(:shard) } + it { is_expected.to have_many(:member_projects) } + end + + describe 'validations' do + let!(:pool_repository) { create(:pool_repository) } + + it { is_expected.to validate_presence_of(:shard) } + end + + describe '#disk_path' do + it 'sets the hashed disk_path' do + pool = create(:pool_repository) + + elements = File.split(pool.disk_path) + + expect(elements).to all( match(/\d{2,}/) ) + end + end +end -- cgit v1.2.1 From 9f287298270b3a833bc53b9280c6e3c6a2f055b8 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 22 Nov 2018 06:02:58 +0000 Subject: Add a rebase API endpoint for merge requests --- .../unreleased/33705-merge-request-rebase-api.yml | 5 +++++ doc/api/merge_requests.md | 25 +++++++++++++++++++++ lib/api/merge_requests.rb | 26 ++++++++++++++++++++++ spec/requests/api/merge_requests_spec.rb | 20 +++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 changelogs/unreleased/33705-merge-request-rebase-api.yml diff --git a/changelogs/unreleased/33705-merge-request-rebase-api.yml b/changelogs/unreleased/33705-merge-request-rebase-api.yml new file mode 100644 index 00000000000..322fe31ce87 --- /dev/null +++ b/changelogs/unreleased/33705-merge-request-rebase-api.yml @@ -0,0 +1,5 @@ +--- +title: Add a rebase API endpoint for merge requests +merge_request: 23296 +author: +type: added diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index da70c74c4ce..2f5b1608882 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1206,6 +1206,31 @@ Parameters: } ``` +## Rebase a merge request + +Automatically rebase the `source_branch` of the merge request against its +`target_branch`. + +If you don't have permissions to push to the merge request's source branch - +you'll get a `403 Forbidden` response. + +``` +PUT /projects/:id/merge_requests/:merge_request_iid/rebase +``` + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `merge_request_iid` | integer | yes | The internal ID of the merge request | + +```bash +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/76/merge_requests/1/rebase +``` + +This is an asynchronous request. The API will return an empty `202 Accepted` +response if the request is enqueued successfully. You should poll the +[Get single MR](#get-single-mr) endpoint to determine success or failure. + ## Comments on merge requests Comments are done via the [notes](notes.md) resource. diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 16f07f16387..23e65dca424 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -74,6 +74,19 @@ module API options end + def authorize_push_to_merge_request!(merge_request) + forbidden!('Source branch does not exist') unless + merge_request.source_branch_exists? + + user_access = Gitlab::UserAccess.new( + current_user, + project: merge_request.source_project + ) + + forbidden!('Cannot push to source branch') unless + user_access.can_push_to_branch?(merge_request.source_branch) + end + params :merge_requests_params do optional :state, type: String, values: %w[opened closed locked merged all], default: 'all', desc: 'Return opened, closed, locked, merged, or all merge requests' @@ -378,6 +391,19 @@ module API .cancel(merge_request) end + desc 'Rebase the merge request against its target branch' do + detail 'This feature was added in GitLab 11.6' + end + put ':id/merge_requests/:merge_request_iid/rebase' do + merge_request = find_project_merge_request(params[:merge_request_iid]) + + authorize_push_to_merge_request!(merge_request) + + RebaseWorker.perform_async(merge_request.id, current_user.id) + + status :accepted + end + desc 'List issues that will be closed on merge' do success Entities::MRNote end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index e4e0ca285e0..19602e7152a 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1181,6 +1181,26 @@ describe API::MergeRequests do end end + describe 'PUT :id/merge_requests/:merge_request_iid/rebase' do + it 'enques a rebase of the merge request against the target branch' do + Sidekiq::Testing.fake! do + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/rebase", user) + end + + expect(response).to have_gitlab_http_status(202) + expect(RebaseWorker.jobs.size).to eq(1) + end + + it 'returns 403 if the user cannot push to the branch' do + guest = create(:user) + project.add_guest(guest) + + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/rebase", guest) + + expect(response).to have_gitlab_http_status(403) + end + end + describe 'Time tracking' do let(:issuable) { merge_request } -- cgit v1.2.1 From 27650cdd339d086e4c1badf0d0c5723b85ecdccb Mon Sep 17 00:00:00 2001 From: Evan Read Date: Wed, 28 Nov 2018 08:44:51 +1000 Subject: Force better alignment in tables --- doc/README.md | 96 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/doc/README.md b/doc/README.md index bf93c73843f..ba2bb89533b 100644 --- a/doc/README.md +++ b/doc/README.md @@ -8,7 +8,7 @@ description: 'Learn how to use and administer GitLab, the most scalable Git-base Welcome to [GitLab](https://about.gitlab.com/) Documentation. Here you can access the complete documentation for GitLab, the single application for the -[entire DevOps lifecycle](#complete-devops-with-gitlab). +[entire DevOps lifecycle](#the-entire-devops-lifecycle). ## Overview @@ -72,11 +72,11 @@ GitLab provides statistics and insight into ways you can maximize the value of G The following documentation relates to the DevOps **Manage** stage: -| Manage Topics | Description | -|:----------------------------------------------------------------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| [Authentication and Authorization](administration/auth/README.md) **[CORE ONLY]** | Supported authentication and authorization providers. | -| [GitLab Cycle Analytics](user/project/cycle_analytics.md) | Measure the time it takes to go from an [idea to production](https://about.gitlab.com/2016/08/05/continuous-integration-delivery-and-deployment-with-gitlab/#from-idea-to-production-with-gitlab) for each project you have. | -| [Instance Statistics](user/instance_statistics/index.md) | Discover statistics on how many GitLab features you use and user activity. | +| Manage Topics | Description | +|:--------------------------------------------------------------------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| [Authentication and
Authorization](administration/auth/README.md) **[CORE ONLY]** | Supported authentication and authorization providers. | +| [GitLab Cycle Analytics](user/project/cycle_analytics.md) | Measure the time it takes to go from an [idea to production](https://about.gitlab.com/2016/08/05/continuous-integration-delivery-and-deployment-with-gitlab/#from-idea-to-production-with-gitlab) for each project you have. | +| [Instance Statistics](user/instance_statistics/index.md) | Discover statistics on how many GitLab features you use and user activity. |
@@ -93,17 +93,17 @@ management tools. The following documentation relates to the DevOps **Plan** stage: -| Plan Topics | Description | -|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------| -| [Discussions](user/discussions/index.md) | Threads, comments, and resolvable discussions in issues, commits, and merge requests. | -| [Due Dates](user/project/issues/due_dates.md) | Keep track of issue deadlines. | -| [Quick Actions](user/project/quick_actions.md) | Shortcuts for common actions on issues or merge requests, replacing the need to click buttons or use dropdowns in GitLab's UI. | -| [Issues](user/project/issues/index.md), including [confidential issues](user/project/issues/confidential_issues.md), [issue and merge request templates](user/project/description_templates.md), and [moving issues](user/project/issues/moving_issues.md) | Project issues, restricting access to issues, create templates for submitting new issues and merge requests, and moving issues between projects. | -| [Labels](user/project/labels.md) | Categorize issues or merge requests with descriptive labels. | -| [Milestones](user/project/milestones/index.md) | Set milestones for delivery of issues and merge requests, with optional due date. | -| [Project Issue Board](user/project/issue_board.md) | Display issues on a Scrum or Kanban board. | -| [Time Tracking](workflow/time_tracking.md) | Track time spent on issues and merge requests. | -| [Todos](workflow/todos.md) | Keep track of work requiring attention with a chronological list displayed on a simple dashboard. | +| Plan Topics | Description | +|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------------------------------------------------------------------------| +| [Discussions](user/discussions/index.md) | Threads, comments, and resolvable discussions in issues, commits, and merge requests. | +| [Due Dates](user/project/issues/due_dates.md) | Keep track of issue deadlines. | +| [Quick Actions](user/project/quick_actions.md) | Shortcuts for common actions on issues or merge requests, replacing the need to click buttons or use dropdowns in GitLab's UI. | +| [Issues](user/project/issues/index.md), including [confidential issues](user/project/issues/confidential_issues.md),
[issue and merge request templates](user/project/description_templates.md),
and [moving issues](user/project/issues/moving_issues.md) | Project issues, restricting access to issues, create templates for submitting new issues and merge requests, and moving issues between projects. | +| [Labels](user/project/labels.md) | Categorize issues or merge requests with descriptive labels. | +| [Milestones](user/project/milestones/index.md) | Set milestones for delivery of issues and merge requests, with optional due date. | +| [Project Issue Board](user/project/issue_board.md) | Display issues on a Scrum or Kanban board. | +| [Time Tracking](workflow/time_tracking.md) | Track time spent on issues and merge requests. | +| [Todos](workflow/todos.md) | Keep track of work requiring attention with a chronological list displayed on a simple dashboard. |
@@ -124,16 +124,16 @@ The following documentation relates to the DevOps **Create** stage: #### Projects and Groups -| Create Topics - Projects and Groups | Description | -|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------| -| [Create](gitlab-basics/create-project.md) and [fork](gitlab-basics/fork-project.md) projects, and [import and export projects between instances](user/project/settings/import_export.md) | Create, duplicate, and move projects. | -| [GitLab Pages](user/project/pages/index.md) | Build, test, and deploy your static website with GitLab Pages. | -| [Groups](user/group/index.md) and [Subgroups](user/group/subgroups/index.md) | Organize your projects in groups. | -| [Projects](user/project/index.md), including [project access](public_access/public_access.md) and [settings](user/project/settings/index.md) | Host source code, and control your project's visibility and set configuration. | -| [Search through GitLab](user/search/index.md) | Search for issues, merge requests, projects, groups, and todos. | -| [Snippets](user/snippets.md) | Snippets allow you to create little bits of code. | -| [Web IDE](user/project/web_ide/index.md) | Edit files within GitLab's user interface. | -| [Wikis](user/project/wiki/index.md) | Enhance your repository documentation with built-in wikis. | +| Create Topics - Projects and Groups | Description | +|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:-------------------------------------------------------------------------------| +| [Create](gitlab-basics/create-project.md) and [fork](gitlab-basics/fork-project.md) projects, and
[import and export
projects between instances](user/project/settings/import_export.md) | Create, duplicate, and move projects. | +| [GitLab Pages](user/project/pages/index.md) | Build, test, and deploy your static website with GitLab Pages. | +| [Groups](user/group/index.md) and [Subgroups](user/group/subgroups/index.md) | Organize your projects in groups. | +| [Projects](user/project/index.md), including [project access](public_access/public_access.md)
and [settings](user/project/settings/index.md) | Host source code, and control your project's visibility and set configuration. | +| [Search through GitLab](user/search/index.md) | Search for issues, merge requests, projects, groups, and todos. | +| [Snippets](user/snippets.md) | Snippets allow you to create little bits of code. | +| [Web IDE](user/project/web_ide/index.md) | Edit files within GitLab's user interface. | +| [Wikis](user/project/wiki/index.md) | Enhance your repository documentation with built-in wikis. |
@@ -143,18 +143,18 @@ The following documentation relates to the DevOps **Create** stage: #### Repositories -| Create Topics - Repositories | Description | -|:--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------------------------------------------------| -| [Branches](user/project/repository/branches/index.md) and the [default branch](user/project/repository/branches/index.md#default-branch) | How to use branches in GitLab. | -| [Commits](user/project/repository/index.md#commits) and [signing commits](user/project/repository/gpg_signed_commits/index.md) | Work with commits, and use GPG to sign your commits. | -| [Create branches](user/project/repository/web_editor.md#create-a-new-branch), [create](user/project/repository/web_editor.md#create-a-file) and [upload](user/project/repository/web_editor.md#upload-a-file) files, and [create directories](user/project/repository/web_editor.md#create-a-directory) | Create branches, create and upload files, and create directories within GitLab. | -| [Delete merged branches](user/project/repository/branches/index.md#delete-merged-branches) | Bulk delete branches after their changes are merged. | -| [File templates](user/project/repository/web_editor.md#template-dropdowns) | File templates for common files. | -| [Files](user/project/repository/index.md#files) | Files management. | -| [Jupyter Notebook files](user/project/repository/index.md#jupyter-notebook-files) | GitLab's support for `.ipynb` files. | -| [Protected branches](user/project/protected_branches.md) | Use protected branches. | -| [Repositories](user/project/repository/index.md) | Manage source code repositories in GitLab's user interface. | -| [Start a merge request](user/project/repository/web_editor.md#tips) | Start merge request when committing via GitLab's user interface. | +| Create Topics - Repositories | Description | +|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------------------------------------------------| +| [Branches](user/project/repository/branches/index.md) and the [default branch](user/project/repository/branches/index.md#default-branch) | How to use branches in GitLab. | +| [Commits](user/project/repository/index.md#commits) and [signing commits](user/project/repository/gpg_signed_commits/index.md) | Work with commits, and use GPG to sign your commits. | +| [Create branches](user/project/repository/web_editor.md#create-a-new-branch), [create](user/project/repository/web_editor.md#create-a-file)
and [upload](user/project/repository/web_editor.md#upload-a-file) files, and [create directories](user/project/repository/web_editor.md#create-a-directory) | Create branches, create and upload files, and create directories within GitLab. | +| [Delete merged branches](user/project/repository/branches/index.md#delete-merged-branches) | Bulk delete branches after their changes are merged. | +| [File templates](user/project/repository/web_editor.md#template-dropdowns) | File templates for common files. | +| [Files](user/project/repository/index.md#files) | Files management. | +| [Jupyter Notebook files](user/project/repository/index.md#jupyter-notebook-files) | GitLab's support for `.ipynb` files. | +| [Protected branches](user/project/protected_branches.md) | Use protected branches. | +| [Repositories](user/project/repository/index.md) | Manage source code repositories in GitLab's user interface. | +| [Start a merge request](user/project/repository/web_editor.md#tips) | Start merge request when committing via GitLab's user interface. |
@@ -268,15 +268,15 @@ configuration. Then customize everything from buildpacks to CI/CD. The following documentation relates to the DevOps **Configure** stage: -| Configure Topics | Description | -|:-------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------------------------------------------| -| [Auto DevOps](topics/autodevops/index.md) | Automatically employ a complete DevOps lifecycle. | -| [Easy creation of Kubernetes clusters on GKE](user/project/clusters/index.md#adding-and-creating-a-new-gke-cluster-via-gitlab) | Use Google Kubernetes Engine and GitLab. | -| [Executable Runbooks](user/project/clusters/runbooks/index.md) | Documented procedures that explain how to carry out particular processes. | -| [Installing Applications](user/project/clusters/index.md#installing-applications) | Deploy Helm, Ingress, and Prometheus on Kubernetes. | -| [Mattermost slash commands](user/project/integrations/mattermost_slash_commands.md) | Enable and use slash commands from within Mattermost. | -| [Protected variables](ci/variables/README.md#protected-variables) | Restrict variables to protected branches and tags. | -| [Slack slash commands](user/project/integrations/slack_slash_commands.md) | Enable and use slash commands from within Slack. | +| Configure Topics | Description | +|:-----------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------------------------------------------| +| [Auto DevOps](topics/autodevops/index.md) | Automatically employ a complete DevOps lifecycle. | +| [Easy creation of Kubernetes
clusters on GKE](user/project/clusters/index.md#adding-and-creating-a-new-gke-cluster-via-gitlab) | Use Google Kubernetes Engine and GitLab. | +| [Executable Runbooks](user/project/clusters/runbooks/index.md) | Documented procedures that explain how to carry out particular processes. | +| [Installing Applications](user/project/clusters/index.md#installing-applications) | Deploy Helm, Ingress, and Prometheus on Kubernetes. | +| [Mattermost slash commands](user/project/integrations/mattermost_slash_commands.md) | Enable and use slash commands from within Mattermost. | +| [Protected variables](ci/variables/README.md#protected-variables) | Restrict variables to protected branches and tags. | +| [Slack slash commands](user/project/integrations/slack_slash_commands.md) | Enable and use slash commands from within Slack. |
-- cgit v1.2.1 From e6297a029fe21730961bf0e01224f6a9dbf8a8c4 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 28 Nov 2018 08:29:46 +0000 Subject: Use #update! over #update to make errors visible --- app/models/pool_repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/pool_repository.rb b/app/models/pool_repository.rb index 4b697d1991f..7351674201e 100644 --- a/app/models/pool_repository.rb +++ b/app/models/pool_repository.rb @@ -19,7 +19,7 @@ class PoolRepository < ActiveRecord::Base private def correct_disk_path - update(disk_path: storage.disk_path) + update!(disk_path: storage.disk_path) end def storage -- cgit v1.2.1 From d7ef517bba7bd14eb280fbadad96a1b434785800 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 28 Nov 2018 10:11:35 +0000 Subject: Adds margin between tags when job is stuck Removes extra comma from text --- app/assets/javascripts/jobs/components/stuck_block.vue | 10 ++++++---- changelogs/unreleased/54571-runner-tags.yml | 5 +++++ locale/gitlab.pot | 6 +++--- 3 files changed, 14 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/54571-runner-tags.yml diff --git a/app/assets/javascripts/jobs/components/stuck_block.vue b/app/assets/javascripts/jobs/components/stuck_block.vue index 7b077d5e621..ec52d272168 100644 --- a/app/assets/javascripts/jobs/components/stuck_block.vue +++ b/app/assets/javascripts/jobs/components/stuck_block.vue @@ -28,20 +28,22 @@ export default {

{{ - s__(`This job is stuck, because you don't have + s__(`This job is stuck because you don't have any active runners online with any of these tags assigned to them:`) }} - {{ tag }} + + {{ tag }} +

{{ - s__(`Job|This job is stuck, because the project + s__(`Job|This job is stuck because the project doesn't have any runners online assigned to it.`) }}

{{ - s__(`This job is stuck, because you don't + s__(`This job is stuck because you don't have any active runners that can run this job.`) }}

diff --git a/changelogs/unreleased/54571-runner-tags.yml b/changelogs/unreleased/54571-runner-tags.yml new file mode 100644 index 00000000000..1bb19d22e9c --- /dev/null +++ b/changelogs/unreleased/54571-runner-tags.yml @@ -0,0 +1,5 @@ +--- +title: Adds margins between tags when a job is stuck +merge_request: +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 948ace91a48..4f304b189af 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3640,7 +3640,7 @@ msgstr "" msgid "Job|The artifacts will be removed in" msgstr "" -msgid "Job|This job is stuck, because the project doesn't have any runners online assigned to it." +msgid "Job|This job is stuck because the project doesn't have any runners online assigned to it." msgstr "" msgid "Jul" @@ -6512,10 +6512,10 @@ msgstr "" msgid "This job is in pending state and is waiting to be picked by a runner" msgstr "" -msgid "This job is stuck, because you don't have any active runners online with any of these tags assigned to them:" +msgid "This job is stuck because you don't have any active runners online with any of these tags assigned to them:" msgstr "" -msgid "This job is stuck, because you don't have any active runners that can run this job." +msgid "This job is stuck because you don't have any active runners that can run this job." msgstr "" msgid "This job is the most recent deployment to %{link}." -- cgit v1.2.1 From 482744d97b22187a0b1597af2213faf042b5cc15 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Tue, 27 Nov 2018 15:39:00 +0100 Subject: Update Knative IP address docs Since we now load the IP address automatically for you per https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23162 we no longer need to mention that it can only be fetched manually. Also since Knative/Serverless features are still in Alpha I did not feel it was necessary to document that this was only available after a certain version. --- doc/user/project/clusters/index.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md index 6735710e2bb..2aa7c7ef815 100644 --- a/doc/user/project/clusters/index.md +++ b/doc/user/project/clusters/index.md @@ -238,13 +238,10 @@ by GitLab before installing any of the above applications. ## Getting the external IP address NOTE: **Note:** -You need a load balancer installed in your cluster in order to obtain the -external IP address with the following procedure. It can be deployed using the -[**Ingress** application](#installing-applications). - -NOTE: **Note:** -Knative will include its own load balancer in the form of [Istio](https://istio.io). -At this time, to determine the external IP address, you will need to follow the manual approach. +With the following procedure, a load balancer must be installed in your cluster +to obtain the external IP address. You can use either +[Ingress](#installing-applications), or Knative's own load balancer +([Istio](https://istio.io)) if using [Knative](#installing-applications). In order to publish your web application, you first need to find the external IP address associated to your load balancer. @@ -253,7 +250,7 @@ address associated to your load balancer. > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17052) in GitLab 10.6. -If you installed the Ingress [via the **Applications**](#installing-applications), +If you [installed Ingress or Knative](#installing-applications), you should see the Ingress IP address on this same page within a few minutes. If you don't see this, GitLab might not be able to determine the IP address of your ingress application in which case you should manually determine it. -- cgit v1.2.1 From 329a890f8baebb2da3b30d4b2fdfc0cc1f96b5e6 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 26 Nov 2018 15:44:13 +0000 Subject: Resolves N+1 RPC call for Project#readme_url Caches repository.path into Repository#readme_path --- app/models/project.rb | 6 ++-- app/models/repository.rb | 9 +++-- .../51061-readme-url-n-1-rpc-call-resolved.yml | 5 +++ spec/models/repository_spec.rb | 40 +++++++++++++++++++++- 4 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml diff --git a/app/models/project.rb b/app/models/project.rb index 4d1917b9ab2..39d5fdd7a7f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -988,9 +988,9 @@ class Project < ActiveRecord::Base end def readme_url - readme = repository.readme - if readme - Gitlab::Routing.url_helpers.project_blob_url(self, File.join(default_branch, readme.path)) + readme_path = repository.readme_path + if readme_path + Gitlab::Routing.url_helpers.project_blob_url(self, File.join(default_branch, readme_path)) end end diff --git a/app/models/repository.rb b/app/models/repository.rb index fff6d4be275..76e10af51d6 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -35,7 +35,7 @@ class Repository # # For example, for entry `:commit_count` there's a method called `commit_count` which # stores its data in the `commit_count` cache key. - CACHED_METHODS = %i(size commit_count rendered_readme contribution_guide + CACHED_METHODS = %i(size commit_count rendered_readme readme_path contribution_guide changelog license_blob license_key gitignore gitlab_ci_yml branch_names tag_names branch_count tag_count avatar exists? root_ref has_visible_content? @@ -48,7 +48,7 @@ class Repository # changed. This Hash maps file types (as returned by Gitlab::FileDetector) to # the corresponding methods to call for refreshing caches. METHOD_CACHES_FOR_FILE_TYPES = { - readme: :rendered_readme, + readme: %i(rendered_readme readme_path), changelog: :changelog, license: %i(license_blob license_key license), contributing: :contribution_guide, @@ -585,6 +585,11 @@ class Repository head_tree&.readme end + def readme_path + readme&.path + end + cache_method :readme_path + def rendered_readme return unless readme diff --git a/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml b/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml new file mode 100644 index 00000000000..86f91fcb427 --- /dev/null +++ b/changelogs/unreleased/51061-readme-url-n-1-rpc-call-resolved.yml @@ -0,0 +1,5 @@ +--- +title: Improves performance of Project#readme_url by caching the README path +merge_request: 23357 +author: +type: performance diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 187283b284b..f09b4b67061 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1488,6 +1488,7 @@ describe Repository do :size, :commit_count, :rendered_readme, + :readme_path, :contribution_guide, :changelog, :license_blob, @@ -1874,6 +1875,42 @@ describe Repository do end end + describe '#readme_path', :use_clean_rails_memory_store_caching do + context 'with a non-existing repository' do + let(:project) { create(:project) } + + it 'returns nil' do + expect(repository.readme_path).to be_nil + end + end + + context 'with an existing repository' do + context 'when no README exists' do + let(:project) { create(:project, :empty_repo) } + + it 'returns nil' do + expect(repository.readme_path).to be_nil + end + end + + context 'when a README exists' do + let(:project) { create(:project, :repository) } + + it 'returns the README' do + expect(repository.readme_path).to eq("README.md") + end + + it 'caches the response' do + expect(repository).to receive(:readme).and_call_original.once + + 2.times do + expect(repository.readme_path).to eq("README.md") + end + end + end + end + end + describe '#expire_statistics_caches' do it 'expires the caches' do expect(repository).to receive(:expire_method_caches) @@ -2042,9 +2079,10 @@ describe Repository do describe '#refresh_method_caches' do it 'refreshes the caches of the given types' do expect(repository).to receive(:expire_method_caches) - .with(%i(rendered_readme license_blob license_key license)) + .with(%i(rendered_readme readme_path license_blob license_key license)) expect(repository).to receive(:rendered_readme) + expect(repository).to receive(:readme_path) expect(repository).to receive(:license_blob) expect(repository).to receive(:license_key) expect(repository).to receive(:license) -- cgit v1.2.1 From eb7c16dee4000525ceacadfac2861ca6e1b01777 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 28 Nov 2018 10:18:54 +0000 Subject: Adds bottom margin to the environment info block --- .../javascripts/jobs/components/environments_block.vue | 2 +- spec/features/projects/jobs_spec.rb | 10 +++++----- spec/javascripts/jobs/components/job_app_spec.js | 12 +++--------- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/jobs/components/environments_block.vue b/app/assets/javascripts/jobs/components/environments_block.vue index 2d09cf5760f..f7fbb9503a0 100644 --- a/app/assets/javascripts/jobs/components/environments_block.vue +++ b/app/assets/javascripts/jobs/components/environments_block.vue @@ -128,7 +128,7 @@ export default { };