Implement review comments from @DouweM for !12300.

- Use a struct for scopes, so we can call `scope.if` instead of `scope[:if]`

- Refactor the "remove scopes whose :if condition returns false" logic to use a
  `select` rather than a `reject`.

4 files changed, 28 insertions, 14 deletions

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 450e90d947d..ee2e93a0d63 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -33,10 +33,10 @@ class AccessTokenValidationService
       true
     else
       # Remove any scopes whose `if` condition does not return `true`
-      scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
+      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 }
+      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?
     end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 8411ad8ec34..56f6da57555 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 << { name: scope, if: options[:if] }
+          allowed_scopes << OpenStruct.new(name: scope.to_sym, if: options[:if])
         end
       end
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 37ac8ecc2f0..ec73255b20a 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -130,16 +130,17 @@ module Gitlab
 
         token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password)
 
-        if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map { |scope| { name: scope.to_s }})
+        if token && valid_scoped_token?(token, AVAILABLE_SCOPES)
           Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes))
         end
       end
 
       def valid_oauth_token?(token)
-        token && token.accessible? && valid_scoped_token?(token, [{ name: "api" }])
+        token && token.accessible? && valid_scoped_token?(token, ['api'])
       end
 
       def valid_scoped_token?(token, scopes)
+        scopes = scopes.map { |scope| OpenStruct.new(name: 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 c8189aa14d8..279f4ed93ac 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -1,62 +1,75 @@
 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 })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }])).to be(true)
+      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 })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true)
+      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 })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      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 })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      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 blank' do
       token = double("token", scopes: [])
+      scopes = []
 
-      expect(described_class.new(token, request: request).include_any_scope?([])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     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 })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :other_scope }])).to be(false)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
     end
 
     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 } })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(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 })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
+        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 } })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
+        expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end
     end
   end