Hopefully fix some bugs.

README.md

@@ -313,7 +313,9 @@ and sign out at `https://<any-app>/_google-auth/logout`.
 - **Websockets** work — upgrade headers are forwarded exactly like dokku's
   stock nginx template.
 - **Plugin updates:** `sudo dokku plugin:update google-auth` rebuilds the
-  service image and restarts the container automatically.
+  service image and recreates the container with freshly written settings.
+  `dokku google-auth:report` names the running service's state; if it ever says
+  it is ignoring per-app lists, `dokku google-auth:restart` recreates it.
 - **An app's own lists need its nginx config to be current**, since that is
   what tells the service which app a request belongs to. The plugin rewrites
   the config whenever you change an app's lists (and on every deploy), so this

functions

@@ -209,7 +209,7 @@ fn-ga-apply-list-change() {
     dokku_log_verbose "auth service is not running; changes apply when it starts"
     return 0
   fi
-  if ! fn-ga-service-has-app-mount; then
+  if ! fn-ga-service-reads-app-lists; then
     dokku_log_info1 "recreating the auth service so it can read per-app lists"
     fn-ga-service-start
     return 0
@@ -615,12 +615,19 @@ fn-ga-service-running() {
   [[ "$(docker container inspect -f '{{.State.Running}}' "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" == "true" ]]
 }
 
-# True when the running container has the per-app config mount. A container
+# True when the running container can actually read per-app lists. That takes
-# started by an older version of this plugin will not, and would silently
+# both halves: the bind mount, and the environment variable pointing at it.
-# ignore per-app lists.
+# Either one alone makes the service ignore every per-app list and fall back to
-fn-ga-service-has-app-mount() {
+# the global one — silently, and in the permissive direction — so both are
-  docker container inspect -f '{{range .Mounts}}{{println .Destination}}{{end}}' \
+# checked. A container started by an older version of this plugin has neither;
-    "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null | grep -qxF "$GOOGLE_AUTH_APP_CONFIG_MOUNT"
+# one recreated from a stale service.env has the mount without the variable.
+fn-ga-service-reads-app-lists() {
+  local inspected
+  inspected="$(docker container inspect \
+    -f '{{range .Mounts}}mount={{println .Destination}}{{end}}{{range .Config.Env}}env={{println .}}{{end}}' \
+    "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" || return 1
+  grep -qxF "mount=$GOOGLE_AUTH_APP_CONFIG_MOUNT" <<<"$inspected" || return 1
+  grep -qxF "env=GOOGLE_AUTH_APP_CONFIG_DIR=$GOOGLE_AUTH_APP_CONFIG_MOUNT" <<<"$inspected"
 }
 
 fn-ga-write-env-file() {

install

@@ -8,30 +8,36 @@ DOKKU_LIB_ROOT=${DOKKU_LIB_ROOT:-/var/lib/dokku}
 DATA_ROOT="$DOKKU_LIB_ROOT/data/google-auth"
 
 mkdir -p "$DATA_ROOT/global" "$DATA_ROOT/apps"
+
+if command -v docker >/dev/null 2>&1; then
+  echo "-----> Building dokku-google-auth service image (first build can take a few minutes)"
+  docker image build -t dokku-google-auth:latest "$PLUGIN_DIR"
+  # Pick up the new image if the service is already running. This goes through
+  # fn-ga-service-start rather than its own `docker run` so an upgrade always
+  # rewrites service.env first: recreating the container from the file an older
+  # version left behind gives it stale settings — most damagingly a missing
+  # GOOGLE_AUTH_APP_CONFIG_DIR, which makes the service ignore every per-app
+  # access list and fall back to the global one.
+  if [[ "$(docker container inspect -f '{{.State.Running}}' dokku-google-auth 2>/dev/null)" == "true" ]]; then
+    echo "-----> Restarting google-auth service with the new image"
+    # shellcheck disable=SC1091
+    source "$PLUGIN_DIR/functions"
+    if fn-ga-configured; then
+      fn-ga-service-start
+    else
+      echo " !     google-auth is not fully configured; the running service keeps the old image" 1>&2
+      echo " !     run 'dokku google-auth:configure' then 'dokku google-auth:restart'" 1>&2
+    fi
+  fi
+else
+  echo " !     docker not found; the google-auth service image was not built" 1>&2
+fi
+
+# Ownership and modes last: the steps above can rewrite service.env as root,
+# and the dokku user has to be able to rewrite it from then on.
 chown -R dokku:dokku "$DATA_ROOT" 2>/dev/null || true
 chmod 700 "$DATA_ROOT"
 # apps/ is bind-mounted into the service container, which runs as an
 # unprivileged uid and must be able to traverse it to read per-app access
 # lists. The 0700 on DATA_ROOT still keeps other host users out.
 chmod 711 "$DATA_ROOT/apps"
-
-if command -v docker >/dev/null 2>&1; then
-  echo "-----> Building dokku-google-auth service image (first build can take a few minutes)"
-  docker image build -t dokku-google-auth:latest "$PLUGIN_DIR"
-  # Pick up the new image if the service is already running.
-  if [[ "$(docker container inspect -f '{{.State.Running}}' dokku-google-auth 2>/dev/null)" == "true" ]]; then
-    echo "-----> Restarting google-auth service with the new image"
-    port="$(head -n1 "$DATA_ROOT/global/port" 2>/dev/null || true)"
-    port="${port:-2999}"
-    docker container rm -f dokku-google-auth >/dev/null 2>&1 || true
-    docker container run -d \
-      --name dokku-google-auth \
-      --restart=unless-stopped \
-      -p "127.0.0.1:${port}:2999" \
-      --env-file "$DATA_ROOT/service.env" \
-      -v "$DATA_ROOT/apps:/data/apps:ro" \
-      dokku-google-auth:latest >/dev/null
-  fi
-else
-  echo " !     docker not found; the google-auth service image was not built" 1>&2
-fi

internal/authproxy/appconfig.go

@@ -1,6 +1,9 @@
 package authproxy
 
 import (
+	"errors"
+	"io/fs"
+	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -14,6 +17,13 @@ type accessLists struct {
 	AllowedDomains []string
 	AllowedEmails  []string
 	DeniedEmails   []string
+
+	// Unreadable records that one of the app's list files exists but could not
+	// be read, leaving its real rules unknown. Falling back to the global list
+	// would then silently widen access — the app is likely narrower than global,
+	// which is why it has a list at all — so an app in this state denies
+	// everyone until the file is readable again.
+	Unreadable bool
 }
 
 func (a accessLists) hasAllowRules() bool {
@@ -64,26 +74,40 @@ func (s *appStore) lists(app string) accessLists {
 		return hit.lists
 	}
 
-	lists := accessLists{
+	var lists accessLists
-		AllowedDomains: readDomainFile(filepath.Join(s.dir, app, "allowed-domains")),
+	read := func(name string) []string {
-		AllowedEmails:  readListFile(filepath.Join(s.dir, app, "allowed-emails")),
+		entries, err := readListFile(filepath.Join(s.dir, app, name))
-		DeniedEmails:   readListFile(filepath.Join(s.dir, app, "denied-emails")),
+		if err != nil {
+			log.Printf("appconfig: cannot read %s for app %q: %v — denying every account "+
+				"for that app until it is readable", name, app, err)
+			lists.Unreadable = true
+		}
+		return entries
 	}
+	lists.AllowedDomains = trimAts(read("allowed-domains"))
+	lists.AllowedEmails = read("allowed-emails")
+	lists.DeniedEmails = read("denied-emails")
+
 	s.cache[app] = cachedLists{lists: lists, loadedAt: s.now()}
 	return lists
 }
 
-func readListFile(path string) []string {
+// readListFile returns the entries in a list file. A missing file is normal —
-	// A missing or unreadable file is normal: most apps have no overrides.
+// most apps have no overrides — and reads as an empty list; any other error is
+// reported, because it means the app's rules are unknown rather than absent.
+func readListFile(path string) ([]string, error) {
 	raw, err := os.ReadFile(path)
 	if err != nil {
-		return nil
+		if errors.Is(err, fs.ErrNotExist) {
+			return nil, nil
+		}
+		return nil, err
 	}
-	return splitList(string(raw))
+	return splitList(string(raw)), nil
 }
 
-func readDomainFile(path string) []string {
+// trimAts accepts domains written either as "signal.org" or "@signal.org".
-	entries := readListFile(path)
+func trimAts(entries []string) []string {
 	for i, d := range entries {
 		entries[i] = strings.TrimPrefix(d, "@")
 	}

internal/authproxy/appconfig_test.go

@@ -53,6 +53,37 @@ func TestAppStoreReadsLists(t *testing.T) {
 	}
 }
 
+// A list file the service cannot read leaves the app's real rules unknown.
+// Inheriting the global list there would hand out access the app's own list was
+// written to withhold, so the app denies instead.
+func TestUnreadableListDeniesInsteadOfInheriting(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root reads files regardless of mode")
+	}
+	dir := t.TempDir()
+	writeAppLists(t, dir, "locked", map[string]string{"allowed-emails": "ceo@signal.org\n"})
+	if err := os.Chmod(filepath.Join(dir, "locked", "allowed-emails"), 0o000); err != nil {
+		t.Fatal(err)
+	}
+
+	s := newTestServer(t, "http://unused.invalid") // global list allows all of signal.org
+	s.cfg.AppConfigDir = dir
+	s.apps = newAppStore(dir)
+
+	if !s.apps.lists("locked").Unreadable {
+		t.Error("an unreadable list file should mark the app's lists unreadable")
+	}
+	for _, email := range []string{"ceo@signal.org", "anyone@signal.org"} {
+		if s.emailAllowedFor("locked", email) {
+			t.Errorf("emailAllowedFor(locked, %q) = true; an app whose list cannot be read must deny", email)
+		}
+	}
+	// Only that app is affected: everyone else still uses the global list.
+	if !s.emailAllowedFor("inherits", "anyone@signal.org") {
+		t.Error("an app with no config of its own should still inherit the global list")
+	}
+}
+
 // The app name reaches the service in a header, so it must never be able to
 // walk out of the config directory.
 func TestAppStoreRejectsUnusableNames(t *testing.T) {

internal/authproxy/server.go

@@ -313,6 +313,7 @@ func (s *Server) effectiveLists(app string) accessLists {
 		AllowedDomains: s.cfg.AllowedDomains,
 		AllowedEmails:  s.cfg.AllowedEmails,
 		DeniedEmails:   s.cfg.DeniedEmails,
+		Unreadable:     own.Unreadable,
 	}
 	if own.hasAllowRules() {
 		out.AllowedDomains = own.AllowedDomains
@@ -326,10 +327,16 @@ func (s *Server) effectiveLists(app string) accessLists {
 
 // emailAllowedFor applies app's rules: the deny list first (it always wins),
 // then the allow list, where an address must match an allowed email or an
-// allowed domain. An empty app means "global rules only".
+// allowed domain. An empty app means "global rules only"; an app whose own
+// lists could not be read is denied outright rather than quietly handed the
+// broader global list.
 func (s *Server) emailAllowedFor(app, email string) bool {
 	email = strings.ToLower(email)
 	lists := s.effectiveLists(app)
+	if lists.Unreadable {
+		// The app has rules we could not read; see accessLists.Unreadable.
+		return false
+	}
 	if slices.Contains(lists.DeniedEmails, email) {
 		return false
 	}

subcommands/report

@@ -94,10 +94,15 @@ cmd-google-auth-report() {
   dokku_log_verbose "Denied emails:   $(fn-ga-global-get-list denied-emails | paste -sd' ' -) (applies to every app)"
   dokku_log_verbose "Session TTL:     $(fn-ga-global-get session-ttl 24h)"
   dokku_log_verbose "Service port:    127.0.0.1:$(fn-ga-global-get port "$GOOGLE_AUTH_DEFAULT_PORT")"
-  if fn-ga-service-running; then
+  if ! fn-ga-service-running; then
+    dokku_log_verbose "Service:         not running"
+  elif fn-ga-service-reads-app-lists; then
     dokku_log_verbose "Service:         running"
   else
-    dokku_log_verbose "Service:         not running"
+    # Worth calling out: every per-app list below is being ignored, and the
+    # apps that have one are falling back to the broader global list.
+    dokku_log_verbose "Service:         running, but IGNORING all per-app lists"
+    dokku_log_verbose "                 (started without the per-app config; fix with: dokku google-auth:restart)"
   fi
 
   local app

test/access-list-test.sh

@@ -215,6 +215,41 @@ grep -q 'proxy_set_header X-Google-Auth-App "my-app";' "$APP_CONF" ||
   fail "changing an app's list should rewrite its nginx config to stamp the app name"
 echo "ok: per-app list change refreshes the nginx config"
 
+# --- a running service only counts if it can really read per-app lists ---
+# It takes both the bind mount and the env var naming it. A container recreated
+# from a service.env written before per-app lists existed has the mount but not
+# the variable, ignores every per-app list, and still looks healthy — so the
+# check must not be satisfied by the mount alone.
+mkdir -p "$WORK/bin-docker"
+cat >"$WORK/bin-docker/docker" <<'EOF'
+#!/bin/sh
+# Stands in for `docker container inspect`, replaying a canned inspection.
+if [ "$1" = "container" ] && [ "$2" = "inspect" ]; then
+  cat "$DOCKER_INSPECT_FIXTURE"
+  exit 0
+fi
+exit 0
+EOF
+chmod +x "$WORK/bin-docker/docker"
+
+# Runs the check against one canned inspection, in a subshell so the stub and
+# its fixture do not leak into the rest of the file.
+reads_app_lists() (
+  export DOCKER_INSPECT_FIXTURE="$WORK/inspect-fixture"
+  printf '%s\n' "$@" >"$DOCKER_INSPECT_FIXTURE"
+  PATH="$WORK/bin-docker:$PATH"
+  fn-ga-service-reads-app-lists
+)
+
+reads_app_lists "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" ||
+  fail "a container with both the mount and the env var should read per-app lists"
+reads_app_lists "mount=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" &&
+  fail "a container with the mount but no GOOGLE_AUTH_APP_CONFIG_DIR ignores per-app lists"
+reads_app_lists "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" &&
+  fail "a container with the env var but no mount has nothing to read"
+reads_app_lists "" && fail "a container with neither should not count"
+echo "ok: per-app list readiness check"
+
 # --- lifecycle triggers carry per-app lists ---
 "$ROOT/post-app-rename" my-app renamed-app
 [[ ! -d "$DATA/apps/my-app" ]] || fail "rename should move the app's directory"