Taught util.misc.mkdir() to set umask to allow group-w for makedirs c… #4000
Conversation
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7653 Required PR: pulp/pulp#4000
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7654 Required PR: pulp/pulp#4000
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7654 Required PR: pulp/pulp#4000
| @@ -124,7 +124,7 @@ def _open_metadata_file_handle(self): | |||
| parent_dir = os.path.dirname(self.metadata_file_path) | |||
|
|
|||
| if not os.path.exists(parent_dir): | |||
There was a problem hiding this comment.
you don't need to check for presence of the path here, remove the if condition
There was a problem hiding this comment.
The os.access() check in the elif is only needed if we're not creating the path in the mkdir() call. I tried a couple of iterations here, and decided that while this one is a little redundant around the mkdir(), it seemed more clear in terms of understanding the code. What do you think?
There was a problem hiding this comment.
oooh right, i did not notice the elif statement, i am cool with leaving if
| @@ -6,22 +6,7 @@ | |||
| from hashlib import sha256 | |||
|
|
|||
| from pulp.server.config import config | |||
|
|
|||
|
|
|||
| def mkdir(path): | |||
There was a problem hiding this comment.
👍 let's say no to the code duplication!
| except OSError as exc: | ||
| if exc.errno != errno.EEXIST: | ||
| raise | ||
| misc.mkdir(profile_directory, mode=0755) |
There was a problem hiding this comment.
i think this is important and i understand correctly how umask works - if having mode 0755 the result will be:
usr grp others
-rwx r-x r-x is represented in octal as 0755
-000 000 010 umask of 002 removes any permission where there is a 1
-rwx r-x r-x is the result achieved requesting 0755 with umask of 002...and we are screwed :)
if having mode 0775 the result will be:
usr grp others
-rwx rwx r-x is represented in octal as 0775
-000 000 010 umask of 002 removes any permission where there is a 1
-rwx rwx r-x is the result achieved requesting 0775 with umask of 002
There was a problem hiding this comment.
I left a few things like certificate-directories and this profiling-directory alone - they're not 'content', and are more sensitive to permissions. Unless we're migrating profiling-information, this set of perms shouldn't affect migration, and I felt it better to leave the permissions as they had been specifically set.
(Your understanding of umask is correct - it turns off permission-bits where the umask has a 1. So 0755, with the mkdirs() umask of 0002, leaves 0755)
|
@rohanpm please take a look to be sure that you are not impacted by this change in an unexpected way |
|
|
||
| :param args: path[, mode] that goes to os.makedirs | ||
| :param kwargs: path | ||
| :return: | ||
| """ | ||
| try: | ||
| mask = os.umask(0002) | ||
| os.makedirs(*args, **kwargs) |
There was a problem hiding this comment.
The umask is apparently never restored if this raises an exception. Should the original os.umask be above the "try", and the umask reset in a new "finally" block?
There was a problem hiding this comment.
...I am an idiot. I had exactly that at some point, and apparently lost it somewhere along the way. Thank you for the great catch, that would have been very embarrassing to allow out into the wild!
Thanks Ina, I don't see any reason this would be harmful for RHSM Pulp, though I believe there is a bug where umask may not be restored when intended. |
…alls. Hunted down a LOT of makedirs calls and replaced with call to misc.mkdir() fixes #7445
ggainey
force-pushed
the
7445_pulp2_makedirs
branch
from
f16d972
to
8f8b9d5
Compare
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7653 Required PR: pulp/pulp#4000
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7653 Required PR: pulp/pulp#4000 (cherry picked from commit 234c399)
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7654 Required PR: pulp/pulp#4000 (cherry picked from commit 69dbf8d)
Tracked down a lot of makedirs() calls and replaced them with misc.mkdir() Note: required-PR is needed to fix the permission problem, but the code will build/work without it. fixes #7653 Required PR: pulp/pulp#4000 (cherry picked from commit 234c399)
…alls.
Hunted down a LOT of makedirs calls and replaced with call to misc.mkdir()
fixes #7445