Project

General

Profile

Actions

Bug #423

closed

777 permissions

Added by Gabriel Lander over 14 years ago. Updated over 14 years ago.

Status:
Closed
Priority:
Normal
Category:
-
Target version:
-
Start date:
05/04/2010
Due date:
% Done:

100%

Estimated time:
Affected Version:
Show in known bugs:
Workaround:

Description

Our sys admin is very uncomfortable with the default "0777" permissions set on all appion folders - is there a reason they can't be "2775"?


Related issues 1 (0 open1 closed)

Related to Appion - Bug #1487: AMI group members cannot process other AMI members dataWon't Fix or Won't Do Sargis Dallakyan11/30/2011

Actions
Actions #1

Updated by Eric Hou over 14 years ago

Why do you have permission issue at the beginning?

Actions #2

Updated by Gabriel Lander over 14 years ago

when appionlib/apParam.py creates a directory, it is set to 0777

Actions #3

Updated by Christopher Irving over 14 years ago

This bug surprised me because I knew that wasn't the default for us and I though Gabe must have made a mistake. However, when I looked at the source I found this horrible bit of code in the file apParam.py (source:/trunk/appion/appionlib/apParam.py@13489#L485)

def setUmask(msg=False):
        os.umask(0)
        os.umask(0)
        return
        if os.getgid() == 773:
                prev = os.umask(002)
                curr = os.umask(002)
        else:
                prev = os.umask(000)
                curr = os.umask(000)
        if msg is True:
                apDisplay.printMsg("Umask changed from "+str(prev)+" to "+str(curr))

So basically we're setting the umask to 002 if the user is in the Unix group 773, which is AMI here but could be anything somewhere else. That needs to be removed. Also, the umask should only be set to 000 if you want to ensure the file permission mode passed in a open() or other file/directory creation call isn't overridden by the users umask setting. However, you have to set that file permission mode when creating the file otherwise a file will have mode 666 and a directory will have mode 777 which is a very bad idea.

Actions #4

Updated by Amber Herold over 14 years ago

  • Assignee changed from Amber Herold to Christopher Irving
Actions #5

Updated by Christopher Irving over 14 years ago

  • Status changed from New to In Test
  • % Done changed from 0 to 100

Applied in changeset r13702.

Actions #6

Updated by Gabriel Lander over 14 years ago

can we also change line 299 in apParam.py:
def createDirectory(path, mode=0777, warning=True):
to mode=0775?

Actions #7

Updated by Neil Voss over 14 years ago

  • Status changed from In Test to In Code Review

Hey guys. I am sorry I added this code. I get so fed up with trying to help people process data and then I have to go the extra steps to give myself access. It was an error on my part.

Nonetheless, Christopher's fix does nothing! The umask function in python does not work as expected. You can set it all you want and it does nothing. This code has been present since 2007, but did not work so no effect was seen.

The code that is making the effect happen is here in appionScript.py (source:/trunk/appion/appionlib/appionScript.py@13705#L261):

### make directories writable
sessiondir = os.path.abspath(os.path.join(self.params['rundir'], "../.."))
cmd = "chmod 777 `find %s -type d`"%(sessiondir)
subprocess.Popen(cmd, shell=True, 
    stdout=subprocess.PIPE, stderr=subprocess.PIPE)

I have since removed this code in r13706 and the line that sets the umask we should just make sure everyone sets things to an appropriate umask in their .cshrc/.bashrc files.

Actions #8

Updated by Christopher Irving over 14 years ago

Actually python's umask function works just as it should. The only place the setUmask()function is called on line 37 appion/appionlib/basicScript.py and createDirectory() is called in several places. As long as they are used it will work. The umask set by calling setUmask() doesn't work in the above code because it's forking a new shell to run the command. So if the user sets the umask in his startup scripts, that's the one that will be in effect when the chmod is executed. Neil's bit of code is unnecessary if you use the createDirectory() to create any directories.

Actions #9

Updated by Amber Herold over 14 years ago

  • Status changed from In Code Review to In Test

looks good. How to test?

Actions #10

Updated by Gabriel Lander over 14 years ago

  • Assignee changed from Christopher Irving to Gabriel Lander

I can check out a testing version of myami so it doesn't mess up our current installation - would that work?

Actions #11

Updated by Gabriel Lander over 14 years ago

  • Status changed from In Test to Closed

looks good

Actions

Also available in: Atom PDF