From 87331e7c9717329c478c81dd51c1673b6ef25e65 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Mon, 11 Aug 2025 14:10:20 -0700 Subject: [PATCH] gitweb: Don't use privileged action feature to run as different user - Instead implement running specific commands inside the privileged action as a specific user. Tests: - Gitweb functional tests and unit tests work. - Running various operations such as clone, create, set branch, rename, etc. all result in repositories (and all their contents) owned by www-data:www-data. Signed-off-by: Sunil Mohan Adapa Reviewed-by: Joseph Nuthalapati --- plinth/conftest.py | 13 ++++++++++++ plinth/modules/gitweb/__init__.py | 7 +++---- plinth/modules/gitweb/forms.py | 3 +-- plinth/modules/gitweb/privileged.py | 20 ++++++++++--------- .../modules/gitweb/tests/test_privileged.py | 6 +++--- 5 files changed, 31 insertions(+), 18 deletions(-) diff --git a/plinth/conftest.py b/plinth/conftest.py index fecb13b8f..52d32e839 100644 --- a/plinth/conftest.py +++ b/plinth/conftest.py @@ -6,6 +6,7 @@ pytest configuration for all tests. import importlib import os import pathlib +import subprocess from unittest.mock import patch import pytest @@ -191,6 +192,18 @@ def fixture_mock_privileged(request): module.__dict__[name] = wrapper +@pytest.fixture(name='mock_run_as_user') +def fixture_mock_run_as_user(): + """A fixture to override action_utils.run_as_user.""" + + def _bypass_runuser(*args, username, **kwargs): + return subprocess.run(*args, **kwargs) + + with patch('plinth.action_utils.run_as_user') as mock: + mock.side_effect = _bypass_runuser + yield + + @pytest.fixture(name='splinter_screenshot_dir', scope='session') def fixture_splinter_screenshot_dir(request): """Set default screenshot directory to ./screenshots. diff --git a/plinth/modules/gitweb/__init__.py b/plinth/modules/gitweb/__init__.py index 56933c3e6..3dcd79829 100644 --- a/plinth/modules/gitweb/__init__.py +++ b/plinth/modules/gitweb/__init__.py @@ -14,7 +14,7 @@ from plinth.package import Packages from . import manifest, privileged from .forms import is_repo_url -from .manifest import GIT_REPO_PATH, REPO_DIR_OWNER +from .manifest import GIT_REPO_PATH _description = [ _('Git is a distributed version-control system for tracking changes in ' @@ -217,7 +217,7 @@ def get_repo_list(): def repo_info(repo): """Get information about repository.""" - info = privileged.repo_info(repo, _run_as_user=REPO_DIR_OWNER) + info = privileged.repo_info(repo) if info['access'] == 'private': info['is_private'] = True else: @@ -248,5 +248,4 @@ def edit_repo(form_initial, form_cleaned): privileged.set_repo_access(repo, 'public') if form_cleaned['default_branch'] != form_initial['default_branch']: - privileged.set_default_branch(repo, form_cleaned['default_branch'], - _run_as_user=REPO_DIR_OWNER) + privileged.set_default_branch(repo, form_cleaned['default_branch']) diff --git a/plinth/modules/gitweb/forms.py b/plinth/modules/gitweb/forms.py index b47ef9e5a..76ba8a314 100644 --- a/plinth/modules/gitweb/forms.py +++ b/plinth/modules/gitweb/forms.py @@ -14,12 +14,11 @@ from django.utils.translation import gettext_lazy as _ from plinth.modules import gitweb from . import privileged -from .manifest import REPO_DIR_OWNER def _get_branches(repo): """Get all the branches in the repository.""" - branch_data = privileged.get_branches(repo, _run_as_user=REPO_DIR_OWNER) + branch_data = privileged.get_branches(repo) default_branch = branch_data['default_branch'] branches = branch_data['branches'] diff --git a/plinth/modules/gitweb/privileged.py b/plinth/modules/gitweb/privileged.py index 2dc79b008..25e50d73b 100644 --- a/plinth/modules/gitweb/privileged.py +++ b/plinth/modules/gitweb/privileged.py @@ -199,9 +199,11 @@ def _get_default_branch(repo): """Get default branch of the repository.""" repo_path = GIT_REPO_PATH / repo - return subprocess.check_output( + return action_utils.run_as_user( ['git', '-C', - str(repo_path), 'symbolic-ref', '--short', 'HEAD']).decode().strip() + str(repo_path), 'symbolic-ref', '--short', 'HEAD'], + username=REPO_DIR_OWNER, check=True, + stdout=subprocess.PIPE).stdout.decode().strip() def _get_repo_description(repo): @@ -267,11 +269,12 @@ def _set_access_status(repo, status): def _get_branches(repo): """Return list of the branches in the repository.""" - output = subprocess.check_output( + process = action_utils.run_as_user( ['git', '-C', repo, 'branch', '--format=%(refname:short)'], - cwd=GIT_REPO_PATH) + cwd=GIT_REPO_PATH, username=REPO_DIR_OWNER, check=True, + stdout=subprocess.PIPE) - return output.decode().strip().split() + return process.stdout.decode().strip().split() @privileged @@ -299,10 +302,9 @@ def set_default_branch(name: str, branch: str): if branch not in _get_branches(repo): raise ValueError('No such branch') - subprocess.check_call([ - 'git', '-C', repo, 'symbolic-ref', 'HEAD', - "refs/heads/{}".format(branch) - ], cwd=GIT_REPO_PATH) + action_utils.run_as_user( + ['git', '-C', repo, 'symbolic-ref', 'HEAD', f'refs/heads/{branch}'], + cwd=GIT_REPO_PATH, check=True, username=REPO_DIR_OWNER) @privileged diff --git a/plinth/modules/gitweb/tests/test_privileged.py b/plinth/modules/gitweb/tests/test_privileged.py index 36b6c0f6e..f9ddf6d12 100644 --- a/plinth/modules/gitweb/tests/test_privileged.py +++ b/plinth/modules/gitweb/tests/test_privileged.py @@ -16,16 +16,16 @@ REPO_DATA = { 'access': 'private', } -pytestmark = pytest.mark.usefixtures('mock_privileged') +pytestmark = pytest.mark.usefixtures('mock_privileged', 'mock_run_as_user') privileged_modules_to_mock = ['plinth.modules.gitweb.privileged'] git_installed = pytest.mark.skipif(not pathlib.Path('/usr/bin/git').exists(), reason='git is not installed') @pytest.fixture(autouse=True) -def fixture_set_repo_path(tmpdir): +def fixture_set_repo_path(tmp_path): """Set a repository path in the actions module.""" - privileged.GIT_REPO_PATH = tmpdir + privileged.GIT_REPO_PATH = tmp_path @pytest.fixture(name='existing_repo')