Merge branch 'graphql-error-when-authorizing-with-no-permissions-defined' into 'master'

Sanity check for GraphQL authorized?

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

3 files changed, 46 insertions, 19 deletions

diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md
index 2ed2a905db7..aeddad14995 100644
--- a/doc/development/api_graphql_styleguide.md
+++ b/doc/development/api_graphql_styleguide.md
@@ -447,7 +447,7 @@ want to validate the abilities for.
 
 Alternatively, we can add a `find_object` method that will load the
 object on the mutation. This would allow you to use the
-`authorized_find!` and `authorized_find!` helper methods.
+`authorized_find!` helper method.
 
 When a user is not allowed to perform the action, or an object is not
 found, we should raise a
diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb
index b367a97105c..ef5caaf5b0e 100644
--- a/lib/gitlab/graphql/authorize/authorize_resource.rb
+++ b/lib/gitlab/graphql/authorize/authorize_resource.rb
@@ -27,12 +27,6 @@ module Gitlab
           raise NotImplementedError, "Implement #find_object in #{self.class.name}"
         end
 
-        def authorized_find(*args)
-          object = find_object(*args)
-
-          object if authorized?(object)
-        end
-
         def authorized_find!(*args)
           object = find_object(*args)
           authorize!(object)
@@ -48,6 +42,12 @@ module Gitlab
         end
 
         def authorized?(object)
+          # Sanity check. We don't want to accidentally allow a developer to authorize
+          # without first adding permissions to authorize against
+          if self.class.required_permissions.empty?
+            raise Gitlab::Graphql::Errors::ArgumentError, "#{self.class.name} has no authorizations"
+          end
+
           self.class.required_permissions.all? do |ability|
             # The actions could be performed across multiple objects. In which
             # case the current user is common, and we could benefit from the
diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb
index 13cf52fd795..20842f55014 100644
--- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb
+++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb
@@ -34,12 +34,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
       end
     end
 
-    describe '#authorized_find' do
-      it 'returns the object' do
-        expect(loading_resource.authorized_find).to eq(project)
-      end
-    end
-
     describe '#authorized_find!' do
       it 'returns the object' do
         expect(loading_resource.authorized_find!).to eq(project)
@@ -66,12 +60,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
       end
     end
 
-    describe '#authorized_find' do
-      it 'returns `nil`' do
-        expect(loading_resource.authorized_find).to be_nil
-      end
-    end
-
     describe '#authorized_find!' do
       it 'raises an error' do
         expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
@@ -101,6 +89,45 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
     end
   end
 
+  context 'when the class does not define authorize' do
+    let(:fake_class) do
+      Class.new do
+        include Gitlab::Graphql::Authorize::AuthorizeResource
+
+        attr_reader :user, :found_object
+
+        def initialize(user, found_object)
+          @user, @found_object = user, found_object
+        end
+
+        def find_object(*_args)
+          found_object
+        end
+
+        def current_user
+          user
+        end
+
+        def self.name
+          'TestClass'
+        end
+      end
+    end
+    let(:error) { /#{fake_class.name} has no authorizations/ }
+
+    describe '#authorized_find!' do
+      it 'raises a comprehensive error message' do
+        expect { loading_resource.authorized_find! }.to raise_error(error)
+      end
+    end
+
+    describe '#authorized?' do
+      it 'raises a comprehensive error message' do
+        expect { loading_resource.authorized?(project) }.to raise_error(error)
+      end
+    end
+  end
+
   describe '#authorize' do
     it 'adds permissions from subclasses to those of superclasses when used on classes' do
       base_class = Class.new do