Commit 5e400e08 authored by David Fernandez's avatar David Fernandez

Check for NPM packages before path updates

NPM packages presence is checked before these actions:
* group at the root namespace has a path update
* subgroup is transfered to a different root namespace
* project is transfered to a different root namespace
parent 47472ce5
...@@ -13,7 +13,7 @@ module Groups ...@@ -13,7 +13,7 @@ module Groups
TransferError = Class.new(StandardError) TransferError = Class.new(StandardError)
attr_reader :error attr_reader :error, :new_parent_group
def initialize(group, user, params = {}) def initialize(group, user, params = {})
super super
...@@ -115,3 +115,5 @@ module Groups ...@@ -115,3 +115,5 @@ module Groups
end end
end end
end end
Groups::TransferService.prepend_if_ee('EE::Groups::TransferService')
...@@ -13,6 +13,8 @@ module Projects ...@@ -13,6 +13,8 @@ module Projects
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
TransferError = Class.new(StandardError) TransferError = Class.new(StandardError)
attr_reader :new_namespace
def execute(new_namespace) def execute(new_namespace)
@new_namespace = new_namespace @new_namespace = new_namespace
......
...@@ -42,6 +42,9 @@ it is not possible due to a naming collision. For example: ...@@ -42,6 +42,9 @@ it is not possible due to a naming collision. For example:
| `gitlab-org/gitlab` | `@gitlab-org/gitlab` | Yes | | `gitlab-org/gitlab` | `@gitlab-org/gitlab` | Yes |
| `gitlab-org/gitlab` | `@foo/bar` | No | | `gitlab-org/gitlab` | `@foo/bar` | No |
CAUTION: **When updating the path of a user/group or transferring a (sub)group/project:**
If you update the root namespace of a project with NPM packages, your changes will be rejected. To be allowed to do that, make sure to remove any NPM package first. Don't forget to update your `.npmrc` files to follow the above naming convention and run `npm publish` if necessary.
Now, you can configure your project to authenticate with the GitLab NPM Now, you can configure your project to authenticate with the GitLab NPM
Registry. Registry.
......
...@@ -4,9 +4,10 @@ module Packages ...@@ -4,9 +4,10 @@ module Packages
class GroupPackagesFinder class GroupPackagesFinder
attr_reader :current_user, :group attr_reader :current_user, :group
def initialize(current_user, group) def initialize(current_user, group, params = {})
@current_user = current_user @current_user = current_user
@group = group @group = group
@params = params
end end
def execute def execute
...@@ -18,7 +19,11 @@ module Packages ...@@ -18,7 +19,11 @@ module Packages
private private
def packages_for_group_projects def packages_for_group_projects
::Packages::Package.for_projects(group_projects_visible_to_current_user) packages = ::Packages::Package.for_projects(group_projects_visible_to_current_user)
return packages unless package_type
packages.with_package_type(package_type)
end end
def group_projects_visible_to_current_user def group_projects_visible_to_current_user
...@@ -26,5 +31,9 @@ module Packages ...@@ -26,5 +31,9 @@ module Packages
.in_namespace(group.self_and_descendants.select(:id)) .in_namespace(group.self_and_descendants.select(:id))
.public_or_visible_to_user(current_user, Gitlab::Access::REPORTER) .public_or_visible_to_user(current_user, Gitlab::Access::REPORTER)
end end
def package_type
@params[:package_type].presence
end
end end
end end
...@@ -668,6 +668,12 @@ module EE ...@@ -668,6 +668,12 @@ module EE
marked_for_deletion_at.present? marked_for_deletion_at.present?
end end
def has_packages?(package_type)
return false unless feature_available?(:packages)
packages.where(package_type: package_type).exists?
end
private private
def set_override_pull_mirror_available def set_override_pull_mirror_available
......
...@@ -23,6 +23,7 @@ class Packages::Package < ApplicationRecord ...@@ -23,6 +23,7 @@ class Packages::Package < ApplicationRecord
scope :with_name, ->(name) { where(name: name) } scope :with_name, ->(name) { where(name: name) }
scope :with_name_like, ->(name) { where(arel_table[:name].matches(name)) } scope :with_name_like, ->(name) { where(arel_table[:name].matches(name)) }
scope :with_version, ->(version) { where(version: version) } scope :with_version, ->(version) { where(version: version) }
scope :with_package_type, ->(package_type) { where(package_type: package_type) }
scope :has_version, -> { where.not(version: nil) } scope :has_version, -> { where.not(version: nil) }
scope :preload_files, -> { preload(:package_files) } scope :preload_files, -> { preload(:package_files) }
scope :last_of_each_version, -> { where(id: all.select('MAX(id) AS id').group(:version)) } scope :last_of_each_version, -> { where(id: all.select('MAX(id) AS id').group(:version)) }
......
# frozen_string_literal: true
module EE
module Groups
module TransferService
extend ::Gitlab::Utils::Override
EE_ERROR_MESSAGES = {
group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
}.freeze
private
override :ensure_allowed_transfer
def ensure_allowed_transfer
super
return unless group.packages_feature_available?
npm_packages = Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
if !new_parent_group_has_same_root_ancestor? && npm_packages.exists?
raise_ee_transfer_error(:group_contains_npm_packages)
end
end
def raise_ee_transfer_error(message)
raise ::Groups::TransferService::TransferError, EE_ERROR_MESSAGES[message]
end
def new_parent_group_has_same_root_ancestor?
group.root_ancestor == new_parent_group.root_ancestor
end
end
end
end
...@@ -12,6 +12,8 @@ module EE ...@@ -12,6 +12,8 @@ module EE
return false if group.errors.present? return false if group.errors.present?
end end
return false unless valid_path_change_with_npm_packages?
handle_changes handle_changes
remove_insight_if_insight_project_absent remove_insight_if_insight_project_absent
...@@ -108,6 +110,19 @@ module EE ...@@ -108,6 +110,19 @@ module EE
end end
end end
def valid_path_change_with_npm_packages?
return true unless group.packages_feature_available?
return true if !group.has_parent? && group.path == params[:path]
npm_packages = Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
if npm_packages.exists?
group.errors.add(:path, s_('GroupSettings|cannot change when group contains projects with NPM packages'))
return
end
true
end
def allowed_domain_params def allowed_domain_params
@allowed_domain_params ||= params[:allowed_email_domain_attributes] @allowed_domain_params ||= params[:allowed_email_domain_attributes]
end end
......
...@@ -19,6 +19,19 @@ module EE ...@@ -19,6 +19,19 @@ module EE
old_path_with_namespace: @old_path # rubocop:disable Gitlab/ModuleWithInstanceVariables old_path_with_namespace: @old_path # rubocop:disable Gitlab/ModuleWithInstanceVariables
).create! ).create!
end end
override :transfer
def transfer(project)
if project.feature_available?(:packages) && project.has_packages?(:npm) && !new_namespace_has_same_root?(project)
raise ::Projects::TransferService::TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages"))
end
super
end
def new_namespace_has_same_root?(project)
new_namespace.root_ancestor == project.namespace.root_ancestor
end
end end
end end
end end
---
title: Add messages to warn and stop users when attempting to change the path of projects with NPM packages
merge_request: 18515
author:
type: fixed
...@@ -2,41 +2,59 @@ ...@@ -2,41 +2,59 @@
require 'spec_helper' require 'spec_helper'
describe Packages::GroupPackagesFinder do describe Packages::GroupPackagesFinder do
set(:user) { create(:user) } let_it_be(:user) { create(:user) }
set(:group) { create(:group) } let_it_be(:group) { create(:group) }
set(:project) { create(:project, namespace: group) } let_it_be(:project) { create(:project, namespace: group) }
set(:package1) { create(:maven_package, project: project) }
set(:package2) { create(:maven_package, project: project) }
set(:package3) { create(:maven_package) }
set(:another_group) { create(:group) }
before do before do
group.add_developer(user) group.add_developer(user)
end end
describe '#execute' do describe '#execute' do
context 'group has packages' do subject { described_class.new(user, group).execute }
it 'returns group packages' do
finder = described_class.new(user, group) shared_examples 'with package type' do |package_type|
subject { described_class.new(user, group, package_type: package_type).execute }
expect(finder.execute).to match_array([package1, package2]) it { is_expected.to match_array([send("package_#{package_type}")]) }
end end
def self.package_types
@package_types ||= Packages::Package.package_types.keys
end end
context 'group has no packages' do context 'group has packages' do
it 'returns an empty collection' do let(:package1) { create(:maven_package, project: project) }
finder = described_class.new(user, another_group) let(:package2) { create(:maven_package, project: project) }
let_it_be(:package3) { create(:maven_package) }
it { is_expected.to match_array([package1, package2]) }
end
expect(finder.execute).to be_empty context 'group has package of all types' do
package_types.each { |pt| let!("package_#{pt}") { create("#{pt}_package", project: project) } }
package_types.each do |package_type|
it_behaves_like 'with package type', package_type
end
end end
context 'group has no packages' do
it { is_expected.to be_empty }
end end
context 'group is nil' do context 'group is nil' do
it 'returns an empty collection' do subject { described_class.new(user, nil).execute }
finder = described_class.new(user, nil)
expect(finder.execute).to be_empty it { is_expected.to be_empty}
end end
context 'package type is nil' do
let_it_be(:package1) { create(:maven_package, project: project) }
subject { described_class.new(user, group, package_type: nil).execute }
it { is_expected.to match_array([package1])}
end end
end end
end end
...@@ -2263,4 +2263,61 @@ describe Project do ...@@ -2263,4 +2263,61 @@ describe Project do
end end
end end
end end
describe '#has_packages?' do
let(:project) { create(:project, :public) }
subject { project.has_packages?(package_type) }
shared_examples 'returning true examples' do
let!(:package) { create("#{package_type}_package", project: project) }
it { is_expected.to be true }
end
shared_examples 'returning false examples' do
it { is_expected.to be false }
end
context 'with packages disabled' do
before do
stub_licensed_features(packages: false)
end
it_behaves_like 'returning false examples' do
let!(:package) { create(:maven_package, project: project) }
let(:package_type) { :maven }
end
end
context 'with packages enabled' do
before do
stub_licensed_features(packages: true)
end
context 'with maven packages' do
it_behaves_like 'returning true examples' do
let(:package_type) { :maven }
end
end
context 'with npm packages' do
it_behaves_like 'returning true examples' do
let(:package_type) { :npm }
end
end
context 'with conan packages' do
it_behaves_like 'returning true examples' do
let(:package_type) { :conan }
end
end
context 'with no package type' do
it_behaves_like 'returning false examples' do
let(:package_type) { nil }
end
end
end
end
end end
# frozen_string_literal: true
require 'spec_helper'
describe Groups::TransferService, '#execute' do
let(:user) { create(:user) }
let(:group) { create(:group, :public) }
let(:project) { create(:project, :public, namespace: group) }
let(:new_group) { create(:group, :public) }
let(:transfer_service) { described_class.new(group, user) }
before do
stub_licensed_features(packages: true)
group.add_owner(user)
new_group.add_owner(user)
end
context 'with an npm package' do
before do
create(:npm_package, project: project)
end
shared_examples 'transfer not allowed' do
it 'does not allow transfer when there is a root namespace change' do
transfer_service.execute(new_group)
expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.')
end
end
it_behaves_like 'transfer not allowed'
context 'with a project within subgroup' do
let(:root_group) { create(:group) }
let(:group) { create(:group, parent: root_group) }
before do
root_group.add_owner(user)
end
it_behaves_like 'transfer not allowed'
context 'without a root namespace change' do
let(:new_group) { create(:group, parent: root_group) }
it 'allows transfer' do
transfer_service.execute(new_group)
expect(transfer_service.error).not_to be
expect(group.parent).to eq(new_group)
end
end
end
end
end
...@@ -132,6 +132,44 @@ describe Groups::UpdateService, '#execute' do ...@@ -132,6 +132,44 @@ describe Groups::UpdateService, '#execute' do
end end
end end
context 'with project' do
let(:project) { create(:project, namespace: group) }
shared_examples 'transfer not allowed' do
before do
stub_licensed_features(packages: true)
group.add_owner(user)
end
context 'with npm packages' do
let!(:package) { create(:npm_package, project: project) }
it 'does not allow a path update' do
expect(update_group(group, user, path: 'updated')).to be false
expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages')
end
end
end
it_behaves_like 'transfer not allowed'
context 'located in a subgroup' do
let(:subgroup) { create(:group, parent: group) }
let!(:project) { create(:project, namespace: subgroup) }
before do
subgroup.add_owner(user)
end
it_behaves_like 'transfer not allowed'
it 'does allow a path update if there is not a root namespace change' do
expect(update_group(subgroup, user, path: 'updated')).to be true
expect(subgroup.errors[:path]).to be_empty
end
end
end
context 'repository_size_limit assignment as Bytes' do context 'repository_size_limit assignment as Bytes' do
let(:group) { create(:group, :public, repository_size_limit: 0) } let(:group) { create(:group, :public, repository_size_limit: 0) }
......
...@@ -51,4 +51,35 @@ describe Projects::TransferService do ...@@ -51,4 +51,35 @@ describe Projects::TransferService do
end end
end end
end end
context 'with npm packages' do
let!(:package) { create(:npm_package, project: project) }
before do
stub_licensed_features(packages: true)
end
context 'with a root namespace change' do
it 'does not allow the transfer' do
expect(subject.execute(group)).to be false
expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages")
end
end
context 'without a root namespace change' do
let(:root) { create(:group) }
let(:group) { create(:group, parent: root) }
let(:other_group) { create(:group, parent: root) }
let(:project) { create(:project, :repository, namespace: group) }
before do
other_group.add_owner(user)
end
it 'does allow the transfer' do
expect(subject.execute(other_group)).to be true
expect(project.errors[:new_namespace]).to be_empty
end
end
end
end end
...@@ -8492,6 +8492,9 @@ msgstr "" ...@@ -8492,6 +8492,9 @@ msgstr ""
msgid "GroupSettings|cannot be disabled when the parent group \"Share with group lock\" is enabled, except by the owner of the parent group" msgid "GroupSettings|cannot be disabled when the parent group \"Share with group lock\" is enabled, except by the owner of the parent group"
msgstr "" msgstr ""
msgid "GroupSettings|cannot change when group contains projects with NPM packages"
msgstr ""
msgid "GroupSettings|remove the share with group lock from %{ancestor_group_name}" msgid "GroupSettings|remove the share with group lock from %{ancestor_group_name}"
msgstr "" msgstr ""
...@@ -17633,6 +17636,9 @@ msgstr "" ...@@ -17633,6 +17636,9 @@ msgstr ""
msgid "TransferGroup|Database is not supported." msgid "TransferGroup|Database is not supported."
msgstr "" msgstr ""
msgid "TransferGroup|Group contains projects with NPM packages."
msgstr ""
msgid "TransferGroup|Group is already a root group." msgid "TransferGroup|Group is already a root group."
msgstr "" msgstr ""
...@@ -17660,6 +17666,9 @@ msgstr "" ...@@ -17660,6 +17666,9 @@ msgstr ""
msgid "TransferProject|Project with same name or path in target namespace already exists" msgid "TransferProject|Project with same name or path in target namespace already exists"
msgstr "" msgstr ""
msgid "TransferProject|Root namespace can't be updated if project has NPM packages"
msgstr ""
msgid "TransferProject|Transfer failed, please contact an admin." msgid "TransferProject|Transfer failed, please contact an admin."
msgstr "" msgstr ""
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment