From 32042ef56adfa24ce5952c6f3b7dc97dea5fd2d4 Mon Sep 17 00:00:00 2001
From: Alejandro Rodriguez <alejandro@gitlab.com>
Date: Tue, 8 Nov 2016 18:37:15 +0000
Subject: [PATCH] Merge branch 'unauthenticated-container-registry-access' into
 'security'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Restore unauthenticated access to public container registries

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/24284

See merge request !2025

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/controllers/jwt_controller.rb              |  4 ++--
 ...ontainer_registry_authentication_service.rb | 16 ++++++++++------
 spec/requests/jwt_controller_spec.rb           | 18 ++++++++++++++++--
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb
index 7e4da73bc11..c736200a104 100644
--- a/app/controllers/jwt_controller.rb
+++ b/app/controllers/jwt_controller.rb
@@ -12,7 +12,7 @@ class JwtController < ApplicationController
     return head :not_found unless service
 
     result = service.new(@authentication_result.project, @authentication_result.actor, auth_params).
-      execute(authentication_abilities: @authentication_result.authentication_abilities || [])
+      execute(authentication_abilities: @authentication_result.authentication_abilities)
 
     render json: result, status: result[:http_status]
   end
@@ -20,7 +20,7 @@ class JwtController < ApplicationController
   private
 
   def authenticate_project_or_user
-    @authentication_result = Gitlab::Auth::Result.new
+    @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities)
 
     authenticate_with_http_basic do |login, password|
       @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip)
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 3fc1c70be75..c00c5aebf57 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -9,8 +9,8 @@ module Auth
 
       return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled
 
-      unless current_user || project
-        return error('DENIED', status: 403, message: 'access forbidden') unless scope
+      unless scope || current_user || project
+        return error('DENIED', status: 403, message: 'access forbidden')
       end
 
       { token: authorized_token(scope).encoded }
@@ -92,23 +92,23 @@ module Auth
       # Build can:
       # 1. pull from its own project (for ex. a build)
       # 2. read images from dependent projects if creator of build is a team member
-      @authentication_abilities.include?(:build_read_container_image) &&
+      has_authentication_ability?(:build_read_container_image) &&
         (requested_project == project || can?(current_user, :build_read_container_image, requested_project))
     end
 
     def user_can_pull?(requested_project)
-      @authentication_abilities.include?(:read_container_image) &&
+      has_authentication_ability?(:read_container_image) &&
         can?(current_user, :read_container_image, requested_project)
     end
 
     def build_can_push?(requested_project)
       # Build can push only to the project from which it originates
-      @authentication_abilities.include?(:build_create_container_image) &&
+      has_authentication_ability?(:build_create_container_image) &&
         requested_project == project
     end
 
     def user_can_push?(requested_project)
-      @authentication_abilities.include?(:create_container_image) &&
+      has_authentication_ability?(:create_container_image) &&
         can?(current_user, :create_container_image, requested_project)
     end
 
@@ -118,5 +118,9 @@ module Auth
         http_status: status
       }
     end
+
+    def has_authentication_ability?(capability)
+      (@authentication_abilities || []).include?(capability)
+    end
   end
 end
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index f0ef155bd7b..a3e7844b2f3 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -20,7 +20,7 @@ describe JwtController do
     end
   end
 
-  context 'when using authorized request' do
+  context 'when using authenticated request' do
     context 'using CI token' do
       let(:build) { create(:ci_build, :running) }
       let(:project) { build.project }
@@ -65,7 +65,7 @@ describe JwtController do
           let(:access_token) { create(:personal_access_token, user: user) }
           let(:headers) { { authorization: credentials(user.username, access_token.token) } }
 
-          it 'rejects the authorization attempt' do
+          it 'accepts the authorization attempt' do
             expect(response).to have_http_status(200)
           end
         end
@@ -81,6 +81,20 @@ describe JwtController do
     end
   end
 
+  context 'when using unauthenticated request' do
+    it 'accepts the authorization attempt' do
+      get '/jwt/auth', parameters
+
+      expect(response).to have_http_status(200)
+    end
+
+    it 'allows read access' do
+      expect(service).to receive(:execute).with(authentication_abilities: Gitlab::Auth.read_authentication_abilities)
+
+      get '/jwt/auth', parameters
+    end
+  end
+
   context 'unknown service' do
     subject! { get '/jwt/auth', service: 'unknown' }