diff --git a/CHANGELOG-EE.md b/CHANGELOG-EE.md index b0c839b6c805aef6d82058d55d04388bddc8acf7..66b196c3b084c7ea0471174a524b15d8dcbfdcee 100644 --- a/CHANGELOG-EE.md +++ b/CHANGELOG-EE.md @@ -1,5 +1,20 @@ Please view this file on the master branch, on stable branches it's out of date. +## 11.9.3 (2019-03-27) + +### Security (1 change) + +- Check label_ids parent when updating issue board. + + +## 11.9.2 (2019-03-26) + +### Security (2 changes) + +- Geo - Improve security while redirecting user back to the secondary after a logout & re-login via the primary. +- Check label_ids parent when updating issue board. + + ## 11.9.1 (2019-03-25) ### Fixed (1 change) @@ -222,6 +237,21 @@ Please view this file on the master branch, on stable branches it's out of date. - Creates an EE component for the pipeline graph. +## 11.7.10 (2019-03-28) + +### Security (1 change) + +- Check label_ids parent when updating issue board. + + +## 11.7.8 (2019-03-26) + +### Security (2 changes) + +- Geo - Improve security while redirecting user back to the secondary after a logout & re-login via the primary. +- Check label_ids parent when updating issue board. + + ## 11.7.7 (2019-03-19) - No changes. diff --git a/CHANGELOG.md b/CHANGELOG.md index a4c783d8809c029761d33692a645383683ba754a..7f3704729fc97e0f4b34c8cb6ae737fa58adaa15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.9.3 (2019-03-27) + +- No changes. + +## 11.9.2 (2019-03-26) + +- No changes. + ## 11.9.1 (2019-03-25) ### Fixed (7 changes) @@ -548,6 +556,23 @@ entry. - Creates mixin to reduce code duplication between CE and EE in graph component. +## 11.7.10 (2019-03-28) + +### Security (7 changes) + +- Disallow guest users from accessing Releases. +- Fix PDF.js vulnerability. +- Hide "related branches" when user does not have permission. +- Fix XSS in resolve conflicts form. +- Added rake task for removing EXIF data from existing uploads. +- Disallow updating namespace when updating a project. +- Use UntrustedRegexp for matching refs policy. + + +## 11.7.8 (2019-03-26) + +- No changes. + ## 11.7.7 (2019-03-19) ### Security (2 changes) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8d851bb5733e55e8b003ce20bc385ed68a6025b6..e460917316d9afc0e96dbbb33ab0f5c9e40f56c7 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -47,7 +47,7 @@ def edit end def create - @project = ::Projects::CreateService.new(current_user, project_params).execute + @project = ::Projects::CreateService.new(current_user, project_params(attributes: project_params_create_attributes)).execute if @project.saved? cookies[:issue_board_welcome_hidden] = { path: project_path(@project), value: nil, expires: Time.at(0) } @@ -328,9 +328,9 @@ def load_events end # rubocop: enable CodeReuse/ActiveRecord - def project_params + def project_params(attributes: []) params.require(:project) - .permit(project_params_attributes) + .permit(project_params_attributes + attributes) end def project_params_attributes @@ -349,11 +349,10 @@ def project_params_attributes :last_activity_at, :lfs_enabled, :name, - :namespace_id, :only_allow_merge_if_all_discussions_are_resolved, :only_allow_merge_if_pipeline_succeeds, - :printing_merge_request_link_enabled, :path, + :printing_merge_request_link_enabled, :public_builds, :request_access_enabled, :runners_token, @@ -375,6 +374,10 @@ def project_params_attributes ] end + def project_params_create_attributes + [:namespace_id] + end + def custom_import_params {} end diff --git a/app/models/label.rb b/app/models/label.rb index 024daeb4fae5ab692549dda80868c25f370ea095..c7fff0d393eb0d67bfd683c84cccd03dcf3da7a1 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -133,6 +133,10 @@ def self.min_chars_for_partial_matching 1 end + def self.by_ids(ids) + where(id: ids) + end + def open_issues_count(user = nil) issues_count(user, state: 'opened') end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index f8ce93201edd7d69ccbb162a8d84696632a45db4..38fe38818c95848100ceb1dda7eb45292950daaf 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -70,10 +70,14 @@ def filter_milestone end def filter_labels - filter_labels_in_param(:add_label_ids) - filter_labels_in_param(:remove_label_ids) - filter_labels_in_param(:label_ids) - find_or_create_label_ids + params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) if params[:add_label_ids] + params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) if params[:remove_label_ids] + + if params[:label_ids] + params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids) + elsif params[:labels] + params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id) + end end def filter_labels_in_param(key) @@ -99,6 +103,10 @@ def find_or_create_label_ids end.compact end + def labels_service + @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params) + end + def process_label_ids(attributes, existing_label_ids: nil) label_ids = attributes.delete(:label_ids) add_label_ids = attributes.delete(:add_label_ids) @@ -116,10 +124,6 @@ def process_label_ids(attributes, existing_label_ids: nil) new_label_ids.uniq end - def available_labels - @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id, include_ancestor_groups: true).execute - end - def handle_quick_actions_on_create(issuable) merge_quick_actions_into_params!(issuable) end diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..fe477d96970edcbaf1c2bcefd16ed2ef46ee7e54 --- /dev/null +++ b/app/services/labels/available_labels_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true +module Labels + class AvailableLabelsService + attr_reader :current_user, :parent, :params + + def initialize(current_user, parent, params) + @current_user = current_user + @parent = parent + @params = params + end + + def find_or_create_by_titles + labels = params.delete(:labels) + + return [] unless labels + + labels = labels.split(',') if labels.is_a?(String) + + labels.map do |label_name| + label = Labels::FindOrCreateService.new( + current_user, + parent, + include_ancestor_groups: true, + title: label_name.strip, + available_labels: available_labels + ).execute + + label + end.compact + end + + def filter_labels_ids_in_param(key) + return [] if params[key].to_a.empty? + + # rubocop:disable CodeReuse/ActiveRecord + available_labels.by_ids(params[key]).pluck(:id) + # rubocop:enable CodeReuse/ActiveRecord + end + + private + + def available_labels + @available_labels ||= LabelsFinder.new(current_user, finder_params).execute + end + + def finder_params + params = { include_ancestor_groups: true } + + case parent + when Group + params[:group_id] = parent.id + params[:only_group_labels] = true + when Project + params[:project_id] = parent.id + end + + params + end + end +end diff --git a/changelogs/unreleased/security-mass-assignment-on-project-update.yml b/changelogs/unreleased/security-mass-assignment-on-project-update.yml new file mode 100644 index 0000000000000000000000000000000000000000..93561cd91b3a1ae4169e15620b1a7c0aa36a9296 --- /dev/null +++ b/changelogs/unreleased/security-mass-assignment-on-project-update.yml @@ -0,0 +1,5 @@ +--- +title: Disallow updating namespace when updating a project +merge_request: +author: +type: security diff --git a/ee/app/controllers/ee/sessions_controller.rb b/ee/app/controllers/ee/sessions_controller.rb index e6a6e31cdd02dc57c5b21be3ad584703cc6718a7..11e92e664912a496ec8dc22a8555a193c7c1da25 100644 --- a/ee/app/controllers/ee/sessions_controller.rb +++ b/ee/app/controllers/ee/sessions_controller.rb @@ -14,7 +14,7 @@ def new return super if signed_in? if ::Gitlab::Geo.secondary_with_primary? - redirect_to oauth_geo_auth_url(state: geo_login_state.encode) + redirect_to oauth_geo_auth_url(host: ::Gitlab::Geo.current_node.url, state: geo_login_state.encode) else super end @@ -33,7 +33,7 @@ def gitlab_geo_logout end def geo_login_state - ::Gitlab::Geo::Oauth::LoginState.new(return_to: geo_return_to_after_login) + ::Gitlab::Geo::Oauth::LoginState.new(return_to: sanitize_redirect(geo_return_to_after_login)) end def geo_logout_state diff --git a/ee/app/services/ee/boards/base_service.rb b/ee/app/services/ee/boards/base_service.rb index 591bebf85e74dd8d5f402c071774e3e9383fb393..4670aa3d53338372c36b8b84cd50c9582358ce62 100644 --- a/ee/app/services/ee/boards/base_service.rb +++ b/ee/app/services/ee/boards/base_service.rb @@ -35,20 +35,15 @@ def set_milestone # rubocop: enable CodeReuse/ActiveRecord def set_labels - labels = params.delete(:labels) - - return unless labels - - params[:label_ids] = labels.split(",").map do |label_name| - label = Labels::FindOrCreateService.new( - current_user, - parent, - title: label_name.strip, - include_ancestor_groups: true - ).execute + if params[:label_ids] + params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids) + elsif params[:labels] + params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id) + end + end - label.try(:id) - end.compact + def labels_service + @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params) end end end diff --git a/ee/changelogs/unreleased/security-geo-2019-q1-recurity-assessment.yml b/ee/changelogs/unreleased/security-geo-2019-q1-recurity-assessment.yml new file mode 100644 index 0000000000000000000000000000000000000000..a29b43c30970250d5c5ebef4196577b01752e424 --- /dev/null +++ b/ee/changelogs/unreleased/security-geo-2019-q1-recurity-assessment.yml @@ -0,0 +1,6 @@ +--- +title: Geo - Improve security while redirecting user back to the secondary after a + logout & re-login via the primary +merge_request: +author: +type: security diff --git a/ee/changelogs/unreleased/security-milestone-labels.yml b/ee/changelogs/unreleased/security-milestone-labels.yml new file mode 100644 index 0000000000000000000000000000000000000000..4f8abcbc8bee094e5b04416c36047e7be8ce7b06 --- /dev/null +++ b/ee/changelogs/unreleased/security-milestone-labels.yml @@ -0,0 +1,5 @@ +--- +title: Check label_ids parent when updating issue board +merge_request: +author: +type: security diff --git a/ee/lib/gitlab/geo/oauth/login_state.rb b/ee/lib/gitlab/geo/oauth/login_state.rb index c855f7a381f1d8d30780682e6eccb71e24af6bde..18be2038be57e596a77b4ecc0571c70b3ed5dc0a 100644 --- a/ee/lib/gitlab/geo/oauth/login_state.rb +++ b/ee/lib/gitlab/geo/oauth/login_state.rb @@ -7,40 +7,57 @@ class LoginState attr_reader :return_to def self.from_state(state) - salt, hmac, return_to = state.to_s.split(':', 3) - self.new(salt: salt, hmac: hmac, return_to: return_to) + salt, token, return_to = state.to_s.split(':', 3) + self.new(salt: salt, token: token, return_to: return_to) end - def initialize(return_to:, salt: nil, hmac: nil) - @return_to = return_to + def initialize(return_to:, salt: nil, token: nil) + @return_to = Gitlab::ReturnToLocation.new(return_to).full_path @salt = salt - @hmac = hmac + @token = token end - def valid? - return false unless salt.present? && hmac.present? - - hmac == generate_hmac + def encode + "#{salt}:#{hmac_token}:#{return_to}" end - def encode - "#{salt}:#{generate_hmac}:#{return_to}" + def valid? + return false unless salt.present? && token.present? + + decoded_token = JSONWebToken::HMACToken.decode(token, key).first + secure_compare(decoded_token.dig('data', 'return_to')) + rescue JWT::DecodeError + false end private - attr_reader :hmac + attr_reader :token - def generate_hmac - digest = OpenSSL::Digest::SHA256.new - key = Gitlab::Application.secrets.secret_key_base + salt + def expiration_time + 1.minute + end + + def hmac_token + hmac_token = JSONWebToken::HMACToken.new(key) + hmac_token.expire_time = Time.now + expiration_time + hmac_token[:data] = { return_to: return_to.to_s } + hmac_token.encoded + end - OpenSSL::HMAC.hexdigest(digest, key, return_to.to_s) + def key + ActiveSupport::KeyGenerator + .new(Gitlab::Application.secrets.secret_key_base) + .generate_key(salt) end def salt @salt ||= SecureRandom.hex(8) end + + def secure_compare(value) + ActiveSupport::SecurityUtils.secure_compare(return_to.to_s, value) + end end end end diff --git a/ee/spec/controllers/ee/sessions_controller_spec.rb b/ee/spec/controllers/ee/sessions_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..eed553c720a026ed0f79782304794cd167cae8e4 --- /dev/null +++ b/ee/spec/controllers/ee/sessions_controller_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SessionsController do + include DeviseHelpers + include EE::GeoHelpers + + describe '#new' do + before do + set_devise_mapping(context: @request) + end + + context 'on a Geo secondary node' do + set(:primary_node) { create(:geo_node, :primary) } + set(:secondary_node) { create(:geo_node) } + + before do + stub_current_geo_node(secondary_node) + end + + shared_examples 'a valid oauth authentication redirect' do + it 'redirects to the correct oauth_geo_auth_url' do + get(:new) + + redirect_uri = URI.parse(response.location) + redirect_params = CGI.parse(redirect_uri.query) + + expect(response).to have_gitlab_http_status(302) + expect(response).to redirect_to %r(\A#{primary_node.url}oauth/geo/auth) + expect(redirect_params['state'].first).to end_with(':') + end + end + + context 'with a tampered HOST header' do + before do + request.headers['HOST'] = 'http://this.is.not.my.host' + end + + it_behaves_like 'a valid oauth authentication redirect' + end + + context 'with a tampered X-Forwarded-Host header' do + before do + request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host' + end + + it_behaves_like 'a valid oauth authentication redirect' + end + + context 'without a tampered header' do + it_behaves_like 'a valid oauth authentication redirect' + end + end + end +end diff --git a/ee/spec/controllers/oauth/geo_auth_controller_spec.rb b/ee/spec/controllers/oauth/geo_auth_controller_spec.rb index 6a262365dc05383e98136ae0c4188cb5ce9f69b2..8e23b27057e621ec111e0cfbeb183be8114d0b9b 100644 --- a/ee/spec/controllers/oauth/geo_auth_controller_spec.rb +++ b/ee/spec/controllers/oauth/geo_auth_controller_spec.rb @@ -27,12 +27,34 @@ expect(response).to redirect_to(root_url) end - it "redirects to primary node's oauth endpoint" do - oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state) + shared_examples "a valid redirect to to primary node's oauth endpoint" do + it "redirects to primary node's oauth endpoint" do + oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state) - get :auth, params: { state: login_state } + get :auth, params: { state: login_state } + + expect(response).to redirect_to(oauth_endpoint) + end + end + + context 'without a tampered header' do + it_behaves_like "a valid redirect to to primary node's oauth endpoint" + end + + context 'with a tampered HOST header' do + before do + request.headers['HOST'] = 'http://this.is.not.my.host' + end + + it_behaves_like "a valid redirect to to primary node's oauth endpoint" + end + + context 'with a tampered X-Forwarded-Host header' do + before do + request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host' + end - expect(response).to redirect_to(oauth_endpoint) + it_behaves_like "a valid redirect to to primary node's oauth endpoint" end end @@ -55,16 +77,40 @@ expect(response).to redirect_to(new_user_session_path) end - it 'redirects to redirect_url if state is valid' do - get :callback, params: { state: login_state } + context 'with a valid state' do + shared_examples 'a valid redirect to redirect_url' do + it "redirects to primary node's oauth endpoint" do + get :callback, params: { state: login_state } - expect(response).to redirect_to(secondary_node.url) - end + expect(response).to redirect_to('/') + end + end - it 'does not display a flash message if state is valid' do - get :callback, params: { state: login_state } + context 'without a tampered header' do + it_behaves_like 'a valid redirect to redirect_url' + end + + context 'with a tampered HOST header' do + before do + request.headers['HOST'] = 'http://this.is.not.my.host' + end + + it_behaves_like 'a valid redirect to redirect_url' + end + + context 'with a tampered X-Forwarded-Host header' do + before do + request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host' + end + + it_behaves_like 'a valid redirect to redirect_url' + end + + it 'does not display a flash message' do + get :callback, params: { state: login_state } - expect(controller).to set_flash[:alert].to(nil) + expect(controller).to set_flash[:alert].to(nil) + end end end diff --git a/ee/spec/controllers/projects/boards_controller_spec.rb b/ee/spec/controllers/projects/boards_controller_spec.rb index 08079f06b38ef3bb8ab760d544ba034ea9358feb..df4bbdde147c6938ca3c3090f39e2895a3254f38 100644 --- a/ee/spec/controllers/projects/boards_controller_spec.rb +++ b/ee/spec/controllers/projects/boards_controller_spec.rb @@ -141,7 +141,7 @@ def create_board(board_params) let(:board) { create(:board, project: project, name: 'Backend') } let(:user) { create(:user) } let(:milestone) { create(:milestone, project: project) } - let(:label) { create(:label) } + let(:label) { create(:label, project: project) } let(:update_params) do { name: 'Frontend', diff --git a/ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb b/ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb index e3c4fd2a80b21cd0115ec3b6ed7bf5ed31c5f4c3..b7e3e612a9c9186ecbb0da3e0ae4b32d97059363 100644 --- a/ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb +++ b/ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb @@ -3,9 +3,14 @@ require 'spec_helper' describe Gitlab::Geo::Oauth::LoginState do - let(:salt) { '100d8cbd1750a2bb' } - let(:hmac) { '62fdcface89baab582f33de6672f10499974c28b5cc269795c4830b8b3ab06be' } - let(:oauth_return_to) { 'http://fake-secondary.com:3000/project/test' } + let(:salt) { 'b9653b6aa2ff6b54' } + let(:token) { 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjp7InJldHVybl90byI6Ii9wcm9qZWN0L3Rlc3Q_Zm9vPWJhciN6b28ifSwianRpIjoiODdjZDQ2M2MtOTgyNC00ZjliLWI5NDMtOGFkMjJmY2E2MmZhIiwiaWF0IjoxNTQ5ODI1MjAwLCJuYmYiOjE1NDk4MjUxOTUsImV4cCI6MTU0OTgyNTI2MH0.qZE6kuoeW6BK1URuIl8l8MiCfGjtTTXixVdMCE80gVA' } + let(:return_to) { 'http://fake-secondary.com:3000/project/test?foo=bar#zoo' } + let(:timestamp) { Time.utc(2019, 2, 10, 19, 0, 0) } + + around do |example| + Timecop.freeze(timestamp) { example.run } + end before do allow(Gitlab::Application.secrets).to receive(:secret_key_base) @@ -22,7 +27,7 @@ end it 'returns a valid instance when state is valid' do - expect(described_class.from_state("#{salt}:#{hmac}:#{oauth_return_to}")).to be_valid + expect(described_class.from_state("#{salt}:#{token}:#{return_to}")).to be_valid end end @@ -39,32 +44,42 @@ expect(subject.valid?).to eq false end - it 'returns false when hmac is nil' do - subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: nil) + it 'returns false when token is nil' do + subject = described_class.new(return_to: return_to, salt: salt, token: nil) expect(subject.valid?).to eq false end - it 'returns false when hmac is empty' do - subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: '') + it 'returns false when token is empty' do + subject = described_class.new(return_to: return_to, salt: salt, token: '') expect(subject.valid?).to eq false end it 'returns false when salt not match' do - subject = described_class.new(return_to: oauth_return_to, salt: 'salt', hmac: hmac) + subject = described_class.new(return_to: return_to, salt: 'invalid-salt', token: token) expect(subject.valid?).to eq(false) end - it 'returns false when hmac does not match' do - subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: 'hmac') + it 'returns false when token does not match' do + subject = described_class.new(return_to: return_to, salt: salt, token: 'invalid-token') expect(subject.valid?).to eq(false) end - it 'returns true when hmac matches' do - subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: hmac) + it "returns false when token's expired" do + subject = described_class.new(return_to: return_to, salt: salt, token: token) + + # Needs to be at least 120 seconds, because the default expiry is + # 60 seconds with an additional 60 second leeway. + Timecop.freeze(timestamp + 125) do + expect(subject.valid?).to eq(false) + end + end + + it 'returns true when token matches' do + subject = described_class.new(return_to: return_to, salt: salt, token: token) expect(subject.valid?).to eq(true) end @@ -77,14 +92,34 @@ expect { subject.encode }.not_to raise_error end - it 'returns a string with salt, hmac, and return_to colon separated' do - subject = described_class.new(return_to: oauth_return_to) + it 'returns a string with salt, token, and return_to colon separated' do + subject = described_class.new(return_to: return_to) - salt, hmac, return_to = subject.encode.split(':', 3) + salt, token, return_to = subject.encode.split(':', 3) expect(salt).not_to be_blank - expect(hmac).not_to be_blank - expect(return_to).to eq oauth_return_to + expect(token).not_to be_blank + expect(return_to).to eq return_to + end + end + + describe '#return_to' do + it 'returns nil when return_to is nil' do + subject = described_class.new(return_to: nil) + + expect(subject.return_to).to be_nil + end + + it 'returns an empty string when return_to is empty' do + subject = described_class.new(return_to: '') + + expect(subject.return_to).to eq('') + end + + it 'returns the full path of the return_to URL' do + subject = described_class.new(return_to: return_to) + + expect(subject.return_to).to eq('/project/test?foo=bar#zoo') end end end diff --git a/ee/spec/services/boards/update_service_spec.rb b/ee/spec/services/boards/update_service_spec.rb index 4324611fd71553575d0e7f74adc5f011b9253cd1..1833d931385be16612f1a82c5193f7f172835c21 100644 --- a/ee/spec/services/boards/update_service_spec.rb +++ b/ee/spec/services/boards/update_service_spec.rb @@ -17,7 +17,7 @@ end describe '#execute' do - let(:project) { create(:project) } + let(:project) { create(:project, group: group) } let(:group) { create(:group) } let!(:board) { create(:board, group: group, name: 'Backend') } @@ -44,10 +44,11 @@ it 'updates the configuration params when scoped issue board is enabled' do stub_licensed_features(scoped_issue_board: true) assignee = create(:user) - milestone = create(:milestone, project: project) - label = create(:label, project: project) + milestone = create(:milestone, group: group) + label = create(:group_label, group: board.group) + user = create(:user) - service = described_class.new(project, double, + service = described_class.new(group, user, milestone_id: milestone.id, assignee_id: assignee.id, label_ids: [label.id]) @@ -213,6 +214,18 @@ def expect_label_assigned(user, board, input_labels, expected_labels) expect_label_assigned(user, board, %w{group_label project_label new_label}, %w{group_label project_label}) end end + + context 'when label_ids param is provided' do + it 'updates using only labels accessible by the project board' do + other_project_label = create(:label, title: 'other_project_label') + other_group_label = create(:group_label, title: 'other_group_label') + label_ids = [group_label.id, label.id, other_project_label.id, other_group_label.id] + + described_class.new(board.parent, user, label_ids: label_ids).execute(board) + + expect(board.reload.labels).to contain_exactly(group_label, label) + end + end end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 578b6d814618476a3ae0897ef073411930625896..d2e6a58a71df6735fcdabe7e64f587c6c97d0a96 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -392,6 +392,23 @@ end end + it 'does not update namespace' do + controller.instance_variable_set(:@project, project) + + params = { + namespace_id: 'test' + } + + expect do + put :update, + params: { + namespace_id: project.namespace, + id: project.id, + project: params + } + end.not_to change { project.namespace.reload } + end + def update_project(**parameters) put :update, params: { diff --git a/spec/services/labels/available_labels_service_spec.rb b/spec/services/labels/available_labels_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..4d5c87ecc5367130eb6ca9ceeae8f3a491f4ed05 --- /dev/null +++ b/spec/services/labels/available_labels_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Labels::AvailableLabelsService do + let(:user) { create(:user) } + let(:project) { create(:project, :public, group: group) } + let(:group) { create(:group) } + + let(:project_label) { create(:label, project: project) } + let(:other_project_label) { create(:label) } + let(:group_label) { create(:group_label, group: group) } + let(:other_group_label) { create(:group_label) } + let(:labels) { [project_label, other_project_label, group_label, other_group_label] } + + context '#find_or_create_by_titles' do + let(:label_titles) { labels.map(&:title).push('non existing title') } + + context 'when parent is a project' do + context 'when a user is not a project member' do + it 'returns only relevant label ids' do + result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles + + expect(result).to match_array([project_label, group_label]) + end + end + + context 'when a user is a project member' do + before do + project.add_developer(user) + end + + it 'creates new labels for not found titles' do + result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles + + expect(result.count).to eq(5) + expect(result).to include(project_label, group_label) + expect(result).not_to include(other_project_label, other_group_label) + end + end + end + + context 'when parent is a group' do + context 'when a user is not a group member' do + it 'returns only relevant label ids' do + result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles + + expect(result).to match_array([group_label]) + end + end + + context 'when a user is a group member' do + before do + group.add_developer(user) + end + + it 'creates new labels for not found titles' do + result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles + + expect(result.count).to eq(5) + expect(result).to include(group_label) + expect(result).not_to include(project_label, other_project_label, other_group_label) + end + end + end + end + + context '#filter_labels_ids_in_param' do + let(:label_ids) { labels.map(&:id).push(99999) } + + context 'when parent is a project' do + it 'returns only relevant label ids' do + result = described_class.new(user, project, ids: label_ids).filter_labels_ids_in_param(:ids) + + expect(result).to match_array([project_label.id, group_label.id]) + end + end + + context 'when parent is a group' do + it 'returns only relevant label ids' do + result = described_class.new(user, group, ids: label_ids).filter_labels_ids_in_param(:ids) + + expect(result).to match_array([group_label.id]) + end + end + end +end