Repository: lektor/lektor (python)

Issue:
File mask for build artifacts 
```
> ls -l build/*
-rw------- 1 xyz xyz 20451 Nov 26 00:08 build/index.html

build/de:
total 24
-rw------- 1 xyz xyz 20518 Nov 26 00:08 index.html

build/static:
total 16
drwxr-xr-x  2 xyz xyz 4096 Nov 26 00:03 css/
drwxr-xr-x  2 xyz xyz 4096 Nov 26 00:03 img/
drwxr-xr-x  2 xyz xyz 4096 Nov 26 00:03 js/
```

Looks like currently directories are created with the convenient `drwxr-xr-x` permissions, files however are assigned `read`/`write` only to the user `-rw-------`. In my case it would be much more convenient, if file group would also have the same or just the read permission: `-rw-r-----`.

I've managed to achieve the desired effect with the following plugin:
```
from lektor.pluginsystem import Plugin
from pathlib import Path


class UmaskPlugin(Plugin):
    name = 'umask'
    description = "640 permissions on output files."

    def on_after_build(self, prog, **extra):
        filemode = "640"
        artifact = prog.primary_artifact
        if not artifact:
            return
        path = Path(artifact.dst_filename)
        if path.is_file():
            path.chmod(int(filemode, 8))
```

But despite my great success, I guess ideally there should be `umask` setting within the `Project File`, so that users may choose whatever works best for them? :thinking: 

Pull request:
Fix file mode of created files
<!---
Remember to please check that your code passes tests locally. Test with the Makefile
commands `make test`, or if you only need to test just Python / JS `make test-python`
or `make test-js`.
--->

### Issue(s) Resolved

<!--- List the issue(s) below, in the form "Fixes #1234"; one per line --->

Fixes #1213

This PR also fixes a related buglet having to do with the mode of files created/edited using `lektor.utils.atomic_open`.
`Atomic_open` is used when editing `contents.lr` files via the admin GUI.

<!---
### Related Issues / Links


Are there any similar or related issues or pull requests?
Did you make a pull request to update the docs?
--->

### Description of Changes

- [x] Wrote at least one-line docstrings (for any new functions)
- [x] Added unit test(s) covering the changes (if testable)


<!--- Explain what you've done and why --->

This fixes things so that artifacts are created with the same permissions as any normally created file would have.
I.e. newly created files will now have mode `0o666 & ~umask` — previously, artifacts either got mode 0o644 (prior to 3.4.0b7) or 0o600 (since then).

<!--- Thanks for your help making Lektor better for everyone! --->


Patch:
diff --git a/lektor/builder.py b/lektor/builder.py
index 68667c7..94e74c8 100644
--- a/lektor/builder.py
+++ b/lektor/builder.py
@@ -6,7 +6,6 @@ import shutil
 import sqlite3
 import stat
 import sys
-import tempfile
 from collections import deque
 from collections import namedtuple
 from contextlib import contextmanager
@@ -23,6 +22,7 @@ from lektor.constants import PRIMARY_ALT
 from lektor.context import Context
 from lektor.reporter import reporter
 from lektor.sourcesearch import find_files
+from lektor.utils import create_temp
 from lektor.utils import fs_enc
 from lektor.utils import process_extra_flags
 from lektor.utils import prune_file_and_folder
@@ -666,6 +666,7 @@ class Artifact:
         artifact_name,
         dst_filename,
         sources,
+        *,
         source_obj=None,
         extra=None,
         config_hash=None,
@@ -725,9 +726,11 @@ class Artifact:
 
         if ensure_dir:
             self.ensure_dir()
-        fd, self._new_artifact_file = tempfile.mkstemp(
-            dir=os.path.dirname(self.dst_filename),
+
+        fd, self._new_artifact_file = create_temp(
             prefix=".__trans",
+            dir=os.path.dirname(self.dst_filename),
+            text="b" not in mode,
         )
         return open(fd, mode, encoding=encoding)
 
diff --git a/lektor/cli.py b/lektor/cli.py
index 43429b0..b86923e 100644
--- a/lektor/cli.py
+++ b/lektor/cli.py
@@ -92,6 +92,7 @@ def cli(ctx, project=None, language=None):
 @pass_context
 def build_cmd(
     ctx,
+    *,
     output_path,
     watch,
     prune,
@@ -175,7 +176,7 @@ def build_cmd(
 @click.confirmation_option(help="Confirms the cleaning.")
 @extraflag
 @pass_context
-def clean_cmd(ctx, output_path, verbosity, extra_flags):
+def clean_cmd(ctx, *, output_path, verbosity, extra_flags):
     """Cleans the entire build folder.
 
     If not build folder is provided, the default build folder of the project
@@ -229,7 +230,7 @@ def clean_cmd(ctx, output_path, verbosity, extra_flags):
 )
 @extraflag
 @pass_context
-def deploy_cmd(ctx, server, output_path, extra_flags, **credentials):
+def deploy_cmd(ctx, *, server, output_path, extra_flags, **credentials):
     """This command deploys the entire contents of the build folder
     (`--output-path`) onto a configured remote server.  The name of the
     server must fit the name from a target in the project configuration.
@@ -324,7 +325,7 @@ def deploy_cmd(ctx, server, output_path, extra_flags, **credentials):
 @extraflag
 @click.option("--browse", is_flag=True)
 @pass_context
-def server_cmd(ctx, host, port, output_path, prune, verbosity, extra_flags, browse):
+def server_cmd(ctx, *, host, port, output_path, prune, verbosity, extra_flags, browse):
     """The server command will launch a local server for development.
 
     Lektor's development server will automatically build all files into
@@ -382,7 +383,7 @@ def server_cmd(ctx, host, port, output_path, prune, verbosity, extra_flags, brow
     help="Print the path to the package cache.",
 )
 @pass_context
-def project_info_cmd(ctx, as_json, **opts):
+def project_info_cmd(ctx, *, as_json, **opts):
     """Prints out information about the project.  This is particular
     useful for script usage or for discovering information about a
     Lektor project that is not immediately obvious (like the paths
@@ -412,7 +413,7 @@ def project_info_cmd(ctx, as_json, **opts):
 @click.option("as_json", "--json", is_flag=True, help="Prints out the data as json.")
 @click.argument("files", nargs=-1, type=click.Path(dir_okay=False))
 @pass_context
-def content_file_info_cmd(ctx, files, as_json):
+def content_file_info_cmd(ctx, files, *, as_json):
     """Given a list of files this returns the information for those files
     in the context of a project.  If the files are from different projects
     an error is generated.
@@ -539,7 +540,7 @@ def plugins_remove_cmd(ctx, name):
     help="Increases the verbosity of the output.",
 )
 @pass_context
-def plugins_list_cmd(ctx, as_json, verbosity):
+def plugins_list_cmd(ctx, *, as_json, verbosity):
     """This returns a list of all currently actively installed plugins
     in the project.  By default it only prints out the plugin IDs and
     version numbers but the entire information can be returned by
diff --git a/lektor/datamodel.py b/lektor/datamodel.py
index d493018..19d7b34 100644
--- a/lektor/datamodel.py
+++ b/lektor/datamodel.py
@@ -19,6 +19,7 @@ from lektor.utils import slugify
 class ChildConfig:
     def __init__(
         self,
+        *,
         enabled=None,
         slug_format=None,
         model=None,
@@ -47,7 +48,9 @@ class ChildConfig:
 
 
 class PaginationConfig:
-    def __init__(self, env, enabled=None, per_page=None, url_suffix=None, items=None):
+    def __init__(
+        self, env, *, enabled=None, per_page=None, url_suffix=None, items=None
+    ):
         self.env = env
         if enabled is None:
             enabled = False
@@ -153,7 +156,7 @@ class PaginationConfig:
 
 
 class AttachmentConfig:
-    def __init__(self, enabled=None, model=None, order_by=None, hidden=None):
+    def __init__(self, *, enabled=None, model=None, order_by=None, hidden=None):
         if enabled is None:
             enabled = True
         if hidden is None:
@@ -231,6 +234,7 @@ class DataModel:
         env,
         id,
         name_i18n,
+        *,
         label_i18n=None,
         filename=None,
         hidden=None,
@@ -394,6 +398,7 @@ class FlowBlockModel:
         env,
         id,
         name_i18n,
+        *,
         filename=None,
         fields=None,
         order=None,
diff --git a/lektor/devserver.py b/lektor/devserver.py
index 691459d..93c2e2a 100644
--- a/lektor/devserver.py
+++ b/lektor/devserver.py
@@ -93,6 +93,7 @@ def run_server(
     bindaddr: BindAddr,
     env: Environment,
     output_path: StrPath,
+    *,
     prune: bool = True,
     verbosity: int = 0,
     lektor_dev: bool = False,
diff --git a/lektor/editor.py b/lektor/editor.py
index e5ea692..c50ee80 100644
--- a/lektor/editor.py
+++ b/lektor/editor.py
@@ -106,13 +106,13 @@ def make_editor_session(pad, path, is_attachment=None, alt=PRIMARY_ALT, datamode
         pad,
         id,
         str(path),
-        raw_data,
-        raw_data_fallback,
-        datamodel,
-        record,
-        exists,
-        is_attachment,
-        alt,
+        original_data=raw_data,
+        fallback_data=raw_data_fallback,
+        datamodel=datamodel,
+        record=record,
+        exists=exists,
+        is_attachment=is_attachment,
+        alt=alt,
     )
 
 
@@ -143,6 +143,7 @@ class EditorSession:
         pad,
         id,
         path,
+        *,
         original_data,
         fallback_data,
         datamodel,
diff --git a/lektor/imagetools/thumbnail.py b/lektor/imagetools/thumbnail.py
index 2505608..2899390 100644
--- a/lektor/imagetools/thumbnail.py
+++ b/lektor/imagetools/thumbnail.py
@@ -410,6 +410,7 @@ def make_image_thumbnail(
     ctx: Context,
     source_image: str | Path,
     source_url_path: str,
+    *,
     width: int | None = None,
     height: int | None = None,
     mode: ThumbnailMode = ThumbnailMode.DEFAULT,
diff --git a/lektor/publisher.py b/lektor/publisher.py
index 4739f1f..e869bf5 100644
--- a/lektor/publisher.py
+++ b/lektor/publisher.py
@@ -149,6 +149,7 @@ class Command(AbstractContextManager["Command"]):
     def __init__(
         self,
         argline: Iterable[str],
+        *,
         cwd: StrOrBytesPath | None = None,
         env: Mapping[str, str] | None = None,
         capture: bool = True,
diff --git a/lektor/sourceobj.py b/lektor/sourceobj.py
index 3d805bc..55dcdb2 100644
--- a/lektor/sourceobj.py
+++ b/lektor/sourceobj.py
@@ -105,6 +105,7 @@ class SourceObject:
     def url_to(
         self,
         path,  # : Union[str, "SourceObject", "SupportsUrlPath"]
+        *,
         alt: str | None = None,
         absolute: bool | None = None,
         external: bool | None = None,
diff --git a/lektor/utils.py b/lektor/utils.py
index 05e7bd8..9cbcd9a 100644
--- a/lektor/utils.py
+++ b/lektor/utils.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import codecs
+import io
 import json
 import os
 import posixpath
@@ -15,18 +16,24 @@ import uuid
 import warnings
 from collections.abc import Hashable
 from collections.abc import Iterable
+from collections.abc import Iterator
 from contextlib import contextmanager
 from contextlib import suppress
 from dataclasses import dataclass
 from datetime import datetime
 from functools import lru_cache
 from functools import wraps
+from pathlib import Path
 from pathlib import PurePosixPath
 from typing import Any
 from typing import Callable
 from typing import ClassVar
+from typing import IO
+from typing import Literal
 from typing import overload
+from typing import TYPE_CHECKING
 from typing import TypeVar
+from typing import Union
 
 from jinja2 import is_undefined
 from markupsafe import Markup
@@ -35,6 +42,9 @@ from werkzeug.http import http_date
 from werkzeug.urls import iri_to_uri
 from werkzeug.urls import uri_to_iri
 
+if TYPE_CHECKING:
+    from _typeshed import StrPath
+
 
 is_windows = os.name == "nt"
 
@@ -521,33 +531,124 @@ def get_dependent_url(url_path, suffix, ext=None):
     return posixpath.join(url_directory, url_base + "@" + suffix + ext)
 
 
+# These are the only modes we really support
+_AtomicOpenTextMode = Literal["w", "wt", "tw", "r", "rt", "tr"]
+_AtomicOpenBinaryModeWriting = Literal["wb", "bw"]
+_AtomicOpenBinaryModeReading = Literal["rb", "br"]
+_AtomicOpenMode = Union[
+    _AtomicOpenTextMode, _AtomicOpenBinaryModeWriting, _AtomicOpenBinaryModeReading
+]
+
+
+@overload
 @contextmanager
-def atomic_open(filename, mode="r", encoding=None):
-    if "r" not in mode:
-        fd, tmp_filename = tempfile.mkstemp(
-            dir=os.path.dirname(filename), prefix=".__atomic-write"
-        )
-        os.chmod(tmp_filename, 0o644)
-        f = os.fdopen(fd, mode)
-    else:
-        f = open(filename, mode=mode, encoding=encoding)
-        tmp_filename = None
+def atomic_open(
+    filename: StrPath,
+    mode: _AtomicOpenTextMode = "r",
+    encoding: str | None = None,
+) -> Iterator[io.TextIOWrapper]:
+    ...
+
+
+@overload
+@contextmanager
+def atomic_open(
+    filename: StrPath,
+    mode: _AtomicOpenBinaryModeWriting,
+    encoding: None = None,
+) -> Iterator[io.BufferedWriter]:
+    ...
+
+
+@overload
+@contextmanager
+def atomic_open(
+    filename: StrPath,
+    mode: _AtomicOpenBinaryModeReading,
+    encoding: None = None,
+) -> Iterator[io.BufferedReader]:
+    ...
+
+
+@contextmanager
+def atomic_open(
+    filename: StrPath, mode: _AtomicOpenMode = "r", encoding: str | None = None
+) -> Iterator[IO[Any]]:
+    """Open a file for atomic update.
+
+    Perform an "all-or-nothing" write to a file.
+
+    This opens a temporary file to receive writes. It is meant to be used as a context
+    manager. When the context is exited normally, the temporary file is closed then
+    atomically renamed to the target file name.
+
+    If an exception is thrown during this process the temporary file is silently
+    deleted.
+
+    """
+    if any(c in mode for c in "ax+"):
+        raise ValueError(f"unsupported open mode: {mode}")
+
+    if "r" in mode:
+        with open(filename, mode=mode, encoding=encoding) as fp:
+            yield fp
+        return
+
+    fd, tmp_filename = create_temp(
+        prefix=".__atomic-write",
+        dir=Path(filename).parent,
+        text="b" not in mode,
+    )
     try:
-        yield f
-    except Exception as e:
-        f.close()
-        _exc_type, exc_value, tb = sys.exc_info()
-        if tmp_filename is not None:
-            with suppress(OSError):
-                os.remove(tmp_filename)
-
-        if exc_value.__traceback__ is not tb:
-            raise exc_value.with_traceback(tb) from e
-        raise exc_value from e
-
-    f.close()
-    if tmp_filename is not None:
+        with open(fd, mode, encoding=encoding) as fp:
+            yield fp
         os.replace(tmp_filename, filename)
+    except Exception:
+        with suppress(OSError):
+            os.remove(tmp_filename)
+        raise
+
+
+def create_temp(
+    suffix: str = "",
+    prefix: str = "tmp",
+    dir: StrPath | None = None,
+    text: bool = False,
+    mode: int = 0o666,
+) -> tuple[int, str | bytes]:
+    """Create and return a unique temporary file.
+
+    The return value is a pair (fd, name) where fd is the file descriptor returned by
+    os.open, and name is the filename.
+
+    This works very much like `tempfile.mkstemp`, except that it allows more control
+    over the mode (access permissions) of the created file.
+
+    The access permissions of the created file is determined by the value of 'mode'
+    (which defaults to 0o666) combined with any umask or default ACL that may be in
+    place.
+
+    """
+    if (tmp_dir := dir) is None:
+        tmp_dir = os.getcwd()
+
+    flags = os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0)
+    if not text:
+        flags |= getattr(os, "O_BINARY", 0)
+
+    for _ in range(32000):
+        filename = tempfile.mktemp(suffix=suffix, prefix=prefix, dir=tmp_dir)
+        try:
+            fd = os.open(filename, flags, mode)
+        except FileExistsError:
+            continue
+        # NB: Under Windows, PermissionError can be raised by os.open if a directory
+        # with the chosen name already exists. Since mktemp doesn't return names to
+        # existing files/dirs, the likelihood of this happening is slim. For now, we
+        # don't check for it here.
+        return fd, filename
+
+    raise AssertionError("Unable to find temporary file name")
 
 
 def portable_popen(cmd, *args, **kwargs):
diff --git a/lektor/videotools.py b/lektor/videotools.py
index 9869ece..1ab75a9 100644
--- a/lektor/videotools.py
+++ b/lektor/videotools.py
@@ -280,6 +280,7 @@ def make_video_thumbnail(
     source_video,
     source_url_path,
     seek,
+    *,
     width=None,
     height=None,
     mode=ThumbnailMode.DEFAULT,
diff --git a/pyproject.toml b/pyproject.toml
index 9738aea..a8c471e 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -141,11 +141,13 @@ max-module-lines = 2000
 
 [tool.pylint.design]
 max-attributes = 20
+max-positional-arguments = 7   # default is 5
 max-locals = 30
 max-branches = 20
 max-nested-blocks = 8
 max-returns = 8
 
+
 [tool.pylint."messages control"]
 disable = [
     "redundant-u-string-prefix",
diff --git a/tox.ini b/tox.ini
index a204029..3957b0f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -68,7 +68,7 @@ commands =
 base_python = py313,py312,py311,py310,py39
 use_develop = true
 deps =
-    pylint==3.2.7
+    pylint==3.3.2
     pytest>=6
     pytest-mock
     importlib_metadata; python_version<"3.10"


Test patch:
diff --git a/tests/test_builder.py b/tests/test_builder.py
index e9e172c..0fceaa2 100644
--- a/tests/test_builder.py
+++ b/tests/test_builder.py
@@ -431,6 +431,22 @@ def test_Artifact_open_encoding(builder):
         assert fp.read() == "Ciarán"
 
 
+def test_Artifact_file_mode(builder, tmp_path):
+    # Check that created artifacts have same file mode as normally created files.
+    new_file = tmp_path / "dummy-test-file"
+    new_file.touch()
+    new_file_mode = new_file.stat().st_mode
+
+    build_state = builder.new_build_state()
+    artifact = build_state.new_artifact("dummy-artifact", sources=())
+    with artifact.update(), artifact.open("w"):
+        pass
+    artifact_mode = Path(artifact.dst_filename).stat().st_mode
+
+    # applying oct makes failures more readable
+    assert oct(artifact_mode) == oct(new_file_mode)
+
+
 def test_FileInfo_unchanged(env, tmp_path):
     file_path = tmp_path / "file"
     file_path.write_text("foo")
diff --git a/tests/test_utils.py b/tests/test_utils.py
index de26ecf..04ea966 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -1,13 +1,17 @@
 import os
+import stat
 import warnings
 from contextlib import contextmanager
 from dataclasses import dataclass
 from itertools import islice
+from pathlib import Path
 from urllib.parse import urlsplit
 
 import pytest
 
+from lektor.utils import atomic_open
 from lektor.utils import build_url
+from lektor.utils import create_temp
 from lektor.utils import deprecated
 from lektor.utils import is_path_child_of
 from lektor.utils import join_path
@@ -241,6 +245,72 @@ def test_make_relative_url_relative_source_absolute_target():
         make_relative_url("rel/a/tive/", "/abs/o/lute")
 
 
+def test_atomic_open(tmp_path):
+    path = tmp_path / "test.txt"
+    path.write_text("previous")
+
+    with atomic_open(path, "w") as fp:
+        fp.write("new")
+        fp.flush()
+        assert path.read_text() == "previous"
+        assert len(list(tmp_path.iterdir())) == 2
+    assert path.read_text() == "new"
+    assert len(list(tmp_path.iterdir())) == 1
+
+
+def test_atomic_open_exception(tmp_path):
+    path = tmp_path / "test.txt"
+
+    with pytest.raises(RuntimeError, match="test"):
+        with atomic_open(path, "w") as fp:
+            fp.write("new")
+            fp.flush()
+            raise RuntimeError("test")
+    assert len(list(tmp_path.iterdir())) == 0
+
+
+@pytest.fixture(params=[0o022, 0o002, 0x000])
+def umask(request):
+    umask = request.param
+    if os.name == "nt":
+        pytest.skip("Windows does not support umask")
+    saved = os.umask(umask)
+    try:
+        yield umask
+    finally:
+        os.umask(saved)
+
+
+def test_atomic_open_respects_umask(tmp_path, umask):
+    path = tmp_path / "test.txt"
+    with atomic_open(path, "w"):
+        pass
+    assert oct(stat.S_IMODE(path.stat().st_mode)) == oct(0o666 & ~umask)
+
+
+@pytest.mark.parametrize("mode", ["a", "w+", "x", "rw", "foo"])
+def test_atomic_open_raises_on_bad_mode(tmp_path, mode):
+    with pytest.raises(ValueError, match="mode"):
+        with atomic_open(tmp_path / "file.txt", mode):
+            pass
+
+
+def test_create_temp(tmp_path):
+    assert sum(1 for _ in tmp_path.iterdir()) == 0
+    fd, filename = create_temp(prefix="a", suffix=".bin", dir=tmp_path)
+    assert sum(1 for _ in tmp_path.iterdir()) == 1
+    assert Path(filename).parent == tmp_path
+    os.write(fd, b"test\n")
+    os.close(fd)
+    assert Path(filename).read_bytes() == b"test\n"
+
+
+@pytest.mark.parametrize("mode", [0o666, 0o777])
+def test_create_temp_respects_umask(tmp_path, mode, umask):
+    _, filename = create_temp(dir=tmp_path, mode=mode)
+    assert oct(stat.S_IMODE(os.stat(filename).st_mode)) == oct(mode & ~umask)
+
+
 @pytest.mark.parametrize(
     "seq, expected",
     [