Make framework consider krbcanonicalname as service primary key

The framework does not allow single param to appear as both positional
argument and option in a single command, or to represent two different
positional arguments for that matter. Since principal aliases shall go to
krbprincipalname attribute, the framework has to be tricked to believe
krbcanonicalname is the service's primary key. The entry DN stored in LDAP
remains the same.

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

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

3 files changed, 97 insertions, 20 deletions

diff --git a/API.txt b/API.txt
index 0c307fbbd..5d99f5a0b 100644
--- a/API.txt
+++ b/API.txt
@@ -4271,7 +4271,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_add/1
 args: 1,12,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Str('addattr*', cli_name='addattr')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('force', autofill=True, default=False)
@@ -4289,7 +4289,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_add_cert/1
 args: 1,5,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
@@ -4300,7 +4300,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_add_host/1
 args: 1,5,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('host*', alwaysask=True, cli_name='hosts')
 option: Flag('no_members', autofill=True, default=False)
@@ -4311,7 +4311,7 @@ output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_allow_create_keytab/1
 args: 1,8,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('group*', alwaysask=True, cli_name='groups')
 option: Str('host*', alwaysask=True, cli_name='hosts')
@@ -4325,7 +4325,7 @@ output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_allow_retrieve_keytab/1
 args: 1,8,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('group*', alwaysask=True, cli_name='groups')
 option: Str('host*', alwaysask=True, cli_name='hosts')
@@ -4339,7 +4339,7 @@ output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_del/1
 args: 1,2,3
-arg: Principal('krbprincipalname+', cli_name='principal')
+arg: Principal('krbcanonicalname+', cli_name='canonical_principal')
 option: Flag('continue', autofill=True, cli_name='continue', default=False)
 option: Str('version?')
 output: Output('result', type=[<type 'dict'>])
@@ -4347,14 +4347,14 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: ListOfPrimaryKeys('value')
 command: service_disable/1
 args: 1,1,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Str('version?')
 output: Output('result', type=[<type 'bool'>])
 output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_disallow_create_keytab/1
 args: 1,8,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('group*', alwaysask=True, cli_name='groups')
 option: Str('host*', alwaysask=True, cli_name='hosts')
@@ -4368,7 +4368,7 @@ output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_disallow_retrieve_keytab/1
 args: 1,8,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('group*', alwaysask=True, cli_name='groups')
 option: Str('host*', alwaysask=True, cli_name='hosts')
@@ -4381,10 +4381,11 @@ output: Output('completed', type=[<type 'int'>])
 output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_find/1
-args: 1,12,4
+args: 1,13,4
 arg: Str('criteria?')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: StrEnum('ipakrbauthzdata*', autofill=False, cli_name='pac_type', values=[u'MS-PAC', u'PAD', u'NONE'])
+option: Principal('krbcanonicalname?', autofill=False, cli_name='canonical_principal')
 option: Str('krbprincipalauthind*', autofill=False, cli_name='auth_ind')
 option: Principal('krbprincipalname?', autofill=False, cli_name='principal')
 option: Str('man_by_host*', cli_name='man_by_hosts')
@@ -4401,7 +4402,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: Output('truncated', type=[<type 'bool'>])
 command: service_mod/1
 args: 1,13,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Str('addattr*', cli_name='addattr')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('delattr*', cli_name='delattr')
@@ -4420,7 +4421,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_remove_cert/1
 args: 1,5,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
@@ -4431,7 +4432,7 @@ output: Output('summary', type=[<type 'unicode'>, <type 'NoneType'>])
 output: PrimaryKey('value')
 command: service_remove_host/1
 args: 1,5,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('host*', alwaysask=True, cli_name='hosts')
 option: Flag('no_members', autofill=True, default=False)
@@ -4442,7 +4443,7 @@ output: Output('failed', type=[<type 'dict'>])
 output: Entry('result')
 command: service_show/1
 args: 1,6,3
-arg: Principal('krbprincipalname', cli_name='principal')
+arg: Principal('krbcanonicalname', cli_name='canonical_principal')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Flag('no_members', autofill=True, default=False)
 option: Str('out?')
diff --git a/VERSION b/VERSION
index a35aec56c..dd2e1aba1 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=206
-# Last change: mbabinsk: commands that use positional parameters to manage attributes
+IPA_API_VERSION_MINOR=207
+# Last change: mbabinsk: Make framework consider krbcanonicalname as service primary key
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index ec0bc5fbe..7b0832b23 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -436,9 +436,9 @@ class service(LDAPObject):
 
     takes_params = (
         Principal(
-            'krbprincipalname',
+            'krbcanonicalname',
             validate_realm,
-            cli_name='principal',
+            cli_name='canonical_principal',
             label=_('Principal'),
             doc=_('Service principal'),
             primary_key=True,
@@ -503,6 +503,16 @@ class service(LDAPObject):
                   " Use 'radius' to allow RADIUS-based 2FA authentications."
                   " Other values may be used for custom configurations."),
         ),
+        Principal(
+            'krbprincipalname',
+            validate_realm,
+            cli_name='principal',
+            label=_('Principal Alias'),
+            doc=_('Service principal alias'),
+            normalizer=normalize_principal,
+            require_service=True,
+            flags={'no_create', 'no_update'}
+        ),
     ) + ticket_flags_params
 
     def validate_ipakrbauthzdata(self, entry):
@@ -521,8 +531,51 @@ class service(LDAPObject):
                 error=_('NONE value cannot be combined with other PAC types'))
 
     def get_dn(self, *keys, **kwargs):
-        keys = (normalize_principal(k) for k in keys)
-        return super(service, self).get_dn(*keys, **kwargs)
+        key = keys[0]
+        if isinstance(key, six.text_type):
+            key = kerberos.Principal(key)
+
+        key = unicode(normalize_principal(key))
+
+        parent_dn = DN(self.container_dn, self.api.env.basedn)
+        true_rdn = 'krbprincipalname'
+
+        return self.backend.make_dn_from_attr(
+            true_rdn, key, parent_dn
+        )
+
+    def get_primary_key_from_dn(self, dn):
+        """
+        If the entry has krbcanonicalname set return the value of the
+        attribute. If the attribute is not found, assume old-style entry which
+        should have only single value of krbprincipalname and return it.
+
+        Otherwise return input DN.
+        """
+        assert isinstance(dn, DN)
+
+        try:
+            entry_attrs = self.backend.get_entry(
+                dn, [self.primary_key.name]
+            )
+            try:
+                return entry_attrs[self.primary_key.name][0]
+            except (KeyError, IndexError):
+                return ''
+        except errors.NotFound:
+            pass
+
+        try:
+            return dn['krbprincipalname'][0]
+        except KeyError:
+            return unicode(dn)
+
+    def populate_krbcanonicalname(self, entry_attrs, options):
+        if options.get('raw', False):
+            return
+
+        entry_attrs.setdefault(
+            'krbcanonicalname', entry_attrs['krbprincipalname'])
 
 
 @register()
@@ -587,6 +640,7 @@ class service_add(LDAPCreate):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         set_kerberos_attrs(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return dn
 
 
@@ -655,6 +709,7 @@ class service_mod(LDAPUpdate):
         set_certificate_attrs(entry_attrs)
         set_kerberos_attrs(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return dn
 
 
@@ -667,6 +722,8 @@ class service_find(LDAPSearch):
         '%(count)d service matched', '%(count)d services matched', 0
     )
     member_attributes = ['managedby']
+    sort_result_entries = False
+
     takes_options = LDAPSearch.takes_options
     has_output_params = LDAPSearch.has_output_params + output_params
 
@@ -680,12 +737,25 @@ class service_find(LDAPSearch):
                               '(krbprincipalname=krbtgt/*))' \
                           ')' \
                         ')'
+        if options.get('pkey_only', False):
+            attrs_list.append('krbprincipalname')
+
         return (
             ldap.combine_filters((custom_filter, filter), rules=ldap.MATCH_ALL),
             base_dn, scope
         )
 
     def post_callback(self, ldap, entries, truncated, *args, **options):
+        # we have to sort entries manually instead of relying on inherited
+        # mechanisms
+        def sort_key(x):
+            if 'krbcanonicalname' in x:
+                return x['krbcanonicalname'][0]
+            else:
+                return x['krbprincipalname'][0]
+
+        entries.sort(key=sort_key)
+
         if options.get('pkey_only', False):
             return truncated
         for entry_attrs in entries:
@@ -707,6 +777,7 @@ class service_find(LDAPSearch):
 
             set_kerberos_attrs(entry_attrs, options)
             rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
+            self.obj.populate_krbcanonicalname(entry_attrs, options)
         return truncated
 
 
@@ -744,6 +815,7 @@ class service_show(LDAPRetrieve):
 
         set_kerberos_attrs(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
 
         return dn
 
@@ -781,6 +853,7 @@ class service_allow_retrieve_keytab(LDAPAddMember):
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(failed, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return (completed, dn)
 
 
@@ -799,6 +872,7 @@ class service_disallow_retrieve_keytab(LDAPRemoveMember):
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(failed, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return (completed, dn)
 
 
@@ -818,6 +892,7 @@ class service_allow_create_keytab(LDAPAddMember):
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(failed, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return (completed, dn)
 
 
@@ -836,6 +911,7 @@ class service_disallow_create_keytab(LDAPRemoveMember):
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
         rename_ipaallowedtoperform_from_ldap(failed, options)
+        self.obj.populate_krbcanonicalname(entry_attrs, options)
         return (completed, dn)