🌱 Logging a warning if readGitHubTokens finds several values which cl…

…ash. (ossf#4483)

* Logging a warning if readGitHubTokens finds several values which clash.

Signed-off-by: Simon Heidrich <[email protected]>

* Using log.Printf instead of fmt.Printf.

Signed-off-by: Simon Heidrich <[email protected]>

* Removing obsolete t.Helper calls.

Signed-off-by: Simon Heidrich <[email protected]>

* Unsetting local env vars in tests.

Signed-off-by: Simon Heidrich <[email protected]>

* Using string.Builder to generate warning message, as the linter considered the line too long.

Signed-off-by: Simon Heidrich <[email protected]>

---------

Signed-off-by: Simon Heidrich <[email protected]>

clients/githubrepo/roundtripper/tokens/accessor.go

@@ -16,6 +16,7 @@
 package tokens
 
 import (
+	"log"
 	"os"
 	"strings"
 )
@@ -33,13 +34,33 @@ type TokenAccessor interface {
 	Release(uint64)
 }
 
+var logDuplicateTokenWarning = func(firstName string, clashingName string) {
+	var stringBuilder strings.Builder
+	stringBuilder.WriteString("Warning: PATs stored in env variables ")
+	stringBuilder.WriteString(firstName)
+	stringBuilder.WriteString(" and ")
+	stringBuilder.WriteString(clashingName)
+	stringBuilder.WriteString(" differ. Scorecard will use the former.")
+	log.Println(stringBuilder.String())
+}
+
 func readGitHubTokens() (string, bool) {
+	var firstName, firstToken string
 	for _, name := range githubAuthTokenEnvVars {
 		if token, exists := os.LookupEnv(name); exists && token != "" {
-			return token, exists
+			if firstName == "" {
+				firstName = name
+				firstToken = token
+			} else if token != firstToken {
+				logDuplicateTokenWarning(firstName, name)
+			}
 		}
 	}
-	return "", false
+	if firstName == "" {
+		return "", false
+	} else {
+		return firstToken, true
+	}
 }
 
 // MakeTokenAccessor is a factory function of TokenAccessor.

clients/githubrepo/roundtripper/tokens/accessor_test.go

@@ -16,11 +16,12 @@ package tokens
 import (
 	"net/http/httptest"
 	"net/rpc"
+	"os"
 	"strings"
 	"testing"
 )
 
-//nolint:paralleltest // test uses t.Setenv
+//nolint:paralleltest // test uses t.Setenv indirectly
 func TestMakeTokenAccessor(t *testing.T) {
 	tests := []struct {
 		name           string
@@ -40,10 +41,7 @@ func TestMakeTokenAccessor(t *testing.T) {
 			useServer: true,
 		},
 	}
-	// clear all env variables devs may have defined, or the test will fail locally
-	for _, envVar := range githubAuthTokenEnvVars {
-		t.Setenv(envVar, "")
-	}
+	unsetTokens(t)
 	t.Setenv(githubAuthServer, "")
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -91,3 +89,93 @@ func testServer(t *testing.T) {
 		t.Errorf("MakeTokenAccessor() = nil, want not nil")
 	}
 }
+
+func TestClashingTokensDisplayWarning(t *testing.T) {
+	unsetTokens(t)
+
+	someToken := "test_token"
+	otherToken := "clashing_token"
+	t.Setenv("GITHUB_AUTH_TOKEN", someToken)
+	t.Setenv("GITHUB_TOKEN", otherToken)
+
+	warningCalled := false
+	originalLogWarning := logDuplicateTokenWarning
+	logDuplicateTokenWarning = func(firstName string, clashingName string) {
+		warningCalled = true
+	}
+	defer func() { logDuplicateTokenWarning = originalLogWarning }()
+
+	token, exists := readGitHubTokens()
+
+	if token != someToken {
+		t.Errorf("Received wrong token")
+	}
+	if !exists {
+		t.Errorf("Token is expected to exist")
+	}
+	if !warningCalled {
+		t.Errorf("Expected logWarning to be called for clashing tokens, but it was not.")
+	}
+}
+
+func TestConsistentTokensDoNotDisplayWarning(t *testing.T) {
+	unsetTokens(t)
+
+	someToken := "test_token"
+	t.Setenv("GITHUB_AUTH_TOKEN", someToken)
+	t.Setenv("GITHUB_TOKEN", someToken)
+
+	warningCalled := false
+	originalLogWarning := logDuplicateTokenWarning
+	logDuplicateTokenWarning = func(firstName string, clashingName string) {
+		warningCalled = true
+	}
+	defer func() { logDuplicateTokenWarning = originalLogWarning }()
+
+	token, exists := readGitHubTokens()
+
+	if token != someToken {
+		t.Errorf("Received wrong token")
+	}
+	if !exists {
+		t.Errorf("Token is expected to exist")
+	}
+	if warningCalled {
+		t.Errorf("Expected logWarning to not have been called for consistent tokens, but it was.")
+	}
+}
+
+//nolint:paralleltest // test uses t.Setenv indirectly
+func TestNoTokensDoNoDisplayWarning(t *testing.T) {
+	unsetTokens(t)
+
+	warningCalled := false
+	originalLogWarning := logDuplicateTokenWarning
+	logDuplicateTokenWarning = func(firstName string, clashingName string) {
+		warningCalled = true
+	}
+	defer func() { logDuplicateTokenWarning = originalLogWarning }()
+
+	token, exists := readGitHubTokens()
+
+	if token != "" {
+		t.Errorf("Scorecard found a token somewhere")
+	}
+	if exists {
+		t.Errorf("Token is not expected to exist")
+	}
+	if warningCalled {
+		t.Errorf("Expected logWarning to not have been called for no set tokens, but it was not.")
+	}
+}
+
+// temporarily unset all of the github token env vars,
+// as tests may otherwise fail depending on the local environment.
+func unsetTokens(t *testing.T) {
+	t.Helper()
+	for _, name := range githubAuthTokenEnvVars {
+		// equivalent to t.Unsetenv (which does not exist)
+		t.Setenv(name, "")
+		os.Unsetenv(name)
+	}
+}