classification
Title: pathlib.Path: add `username` argument to `home()`
Type: enhancement Stage: resolved
Components: Library (Lib) Versions:
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: barneygale, eryksun, serhiy.storchaka, steve.dower
Priority: normal Keywords: patch

Created on 2021-01-22 03:57 by barneygale, last changed 2021-06-13 15:51 by barneygale. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 25271 closed barneygale, 2021-04-08 01:24
Messages (12)
msg385472 - (view) Author: Barney Gale (barneygale) * Date: 2021-01-22 03:57
The `pathlib.Path.home()` function looks like:

    @classmethod
    def home(cls):
        """Return a new path pointing to the user's home directory (as
        returned by os.path.expanduser('~')).
        """
        return cls(cls()._flavour.gethomedir(None))

If we add a `username=None` parameter and pass this to `gethomedir()` we can easily add a lookup of another user's home directory, so:

    import pathlib
    username = 'phil'
    pathlib.Path.home(username) == pathlib.Path('~' + username).expanduser()
msg385473 - (view) Author: Barney Gale (barneygale) * Date: 2021-01-22 04:11
I should add that this is part of a plan to spin out some `Path` methods into a new `UserPath` class. Notably, in this case, `UserPath.expanduser()` will call `self.home()` under-the-hood. This is done to reduce the surface area of abstract methods that subclasses of `UserPath` need implement. More discussion here: https://discuss.python.org/t/make-pathlib-extensible/3428
msg390503 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-04-08 07:09
os.path.expanduser() has flaws.

1. It can return an argument unchanged if failed to determine the home directory.
2. On Unix it does not work for users not found in the local password database.
3. On Windows it only guess the home directory for other users. It does not work if you have home directory at nonstandard location (i.e. on other disk).

Until we fix these flaws it is better to not add flawed by design feature in pathlib.
msg390527 - (view) Author: Barney Gale (barneygale) * Date: 2021-04-08 11:51
Thanks for the feedback.

1. We can check the return value, like we do in `Path.expanduser()`
2. Seems like expected behaviour?
3. Worth fixing in `ntpath.expanduser()` I think. See Eryk Sun's comment here: https://bugs.python.org/issue39899#msg390507
msg390535 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-04-08 15:50
> 3. Worth fixing in `ntpath.expanduser()` 

To expand on the suggestion in msg390507, here's a demo that implements getting the profile path of a local user account, with support for the special profile directories "Default", "Public", and "ProgramData" (i.e. ALLUSERSPROFILE).

    ERROR_NONE_MAPPED = 1332

    def get_profile_path(name):
        if not isinstance(name, str):
            raise TypeError(f'name must be str, not {type(name).__name__}')
        profile_list = r'Software\Microsoft\Windows NT\CurrentVersion\ProfileList'
        if name.lower() in ('default', 'public', 'programdata'):
            subkey = profile_list
            value = name
        else:
            try:
                sid = lookup_account_name(name)[0]
            except OSError as e:
                if e.winerror != ERROR_NONE_MAPPED:
                    raise
                raise KeyError(f'name not found: {name}')
            subkey = f'{profile_list}\\{sid}'
            value = 'ProfileImagePath'
        try:
            with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, subkey) as hkey:
                path, dtype = winreg.QueryValueEx(hkey, value)
        except FileNotFoundError:
            raise KeyError(f'name not found: {name}')
        if dtype == winreg.REG_SZ:
            return path
        if dtype == winreg.REG_EXPAND_SZ:
            return winreg.ExpandEnvironmentStrings(path)
        raise TypeError('profile path value must be a string type (1 or 2), '
                         f'not {dtype}')

For example:

    >>> print(get_profile_path('administrator'))
    C:\Users\Administrator

    >>> print(get_profile_path('default'))
    C:\Users\Default

    >>> print(get_profile_path('public'))
    C:\Users\Public

    >>> print(get_profile_path('programdata'))
    C:\ProgramData

    >>> print(get_profile_path('system'))
    C:\Windows\system32\config\systemprofile

    >>> print(get_profile_path('localservice'))
    C:\Windows\ServiceProfiles\LocalService

    >>> print(get_profile_path('networkservice'))
    C:\Windows\ServiceProfiles\NetworkService

For lookup_account_name(), _winapi.LookupAccountName(system_name, account_name) has to be implemented. It should convert the SID result to string form via ConvertSidToStringSidW() and return the tuple (sid_string, domain_name, account_type). Here's a ctypes prototype implementation of lookup_account_name():

    import ctypes

    lsalookup = ctypes.WinDLL(
        'api-ms-win-security-lsalookup-l2-1-0', use_last_error=True)
    sddl = ctypes.WinDLL('api-ms-win-security-sddl-l1-1-0', use_last_error=True)
    heap = ctypes.WinDLL('api-ms-win-core-heap-l2-1-0', use_last_error=True)

    ERROR_INSUFFICIENT_BUFFER = 122

    def lookup_account_name(name):
        sid = (ctypes.c_char * 1)()
        cb = ctypes.c_ulong()
        cch = ctypes.c_ulong()
        name_use = ctypes.c_ulong()
        lsalookup.LookupAccountNameW(None, name, sid, ctypes.byref(cb),
            None, ctypes.byref(cch), ctypes.byref(name_use))
        error = ctypes.get_last_error()
        if error != ERROR_INSUFFICIENT_BUFFER:
            raise ctypes.WinError(error)

        sid = (ctypes.c_char * cb.value)()
        domain_name = (ctypes.c_wchar * cch.value)()
        success = lsalookup.LookupAccountNameW(None, name, sid,
                        ctypes.byref(cb), domain_name, ctypes.byref(cch),
                        ctypes.byref(name_use))
        if not success:
            raise ctypes.WinError(ctypes.get_last_error())

        ssid = ctypes.c_wchar_p()
        if not sddl.ConvertSidToStringSidW(sid, ctypes.byref(ssid)):
            raise ctypes.WinError(ctypes.get_last_error())
        string_sid = ssid.value
        heap.LocalFree(ssid)

        return string_sid, domain_name.value, name_use.value
msg391965 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-26 18:32
I'm not convinced this is a good change, mainly for the reasons Serhiy mentioned (and others have mentioned when I've spoken with them privately).

What would you do with your custom subclass if home() did not have this argument?
msg391967 - (view) Author: Barney Gale (barneygale) * Date: 2021-04-26 19:08
Thanks very much for taking a look. I can understand the view that, given the unreliability of `os.path.expanduser('~foo')`, we shouldn't be making that functionality *more* available.

My argument for this being a reasonable change is as follows:

Firstly, `os.getcwd()` is a more fundamental operation than `os.path.absolute()`. In both pathlib and os.path, the `absolute()` implementation is built on the `getcwd()` implementation. The process is: call the more fundamental operation, and append whatever else was passed as an argument.

Analagously, `gethomedir()` is a more fundamental operation than `expanduser()`. In the same way, `expanduser()` can call a more fundamental operation and then append the path passed in.

Secondly, for a `pathlib.AbstractPath` implementation, it's a much cleaner API to have `getcwd()` and `gethomedir()` as abstract methods with identical signatures, and `absolute()` and `expanduser()` with default implementations.

Thirdly, if there's are issues with the implementation for retrieving other user's home directories, those should be treated as individual issues with their own bugs, and we should add suitable warnings in the docs in the short term. My patch should make these cases *easier* to solve as we no longer have two competing implementations of the same thing.

Finally, I don't think anyone is going to see `home('barney')` in the docs and decided to retrieve a user's home directory on a whim. If someone *does* need to retrieve another user's home directory, they're going to use `Path('~' + user).expanduser()` if `Path.home(user)` isn't available.

My argument mostly comes from the similarities I perceive between getcwd/absolute and gethomedir/expanduser. We don't expect users to `os.path.absolute('.')` to get the current working directory, after all! But perhaps I've overestimated the similarities here. Any thoughts?

Thanks again for taking a look.
msg391968 - (view) Author: Barney Gale (barneygale) * Date: 2021-04-26 19:10
In the previous comment, I was referring to bpo-39899 when I referred to "my patch". Long day! :D
msg391970 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-26 19:23
Yeah, that's all reasonable.

But why wouldn't we go the other way and say "getting a user directory for a different user is not supported and all existing ways to do it are now deprecated"?

I know you think that's not a relevant argument right now, but it is a related one, and any change to the status quo justifies bringing it back up. So now it's up, we want to decide on it before we make another change that could run counter to the overall direction.

So I think, given the unlikelihood of us being able to properly fix the functionality, that we should deprecate getting another user's directory completely and, hence, not add new entry points to deprecated API.

Convince me I'm wrong :)
msg391972 - (view) Author: Barney Gale (barneygale) * Date: 2021-04-26 19:29
Totally valid!

I suppose it hinges on the relatively likelihood/unlikelihood of us being able to make `expanduser('~foo')` reliable in future. On this question I'm relieved to defer to the experts!

@eryksun: penny for your thoughts?
msg392241 - (view) Author: Barney Gale (barneygale) * Date: 2021-04-28 18:07
So far I've been keen to keep the Path and AbstractPath APIs the same, but perhaps this case warrants an exception.

What if AbstractPath lacked both home() and expanduser(), and those methods only existed on Path? Of all the methods on Path, these two are the *least* useful for non-local filesystems.

We can then circle back round to this issue when the fate of expanduser() becomes clearer.

Does that sound any good?
msg395755 - (view) Author: Barney Gale (barneygale) * Date: 2021-06-13 15:51
Per discussion, expanduser('~other') is considered unreliable, and we shouldn't add new functions that call through to it. Resolving as 'rejected'.
History
Date User Action Args
2021-06-13 15:51:38barneygalesetstatus: open -> closed
resolution: rejected
messages: + msg395755

stage: patch review -> resolved
2021-04-28 18:07:09barneygalesetmessages: + msg392241
2021-04-26 19:29:48barneygalesetmessages: + msg391972
2021-04-26 19:23:57steve.dowersetmessages: + msg391970
2021-04-26 19:10:56barneygalesetmessages: + msg391968
2021-04-26 19:08:15barneygalesetmessages: + msg391967
2021-04-26 18:32:48steve.dowersetnosy: + steve.dower
messages: + msg391965
2021-04-08 15:50:33eryksunsetnosy: + eryksun
messages: + msg390535
2021-04-08 11:51:40barneygalesetmessages: + msg390527
2021-04-08 07:09:57serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg390503
2021-04-08 01:24:25barneygalesetkeywords: + patch
stage: patch review
pull_requests: + pull_request24004
2021-01-22 04:11:31barneygalesetmessages: + msg385473
2021-01-22 03:57:15barneygalecreate