-
Notifications
You must be signed in to change notification settings - Fork 13
fix: create DB user with correct host scope for TCP connections to MariaDB #13
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| # Benches (created by `bench new`) | ||
| benches/ | ||
|
|
||
| # Research | ||
| research-folder/ | ||
|
|
||
| # Python | ||
| __pycache__/ | ||
| *.py[cod] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,43 @@ def _apt_package(self) -> str: | |
| return f"mariadb-server-{self.config.version}" | ||
| return "mariadb-server" | ||
|
|
||
| def _grant_host(self) -> str: | ||
| """Return the MySQL grant host for CREATE USER / GRANT statements. | ||
|
|
||
| Use '%' when connecting over TCP so that the site DB user can reach | ||
| MariaDB from 127.0.0.1 (TCP loopback). Fall back to 'localhost' | ||
| only when a unix socket is detected — in MySQL's privilege tables | ||
| 'localhost' matches socket connections exclusively. | ||
| """ | ||
| return "localhost" if self._detect_socket() else "%" | ||
|
|
||
| def create_user(self, username: str, password: str, db_name: str) -> None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bench new-site is managed by frappe. how is this managed in the old bench?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old bench never managed this at all — it always delegated to Frappe completely, and users had to work around the @'localhost' issue manually (e.g., by creating sites locally then dumping/restoring to remote DBs, as seen in frappe/bench#1513). The correct fix is to use Frappe's built-in --mariadb-user-host-login-scope='%' option when calling frappe new-site for TCP connections, rather than doing post-processing. I'll update the PR to: Remove the implicit create_user() post-processing from site.create()
|
||
| """Create the DB user and grant all privileges with the correct host scope. | ||
|
|
||
| Frappe's new-site always creates the user as @'localhost', which | ||
| breaks TCP connections (127.0.0.1 != localhost in privilege tables). | ||
| This method uses the grant host derived from _grant_host() so TCP | ||
| connections work when no unix socket is available. | ||
| """ | ||
| grant_host = self._grant_host() | ||
| sql = ( | ||
| f"CREATE USER IF NOT EXISTS '{username}'@'{grant_host}'" | ||
| f" IDENTIFIED BY '{password}';" | ||
| f"GRANT ALL PRIVILEGES ON `{db_name}`.* TO '{username}'@'{grant_host}';" | ||
| "FLUSH PRIVILEGES;" | ||
| ) | ||
| mysql_bin = shutil.which("mariadb") or shutil.which("mysql") or "mysql" | ||
| cmd = [mysql_bin, f"-u{self.config.admin_user}"] | ||
| if self.config.root_password: | ||
| cmd.append(f"-p{self.config.root_password}") | ||
| socket_path = self._detect_socket() | ||
| if socket_path: | ||
| cmd.append(f"--socket={socket_path}") | ||
| else: | ||
| cmd += [f"-h{self.config.host}", f"-P{self.config.port}"] | ||
| cmd += ["-e", sql] | ||
| run_command(cmd) | ||
|
|
||
| def _detect_socket(self) -> str: | ||
| if self.config.socket_path: | ||
| return self.config.socket_path | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| """Tests for MariaDBManager.create_user() and _grant_host().""" | ||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, call, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from bench_cli.config.bench_config import BenchConfig | ||
| from bench_cli.config.mariadb_config import MariaDBConfig | ||
| from bench_cli.config.site_config import SiteConfig | ||
| from bench_cli.core.bench import Bench | ||
| from bench_cli.core.site import Site | ||
| from bench_cli.managers.mariadb_manager import MariaDBManager | ||
|
|
||
|
|
||
| def make_manager(host: str = "localhost", port: int = 3306, root_password: str = "secret") -> MariaDBManager: | ||
| return MariaDBManager(MariaDBConfig(host=host, port=port, root_password=root_password)) | ||
|
|
||
|
|
||
| # ── _grant_host() ───────────────────────────────────────────────────────────── | ||
|
|
||
|
|
||
| def test_grant_host_returns_percent_when_no_socket() -> None: | ||
| manager = make_manager() | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| assert manager._grant_host() == "%" | ||
|
|
||
|
|
||
| def test_grant_host_returns_localhost_when_socket_detected() -> None: | ||
| manager = make_manager() | ||
| with patch.object(manager, "_detect_socket", return_value="/tmp/mysql.sock"): | ||
| assert manager._grant_host() == "localhost" | ||
|
|
||
|
|
||
| # ── create_user() ───────────────────────────────────────────────────────────── | ||
|
|
||
|
|
||
| def test_create_user_tcp_uses_percent_host() -> None: | ||
| manager = make_manager(host="127.0.0.1", root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| with patch("shutil.which", side_effect=lambda b: "/usr/bin/mysql" if b == "mysql" else None): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert any("'mydb'@'%'" in part for part in called_cmd) | ||
|
|
||
|
|
||
| def test_create_user_socket_uses_localhost_host() -> None: | ||
| manager = make_manager(root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value="/tmp/mysql.sock"): | ||
| with patch("shutil.which", side_effect=lambda b: "/usr/bin/mysql" if b == "mysql" else None): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert any("'mydb'@'localhost'" in part for part in called_cmd) | ||
|
|
||
|
|
||
| def test_create_user_prefers_mariadb_binary() -> None: | ||
| manager = make_manager(root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| with patch("shutil.which", side_effect=lambda b: "/usr/bin/mariadb" if b == "mariadb" else None): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert called_cmd[0] == "/usr/bin/mariadb" | ||
|
|
||
|
|
||
| def test_create_user_falls_back_to_mysql_binary() -> None: | ||
| manager = make_manager(root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| with patch("shutil.which", side_effect=lambda b: "/usr/bin/mysql" if b == "mysql" else None): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert called_cmd[0] == "/usr/bin/mysql" | ||
|
|
||
|
|
||
| def test_create_user_tcp_passes_host_and_port() -> None: | ||
| manager = make_manager(host="127.0.0.1", port=3307, root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| with patch("shutil.which", return_value="/usr/bin/mysql"): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert "-h127.0.0.1" in called_cmd | ||
| assert "-P3307" in called_cmd | ||
|
|
||
|
|
||
| def test_create_user_socket_passes_socket_path() -> None: | ||
| manager = make_manager(root_password="root") | ||
| with patch.object(manager, "_detect_socket", return_value="/tmp/mysql.sock"): | ||
| with patch("shutil.which", return_value="/usr/bin/mysql"): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert "--socket=/tmp/mysql.sock" in called_cmd | ||
|
|
||
|
|
||
| def test_create_user_includes_root_password() -> None: | ||
| manager = make_manager(root_password="s3cr3t") | ||
| with patch.object(manager, "_detect_socket", return_value=""): | ||
| with patch("shutil.which", return_value="/usr/bin/mysql"): | ||
| with patch("bench_cli.managers.mariadb_manager.run_command") as mock_run: | ||
| manager.create_user("mydb", "mypass", "mydb") | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert "-ps3cr3t" in called_cmd | ||
|
|
||
|
|
||
| # ── site.create() with --mariadb-user-host-login-scope ────────────────────── | ||
|
|
||
|
|
||
| def make_bench(tmp_path: Path) -> Bench: | ||
| from bench_cli.config.app_config import AppConfig | ||
| from bench_cli.config.redis_config import RedisConfig | ||
| from bench_cli.config.worker_config import WorkerConfig | ||
| config = BenchConfig( | ||
| name="test-bench", | ||
| python_version="3.14", | ||
| apps=[AppConfig(name="frappe", repo="https://github.com/frappe/frappe", branch="version-16")], | ||
| mariadb=MariaDBConfig(host="127.0.0.1", port=3306, root_password="root"), | ||
| redis=RedisConfig(cache_port=13000, queue_port=11000, socketio_port=12000), | ||
| workers=__import__("bench_cli.config.worker_config", fromlist=["WorkerConfig"]).WorkerConfig(default_count=1, short_count=1, long_count=1), | ||
| ) | ||
| return Bench(config, tmp_path) | ||
|
|
||
|
|
||
| def test_site_create_passes_host_scope_for_tcp(tmp_path: Path) -> None: | ||
| bench = make_bench(tmp_path) | ||
| bench.create_directories() | ||
| site = Site(SiteConfig(name="site1.localhost", apps=["frappe"], admin_password="admin"), bench) | ||
|
|
||
| with patch("bench_cli.core.site.run_command") as mock_run: | ||
| with patch("bench_cli.managers.mariadb_manager.MariaDBManager._detect_socket", return_value=""): | ||
| site.create() | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert "--mariadb-user-host-login-scope" in called_cmd | ||
| assert "%" in called_cmd | ||
|
|
||
|
|
||
| def test_site_create_skips_host_scope_for_socket(tmp_path: Path) -> None: | ||
| bench = make_bench(tmp_path) | ||
| bench.create_directories() | ||
| site = Site(SiteConfig(name="site1.localhost", apps=["frappe"], admin_password="admin"), bench) | ||
|
|
||
| with patch("bench_cli.core.site.run_command") as mock_run: | ||
| with patch("bench_cli.managers.mariadb_manager.MariaDBManager._detect_socket", return_value="/tmp/mysql.sock"): | ||
| site.create() | ||
| called_cmd = mock_run.call_args[0][0] | ||
| assert "--mariadb-user-host-login-scope" not in called_cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why implicit?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right to question this i did some more digging. After reviewing Frappe's new-site code, I realized the current approach is unnecessarily implicit.
Frappe already has a --mariadb-user-host-login-scope option specifically for this purpose:
@click.option( "--mariadb-user-host-login-scope", help=( "Set the mariadb host for the user login scope if you don't want to use the current host as login " "scope which typically is ''@'localhost' - may be used when initializing a user on a remote host." ), )The post-processing approach with create_user() was implicit because I was trying to work around what I thought was a Frappe limitation, but Frappe already provides the right mechanism.