From 6875b64cafd26047b4abb89a95748b997e6ae37c Mon Sep 17 00:00:00 2001
From: Oded Ben-Ozer <obenozer@wayfair.com>
Date: Mon, 9 Oct 2023 12:30:21 +0200
Subject: [PATCH] Address issues raised in review: - Rename some vars - Cleanup
 some comments - Tiny refactor to improve readability

Signed-off-by: Oded Ben-Ozer <obenozer@wayfair.com>
---
 connector/oidc/oidc.go      | 33 +++++++++++++++++----------------
 connector/oidc/oidc_test.go |  2 +-
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 2b29284272..5b43d2e0ae 100644
--- a/connector/oidc/oidc.go
+++ b/connector/oidc/oidc.go
@@ -88,7 +88,7 @@ type Config struct {
 		GroupsKey string `json:"groups"` // defaults to "groups"
 	} `json:"claimMapping"`
 
-	// ClaimModifications holds all claim modifications options, current has only newGroupsFromClaims
+	// ClaimModifications holds all claim modifications options
 	ClaimModifications struct {
 		NewGroupsFromClaims []NewGroupsFromClaims `json:"newGroupsFromClaims"`
 	} `json:"claimModifications"`
@@ -450,25 +450,26 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		}
 	}
 
-	for _, newGroupsElementConfig := range c.newGroupsFromClaims {
-		newGroupsElement := []string{
-			newGroupsElementConfig.Prefix,
+	for _, config := range c.newGroupsFromClaims {
+		newGroupSegments := []string{
+			config.Prefix,
 		}
-		for _, claimName := range newGroupsElementConfig.ClaimList {
+		for _, claimName := range config.ClaimList {
+			claimValue, ok := claims[claimName].(string)
 			// Non string claim value are ignored, concatenating them doesn't really make any sense
-			if claimValue, ok := claims[claimName].(string); ok {
-				if newGroupsElementConfig.ClearDelimiter {
-					// Removing the delimiier string from the concatenated claim to ensure resulting claim structure
-					// is in full control of Dex operator
-					claimCleanedValue := strings.ReplaceAll(claimValue, newGroupsElementConfig.Delimiter, "")
-					newGroupsElement = append(newGroupsElement, claimCleanedValue)
-				} else {
-					newGroupsElement = append(newGroupsElement, claimValue)
-				}
+			if !ok {
+				continue
 			}
+			if config.ClearDelimiter {
+				// Removing the delimiter string from the concatenated claim to ensure resulting claim structure
+				// is in full control of Dex operator
+				claimValue = strings.ReplaceAll(claimValue, config.Delimiter, "")
+			}
+			newGroupSegments = append(newGroupSegments, claimValue)
 		}
-		if len(newGroupsElement) > 1 {
-			groups = append(groups, strings.Join(newGroupsElement, newGroupsElementConfig.Delimiter))
+
+		if len(newGroupSegments) > 1 {
+			groups = append(groups, strings.Join(newGroupSegments, config.Delimiter))
 		}
 	}
 
diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go
index 2de6cf0bba..13c71ab917 100644
--- a/connector/oidc/oidc_test.go
+++ b/connector/oidc/oidc_test.go
@@ -290,7 +290,7 @@ func TestHandleCallback(t *testing.T) {
 			},
 		},
 		{
-			name:               "concatinateClaim",
+			name:               "newGroupFromClaims",
 			userIDKey:          "", // not configured
 			userNameKey:        "", // not configured
 			expectUserID:       "subvalue",