From a3801d66f5cf3f08062c3aa67a7b33d13fae56b7 Mon Sep 17 00:00:00 2001

From: Martin Babinsky <mbabinsk@redhat.com>

Date: Thu, 11 May 2017 15:55:53 +0200

Subject: [PATCH] Allow for multivalued server attributes

In order to achieve the task, the following changes were required:

* vectorize the base class for server attributes

* add a child class that enforces single-value attributes. It still

  accepts/returns single-value lists in order to not break Liskov

  substitution principle

* Existing attributes inherit from the child class

https://pagure.io/freeipa/issue/6937

Reviewed-By: Jan Cholasta <jcholast@redhat.com>

Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>

---

 ipaserver/plugins/serverroles.py            |   4 +-

 ipaserver/servroles.py                      | 109 +++++++++++++++++++---------

 ipatests/test_ipaserver/test_serverroles.py |  10 +--

 3 files changed, 79 insertions(+), 44 deletions(-)

diff --git a/ipaserver/plugins/serverroles.py b/ipaserver/plugins/serverroles.py

index e22eadd7b163469cc9fc4472640aa64d21c9d38f..e81635c3315cc3fca84450f43fb7df883aae57d9 100644

--- a/ipaserver/plugins/serverroles.py

+++ b/ipaserver/plugins/serverroles.py

@@ -136,9 +136,7 @@ class serverroles(Backend):

         for name, attr in assoc_attributes.items():

             attr_value = attr.get(self.api)

-

-            if attr_value is not None:

-                result.update({name: attr_value})

+            result.update({name: attr_value})

         return result

diff --git a/ipaserver/servroles.py b/ipaserver/servroles.py

index cf4599995118c589a5b51236c68e13f14ac1257b..84fed1046b3b46dc9f5f8bbe6e03354725f1136c 100644

--- a/ipaserver/servroles.py

+++ b/ipaserver/servroles.py

@@ -277,29 +277,33 @@ class ServerAttribute(LDAPBasedProperty):

         try:

             entries = ldap2.get_entries(search_base, filter=search_filter)

         except errors.EmptyResult:

-            return

+            return []

-        master_cn = entries[0].dn[1]['cn']

+        master_cns = {e.dn[1]['cn'] for e in entries}

         associated_role_providers = set(

             self._get_assoc_role_providers(api_instance))

-        if master_cn not in associated_role_providers:

+        if not master_cns.issubset(associated_role_providers):

             raise errors.ValidationError(

                 name=self.name,

                 error=_("all masters must have %(role)s role enabled" %

                         {'role': self.associated_role.name})

             )

-        return master_cn

+        return sorted(master_cns)

-    def _get_master_dn(self, api_instance, server):

-        return DN(('cn', server), api_instance.env.container_masters,

-                  api_instance.env.basedn)

+    def _get_master_dns(self, api_instance, servers):

+        return [

+            DN(('cn', server), api_instance.env.container_masters,

+               api_instance.env.basedn) for server in servers]

+

+    def _get_masters_service_entries(self, ldap, master_dns):

+        service_dns = [

+            DN(('cn', self.associated_service_name), master_dn) for master_dn

+            in master_dns]

-    def _get_masters_service_entry(self, ldap, master_dn):

-        service_dn = DN(('cn', self.associated_service_name), master_dn)

-        return ldap.get_entry(service_dn)

+        return [ldap.get_entry(service_dn) for service_dn in service_dns]

     def _add_attribute_to_svc_entry(self, ldap, service_entry):

         """

@@ -341,65 +345,98 @@ class ServerAttribute(LDAPBasedProperty):

             r[u'server_server'] for r in self.associated_role.status(

                 api_instance) if r[u'status'] == ENABLED]

-    def _remove(self, api_instance, master):

+    def _remove(self, api_instance, masters):

         """

-        remove attribute from the master

+        remove attribute from one or more masters

         :param api_instance: API instance

-        :param master: master FQDN

+        :param master: list or iterable containing master FQDNs

         """

         ldap = api_instance.Backend.ldap2

-        master_dn = self._get_master_dn(api_instance, master)

-        service_entry = self._get_masters_service_entry(ldap, master_dn)

-        self._remove_attribute_from_svc_entry(ldap, service_entry)

+        master_dns = self._get_master_dns(api_instance, masters)

+        service_entries = self._get_masters_service_entries(ldap, master_dns)

+

+        for service_entry in service_entries:

+            self._remove_attribute_from_svc_entry(ldap, service_entry)

-    def _add(self, api_instance, master):

+    def _add(self, api_instance, masters):

         """

         add attribute to the master

         :param api_instance: API instance

-        :param master: master FQDN

+        :param master: iterable containing master FQDNs

         :raises: * errors.ValidationError if the associated role is not enabled

                    on the master

         """

-        assoc_role_providers = self._get_assoc_role_providers(api_instance)

+        assoc_role_providers = set(

+            self._get_assoc_role_providers(api_instance))

+        masters_set = set(masters)

         ldap = api_instance.Backend.ldap2

-        if master not in assoc_role_providers:

+        masters_without_role = masters_set - assoc_role_providers

+

+        if masters_without_role:

             raise errors.ValidationError(

-                name=master,

+                name=', '.join(sorted(masters_without_role)),

                 error=_("must have %(role)s role enabled" %

                         {'role': self.associated_role.name})

             )

-        master_dn = self._get_master_dn(api_instance, master)

-        service_entry = self._get_masters_service_entry(ldap, master_dn)

-        self._add_attribute_to_svc_entry(ldap, service_entry)

+        master_dns = self._get_master_dns(api_instance, masters)

+        service_entries = self._get_masters_service_entries(ldap, master_dns)

+        for service_entry in service_entries:

+            self._add_attribute_to_svc_entry(ldap, service_entry)

-    def set(self, api_instance, master):

+    def set(self, api_instance, masters):

         """

-        set the attribute on master

+        set the attribute on masters

         :param api_instance: API instance

-        :param master: FQDN of the new master

+        :param masters: an interable with FQDNs of the new masters

-        the attribute is automatically unset from previous master if present

+        the attribute is automatically unset from previous masters if present

         :raises: errors.EmptyModlist if the new masters is the same as

-                 the original on

+                 the original ones

         """

-        old_master = self.get(api_instance)

+        old_masters = self.get(api_instance)

-        if old_master == master:

+        if sorted(old_masters) == sorted(masters):

             raise errors.EmptyModlist

-        self._add(api_instance, master)

+        if old_masters:

+            self._remove(api_instance, old_masters)

+

+        self._add(api_instance, masters)

+

+

+class SingleValuedServerAttribute(ServerAttribute):

+    """

+    Base class for server attributes that are forced to be single valued

+

+    this means that `get` method will return a one-element list, and `set`

+    method will accept only one-element list

+    """

+

+    def set(self, api_instance, masters):

+        if len(masters) > 1:

+            raise errors.ValidationError(

+                name=self.attr_name,

+                error=_("must be enabled only on a single master"))

+

+        super(SingleValuedServerAttribute, self).set(api_instance, masters)

+

+    def get(self, api_instance):

+        masters = super(SingleValuedServerAttribute, self).get(api_instance)

+        num_masters = len(masters)

+

+        if num_masters > 1:

+            raise errors.SingleMatchExpected(found=num_masters)

-        if old_master is not None:

-            self._remove(api_instance, old_master)

+        return masters

 _Service = namedtuple('Service', ['name', 'enabled'])

@@ -574,14 +611,14 @@ role_instances = (

 )

 attribute_instances = (

-    ServerAttribute(

+    SingleValuedServerAttribute(

         u"ca_renewal_master_server",

         u"CA renewal master",

         u"ca_server_server",

         u"CA",

         u"caRenewalMaster",

     ),

-    ServerAttribute(

+    SingleValuedServerAttribute(

         u"dnssec_key_master_server",

         u"DNSSec key master",

         u"dns_server_server",

diff --git a/ipatests/test_ipaserver/test_serverroles.py b/ipatests/test_ipaserver/test_serverroles.py

index d8844df3007f076532a34d625379ab552ef45363..e671272783d8d71c2ee56074459433b98b79dd0a 100644

--- a/ipatests/test_ipaserver/test_serverroles.py

+++ b/ipatests/test_ipaserver/test_serverroles.py

@@ -706,7 +706,7 @@ class TestServerAttributes(object):

         actual_attr_masters = self.config_retrieve(

             assoc_role, mock_api)[attr_name]

-        assert actual_attr_masters == fqdn

+        assert actual_attr_masters == [fqdn]

     def test_set_attribute_on_the_same_provider_raises_emptymodlist(

             self, mock_api, mock_masters):

@@ -727,7 +727,7 @@ class TestServerAttributes(object):

         non_ca_fqdn = mock_masters.get_fqdn('trust-controller-dns')

         with pytest.raises(errors.ValidationError):

-            self.config_update(mock_api, **{attr_name: non_ca_fqdn})

+            self.config_update(mock_api, **{attr_name: [non_ca_fqdn]})

     def test_set_unknown_attribute_on_master_raises_notfound(

             self, mock_api, mock_masters):

@@ -735,7 +735,7 @@ class TestServerAttributes(object):

         fqdn = mock_masters.get_fqdn('trust-controller-ca')

         with pytest.raises(errors.NotFound):

-            self.config_update(mock_api, **{attr_name: fqdn})

+            self.config_update(mock_api, **{attr_name: [fqdn]})

     def test_set_ca_renewal_master_on_other_ca_and_back(self, mock_api,

                                                         mock_masters):

@@ -747,7 +747,7 @@ class TestServerAttributes(object):

         other_ca_server = mock_masters.get_fqdn('trust-controller-ca')

         for host in (other_ca_server, original_renewal_master):

-            self.config_update(mock_api, **{attr_name: host})

+            self.config_update(mock_api, **{attr_name: [host]})

             assert (

-                self.config_retrieve(role_name, mock_api)[attr_name] == host)

+                self.config_retrieve(role_name, mock_api)[attr_name] == [host])

-- 

2.9.4