[Kimchi-devel] [PATCH] issue #545: Handle simultaneous authentication methods when updating VM permission

[Aline Manera]

On 24/02/2015 18:03, Crístian Viana wrote:
> If a VM has one authentication method in its metadata (e.g. 'pam') and
> a user sets some permissions with a different authentication method
> (e.g. 'ldap'), then the group of the first authentication method is
> lost and only the last type is kept. In other words, it is not possible
> to have more than one authenticatin type in a VM's metadata.
>
> Add the ability to handle different authentication methods
> simultaneously when updating VM permission. Whatever is already set will
> always be kept, even if the user chooses a different method.
>
> Fix issue #545 (VM permission data is lost when changing authentication
> method).
>
> Signed-off-by: Crístian Viana <vianac at linux.vnet.ibm.com>
> ---
>   src/kimchi/model/vms.py | 54 ++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py
> index 379c850..018df9e 100644
> --- a/src/kimchi/model/vms.py
> +++ b/src/kimchi/model/vms.py
> @@ -552,18 +552,48 @@ class VMModel(object):
>                   # remove the new object store entry should an error occur later
>                   rollback.prependDefer(_rollback_objstore)
>   
> -    def _build_access_elem(self, users, groups):
> +    def _build_access_elem(self, dom, users, groups):
>           auth = config.get("authentication", "method")
> -        auth_elem = E.auth(type=auth)
> -        for user in users:
> -            auth_elem.append(E.user(user))
> +        access_xml = get_metadata_node(dom, "access",
> +                                       self.caps.metadata_support)
>   
> -        for group in groups:
> -            auth_elem.append(E.group(group))
> +        auth_elem = None
>   
> -        access = E.access()
> -        access.append(auth_elem)
> -        return access
> +        if not access_xml:
> +            # there is no metadata element 'access'
> +            access_elem = E.access()
> +        else:

> +            access_elem = ET.fromstring(access_xml)
> +
> +            same_auth = access_elem.xpath('./auth[@type="%s"]' % auth)
> +            if len(same_auth) > 0:
> +                # there is already a sub-element 'auth' with the same type;
> +                # update it.
> +                auth_elem = same_auth[0]
> +
> +                if users is not None:
> +                    for u in auth_elem.findall('user'):
> +                        auth_elem.remove(u)
> +
> +                if groups is not None:
> +                    for g in auth_elem.findall('group'):
> +                        auth_elem.remove(g)

I understood this block as a clean up - to remove old data and then 
insert the new one. Is that correct?
If so, I suggest to remove the whole "auth" element and create a new one 
below, as you already do.

> +
> +        if auth_elem is None:
> +            # there is no sub-element 'auth' with the same type
> +            # (or no 'auth' at all); create it.
> +            auth_elem = E.auth(type=auth)
> +            access_elem.append(auth_elem)
> +
> +        if users is not None:
> +            for u in users:
> +                auth_elem.append(E.user(u))
> +
> +        if groups is not None:
> +            for g in groups:
> +                auth_elem.append(E.group(g))
> +
> +        return access_elem
>   
>       def _vm_update_access_metadata(self, dom, params):
>           users = groups = None
> @@ -583,11 +613,7 @@ class VMModel(object):
>           if users is None and groups is None:
>               return
>   
> -        old_users, old_groups = self._get_access_info(dom)
> -        users = old_users if users is None else users
> -        groups = old_groups if groups is None else groups
> -
> -        node = self._build_access_elem(users, groups)
> +        node = self._build_access_elem(dom, users, groups)
>           set_metadata_node(dom, node, self.caps.metadata_support)
>   
>       def _get_access_info(self, dom):