Repository: oauth2-proxy/oauth2-proxy
Pull request #839 (2020-10-13 14:24:56)

Enable custom structure for group claim with default name group
## Description

I ignore the default group claim for unmarshalling json, because we can not be sure that it has the type `[]string`.
Instead I extract only the group claim defined via config. If the group value is not a string I use the json representation of the group value. With this change the application can handle the group values individually.

Maybe you have a better idea for handling the group value, but at least for me it would work.

## Motivation and Context

[Couldn't extract claims from id_token because group claim has invalid structure](https://github.com/oauth2-proxy/oauth2-proxy/issues/838)

## How Has This Been Tested?

I created an additional test case.

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [X] I have created a feature (non-master) branch for my PR.


NickMeves: Hi @Bibob7 -- This might be a duplicate of this PR that is in work: https://github.com/oauth2-proxy/oauth2-proxy/pull/816

Can you take a look over there and see if your needs are covered?
Bibob7: @NickMeves You are right, there is an overlap. But the decisive difference is that in my PR, even the default group claim can have a completely different structure.
The most important thing for me is the adjustment in line 314 in `providers/oidc.go`.
NickMeves: > @NickMeves You are right, there is an overlap. But the decisive difference is that in my PR, even the default group claim can have a completely different structure.
> The most important thing for me is the adjustment in line 314 in `providers/oidc.go`.

If you don't mind, can you support getting the proper fixes in the other PR? We try to lean toward letting the first submitter of a PR the precedence to encourage the community to contribute and get the experience of getting a PR merged on an open source project.

If it starts to go stale, we can pick up the work here. We won't wait the normal 60 days because we want this feature in v7 which we want to release in 4-6 weeks.
Bibob7: > Thanks for the work on this!

You are welcome! 😊

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cb496b3a..f9f3f1f1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
   - Sessions from v6.0.0 or later had a graceful conversion to SHA256 that resulted in no reauthentication
   - Upgrading from v5.1.1 or earlier will result in a reauthentication
 - [#616](https://github.com/oauth2-proxy/oauth2-proxy/pull/616) Ensure you have configured oauth2-proxy to use the `groups` scope. The user may be logged out initially as they may not currently have the `groups` claim however after going back through login process wil be authenticated.
+- [#839](https://github.com/oauth2-proxy/oauth2-proxy/pull/839) Enables complex data structures for group claim entries, which are output as Json by default.
 
 ## Breaking Changes
 
diff --git a/providers/oidc.go b/providers/oidc.go
index 24221caa..0f9fc28a 100644
--- a/providers/oidc.go
+++ b/providers/oidc.go
@@ -2,7 +2,9 @@ package providers
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
+	"reflect"
 	"strings"
 	"time"
 
@@ -10,6 +12,7 @@ import (
 	"golang.org/x/oauth2"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests"
 )
 
@@ -285,21 +288,36 @@ func (p *OIDCProvider) extractGroupsFromRawClaims(rawClaims map[string]interface
 	rawGroups, ok := rawClaims[p.GroupsClaim].([]interface{})
 	if rawGroups != nil && ok {
 		for _, rawGroup := range rawGroups {
-			group, ok := rawGroup.(string)
-			if ok {
-				groups = append(groups, group)
+			formattedGroup, err := formatGroup(rawGroup)
+			if err != nil {
+				logger.Errorf("unable to format group of type %s with error %s", reflect.TypeOf(rawGroup), err)
+				continue
 			}
+			groups = append(groups, formattedGroup)
 		}
 	}
 
 	return groups
+
+}
+
+func formatGroup(rawGroup interface{}) (string, error) {
+	group, ok := rawGroup.(string)
+	if !ok {
+		jsonGroup, err := json.Marshal(rawGroup)
+		if err != nil {
+			return "", err
+		}
+		group = string(jsonGroup)
+	}
+	return group, nil
 }
 
 type OIDCClaims struct {
 	rawClaims         map[string]interface{}
 	UserID            string
-	Subject           string `json:"sub"`
-	Verified          *bool  `json:"email_verified"`
-	PreferredUsername string `json:"preferred_username"`
-	Groups            []string
+	Subject           string   `json:"sub"`
+	Verified          *bool    `json:"email_verified"`
+	PreferredUsername string   `json:"preferred_username"`
+	Groups            []string `json:"-"`
 }


Test patch:
diff --git a/providers/oidc_test.go b/providers/oidc_test.go
index 5b88b9a7..2293428b 100644
--- a/providers/oidc_test.go
+++ b/providers/oidc_test.go
@@ -29,12 +29,12 @@ const clientID = "https://test.myapp.com"
 const secret = "secret"
 
 type idTokenClaims struct {
-	Name        string   `json:"name,omitempty"`
-	Email       string   `json:"email,omitempty"`
-	Phone       string   `json:"phone_number,omitempty"`
-	Picture     string   `json:"picture,omitempty"`
-	Groups      []string `json:"groups,omitempty"`
-	OtherGroups []string `json:"other_groups,omitempty"`
+	Name        string      `json:"name,omitempty"`
+	Email       string      `json:"email,omitempty"`
+	Phone       string      `json:"phone_number,omitempty"`
+	Picture     string      `json:"picture,omitempty"`
+	Groups      interface{} `json:"groups,omitempty"`
+	OtherGroups interface{} `json:"other_groups,omitempty"`
 	jwt.StandardClaims
 }
 
@@ -64,6 +64,29 @@ var defaultIDToken idTokenClaims = idTokenClaims{
 	},
 }
 
+var customGroupClaimIDToken idTokenClaims = idTokenClaims{
+	"Jane Dobbs",
+	"janed@me.com",
+	"+4798765432",
+	"http://mugbook.com/janed/me.jpg",
+	[]map[string]interface{}{
+		{
+			"groupId": "Admin Group Id",
+			"roles":   []string{"Admin"},
+		},
+	},
+	[]string{"test:c", "test:d"},
+	jwt.StandardClaims{
+		Audience:  "https://test.myapp.com",
+		ExpiresAt: time.Now().Add(time.Duration(5) * time.Minute).Unix(),
+		Id:        "id-some-id",
+		IssuedAt:  time.Now().Unix(),
+		Issuer:    "https://issuer.example.com",
+		NotBefore: 0,
+		Subject:   "123456789",
+	},
+}
+
 var minimalIDToken idTokenClaims = idTokenClaims{
 	"",
 	"",
@@ -283,7 +306,7 @@ func TestCreateSessionStateFromBearerToken(t *testing.T) {
 		GroupsClaim    string
 		ExpectedUser   string
 		ExpectedEmail  string
-		ExpectedGroups []string
+		ExpectedGroups interface{}
 	}{
 		"Default IDToken": {
 			IDToken:        defaultIDToken,
@@ -306,6 +329,13 @@ func TestCreateSessionStateFromBearerToken(t *testing.T) {
 			ExpectedEmail:  defaultIDToken.Email,
 			ExpectedGroups: []string{"test:c", "test:d"},
 		},
+		"Custom Groups Claim2": {
+			IDToken:        customGroupClaimIDToken,
+			GroupsClaim:    "groups",
+			ExpectedUser:   customGroupClaimIDToken.Subject,
+			ExpectedEmail:  customGroupClaimIDToken.Email,
+			ExpectedGroups: []string{"{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}"},
+		},
 	}
 	for testName, tc := range testCases {
 		t.Run(testName, func(t *testing.T) {
@@ -373,3 +403,31 @@ func TestOIDCProvider_findVerifiedIdToken(t *testing.T) {
 	assert.Equal(t, nil, err)
 	assert.Equal(t, true, verifiedIDToken == nil)
 }
+
+func Test_formatGroup(t *testing.T) {
+	testCases := map[string]struct {
+		RawGroup                    interface{}
+		ExpectedFormattedGroupValue string
+	}{
+		"String Group": {
+			RawGroup:                    "group",
+			ExpectedFormattedGroupValue: "group",
+		},
+		"Map Group": {
+			RawGroup:                    map[string]string{"id": "1", "name": "Test"},
+			ExpectedFormattedGroupValue: "{\"id\":\"1\",\"name\":\"Test\"}",
+		},
+		"List Group": {
+			RawGroup:                    []string{"First", "Second"},
+			ExpectedFormattedGroupValue: "[\"First\",\"Second\"]",
+		},
+	}
+
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			formattedGroup, err := formatGroup(tc.RawGroup)
+			assert.Nil(t, err)
+			assert.Equal(t, tc.ExpectedFormattedGroupValue, formattedGroup)
+		})
+	}
+}