Address issues raised in review:

- Rename some vars - Cleanup some comments - Tiny refactor to improve readability Signed-off-by: Oded Ben-Ozer <[email protected]>
Oded-B · Oct 9, 2023 · 6875b64 · 6875b64
1 parent 7f0056c
commit 6875b64
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 17 deletions.
diff --git 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
@@ -290,7 +290,7 @@ func TestHandleCallback(t *testing.T) {
 			},
 		},
 		{
-			name:               "concatinateClaim",
+			name:               "newGroupFromClaims",
 			userIDKey:          "", // not configured
 			userNameKey:        "", // not configured
 			expectUserID:       "subvalue",