diff options
| -rw-r--r-- | app/controllers/jwt_controller.rb | 18 | ||||
| -rw-r--r-- | app/services/auth/container_registry_authentication_service.rb | 12 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-multiple-scopes.yml | 5 | ||||
| -rw-r--r-- | spec/requests/jwt_controller_spec.rb | 19 | ||||
| -rw-r--r-- | spec/services/auth/container_registry_authentication_service_spec.rb | 140 |
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 |