Small changes for making the unit testing easier

library/network/src/lib/y2firewall/firewalld.rb

@@ -51,12 +51,14 @@ class Firewalld
     include Yast::Logger
     extend Forwardable
 
-    # @return Y2Firewall::Firewalld::Api instance
-    attr_accessor :api
+    # @param Y2Firewall::Firewalld::Api instance
+    attr_writer :api
     # @return [Array <Y2Firewall::Firewalld::Zone>] firewalld zones
     attr_accessor :zones
-    # @return [Array <String>] current zone names
-    attr_accessor :current_zones
+    # @return [Array <String>] current zone names.
+    attr_accessor :current_zone_names
+    # @return [Array <String>] current service names.
+    attr_accessor :current_service_names
     # @return [Array <Y2Firewall::Firewalld::Service>] firewalld services. To
     # avoid performance problems it is empty by default and the services are
     # added when needed by the find_service method.
@@ -70,11 +72,12 @@ class Firewalld
     PACKAGE = "firewalld".freeze
     SERVICE = "firewalld".freeze
 
-    def_delegators :@api, :enable!, :disable!, :reload, :running?
+    def_delegators :api, :enable!, :disable!, :reload, :running?
 
     # Constructor
     def initialize
-      @api = Api.new
+      @current_zone_names = []
+      @current_service_names = []
       @zones = []
       @services = []
       @read = false
@@ -86,8 +89,9 @@ def initialize
     # @return [Boolean] true
     def read
       return false unless installed?
-      @current_zones = api.zones
       @zones = zone_parser.parse
+      @current_zone_names = api.zones
+      @current_service_names = api.services
       @log_denied_packets = api.log_denied_packets
       @default_zone       = api.default_zone
       # The list of services is not read or initialized because takes time and
@@ -171,10 +175,10 @@ def write_only
     # delete all the new or removed zones depending on each case.
     def apply_zones_changes!
       zones.each do |zone|
-        api.create_zone(zone.name) unless current_zones.include?(zone.name)
+        api.create_zone(zone.name) unless current_zone_names.include?(zone.name)
         zone.apply_changes! if zone.modified?
       end
-      current_zones.each do |name|
+      current_zone_names.each do |name|
         api.delete_zone(name) unless zones.any? { |zone| zone.name == name }
       end
       true
@@ -184,7 +188,7 @@ def apply_zones_changes!
     #
     # @return [Boolean] true if some zone have changed; false otherwise
     def zones_modified?
-      (current_zones.sort != zones.map(&:name).sort) || zones.any?(&:modified?)
+      (current_zone_names.sort != zones.map(&:name).sort) || zones.any?(&:modified?)
     end
 
     # Return a map with current firewalld settings.
@@ -254,12 +258,23 @@ def read?
       @read
     end
 
+    # Convenience method for initializing and retrieving an API instance
+    def api
+      @api ||= Api.new
+    end
+
   private
 
+    # Convenience method for instantiate a new zone parser
+    #
+    # @return [ZoneParser]
     def zone_parser
       ZoneParser.new(api.zones, api.list_all_zones(verbose: true))
     end
 
+    # Convenience method for instantiate a services parser
+    #
+    # @return [ServiceParser]
     def service_parser
       ServiceParser.new
     end

library/network/src/lib/y2firewall/firewalld/api.rb

@@ -109,7 +109,7 @@ def disable!
       # @return [String] firewalld service state
       # @see http://www.firewalld.org/documentation/man-pages/firewall-cmd.html
       def state
-        case Yast::Execute.on_target("firewall-cmd", "--state", allowed_exitstatus: [0, 252])
+        case Yast::Execute.on_target!("firewall-cmd", "--state", allowed_exitstatus: [0, 252])
         when 0
           "running"
         when 252
@@ -197,7 +197,7 @@ def run_command(*args, permanent: false, allowed_exitstatus: nil)
         arguments.concat(args)
         log.info("Executing #{command} with #{arguments.inspect}")
 
-        Yast::Execute.on_target(
+        Yast::Execute.on_target!(
           command, *arguments, stdout: :capture, allowed_exitstatus: allowed_exitstatus
         )
       end

library/network/src/lib/y2firewall/firewalld/api/zones.rb

@@ -38,15 +38,15 @@ def zones
         #
         # @param zone [String] The firewall zone name
         def create_zone(zone)
-          run_command("--new-zone=#{zone}", permanent: offline? ? false : true)
+          run_command("--new-zone=#{zone}", permanent: !offline?)
         end
 
         # Delete the given zone from firewalld. Deleted zones must be deleted
         # permanently
         #
         # @param zone [String] The firewall zone name to be deleted
         def delete_zone(zone)
-          run_command("--delete-zone=#{zone}", permanent: offline? ? false : true)
+          run_command("--delete-zone=#{zone}", permanent: !offline?)
         end
 
         # @param zone [String] The firewall zone
@@ -342,48 +342,73 @@ def remove_protocol(zone, protocol, permanent: permanent?)
 
         # @param zone [String] The firewall zone
         # @return [Boolean] True if masquerade is enabled in zone
-        def masquerade_enabled?(zone)
-          query_command("--zone=#{zone}", "--query-masquerade")
+        def masquerade_enabled?(zone, permanent: permanent?)
+          query_command("--zone=#{zone}", "--query-masquerade", permanent: permanent)
         end
 
         # @param zone [String] The firewall zone
         # @return [Boolean] True if masquerade was enabled in zone
-        def add_masquerade(zone)
-          return true if masquerade_enabled?(zone)
+        def add_masquerade(zone, permanent: permanent?)
+          return true if masquerade_enabled?(zone, permanent: permanent)
 
-          run_command("--zone=#{zone}", "--add-masquerade")
+          run_command("--zone=#{zone}", "--add-masquerade", permanent: permanent)
         end
 
         # @param zone [String] The firewall zone
         # @return [Boolean] True if masquerade was removed in zone
-        def remove_masquerade(zone)
-          return true if !masquerade_enabled?(zone)
+        def remove_masquerade(zone, permanent: permanent?)
+          return true if !masquerade_enabled?(zone, permanent: permanent)
 
-          run_command("--zone=#{zone}", "--remove-masquerade")
+          run_command("--zone=#{zone}", "--remove-masquerade", permanent: permanent)
         end
 
+        # Full name or short description of the zone
+        #
+        # @param zone [String] The firewall zone
         def short(zone)
-          string_command("--zone=#{zone}", "--get-short")
+          string_command("--zone=#{zone}", "--get-short", permanent: !offline?)
         end
 
+        # Modiy the full name or short description of the zone
+        #
+        # @param zone [String] The firewall zone
+        # @param short_description [String] the new zone name or description
         def short=(zone, short_description)
-          string_command("--zone=#{zone}", "--set-short=#{short_description}")
+          string_command("--zone=#{zone}", "--set-short=#{short_description}",
+            permanent: !offline?)
         end
 
+        # Long description of the zone
+        #
+        # @param zone [String] The firewall zone
         def description(zone)
-          string_command("--zone=#{zone}", "--get-description")
+          string_command("--zone=#{zone}", "--get-description",
+            permanent: !offline?)
         end
 
+        # Modiy the long description of the zone
+        #
+        # @param zone [String] The firewall zone
+        # @param long_description [String] the new zone description
         def description=(zone, long_description)
-          run_command("--zone=#{zone}", "--set-description=#{long_description}")
+          run_command("--zone=#{zone}", "--set-description=#{long_description}",
+            permanent: !offline?)
         end
 
+        # The target of the zone
+        #
+        # @param zone [String] The firewall zone
         def target(zone)
           string_command("--zone=#{zone}", "--get-target")
         end
 
+        # Modiy the current target of the zone
+        #
+        # @param zone [String] The firewall zone
+        # @param target [String] the new target
         def target=(zone, target)
-          run_command("--zone=#{zone}", "--set-target=#{target}")
+          run_command("--zone=#{zone}", "--set-target=#{target}",
+            permanent: !offline?)
         end
       end
     end

library/network/test/y2firewall/firewalld/service_parser_test.rb

@@ -25,9 +25,15 @@
 require "y2firewall/firewalld/service_parser"
 
 describe Y2Firewall::Firewalld::ServiceParser do
+  let(:firewalld) { Y2Firewall::Firewalld.instance }
+  let(:api) { instance_double("Y2Firewall::Firewalld::Api", state: "not_running") }
+
+  before do
+    allow(firewalld).to receive(:api).and_return(api)
+    `echo true`
+  end
+
   describe "#parse" do
-    let(:firewalld) { Y2Firewall::Firewalld.instance }
-    let(:api) { Y2Firewall::Firewalld::Api.new }
     let(:service_info) do
       [
         "radius",
@@ -44,16 +50,14 @@
       ]
     end
 
-    before do
-      allow(firewalld).to receive(:api).and_return(api)
-    end
-
     context "when the service is not present" do
       let(:service_name) { "not_present" }
-      it "raises a non Found exception" do
-        expect(api).to receive(:info_service).with(service_name, verbose: true)
-        expect($CHILD_STATUS).to receive(:exitstatus).and_return(101)
+      before do
+        allow(api).to receive(:info_service).with(service_name, verbose: true)
+        allow($CHILD_STATUS).to receive(:exitstatus).and_return(101)
+      end
 
+      it "raises a non Found exception" do
         expect { subject.parse(service_name) }.to raise_error(Y2Firewall::Firewalld::Service::NotFound)
       end
     end

library/network/test/y2firewall/firewalld/service_test.rb

@@ -0,0 +1,72 @@
+#!/usr/bin/env rspec
+# encoding: utf-8
+#
+# Copyright (c) [2018] SUSE LLC
+#
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of version 2 of the GNU General Public License as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may
+# find current contact information at www.suse.com.
+
+require_relative "../../test_helper"
+require "y2firewall/firewalld"
+require "y2firewall/firewalld/service"
+
+describe Y2Firewall::Firewalld::Service do
+  let(:firewalld) { Y2Firewall::Firewalld.instance }
+  let(:api) { instance_double("Y2Firewall::Firewalld::Api") }
+  let(:installed?) { true }
+
+  before do
+    allow(firewalld).to receive(:find_service).with("service")
+    allow(firewalld).to receive(:api).and_return(api)
+    allow(firewalld).to receive(:installed?).and_return(installed?)
+  end
+
+  describe ".modify_ports" do
+    subject { described_class }
+
+    let(:service) { described_class.new(name: "service") }
+
+    context "when firewalld is not installed" do
+      let(:installed?) { false }
+
+      it "returns false" do
+        expect(subject.modify_ports(name: "service", tcp_ports: ["80", "8080"])).to eq(false)
+      end
+    end
+
+    context "when firewalld is installed" do
+      before do
+        allow(service).to receive(:ports=)
+        allow(service).to receive(:apply_changes!)
+        allow(firewalld).to receive(:find_service).with("service").and_return(service)
+      end
+
+      it "looks for the the service with the name given if exists" do
+        expect(firewalld).to receive(:find_service).with("service").and_return(service)
+
+        subject.modify_ports(name: "service", tcp_ports: ["80"])
+      end
+
+      it "modifies the service tcp and udp ports" do
+        expect(service).to receive(:ports=).with(["80/tcp", "8080/tcp", "53/udp"])
+        expect(service).to receive(:apply_changes!)
+
+        subject.modify_ports(name: "service", tcp_ports: ["80", "8080"], udp_ports: ["53"])
+      end
+    end
+  end
+end

library/network/test/y2firewall/firewalld/zone_test.rb

@@ -22,13 +22,16 @@
 
 require_relative "../../test_helper"
 require "y2firewall/firewalld"
+require "y2firewall/firewalld/api"
 require "y2firewall/firewalld/zone"
 
 describe Y2Firewall::Firewalld::Zone do
   let(:firewalld) { Y2Firewall::Firewalld.instance }
+  let(:api) { instance_double("Y2Firewall::Firewalld::Api", default_zone: "default") }
 
   before do
     allow(firewalld).to receive(:installed?).and_return(true)
+    allow(firewalld).to receive(:api).and_return(api)
   end
 
   describe "#initialize" do
@@ -40,11 +43,7 @@
     end
 
     context "when :name is not specified" do
-      let(:api) { instance_double("Y2Firewall::Firewalld::Api", default_zone: "default") }
-
       it "uses the default zone name" do
-        allow(firewalld).to receive(:api).and_return(api)
-
         expect(subject.name).to eq("default")
       end
     end