Page MenuHomePhabricator

Add read_private() function to python-toolforge library
Closed, ResolvedPublicFeature

Description

I have this function in cookiecutter-toolforge and all tools generated with it:

@decorator.decorator
def read_private(func, *args, **kwargs):
    try:
        f = args[0]
        fd = f.fileno()
    except AttributeError:
        pass
    except IndexError:
        pass
    else:
        mode = os.stat(fd).st_mode
        if (stat.S_IRGRP | stat.S_IROTH) & mode:
            name = getattr(f, "name", "config file")
            raise ValueError(f'{name} is readable to others, '
                             'must be exclusively user-readable!')
    return func(*args, **kwargs)

It’s used to load the tool’s config:

has_config = app.config.from_file('config.yaml',
                                  load=read_private(yaml.safe_load),
                                  silent=True)

And ensures that, if the config is world-readable, the tool will immediately refuse to start up. (Ideally, the user would then realize that they need to rotate the secret key and, if there was an OAuth consumer in the config file, request a new one; but some will probably just chmod 600 the file and hope nobody saw it.)

It would be nice to have this function in a shared place instead of copy+pasting it into every tool, and the toolforge library might be a good place for it. What do you think?

Event Timeline

bd808 changed the subtype of this task from "Task" to "Feature Request".Mar 31 2023, 10:07 PM
bd808 subscribed.

That seems like a nice helper for a common foot-gun in the current Toolforge world.

Minor quibbles for the actual implementation:

  • read_private sounds to me like it actually does the work of reading something. The implementation is maybe more descriptively assert_private_file?
  • stat.S_IRGRP (group read) is technically fine as well as long as the group is the tool's group (meaning tools.<name>) and not wikidev or another cross-tool group. Maybe it is easier though to ignore that reality for the sake of code complexity?

read_private sounds to me like it actually does the work of reading something. The implementation is maybe more descriptively assert_private_file?

Feels a bit long to me ^^ maybe an adverb would read nicely when used as a decorator? load=privately(yaml.safe_load)

Though I guess we could also do something like

def assert_private_file(...):
    ...

try:
    import yaml
except ModuleNotFoundError:
    pass
else:
    load_private_yaml = assert_private_file(yaml.safe_load)

# same for json, toml?, whatever

and then most users can from toolforge import load_private_yaml and not care about the decorator itself. (And the try/except/else dance would ensure that we can keep the PyYAML dependency optional.)

stat.S_IRGRP (group read) is technically fine as well as long as the group is the tool's group (meaning tools.<name>) and not wikidev or another cross-tool group. Maybe it is easier though to ignore that reality for the sake of code complexity?

If we want to allow group-readable files, I think we can just adjust the bitmask. I don’t think we need to check what the file’s group is – I don’t think a nonstandard group is likely to happen as an accident that this function needs to protect against.

read_private sounds to me like it actually does the work of reading something. The implementation is maybe more descriptively assert_private_file?

Feels a bit long to me ^^ maybe an adverb would read nicely when used as a decorator? load=privately(yaml.safe_load)

Long as in too many characters to type? That feels like an unimportant concern. Omnicomplete is a common thing, and even when it is not available this would not be something that people are using dozens of times in an application. It more likely to be used once.

Though I guess we could also do something like

def assert_private_file(...):
    ...

try:
    import yaml
except ModuleNotFoundError:
    pass
else:
    load_private_yaml = assert_private_file(yaml.safe_load)

# same for json, toml?, whatever

and then most users can from toolforge import load_private_yaml and not care about the decorator itself. (And the try/except/else dance would ensure that we can keep the PyYAML dependency optional.)

This sounds useful, but also at least partially speculative. Maybe just make the YAML wrapper that would match your known usage and we can let variants be added as people ask for them/submit patches?

stat.S_IRGRP (group read) is technically fine as well as long as the group is the tool's group (meaning tools.<name>) and not wikidev or another cross-tool group. Maybe it is easier though to ignore that reality for the sake of code complexity?

If we want to allow group-readable files, I think we can just adjust the bitmask. I don’t think we need to check what the file’s group is – I don’t think a nonstandard group is likely to happen as an accident that this function needs to protect against.

I think at least optionally allowing g+r could be useful.

(The length of the name matters not only when it’s written, but also when it’s read, which is more frequent. But whatever, with the added YAML function it doesn’t matter.)

I started a draft MR (!21); maybe I formatted the description incorrectly, or else the bot that’s supposed to mention it here is configured to skip draft MRs. I’ll add tests later. (Edit: Tests are now added and the MR should be ready.)

bd808 changed the task status from Open to In Progress.Apr 10 2023, 5:58 PM
bd808 assigned this task to LucasWerkmeister.
bd808 triaged this task as Medium priority.
bd808 moved this task from Needs Discussion to Doing on the Tool-python-toolforge board.

During code review I missed that the implementation MR did not add a description of the new feature to HISTORY.rst.

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T12:22:59Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed cae8c3c341 (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T17:19:42Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed 86ffb4c5c0 (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T17:32:27Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed 56f47f15b7 (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T17:41:01Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed a167fc8e71 (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T21:56:07Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed 9d8920826c (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

Mentioned in SAL (#wikimedia-cloud) [2025-06-11T22:00:33Z] <wmbot~lucaswerkmeister@tools-bastion-13> deployed 9d8920826c (upgrade dependencies, including toolforge 6.1.0; use toolforge.load_private_yaml() from T333728)

cookiecutter-toolforge also updated; with that, I think this task is now properly and fully done ^^

(Codesearch finds two other tools that have their own read_private() decorator, created via cookiecutter-toolforge, which could now use the toolforge library instead, but I don’t think it’s worth bothering the maintainers about that proactively.)