Re: [Kimchi-devel] [PATCH V5] add a method to fix search permissions

On 01/07/2014 03:15 AM, Aline Manera wrote: > On 01/06/2014 01:55 PM, shaohef(a)linux.vnet.ibm.com wrote: >> From: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; >> >> First will use setfacl to set the path search permission for user or >> group, if failed then set search permission for others on the path. >> >> Usage: >> check_path_permission("/tmp/need/to/fix/path", username) >> fix_path_permission("/tmp/need/to/fix/path", username) >> >> Signed-off-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; >> Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; >> --- >> Makefile.am | 1 + >> src/kimchi/utils.py | 125 >> +++++++++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 125 insertions(+), 1 deletion(-) >> >> diff --git a/Makefile.am b/Makefile.am >> index 07b2aa0..bf8e0a0 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -46,6 +46,7 @@ PEP8_WHITELIST = \ >> src/kimchi/cachebust.py \ >> src/kimchi/config.py.in \ >> src/kimchi/control/*.py \ >> + src/kimchi/utils.py \ >> src/kimchi/disks.py \ >> src/kimchi/featuretests.py \ >> src/kimchi/iscsi.py \ >> diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py >> index b6d84fd..5de982f 100644 >> --- a/src/kimchi/utils.py >> +++ b/src/kimchi/utils.py >> @@ -18,11 +18,16 @@ >> # >> # You should have received a copy of the GNU Lesser General Public >> # License along with this library; if not, write to the Free Software >> -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> # >> >> import cherrypy >> +import grp >> import os >> +import pwd >> +import re >> +import stat >> +import subprocess >> >> >> from cherrypy.lib.reprconf import Parser >> @@ -33,6 +38,7 @@ from kimchi import config >> >> kimchi_log = cherrypy.log.error_log >> >> + >> def is_digit(value): >> if isinstance(value, int): >> return True >> @@ -84,3 +90,120 @@ def import_class(class_path): >> >> def import_module(module_name): >> return __import__(module_name, globals(), locals(), ['']) >> + >> + >> +def run_command(cmd): >> + try: >> + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, >> + stderr=subprocess.PIPE) >> + out, error = proc.communicate() >> + >> + kimchi_log.debug("Run command '%s'", " ".join(cmd)) >> + if out or error: >> + kimchi_log.debug("out:\n %s\nerror:\n %s", out, error) >> + except OSError as e: >> + kimchi_log.error("Failed to run command %s. %s", " ".join(cmd), >> + e.message) >> + return (None, None, None) >> + if proc.returncode != 0: >> + kimchi_log.debug("Cmd '%s' failed, error code: %s", cmd, error) >> + return (out, error, proc.returncode) >> + >> + >> +def name_uid(user): >> + return pwd.getpwnam(user).pw_uid >> + >> + >> +def get_group_ids(user): >> + gids = [g.gr_gid for g in grp.getgrall() if user in g.gr_mem] >> + gid = pwd.getpwnam(user).pw_gid >> + gids.append(grp.getgrgid(gid).gr_gid) >> + return gids >> + >> + >> +def get_groups(user): >> + gids = get_group_ids(user) >> + return [grp.getgrgid(gid).gr_name for gid in gids] >> + >> + >> +def run_setfacl_set_x(path, user="qemu", group=None): > > I think the correct default value for 'groups' is "" instead of None > Or we will run: setfacl --modify group:None:x <path> sure, "" is good. will change it in next version,. I have check in the follow code: + if ret != 0 and group: + out, error, ret = run_command(set_group) In my option, if we can set user ACL successful, do we still need to set group ACL?

> >> + set_user = ["setfacl", "--modify", "user:%s:x" % user, path] >> + set_group = ["setfacl", "--modify", "group:%s:x" % group, path] >> + out, error, ret = run_command(set_user) >> + if ret != 0 and group: >> + out, error, ret = run_command(set_group) >> + return ret == 0 >> + >> + >> +def run_getfacl(path, user="qemu", groups=None): > > Same I commented above about the default value for 'groups' parameter > >> + cmd = ["getfacl", path] >> + out, error, ret = run_command(cmd) >> + if out and ret == 0: >> + res = re.findall("user:%s:..x" % user, out) >> + if res: >> + return True >> + res = re.findall("(?<=group:)(.*)(?=:..x)", >> + out) if groups else [] >> + return list(set(res) & set(groups)) != [] >> + return False >> + >> + >> +def set_x_on_path(path, who="other"): >> + S_IXWHO = ((stat.S_IXUSR if who == "user" else None) or >> + (stat.S_IXGRP if who == "group" else None) or >> + stat.S_IXOTH) >> + mode = os.stat(path).st_mode | S_IXWHO >> + os.chmod(path, mode) >> + ret = os.stat(path).st_mode == mode >> + if not ret: >> + kimchi_log.debug("faild to set +x attribute on %s", path) >> + return ret >> + >> + >> +def get_x_on_path(path, uid, gids=[]): >> + try: >> + st = os.stat(path) >> + mode = st.st_mode >> + return ((uid == st.st_uid and mode & stat.S_IXUSR == stat.S_IXUSR) or >> + (st.st_gid in gids and mode & stat.S_IXGRP == stat.S_IXGRP) or >> + mode & stat.S_IXOTH == stat.S_IXOTH) >> + except OSError as e: >> + kimchi_log.error("faild to get attribute on %s as user id: %s. %s", >> + path, uid, e.message) >> + return False >> + >> + > >> +def reverse_paths(path): >> + paths = [] >> + while path: >> + paths.append(path) >> + if path == "/": >> + break >> + path = os.path.dirname(path) >> + >> + paths.reverse() >> + return paths >> + > > As I commented before we can simplify it by using just one loop: > > paths = path.split("/") > paths = paths[1:] > > path = "/" > for p in paths: > path = os.path.join(path, p) > # check_path_permission code > will change in the next version. >> + >> +def check_path_permission(path, user='qemu', groups=[]): >> + groups = groups if groups else get_groups(user) >> + gids = [grp.getgrnam(group).gr_gid for group in groups] if groups >> else [] >> + paths = reverse_paths(path) >> + for path in paths: >> + if not (get_x_on_path(path, name_uid(user), gids) or >> + run_getfacl(path, user, groups)): >> + return False >> + return True >> + >> + > > We don't need this check_path_permission function > We can run fix_path_permission directly and it solves the problem > when needed. > >> +def fix_path_permission(path, user='qemu', groups=[]): >> + groups = groups if groups else get_groups(user) >> + gids = [grp.getgrnam(group).gr_gid for group in groups] if groups >> else [] >> + paths = reverse_paths(path) >> + for path in paths: >> + if not (get_x_on_path(path, name_uid(user), gids) or >> + run_getfacl(path, user, groups) or >> + run_setfacl_set_x(path, user) or >> + set_x_on_path(path)): >> + return False >> + return True > > >