Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composite claims in OIDC connector #3056

Merged
merged 15 commits into from
Oct 25, 2023
Merged
47 changes: 47 additions & 0 deletions connector/oidc/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,27 @@ type Config struct {
// Configurable key which contains the groups claims
GroupsKey string `json:"groups"` // defaults to "groups"
} `json:"claimMapping"`

// ClaimModifications holds all claim modifications options
ClaimModifications struct {
Oded-B marked this conversation as resolved.
Show resolved Hide resolved
NewGroupsFromClaims []NewGroupsFromClaims `json:"newGroupsFromClaims"`
Oded-B marked this conversation as resolved.
Show resolved Hide resolved
} `json:"claimModifications"`
}

// 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"`
Oded-B marked this conversation as resolved.
Show resolved Hide resolved

// 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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Prefix string `json:"prefix"`
Prefix string `json:"prefix"`

Prefix scales badly because people may want to add prefixes, suffixes, or both, or only prefix specific groups.
This is the same problem we are struggling in Kubernetes (that will be fixed in upcoming releases with CEL).

}

// Domains that don't support basic auth. golang.org/x/oauth2 has an internal
Expand Down Expand Up @@ -189,6 +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,
}, nil
}

Expand Down Expand Up @@ -216,6 +238,7 @@ type oidcConnector struct {
preferredUsernameKey string
emailKey string
groupsKey string
newGroupsFromClaims []NewGroupsFromClaims
}

func (c *oidcConnector) Close() error {
Expand Down Expand Up @@ -427,6 +450,30 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I
}
}

for _, config := range c.newGroupsFromClaims {
newGroupSegments := []string{
config.Prefix,
}
for _, claimName := range config.ClaimList {
claimValue, ok := claims[claimName].(string)
if !ok { // Non string claim value are ignored, concatenating them doesn't really make any sense
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obscure for users.

  1. At least an error should be returned.
  2. I do not see any obstacles to support integers or boolean claims.

continue
}
Oded-B marked this conversation as resolved.
Show resolved Hide resolved

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, "")
}
Oded-B marked this conversation as resolved.
Show resolved Hide resolved

newGroupSegments = append(newGroupSegments, claimValue)
}
Oded-B marked this conversation as resolved.
Show resolved Hide resolved

if len(newGroupSegments) > 1 {
groups = append(groups, strings.Join(newGroupSegments, config.Delimiter))
}
}

cd := connectorData{
RefreshToken: []byte(token.RefreshToken),
}
Expand Down
75 changes: 75 additions & 0 deletions connector/oidc/oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func TestHandleCallback(t *testing.T) {
expectPreferredUsername string
expectedEmailField string
token map[string]interface{}
newGroupsFromClaims []NewGroupsFromClaims
}{
{
name: "simpleCase",
Expand Down Expand Up @@ -288,6 +289,79 @@ func TestHandleCallback(t *testing.T) {
"email_verified": true,
},
},
{
name: "newGroupFromClaims",
userIDKey: "", // not configured
userNameKey: "", // not configured
expectUserID: "subvalue",
expectUserName: "namevalue",
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".
ClaimList: []string{
"organization",
"pipeline",
},
Delimiter: "::",
Prefix: "gh",
},
{ // Non existing claims, should not generate any any new group claim.
ClaimList: []string{
"non-existing1",
"non-existing2",
},
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 "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: "-",
// ClearDelimiter: false,
Prefix: "keep_delim",
},
{ // Ignore non string claims (like arrays), this should result in "bk-emailvalue".
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 {
Expand Down Expand Up @@ -323,6 +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

conn, err := newConnector(config)
if err != nil {
Expand Down