fix(server): don't return undefined SA NS (#13347)

Signed-off-by: Anton Gilgur <[email protected]>
argoproj · Jul 28, 2024 · 1239fd0 · 1239fd0
1 parent 2cb9118
commit 1239fd0
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 38 deletions.
diff --git a/server/auth/serviceaccount/claims.go b/server/auth/serviceaccount/claims.go
@@ -17,32 +17,46 @@ func ClaimSetFor(restConfig *rest.Config) (*types.Claims, error) {
 	username := restConfig.Username
 	if username != "" {
 		return &types.Claims{Claims: jwt.Claims{Subject: username}}, nil
-	} else if restConfig.BearerToken != "" || restConfig.BearerTokenFile != "" {
-		bearerToken := restConfig.BearerToken
-		if bearerToken == "" {
-			// should only ever be used for service accounts
-			data, err := os.ReadFile(restConfig.BearerTokenFile)
-			if err != nil {
-				return nil, fmt.Errorf("failed to read bearer token file: %w", err)
-			}
-			bearerToken = string(data)
-		}
-		parts := strings.SplitN(bearerToken, ".", 3)
-		if len(parts) != 3 {
-			return nil, fmt.Errorf("expected bearer token to be a JWT and therefore have 3 dot-delimited parts")
-		}
-		payload := parts[1]
-		data, err := base64.RawStdEncoding.DecodeString(payload)
-		if err != nil {
-			return nil, fmt.Errorf("failed to decode bearer token's JWT payload: %w", err)
-		}
-		claims := &types.Claims{}
-		err = json.Unmarshal(data, &claims)
+	}
+	if restConfig.BearerToken == "" && restConfig.BearerTokenFile == "" {
+		return nil, nil
+	}
+
+	bearerToken := restConfig.BearerToken
+	if bearerToken == "" {
+		// should only ever be used for service accounts
+		data, err := os.ReadFile(restConfig.BearerTokenFile)
 		if err != nil {
-			return nil, fmt.Errorf("failed to unmarshal bearer token's JWT payload: %w", err)
+			return nil, fmt.Errorf("failed to read bearer token file: %w", err)
 		}
+		bearerToken = string(data)
+	}
+
+	parts := strings.SplitN(bearerToken, ".", 3)
+	if len(parts) != 3 {
+		return nil, fmt.Errorf("expected bearer token to be a JWT and therefore have 3 dot-delimited parts")
+	}
+	payload := parts[1]
+	data, err := base64.RawStdEncoding.DecodeString(payload)
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode bearer token's JWT payload: %w", err)
+	}
+
+	claims := &types.Claims{}
+	err = json.Unmarshal(data, &claims)
+	if err != nil {
+		return nil, fmt.Errorf("failed to unmarshal bearer token's JWT payload: %w", err)
+	}
+
+	// attempt to derive SA name and namespace from Subject
+	// "system:serviceaccount:argo:jenkins" -> "argo", "jenkins"
+	// note that the SA name can have a colon in it, although the rest cannot
+	parts = strings.SplitN(claims.Subject, ":", 4)
+	if len(parts) < 4 {
 		return claims, nil
-	} else {
-		return nil, nil
 	}
+	claims.ServiceAccountNamespace = parts[2]
+	claims.ServiceAccountName = parts[3]
+
+	return claims, nil
 }
diff --git a/server/auth/serviceaccount/claims_test.go b/server/auth/serviceaccount/claims_test.go
@@ -5,12 +5,19 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/client-go/rest"
 )
 
-// sub = 1234567890
+const sub = "1234567890"
 const token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c" //nolint:gosec
 
+const sub2 = "system:serviceaccount:argo:jenkins"
+const saName2 = "jenkins"
+const saNs2 = "argo"
+const iss2 = "https://kubernetes.default.svc.cluster.local"
+const token2 = "eyJhbGciOiJSUzI1NiIsImtpZCI6Ijc5dVprMUl0VHZkTXFpLWc4dVQwVkV1Y05UZ21XXzJvZjNuZi1iZkpfVW8ifQ.eyJhdWQiOlsiaHR0cHM6Ly9rdWJlcm5ldGVzLmRlZmF1bHQuc3ZjLmNsdXN0ZXIubG9jYWwiLCJrM3MiXSwiZXhwIjoxNzIwNzYzNjgyLCJpYXQiOjE3MjA3NjAwODIsImlzcyI6Imh0dHBzOi8va3ViZXJuZXRlcy5kZWZhdWx0LnN2Yy5jbHVzdGVyLmxvY2FsIiwia3ViZXJuZXRlcy5pbyI6eyJuYW1lc3BhY2UiOiJhcmdvIiwic2VydmljZWFjY291bnQiOnsibmFtZSI6ImplbmtpbnMiLCJ1aWQiOiIyMGNjYzBjNS00NmNjLTQ3MjctYmUxMi1iZWY0ZTQ0ZTkxMjYifX0sIm5iZiI6MTcyMDc2MDA4Miwic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmFyZ286amVua2lucyJ9.k0SKX4yKga70R12ScI-mSpEWbwdRtlphxXB-drHNyxPyzz2fpur29ZzdcjE8-TlZE2fQrrV-MaqMBWqaD0TWsld0ILBYRYFOcIwuUOX8s611vqmpDejTT6oAroBEd4WSduP9WoxMsiN82EdDvDJeMpNpq8i-Nz8nYgfWe2VkqV_oYCenWKe9JC3QL1TOxdkqer2qgflGnIpTzSVf7y47vmdlqsS9GPbdXyCg4MdcyDnLgY2VoVQnymD22uTG3Ugp3dO3zhaS-puHMPDOa-_EbAn2MsqETrA8H8iKal-z4sx-R6MxN8fdBrrIkWZLZZD4i_EhBS5paFTNcXXq0-T90w" //nolint:gosec
+
 func TestClaimSetFor(t *testing.T) {
 	t.Run("Empty", func(t *testing.T) {
 		claims, err := ClaimSetFor(&rest.Config{})
@@ -19,10 +26,11 @@ func TestClaimSetFor(t *testing.T) {
 		}
 	})
 	t.Run("Basic", func(t *testing.T) {
-		claims, err := ClaimSetFor(&rest.Config{Username: "my-username"})
+		const username = "my-username"
+		claims, err := ClaimSetFor(&rest.Config{Username: username})
 		if assert.NoError(t, err) {
 			assert.Empty(t, claims.Issuer)
-			assert.Equal(t, "my-username", claims.Subject)
+			assert.Equal(t, username, claims.Subject)
 		}
 	})
 	t.Run("BadBearerToken", func(t *testing.T) {
@@ -33,7 +41,7 @@ func TestClaimSetFor(t *testing.T) {
 		claims, err := ClaimSetFor(&rest.Config{BearerToken: token})
 		if assert.NoError(t, err) {
 			assert.Empty(t, claims.Issuer)
-			assert.Equal(t, "1234567890", claims.Subject)
+			assert.Equal(t, sub, claims.Subject)
 		}
 	})
 
@@ -48,7 +56,16 @@ func TestClaimSetFor(t *testing.T) {
 		claims, err := ClaimSetFor(&rest.Config{BearerTokenFile: tmp.Name()})
 		if assert.NoError(t, err) {
 			assert.Empty(t, claims.Issuer)
-			assert.Equal(t, "1234567890", claims.Subject)
+			assert.Equal(t, sub, claims.Subject)
 		}
 	})
+
+	t.Run("BearerToken with SA details", func(t *testing.T) {
+		claims, err := ClaimSetFor(&rest.Config{BearerToken: token2})
+		require.NoError(t, err)
+		assert.Equal(t, iss2, claims.Issuer)
+		assert.Equal(t, sub2, claims.Subject)
+		assert.Equal(t, saName2, claims.ServiceAccountName)
+		assert.Equal(t, saNs2, claims.ServiceAccountNamespace)
+	})
 }
diff --git a/ui/src/app/app-router.tsx b/ui/src/app/app-router.tsx
@@ -82,7 +82,7 @@ export function AppRouter({popupManager, history, notificationsManager}: {popupM
             .catch(setError);
     }, []);
 
-    const namespaceSuffix = Utils.managedNamespace ? '' : '/' + namespace;
+    const namespaceSuffix = Utils.managedNamespace ? '' : '/' + (namespace || '');
     return (
         <>
             {popupProps && <Popup {...popupProps} />}

diff --git a/ui/src/app/shared/utils.ts b/ui/src/app/shared/utils.ts
@@ -84,9 +84,10 @@ export const Utils = {
 
     fixLocalStorageString(x: string): string {
         // empty string is valid, so we cannot use `truthy`
-        if (x !== null && x !== 'null' && x !== 'undefined') {
-            return x;
+        if (x == null || x == 'null' || x == 'undefined') {
+            return undefined; // explicitly return undefined
         }
+        return x;
     },
 
     // TODO: some of these utils should probably be moved to context
@@ -105,12 +106,7 @@ export const Utils = {
     },
 
     get currentNamespace() {
-        // we always prefer the managed namespace
-        if (localStorage.getItem(currentNamespaceKey) === null) {
-            return this.userNamespace || this.managedNamespace;
-        } else {
-            return this.fixLocalStorageString(localStorage.getItem(currentNamespaceKey));
-        }
+        return this.fixLocalStorageString(localStorage.getItem(currentNamespaceKey)) ?? (this.userNamespace || this.managedNamespace);
     },
 
     // return a namespace, favoring managed namespace when set