Address issues raised in review:

- Add missing json tag.
- Control delimiter cleaning with a configuration key.
- Use better variable names
- concatenate string using slice and join

Signed-off-by: Oded Ben-Ozer <obenozer@wayfair.com>

connector/oidc/oidc.go

@@ -91,7 +91,7 @@ type Config struct {
 	// ClaimModifications holds all claim modifications options, current has only newGroupsFromClaims
 	ClaimModifications struct {
 		NewGroupsFromClaims []NewGroupsFromClaims `json:"newGroupsFromClaims"`
-	}
+	} `json:"claimModifications"`
 }
 
 // List of groups claim elements to create by concatenating other claims
@@ -102,6 +102,10 @@ type NewGroupsFromClaims struct {
 	// String to separate the claims
 	Delimiter string `json:"delimiter"`
 
+	// Should Dex remove the Delimiter string from claim values
+	// This is done to keep resulting claim structure in full control of the Dex operator
+	ClearDelimiter bool `json:"clearDelimiter"`
+
 	// String to place before the first claim
 	Prefix string `json:"prefix"`
 }
@@ -446,23 +450,26 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
 		}
 	}
 
-	for _, cc := range c.newGroupsFromClaims {
-		newElement := ""
-		for _, clm := range cc.ClaimList {
+	for _, newGroupsElementConfig := range c.newGroupsFromClaims {
+		newGroupsElement := []string{
+			newGroupsElementConfig.Prefix,
+		}
+		for _, claimName := range newGroupsElementConfig.ClaimList {
 			// Non string claim value are ignored, concatenating them doesn't really make any sense
-			if claimValue, ok := claims[clm].(string); ok {
-				// Removing the delimiier string from the concatenated claim to ensure resulting claim structure
-				// is in full control of Dex operator
-				claimCleanedValue := strings.ReplaceAll(claimValue, cc.Delimiter, "")
-				if newElement == "" {
-					newElement = cc.Prefix + cc.Delimiter + claimCleanedValue
+			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 {
-					newElement = newElement + cc.Delimiter + claimCleanedValue
+					newGroupsElement = append(newGroupsElement, claimValue)
 				}
 			}
 		}
-		if newElement != "" {
-			groups = append(groups, newElement)
+		if len(newGroupsElement) > 1 {
+			groups = append(groups, strings.Join(newGroupsElement, newGroupsElementConfig.Delimiter))
 		}
 	}
 

connector/oidc/oidc_test.go

@@ -295,7 +295,7 @@ func TestHandleCallback(t *testing.T) {
 			userNameKey:        "", // not configured
 			expectUserID:       "subvalue",
 			expectUserName:     "namevalue",
-			expectGroups:       []string{"group1", "gh::acme::pipeline-one", "tfe-acme-foobar", "bk-emailvalue"},
+			expectGroups:       []string{"group1", "gh::acme::pipeline-one", "clr_delim-acme-foobar", "keep_delim-acme-foo-bar", "bk-emailvalue"},
 			expectedEmailField: "emailvalue",
 			newGroupsFromClaims: []NewGroupsFromClaims{
 				{ // The basic functionality, should create "gh::acme::pipeline-one".
@@ -316,13 +316,24 @@ func TestHandleCallback(t *testing.T) {
 				},
 				{ // In this case the delimiter character("-") should be removed removed from "claim-with-delimiter" claim to ensure the resulting
 					// claim structure is in full control of the Dex operator and not the person creating a new pipeline.
-					// Should create "tfe-acme-foobar" and not "tfe-acme-foo-bar".
+					// Should create "clr_delim-acme-foobar" and not "tfe-acme-foo-bar".
+					ClaimList: []string{
+						"organization",
+						"claim-with-delimiter",
+					},
+					Delimiter:      "-",
+					ClearDelimiter: true,
+					Prefix:         "clr_delim",
+				},
+				{ // In this case the delimiter character("-") should be NOT removed from "claim-with-delimiter" claim.
+					// Should create  "keep_delim-acme-foo-bar".
 					ClaimList: []string{
 						"organization",
 						"claim-with-delimiter",
 					},
 					Delimiter: "-",
-					Prefix:    "tfe",
+					// ClearDelimiter: false,
+					Prefix: "keep_delim",
 				},
 				{ // Ignore non string claims (like arrays), this should result in "bk-emailvalue".
 					ClaimList: []string{