handle multiple scopes in token requests

According to the official spec the scope parameter can be passed more than
once: "This query parameter should be specified multiple times ifthere is more
than one scope entry from the WWW-Authenticate header".
At least the latest docker daemon 1.10.0 does use multiple scope parameters in
a single token request (e.g. when pushing multiple tags in parallel).

Additionaly this commit changes the token endpoint to not return
"401 Unauthorized" on any kind of authorization error, only authentication
errors should return 401 errors.
This also conforms with the spec: "If the client only has a subset of the
requested access it must not be considered an error as it is not the
responsibility of the token server to indicate authorization errors as part of
this workflow."

Signed-off-by: Fabian Ruff <fabian.ruff@sap.com>

app/controllers/api/v2/tokens_controller.rb

@@ -5,13 +5,13 @@ class Api::V2::TokensController < Api::BaseController
   # Returns the token that the docker client should use in order to perform
   # operation into the private registry.
   def show
+    authenticate_user! if request.headers["Authorization"]
     registry = Registry.find_by(hostname: params[:service])
-    raise RegistryNotHandled, "Cannot find registry #{params[:service]}" if registry.nil?
 
-    auth_scope = authorize_scopes(registry)
-    authenticate_user! if auth_scope.nil?
+    auth_scopes = []
+    auth_scopes = authorize_scopes(registry) unless registry.nil?
 
-    token = Portus::JwtToken.new(params[:account], params[:service], auth_scope)
+    token = Portus::JwtToken.new(params[:account], params[:service], auth_scopes)
     logger.tagged("jwt_token", "claim") { logger.debug token.claim }
     render json: token.encoded_hash
   end
@@ -24,37 +24,47 @@ def show
   #
   # If no scope was specified, this is a login request and it just returns nil.
   def authorize_scopes(registry)
-    return unless params[:scope]
+    scopes =  Array(Rack::Utils.parse_query(request.query_string)["scope"])
+    return if scopes.empty?
+
+    auth_scopes = {}
 
     # First try to fetch the requested scopes and the handler. If no scopes
     # were successfully given, respond with a 401.
-    auth_scope, scopes = scope_handler(registry, params[:scope])
-    raise Pundit::NotAuthorizedError, "No scopes to handle" if scopes.empty?
-
     scopes.each do |scope|
-      # It will try to check if the current user is authorized to access the
-      # scope given in this iteration. If everything is fine, then nothing will
-      # happen, otherwise there are two possible exceptions that can be raised:
-      #
-      #   - NoMethodError: the targeted resource does not handle the scope that
-      #     is being checked. It will raise a ScopeNotHandled.
-      #   - Pundit::NotAuthorizedError: the targeted resource unauthorized the
-      #     given user for the scope that is being checked. In this case this
-      #     scope gets removed from `auth_scope.actions`.
-      begin
-        authorize auth_scope.resource, "#{scope}?".to_sym
-      rescue NoMethodError, Pundit::NotAuthorizedError
-        logger.debug "scope #{scope} not handled/authorized, removing from actions"
-        auth_scope.actions.delete_if { |a| a == scope }
+      auth_scope, actions = scope_handler(registry, scope)
+
+      actions.each do |action|
+        # It will try to check if the current user is authorized to access the
+        # scope given in this iteration. If everything is fine, then nothing will
+        # happen, otherwise there are two possible exceptions that can be raised:
+        #
+        #   - NoMethodError: the targeted resource does not handle the scope that
+        #     is being checked. It will raise a ScopeNotHandled.
+        #   - Pundit::NotAuthorizedError: the targeted resource unauthorized the
+        #     given user for the scope that is being checked. In this case this
+        #     scope gets removed from `auth_scope.actions`.
+        begin
+          authorize auth_scope.resource, "#{action}?".to_sym
+        rescue NoMethodError, Pundit::NotAuthorizedError, Portus::AuthScope::ResourceNotFound
+          logger.debug "action #{action} not handled/authorized, removing from actions"
+          auth_scope.actions.delete_if { |a| a == action }
+        end
       end
-    end
 
-    # If auth_scope.actions is empty, it means that the previous loop
-    # unauthorized all the requested scopes for the current user. Therefore
-    # respond with a 401. Otherwise, return the resulting auth_scope.
-    msg = "None of the given scopes were authorized for the current user"
-    raise Pundit::NotAuthorizedError, msg if auth_scope.actions.empty?
-    auth_scope
+      next if auth_scope.actions.empty?
+      # if there is already a similar scope (type and resource name),
+      # we combine them into one:
+      # e.g. scope=repository:busybox:push&scope=repository:busybox:pull
+      #      -> access=>[{:type=>"repository", :name=>"busybox", :actions=>["push", "pull"]}
+      k = [auth_scope.resource_type,  auth_scope.resource_name]
+      if auth_scopes[k]
+        auth_scopes[k].actions.concat(auth_scope.actions).uniq!
+      else
+        auth_scopes[k] = auth_scope
+      end
+    end
+    auth_scopes.values
   end
 
   # From the given scope string, try to fetch a scope handler class for it.

lib/portus/jwt_token.rb

@@ -6,12 +6,12 @@ module Portus
   #
   class JwtToken
     # The constructor takes the query parameters as specified in the
-    # specification. The given scope is assumed to have already been processed
+    # specification. The given scopes are assumed to have already been processed
     # by the caller with the authorized scopes.
-    def initialize(account, service, scope)
+    def initialize(account, service, scopes)
       @account = account
       @service = service
-      @scope   = scope
+      @scopes   = scopes
     end
 
     # Returns a hash containing the encoded token, ready to be sent as a JSON
@@ -32,7 +32,7 @@ def claim
         hash[:nbf]    = issued_at - 5.seconds
         hash[:exp]    = issued_at + expiration_time
         hash[:jti]    = jwt_id
-        hash[:access] = authorized_access if @scope
+        hash[:access] = authorized_access if @scopes
       end
     end
 
@@ -56,11 +56,13 @@ def expiration_time
 
     # Returns an array with the authorized actions hash.
     def authorized_access
-      [{
-        type:    @scope.resource_type,
-        name:    @scope.resource_name,
-        actions: @scope.actions
-      }]
+      @scopes.map do |scope|
+        {
+          type:    scope.resource_type,
+          name:    scope.resource_name,
+          actions: scope.actions
+        }
+      end
     end
 
     # Returns a (hopefully) unique id for the JWT token.

spec/api/v2/token_spec.rb

@@ -4,6 +4,11 @@
 
   describe "get token" do
 
+    def parse_token(body)
+      token = JSON.parse(body)["token"]
+      JWT.decode(token, nil, false, leeway: 2)[0]
+    end
+
     let(:auth_mech) { ActionController::HttpAuthentication::Basic }
     let(:password) { "this is a test" }
     let(:user) { create(:user, password: password) }
@@ -29,27 +34,31 @@
           scope:   "repository:foo_namespace/me:push"
         }
       end
+      before do
+        create(:namespace, name: "foo_namespace", registry: registry)
+      end
 
       it "denies access when the password is wrong" do
         get v2_token_url,
-          { service: registry.hostname, account: "account", scope: "repository:foo/me:push" },
+          valid_request,
           invalid_auth_header
 
         expect(response.status).to eq 401
       end
 
       it "denies access when the user does not exist" do
         get v2_token_url,
-          { service: registry.hostname, account: "account", scope: "repository:foo/me:push" },
+          valid_request,
           nonexistent_auth_header
 
         expect(response.status).to eq 401
       end
 
       it "denies access when basic auth credentials are not defined" do
-        get v2_token_url,
-          service: registry.hostname, account: "account", scope: "repository:foo/me:push"
-        expect(response.status).to eq 401
+        get v2_token_url, valid_request
+
+        payload = parse_token response.body
+        expect(payload["access"]).to be_empty
       end
 
       it "denies access to a disabled user" do
@@ -73,6 +82,8 @@
           "HTTP_AUTHORIZATION" => auth_mech.encode_credentials(user.username, password)
 
         expect(response.status).to eq 200
+        payload = parse_token response.body
+        expect(payload["access"]).not_to be_empty
 
         # But not for another
         get v2_token_url,
@@ -83,7 +94,9 @@
           },
           "HTTP_AUTHORIZATION" => auth_mech.encode_credentials(another.username, password)
 
-        expect(response.status).to eq 401
+        expect(response.status).to eq 200
+        payload = parse_token response.body
+        expect(payload["access"]).to be_empty
       end
     end
 
@@ -111,15 +124,6 @@
         expect(ldapuser.valid_password?("12341234")).to be true
       end
 
-      it "does not authenticate the LDAP user if not Basic authentication was given" do
-        get v2_token_url,
-          {
-            service: registry.hostname,
-            account: "ldapuser"
-          }, {}
-
-        expect(response.status).to eq 401
-      end
     end
 
     context "as valid user" do
@@ -144,8 +148,7 @@
 
       it "decoded payload should conform with params sent" do
         get v2_token_url, valid_request, valid_auth_header
-        token = JSON.parse(response.body)["token"]
-        payload = JWT.decode(token, nil, false, leeway: 2)[0]
+        payload = parse_token response.body
         expect(payload["sub"]).to eq "account"
         expect(payload["aud"]).to eq registry.hostname
         expect(payload["access"][0]["type"]).to eq "repository"
@@ -163,8 +166,7 @@
         end
 
         it "decoded payload should not contain access key" do
-          token = JSON.parse(response.body)["token"]
-          payload = JWT.decode(token, nil, false, leeway: 2)[0]
+          payload = parse_token response.body
           expect(payload).to_not have_key("access")
         end
 
@@ -212,8 +214,7 @@
           expect(response.status).to eq 200
 
           # And check that the only authorized scope is "pull"
-          token = JSON.parse(response.body)["token"]
-          payload = JWT.decode(token, nil, false, leeway: 2)[0]
+          payload = parse_token response.body
           expect(payload["access"][0]["name"]).to eq "#{user.username}/busybox"
           expect(payload["access"][0]["actions"]).to match_array ["pull"]
         end
@@ -247,22 +248,13 @@
         it "allows portus to access the Catalog API" do
           get v2_token_url, valid_request, valid_portus_auth_header
           expect(response.status).to eq 200
-
-          token = JSON.parse(response.body)["token"]
-          payload = JWT.decode(token, nil, false, leeway: 2)[0]
+          payload = parse_token response.body
           expect(payload["sub"]).to eq "portus"
           expect(payload["aud"]).to eq registry.hostname
           expect(payload["access"][0]["type"]).to eq "registry"
           expect(payload["access"][0]["name"]).to eq "catalog"
           expect(payload["access"][0]["actions"][0]).to eq "*"
         end
-
-        it "forbids unhandled methods from the registry type" do
-          wr = valid_request
-          wr[:scope] = "registry:catalog:lala"
-          get v2_token_url, wr, valid_portus_auth_header
-          expect(response.status).to eq 401
-        end
       end
 
       context "unknown scope" do
@@ -274,7 +266,25 @@
                 scope:   "repository:busybox:fork"
               },
               valid_auth_header
-          expect(response.status).to eq 401
+          payload = parse_token response.body
+          expect(payload["access"]).to be_empty
+        end
+      end
+
+      context "multiple scopes" do
+        it "allows access" do
+          query_string = "service=#{registry.hostname}&" \
+                         "account=user.username&" \
+                         "scope=repository%3Abusybox%3Apush&" \
+                         "scope=repository%3Abusybox%3Apull"
+          allow_any_instance_of(ActionDispatch::Request).to receive(:query_string)
+            .and_return(query_string)
+          get v2_token_url, query_string, valid_auth_header
+          expect(response.status).to eq 200
+          payload = parse_token response.body
+          expect(payload["access"].size).to eq(1)
+          expect(payload["access"][0]["actions"]).to eq(["push", "pull"])
+
         end
       end
 
@@ -287,16 +297,18 @@
         end
       end
 
-      context "unkwnow registry" do
+      context "unknown registry" do
         context "no scope requested" do
-          it "respond with 401" do
+          it "respond with 200 and no access" do
             get v2_token_url, { service: "does not exist", account: "account" }, valid_auth_header
-            expect(response.status).to eq 401
+            expect(response.status).to eq 200
+            payload = parse_token response.body
+            expect(payload["access"]).to be_empty
           end
         end
 
         context "reposity scope" do
-          it "it responde with 401" do
+          it "it responds with 200 and no access" do
             # force creation of the namespace
             namespace = create(:namespace,
                                team:     Team.find_by(name: user.username),
@@ -312,7 +324,9 @@
               },
               valid_auth_header
             )
-            expect(response.status).to eq(401)
+            expect(response.status).to eq(200)
+            payload = parse_token response.body
+            expect(payload["access"]).to be_empty
           end
         end
       end

spec/lib/portus/jwt_token_spec.rb

@@ -21,7 +21,7 @@
   end
 
   describe "#encoded_token" do
-    subject { described_class.new("jlhawn", "registry.docker.com", scope) }
+    subject { described_class.new("jlhawn", "registry.docker.com", [scope]) }
 
     it "calls JWT#encode with claim with stringified_keys" do
       expect(JWT).to receive(:encode).with(
@@ -51,7 +51,7 @@
   end
 
   describe "#claim" do
-    subject { described_class.new("jlhawn", "registry.docker.com", scope) }
+    subject { described_class.new("jlhawn", "registry.docker.com", [scope]) }
 
     describe "basic fields" do
       describe ":iss" do