Merge branch 'deploy-tokens-container-registry-specs' into 'master'

Verify that deploy token has valid access when pulling container registry image

Closes #45148

See merge request gitlab-org/gitlab-ce!18260

5 files changed, 165 insertions, 12 deletions

diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb
index b47b2ff4c3f..8dae821a10e 100644
--- a/app/models/deploy_token.rb
+++ b/app/models/deploy_token.rb
@@ -34,7 +34,7 @@ class DeployToken < ActiveRecord::Base
   end
 
   def has_access_to?(requested_project)
-    project == requested_project
+    active? && project == requested_project
   end
 
   # This is temporal. Currently we limit DeployToken
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 8f050072f74..f28cddb2af3 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -149,7 +149,8 @@ module Auth
     def deploy_token_can_pull?(requested_project)
       has_authentication_ability?(:read_container_image) &&
         current_user.is_a?(DeployToken) &&
-        current_user.has_access_to?(requested_project)
+        current_user.has_access_to?(requested_project) &&
+        current_user.read_registry?
     end
 
     ##
diff --git a/changelogs/unreleased/deploy-tokens-container-registry-specs.yml b/changelogs/unreleased/deploy-tokens-container-registry-specs.yml
new file mode 100644
index 00000000000..d86f955c966
--- /dev/null
+++ b/changelogs/unreleased/deploy-tokens-container-registry-specs.yml
@@ -0,0 +1,5 @@
+---
+title: Verify that deploy token has valid access when pulling container registry image
+merge_request: 18260
+author:
+type: fixed
diff --git a/spec/models/deploy_token_spec.rb b/spec/models/deploy_token_spec.rb
index 5a15c23def4..780b200e837 100644
--- a/spec/models/deploy_token_spec.rb
+++ b/spec/models/deploy_token_spec.rb
@@ -78,19 +78,30 @@ describe DeployToken do
   describe '#has_access_to?' do
     let(:project) { create(:project) }
 
-    subject(:deploy_token) { create(:deploy_token, projects: [project]) }
+    subject { deploy_token.has_access_to?(project) }
 
-    context 'when the deploy token has access to the project' do
-      it 'should return true' do
-        expect(deploy_token.has_access_to?(project)).to be_truthy
-      end
+    context 'when deploy token is active and related to project' do
+      let(:deploy_token) { create(:deploy_token, projects: [project]) }
+
+      it { is_expected.to be_truthy }
     end
 
-    context 'when the deploy token does not have access to the project' do
-      it 'should return false' do
-        another_project = create(:project)
-        expect(deploy_token.has_access_to?(another_project)).to be_falsy
-      end
+    context 'when deploy token is active but not related to project' do
+      let(:deploy_token) { create(:deploy_token) }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when deploy token is revoked and related to project' do
+      let(:deploy_token) { create(:deploy_token, :revoked, projects: [project]) }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when deploy token is revoked and not related to the project' do
+      let(:deploy_token) { create(:deploy_token, :revoked) }
+
+      it { is_expected.to be_falsy }
     end
   end
 
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb
index 290eeae828e..da8e660c16b 100644
--- a/spec/services/auth/container_registry_authentication_service_spec.rb
+++ b/spec/services/auth/container_registry_authentication_service_spec.rb
@@ -585,4 +585,140 @@ describe Auth::ContainerRegistryAuthenticationService do
       it_behaves_like 'not a container repository factory'
     end
   end
+
+  context 'for deploy tokens' do
+    let(:current_params) do
+      { scope: "repository:#{project.full_path}:pull" }
+    end
+
+    context 'when deploy token has read_registry as a scope' do
+      let(:current_user) { create(:deploy_token, projects: [project]) }
+
+      context 'for public project' do
+        let(:project) { create(:project, :public) }
+
+        context 'when pulling' do
+          it_behaves_like 'a pullable'
+        end
+
+        context 'when pushing' do
+          let(:current_params) do
+            { scope: "repository:#{project.full_path}:push" }
+          end
+
+          it_behaves_like 'an inaccessible'
+        end
+      end
+
+      context 'for internal project' do
+        let(:project) { create(:project, :internal) }
+
+        context 'when pulling' do
+          it_behaves_like 'a pullable'
+        end
+
+        context 'when pushing' do
+          let(:current_params) do
+            { scope: "repository:#{project.full_path}:push" }
+          end
+
+          it_behaves_like 'an inaccessible'
+        end
+      end
+
+      context 'for private project' do
+        let(:project) { create(:project, :private) }
+
+        context 'when pulling' do
+          it_behaves_like 'a pullable'
+        end
+
+        context 'when pushing' do
+          let(:current_params) do
+            { scope: "repository:#{project.full_path}:push" }
+          end
+
+          it_behaves_like 'an inaccessible'
+        end
+      end
+    end
+
+    context 'when deploy token does not have read_registry scope' do
+      let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) }
+
+      context 'for public project' do
+        let(:project) { create(:project, :public) }
+
+        context 'when pulling' do
+          it_behaves_like 'a pullable'
+        end
+      end
+
+      context 'for internal project' do
+        let(:project) { create(:project, :internal) }
+
+        context 'when pulling' do
+          it_behaves_like 'an inaccessible'
+        end
+      end
+
+      context 'for private project' do
+        let(:project) { create(:project, :internal) }
+
+        context 'when pulling' do
+          it_behaves_like 'an inaccessible'
+        end
+      end
+    end
+
+    context 'when deploy token is not related to the project' do
+      let(:current_user) { create(:deploy_token, read_registry: false) }
+
+      context 'for public project' do
+        let(:project) { create(:project, :public) }
+
+        context 'when pulling' do
+          it_behaves_like 'a pullable'
+        end
+      end
+
+      context 'for internal project' do
+        let(:project) { create(:project, :internal) }
+
+        context 'when pulling' do
+          it_behaves_like 'an inaccessible'
+        end
+      end
+
+      context 'for private project' do
+        let(:project) { create(:project, :internal) }
+
+        context 'when pulling' do
+          it_behaves_like 'an inaccessible'
+        end
+      end
+    end
+
+    context 'when deploy token has been revoked' do
+      let(:current_user) { create(:deploy_token, :revoked, projects: [project]) }
+
+      context 'for public project' do
+        let(:project) { create(:project, :public) }
+
+        it_behaves_like 'a pullable'
+      end
+
+      context 'for internal project' do
+        let(:project) { create(:project, :internal) }
+
+        it_behaves_like 'an inaccessible'
+      end
+
+      context 'for private project' do
+        let(:project) { create(:project, :internal) }
+
+        it_behaves_like 'an inaccessible'
+      end
+    end
+  end
 end