Merge branch '54417-improve-authorize-dsl' into 'master'

Improve GraphQL Authorization DSL

Closes #57828

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

app/graphql/types/issue_type.rb

@@ -15,18 +15,16 @@ module Types
 
     field :author, Types::UserType,
           null: false,
-          resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find } do
-      authorize :read_user
-    end
+          resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find },
+          authorize: :read_user
 
     field :assignees, Types::UserType.connection_type, null: true
 
     field :labels, Types::LabelType.connection_type, null: true
     field :milestone, Types::MilestoneType,
           null: true,
-          resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find } do
-      authorize :read_milestone
-    end
+          resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find },
+          authorize: :read_milestone
 
     field :due_date, Types::TimeType, null: true
     field :confidential, GraphQL::BOOLEAN_TYPE, null: false

app/graphql/types/merge_request_type.rb

@@ -48,9 +48,7 @@ module Types
     field :downvotes, GraphQL::INT_TYPE, null: false
     field :subscribed, GraphQL::BOOLEAN_TYPE, method: :subscribed?, null: false
 
-    field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline do
-      authorize :read_pipeline
-    end
+    field :head_pipeline, Types::Ci::PipelineType, null: true, method: :actual_head_pipeline, authorize: :read_pipeline
     field :pipelines, Types::Ci::PipelineType.connection_type,
           resolver: Resolvers::MergeRequestPipelinesResolver
   end

app/graphql/types/project_type.rb

@@ -69,16 +69,14 @@ module Types
     field :merge_requests,
           Types::MergeRequestType.connection_type,
           null: true,
-          resolver: Resolvers::MergeRequestsResolver do
-      authorize :read_merge_request
-    end
+          resolver: Resolvers::MergeRequestsResolver,
+          authorize: :read_merge_request
 
     field :merge_request,
           Types::MergeRequestType,
           null: true,
-          resolver: Resolvers::MergeRequestsResolver.single do
-      authorize :read_merge_request
-    end
+          resolver: Resolvers::MergeRequestsResolver.single,
+          authorize: :read_merge_request
 
     field :issues,
           Types::IssueType.connection_type,

app/graphql/types/query_type.rb

@@ -7,9 +7,8 @@ module Types
     field :project, Types::ProjectType,
           null: true,
           resolver: Resolvers::ProjectResolver,
-          description: "Find a project" do
-      authorize :read_project
-    end
+          description: "Find a project",
+          authorize: :read_project
 
     field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new
   end

config/initializers/graphql.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_key(:authorize))
+Types::BaseField.accepts_definition(:authorize)

doc/development/api_graphql_styleguide.md

@@ -12,24 +12,34 @@ add a `HTTP_PRIVATE_TOKEN` header.
 ### Authorization
 
 Fields can be authorized using the same abilities used in the Rails
-app. This can be done using the `authorize` helper:
+app. This can be done by supplying the `authorize` option:
 
 ```ruby
 module Types
   class QueryType < BaseObject
     graphql_name 'Query'
 
-    field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do
-      authorize :read_project
-    end
+    field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver, authorize: :read_project
   end
+end
+```
+
+Fields can be authorized against multiple abilities, in which case all
+ability checks must pass. This requires explicitly passing a block to `field`:
+
+```ruby
+field :project, Types::ProjectType, null: true, resolver: Resolvers::ProjectResolver do
+  authorize [:read_project, :another_ability]
+end
 ```
 
 The object found by the resolve call is used for authorization.
 
-This works for authorizing a single record, for authorizing
-collections, we should only load what the currently authenticated user
-is allowed to view. Preferably we use our existing finders for that.
+TIP: **Tip:**
+When authorizing collections, try to load only what the currently
+authenticated user is allowed to view with our existing finders first.
+This minimizes database queries and unnecessary authorization checks of
+the loaded records.
 
 ## Types
 

lib/gitlab/graphql/authorize.rb

@@ -10,21 +10,6 @@ module Gitlab
       def self.use(schema_definition)
         schema_definition.instrument(:field, Instrumentation.new)
       end
-
-      def required_permissions
-        # If the `#authorize` call is used on multiple classes, we add the
-        # permissions specified on a subclass, to the ones that were specified
-        # on it's superclass.
-        @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions)
-                                    superclass.required_permissions.dup
-                                  else
-                                    []
-                                  end
-      end
-
-      def authorize(*permissions)
-        required_permissions.concat(permissions)
-      end
     end
   end
 end

lib/gitlab/graphql/authorize/authorize_resource.rb

@@ -6,8 +6,21 @@ module Gitlab
       module AuthorizeResource
         extend ActiveSupport::Concern
 
-        included do
-          extend Gitlab::Graphql::Authorize
+        class_methods do
+          def required_permissions
+            # If the `#authorize` call is used on multiple classes, we add the
+            # permissions specified on a subclass, to the ones that were specified
+            # on it's superclass.
+            @required_permissions ||= if self.respond_to?(:superclass) && superclass.respond_to?(:required_permissions)
+                                        superclass.required_permissions.dup
+                                      else
+                                        []
+                                      end
+          end
+
+          def authorize(*permissions)
+            required_permissions.concat(permissions)
+          end
         end
 
         def find_object(*args)

lib/gitlab/graphql/authorize/instrumentation.rb

@@ -6,19 +6,15 @@ module Gitlab
       class Instrumentation
         # Replace the resolver for the field with one that will only return the
         # resolved object if the permissions check is successful.
-        #
-        # Collections are not supported. Apply permissions checks for those at the
-        # database level instead, to avoid loading superfluous data from the DB
         def instrument(_type, field)
-          field_definition = field.metadata[:type_class]
-          return field unless field_definition.respond_to?(:required_permissions)
-          return field if field_definition.required_permissions.empty?
+          required_permissions = Array.wrap(field.metadata[:authorize])
+          return field if required_permissions.empty?
 
           old_resolver = field.resolve_proc
 
           new_resolver = -> (obj, args, ctx) do
             resolved_obj = old_resolver.call(obj, args, ctx)
-            checker = build_checker(ctx[:current_user], field_definition.required_permissions)
+            checker = build_checker(ctx[:current_user], required_permissions)
 
             if resolved_obj.respond_to?(:then)
               resolved_obj.then(&checker)

spec/graphql/features/authorization_spec.rb

@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Gitlab::Graphql::Authorization' do
+  set(:user) { create(:user) }
+
+  let(:test_object) { double(name: 'My name') }
+  let(:object_type) { object_type_class }
+  let(:query_type) { query_type_class(object_type, test_object) }
+  let(:schema) { schema_class(query_type) }
+
+  let(:execute) do
+    schema.execute(
+      query_string,
+      context: { current_user: user },
+      variables: {}
+    )
+  end
+
+  let(:result) { execute['data'] }
+
+  before do
+    # By default, disallow all permissions.
+    allow(Ability).to receive(:allowed?).and_return(false)
+  end
+
+  describe 'authorizing with a single permission' do
+    let(:query_string) { '{ singlePermission() { name } }' }
+
+    subject { result['singlePermission'] }
+
+    it 'should return the protected field when user has permission' do
+      permit(:foo)
+
+      expect(subject['name']).to eq(test_object.name)
+    end
+
+    it 'should return nil when user is not authorized' do
+      expect(subject).to be_nil
+    end
+  end
+
+  describe 'authorizing with an Array of permissions' do
+    let(:query_string) { '{ permissionCollection() { name } }' }
+
+    subject { result['permissionCollection'] }
+
+    it 'should return the protected field when user has all permissions' do
+      permit(:foo, :bar)
+
+      expect(subject['name']).to eq(test_object.name)
+    end
+
+    it 'should return nil when user only has one of the permissions' do
+      permit(:foo)
+
+      expect(subject).to be_nil
+    end
+
+    it 'should return nil when user only has none of the permissions' do
+      expect(subject).to be_nil
+    end
+  end
+
+  private
+
+  def permit(*permissions)
+    permissions.each do |permission|
+      allow(Ability).to receive(:allowed?).with(user, permission, test_object).and_return(true)
+    end
+  end
+
+  def object_type_class
+    Class.new(Types::BaseObject) do
+      graphql_name 'TestObject'
+
+      field :name, GraphQL::STRING_TYPE, null: true
+    end
+  end
+
+  def query_type_class(type, object)
+    Class.new(Types::BaseObject) do
+      graphql_name 'TestQuery'
+
+      field :single_permission, type,
+        null: true,
+        authorize: :foo,
+        resolve: ->(obj, args, ctx) { object }
+
+      field :permission_collection, type,
+        null: true,
+        resolve: ->(obj, args, ctx) { object } do
+        authorize [:foo, :bar]
+      end
+    end
+  end
+
+  def schema_class(query)
+    Class.new(GraphQL::Schema) do
+      use Gitlab::Graphql::Authorize
+
+      query(query)
+    end
+  end
+end

spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb

@@ -100,4 +100,22 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
       expect { fake_class.new.find_object }.to raise_error(/Implement #find_object in #{fake_class.name}/)
     end
   end
+
+  describe '#authorize' do
+    it 'adds permissions from subclasses to those of superclasses when used on classes' do
+      base_class = Class.new do
+        include Gitlab::Graphql::Authorize::AuthorizeResource
+
+        authorize :base_authorization
+      end
+
+      sub_class = Class.new(base_class) do
+        authorize :sub_authorization
+      end
+
+      expect(base_class.required_permissions).to contain_exactly(:base_authorization)
+      expect(sub_class.required_permissions)
+        .to contain_exactly(:base_authorization, :sub_authorization)
+    end
+  end
 end

spec/lib/gitlab/graphql/authorize_spec.rb

@@ -1,20 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::Graphql::Authorize do
-  describe '#authorize' do
-    it 'adds permissions from subclasses to those of superclasses when used on classes' do
-      base_class = Class.new do
-        extend Gitlab::Graphql::Authorize
-
-        authorize :base_authorization
-      end
-      sub_class = Class.new(base_class) do
-        authorize :sub_authorization
-      end
-
-      expect(base_class.required_permissions).to contain_exactly(:base_authorization)
-      expect(sub_class.required_permissions)
-        .to contain_exactly(:base_authorization, :sub_authorization)
-    end
-  end
-end

spec/support/matchers/graphql_matchers.rb

@@ -1,8 +1,6 @@
 RSpec::Matchers.define :require_graphql_authorizations do |*expected|
   match do |field|
-    field_definition = field.metadata[:type_class]
-    expect(field_definition).to respond_to(:required_permissions)
-    expect(field_definition.required_permissions).to contain_exactly(*expected)
+    expect(field.metadata[:authorize]).to eq(*expected)
   end
 end