Merge branch 'allow-to-pull-public-images' into 'master'

Allow to access Container Registry for Public and Internal projects

## What does this MR do?
Adds missing ability `read_container_image` for `public scope` which affects users accessing public and internal projects.

## Why was this MR needed?
Previously, we allowed to use container registry for anonymous users, but did not allow to do the same for logged in.
This MR makes it possible.

## What are the relevant issue numbers?
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/18176 https://gitlab.com/gitlab-org/gitlab-ce/issues/19668 https://gitlab.com/gitlab-org/gitlab-ce/issues/19117

## Does this MR meet the acceptance criteria?

- [ ] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [ ] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5279

CHANGELOG

@@ -86,6 +86,7 @@ v 8.10.0 (unreleased)
   - More descriptive message for git hooks and file locks
   - Aliases of award emoji should be stored as original name. !5060 (dixpac)
   - Handle custom Git hook result in GitLab UI
+  - Allow to access Container Registry for Public and Internal projects
   - Allow '?', or '&' for label names
   - Fix importer for GitHub Pull Requests when a branch was reused across Pull Requests
   - Add date when user joined the team on the member page

app/models/ability.rb

@@ -204,7 +204,8 @@ class Ability
         :download_code,
         :fork_project,
         :read_commit_status,
-        :read_pipeline
+        :read_pipeline,
+        :read_container_image
       ]
     end
 

spec/features/security/project/internal_access_spec.rb

@@ -426,4 +426,23 @@ describe "Internal Project Access", feature: true  do
     it { is_expected.to be_denied_for :external }
     it { is_expected.to be_denied_for :visitor }
   end
+
+  describe "GET /:project_path/container_registry" do
+    before do
+      stub_container_registry_tags('latest')
+      stub_container_registry_config(enabled: true)
+    end
+
+    subject { namespace_project_container_registry_index_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
 end

spec/features/security/project/private_access_spec.rb

@@ -362,4 +362,23 @@ describe "Private Project Access", feature: true  do
     it { is_expected.to be_denied_for :external }
     it { is_expected.to be_denied_for :visitor }
   end
+
+  describe "GET /:project_path/container_registry" do
+    before do
+      stub_container_registry_tags('latest')
+      stub_container_registry_config(enabled: true)
+    end
+
+    subject { namespace_project_container_registry_index_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_denied_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
 end

spec/features/security/project/public_access_spec.rb

@@ -426,4 +426,23 @@ describe "Public Project Access", feature: true  do
     it { is_expected.to be_denied_for :external }
     it { is_expected.to be_denied_for :visitor }
   end
+
+  describe "GET /:project_path/container_registry" do
+    before do
+      stub_container_registry_tags('latest')
+      stub_container_registry_config(enabled: true)
+    end
+
+    subject { namespace_project_container_registry_index_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_allowed_for :external }
+    it { is_expected.to be_allowed_for :visitor }
+  end
 end

spec/services/auth/container_registry_authentication_service_spec.rb

@@ -87,51 +87,105 @@ describe Auth::ContainerRegistryAuthenticationService, services: true do
   end
 
   context 'user authorization' do
-    let(:project) { create(:project) }
     let(:current_user) { create(:user) }
 
-    context 'allow to use scope-less authentication' do
-      it_behaves_like 'a valid token'
-    end
+    context 'for private project' do
+      let(:project) { create(:empty_project) }
 
-    context 'allow developer to push images' do
-      before { project.team << [current_user, :developer] }
+      context 'allow to use scope-less authentication' do
+        it_behaves_like 'a valid token'
+      end
 
-      let(:current_params) do
-        { scope: "repository:#{project.path_with_namespace}:push" }
+      context 'allow developer to push images' do
+        before { project.team << [current_user, :developer] }
+
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:push" }
+        end
+
+        it_behaves_like 'a pushable'
       end
 
-      it_behaves_like 'a pushable'
-    end
+      context 'allow reporter to pull images' do
+        before { project.team << [current_user, :reporter] }
+
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:pull" }
+        end
 
-    context 'allow reporter to pull images' do
-      before { project.team << [current_user, :reporter] }
+        it_behaves_like 'a pullable'
+      end
 
-      let(:current_params) do
-        { scope: "repository:#{project.path_with_namespace}:pull" }
+      context 'return a least of privileges' do
+        before { project.team << [current_user, :reporter] }
+
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:push,pull" }
+        end
+
+        it_behaves_like 'a pullable'
       end
 
-      it_behaves_like 'a pullable'
+      context 'disallow guest to pull or push images' do
+        before { project.team << [current_user, :guest] }
+
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:pull,push" }
+        end
+
+        it_behaves_like 'an inaccessible'
+      end
     end
 
-    context 'return a least of privileges' do
-      before { project.team << [current_user, :reporter] }
+    context 'for public project' do
+      let(:project) { create(:empty_project, :public) }
 
-      let(:current_params) do
-        { scope: "repository:#{project.path_with_namespace}:push,pull" }
+      context 'allow anyone to pull images' do
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:pull" }
+        end
+
+        it_behaves_like 'a pullable'
       end
 
-      it_behaves_like 'a pullable'
+      context 'disallow anyone to push images' do
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:push" }
+        end
+
+        it_behaves_like 'an inaccessible'
+      end
     end
 
-    context 'disallow guest to pull or push images' do
-      before { project.team << [current_user, :guest] }
+    context 'for internal project' do
+      let(:project) { create(:empty_project, :internal) }
 
-      let(:current_params) do
-        { scope: "repository:#{project.path_with_namespace}:pull,push" }
+      context 'for internal user' do
+        context 'allow anyone to pull images' do
+          let(:current_params) do
+            { scope: "repository:#{project.path_with_namespace}:pull" }
+          end
+
+          it_behaves_like 'a pullable'
+        end
+
+        context 'disallow anyone to push images' do
+          let(:current_params) do
+            { scope: "repository:#{project.path_with_namespace}:push" }
+          end
+
+          it_behaves_like 'an inaccessible'
+        end
       end
 
-      it_behaves_like 'an inaccessible'
+      context 'for external user' do
+        let(:current_user) { create(:user, external: true) }
+        let(:current_params) do
+          { scope: "repository:#{project.path_with_namespace}:pull,push" }
+        end
+
+        it_behaves_like 'an inaccessible'
+      end
     end
   end