On 03/23/2016 02:24 PM, Archana Singh wrote: > > > On 3/23/2016 12:19 PM, Abhiram wrote: >> On Wed, 2016-03-23 at 11:39 -0500, Archana Singh wrote: >>> Hi Suresh, >>> >>> As all of these methods are not private methods, I will suggest to >>> have >>> check in all the methods. >> Eventhough the methods arent private, the initial caller has to be >> create,getlist,delete,lookup atleast as suresh mentioned from ui point. >> I feel it has to be one place, rather than having in all sub methods >> which is redundant in nature. >> >> If we want to have all places, do we want to have the unicode checking >> and conversion into one common method and reuse the same everywhere? If >> at all we have any change in future probably only one method we can >> modify. > > I do understand the redundant point of view, but as these are not > private methods can be called from any place. If it's a widespread issue then can we look at WoK engine code to fix it for all cases and plug-ins? I don't know if this is actually viable but at first it makes sense to me. Would need some investigation. Daniel

> As we do not have type check for the parameter in the method, it is > safe to ensure that we do not end up in some encoding issue. > >>> Thanks, >>> Archana Singh >>> >>> On 3/23/2016 2:05 AM, Suresh Babu Angadi wrote: >>>> Hi Archana, >>>> Instead of checking unicode in every method, can we have it only in >>>> UsersModel and UserModel - create, getlist, delete, lookup? >>>> >>>> On 03/22/2016 03:52 AM, archus(a)linux.vnet.ibm.com wrote: >>>>> From: Archana Singh <archus(a)linux.vnet.ibm.com&gt; >>>>> >>>>> --- >>>>> i18n.py | 3 ++- >>>>> model/users.py | 69 >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++---------- >>>>> 2 files changed, 59 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/i18n.py b/i18n.py >>>>> index 78260a0..6729bf3 100644 >>>>> --- a/i18n.py >>>>> +++ b/i18n.py >>>>> @@ -124,10 +124,11 @@ messages = { >>>>> "GINUSER0006E": _("Could not add user '%(user)s' to kvm >>>>> group."), >>>>> "GINUSER0007E": _("Could not add user '%(user)s' to sudoers >>>>> list."), >>>>> "GINUSER0008E": _("The user name '%(user)s' is already in >>>>> use'."), >>>>> - "GINUSER0009E": _("Could not create user '%(user)s'."), >>>>> + "GINUSER0009E": _("Could not create user '%(user)s', error: >>>>> %(err)s'."), >>>>> "GINUSER0010E": _("Could not delete user '%(user)s'."), >>>>> "GINUSER0011E": _("User '%(user)s' does not exist."), >>>>> "GINUSER0012E": _("Could not delete group '%(group)s'"), >>>>> + "GINUSER0013E": _("Could not create group name'%(group)s', >>>>> error: %(err)s'."), >>>>> >>>>> "GINFW0001E": _("Cannot update system firmware while running >>>>> VMs."), >>>>> "GINFW0002E": _("Firmware image unpack failed: rc = %(rc)s. " >>>>> diff --git a/model/users.py b/model/users.py >>>>> index a05bd5f..f8f450d 100644 >>>>> --- a/model/users.py >>>>> +++ b/model/users.py >>>>> @@ -1,3 +1,4 @@ >>>>> +# -*- coding: utf-8 -*- >>>>> # >>>>> # Project Ginger >>>>> # >>>>> @@ -51,6 +52,8 @@ def get_group_obj_by_gid(gid): >>>>> >>>>> def get_group_gid(groupname): >>>>> adm = libuser.admin() >>>>> + if isinstance(groupname, unicode): >>>>> + groupname = groupname.encode('utf-8') >>>>> group = adm.lookupGroupByName(groupname) >>>>> if group is None: >>>>> return None >>>>> @@ -58,19 +61,30 @@ def get_group_gid(groupname): >>>>> >>>>> >>>>> def create_group(groupname, gid): >>>>> - adm = libuser.admin() >>>>> - group = adm.lookupGroupByName(groupname) >>>>> - if not group: >>>>> - new_group = adm.initGroup(groupname) >>>>> - new_group.set(libuser.GIDNUMBER, gid) >>>>> - adm.addGroup(new_group) >>>>> - return gid >>>>> - else: >>>>> - return group.get('pw_gid')[0] >>>>> + try: >>>>> + adm = libuser.admin() >>>>> + if isinstance(groupname, unicode): >>>>> + groupname = groupname.encode('utf-8') >>>>> + if isinstance(gid, unicode): >>>>> + gid = gid.encode('utf-8') >>>>> + group = adm.lookupGroupByName(groupname) >>>>> + if not group: >>>>> + new_group = adm.initGroup(groupname) >>>>> + new_group.set(libuser.GIDNUMBER, gid) >>>>> + adm.addGroup(new_group) >>>>> + return gid >>>>> + else: >>>>> + return group.get('pw_gid')[0] >>>>> + except Exception as e: >>>>> + wok_log.error('Could not create group name %s, error: %s', >>>>> + groupname, e) >>>>> + raise OperationFailed('GINUSER0013E', {'group': groupname, >>>>> 'err': e}) >>>>> >>>>> >>>>> def delete_group(groupname): >>>>> adm = libuser.admin() >>>>> + if isinstance(groupname, unicode): >>>>> + groupname = groupname.encode('utf-8') >>>>> group_obj = adm.lookupGroupById( >>>>> int(get_group_gid(groupname)) >>>>> ) >>>>> @@ -89,6 +103,8 @@ def delete_group(groupname): >>>>> >>>>> def get_users_from_group(groupname): >>>>> adm = libuser.admin() >>>>> + if isinstance(groupname, unicode): >>>>> + groupname = groupname.encode('utf-8') >>>>> group_obj = adm.lookupGroupById( >>>>> int(get_group_gid(groupname)) >>>>> ) >>>>> @@ -108,6 +124,8 @@ def get_users(exclude_system_users=True): >>>>> >>>>> def get_user_obj(username): >>>>> adm = libuser.admin() >>>>> + if isinstance(username, unicode): >>>>> + username = username.encode('utf-8') >>>>> return adm.lookupUserByName(username) >>>>> >>>>> >>>>> @@ -120,6 +138,10 @@ def gen_salt(): >>>>> >>>>> def create_user(name, plain_passwd, profile=None, gid=None): >>>>> adm = libuser.admin() >>>>> + if isinstance(name, unicode): >>>>> + name = name.encode('utf-8') >>>>> + if isinstance(gid, unicode): >>>>> + gid = gid.encode('utf-8') >>>>> user = adm.lookupUserByName(name) >>>>> if user: >>>>> msg = 'User/Login "%s" already in use' % name >>>>> @@ -150,15 +172,23 @@ def create_user(name, plain_passwd, >>>>> profile=None, gid=None): >>>>> enc_pwd = crypt.crypt(plain_passwd, salt) >>>>> >>>>> adm.setpassUser(new_user, enc_pwd, True) >>>>> + except UnicodeEncodeError as ue: >>>>> + err_msg = ue.message if ue.message else 'Username / >>>>> password \ >>>>> + has NON - ASCII charater' >>>>> + wok_log.error('Could not create user %s, error: %s', name, >>>>> err_msg) >>>>> + raise OperationFailed('GINUSER0009E', {'user': name, 'err': >>>>> err_msg}) >>>>> except Exception as e: >>>>> - wok_log.error('Could not create user %s, error: %s', >>>>> name, e) >>>>> - raise OperationFailed('GINUSER0009E', {'user': name}) >>>>> + err_msg = e.message if e.message else e >>>>> + wok_log.error('Could not create user %s, error: %s', name, >>>>> err_msg) >>>>> + raise OperationFailed('GINUSER0009E', {'user': name, 'err': >>>>> err_msg}) >>>>> >>>>> return new_user >>>>> >>>>> >>>>> def delete_user(username): >>>>> adm = libuser.admin() >>>>> + if isinstance(username, unicode): >>>>> + username = username.encode('utf-8') >>>>> user_obj = adm.lookupUserByName(username) >>>>> >>>>> if user_obj is None: >>>>> @@ -182,6 +212,11 @@ class UsersModel(object): >>>>> profile = params['profile'] >>>>> groupname = params['group'] >>>>> >>>>> + if isinstance(username, unicode): >>>>> + username = username.encode('utf-8') >>>>> + if isinstance(groupname, unicode): >>>>> + groupname = groupname.encode('utf-8') >>>>> + >>>>> gid = get_group_gid(groupname) >>>>> if gid is None: >>>>> user = create_user(username, passwd, profile=profile) >>>>> @@ -213,6 +248,8 @@ class UsersModel(object): >>>>> # Add new user to KVM group >>>>> adm = libuser.admin() >>>>> kvmgrp = get_group_obj('kvm') >>>>> + if isinstance(user, unicode): >>>>> + user = user.encode('utf-8') >>>>> kvmgrp.add('gr_mem', user) >>>>> ret = adm.modifyGroup(kvmgrp) >>>>> if ret != 1: >>>>> @@ -227,6 +264,7 @@ class UsersModel(object): >>>>> >>>>> >>>>> class UserModel(object): >>>>> + >>>>> def delete(self, user): >>>>> user_obj = get_user_obj(user) >>>>> group_obj = get_group_obj_by_gid( >>>>> @@ -252,7 +290,10 @@ class UserModel(object): >>>>> group_name = grp.getgrgid(user_info.pw_gid).gr_name >>>>> >>>>> except KeyError: >>>>> - group_name = str(user_info.pw_gid) >>>>> + pw_gid = user_info.pw_gid >>>>> + if isinstance(pw_gid, unicode): >>>>> + pw_gid = pw_gid.encode('utf-8') >>>>> + group_name = str(pw_gid) >>>>> >>>>> return {"name": user, >>>>> "uid": user_info.pw_uid, >>>>> @@ -263,6 +304,8 @@ class UserModel(object): >>>>> def _get_user_profile(self, user): >>>>> # ADMIN: Check /etc/sudoers.d >>>>> adm = libuser.admin() >>>>> + if isinstance(user, unicode): >>>>> + user = user.encode('utf-8') >>>>> if os.path.isfile(SUDOERS_FILE % user): >>>>> return 'admin' >>>>> else: >>>>> @@ -278,6 +321,8 @@ class UserModel(object): >>>>> return 'kimchiuser' >>>>> >>>>> def _delete_profile_settings(self, user): >>>>> + if isinstance(user, unicode): >>>>> + user = user.encode('utf-8') >>>>> profile = self._get_user_profile(user) >>>>> if profile == 'kimchiuser': >>>>> return >> >