Added firewall_zone unit tests and documented methods

package/yast2-network.spec

@@ -100,6 +100,7 @@ rake install DESTDIR="%{buildroot}"
 %{yast_schemadir}/autoyast/rnc/networking.rnc
 %{yast_schemadir}/autoyast/rnc/host.rnc
 %{yast_libdir}/network
+%{yast_libdir}/y2network
 %{yast_libdir}/y2remote
 %{yast_libdir}/cfa/
 %{yast_ydatadir}/network

src/include/network/lan/help.rb

@@ -223,16 +223,6 @@ def initialize_network_lan_help(_include_target)
                                _(
                                  "<p>DHCP configuration is not recommended for this product.\nComponents of this product might not work with DHCP.</p>"
                                ),
-        "fwzone"            =>
-                               _(
-                                 "<p><b><big>Firewall Zone</big></b></p>\n" \
-                                 "<p>Select the firewall zone to put the interface into. If you\n" \
-                                 "select a zone, the firewall will be enabled. If you do not and other \n" \
-                                 "firewalled interfaces exist, the firewall\n" \
-                                 "will stay enabled but all traffic will be blocked for this\n" \
-                                 "interface. If you do not select a zone and no others exist, \n" \
-                                 "the firewall will be disabled.</p>"
-                               ),
         "mandatory"         =>
                                _(
                                  "<p><b>Mandatory Interface</b> specifies whether the network service reports failure if the interface fails to start at boot time.</p>"

src/lib/y2network/widgets/firewall_zone.rb

@@ -1,82 +1,148 @@
+# encoding: utf-8
+#
+# Copyright (c) [2019] 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 "cwm"
 require "y2firewall/firewalld"
 require "y2firewall/helpers/interfaces"
 require "y2firewall/firewalld/interface"
 
 module Y2Network
   module Widgets
+    # This widget offers a checkbox for enabling the firewalld interface ZONE
+    # mapping through the ifcfg file and a selection list for choose the ZONE
+    # to be used.
     class FirewallZone < ::CWM::CustomWidget
       include Y2Firewall::Helpers::Interfaces
 
+      # Constructor
+      #
+      # @param name [String]
       def initialize(name)
         textdomain "network"
         @value = nil
         @interface = Y2Firewall::Firewalld::Interface.new(name)
       end
 
+      # @see CWM::AbstractWidget
+      # @return [String]
       def label
         _("Assign Interface to Firewall &Zone")
       end
 
+      # @see CWM::AbstractWidget
       def init
-        Yast::UI.ChangeWidget(Id(:zones), :Items, firewall_zones)
+        return unless installed?
+
+        populate_select(firewall_zones)
         self.value = @value
         enable_zones(managed?)
       end
 
+      # @see CWM::AbstractWidget
       def contents
+        return Label(_("Firewall is not installed.")) unless installed?
+
         VBox(
           Left(manage_widget),
           Left(zones_widget),
-          Left(current_zone_widget)
+          Left(permanent_config_widget)
         )
       end
 
+      # @see CWM::AbstractWidget
+      # @param event [Hash]
       def handle(event)
         enable_zones(managed?) if event["ID"] == :manage_zone
 
         nil
       end
 
+      # Stores the given name and when it is enabled to configure the
+      # interface ZONE through the ifcfg file. It also selects the given zone
+      # in the select list
+      #
+      # @see CWM::AbstractWidget
+      # @param name [String,nil] zone name
       def value=(name)
         @value = name
-        Yast::UI.ChangeWidget(Id(:manage_zone), :Value, !!name)
-        return if name.nil?
-        select_zone(name)
+        return unless installed?
+        manage_zone!(!!name) && select_zone(name)
       end
 
+      # It returns the current ZONE selection or nil in case of not enabled
+      # the management through the ifcfg files.
+      #
+      # @return [String, nil] current zone or nil when not managed
       def value
         return @value unless Yast::UI.WidgetExists(Id(:manage_zone))
 
         managed? ? zone : nil
       end
 
+      # Stores the current value
+      #
+      # @see CWM::AbstractWidget
+      # @return [String, nil]
       def store
         @value = value
       end
 
+      # Stores the selected zone permanently when it has change and it is
+      # enabled to be managed through the ifcfg files
+      #
+      # @return [String, nil] the current zone selection
       def store_zone
-        @interface.zone = converted_value if zone_changed?
+        return @value unless installed?
+
+        @interface.zone = @value if zone_changed?
         @value
       end
 
-    private
-
-      def converted_value
-        return "" if @value == ""
-        @value
+      # @see CWM::AbstractWidget
+      def help
+        _("<p><b><big>FIREWALL ZONE</big></b></p>" \
+          "<p>A network zone defines the level of trust for network connections. " \
+          "The <b>ZONE</b> can be set by yast-firewall, by different firewalld" \
+          "utilities or via the ifcfg file.</p>" \
+          "<p>When enabled in yast-network, it sets the <b>ZONE</b> which this " \
+          "interface belongs to modifying also the firewalld permanent " \
+          "configuration</p>")
       end
 
+    private
+
+      # Return whether the permanent ZONE match or not the selected one.
+      #
+      # @return [Boolean]
       def zone_changed?
-        @value && (current_zone.to_s != converted_value)
+        @value && (current_zone.to_s != @value)
       end
 
-      def current_zone_widget
-        label = current_zone ? current_zone : _("DEFAULT")
+      def permanent_config_widget
+        label = current_zone ? current_zone : default_label
 
         VBox(
           VSpacing(1),
-          Label(_("Current ZONE (permanent config): %s") % label)
+          Label(_("Current ZONE (permanent config): %s") % label),
+          Label(_("Default ZONE (permanent config): %s") % firewalld.default_zone)
         )
       end
 
@@ -86,7 +152,12 @@ def current_zone
       end
 
       def manage_widget
-        Yast::UI.CheckBox(Id(:manage_zone), Opt(:notify), _("Define Ifcfg ZONE"))
+        CheckBox(Id(:manage_zone), Opt(:notify), _("Define Ifcfg ZONE"))
+      end
+
+      def manage_zone!(value)
+        Yast::UI.ChangeWidget(Id(:manage_zone), :Value, value)
+        value
       end
 
       def managed?
@@ -109,21 +180,28 @@ def enable_zones(value)
         Yast::UI.ChangeWidget(Id(:zones), :Enabled, value)
       end
 
+      def populate_select(zones)
+        items = zones.map { |z| Item(Id(z[0]), z[1]) }
+        Yast::UI.ChangeWidget(Id(:zones), :Items, items)
+      end
+
       # Return a list of items for ComboBox with all the known firewalld zones
       # and also an empty string option for the default zone.
       #
       # @return [Array <Array <String, String>>] list of names an description of
       # known zones
       def firewall_zones
-        zones = [["", _("Default")]]
+        zones = [["", default_label]]
+        firewalld.zones.each { |z| zones << [z.name, z.short] }
+        zones
+      end
 
-        if firewalld.installed?
-          firewalld.zones.each { |z| zones << [z.name, z.short] }
-        else
-          zones = [["", _("Firewall is not installed.")]]
-        end
+      def default_label
+        _("DEFAULT")
+      end
 
-        zones.map { |z| Item(Id(z[0]), z[1]) }
+      def installed?
+        @installed ||= firewalld.installed?
       end
     end
   end

test/lib/y2network/widgets/firewall_zone_test.rb

@@ -0,0 +1,154 @@
+#!/usr/bin/env rspec
+
+require_relative "../../../test_helper.rb"
+require "y2network/widgets/firewall_zone"
+
+require "cwm/rspec"
+
+describe Y2Network::Widgets::FirewallZone do
+  let(:subject) { described_class.new("eth0") }
+  let(:firewalld) { Y2Firewall::Firewalld.instance }
+  let(:firewall_zones) { [["", "Default"], ["custom", "custom"]] }
+  let(:managed?) { true }
+  let(:installed?) { true }
+
+  before do
+    allow(firewalld).to receive(:installed?).and_return(installed?)
+    allow(subject).to receive(:firewall_zones).and_return(firewall_zones)
+    allow(subject).to receive(:managed?).and_return(managed?)
+  end
+
+  include_examples "CWM::CustomWidget"
+
+  describe "#init" do
+    it "populates the zones list with the firewalld zones" do
+      expect(subject).to receive(:populate_select).with(firewall_zones)
+      subject.init
+    end
+
+    it "selects the current zone from the list if it was cached" do
+      subject.value = "custom"
+      expect(subject).to receive(:select_zone).with("custom")
+      subject.init
+    end
+
+    it "enables / disables the selection of zone" do
+      allow(subject).to receive(:managed?).and_return(false, true)
+      expect(subject).to receive(:enable_zones).with(false)
+      subject.init
+      expect(subject).to receive(:enable_zones).with(true)
+      subject.init
+    end
+  end
+
+  describe "#value" do
+    before do
+      allow(Yast::UI).to receive(:WidgetExists).and_return(true)
+    end
+
+    context "when the checkbox is not checked" do
+      let(:managed?) { false }
+
+      it "returns nil" do
+        expect(subject.value).to eql(nil)
+      end
+    end
+
+    context "when the checkbox is checked" do
+      it "returns the selected element" do
+        expect(subject).to receive(:zone).and_return("")
+        expect(subject.value).to eql("")
+      end
+    end
+
+    context "when the checkbox widget does not exist" do
+      before do
+        allow(Yast::UI).to receive(:WidgetExists).and_return(false)
+      end
+
+      it "returns the cached value" do
+        expect(subject.value).to eql(nil)
+        subject.value = "external"
+        expect(subject.value).to eql("external")
+      end
+    end
+  end
+
+  describe "#store" do
+    before do
+      allow(Yast::UI).to receive(:WidgetExists).and_return(true, false)
+    end
+
+    it "caches the current value" do
+      expect(subject).to receive(:managed?).and_return(true)
+      expect(subject).to receive(:zone).and_return("external")
+      subject.store
+      expect(subject).to_not receive(:managed?)
+      expect(subject.store).to eql("external")
+    end
+  end
+
+  describe "#store_zone" do
+    before do
+      subject.value = "custom"
+    end
+
+    context "when firewalld is not installed" do
+      let(:installed?) { false }
+
+      it "returns the cached value" do
+        expect(subject.store_zone).to eql("custom")
+      end
+    end
+
+    context "when firewalld is installed" do
+      context "but the firewall zone will not be managed by the ifcfg file" do
+        let(:managed?) { false }
+
+        it "returns the cached value" do
+          expect(subject.store_zone).to eql("custom")
+        end
+      end
+
+      context "and the cached value is not equal to the firewalld interface zone" do
+        it "modifies the firewalld interface ZONE" do
+          allow(subject).to receive(:current_zone).and_return("external")
+          expect_any_instance_of(Y2Firewall::Firewalld::Interface).to receive(:zone=).with("custom")
+          subject.store_zone
+        end
+      end
+
+      context "and the cached value is the same than the firewalld interface zone" do
+
+      end
+    end
+  end
+
+  describe "#handle" do
+    it "returns nil" do
+      expect(subject.handle("ID" => "fake_event")).to eql(nil)
+    end
+
+    context "when the checkbox is checked" do
+      before do
+        allow(subject).to receive(:managed?).and_return(true)
+      end
+
+      it "enables the zone list selection" do
+        expect(subject).to receive(:enable_zones).with(true)
+        subject.handle("ID" => :manage_zone)
+      end
+    end
+
+    context "then the checkbox is unchecked" do
+      before do
+        allow(subject).to receive(:managed?).and_return(false)
+      end
+
+      it "disables the zone list selection when not checked" do
+        expect(subject).to receive(:enable_zones).with(false)
+        subject.handle("ID" => :manage_zone)
+      end
+    end
+  end
+end