From 91cd939287a1b25e10c6d5f47afd2a596d1fde44 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 6 Jul 2023 20:10:01 +0200 Subject: [PATCH 01/14] build(deps): bump golang.org/x/crypto from 0.10.0 to 0.11.0 (#3035) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.10.0 to 0.11.0. - [Commits](https://github.com/golang/crypto/compare/v0.10.0...v0.11.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Oded Ben-Ozer From 7b29cfaeb1b5fc377360aa8744885cd56e79b9fc Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 12 Jul 2023 11:33:21 +0200 Subject: [PATCH 02/14] build(deps): bump helm/kind-action from 1.7.0 to 1.8.0 (#3041) Bumps [helm/kind-action](https://github.com/helm/kind-action) from 1.7.0 to 1.8.0. - [Release notes](https://github.com/helm/kind-action/releases) - [Commits](https://github.com/helm/kind-action/compare/v1.7.0...v1.8.0) --- updated-dependencies: - dependency-name: helm/kind-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Oded Ben-Ozer --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 42400e4d41..0b9713773b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -82,7 +82,7 @@ jobs: run: docker-compose -f docker-compose.test.yaml up -d - name: Create kind cluster - uses: helm/kind-action@v1.7.0 + uses: helm/kind-action@v1.8.0 with: version: "v0.17.0" node_image: "kindest/node:v1.25.3@sha256:cd248d1438192f7814fbca8fede13cfe5b9918746dfa12583976158a834fd5c5" From 6c00fe05800c517d205b68ab7059b7df54c21626 Mon Sep 17 00:00:00 2001 From: Cedric-Magnan <43343135+Cedric-Magnan@users.noreply.github.com> Date: Wed, 18 May 2022 15:31:51 +0200 Subject: [PATCH 03/14] Update oauth2.go Signed-off-by: Cedric-Magnan Signed-off-by: Oded Ben-Ozer --- server/oauth2.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/oauth2.go b/server/oauth2.go index cfae540528..a82d172ff0 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -148,6 +148,10 @@ const ( responseTypeCode = "code" // "Regular" flow responseTypeToken = "token" // Implicit flow for frontend apps. responseTypeIDToken = "id_token" // ID Token in url fragment + responseTypeCodeToken = "code token" // "Regular" flow + Implicit flow + responseTypeCodeIDToken = "code id_token" // "Regular" flow + ID Token + responseTypeIDTokenToken = "id_token token" // ID Token + Implicit flow + responseTypeCodeIDTokenToken = "code id_token token" // "Regular" flow + ID Token + Implicit flow ) const ( From a72413dd47b373983c039146f35cc2c9fa8ef93f Mon Sep 17 00:00:00 2001 From: Cedric-Magnan <43343135+Cedric-Magnan@users.noreply.github.com> Date: Wed, 18 May 2022 15:38:36 +0200 Subject: [PATCH 04/14] Update server.go Signed-off-by: Cedric-Magnan Signed-off-by: Oded Ben-Ozer --- server/server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/server.go b/server/server.go index 444fb7e15a..bf83dd81f0 100644 --- a/server/server.go +++ b/server/server.go @@ -225,9 +225,9 @@ func newServer(ctx context.Context, c Config, rotationStrategy rotationStrategy) for _, respType := range c.SupportedResponseTypes { switch respType { - case responseTypeCode, responseTypeIDToken: + case responseTypeCode, responseTypeIDToken, responseTypeCodeIDToken: // continue - case responseTypeToken: + case responseTypeToken, responseTypeCodeToken, responseTypeIDTokenToken, responseTypeCodeIDTokenToken: // response_type=token is an implicit flow, let's add it to the discovery info // https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.1 allSupportedGrants[grantTypeImplicit] = true From 139845c0a51f604d4c31747dbf032f8cade85fcf Mon Sep 17 00:00:00 2001 From: Cedric-Magnan Date: Wed, 1 Jun 2022 10:28:03 +0200 Subject: [PATCH 05/14] fix: linting with gofmt Signed-off-by: Cedric-Magnan Signed-off-by: Oded Ben-Ozer --- server/oauth2.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/oauth2.go b/server/oauth2.go index a82d172ff0..b72431e0e8 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -145,12 +145,12 @@ const ( ) const ( - responseTypeCode = "code" // "Regular" flow - responseTypeToken = "token" // Implicit flow for frontend apps. - responseTypeIDToken = "id_token" // ID Token in url fragment - responseTypeCodeToken = "code token" // "Regular" flow + Implicit flow - responseTypeCodeIDToken = "code id_token" // "Regular" flow + ID Token - responseTypeIDTokenToken = "id_token token" // ID Token + Implicit flow + responseTypeCode = "code" // "Regular" flow + responseTypeToken = "token" // Implicit flow for frontend apps. + responseTypeIDToken = "id_token" // ID Token in url fragment + responseTypeCodeToken = "code token" // "Regular" flow + Implicit flow + responseTypeCodeIDToken = "code id_token" // "Regular" flow + ID Token + responseTypeIDTokenToken = "id_token token" // ID Token + Implicit flow responseTypeCodeIDTokenToken = "code id_token token" // "Regular" flow + ID Token + Implicit flow ) From 6d143f16c118eb574306d09b68e53b055fb001b4 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 27 Jul 2023 11:58:28 +0200 Subject: [PATCH 06/14] Composite claims in OIDC connector (#3) * Add the ability to composite new claims in the OIDC connector, based on upstream claims Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc.go | 37 ++++++++++++++++++++++ connector/oidc/oidc_test.go | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 14329c0040..00e9dc48e7 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -87,6 +87,21 @@ type Config struct { // Configurable key which contains the groups claims GroupsKey string `json:"groups"` // defaults to "groups" } `json:"claimMapping"` + + // List of new claim to generate based on concatinate existing claims + ClaimConcatenations []ClaimConcatenation `json:"claimConcatenations"` +} + +// List of groups claim elements to create by concatenating other claims +type ClaimConcatenation struct { + // List of claim to join together + ClaimList []string `json:"claimList"` + + // String to separate the claims + Delimiter string `json:"delimiter"` + + // String to place before the first claim + Prefix string `json:"prefix"` } // Domains that don't support basic auth. golang.org/x/oauth2 has an internal @@ -189,6 +204,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e preferredUsernameKey: c.ClaimMapping.PreferredUsernameKey, emailKey: c.ClaimMapping.EmailKey, groupsKey: c.ClaimMapping.GroupsKey, + claimConcatenations: c.ClaimConcatenations, }, nil } @@ -216,6 +232,7 @@ type oidcConnector struct { preferredUsernameKey string emailKey string groupsKey string + claimConcatenations []ClaimConcatenation } func (c *oidcConnector) Close() error { @@ -416,6 +433,26 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I } } + for _, cc := range c.claimConcatenations { + newElement := "" + for _, clm := range cc.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 + } else { + newElement = newElement + cc.Delimiter + claimCleanedValue + } + } + } + if newElement != "" { + groups = append(groups, newElement) + } + } + cd := connectorData{ RefreshToken: []byte(token.RefreshToken), } diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index 5c5208a60e..2f19d9674f 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -62,6 +62,7 @@ func TestHandleCallback(t *testing.T) { expectPreferredUsername string expectedEmailField string token map[string]interface{} + claimConcatenations []ClaimConcatenation }{ { name: "simpleCase", @@ -288,6 +289,66 @@ func TestHandleCallback(t *testing.T) { "email_verified": true, }, }, + { + name: "concatinateClaim", + userIDKey: "", // not configured + userNameKey: "", // not configured + expectUserID: "subvalue", + expectUserName: "namevalue", + expectGroups: []string{"group1", "gh::acme::pipeline-one", "tfe-acme-foobar", "bk-emailvalue"}, + expectedEmailField: "emailvalue", + claimConcatenations: []ClaimConcatenation{ + { + ClaimList: []string{ + "organization", + "pipeline", + }, + Delimiter: "::", + Prefix: "gh", + }, + { + ClaimList: []string{ + "non-existing1", + "non-existing2", + }, + Delimiter: "::", + Prefix: "tfe", + }, + { + ClaimList: []string{ + "organization", + "claim-with-delimiter", + }, + Delimiter: "-", + Prefix: "tfe", + }, + { + ClaimList: []string{ + "non-string-claim", + "non-string-claim2", + "email", + }, + Delimiter: "-", + Prefix: "bk", + }, + }, + + token: map[string]interface{}{ + "sub": "subvalue", + "name": "namevalue", + "groups": "group1", + "organization": "acme", + "pipeline": "pipeline-one", + "email": "emailvalue", + "email_verified": true, + "claim-with-delimiter": "foo-bar", + "non-string-claim": []string{ + "element1", + "element2", + }, + "non-string-claim2": 666, + }, + }, } for _, tc := range tests { @@ -319,6 +380,7 @@ func TestHandleCallback(t *testing.T) { InsecureEnableGroups: true, BasicAuthUnsupported: &basicAuth, OverrideClaimMapping: tc.overrideClaimMapping, + ClaimConcatenations: tc.claimConcatenations, } config.ClaimMapping.PreferredUsernameKey = tc.preferredUsernameKey config.ClaimMapping.EmailKey = tc.emailKey From 316296b0d3279fe74ca43793998a4383d130066d Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Sun, 6 Aug 2023 13:13:39 +0200 Subject: [PATCH 07/14] Document each test case Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index 2f19d9674f..459e2ca2c5 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -298,7 +298,7 @@ func TestHandleCallback(t *testing.T) { expectGroups: []string{"group1", "gh::acme::pipeline-one", "tfe-acme-foobar", "bk-emailvalue"}, expectedEmailField: "emailvalue", claimConcatenations: []ClaimConcatenation{ - { + { // The basic functionality, should create "gh::acme::pipeline-one". ClaimList: []string{ "organization", "pipeline", @@ -306,7 +306,7 @@ func TestHandleCallback(t *testing.T) { Delimiter: "::", Prefix: "gh", }, - { + { // Non existing claims, should not generate any any new group claim. ClaimList: []string{ "non-existing1", "non-existing2", @@ -314,7 +314,9 @@ func TestHandleCallback(t *testing.T) { Delimiter: "::", Prefix: "tfe", }, - { + { // 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". ClaimList: []string{ "organization", "claim-with-delimiter", @@ -322,7 +324,7 @@ func TestHandleCallback(t *testing.T) { Delimiter: "-", Prefix: "tfe", }, - { + { // Ignore non string claims (like arrays), this should result in "bk-emailvalue". ClaimList: []string{ "non-string-claim", "non-string-claim2", From a52848418abf2d69ed1e8a75cf692d3eb222a528 Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Mon, 7 Aug 2023 11:35:22 +0200 Subject: [PATCH 08/14] Rename configuration option to include a reference to groups and structure for future claim modification additions Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc.go | 14 ++++++++------ connector/oidc/oidc_test.go | 6 +++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index dfcbc8c196..9ffb1dee2e 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -88,12 +88,14 @@ type Config struct { GroupsKey string `json:"groups"` // defaults to "groups" } `json:"claimMapping"` - // List of new claim to generate based on concatinate existing claims - ClaimConcatenations []ClaimConcatenation `json:"claimConcatenations"` + // ClaimModifications holds all claim modifications options, current has only newGroupsFromClaims + ClaimModifications struct { + NewGroupsFromClaims []NewGroupsFromClaims `json:"newGroupsFromClaims"` + } } // List of groups claim elements to create by concatenating other claims -type ClaimConcatenation struct { +type NewGroupsFromClaims struct { // List of claim to join together ClaimList []string `json:"claimList"` @@ -204,7 +206,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e preferredUsernameKey: c.ClaimMapping.PreferredUsernameKey, emailKey: c.ClaimMapping.EmailKey, groupsKey: c.ClaimMapping.GroupsKey, - claimConcatenations: c.ClaimConcatenations, + newGroupsFromClaims: c.ClaimModifications.NewGroupsFromClaims, }, nil } @@ -232,7 +234,7 @@ type oidcConnector struct { preferredUsernameKey string emailKey string groupsKey string - claimConcatenations []ClaimConcatenation + newGroupsFromClaims []NewGroupsFromClaims } func (c *oidcConnector) Close() error { @@ -444,7 +446,7 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I } } - for _, cc := range c.claimConcatenations { + for _, cc := range c.newGroupsFromClaims { newElement := "" for _, clm := range cc.ClaimList { // Non string claim value are ignored, concatenating them doesn't really make any sense diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index fe91c7c66d..d1b7ee1c7e 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -62,7 +62,7 @@ func TestHandleCallback(t *testing.T) { expectPreferredUsername string expectedEmailField string token map[string]interface{} - claimConcatenations []ClaimConcatenation + newGroupsFromClaims []NewGroupsFromClaims }{ { name: "simpleCase", @@ -297,7 +297,7 @@ func TestHandleCallback(t *testing.T) { expectUserName: "namevalue", expectGroups: []string{"group1", "gh::acme::pipeline-one", "tfe-acme-foobar", "bk-emailvalue"}, expectedEmailField: "emailvalue", - claimConcatenations: []ClaimConcatenation{ + newGroupsFromClaims: []NewGroupsFromClaims{ { // The basic functionality, should create "gh::acme::pipeline-one". ClaimList: []string{ "organization", @@ -382,11 +382,11 @@ func TestHandleCallback(t *testing.T) { InsecureEnableGroups: true, BasicAuthUnsupported: &basicAuth, OverrideClaimMapping: tc.overrideClaimMapping, - ClaimConcatenations: tc.claimConcatenations, } config.ClaimMapping.PreferredUsernameKey = tc.preferredUsernameKey config.ClaimMapping.EmailKey = tc.emailKey config.ClaimMapping.GroupsKey = tc.groupsKey + config.ClaimModifications.NewGroupsFromClaims = tc.newGroupsFromClaims conn, err := newConnector(config) if err != nil { From b1f4bd019533e0d65a6f196114805f56678f9961 Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Fri, 25 Aug 2023 13:57:34 +0200 Subject: [PATCH 09/14] 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 --- 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 9ffb1dee2e..30b759ce97 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 d1b7ee1c7e..2de6cf0bba 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{ From 7f0056cf13eaa54001691e7395a964d4339e6231 Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Fri, 25 Aug 2023 14:11:16 +0200 Subject: [PATCH 10/14] Fix lint issue Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc.go | 1 - 1 file changed, 1 deletion(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 30b759ce97..2b29284272 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -457,7 +457,6 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I for _, claimName := range newGroupsElementConfig.ClaimList { // 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 From 6875b64cafd26047b4abb89a95748b997e6ae37c Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Mon, 9 Oct 2023 12:30:21 +0200 Subject: [PATCH 11/14] Address issues raised in review: - Rename some vars - Cleanup some comments - Tiny refactor to improve readability Signed-off-by: Oded Ben-Ozer --- 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", From 033717a07e1bd2e44d5d28578ba8a20ac87f1746 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 25 Oct 2023 14:08:34 +0200 Subject: [PATCH 12/14] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Márk Sági-Kazár Signed-off-by: Oded Ben Ozer --- connector/oidc/oidc.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 5b43d2e0ae..7f7145185c 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -94,7 +94,7 @@ type Config struct { } `json:"claimModifications"` } -// List of groups claim elements to create by concatenating other claims +// NewGroupFromClaims creates a new group from a list of claims and appends it to the list of existing groups. type NewGroupsFromClaims struct { // List of claim to join together ClaimList []string `json:"claimList"` @@ -456,15 +456,16 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I } 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 !ok { + if !ok { // Non string claim value are ignored, concatenating them doesn't really make any sense 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) } From 115425960c4fabd724e9bd5de597ef0baa7708a0 Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Wed, 25 Oct 2023 14:13:38 +0200 Subject: [PATCH 13/14] Address issues raised in review: Improve naming Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc.go | 18 +++++++++--------- connector/oidc/oidc_test.go | 16 ++++++++-------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 7f7145185c..a3f98680d1 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -88,16 +88,16 @@ type Config struct { GroupsKey string `json:"groups"` // defaults to "groups" } `json:"claimMapping"` - // ClaimModifications holds all claim modifications options - ClaimModifications struct { - NewGroupsFromClaims []NewGroupsFromClaims `json:"newGroupsFromClaims"` + // ClaimMutations holds all claim mutations options + ClaimMutations struct { + NewGroupFromClaims []NewGroupFromClaims `json:"newGroupFromClaims"` } `json:"claimModifications"` } // NewGroupFromClaims creates a new group from a list of claims and appends it to the list of existing groups. -type NewGroupsFromClaims struct { +type NewGroupFromClaims struct { // List of claim to join together - ClaimList []string `json:"claimList"` + Claims []string `json:"claims"` // String to separate the claims Delimiter string `json:"delimiter"` @@ -210,7 +210,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e preferredUsernameKey: c.ClaimMapping.PreferredUsernameKey, emailKey: c.ClaimMapping.EmailKey, groupsKey: c.ClaimMapping.GroupsKey, - newGroupsFromClaims: c.ClaimModifications.NewGroupsFromClaims, + newGroupFromClaims: c.ClaimMutations.NewGroupFromClaims, }, nil } @@ -238,7 +238,7 @@ type oidcConnector struct { preferredUsernameKey string emailKey string groupsKey string - newGroupsFromClaims []NewGroupsFromClaims + newGroupFromClaims []NewGroupFromClaims } func (c *oidcConnector) Close() error { @@ -450,11 +450,11 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I } } - for _, config := range c.newGroupsFromClaims { + for _, config := range c.newGroupFromClaims { newGroupSegments := []string{ config.Prefix, } - for _, claimName := range config.ClaimList { + for _, claimName := range config.Claims { claimValue, ok := claims[claimName].(string) if !ok { // Non string claim value are ignored, concatenating them doesn't really make any sense continue diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index 13c71ab917..20ff95dc52 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -62,7 +62,7 @@ func TestHandleCallback(t *testing.T) { expectPreferredUsername string expectedEmailField string token map[string]interface{} - newGroupsFromClaims []NewGroupsFromClaims + newGroupFromClaims []NewGroupFromClaims }{ { name: "simpleCase", @@ -297,9 +297,9 @@ func TestHandleCallback(t *testing.T) { expectUserName: "namevalue", expectGroups: []string{"group1", "gh::acme::pipeline-one", "clr_delim-acme-foobar", "keep_delim-acme-foo-bar", "bk-emailvalue"}, expectedEmailField: "emailvalue", - newGroupsFromClaims: []NewGroupsFromClaims{ + newGroupFromClaims: []NewGroupFromClaims{ { // The basic functionality, should create "gh::acme::pipeline-one". - ClaimList: []string{ + Claims: []string{ "organization", "pipeline", }, @@ -307,7 +307,7 @@ func TestHandleCallback(t *testing.T) { Prefix: "gh", }, { // Non existing claims, should not generate any any new group claim. - ClaimList: []string{ + Claims: []string{ "non-existing1", "non-existing2", }, @@ -317,7 +317,7 @@ 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 "clr_delim-acme-foobar" and not "tfe-acme-foo-bar". - ClaimList: []string{ + Claims: []string{ "organization", "claim-with-delimiter", }, @@ -327,7 +327,7 @@ func TestHandleCallback(t *testing.T) { }, { // In this case the delimiter character("-") should be NOT removed from "claim-with-delimiter" claim. // Should create "keep_delim-acme-foo-bar". - ClaimList: []string{ + Claims: []string{ "organization", "claim-with-delimiter", }, @@ -336,7 +336,7 @@ func TestHandleCallback(t *testing.T) { Prefix: "keep_delim", }, { // Ignore non string claims (like arrays), this should result in "bk-emailvalue". - ClaimList: []string{ + Claims: []string{ "non-string-claim", "non-string-claim2", "email", @@ -397,7 +397,7 @@ func TestHandleCallback(t *testing.T) { config.ClaimMapping.PreferredUsernameKey = tc.preferredUsernameKey config.ClaimMapping.EmailKey = tc.emailKey config.ClaimMapping.GroupsKey = tc.groupsKey - config.ClaimModifications.NewGroupsFromClaims = tc.newGroupsFromClaims + config.ClaimMutations.NewGroupFromClaims = tc.newGroupFromClaims conn, err := newConnector(config) if err != nil { From a6a72453b53400c13539f7ce589827cbb90260ef Mon Sep 17 00:00:00 2001 From: Oded Ben-Ozer Date: Wed, 25 Oct 2023 14:27:43 +0200 Subject: [PATCH 14/14] fix some small formatting issue Signed-off-by: Oded Ben-Ozer --- connector/oidc/oidc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index 20ff95dc52..4bb84a40d6 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -62,7 +62,7 @@ func TestHandleCallback(t *testing.T) { expectPreferredUsername string expectedEmailField string token map[string]interface{} - newGroupFromClaims []NewGroupFromClaims + newGroupFromClaims []NewGroupFromClaims }{ { name: "simpleCase",