[PATCH] issue #545: Handle simultaneous authentication methods when updating VM permission
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@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) + + 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): -- 2.1.0
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@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):
On 27-02-2015 13:34, Aline Manera wrote:
+ 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?
Yes.
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)
The cleanup code doesn't always remove all users and all groups from "auth_elem"; it only removes the users if the request is setting new users, and it only removes the groups if the request is settings new groups. So there's an optimization in that code for when only one of "users"/"groups" is set. Consider this example. Suppose the VM "foovm" has this XML data: [...] <auth type='pam'> <user>u1</user> <user>u2</user> <group>g1</group <group>g2</group> </auth> [...] and the following command is sent to the server: PUT /vms/foovm {'users': ['u10']} That command shouldn't change the current <group>s, only the <user>s (because it didn't sent the params "groups"), so the server must keep that VM's current <group>s settings. According to this patch, only <user>s will be removed so the new values can be added later; <group>s should remain. If the patch removes the "auth_elem" completely, as you suggested, it will still need the old values from the XML data to set the same groups again, and that will require more data/processing. In summary, this is the algorithm of this patch for the case described above: 1) Read the XML from libvirt. 2) Remove the user data from the XML. 3) Add the new users to the XML, from params. 4) Send the new XML to libvirt. And this is the suggested algorithm: 1) Read the XML from libvirt. 2) Backup the group data. 3) Discard the XML. 4) Create an XML. 5) Add the new users to the XML, from params. 6) Add the old groups (from step (2)) to the XML. 7) Send the new XML to libvirt. Of course, when both "users" and "groups" are set in the same command, these algorithms become equivalent. But it's always better to save some time and memory when it's possible.
On 02/03/2015 16:19, Crístian Viana wrote:
On 27-02-2015 13:34, Aline Manera wrote:
+ 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?
Yes.
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)
The cleanup code doesn't always remove all users and all groups from "auth_elem"; it only removes the users if the request is setting new users, and it only removes the groups if the request is settings new groups. So there's an optimization in that code for when only one of "users"/"groups" is set.
Consider this example. Suppose the VM "foovm" has this XML data:
[...] <auth type='pam'> <user>u1</user> <user>u2</user> <group>g1</group <group>g2</group> </auth> [...]
and the following command is sent to the server:
PUT /vms/foovm {'users': ['u10']}
That command shouldn't change the current <group>s, only the <user>s (because it didn't sent the params "groups"), so the server must keep that VM's current <group>s settings. According to this patch, only <user>s will be removed so the new values can be added later; <group>s should remain. If the patch removes the "auth_elem" completely, as you suggested, it will still need the old values from the XML data to set the same groups again, and that will require more data/processing.
In summary, this is the algorithm of this patch for the case described above:
1) Read the XML from libvirt. 2) Remove the user data from the XML. 3) Add the new users to the XML, from params. 4) Send the new XML to libvirt.
And this is the suggested algorithm:
1) Read the XML from libvirt. 2) Backup the group data. 3) Discard the XML. 4) Create an XML. 5) Add the new users to the XML, from params. 6) Add the old groups (from step (2)) to the XML. 7) Send the new XML to libvirt.
Of course, when both "users" and "groups" are set in the same command, these algorithms become equivalent. But it's always better to save some time and memory when it's possible.
OK.
Applied. Thanks. Regards, Aline Manera
participants (2)
-
Aline Manera -
Crístian Viana