Merge branch 'fix-multiple-scopes' into 'master'

Support multiple scopes when authing container registry scopes

Closes #48968

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

5 files changed, 155 insertions, 39 deletions

diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb
index 3cb9e46b548..d172aee5436 100644
--- a/app/controllers/jwt_controller.rb
+++ b/app/controllers/jwt_controller.rb
@@ -54,6 +54,22 @@ class JwtController < ApplicationController
   end
 
   def auth_params
-    params.permit(:service, :scope, :account, :client_id)
+    params.permit(:service, :account, :client_id)
+          .merge(additional_params)
+  end
+
+  def additional_params
+    { scopes: scopes_param }.compact
+  end
+
+  # We have to parse scope here, because Docker Client does not send an array of scopes,
+  # but rather a flat list and we loose second scope when being processed by Rails:
+  # scope=scopeA&scope=scopeB
+  #
+  # This method makes to always return an array of scopes
+  def scopes_param
+    return unless params[:scope].present?
+
+    Array(Rack::Utils.parse_query(request.query_string)['scope'])
   end
 end
diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 81857d0cb4c..893b37b831a 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -9,11 +9,11 @@ module Auth
 
       return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled
 
-      unless scope || current_user || project
+      unless scopes.any? || current_user || project
         return error('DENIED', status: 403, message: 'access forbidden')
       end
 
-      { token: authorized_token(scope).encoded }
+      { token: authorized_token(*scopes).encoded }
     end
 
     def self.full_access_token(*names)
@@ -47,10 +47,12 @@ module Auth
       end
     end
 
-    def scope
-      return unless params[:scope]
+    def scopes
+      return [] unless params[:scopes]
 
-      @scope ||= process_scope(params[:scope])
+      @scopes ||= params[:scopes].map do |scope|
+        process_scope(scope)
+      end.compact
     end
 
     def process_scope(scope)
diff --git a/changelogs/unreleased/fix-multiple-scopes.yml b/changelogs/unreleased/fix-multiple-scopes.yml
new file mode 100644
index 00000000000..24e5172d9a1
--- /dev/null
+++ b/changelogs/unreleased/fix-multiple-scopes.yml
@@ -0,0 +1,5 @@
+---
+title: Support multiple scopes when authing container registry scopes
+merge_request: 20617
+author:
+type: fixed
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index 6f40a02aaa9..e042d772718 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -70,6 +70,25 @@ describe JwtController do
 
       it { expect(service_class).to have_received(:new).with(nil, user, parameters) }
 
+      context 'when passing a flat array of scopes' do
+        # We use this trick to make rails to generate a query_string:
+        # scope=scope1&scope=scope2
+        # It works because :scope and 'scope' are the same as string, but different objects
+        let(:parameters) do
+          {
+            :service => service_name,
+            :scope => 'scope1',
+            'scope' => 'scope2'
+          }
+        end
+
+        let(:service_parameters) do
+          { service: service_name, scopes: %w(scope1 scope2) }
+        end
+
+        it { expect(service_class).to have_received(:new).with(nil, user, service_parameters) }
+      end
+
       context 'when user has 2FA enabled' do
         let(:user) { create(:user, :two_factor) }
 
diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb
index 037484931b8..c7f88e45c84 100644
--- a/spec/services/auth/container_registry_authentication_service_spec.rb
+++ b/spec/services/auth/container_registry_authentication_service_spec.rb
@@ -142,7 +142,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'for registry catalog' do
       let(:current_params) do
-        { scope: "registry:catalog:*" }
+        { scopes: ["registry:catalog:*"] }
       end
 
       context 'disallow browsing for users without Gitlab admin rights' do
@@ -164,7 +164,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:push" }
+          { scopes: ["repository:#{project.full_path}:push"] }
         end
 
         it_behaves_like 'a pushable'
@@ -177,7 +177,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:*" }
+          { scopes: ["repository:#{project.full_path}:*"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -191,7 +191,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'when pulling from root level repository' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:pull" }
+            { scopes: ["repository:#{project.full_path}:pull"] }
           end
 
           it_behaves_like 'a pullable'
@@ -205,7 +205,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:*" }
+          { scopes: ["repository:#{project.full_path}:*"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -218,7 +218,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:push,pull" }
+          { scopes: ["repository:#{project.full_path}:push,pull"] }
         end
 
         it_behaves_like 'a pullable'
@@ -231,7 +231,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:pull,push" }
+          { scopes: ["repository:#{project.full_path}:pull,push"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -244,7 +244,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         end
 
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:*" }
+          { scopes: ["repository:#{project.full_path}:*"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -257,7 +257,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'allow anyone to pull images' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:pull" }
+          { scopes: ["repository:#{project.full_path}:pull"] }
         end
 
         it_behaves_like 'a pullable'
@@ -266,7 +266,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'disallow anyone to push images' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:push" }
+          { scopes: ["repository:#{project.full_path}:push"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -275,7 +275,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'disallow anyone to delete images' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:*" }
+          { scopes: ["repository:#{project.full_path}:*"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -284,7 +284,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'when repository name is invalid' do
         let(:current_params) do
-          { scope: 'repository:invalid:push' }
+          { scopes: ['repository:invalid:push'] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -298,7 +298,7 @@ describe Auth::ContainerRegistryAuthenticationService do
       context 'for internal user' do
         context 'allow anyone to pull images' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:pull" }
+            { scopes: ["repository:#{project.full_path}:pull"] }
           end
 
           it_behaves_like 'a pullable'
@@ -307,7 +307,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'disallow anyone to push images' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:push" }
+            { scopes: ["repository:#{project.full_path}:push"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -316,7 +316,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'disallow anyone to delete images' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:*" }
+            { scopes: ["repository:#{project.full_path}:*"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -328,7 +328,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'disallow anyone to pull or push images' do
           let(:current_user) { create(:user, external: true) }
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:pull,push" }
+            { scopes: ["repository:#{project.full_path}:pull,push"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -338,7 +338,7 @@ describe Auth::ContainerRegistryAuthenticationService do
         context 'disallow anyone to delete images' do
           let(:current_user) { create(:user, external: true) }
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:*" }
+            { scopes: ["repository:#{project.full_path}:*"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -364,7 +364,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'allow to delete images' do
       let(:current_params) do
-        { scope: "repository:#{current_project.full_path}:*" }
+        { scopes: ["repository:#{current_project.full_path}:*"] }
       end
 
       it_behaves_like 'a deletable' do
@@ -397,7 +397,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'allow to pull and push images' do
       let(:current_params) do
-        { scope: "repository:#{current_project.full_path}:pull,push" }
+        { scopes: ["repository:#{current_project.full_path}:pull,push"] }
       end
 
       it_behaves_like 'a pullable and pushable' do
@@ -411,7 +411,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'disallow to delete images' do
       let(:current_params) do
-        { scope: "repository:#{current_project.full_path}:*" }
+        { scopes: ["repository:#{current_project.full_path}:*"] }
       end
 
       it_behaves_like 'an inaccessible' do
@@ -422,7 +422,7 @@ describe Auth::ContainerRegistryAuthenticationService do
     context 'for other projects' do
       context 'when pulling' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:pull" }
+          { scopes: ["repository:#{project.full_path}:pull"] }
         end
 
         context 'allow for public' do
@@ -489,7 +489,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'when pushing' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:push" }
+          { scopes: ["repository:#{project.full_path}:push"] }
         end
 
         context 'disallow for all' do
@@ -523,7 +523,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'disallow when pulling' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:pull" }
+          { scopes: ["repository:#{project.full_path}:pull"] }
         end
 
         it_behaves_like 'an inaccessible'
@@ -534,14 +534,66 @@ describe Auth::ContainerRegistryAuthenticationService do
 
   context 'registry catalog browsing authorized as admin' do
     let(:current_user) { create(:user, :admin) }
+    let(:project) { create(:project, :public) }
 
     let(:current_params) do
-      { scope: "registry:catalog:*" }
+      { scopes: ["registry:catalog:*"] }
     end
 
     it_behaves_like 'a browsable'
   end
 
+  context 'support for multiple scopes' do
+    let(:internal_project) { create(:project, :internal) }
+    let(:private_project) { create(:project, :private) }
+
+    let(:current_params) do
+      {
+        scopes: [
+          "repository:#{internal_project.full_path}:pull",
+          "repository:#{private_project.full_path}:pull"
+        ]
+      }
+    end
+
+    context 'user has access to all projects' do
+      let(:current_user) { create(:user, :admin) }
+
+      it_behaves_like 'a browsable' do
+        let(:access) do
+          [
+            { 'type' => 'repository',
+              'name' => internal_project.full_path,
+              'actions' => ['pull'] },
+            { 'type' => 'repository',
+              'name' => private_project.full_path,
+              'actions' => ['pull'] }
+          ]
+        end
+      end
+    end
+
+    context 'user only has access to internal project' do
+      let(:current_user) { create(:user) }
+
+      it_behaves_like 'a browsable' do
+        let(:access) do
+          [
+            { 'type' => 'repository',
+              'name' => internal_project.full_path,
+              'actions' => ['pull'] }
+          ]
+        end
+      end
+    end
+
+    context 'anonymous access is rejected' do
+      let(:current_user) { nil }
+
+      it_behaves_like 'a forbidden'
+    end
+  end
+
   context 'unauthorized' do
     context 'disallow to use scope-less authentication' do
       it_behaves_like 'a forbidden'
@@ -550,7 +602,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'for invalid scope' do
       let(:current_params) do
-        { scope: 'invalid:aa:bb' }
+        { scopes: ['invalid:aa:bb'] }
       end
 
       it_behaves_like 'a forbidden'
@@ -561,7 +613,7 @@ describe Auth::ContainerRegistryAuthenticationService do
       let(:project) { create(:project, :private) }
 
       let(:current_params) do
-        { scope: "repository:#{project.full_path}:pull" }
+        { scopes: ["repository:#{project.full_path}:pull"] }
       end
 
       it_behaves_like 'a forbidden'
@@ -572,7 +624,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'when pulling and pushing' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:pull,push" }
+          { scopes: ["repository:#{project.full_path}:pull,push"] }
         end
 
         it_behaves_like 'a pullable'
@@ -581,7 +633,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
       context 'when pushing' do
         let(:current_params) do
-          { scope: "repository:#{project.full_path}:push" }
+          { scopes: ["repository:#{project.full_path}:push"] }
         end
 
         it_behaves_like 'a forbidden'
@@ -591,7 +643,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
     context 'for registry catalog' do
       let(:current_params) do
-        { scope: "registry:catalog:*" }
+        { scopes: ["registry:catalog:*"] }
       end
 
       it_behaves_like 'a forbidden'
@@ -601,7 +653,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
   context 'for deploy tokens' do
     let(:current_params) do
-      { scope: "repository:#{project.full_path}:pull" }
+      { scopes: ["repository:#{project.full_path}:pull"] }
     end
 
     context 'when deploy token has read_registry as a scope' do
@@ -616,7 +668,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'when pushing' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:push" }
+            { scopes: ["repository:#{project.full_path}:push"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -632,7 +684,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'when pushing' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:push" }
+            { scopes: ["repository:#{project.full_path}:push"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -648,7 +700,7 @@ describe Auth::ContainerRegistryAuthenticationService do
 
         context 'when pushing' do
           let(:current_params) do
-            { scope: "repository:#{project.full_path}:push" }
+            { scopes: ["repository:#{project.full_path}:push"] }
           end
 
           it_behaves_like 'an inaccessible'
@@ -734,4 +786,26 @@ describe Auth::ContainerRegistryAuthenticationService do
       end
     end
   end
+
+  context 'user authorization' do
+    let(:current_user) { create(:user) }
+
+    context 'with multiple scopes' do
+      let(:project) { create(:project) }
+      let(:project2) { create }
+
+      context 'allow developer to push images' do
+        before do
+          project.add_developer(current_user)
+        end
+
+        let(:current_params) do
+          { scopes: ["repository:#{project.full_path}:push"] }
+        end
+
+        it_behaves_like 'a pushable'
+        it_behaves_like 'container repository factory'
+      end
+    end
+  end
 end