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.

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

Upgrade ruff, remove black #7233

Merged
merged 3 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions .githooks/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,9 @@ if [[ -n "$PY_FILES" ]]; then
if [[ ! -v VIRTUAL_ENV ]]; then
source .venv/bin/activate
fi
echo "Checking:"
echo "$PY_FILES"
# Run black against changed python files for this commit
black --check --diff ${PY_FILES//$'\n'/ }
# Run ruff (against all files, it's fast enough)
ruff check . --diff && echo "ruff passed!"
ruff format . && ruff check . --diff \
&& echo "ruff passed!"
else
exit 0
fi
Expand Down
20 changes: 7 additions & 13 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,6 @@ update-pip-requirements: update-admin-pip-requirements update-python3-requiremen
#
#################

.PHONY: check-black
check-black: ## Check Python source code formatting with black
@echo "███ Running black check..."
@black --check --diff .
@echo

.PHONY: black
black: ## Update Python source code formatting with black
@black securedrop .

.PHONY: ansible-config-lint
ansible-config-lint: ## Run custom Ansible linting tasks.
@echo "███ Linting Ansible configuration..."
Expand All @@ -94,15 +84,19 @@ app-lint-full: ## Test pylint compliance, with no checks disabled.
@echo

.PHONY: check-ruff
check-ruff: ## Lint Python source files.
check-ruff: ## Check linting and formatting of Python source files.
@echo "███ Running ruff..."
@ruff check . --show-source
@ruff format . --diff
@ruff check .
@echo

.PHONY: ruff
ruff: ## Update Python source file formatting.
@ruff format .
@ruff check . --fix

fix: ruff ## Apply automatic fixes.

# The --disable=names is required to use the BEM syntax
# # https://csswizardry.com/2013/01/mindbemding-getting-your-head-round-bem-syntax/
.PHONY: html-lint
Expand Down Expand Up @@ -139,7 +133,7 @@ yamllint: ## Lint YAML files (does not validate syntax!).
# While the order mostly doesn't matter here, keep "check-ruff" first, since it
# gives the broadest coverage and runs (and therefore fails) fastest.
.PHONY: lint
lint: check-ruff ansible-config-lint app-lint check-black html-lint shellcheck typelint yamllint check-strings check-supported-locales check-desktop-files ## Runs all lint checks
lint: check-ruff ansible-config-lint app-lint html-lint shellcheck typelint yamllint check-strings check-supported-locales check-desktop-files ## Runs all lint checks

.PHONY: safety
safety: ## Run `safety check` to check python dependencies for vulnerabilities.
Expand Down
10 changes: 3 additions & 7 deletions admin/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,7 @@ def is_missing_dependency() -> bool:
return True

# If any packages couldn't be found, it may point to an apt cache issue
if "Unable to locate package" in apt_query_result.stderr:
return True

return False
return "Unable to locate package" in apt_query_result.stderr

except subprocess.CalledProcessError as e:
sdlog.error("Error checking apt dependencies")
Expand Down Expand Up @@ -185,7 +182,6 @@ def envsetup(args: argparse.Namespace, virtualenv_dir: str = VENV_DIR) -> None:

# virtualenv doesnt exist? Install dependencies and create
if not os.path.exists(virtualenv_dir):

# Technically you can create a virtualenv from within python
# but pip can only be run over Tor on Tails, and debugging that
# along with instaling a third-party dependency is not worth
Expand Down Expand Up @@ -260,12 +256,12 @@ def install_pip_dependencies(
]

ansible_ver = subprocess.run(
maybe_torify() + ansible_vercheck_cmd, text=True, capture_output=True
maybe_torify() + ansible_vercheck_cmd, text=True, capture_output=True, check=False
)
if ansible_ver.stdout.startswith("2.9"):
sdlog.info("Ansible is out-of-date, removing it.")
delete_result = subprocess.run(
maybe_torify() + ansible_uninstall_cmd, capture_output=True, text=True
maybe_torify() + ansible_uninstall_cmd, capture_output=True, text=True, check=False
)
if delete_result.returncode != 0:
sdlog.error(
Expand Down
16 changes: 7 additions & 9 deletions admin/securedrop_admin/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
# Check OpenSSH version - ansible requires an extra argument for scp on OpenSSH 9
def openssh_version() -> int:
try:
result = subprocess.run(["ssh", "-V"], capture_output=True, text=True)
result = subprocess.run(["ssh", "-V"], capture_output=True, text=True, check=False)
if result.stderr.startswith("OpenSSH_9"):
return 9
elif result.stderr.startswith("OpenSSH_8"):
Expand All @@ -75,7 +75,6 @@ def openssh_version() -> int:
return 0
except subprocess.CalledProcessError:
return 0
pass
return 0


Expand Down Expand Up @@ -130,14 +129,14 @@ def validate(self, document: Document) -> bool:

class ValidateTime(Validator):
def validate(self, document: Document) -> bool:
if document.text.isdigit() and int(document.text) in range(0, 24):
if document.text.isdigit() and int(document.text) in range(24):
return True
raise ValidationError(message="Must be an integer between 0 and 23")

class ValidateUser(Validator):
def validate(self, document: Document) -> bool:
text = document.text
if text != "" and text != "root" and text != "amnesia":
if text not in ("", "root", "amnesia"):
return True
raise ValidationError(message="Must not be root, amnesia or an empty string")

Expand All @@ -160,8 +159,8 @@ def validate(self, document: Document) -> bool:
raise ValidationError(
message=(
"DNS server(s) should be a space/comma-separated list "
"of up to {} IP addresses"
).format(MAX_NAMESERVERS)
f"of up to {MAX_NAMESERVERS} IP addresses"
)
)
return True

Expand Down Expand Up @@ -194,7 +193,7 @@ def validate(self, document: Document) -> bool:
class ValidateYesNo(Validator):
def validate(self, document: Document) -> bool:
text = document.text.lower()
if text == "yes" or text == "no":
if text in ("yes", "no"):
return True
raise ValidationError(message="Must be either yes or no")

Expand Down Expand Up @@ -769,7 +768,6 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:

update_status, latest_tag = check_for_updates(cli_args)
if update_status is True:

# Useful for troubleshooting
branch_status = get_git_branch(cli_args)

Expand All @@ -793,7 +791,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
)
sdlog.error(
"If you are certain you want to proceed, run:\n\n\t"
"./securedrop-admin --force {}\n".format(cmd_name)
f"./securedrop-admin --force {cmd_name}\n"
)
sdlog.error("To apply the latest updates, run:\n\n\t" "./securedrop-admin update\n")
sdlog.error(
Expand Down
2 changes: 1 addition & 1 deletion admin/tests/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ def test_sdconfig_enable_https_disable_pow_on_source_interface():
"""


@pytest.fixture()
@pytest.fixture
def securedrop_git_repo(tmpdir):
cwd = os.getcwd()
os.chdir(str(tmpdir))
Expand Down
18 changes: 6 additions & 12 deletions admin/tests/test_securedrop-admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ def test_update_check_decorator_when_no_update_needed(self, caplog):
"securedrop_admin.check_for_updates", side_effect=[[False, "1.5.0"]]
) as mocked_check, mock.patch(
"securedrop_admin.get_git_branch", side_effect=["develop"]
), mock.patch(
"sys.exit"
) as mocked_exit:
), mock.patch("sys.exit") as mocked_exit:
# The decorator itself interprets --force
args = argparse.Namespace(force=False)
rv = securedrop_admin.update_check_required("update_check_test")(lambda _: 100)(args)
Expand All @@ -101,9 +99,7 @@ def test_update_check_decorator_when_update_needed(self, caplog):
"securedrop_admin.check_for_updates", side_effect=[[True, "1.5.0"]]
) as mocked_check, mock.patch(
"securedrop_admin.get_git_branch", side_effect=["bad_branch"]
), mock.patch(
"sys.exit"
) as mocked_exit:
), mock.patch("sys.exit") as mocked_exit:
# The decorator itself interprets --force
args = argparse.Namespace(force=False)
securedrop_admin.update_check_required("update_check_test")(lambda _: _)(args)
Expand All @@ -125,9 +121,7 @@ def test_update_check_decorator_when_skipped(self, caplog):
"securedrop_admin.check_for_updates", side_effect=[[True, "1.5.0"]]
) as mocked_check, mock.patch(
"securedrop_admin.get_git_branch", side_effect=["develop"]
), mock.patch(
"sys.exit"
) as mocked_exit:
), mock.patch("sys.exit") as mocked_exit:
# The decorator itself interprets --force
args = argparse.Namespace(force=True)
rv = securedrop_admin.update_check_required("update_check_test")(lambda _: 100)(args)
Expand Down Expand Up @@ -847,7 +841,7 @@ def verify_desc_consistency_optional(self, site_config, desc):
if callable(default):
default = default()
assert site_config.user_prompt_config_one(desc, None) == default
assert type(default) == etype
assert type(default) is etype

def verify_desc_consistency(self, site_config, desc):
self.verify_desc_consistency_optional(site_config, desc)
Expand Down Expand Up @@ -935,7 +929,7 @@ def verify_desc_consistency_allow_empty(self, site_config, desc):
(var, default, etype, prompt, validator, transform, condition) = desc
# verify the default passes validation
assert site_config.user_prompt_config_one(desc, None) == default
assert type(default) == etype
assert type(default) is etype

def verify_prompt_fingerprint(self, site_config, desc):
self.verify_prompt_not_empty(site_config, desc)
Expand All @@ -962,7 +956,7 @@ def verify_prompt_securedrop_supported_locales(self, site_config, desc):
(var, default, etype, prompt, validator, transform, condition) = desc
# verify the default passes validation
assert site_config.user_prompt_config_one(desc, None) == default
assert type(default) == etype
assert type(default) is etype
assert site_config.user_prompt_config_one(desc, "fr_FR en_US") == ["fr_FR", "en_US"]
assert site_config.user_prompt_config_one(desc, ["fr_FR", "en_US"]) == ["fr_FR", "en_US"]
assert site_config.user_prompt_config_one(desc, "") == []
Expand Down
13 changes: 7 additions & 6 deletions devops/scripts/verify-mo.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import shlex
import subprocess
from pathlib import Path
from typing import Any, Iterator, Optional, Set
from typing import Iterator, Set

import polib
from translate.tools.pocompile import convertmo
Expand Down Expand Up @@ -59,9 +59,9 @@ def __enter__(self) -> "CatalogVerifier":

def __exit__(
self,
exc_type: Optional[Any],
exc_value: Optional[Any],
traceback: Optional[Any],
exc_type: object,
exc_value: object,
traceback: object,
) -> None:
"""Clean up."""

Expand Down Expand Up @@ -113,11 +113,12 @@ def diffoscope_call(
# because we want to inherit the Python virtual environment
# in which we're invoked.
# nosemgrep: python.lang.security.audit.subprocess-shell-true.subprocess-shell-true
return subprocess.run(
return subprocess.run( # noqa: S602
cmd,
capture_output=True,
env=os.environ,
shell=True, # noqa: S602
shell=True,
check=False,
)

def reproduce(self) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,12 @@ def strset(s):
sys.exit(0)

print(
"The Tor configuration on the app server offers version {} services.".format(
strset(server_versions)
)
f"The Tor configuration on the app server offers version {strset(server_versions)} "
"services."
)

print(
"The Tor configuration in this backup offers version {} services.".format(
strset(backup_versions)
)
f"The Tor configuration in this backup offers version {strset(backup_versions)} services."
)

print("\nIncompatible configuration: Restoring a backup including a different ")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@
)

if "OK" in verify_proc:

# Updating the cert chain requires sudo privileges
os.setresgid(0, 0, -1)
os.setresuid(0, 0, -1)
Expand Down
6 changes: 1 addition & 5 deletions journalist_gui/journalist_gui/SecureDropUpdater.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,12 @@


def password_is_set():

pwd_flag = subprocess.check_output(["passwd", "--status"]).decode("utf-8").split()[1]

if pwd_flag == "NP":
return False
return True
return pwd_flag != "NP"


def prevent_second_instance(app: QtWidgets.QApplication, name: str) -> None:

# Null byte triggers abstract namespace
IDENTIFIER = "\0" + name
ALREADY_BOUND_ERRNO = 98
Expand Down
1 change: 0 additions & 1 deletion journalist_gui/test_gui.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ def test_default_tab(self):
assert self.window.tabWidget.currentIndex() == 0

def test_output_tab(self):

tab = self.window.tabWidget.tabBar()
QTest.mouseClick(tab, Qt.LeftButton)
assert self.window.tabWidget.currentIndex() == self.window.tabWidget.indexOf(
Expand Down
4 changes: 2 additions & 2 deletions molecule/testinfra/app-code/test_securedrop_app_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_unwanted_packages_absent(host, package):
assert not host.package(package).is_installed


@pytest.mark.skip_in_prod()
@pytest.mark.skip_in_prod
def test_securedrop_application_test_locale(host):
"""
Ensure both SecureDrop DEFAULT_LOCALE and SUPPORTED_LOCALES are present.
Expand All @@ -66,7 +66,7 @@ def test_securedrop_application_test_locale(host):
assert "\nSUPPORTED_LOCALES = ['el', 'ar', 'en_US']\n" in securedrop_config.content_string


@pytest.mark.skip_in_prod()
@pytest.mark.skip_in_prod
def test_securedrop_application_test_journalist_key(host):
"""
Ensure the SecureDrop Application GPG public key file is present.
Expand Down
10 changes: 3 additions & 7 deletions molecule/testinfra/app-code/test_securedrop_rqrequeue.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,9 @@ def test_securedrop_rqrequeue_service(host):
"Wants=redis-server.service",
"",
"[Service]",
'Environment=PYTHONPATH="{}:{}"'.format(
securedrop_test_vars.securedrop_code,
securedrop_test_vars.securedrop_venv_site_packages,
),
"ExecStart={}/python /var/www/securedrop/scripts/rqrequeue --interval 60".format(
securedrop_test_vars.securedrop_venv_bin
),
f'Environment=PYTHONPATH="{securedrop_test_vars.securedrop_code}:{securedrop_test_vars.securedrop_venv_site_packages}"',
f"ExecStart={securedrop_test_vars.securedrop_venv_bin}/python /var/www/securedrop/"
"scripts/rqrequeue --interval 60",
"PrivateDevices=yes",
"PrivateTmp=yes",
"ProtectSystem=full",
Expand Down
5 changes: 1 addition & 4 deletions molecule/testinfra/app-code/test_securedrop_rqworker.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ def test_securedrop_rqworker_service(host):
"Wants=redis-server.service",
"",
"[Service]",
'Environment=PYTHONPATH="{}:{}"'.format(
securedrop_test_vars.securedrop_code,
securedrop_test_vars.securedrop_venv_site_packages,
),
f'Environment=PYTHONPATH="{securedrop_test_vars.securedrop_code}:{securedrop_test_vars.securedrop_venv_site_packages}"',
f"ExecStart={securedrop_test_vars.securedrop_venv_bin}/rqworker -c rq_config",
"PrivateDevices=yes",
"PrivateTmp=yes",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,9 @@ def test_securedrop_shredder_service(host):
"Description=SecureDrop shredder",
"",
"[Service]",
'Environment=PYTHONPATH="{}:{}"'.format(
securedrop_test_vars.securedrop_code,
securedrop_test_vars.securedrop_venv_site_packages,
),
"ExecStart={}/python /var/www/securedrop/scripts/shredder --interval 60".format(
securedrop_test_vars.securedrop_venv_bin
),
f'Environment=PYTHONPATH="{securedrop_test_vars.securedrop_code}:{securedrop_test_vars.securedrop_venv_site_packages}"',
f"ExecStart={securedrop_test_vars.securedrop_venv_bin}/python /var/www/securedrop/"
"scripts/shredder --interval 60",
"PrivateDevices=yes",
"PrivateTmp=yes",
"ProtectSystem=full",
Expand Down
Loading