From 6d519c73430092eddb074af146ccde7436d5a5e9 Mon Sep 17 00:00:00 2001
From: Dao Thanh Tung <ttdao.2015@accountancy.smu.edu.sg>
Date: Sat, 3 Dec 2022 01:19:52 +0800
Subject: [PATCH] Fixing CLI ACL token processing unexpected precedence
 (#15274)

* Fixing CLI ACL token processing unexpected precedence

* Minor flow format and add Changelog

* Fixed failed tests and improve error logging message

* Add unit test cases and minor changes from code review

* Unset env var once the test case finishes running

* remove label FINISH
---
 .changelog/15274.txt |  3 +++
 api/agent_test.go    | 58 ++++++++++++++++++++++++++++++++++++++++++++
 api/api.go           | 25 +++++++++++++++----
 3 files changed, 81 insertions(+), 5 deletions(-)
 create mode 100644 .changelog/15274.txt

diff --git a/.changelog/15274.txt b/.changelog/15274.txt
new file mode 100644
index 000000000..689f65774
--- /dev/null
+++ b/.changelog/15274.txt
@@ -0,0 +1,3 @@
+```release-note: bug
+cli: fix ACL token processing unexpected precedence
+```
\ No newline at end of file
diff --git a/api/agent_test.go b/api/agent_test.go
index 43e50fd7f..0dfc72683 100644
--- a/api/agent_test.go
+++ b/api/agent_test.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/stretchr/testify/assert"
 	"net/http"
 	"net/http/httptest"
 	"net/http/httputil"
@@ -263,6 +264,63 @@ func TestAgent_ServiceRegisterOpts_WithContextTimeout(t *testing.T) {
 	require.True(t, errors.Is(err, context.DeadlineExceeded), "expected timeout")
 }
 
+func TestAPI_NewClient_TokenFileCLIFirstPriority(t *testing.T) {
+	os.Setenv("CONSUL_HTTP_TOKEN_FILE", "httpTokenFile.txt")
+	os.Setenv("CONSUL_HTTP_TOKEN", "httpToken")
+	nonExistentTokenFile := "randomTokenFile.txt"
+	config := Config{
+		Token:     "randomToken",
+		TokenFile: nonExistentTokenFile,
+	}
+
+	_, err := NewClient(&config)
+	errorMessage := fmt.Sprintf("Error loading token file %s : open %s: no such file or directory", nonExistentTokenFile, nonExistentTokenFile)
+	assert.EqualError(t, err, errorMessage)
+	os.Unsetenv("CONSUL_HTTP_TOKEN_FILE")
+	os.Unsetenv("CONSUL_HTTP_TOKEN")
+}
+
+func TestAPI_NewClient_TokenCLISecondPriority(t *testing.T) {
+	os.Setenv("CONSUL_HTTP_TOKEN_FILE", "httpTokenFile.txt")
+	os.Setenv("CONSUL_HTTP_TOKEN", "httpToken")
+	tokenString := "randomToken"
+	config := Config{
+		Token: tokenString,
+	}
+
+	c, err := NewClient(&config)
+	if err != nil {
+		t.Fatalf("Error Initializing new client: %v", err)
+	}
+	assert.Equal(t, c.config.Token, tokenString)
+	os.Unsetenv("CONSUL_HTTP_TOKEN_FILE")
+	os.Unsetenv("CONSUL_HTTP_TOKEN")
+}
+
+func TestAPI_NewClient_HttpTokenFileEnvVarThirdPriority(t *testing.T) {
+	nonExistentTokenFileEnvVar := "httpTokenFile.txt"
+	os.Setenv("CONSUL_HTTP_TOKEN_FILE", nonExistentTokenFileEnvVar)
+	os.Setenv("CONSUL_HTTP_TOKEN", "httpToken")
+
+	_, err := NewClient(DefaultConfig())
+	errorMessage := fmt.Sprintf("Error loading token file %s : open %s: no such file or directory", nonExistentTokenFileEnvVar, nonExistentTokenFileEnvVar)
+	assert.EqualError(t, err, errorMessage)
+	os.Unsetenv("CONSUL_HTTP_TOKEN_FILE")
+	os.Unsetenv("CONSUL_HTTP_TOKEN")
+}
+
+func TestAPI_NewClient_TokenEnvVarFinalPriority(t *testing.T) {
+	httpTokenEnvVar := "httpToken"
+	os.Setenv("CONSUL_HTTP_TOKEN", httpTokenEnvVar)
+
+	c, err := NewClient(DefaultConfig())
+	if err != nil {
+		t.Fatalf("Error Initializing new client: %v", err)
+	}
+	assert.Equal(t, c.config.Token, httpTokenEnvVar)
+	os.Unsetenv("CONSUL_HTTP_TOKEN")
+}
+
 func TestAPI_AgentServices(t *testing.T) {
 	t.Parallel()
 	c, s := makeClient(t)
diff --git a/api/api.go b/api/api.go
index 87d61a6f1..7a17ddee8 100644
--- a/api/api.go
+++ b/api/api.go
@@ -750,20 +750,35 @@ func NewClient(config *Config) (*Client, error) {
 	// If the TokenFile is set, always use that, even if a Token is configured.
 	// This is because when TokenFile is set it is read into the Token field.
 	// We want any derived clients to have to re-read the token file.
-	if config.TokenFile != "" {
+	// The precedence of ACL token should be:
+	// 1. -token-file cli option
+	// 2. -token cli option
+	// 3. CONSUL_HTTP_TOKEN_FILE environment variable
+	// 4. CONSUL_HTTP_TOKEN environment variable
+	if config.TokenFile != "" && config.TokenFile != defConfig.TokenFile {
 		data, err := os.ReadFile(config.TokenFile)
 		if err != nil {
-			return nil, fmt.Errorf("Error loading token file: %s", err)
+			return nil, fmt.Errorf("Error loading token file %s : %s", config.TokenFile, err)
 		}
 
 		if token := strings.TrimSpace(string(data)); token != "" {
 			config.Token = token
 		}
-	}
-	if config.Token == "" {
+	} else if config.Token != "" && defConfig.Token != config.Token {
+		// Fall through
+	} else if defConfig.TokenFile != "" {
+		data, err := os.ReadFile(defConfig.TokenFile)
+		if err != nil {
+			return nil, fmt.Errorf("Error loading token file %s : %s", defConfig.TokenFile, err)
+		}
+
+		if token := strings.TrimSpace(string(data)); token != "" {
+			config.Token = token
+			config.TokenFile = defConfig.TokenFile
+		}
+	} else {
 		config.Token = defConfig.Token
 	}
-
 	return &Client{config: *config, headers: make(http.Header)}, nil
 }