From 4fa3a5316972e3157841c4a402dac357980d8921 Mon Sep 17 00:00:00 2001
From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Thu, 4 Dec 2025 15:49:31 +0100
Subject: [PATCH 1/5] Add validations for npm vregs models

Add validations for Registry, Upstream and RegistryUpstream npm virtual
registries models.

Changelog: added
---
 .../packages/npm/registry.rb                  |   7 +-
 .../packages/npm/registry_upstream.rb         |  28 +-
 .../packages/npm/upstream.rb                  | 132 ++++-
 .../packages/npm/registry_spec.rb             |  32 ++
 .../packages/npm/registry_upstream_spec.rb    |  28 ++
 .../packages/npm/upstream_spec.rb             | 454 ++++++++++++++++++
 6 files changed, 673 insertions(+), 8 deletions(-)

diff --git a/ee/app/models/virtual_registries/packages/npm/registry.rb b/ee/app/models/virtual_registries/packages/npm/registry.rb
index 19da238063cc00..dc0837e85aa5d2 100644
--- a/ee/app/models/virtual_registries/packages/npm/registry.rb
+++ b/ee/app/models/virtual_registries/packages/npm/registry.rb
@@ -3,10 +3,9 @@
 module VirtualRegistries
   module Packages
     module Npm
-      class Registry < ApplicationRecord
-        # TODO: remove after making the class inherit from ::VirtualRegistries::Registry
-        # https://gitlab.com/gitlab-org/gitlab/-/work_items/581343
-        belongs_to :group
+      class Registry < ::VirtualRegistries::Registry
+        MAX_REGISTRY_COUNT = 20
+
         has_many :registry_upstreams,
           -> { order(position: :asc) },
           class_name: 'VirtualRegistries::Packages::Npm::RegistryUpstream',
diff --git a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
index b0fc01aa39314a..7207cbd8c3fd39 100644
--- a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
@@ -4,8 +4,8 @@ module VirtualRegistries
   module Packages
     module Npm
       class RegistryUpstream < ApplicationRecord
-        # TODO: remove after making the class inherit from ::VirtualRegistries::RegistryUpstream
-        # https://gitlab.com/gitlab-org/gitlab/-/work_items/581343
+        MAX_UPSTREAMS_COUNT = 20
+
         belongs_to :group
 
         belongs_to :registry,
@@ -14,6 +14,30 @@ class RegistryUpstream < ApplicationRecord
         belongs_to :upstream,
           class_name: '::VirtualRegistries::Packages::Npm::Upstream',
           inverse_of: :registry_upstreams
+
+        validates :upstream_id, uniqueness: { scope: :registry_id }, if: :upstream_id?
+        validates :registry_id, uniqueness: { scope: [:position] }
+
+        validates :group, top_level_group: true, presence: true
+        validates :position,
+          numericality: {
+            only_integer: true,
+            greater_than_or_equal_to: 1,
+            less_than_or_equal_to: ->(record) { record.class::MAX_UPSTREAMS_COUNT }
+          },
+          presence: true
+
+        before_validation :set_group, :set_position, on: :create
+
+        private
+
+        def set_group
+          self.group ||= (registry || upstream).group
+        end
+
+        def set_position
+          self.position = self.class.where(registry:, group:).maximum(:position).to_i + 1
+        end
       end
     end
   end
diff --git a/ee/app/models/virtual_registries/packages/npm/upstream.rb b/ee/app/models/virtual_registries/packages/npm/upstream.rb
index a6ca0027c0281f..fe4058d42c9adf 100644
--- a/ee/app/models/virtual_registries/packages/npm/upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/upstream.rb
@@ -4,8 +4,16 @@ module VirtualRegistries
   module Packages
     module Npm
       class Upstream < ApplicationRecord
-        # TODO: remove after making the class inherit from ::VirtualRegistries::Upstream
-        # https://gitlab.com/gitlab-org/gitlab/-/work_items/581343
+        include Gitlab::Utils::StrongMemoize
+
+        NPMJS_REGISTRY_URL = 'https://registry.npmjs.org'
+        TRAILING_SLASHES_REGEX = %r{/+$}
+
+        SAME_URL_AND_CREDENTIALS_ERROR = 'already has a remote upstream with the same url and credentials'
+        SAME_LOCAL_PROJECT_OR_GROUP_ERROR = 'already has a local upstream with the same target project or group'
+
+        ALLOWED_GLOBAL_ID_CLASSES = [::Project, ::Group].freeze
+
         belongs_to :group
 
         has_many :registry_upstreams,
@@ -15,6 +23,126 @@ class Upstream < ApplicationRecord
         has_many :registries, class_name: 'VirtualRegistries::Packages::Npm::Registry', through: :registry_upstreams
 
         encrypts :username, :password
+
+        validates :group, top_level_group: true, presence: true
+        validates :url, presence: true, length: { maximum: 255 }
+        validates :username, :password, length: { maximum: 510 }
+        validates :cache_validity_hours, numericality: { greater_than_or_equal_to: 0, only_integer: true }
+        validates :metadata_cache_validity_hours, numericality: { greater_than: 0, only_integer: true }
+        validates :name, presence: true, length: { maximum: 255 }
+        validates :description, length: { maximum: 1024 }
+        validate :credentials_uniqueness_for_group, if: -> { %i[url username password].any? { |f| changes.key?(f) } }
+
+        # remote validations
+        validates :url, addressable_url: {
+          allow_localhost: false,
+          allow_local_network: false,
+          dns_rebind_protection: true,
+          enforce_sanitization: true
+        }, if: :remote?
+        validates :username, presence: true, if: -> { remote? && password? }
+        validates :password, presence: true, if: -> { remote? && username? }
+
+        # local validations
+        with_options if: :local? do
+          validates :username, absence: true
+          validates :password, absence: true
+          validate :ensure_local_project_or_local_group
+        end
+
+        before_validation :normalize_url, if: -> { url? && remote? }
+        before_validation :set_cache_validity_hours_for_npmjs_registry, if: :url?, on: :create
+        after_validation :reset_credentials, if: -> { persisted? && url_changed? }
+
+        prevent_from_serialization(:password) if respond_to?(:prevent_from_serialization)
+
+        scope :for_group, ->(group) { where(group:) }
+
+        def local_project?
+          return false unless local?
+
+          local_global_id&.model_class == Project
+        end
+
+        def local_project_id
+          return unless local_project?
+
+          local_global_id&.model_id&.to_i
+        end
+
+        def local_group?
+          return false unless local?
+
+          local_global_id&.model_class == Group
+        end
+
+        def local_group_id
+          return unless local_group?
+
+          local_global_id&.model_id&.to_i
+        end
+
+        def url=(value)
+          super
+
+          clear_memoization(:local_global_id)
+        end
+
+        def local?
+          !!url&.start_with?('gid://')
+        end
+
+        def remote?
+          !local?
+        end
+
+        private
+
+        def normalize_url
+          self.url = url.sub(TRAILING_SLASHES_REGEX, '')
+        end
+
+        def reset_credentials
+          return if username_changed? && password_changed?
+
+          self.username = nil
+          self.password = nil
+        end
+
+        def set_cache_validity_hours_for_npmjs_registry
+          return unless url.start_with?(NPMJS_REGISTRY_URL)
+
+          self.cache_validity_hours = 0
+        end
+
+        def credentials_uniqueness_for_group
+          return unless group
+
+          return if self.class.for_group(group)
+            .select(:username, :password)
+            .then { |q| new_record? ? q : q.where.not(id:) }
+            .where(url:)
+            .none? { |u| u.username == username && Rack::Utils.secure_compare(u.password.to_s, password.to_s) }
+
+          errors.add(:group, remote? ? SAME_URL_AND_CREDENTIALS_ERROR : SAME_LOCAL_PROJECT_OR_GROUP_ERROR)
+        end
+
+        def ensure_local_project_or_local_group
+          return errors.add(:url, 'is invalid') unless local_global_id
+
+          unless local_global_id.model_class.in?(ALLOWED_GLOBAL_ID_CLASSES)
+            return errors.add(:url, 'should point to a Project or Group')
+          end
+
+          return if local_global_id.model_class.exists?(local_global_id.model_id)
+
+          errors.add(:url, "should point to an existing #{local_global_id.model_class.name}")
+        end
+
+        def local_global_id
+          GlobalID.parse(url)
+        end
+        strong_memoize_attr :local_global_id
       end
     end
   end
diff --git a/ee/spec/models/virtual_registries/packages/npm/registry_spec.rb b/ee/spec/models/virtual_registries/packages/npm/registry_spec.rb
index 883bc4539a069f..a02deabad85b26 100644
--- a/ee/spec/models/virtual_registries/packages/npm/registry_spec.rb
+++ b/ee/spec/models/virtual_registries/packages/npm/registry_spec.rb
@@ -21,7 +21,39 @@
     end
   end
 
+  describe 'validations', :aggregate_failures do
+    it { is_expected.to validate_presence_of(:group) }
+    it { is_expected.to validate_presence_of(:name) }
+    it { is_expected.to validate_length_of(:name).is_at_most(255) }
+    it { is_expected.to validate_length_of(:description).is_at_most(1024) }
+    it { is_expected.to validate_uniqueness_of(:group_id).scoped_to(:name) }
+  end
+
+  describe '.for_group' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:registry) { create(:virtual_registries_packages_npm_registry, group: group) }
+    let_it_be(:other_registry) { create(:virtual_registries_packages_npm_registry) }
+
+    subject { described_class.for_group(group) }
+
+    it { is_expected.to eq([registry]) }
+  end
+
   it_behaves_like 'virtual registries: upstreams ordering',
     registry_factory: :virtual_registries_packages_npm_registry,
     upstream_factory: :virtual_registries_packages_npm_upstream
+
+  it_behaves_like 'virtual registries: group registry limit',
+    registry_factory: :virtual_registries_packages_npm_registry
+
+  it_behaves_like 'virtual registries: has exclusive upstreams',
+    registry_factory: :virtual_registries_packages_npm_registry,
+    upstream_factory: :virtual_registries_packages_npm_upstream
+
+  it_behaves_like 'virtual registries: registry destruction',
+    registry_factory: :virtual_registries_packages_npm_registry,
+    upstream_factory: :virtual_registries_packages_npm_upstream,
+    registry_upstream_factory: :virtual_registries_packages_npm_registry_upstream,
+    upstream_class: VirtualRegistries::Packages::Npm::Upstream,
+    registry_upstream_class: VirtualRegistries::Packages::Npm::RegistryUpstream
 end
diff --git a/ee/spec/models/virtual_registries/packages/npm/registry_upstream_spec.rb b/ee/spec/models/virtual_registries/packages/npm/registry_upstream_spec.rb
index 8d91711ce4d1af..4d60674e4c1d98 100644
--- a/ee/spec/models/virtual_registries/packages/npm/registry_upstream_spec.rb
+++ b/ee/spec/models/virtual_registries/packages/npm/registry_upstream_spec.rb
@@ -20,4 +20,32 @@
         .inverse_of(:registry_upstreams)
     end
   end
+
+  describe 'validations', :aggregate_failures do
+    subject(:registry_upstream) { create(:virtual_registries_packages_npm_registry_upstream) }
+
+    it { is_expected.to validate_presence_of(:group) }
+    it { is_expected.to validate_presence_of(:position) }
+    it { is_expected.to validate_uniqueness_of(:upstream_id).scoped_to(:registry_id) }
+
+    it 'validates position' do
+      is_expected.to validate_numericality_of(:position)
+        .is_greater_than_or_equal_to(1)
+        .is_less_than_or_equal_to(20)
+        .only_integer
+    end
+
+    # position is set before validation on create. Thus, we need to check the registry_id uniqueness validation
+    # manually with two records that are already persisted.
+    context 'for registry_id uniqueness' do
+      let_it_be(:other_registry_upstream) { create(:virtual_registries_packages_npm_registry_upstream) }
+
+      it 'validates it' do
+        other_registry_upstream.assign_attributes(registry_upstream.attributes)
+
+        expect(other_registry_upstream.valid?).to be_falsey
+        expect(other_registry_upstream.errors[:registry_id].first).to eq('has already been taken')
+      end
+    end
+  end
 end
diff --git a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
index 13cbf0be1d9c1f..ce84165ba9837e 100644
--- a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
+++ b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
@@ -3,6 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe VirtualRegistries::Packages::Npm::Upstream, feature_category: :virtual_registry do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:other_project) { create(:project) }
+  let_it_be(:other_project_global_id) { other_project.to_global_id.to_s }
+  let_it_be(:other_group) { create(:group) }
+  let_it_be(:other_group_global_id) { other_group.to_global_id.to_s }
+
   subject(:upstream) { build(:virtual_registries_packages_npm_upstream) }
 
   describe 'associations', :aggregate_failures do
@@ -19,4 +26,451 @@
         .class_name('VirtualRegistries::Packages::Npm::Registry')
     end
   end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:group) }
+    it { is_expected.to validate_presence_of(:url) }
+    it { is_expected.to validate_length_of(:url).is_at_most(255) }
+    it { is_expected.to validate_length_of(:username).is_at_most(510) }
+    it { is_expected.to validate_length_of(:password).is_at_most(510) }
+    it { is_expected.to validate_numericality_of(:cache_validity_hours).only_integer.is_greater_than_or_equal_to(0) }
+    it { is_expected.to validate_numericality_of(:metadata_cache_validity_hours).only_integer.is_greater_than(0) }
+    it { is_expected.to validate_presence_of(:name) }
+    it { is_expected.to validate_length_of(:name).is_at_most(255) }
+    it { is_expected.to validate_length_of(:description).is_at_most(1024) }
+
+    context 'for remote upstream' do
+      it { is_expected.to validate_presence_of(:username) }
+      it { is_expected.to validate_presence_of(:password) }
+
+      context 'for url' do
+        where(:url, :valid, :error_messages) do
+          'http://test.npm'     | true  | nil
+          'https://test.npm'    | true  | nil
+          'git://test.npm'      | false | ['Url is blocked: Only allowed schemes are http, https']
+          nil                   | false | ["Url can't be blank", 'Url must be a valid URL']
+          ''                    | false | ["Url can't be blank", 'Url must be a valid URL']
+          "http://#{'a' * 255}" | false | 'Url is too long (maximum is 255 characters)'
+          'http://127.0.0.1'    | false | 'Url is blocked: Requests to localhost are not allowed'
+          'npm.local'           | false | 'Url is blocked: Only allowed schemes are http, https'
+          'http://192.168.1.2'  | false | 'Url is blocked: Requests to the local network are not allowed'
+          'http://foobar.x'     | false | 'Url is blocked: Host cannot be resolved or invalid'
+        end
+
+        with_them do
+          before do
+            upstream.url = url
+          end
+
+          if params[:valid]
+            it { is_expected.to be_valid }
+          else
+            it { is_expected.to be_invalid.and have_attributes(errors: match_array(Array.wrap(error_messages))) }
+          end
+        end
+
+        context 'for normalization' do
+          where(:url_to_set, :expected_url) do
+            'http://test.npm'       | 'http://test.npm'
+            'http://test.npm/'      | 'http://test.npm'
+            'http://test.npm//'     | 'http://test.npm'
+            'http://test.npm/path'  | 'http://test.npm/path'
+            'http://test.npm/path/' | 'http://test.npm/path'
+          end
+
+          with_them do
+            before do
+              upstream.url = url_to_set
+            end
+
+            it { is_expected.to be_valid.and have_attributes(url: expected_url) }
+          end
+
+          context 'when creating upstreams with same URL but different trailing slashes' do
+            let_it_be(:group) { create(:group) }
+
+            it 'prevents duplicate upstreams with trailing slash' do
+              create(:virtual_registries_packages_npm_upstream, url: 'http://test.npm', group: group)
+
+              duplicate = build(:virtual_registries_packages_npm_upstream, url: 'http://test.npm/', group: group)
+
+              expect(duplicate).to be_invalid.and have_attributes(
+                errors: match_array(Array.wrap(
+                  'Group already has a remote upstream with the same url and credentials'
+                )),
+                url: 'http://test.npm'
+              )
+            end
+          end
+        end
+      end
+
+      context 'for credentials' do
+        where(:username, :password, :valid, :error_message) do
+          'user'      | 'password'   | true  | nil
+          ''          | ''           | true  | nil
+          ''          | nil          | true  | nil
+          nil         | ''           | true  | nil
+          nil         | 'password'   | false | "Username can't be blank"
+          'user'      | nil          | false | "Password can't be blank"
+          ''          | 'password'   | false | "Username can't be blank"
+          'user'      | ''           | false | "Password can't be blank"
+          ('a' * 511) | 'password'   | false | 'Username is too long (maximum is 510 characters)'
+          'user'      | ('a' * 511)  | false | 'Password is too long (maximum is 510 characters)'
+        end
+
+        with_them do
+          before do
+            upstream.assign_attributes(username:, password:)
+          end
+
+          if params[:valid]
+            it { is_expected.to be_valid }
+          else
+            it { is_expected.to be_invalid.and have_attributes(errors: match_array(Array.wrap(error_message))) }
+          end
+        end
+
+        context 'when url is updated' do
+          where(:new_url, :new_user, :new_pwd, :expected_user, :expected_pwd) do
+            'http://original_url.test' | 'test' | 'test' | 'test' | 'test'
+            'http://update_url.test'   | 'test' | 'test' | 'test' | 'test'
+            'http://update_url.test'   | :none  | :none  | nil    | nil
+            'http://update_url.test'   | 'test' | :none  | nil    | nil
+            'http://update_url.test'   | :none  | 'test' | nil    | nil
+          end
+
+          with_them do
+            before do
+              upstream.update!(url: 'http://original_url.test', username: 'original_user', password: 'original_pwd')
+            end
+
+            it 'resets the username and the password when necessary' do
+              new_attributes = { url: new_url, username: new_user, password: new_pwd }.select { |_, v| v != :none }
+              upstream.update!(new_attributes)
+
+              expect(upstream.reload).to have_attributes(
+                url: new_url,
+                username: expected_user,
+                password: expected_pwd
+              )
+            end
+          end
+        end
+      end
+    end
+
+    context 'for local upstreams' do
+      context 'for local project upstream' do
+        subject(:upstream) do
+          build(:virtual_registries_packages_npm_upstream, :without_credentials, url: other_project_global_id)
+        end
+
+        it { is_expected.to validate_absence_of(:username) }
+        it { is_expected.to validate_absence_of(:password) }
+      end
+
+      context 'for local group upstream' do
+        subject(:upstream) do
+          build(:virtual_registries_packages_npm_upstream, :without_credentials, url: other_group_global_id)
+        end
+
+        it { is_expected.to validate_absence_of(:username) }
+        it { is_expected.to validate_absence_of(:password) }
+      end
+
+      describe '#ensure_local_project_or_local_group' do
+        let_it_be(:user_global_id) { create(:user).to_global_id.to_s }
+
+        let(:invalid_project_global_id) { Project.new(id: non_existing_record_id).to_global_id.to_s }
+        let(:invalid_group_global_id) { Group.new(id: non_existing_record_id).to_global_id.to_s }
+
+        where(:url, :expected_error_messages) do
+          ref(:other_project_global_id)   | []
+          ref(:other_group_global_id)     | []
+          ref(:invalid_project_global_id) | ['Url should point to an existing Project']
+          ref(:invalid_group_global_id)   | ['Url should point to an existing Group']
+          nil                             | ["Url can't be blank", 'Url must be a valid URL']
+          'test'                          | ['Url is blocked: Only allowed schemes are http, https']
+          ref(:user_global_id)            | ['Url should point to a Project or Group']
+        end
+
+        with_them do
+          subject(:upstream) { build(:virtual_registries_packages_npm_upstream, :without_credentials, url:) }
+
+          if params[:expected_error_messages].any?
+            it { is_expected.to be_invalid.and have_attributes(errors: match_array(expected_error_messages)) }
+          else
+            it { is_expected.to be_valid }
+          end
+        end
+      end
+    end
+
+    describe '#credentials_uniqueness_for_group' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:group2) { create(:group) }
+      let_it_be(:existing_upstream) do
+        create(
+          :virtual_registries_packages_npm_upstream,
+          group: group,
+          url: 'https://example.com',
+          username: 'user',
+          password: 'pass'
+        )
+      end
+
+      let_it_be(:local_project_url) do
+        other_project_global_id.to_s.tap do |url|
+          create(:virtual_registries_packages_npm_upstream, :without_credentials, group:, url:)
+        end
+      end
+
+      let_it_be(:local_group_url) do
+        other_group_global_id.tap do |url|
+          create(:virtual_registries_packages_npm_upstream, :without_credentials, group:, url:)
+        end
+      end
+
+      where(:test_group, :url, :username, :password, :valid, :description) do
+        ref(:group)  | 'https://example.com'   | 'user'      | 'pass'      | false | 'same group, same credentials'
+        ref(:group)  | 'https://example.com'   | 'user'      | 'different' | true  | 'same group, different password'
+        ref(:group)  | 'https://example.com'   | 'different' | 'pass'      | true  | 'same group, different username'
+        ref(:group)  | 'https://different.com' | 'user'      | 'pass'      | true  | 'same group, different URL'
+        ref(:group2) | 'https://example.com'   | 'user'      | 'pass'      | true  | 'different group, same credentials'
+
+        ref(:group)  | ref(:local_project_url)  | nil | nil | false | 'same local project'
+        ref(:group)  | ref(:local_group_url)    | nil | nil | false | 'same local group'
+      end
+
+      with_them do
+        context 'when creating new upstream' do
+          subject(:new_upstream) do
+            build(
+              :virtual_registries_packages_npm_upstream,
+              group: test_group,
+              url: url,
+              username: username,
+              password: password
+            )
+          end
+
+          it "is #{params[:valid] ? 'valid' : 'invalid'} when #{params[:description]}" do
+            if valid
+              is_expected.to be_valid
+            else
+              error = if new_upstream.remote?
+                        described_class::SAME_URL_AND_CREDENTIALS_ERROR
+                      else
+                        described_class::SAME_LOCAL_PROJECT_OR_GROUP_ERROR
+                      end
+
+              is_expected.to be_invalid.and have_attributes(errors: match_array(["Group #{error}"]))
+            end
+          end
+        end
+
+        context 'when updating existing upstream' do
+          let_it_be(:updated_upstream) do
+            create(
+              :virtual_registries_packages_npm_upstream,
+              group: group,
+              url: 'https://example2.com',
+              username: 'user',
+              password: 'pass'
+            )
+          end
+
+          subject { updated_upstream }
+
+          before do
+            updated_upstream.assign_attributes(
+              group: test_group,
+              url: url,
+              username: username,
+              password: password
+            )
+          end
+
+          it "is #{params[:valid] ? 'valid' : 'invalid'} when updating to #{params[:description]}" do
+            if valid
+              is_expected.to be_valid
+            else
+              expected_message = if updated_upstream.remote?
+                                   'Group already has a remote upstream with the same url and credentials'
+                                 else
+                                   'Group already has a local upstream with the same target project or group'
+                                 end
+
+              is_expected.to be_invalid
+                .and have_attributes(errors: match_array([expected_message]))
+            end
+          end
+        end
+      end
+    end
+  end
+
+  describe 'callbacks' do
+    context 'for set_cache_validity_hours_for_npmjs_registry' do
+      %w[
+        https://registry.npmjs.org
+        https://registry.npmjs.org/
+      ].each do |registry_url|
+        context "with url set to #{registry_url}" do
+          before do
+            upstream.url = registry_url
+          end
+
+          it 'sets the cache validity hours to 0' do
+            upstream.save!
+
+            expect(upstream.cache_validity_hours).to eq(0)
+          end
+        end
+      end
+
+      context 'with url other than npmjs registry' do
+        before do
+          upstream.url = 'https://test.org/npm'
+        end
+
+        it 'sets the cache validity hours to the database default value' do
+          upstream.save!
+
+          expect(upstream.cache_validity_hours).not_to eq(0)
+        end
+      end
+
+      context 'with no url' do
+        before do
+          upstream.url = nil
+        end
+
+        it 'does not set the cache validity hours' do
+          expect(upstream).not_to receive(:set_cache_validity_hours_for_npmjs_registry)
+
+          expect { upstream.save! }.to raise_error(ActiveRecord::RecordInvalid)
+        end
+      end
+    end
+  end
+
+  describe 'scopes' do
+    describe '.for_group' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:upstream) { create(:virtual_registries_packages_npm_upstream, group:) }
+      let_it_be(:other_upstream) { create(:virtual_registries_packages_npm_upstream) }
+
+      subject { described_class.for_group(group) }
+
+      it { is_expected.to eq([upstream]) }
+    end
+  end
+
+  describe 'encryption' do
+    subject(:saved_upstream) { upstream.tap(&:save!) }
+
+    it { is_expected.to be_encrypted_attribute(:username).and be_encrypted_attribute(:password) }
+  end
+
+  describe '#local_project?' do
+    where(:url, :expected_result) do
+      nil                             | false
+      ref(:other_project_global_id)   | true
+      ref(:other_group_global_id)     | false
+      'https://gitlab.com/npm/test'   | false
+    end
+
+    with_them do
+      subject(:local_project_check) do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_project?
+      end
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+
+  describe '#local_project_id' do
+    where(:url, :expected_project) do
+      nil                             | nil
+      ref(:other_project_global_id)   | ref(:other_project)
+      ref(:other_group_global_id)     | nil
+      'https://gitlab.com/npm/test'   | nil
+    end
+
+    with_them do
+      subject(:local_project_id) do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_project_id
+      end
+
+      it { is_expected.to eq(expected_project&.id) }
+    end
+  end
+
+  describe '#local_group?' do
+    where(:url, :expected_result) do
+      nil                             | false
+      ref(:other_project_global_id)   | false
+      ref(:other_group_global_id)     | true
+      'https://gitlab.com/npm/test' | false
+    end
+
+    with_them do
+      subject(:local_group_check) do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_group?
+      end
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+
+  describe '#local_group_id' do
+    where(:url, :expected_group) do
+      nil                             | nil
+      ref(:other_project_global_id)   | nil
+      ref(:other_group_global_id)     | ref(:other_group)
+      'https://gitlab.com/npm/test' | nil
+    end
+
+    with_them do
+      subject(:local_group_id) do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_group_id
+      end
+
+      it { is_expected.to eq(expected_group&.id) }
+    end
+  end
+
+  describe '#local?' do
+    where(:url, :result) do
+      nil                           | false
+      ref(:other_project_global_id) | true
+      'https://gitlab.com/npm/test' | false
+    end
+
+    with_them do
+      subject do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local?
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+
+  describe '#remote?' do
+    where(:url, :result) do
+      nil                           | true
+      ref(:other_project_global_id) | false
+      'https://gitlab.com/npm/test' | true
+    end
+
+    with_them do
+      subject do
+        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).remote?
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
 end
-- 
GitLab


From 8e49373190f48214031f2ea373358de46e3ebfca Mon Sep 17 00:00:00 2001
From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Fri, 12 Dec 2025 16:46:35 +0100
Subject: [PATCH 2/5] Apply review feedback

---
 .../packages/npm/upstream.rb                  |  33 +----
 .../packages/npm/upstream_spec.rb             | 132 +++++-------------
 2 files changed, 35 insertions(+), 130 deletions(-)

diff --git a/ee/app/models/virtual_registries/packages/npm/upstream.rb b/ee/app/models/virtual_registries/packages/npm/upstream.rb
index fe4058d42c9adf..2597b85d8e1291 100644
--- a/ee/app/models/virtual_registries/packages/npm/upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/upstream.rb
@@ -51,37 +51,14 @@ class Upstream < ApplicationRecord
         end
 
         before_validation :normalize_url, if: -> { url? && remote? }
-        before_validation :set_cache_validity_hours_for_npmjs_registry, if: :url?, on: :create
+        before_validation :restore_password!, if: -> { remote? && username? && !password? && !username_changed? },
+          on: :update
         after_validation :reset_credentials, if: -> { persisted? && url_changed? }
 
         prevent_from_serialization(:password) if respond_to?(:prevent_from_serialization)
 
         scope :for_group, ->(group) { where(group:) }
 
-        def local_project?
-          return false unless local?
-
-          local_global_id&.model_class == Project
-        end
-
-        def local_project_id
-          return unless local_project?
-
-          local_global_id&.model_id&.to_i
-        end
-
-        def local_group?
-          return false unless local?
-
-          local_global_id&.model_class == Group
-        end
-
-        def local_group_id
-          return unless local_group?
-
-          local_global_id&.model_id&.to_i
-        end
-
         def url=(value)
           super
 
@@ -109,12 +86,6 @@ def reset_credentials
           self.password = nil
         end
 
-        def set_cache_validity_hours_for_npmjs_registry
-          return unless url.start_with?(NPMJS_REGISTRY_URL)
-
-          self.cache_validity_hours = 0
-        end
-
         def credentials_uniqueness_for_group
           return unless group
 
diff --git a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
index ce84165ba9837e..655efcabc41e0d 100644
--- a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
+++ b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
@@ -312,45 +312,47 @@
   end
 
   describe 'callbacks' do
-    context 'for set_cache_validity_hours_for_npmjs_registry' do
-      %w[
-        https://registry.npmjs.org
-        https://registry.npmjs.org/
-      ].each do |registry_url|
-        context "with url set to #{registry_url}" do
-          before do
-            upstream.url = registry_url
-          end
-
-          it 'sets the cache validity hours to 0' do
-            upstream.save!
-
-            expect(upstream.cache_validity_hours).to eq(0)
-          end
-        end
+    context 'for restore_password!' do
+      let_it_be_with_reload(:upstream) do
+        create(:virtual_registries_packages_npm_upstream,
+          username: 'existing_username',
+          password: 'existing_password'
+        )
       end
 
-      context 'with url other than npmjs registry' do
-        before do
-          upstream.url = 'https://test.org/npm'
-        end
-
-        it 'sets the cache validity hours to the database default value' do
-          upstream.save!
-
-          expect(upstream.cache_validity_hours).not_to eq(0)
-        end
+      # rubocop:disable Layout/LineLength -- Avoid formatting to keep one-line table syntax for readability
+      where(:new_username, :new_password, :update_name, :should_save, :expected_username, :expected_password,
+        :description) do
+        'new_username'      | 'new_password' | false | true  | 'new_username'      | 'new_password'      | 'updates both when username and password are changed'
+        ''                  | ''             | false | true  | ''                  | ''                  | 'updates both when username and password are changed to an empty string'
+        'existing_username' | ''             | false | true  | 'existing_username' | 'existing_password' | 'keeps original password when username remains unchanged'
+        'new_username'      | ''             | false | false | nil                 | nil                 | 'allows validation to fail when username changes with blank password'
+        'existing_username' | ''             | true  | true  | 'existing_username' | 'existing_password' | 'keeps original password when password is an empty string and updating non-credential fields'
+        'existing_username' | nil            | true  | true  | 'existing_username' | 'existing_password' | 'keeps original password when password is nil and updating non-credential fields'
       end
+      # rubocop:enable Layout/LineLength
 
-      context 'with no url' do
+      with_them do
         before do
-          upstream.url = nil
+          upstream.name = 'new name' if update_name
+          upstream.username = new_username
+          upstream.password = new_password
         end
 
-        it 'does not set the cache validity hours' do
-          expect(upstream).not_to receive(:set_cache_validity_hours_for_npmjs_registry)
+        if params[:should_save]
+          it params[:description] do
+            upstream.save!
 
-          expect { upstream.save! }.to raise_error(ActiveRecord::RecordInvalid)
+            expect(upstream).to have_attributes(
+              username: expected_username,
+              password: expected_password
+            )
+          end
+        else
+          it params[:description] do
+            expect(upstream).not_to be_valid
+            expect(upstream.errors[:password]).to include("can't be blank")
+          end
         end
       end
     end
@@ -374,74 +376,6 @@
     it { is_expected.to be_encrypted_attribute(:username).and be_encrypted_attribute(:password) }
   end
 
-  describe '#local_project?' do
-    where(:url, :expected_result) do
-      nil                             | false
-      ref(:other_project_global_id)   | true
-      ref(:other_group_global_id)     | false
-      'https://gitlab.com/npm/test'   | false
-    end
-
-    with_them do
-      subject(:local_project_check) do
-        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_project?
-      end
-
-      it { is_expected.to eq(expected_result) }
-    end
-  end
-
-  describe '#local_project_id' do
-    where(:url, :expected_project) do
-      nil                             | nil
-      ref(:other_project_global_id)   | ref(:other_project)
-      ref(:other_group_global_id)     | nil
-      'https://gitlab.com/npm/test'   | nil
-    end
-
-    with_them do
-      subject(:local_project_id) do
-        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_project_id
-      end
-
-      it { is_expected.to eq(expected_project&.id) }
-    end
-  end
-
-  describe '#local_group?' do
-    where(:url, :expected_result) do
-      nil                             | false
-      ref(:other_project_global_id)   | false
-      ref(:other_group_global_id)     | true
-      'https://gitlab.com/npm/test' | false
-    end
-
-    with_them do
-      subject(:local_group_check) do
-        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_group?
-      end
-
-      it { is_expected.to eq(expected_result) }
-    end
-  end
-
-  describe '#local_group_id' do
-    where(:url, :expected_group) do
-      nil                             | nil
-      ref(:other_project_global_id)   | nil
-      ref(:other_group_global_id)     | ref(:other_group)
-      'https://gitlab.com/npm/test' | nil
-    end
-
-    with_them do
-      subject(:local_group_id) do
-        build(:virtual_registries_packages_npm_upstream, :without_credentials, url:).local_group_id
-      end
-
-      it { is_expected.to eq(expected_group&.id) }
-    end
-  end
-
   describe '#local?' do
     where(:url, :result) do
       nil                           | false
-- 
GitLab


From fd29d5901f63ab11405c065ba356639e21c3d8da Mon Sep 17 00:00:00 2001
From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Fri, 12 Dec 2025 18:24:20 +0100
Subject: [PATCH 3/5] ensure_local_project_or_local_group can't be called
 without url

---
 ee/app/models/virtual_registries/packages/npm/upstream.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ee/app/models/virtual_registries/packages/npm/upstream.rb b/ee/app/models/virtual_registries/packages/npm/upstream.rb
index 2597b85d8e1291..d1bcf4f911868f 100644
--- a/ee/app/models/virtual_registries/packages/npm/upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/upstream.rb
@@ -99,8 +99,6 @@ def credentials_uniqueness_for_group
         end
 
         def ensure_local_project_or_local_group
-          return errors.add(:url, 'is invalid') unless local_global_id
-
           unless local_global_id.model_class.in?(ALLOWED_GLOBAL_ID_CLASSES)
             return errors.add(:url, 'should point to a Project or Group')
           end
-- 
GitLab


From 76adb10799f18a985e2a2d33dff27437399500b5 Mon Sep 17 00:00:00 2001
From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Wed, 17 Dec 2025 14:37:39 +0100
Subject: [PATCH 4/5] Apply review feedback

---
 .../models/virtual_registries/packages/npm/registry_upstream.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
index 7207cbd8c3fd39..2c7a25b02088fe 100644
--- a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
@@ -23,7 +23,7 @@ class RegistryUpstream < ApplicationRecord
           numericality: {
             only_integer: true,
             greater_than_or_equal_to: 1,
-            less_than_or_equal_to: ->(record) { record.class::MAX_UPSTREAMS_COUNT }
+            less_than_or_equal_to: MAX_UPSTREAMS_COUNT
           },
           presence: true
 
-- 
GitLab


From 5889220a2f9a6faee85363400ad04197b684acc2 Mon Sep 17 00:00:00 2001
From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Date: Wed, 17 Dec 2025 17:33:55 +0100
Subject: [PATCH 5/5] Fix undercoverage. Change uniqueness validation.

---
 .../virtual_registries/packages/npm/registry_upstream.rb    | 2 +-
 ee/app/models/virtual_registries/packages/npm/upstream.rb   | 2 +-
 .../models/virtual_registries/packages/npm/upstream_spec.rb | 6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
index 2c7a25b02088fe..887735ce6ccd46 100644
--- a/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/registry_upstream.rb
@@ -15,7 +15,7 @@ class RegistryUpstream < ApplicationRecord
           class_name: '::VirtualRegistries::Packages::Npm::Upstream',
           inverse_of: :registry_upstreams
 
-        validates :upstream_id, uniqueness: { scope: :registry_id }, if: :upstream_id?
+        validates :upstream_id, uniqueness: { scope: :registry_id }
         validates :registry_id, uniqueness: { scope: [:position] }
 
         validates :group, top_level_group: true, presence: true
diff --git a/ee/app/models/virtual_registries/packages/npm/upstream.rb b/ee/app/models/virtual_registries/packages/npm/upstream.rb
index d1bcf4f911868f..eeed42fd2c0191 100644
--- a/ee/app/models/virtual_registries/packages/npm/upstream.rb
+++ b/ee/app/models/virtual_registries/packages/npm/upstream.rb
@@ -55,7 +55,7 @@ class Upstream < ApplicationRecord
           on: :update
         after_validation :reset_credentials, if: -> { persisted? && url_changed? }
 
-        prevent_from_serialization(:password) if respond_to?(:prevent_from_serialization)
+        prevent_from_serialization(:password)
 
         scope :for_group, ->(group) { where(group:) }
 
diff --git a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
index 655efcabc41e0d..a22661e5d347e3 100644
--- a/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
+++ b/ee/spec/models/virtual_registries/packages/npm/upstream_spec.rb
@@ -407,4 +407,10 @@
       it { is_expected.to eq(result) }
     end
   end
+
+  describe '#as_json' do
+    subject { upstream.as_json }
+
+    it { is_expected.not_to include('password') }
+  end
 end
-- 
GitLab