Bug 214981

Summary: [webkitcorepy] Make scripts called with sudo use a userspace autoinstall
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, dewei_zhu, jbedard, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=214950
https://bugs.webkit.org/show_bug.cgi?id=214378
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jonathan Bedard 2020-07-30 12:49:42 PDT
This is important because often, the first script a bot runs will be installing something with sudo, which means that the autoinstall directory will be owned by root.
Comment 1 Radar WebKit Bug Importer 2020-07-30 12:50:01 PDT
<rdar://problem/66342996>
Comment 2 Jonathan Bedard 2020-07-30 12:51:56 PDT
Created attachment 405597 [details]
Patch
Comment 3 dewei_zhu 2020-07-30 13:27:44 PDT
Comment on attachment 405597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405597&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:278
> +            owner = cls.owner()
> +            if owner:
> +                os.chown(creation_root, *owner)
> +                for root, directories, files in os.walk(creation_root):
> +                    for directory in directories:
> +                        os.chown(os.path.join(root, directory), *owner)
> +                    for file in files:
> +                        os.chown(os.path.join(root, file), *owner)
> +

This seems a repeating twice, maybe create a helper which can also help with nested indentation:
@classmethod
def change_ownership_for_path(cls, path, recursive=False):
    # Windows doesn't have sudo
    if not hasattr(os, "geteuid"):
        return

    # If we aren't root, the default behavior is correct
    if os.geteuid() != 0:
        return

    # If running as sudo, we really want the caller of sudo to own the autoinstall directory
    uid = os.environ.get('SUDO_UID', -1))
    gid = int(os.environ.get('SUDO_GID', -1))

    os.chown(path, uid, gid)

    if not recursive:
        return

    if not os.path.isdir(path):
        return

    for root, directories, files in os.walk(path):
        for directory in directories:
            os.chown(os.path.join(root, directory), uid, gid)
        for file in files:
            os.chown(os.path.join(root, file), uid, gid)
Comment 4 Jonathan Bedard 2020-07-30 16:32:14 PDT
Created attachment 405633 [details]
Patch
Comment 5 dewei_zhu 2020-07-30 16:35:51 PDT
Comment on attachment 405633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405633&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:238
> +            return None

Even thought it's equivalent,, may be just `return` to keep the consistency.
Comment 6 Jonathan Bedard 2020-07-30 16:45:42 PDT
Created attachment 405636 [details]
Patch
Comment 7 EWS 2020-07-30 17:42:46 PDT
Committed r265123: <https://trac.webkit.org/changeset/265123>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405636 [details].