From 11ff483e7e18cd069a38fe51cf98cafd6232ad89 Mon Sep 17 00:00:00 2001
From: Ihor Kalnytskyi <ihor@kalnytskyi.com>
Date: Sat, 6 Jan 2024 12:55:42 +0200
Subject: [PATCH] Unset PG* env vars except PGSERVICEFILE

Unfortunately, the Windows runner has some PostgreSQL environment
variables set, which are neither set for Linux or macOS runners. This is
may be especially confusing since variables such as PGUSER or PGPASSWORD
may point to a user that doesn't exist.

Since one of the design decisions made previously is to restrain this
action from pointing to any user by default, we better unset these
environment variables to avoid confusion.

In addition to that change, let's also mention in the README file that
it's up to users to set connection parameters whatever way they prefer.

Reported-by: bkoelman
Fixes: #17
---
 README.md      | 16 ++++++++++++++--
 action.yml     |  7 +++++++
 test_action.py | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index 856259579..276eda50f 100644
--- a/README.md
+++ b/README.md
@@ -14,6 +14,19 @@ key features:
 
 ## Usage
 
+> [!IMPORTANT]
+>
+> In order to connect to a PostgreSQL server, use either connection parameters
+> from the table below ([link](#connection-parameters)), or retrieve a
+> connection URI from the `connection-uri` output ([link](#advanced)).
+
+> [!TIP]
+>
+> `libpq`-using applications may choose to set the `PGSERVICE=postgres`
+> environment variable instead ([link](#create-a-new-user-w-database-via-cli)),
+> where `postgres` is the service name extracted from the `service-name`
+> output.
+
 #### Connection parameters
 
 | Key      | Value                                               |
@@ -73,11 +86,10 @@ steps:
       createuser myuser
       createdb --owner myuser mydatabase
       psql -c "ALTER USER myuser WITH PASSWORD 'mypassword'"
-
     env:
       # This activates connection parameters for the superuser created by
       # the action in the step above. It's mandatory to set this before using
-      # createuser/psql/etc tools.
+      # createuser/psql and other libpq-using applications.
       #
       # The service name is the same as the username (i.e. 'postgres') but
       # it's recommended to use action's output to get the name in order to
diff --git a/action.yml b/action.yml
index 3ca099cea..456b2cd9b 100644
--- a/action.yml
+++ b/action.yml
@@ -38,6 +38,13 @@ runs:
         elif [ "$RUNNER_OS" == "Windows" ]; then
           echo "$PGBIN" >> $GITHUB_PATH
           echo "PQ_LIB_DIR=$PGROOT\lib" >> $GITHUB_ENV
+
+          # The Windows runner has some PostgreSQL environment variables set
+          # that may confuse users since they may be irrelevant to the
+          # PostgreSQL server we're using.
+          for name in "PGROOT" "PGDATA" "PGBIN" "PGUSER" "PGPASSWORD"; do
+            echo "$name=" >> $GITHUB_ENV
+          done
         elif [ "$RUNNER_OS" == "macOS" ]; then
           case "$(sw_vers -productVersion)" in
             13.*)
diff --git a/test_action.py b/test_action.py
index ebf955135..a1f4540ea 100644
--- a/test_action.py
+++ b/test_action.py
@@ -1,5 +1,6 @@
 import locale
 import os
+import pathlib
 import subprocess
 import typing as t
 
@@ -84,6 +85,30 @@ def test_locale(connection: psycopg.Connection):
     assert locale.normalize(lc_ctype) == "en_US.UTF-8"
 
 
+def test_environment_variables():
+    """Test that only expected 'PG*' variables are set."""
+
+    pg_environ = {k: v for k, v in os.environ.items() if k.startswith("PG")}
+
+    # In case of Windows, there might be a mix of forward and backward slashes
+    # as separators. So let's compare paths semantically instead.
+    pg_servicefile = pathlib.Path(pg_environ.pop("PGSERVICEFILE", ""))
+    pg_servicefile_exp = pathlib.Path(os.environ["RUNNER_TEMP"], "pgdata", "pg_service.conf")
+    assert pg_servicefile.resolve() == pg_servicefile_exp.resolve()
+    
+    if os.name == "nt":
+        pg_environ_exp = {
+            "PGBIN": "",
+            "PGDATA": "",
+            "PGPASSWORD": "",
+            "PGROOT": "",
+            "PGUSER": "",
+        }
+    else:
+        pg_environ_exp = {}
+    assert pg_environ == pg_environ_exp
+
+
 def test_user_permissions(connection: psycopg.Connection):
     """Test that a user has super/createdb permissions."""
 
@@ -201,6 +226,19 @@ def test_client_applications(
         subprocess.check_call(["dropuser", username])
 
 
+def test_libpq_applications(service_name: str, monkeypatch: pytest.MonkeyPatch):
+    """Test that libpq-using applications can be used."""
+
+    # Request connection parameters from the connection service file prepared
+    # by our action.
+    monkeypatch.setenv("PGSERVICE", service_name)
+
+    with psycopg.connect() as connection:
+        assert connection \
+            .execute("SELECT usename FROM pg_user WHERE usename = CURRENT_USER") \
+            .fetchone()
+
+
 def test_auth_wrong_username(connection_factory: ConnectionFactory, connection_uri: str):
     """Test that wrong username is rejected!"""