Move `ToJson` cop to `JsonSerialization`; check for `as_json` as wellrs-to_json-cop

4 files changed, 178 insertions, 146 deletions

diff --git a/rubocop/cop/security/to_json.rb b/rubocop/cop/security/json_serialization.rb
index afd3da91136..50f2d73eee8 100644
--- a/rubocop/cop/security/to_json.rb
+++ b/rubocop/cop/security/json_serialization.rb
@@ -1,13 +1,37 @@
 module RuboCop
   module Cop
     module Security
-      class ToJson < RuboCop::Cop::Cop
-        MSG = "Don't use `to_json` without specifying `only`".freeze
+      # This cop checks for `to_json`/`as_json` without `:only` options
+      #
+      # Either method called on an instance of a `Serializer` class will be
+      # ignored. Associations included via `include` are subject to the same
+      # rules.
+      #
+      # @example
+      #
+      #   # bad
+      #   render json: @user.to_json
+      #   render json: @user.to_json(except: %i[password])
+      #   render json: @user.to_json(
+      #     only: %i[username],
+      #     include: { :identities }
+      #   )
+      #
+      #   # acceptable
+      #   render json: UserSerializer.new.to_json
+      #
+      #   # good
+      #   render json: @user.to_json(only: %i[name username])
+      #   render json: @user.to_json(include: {
+      #     identities: { only: %i[provider] }
+      #   })
+      class JsonSerialization < RuboCop::Cop::Cop
+        MSG = "Don't use `%s` without specifying `only`".freeze
 
         # Check for `to_json` sent to any object that's not a Hash literal or
         # Serializer instance
         def_node_matcher :to_json?, <<~PATTERN
-          (send !{nil hash #serializer?} :to_json $...)
+          (send !{nil hash #serializer?} ${:to_json :as_json} $...)
         PATTERN
 
         # Check if node is a `only: ...` pair
@@ -25,17 +49,23 @@ module RuboCop
           (pair (sym :only) (array ...))
         PATTERN
 
+        # Check for `SomeConstant.new`
+        def_node_search :constant_init, <<~PATTERN
+          (send (const nil $_) :new)
+        PATTERN
+
         def on_send(node)
           matched = to_json?(node)
           return unless matched
 
           @_has_top_level_only = false
+          @method = matched.first
 
-          if matched[0].nil?
+          if matched.last.nil? || matched.last.empty?
             # Empty `to_json` call
-            add_offense(node, :expression)
+            add_offense(node, :expression, format_message)
           else
-            options = matched.first
+            options = matched.last.first
 
             # If `to_json` was given an argument that isn't a Hash, we don't
             # know what to do here, so just move along
@@ -48,26 +78,22 @@ module RuboCop
             # Add a top-level offense for the entire argument list, but only if
             # we haven't yet added any offenses to the child Hash values (such
             # as `include`)
-            add_offense(node.children.last, :expression) if requires_only?
+            if requires_only?
+              add_offense(node.children.last, :expression, format_message)
+            end
           end
         end
 
         private
 
-        def_node_search :constant_init, <<~PATTERN
-          (send (const nil $_) :new)
-        PATTERN
+        def format_message
+          format(MSG, @method)
+        end
 
         def serializer?(node)
           constant_init(node).any? { |name| name.to_s.end_with?('Serializer') }
         end
 
-        def requires_only?
-          return false if @_has_top_level_only
-
-          offenses.count.zero?
-        end
-
         def check_pair(pair)
           if only_pair?(pair)
             @_has_top_level_only = true
@@ -77,10 +103,16 @@ module RuboCop
             includes.each_child_node do |child_node|
               next if contains_only?(child_node)
 
-              add_offense(child_node, :expression)
+              add_offense(child_node, :expression, format_message)
             end
           end
         end
+
+        def requires_only?
+          return false if @_has_top_level_only
+
+          offenses.count.zero?
+        end
       end
     end
   end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index 18a18440bfc..1c26ca9820a 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -5,4 +5,4 @@ require_relative 'cop/migration/add_column_with_default'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'
 require_relative 'cop/migration/add_index'
-require_relative 'cop/security/to_json'
+require_relative 'cop/security/json_serialization'
diff --git a/spec/rubocop/cop/security/json_serialization_spec.rb b/spec/rubocop/cop/security/json_serialization_spec.rb
new file mode 100644
index 00000000000..77914710a2e
--- /dev/null
+++ b/spec/rubocop/cop/security/json_serialization_spec.rb
@@ -0,0 +1,128 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/security/json_serialization'
+
+describe RuboCop::Cop::Security::JsonSerialization do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  shared_examples 'an upstanding constable' do |method|
+    it "ignores calls except `#{method}`" do
+      inspect_source(cop, 'render json: foo')
+
+      expect(cop.offenses).to be_empty
+    end
+
+    context "`#{method}` without options" do
+      it 'does nothing when sent to nil receiver' do
+        inspect_source(cop, method.to_s)
+
+        expect(cop.offenses).to be_empty
+      end
+
+      it 'does nothing when sent to a Hash' do
+        inspect_source(cop, "{}.#{method}")
+
+        expect(cop.offenses).to be_empty
+      end
+
+      it 'does nothing when sent to a Serializer instance' do
+        inspect_source(cop, "MergeRequestSerializer.new.represent(issuable).#{method}")
+
+        expect(cop.offenses).to be_empty
+      end
+
+      it 'adds an offense when sent to any other receiver' do
+        inspect_source(cop, "foo.#{method}")
+
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.highlights).to contain_exactly("foo.#{method}")
+        expect(cop.messages.first).to start_with("Don't use `#{method}`")
+      end
+    end
+
+    context "`#{method}` with options" do
+      it 'does nothing when provided `only`' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(only: [:name, :username])
+        EOS
+
+        expect(cop.offenses).to be_empty
+      end
+
+      it 'does nothing when provided `only` and `methods`' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(
+            only: [:name, :username],
+            methods: [:avatar_url]
+          )
+        EOS
+
+        expect(cop.offenses).to be_empty
+      end
+
+      it 'adds an offense to `include`d attributes without `only` option' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(
+            include: {
+              milestone: {},
+              assignee: { methods: :avatar_url },
+              author: { only: %i[foo bar] },
+            }
+          )
+        EOS
+
+        expect(cop.offenses.size).to eq(2)
+        expect(cop.highlights).to contain_exactly(
+          'milestone: {}',
+          'assignee: { methods: :avatar_url }'
+        )
+        expect(cop.messages.first).to start_with("Don't use `#{method}`")
+      end
+
+      it 'handles a top-level `only` with child `include`s' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(
+            only: [:foo, :bar],
+            include: {
+              assignee: { methods: :avatar_url },
+              author: { only: %i[foo bar] }
+            }
+          )
+        EOS
+
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.highlights)
+          .to contain_exactly('assignee: { methods: :avatar_url }')
+        expect(cop.messages.first).to start_with("Don't use `#{method}`")
+      end
+
+      it 'adds an offense for `include: [...]`' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(include: %i[foo bar baz])
+        EOS
+
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.highlights).to contain_exactly('include: %i[foo bar baz]')
+        expect(cop.messages.first).to start_with("Don't use `#{method}`")
+      end
+
+      it 'adds an offense for `except`' do
+        inspect_source(cop, <<~EOS)
+          render json: @issue.#{method}(except: [:private_token])
+        EOS
+
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.highlights).to contain_exactly('except: [:private_token]')
+        expect(cop.messages.first).to start_with("Don't use `#{method}`")
+      end
+    end
+  end
+
+  it_behaves_like 'an upstanding constable', :to_json
+  it_behaves_like 'an upstanding constable', :as_json
+end
diff --git a/spec/rubocop/cop/security/to_json_spec.rb b/spec/rubocop/cop/security/to_json_spec.rb
deleted file mode 100644
index c81d41c0a55..00000000000
--- a/spec/rubocop/cop/security/to_json_spec.rb
+++ /dev/null
@@ -1,128 +0,0 @@
-require 'spec_helper'
-
-require 'rubocop'
-require 'rubocop/rspec/support'
-
-require_relative '../../../../rubocop/cop/security/to_json'
-
-describe RuboCop::Cop::Security::ToJson do
-  include CopHelper
-
-  subject(:cop) { described_class.new }
-
-  it 'ignores calls except `to_json`' do
-    inspect_source(cop, 'render json: foo')
-
-    expect(cop.offenses).to be_empty
-  end
-
-  context '`to_json` without options' do
-    it 'does nothing when sent to nil receiver' do
-      inspect_source(cop, 'to_json')
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'does nothing when sent to a Hash' do
-      inspect_source(cop, '{}.to_json')
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'does nothing when sent to a Serializer instance' do
-      inspect_source(cop, 'MergeRequestSerializer.new.represent(issuable).to_json')
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'adds an offense when sent to any other receiver' do
-      inspect_source(cop, 'foo.to_json')
-
-      aggregate_failures do
-        expect(cop.offenses.size).to eq(1)
-        expect(cop.highlights).to contain_exactly('foo.to_json')
-      end
-    end
-  end
-
-  context '`to_json` with options' do
-    it 'does nothing when provided `only`' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(only: [:name, :username])
-      EOS
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'does nothing when provided `only` and `methods`' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(
-          only: [:name, :username],
-          methods: [:avatar_url]
-        )
-      EOS
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'adds an offense to `include`d attributes without `only` option' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(
-          include: {
-            milestone: {},
-            assignee: { methods: :avatar_url },
-            author: { only: %i[foo bar] },
-          }
-        )
-      EOS
-
-      aggregate_failures do
-        expect(cop.offenses.size).to eq(2)
-        expect(cop.highlights).to contain_exactly(
-          'milestone: {}',
-          'assignee: { methods: :avatar_url }'
-        )
-      end
-    end
-
-    it 'handles a top-level `only` with child `include`s' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(
-          only: [:foo, :bar],
-          include: {
-            assignee: { methods: :avatar_url },
-            author: { only: %i[foo bar] }
-          }
-        )
-      EOS
-
-      aggregate_failures do
-        expect(cop.offenses.size).to eq(1)
-        expect(cop.highlights)
-          .to contain_exactly('assignee: { methods: :avatar_url }')
-      end
-    end
-
-    it 'adds an offense for `include: [...]`' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(include: %i[foo bar baz])
-      EOS
-
-      aggregate_failures do
-        expect(cop.offenses.size).to eq(1)
-        expect(cop.highlights).to contain_exactly('include: %i[foo bar baz]')
-      end
-    end
-
-    it 'adds an offense for `except`' do
-      inspect_source(cop, <<~EOS)
-        render json: @issue.to_json(except: [:private_token])
-      EOS
-
-      aggregate_failures do
-        expect(cop.offenses.size).to eq(1)
-        expect(cop.highlights).to contain_exactly('except: [:private_token]')
-      end
-    end
-  end
-end