Fix DNS permissions and membership in privileges

This resolves two issues:

1. The DNS acis lacked a prefix so weren't tied to permissions
2. The permissions were added before the privileges so the member
   values weren't calculated properly

For updates we need to add in the members and recalculate memberof via
a DS task.

https://fedorahosted.org/freeipa/ticket/1898

6 files changed, 67 insertions, 25 deletions

diff --git a/install/share/dns.ldif b/install/share/dns.ldif
index dc7922218..1ffadb5a9 100644
--- a/install/share/dns.ldif
+++ b/install/share/dns.ldif
@@ -4,6 +4,29 @@ objectClass: nsContainer
 objectClass: top
 cn: dns
 
+dn: $SUFFIX
+changetype: modify
+add: aci
+aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:add dns entries";allow (add) groupdn = "ldap:///cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX";)
+aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:remove dns entries";allow (delete) groupdn = "ldap:///cn=remove dns entries,cn=permissions,cn=pbac,$SUFFIX";)
+aci: (targetattr = "idnsname || cn || idnsallowdynupdate || dnsttl || dnsclass || arecord || aaaarecord || a6record || nsrecord || cnamerecord || ptrrecord || srvrecord || txtrecord || mxrecord || mdrecord || hinforecord || minforecord || afsdbrecord || sigrecord || keyrecord || locrecord || nxtrecord || naptrrecord || kxrecord || certrecord || dnamerecord || dsrecord || sshfprecord || rrsigrecord || nsecrecord || idnsname || idnszoneactive || idnssoamname || idnssoarname || idnssoaserial || idnssoarefresh || idnssoaretry || idnssoaexpire || idnssoaminimum || idnsupdatepolicy")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:update dns entries";allow (write) groupdn = "ldap:///cn=update dns entries,cn=permissions,cn=pbac,$SUFFIX";)
+
+dn: cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX
+changetype: add
+objectClass: top
+objectClass: groupofnames
+objectClass: nestedgroup
+cn: DNS Administrators
+description: DNS Administrators
+
+dn: cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX
+changetype: add
+objectClass: top
+objectClass: groupofnames
+objectClass: nestedgroup
+cn: DNS Servers
+description: DNS Servers
+
 dn: cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX
 changetype: add
 objectClass: groupofnames
@@ -30,26 +53,3 @@ cn: update dns entries
 description: Update DNS entries
 member: cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX
 member: cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX
-
-dn: $SUFFIX
-changetype: modify
-add: aci
-aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Add DNS entries";allow (add) groupdn = "ldap:///cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX";)
-aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Remove DNS entries";allow (delete) groupdn = "ldap:///cn=remove dns entries,cn=permissions,cn=pbac,$SUFFIX";)
-aci: (targetattr = "idnsname || cn || idnsallowdynupdate || dnsttl || dnsclass || arecord || aaaarecord || a6record || nsrecord || cnamerecord || ptrrecord || srvrecord || txtrecord || mxrecord || mdrecord || hinforecord || minforecord || afsdbrecord || sigrecord || keyrecord || locrecord || nxtrecord || naptrrecord || kxrecord || certrecord || dnamerecord || dsrecord || sshfprecord || rrsigrecord || nsecrecord || idnsname || idnszoneactive || idnssoamname || idnssoarname || idnssoaserial || idnssoarefresh || idnssoaretry || idnssoaexpire || idnssoaminimum || idnsupdatepolicy")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Update DNS entries";allow (write) groupdn = "ldap:///cn=update dns entries,cn=permissions,cn=pbac,$SUFFIX";)
-
-dn: cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX
-changetype: add
-objectClass: top
-objectClass: groupofnames
-objectClass: nestedgroup
-cn: DNS Administrators
-description: DNS Administrators
-
-dn: cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX
-changetype: add
-objectClass: top
-objectClass: groupofnames
-objectClass: nestedgroup
-cn: DNS Servers
-description: DNS Servers
diff --git a/install/tools/man/ipa-ldap-updater.1 b/install/tools/man/ipa-ldap-updater.1
index ed140b336..d896a1bed 100644
--- a/install/tools/man/ipa-ldap-updater.1
+++ b/install/tools/man/ipa-ldap-updater.1
@@ -40,6 +40,7 @@ There are 7 keywords:
     * deleteentry: remove the entry
     * replace: replace an existing value, format is old: new
     * addifnew: add a new attribute and value only if the attribute doesn't already exist. Only works with single\-value attributes.
+    * addifexist: add a new attribute and value only if the entry exists. This is used to update optional entries.
 
 Values is a comma\-separated field so multi\-values may be added at one time. Double or single quotes may be put around individual values that contain embedded commas.
 
diff --git a/install/updates/40-delegation.update b/install/updates/40-delegation.update
index 66c62ed54..a23521166 100644
--- a/install/updates/40-delegation.update
+++ b/install/updates/40-delegation.update
@@ -262,3 +262,9 @@ add:member: 'cn=admins,cn=groups,cn=accounts,$SUFFIX'
 # Don't allow admins to update enrolledBy
 dn: $SUFFIX
 replace:aci:'(targetattr = "enrolledby || objectclass")(target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version 3.0;acl "permission:Enroll a host";allow (write) groupdn = "ldap:///cn=Enroll a host,cn=permissions,cn=pbac,$SUFFIX";)::(targetattr = "objectclass")(target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version 3.0;acl "permission:Enroll a host";allow (write) groupdn = "ldap:///cn=Enroll a host,cn=permissions,cn=pbac,$SUFFIX";)'
+
+# The original DNS permissions lacked the tag.
+dn: $SUFFIX
+replace:aci:'(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Add DNS entries";allow (add) groupdn = "ldap:///cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX";)::(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:add dns entries";allow (add) groupdn = "ldap:///cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX";)'
+replace:aci:'(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Remove DNS entries";allow (delete) groupdn = "ldap:///cn=remove dns entries,cn=permissions,cn=pbac,$SUFFIX";)::(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:remove dns entries";allow (delete) groupdn = "ldap:///cn=remove dns entries,cn=permissions,cn=pbac,$SUFFIX";)'
+replace:aci:'(targetattr = "idnsname || cn || idnsallowdynupdate || dnsttl || dnsclass || arecord || aaaarecord || a6record || nsrecord || cnamerecord || ptrrecord || srvrecord || txtrecord || mxrecord || mdrecord || hinforecord || minforecord || afsdbrecord || sigrecord || keyrecord || locrecord || nxtrecord || naptrrecord || kxrecord || certrecord || dnamerecord || dsrecord || sshfprecord || rrsigrecord || nsecrecord || idnsname || idnszoneactive || idnssoamname || idnssoarname || idnssoaserial || idnssoarefresh || idnssoaretry || idnssoaexpire || idnssoaminimum || idnsupdatepolicy")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Update DNS entries";allow (write) groupdn = "ldap:///cn=update dns entries,cn=permissions,cn=pbac,$SUFFIX";)::(targetattr = "idnsname || cn || idnsallowdynupdate || dnsttl || dnsclass || arecord || aaaarecord || a6record || nsrecord || cnamerecord || ptrrecord || srvrecord || txtrecord || mxrecord || mdrecord || hinforecord || minforecord || afsdbrecord || sigrecord || keyrecord || locrecord || nxtrecord || naptrrecord || kxrecord || certrecord || dnamerecord || dsrecord || sshfprecord || rrsigrecord || nsecrecord || idnsname || idnszoneactive || idnssoamname || idnssoarname || idnssoaserial || idnssoarefresh || idnssoaretry || idnssoaexpire || idnssoaminimum || idnsupdatepolicy")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "permission:update dns entries";allow (write) groupdn = "ldap:///cn=update dns entries,cn=permissions,cn=pbac,$SUFFIX";)'
diff --git a/install/updates/40-dns.update b/install/updates/40-dns.update
new file mode 100644
index 000000000..7b1c45754
--- /dev/null
+++ b/install/updates/40-dns.update
@@ -0,0 +1,22 @@
+# Add missing member values to attach permissions to their respective
+# privileges and run a memberOf task.
+dn: cn=add dns entries,cn=permissions,cn=pbac,$SUFFIX
+addifexist:member: 'cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX'
+addifexist:member: 'cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX'
+
+dn: cn=remove dns entries,cn=permissions,cn=pbac,$SUFFIX
+addifexist:member: 'cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX'
+addifexist:member: 'cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX'
+
+dn: cn=update dns entries,cn=permissions,cn=pbac,$SUFFIX
+addifexist:member: 'cn=DNS Administrators,cn=privileges,cn=pbac,$SUFFIX'
+addifexist:member: 'cn=DNS Servers,cn=privileges,cn=pbac,$SUFFIX'
+
+dn: cn=Update PBAC memberOf $TIME, cn=memberof task, cn=tasks, cn=config
+add: objectClass: top
+add: objectClass: extensibleObject
+add: cn: IPA PBAC memberOf $TIME
+add: basedn: 'cn=privileges,cn=pbac,$SUFFIX'
+add: filter: (objectclass=*)
+add: ttl: 10
+
diff --git a/install/updates/Makefile.am b/install/updates/Makefile.am
index bf4d9af96..99b7c56c7 100644
--- a/install/updates/Makefile.am
+++ b/install/updates/Makefile.am
@@ -19,6 +19,7 @@ app_DATA =				\
 	20-winsync_index.update		\
 	21-replicas_container.update	\
 	40-delegation.update		\
+	40-dns.update			\
 	45-roles.update			\
 	50-lockout-policy.update	\
 	50-groupuuid.update		\
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index f2f416b9c..e1f6b1f43 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -267,7 +267,7 @@ class LDAPUpdate:
     def parse_update_file(self, data, all_updates, dn_list):
         """Parse the update file into a dictonary of lists and apply the update
            for each DN in the file."""
-        valid_keywords = ["default", "add", "remove", "only", "deleteentry", "replace", "addifnew"]
+        valid_keywords = ["default", "add", "remove", "only", "deleteentry", "replace", "addifnew", "addifexist"]
         update = {}
         d = ""
         index = ""
@@ -533,6 +533,14 @@ class LDAPUpdate:
                         e.append(v)
                         logging.debug('addifnew: set %s to %s', k, e)
                         entry.setValues(k, e)
+                elif utype == 'addifexist':
+                    logging.debug("addifexist: '%s' to %s, current value %s", v, k, e)
+                    # Only add the attribute if the entry doesn't exist. We
+                    # determine this based on whether it has an objectclass
+                    if entry.getValues('objectclass'):
+                        e.append(v)
+                        logging.debug('addifexist: set %s to %s', k, e)
+                        entry.setValues(k, e)
                 elif utype == 'only':
                     logging.debug("only: set %s to '%s', current value %s", k, v, e)
                     if only.get(k):
@@ -645,7 +653,11 @@ class LDAPUpdate:
             # entry.orig_data = {}
             try:
                 if self.live_run:
-                    self.conn.addEntry(entry.dn, entry.toTupleList())
+                    if len(entry.toTupleList()) > 0:
+                        # addifexist may result in an entry with only a
+                        # dn defined. In that case there is nothing to do.
+                        # It means the entry doesn't exist, so skip it.
+                        self.conn.addEntry(entry.dn, entry.toTupleList())
                 self.modified = True
             except Exception, e:
                 logging.error("Add failure %s", e)