Please support "numeric_owner" in tarfile

[mvo]

BPO	23193
Nosy	@gustaebel, @ericvsmith, @bitdancer, @berkerpeksag, @serhiy-storchaka
Files	tarfile-numeric-owner.diff: Patch (without test!) that adds a new "numeric_owner" kwarg to extract() tarfile-numeric-owner-with-tests.diff tarfile-numeric-owner-with-tests-1.diff tarfile-numeric-owner-with-tests-2.diff tarfile-numeric-owner-with-tests-3.diff tarfile-numeric-owner-with-tests-4.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ericvsmith'
closed_at = <Date 2015-04-15.14:29:57.889>
created_at = <Date 2015-01-08.14:42:24.546>
labels = ['type-feature', 'library']
title = 'Please support "numeric_owner" in tarfile'
updated_at = <Date 2015-05-13.05:01:59.994>
user = 'https://bugs.python.org/mvo'

bugs.python.org fields:

activity = <Date 2015-05-13.05:01:59.994>
actor = 'python-dev'
assignee = 'eric.smith'
closed = True
closed_date = <Date 2015-04-15.14:29:57.889>
closer = 'eric.smith'
components = ['Library (Lib)']
creation = <Date 2015-01-08.14:42:24.546>
creator = 'mvo'
dependencies = []
files = ['37645', '37803', '38914', '38953', '38981', '38998']
hgrepos = []
issue_num = 23193
keywords = ['patch']
message_count = 19.0
messages = ['233663', '233672', '233755', '233883', '233884', '233915', '233935', '233936', '233939', '234427', '240608', '240642', '240729', '240837', '240838', '240963', '241102', '241104', '243034']
nosy_count = 7.0
nosy_names = ['lars.gustaebel', 'eric.smith', 'mvo', 'r.david.murray', 'python-dev', 'berker.peksag', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23193'
versions = ['Python 3.5']

[mvo]

Please consider adding a option to extract a tarfile with the uid/gid instead of the lookup for uname/gname in the tarheader (i.e. what tar --numeric-owner provides).

One use-case is if you unpack a chroot tarfile that contains a /etc/{passwd,group} file with different uid/gid for user/groups like zope that may be present in both host and chroot but have different numbers. With the current approach files owned by this user will get the host uid/gid instead of the uid/gid of the chroot.

Attached is a patch to outline what I have in mind - if there is a chance that this patch goes in I'm happy to write the required test (mocking os.chown()) for this to go in.

Thanks for your consideration,
Michael

[vstinner]

"(...) if there is a chance that this patch goes in I'm happy to write the required test (mocking os.chown()) for this to go in."

We don't accept changes without test. So you must write a test.

Implementing the feature in Python makes sense.

[berkerpeksag]

The patch also needs documentation update for TarFile.extract():

https://docs.python.org/3/library/tarfile.html#tarfile.TarFile.extract

The tarfile documentation is located at Doc/library/tarfile.rst.

[ericvsmith]

I think Michael is asking if the proposed change would ever be accepted. If the answer is "no, not even if you write the tests and update the documentation", then there's no sense putting the work into this. That seems like a reasonable question to me.

I think the proposed change is reasonable, but I'm no tarfile expert.

But since this functionality is available in the tar command as --numeric-owner, I think the feature request itself is a good idea.

[bitdancer]

I concur that this is a reasonable feature request, and it is not one that can be satisfied without modifying the tarfile module (that is, you can't write a simple wrapper to tarfile to get the functionality desired without cutting and pasting the entire chown method).

[gustaebel]

I would argue that a serious alternative to this patch is to simply override the TarFile.chown() method in a subclass. However, I'm not sure if this expects too much of the user.

[bitdancer]

If it weren't for the fact that this feature is something that the tar command provides, I'd agree (the chown method is relatively short). However, since tar *does* provide this feature, it seems reasonable for us to support it as well. Call me +0.5 :)

[ericvsmith]

I don't think we want to encourage the type of coupling that arises from subclassing, especially when when overriding an undocumented method. I'm +1 on the change. I'll review the patch. Michael: can you write the tests, and hopefully docs?

[ericvsmith]

Ignore my review comment on pwd and grp being None. I see that there is a test for it (at least grp), and they're not available on Windows.

[mvo]

Thanks everyone for the comments and feedback!

Attached is a updated patch with tests and a documentation update.

Feedback is very welcome. I decided to skip the test on systems where root is not uid,gid=0. I could also mock that of course if you prefer it this way.

Thanks,
Michael

[ericvsmith]

Updated patch with a few minor doc tweaks.

The one substantive change I did make was to add numeric_owner to the call to self.chown() when setting directory owners. I believe this is correct, but I need to convince myself and to write a test.

[ericvsmith]

Note that this change will break code that subclasses TarFile and overrides chown(), as suggested in msg233915. I'm not too concerned about that, since chown() is not documented. Ideally it would be renamed to _chown(), but that's probably a separate issue.