X Tutup
The Wayback Machine - https://web.archive.org/web/20240502113931/https://github.com/python/cpython/issues/77321
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[shutil] chown should not be defined in Windows #77321

Open
eryksun opened this issue Mar 25, 2018 · 13 comments
Open

[shutil] chown should not be defined in Windows #77321

eryksun opened this issue Mar 25, 2018 · 13 comments
Labels
3.11 only security fixes easy OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@eryksun
Copy link
Contributor

eryksun commented Mar 25, 2018

BPO 33140
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @akulakov, @kamilturek, @jdevries3133, @nightlark
PRs
  • bpo-33140: skip defining shutil.chown() on windows #26973
    		•
    Files
  • roughly_add_depr_warning.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 = None
closed_at = None
created_at = <Date 2018-03-25.22:55:36.117>
labels = ['easy', 'type-bug', 'library', 'OS-windows', '3.11']
title = 'shutil.chown should not be defined in Windows'
updated_at = <Date 2021-08-18.02:34:57.249>
user = 'https://github.com/eryksun'

    bugs.python.org fields:

    activity = <Date 2021-08-18.02:34:57.249>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'Windows']
creation = <Date 2018-03-25.22:55:36.117>
creator = 'eryksun'
dependencies = []
files = ['50203']
hgrepos = []
issue_num = 33140
keywords = ['patch', 'easy']
message_count = 13.0
messages = ['314436', '314437', '389139', '396789', '396796', '396797', '396798', '396799', '399038', '399040', '399054', '399812', '399815']
nosy_count = 9.0
nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'andrei.avk', 'kamilturek', 'jack__d', 'rmast']
pr_nums = ['26973']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33140'
versions = ['Python 3.11']
    @eryksun
    Copy link
    Contributor Author

    eryksun commented Mar 25, 2018

    shutil.chown is defined in Windows even though it's only written for Unix and only documented as available in Unix. Defining it should be skipped on Windows.

    Possibly in 3.8 shutil.chown could be implemented on Windows by calling a new os.set_owner function that supports user/group names and SID strings. It could copy how icacls.exe allows using SDDL aliases and string SIDs that begin with an asterisk (e.g. "*BA" and "*S-1-32-544" for BUILTIN\Administrators). If the string starts with an asterisk, get the SID via ConvertStringSidToSid. Otherwise get the SID via LookupAccountName. Then to modify the file's user and group, try to enable SeRestorePrivilege for the current thread and call Set[Named]SecurityInfo.

    @eryksun eryksun added topic-IO 3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir OS-windows type-bug An unexpected behavior, bug, or error labels Mar 25, 2018
    @eryksun
    Copy link
    Contributor Author

    eryksun commented Mar 25, 2018

    Oops, the SID for BUILTIN\Administrators is S-1-5-32-544. I left out the authority ID (5).

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Mar 20, 2021

    Apparently there's no one else interested in implementing shutil.chown() in Windows. Okay, but as long as that's the case, the definition should be skipped in Windows, which is an easy problem.

    @eryksun eryksun added easy 3.9 only security fixes 3.10 only security fixes and removed topic-IO 3.7 (EOL) end of life labels Mar 20, 2021
    @eryksun eryksun changed the title shutil.chown on Windows shutil.chown should not be defined in Windows Mar 20, 2021
    @akulakov
    Copy link
    Contributor

    Created the PR: https://github.com/python/cpython/pull/26973/files

    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    We can't delete the definition without going through a deprecation process, as it will break existing code with a new exception at the point of access rather than the point of use. At best, we can short-circuit those errors and raise them with a more appropriate description.

    For 3.11, we can deprecate the function on Windows and plan to remove it in 3.13. I don't see any problem with this, but it's a different change from PR 26973.

    @zooba zooba added 3.11 only security fixes and removed 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Jun 30, 2021
    @akulakov
    Copy link
    Contributor

    Steve: it seems to me that the goal of normal deprecation process is, given that a functionality is available and documented, prepare to find a replacement for it by some future version.

    In this case it's documented not to be available and doesn't work so this should perhaps fall under fixing a bug?

    It's true that this is backwards incompatible for the reason you pointed out (and I missed), but I think breaking compatibility is ok in new versions for bug fixes? (as long as it's added to "what's new", which I haven't done yet).

    If I'm wrong about this, I'll go ahead and make another PR for deprecation and close this one..

    @zooba
    Copy link
    Member

    zooba commented Jun 30, 2021

    I agree that's theoretically how it should go, but we've had enough examples of undocumented/buggy behaviour where the fix was worse than the bug (to the point where we brought back an undocumented C field that was deprecated in 3.0 because removing it in 3.8 broke too many people).

    A couple of versions with a warning on will reduce the surprise. People are (slowly) learning that DeprecationWarnings matter, so I don't think it'll be wasted effort.

    @akulakov
    Copy link
    Contributor

    Steve: makes sense, I closed the PR; thanks for looking at this and taking the time to explain!

    @jdevries3133
    Copy link
    Mannequin

    jdevries3133 mannequin commented Aug 5, 2021

    I'm pretty sure the 3.11 dev cycle started since this conversation, right? Can we introduce the deprecation warning now? Maybe something like what is in the attached diff?

    @andrei.avk, if it turns out that the time has come, you can go ahead and take the PR if you wish!

    @akulakov
    Copy link
    Contributor

    akulakov commented Aug 5, 2021

    Jack: I'm not sure if this eventual fix is worth doing.. it might cause issues for users to leave it as is, but can also cause issues to remove it even after deprecation period; I don't have a good feeling if one is more likely than the other, - so I'll have to leave it for others to review the PR; thanks for following up on this though!

    @jdevries3133
    Copy link
    Mannequin

    jdevries3133 mannequin commented Aug 6, 2021

    Yes, I definitely get that, but that's what the deprecation cycle is for. Certainly hold off on a PR until we see what @steve.dower thinks.

    I personally feel that having a function that can be introspected with dir but which should not be used is confusing, and rightfully pointed out as a wrinkle in the API.

    @nightlark
    Copy link
    Mannequin

    nightlark mannequin commented Aug 18, 2021

    If this function were to be implemented on Windows would the preferred approach be the one described in the initial message for this issue of creating a Windows os.set_owner function that uses the appropriate Windows API calls to change owner/group settings, and then using that for Windows in the shutil.chown function?

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Aug 18, 2021

    creating a Windows os.set_owner function that uses the
    appropriate Windows API calls to change owner/group settings,
    and then using that for Windows in the shutil.chown function?

    I'd gladly help with implementing os.set_owner(), as a separate issue. But I've changed my mind about supporting more POSIX functions in Windows. I'd prefer to see more Windows-only functionality added, as well as simplified cross-platform support that's not based on and biased by POSIX.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @eryksun eryksun changed the title shutil.chown should not be defined in Windows [shutil] chown should not be defined in Windows Aug 8, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.11 only security fixes easy OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @akulakov @eryksun @zooba
    X Tutup