From b1f4bd019533e0d65a6f196114805f56678f9961 Mon Sep 17 00:00:00 2001
From: Oded Ben-Ozer <obenozer@wayfair.com>
Date: Fri, 25 Aug 2023 13:57:34 +0200
Subject: [PATCH] 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 | 33 ++++++++++++++++++++-------------
connector/oidc/oidc_test.go | 17 ++++++++++++++---
2 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go
index 9ffb1dee..30b759ce 100644
--- a/connector/oidc/oidc.go
+++ b/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))
}
}
diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go
index d1b7ee1c..2de6cf0b 100644
--- a/connector/oidc/oidc_test.go
+++ b/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{
--
GitLab