From a0b8d7521d4f7b934ef73e4d0d56df6b1b72d59e Mon Sep 17 00:00:00 2001
From: Tom Wieczorek <twieczorek@mirantis.com>
Date: Wed, 10 Apr 2024 08:05:38 +0200
Subject: [PATCH] Use -1 as UID return value in case of errors

The zero value for a UID happens to be root's UID. Assuming root may
or may not be a good default, depending on the use case. Setting file
ownership to root is a restrictive and safe default, running programs as
root is not. By returning "UnknownUID" as a constant to -1, callers must
explicitly decide on the fallback instead of accidentally assuming root.

Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
---
 internal/pkg/users/users.go      | 27 ++++++++++++++++++++-------
 internal/pkg/users/users_test.go |  2 +-
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/internal/pkg/users/users.go b/internal/pkg/users/users.go
index 5d2fdba49c4e..05a5ec24c874 100644
--- a/internal/pkg/users/users.go
+++ b/internal/pkg/users/users.go
@@ -25,15 +25,28 @@ import (
 	"strconv"
 )
 
-const RootUID = 0 // User ID of the root user
+const (
+	// An unknown (i.e. invalid) user ID. It's distinct from a UID's zero value,
+	// which happens to be [RootUID]. Assuming root may or may not be a good
+	// default, depending on the use case. Setting file ownership to root is a
+	// restrictive and safe default, running programs as root is not. Therefore,
+	// this is the preferred return value for UIDs in case of error; callers
+	// must then explicitly decide on the fallback instead of accidentally
+	// assuming root.
+	UnknownUID = -1
 
-// LookupUID returns uid of given username and logs a warning if its missing
+	RootUID = 0 // User ID of the root user
+)
+
+// Lookup looks up a user's UID by username. If the user cannot be found, the
+// returned error is of type [user.UnknownUserError]. If an error is returned,
+// the returned UID will be [UnknownUID].
 func LookupUID(name string) (int, error) {
 	var uid string
 
 	if entry, err := user.Lookup(name); err != nil {
 		if !errors.Is(err, user.UnknownUserError(name)) {
-			return 0, err
+			return UnknownUID, err
 		}
 
 		// fallback to call external `id` in case NSS is used
@@ -42,9 +55,9 @@ func LookupUID(name string) (int, error) {
 			var exitErr *exec.ExitError
 			if errors.As(idErr, &exitErr) {
 				firstLine, _, _ := bytes.Cut(exitErr.Stderr, []byte{'\n'})
-				return 0, fmt.Errorf("%w (%w: %s)", err, idErr, string(firstLine))
+				return UnknownUID, fmt.Errorf("%w (%w: %s)", err, idErr, string(firstLine))
 			}
-			return 0, fmt.Errorf("%w (%w)", err, idErr)
+			return UnknownUID, fmt.Errorf("%w (%w)", err, idErr)
 		}
 
 		uid = string(bytes.TrimSuffix(out, []byte{'\n'}))
@@ -54,10 +67,10 @@ func LookupUID(name string) (int, error) {
 
 	parsedUID, err := strconv.Atoi(uid)
 	if err != nil {
-		return 0, fmt.Errorf("UID %q is not a decimal integer: %w", uid, err)
+		return UnknownUID, fmt.Errorf("UID %q is not a decimal integer: %w", uid, err)
 	}
 	if parsedUID < 0 {
-		return 0, fmt.Errorf("UID is negative: %d", parsedUID)
+		return UnknownUID, fmt.Errorf("UID is negative: %d", parsedUID)
 	}
 
 	return parsedUID, nil
diff --git a/internal/pkg/users/users_test.go b/internal/pkg/users/users_test.go
index 316ab412cd8d..29a730e017f8 100644
--- a/internal/pkg/users/users_test.go
+++ b/internal/pkg/users/users_test.go
@@ -40,6 +40,6 @@ func TestGetUID(t *testing.T) {
 		assert.ErrorIs(t, err, user.UnknownUserError("some-non-existing-user"))
 		var exitErr *exec.ExitError
 		assert.ErrorAs(t, err, &exitErr, "expected external `id` to return an error")
-		assert.Zero(t, uid)
+		assert.Equal(t, UnknownUID, uid)
 	}
 }