Extract a `Gitlab::Scope` class.

- To represent an authorization scope, such as `api` or `read_user`
- This is a better abstraction than the hash we were previously using.

5 files changed, 42 insertions, 24 deletions

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index ee2e93a0d63..bf5aef0055e 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -28,17 +28,16 @@ class AccessTokenValidationService
   end
 
   # True if the token's scope contains any of the passed scopes.
-  def include_any_scope?(scopes)
-    if scopes.blank?
+  def include_any_scope?(required_scopes)
+    if required_scopes.blank?
       true
     else
-      # Remove any scopes whose `if` condition does not return `true`
-      scopes = scopes.select { |scope| scope.if.nil? || scope.if.call(request) }
-
-      # Check whether the token is allowed access to any of the required scopes.
-      passed_scope_names = scopes.map { |scope| scope.name.to_sym }
-      token_scope_names = token.scopes.map(&:to_sym)
-      Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present?
+      # We're comparing each required_scope against all token scopes, which would
+      # take quadratic time. This consideration is irrelevant here because of the
+      # small number of records involved.
+      # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006
+      token_scopes = token.scopes.map(&:to_sym)
+      required_scopes.any? { |scope| scope.sufficient?(token_scopes, request) }
     end
   end
 end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 56f6da57555..0d2d71e336a 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -31,7 +31,7 @@ module API
       # the scopes are all aggregated.
       def allow_access_with_scope(scopes, options = {})
         Array(scopes).each do |scope|
-          allowed_scopes << OpenStruct.new(name: scope.to_sym, if: options[:if])
+          allowed_scopes << Scope.new(scope, options)
         end
       end
 
diff --git a/lib/api/scope.rb b/lib/api/scope.rb
new file mode 100644
index 00000000000..c23846d1e7d
--- /dev/null
+++ b/lib/api/scope.rb
@@ -0,0 +1,23 @@
+# Encapsulate a scope used for authorization, such as `api`, or `read_user`
+module API
+  class Scope
+    attr_reader :name, :if
+
+    def initialize(name, options = {})
+      @name = name.to_sym
+      @if = options[:if]
+    end
+
+    # Are the `scopes` passed in sufficient to adequately authorize the passed
+    # request for the scope represented by the current instance of this class?
+    def sufficient?(scopes, request)
+      verify_if_condition(request) && scopes.include?(self.name)
+    end
+
+    private
+
+    def verify_if_condition(request)
+      self.if.nil? || self.if.call(request)
+    end
+  end
+end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index ec73255b20a..6d0d638ba14 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -136,11 +136,11 @@ module Gitlab
       end
 
       def valid_oauth_token?(token)
-        token && token.accessible? && valid_scoped_token?(token, ['api'])
+        token && token.accessible? && valid_scoped_token?(token, [:api])
       end
 
       def valid_scoped_token?(token, scopes)
-        scopes = scopes.map { |scope| OpenStruct.new(name: scope) }
+        scopes = scopes.map { |scope| API::Scope.new(scope) }
         AccessTokenValidationService.new(token).include_any_scope?(scopes)
       end
 
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 279f4ed93ac..660a05e0b6d 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -1,37 +1,33 @@
 require 'spec_helper'
 
 describe AccessTokenValidationService, services: true do
-  def scope(data)
-    OpenStruct.new(data)
-  end
-
   describe ".include_any_scope?" do
     let(:request) { double("request") }
 
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :api })]
+      scopes = [API::Scope.new(:api)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [scope({ name: :api }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
@@ -45,7 +41,7 @@ describe AccessTokenValidationService, services: true do
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
     end
@@ -53,21 +49,21 @@ describe AccessTokenValidationService, services: true do
     context "conditions" do
       it "ignores any scopes whose `if` condition returns false" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { false } })]
+        scopes = [API::Scope.new(:api, if: ->(_) { false })]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
       end
 
       it "does not ignore scopes whose `if` condition is not set" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { false } }), scope({ name: :read_user })]
+        scopes = [API::Scope.new(:api, if: ->(_) { false }), API::Scope.new(:read_user)]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end
 
       it "does not ignore scopes whose `if` condition returns true" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { true } }), scope({ name: :read_user, if: ->(_) { false } })]
+        scopes = [API::Scope.new(:api, if: ->(_) { true }), API::Scope.new(:read_user, if: ->(_) { false })]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end