Merge pull request #369 from avenga/missing-arrayAttributes

Missing array attributes
coupergateway · Nov 17, 2021 · 940c6a0 · 940c6a0
2 parents d4726a7 + e1f2d57
commit 940c6a0
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ Unreleased changes are available as `avenga/couper:edge` container.
   * [CORS](./docs/REFERENCE.md#cors-block) preflight requests are not blocked by access controls anymore ([#366](https://github.com/avenga/couper/pull/366))
   * Reduced memory usage for backend response bodies which just get piped to the client and are not required to be read by Couper due to a variable references ([#375](https://github.com/avenga/couper/pull/375))
     * However, if a huge message body is passed and additionally referenced via e.g. `json_body`, Couper may require a lot of memory for storing the data structure.
+  * For each SAML attribute listed in [`array_attributes`](./docs/REFERENCE.md#saml-block) at least an empty array is created in `request.context.<label>.attributes.<name>` ([#369](https://github.com/avenga/couper/pull/369))
 
 ---
 

diff --git a/accesscontrol/saml2.go b/accesscontrol/saml2.go
@@ -126,13 +126,18 @@ func (s *Saml2) ValidateAssertionInfo(assertionInfo *saml2.AssertionInfo) error
 
 func (s *Saml2) GetAssertionData(assertionInfo *saml2.AssertionInfo) map[string]interface{} {
 	attributes := make(map[string]interface{})
+	// default empty slice for all arrayAttributes
+	for _, arrayAttrName := range s.arrayAttributes {
+		attributes[arrayAttrName] = []string{}
+	}
 	for _, attribute := range assertionInfo.Values {
 		if !contains(s.arrayAttributes, attribute.Name) {
 			for _, attributeValue := range attribute.Values {
 				attributes[attribute.Name] = attributeValue.Value
 			}
 		} else {
-			var attributeValues []string
+			// default empty slice for this arrayAttribute (instead of nil slice)
+			attributeValues := []string{}
 			for _, attributeValue := range attribute.Values {
 				attributeValues = append(attributeValues, attributeValue.Value)
 			}

diff --git a/accesscontrol/saml2_test.go b/accesscontrol/saml2_test.go
@@ -4,13 +4,13 @@ import (
 	"bytes"
 	"encoding/base64"
 	"encoding/xml"
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	saml2 "github.com/russellhaering/gosaml2"
 	"github.com/russellhaering/gosaml2/types"
 
@@ -150,7 +150,7 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
 		t.Fatal("Expected a saml acs object")
 	}
 
-	values := saml2.Values{
+	valuesWith2MemberOf := saml2.Values{
 		"displayName": types.Attribute{
 			Name: "displayName",
 			Values: []types.AttributeValue{
@@ -174,6 +174,48 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
 			},
 		},
 	}
+	valuesWith1MemberOf := saml2.Values{
+		"displayName": types.Attribute{
+			Name: "displayName",
+			Values: []types.AttributeValue{
+				{
+					Value: "Jane Doe",
+				},
+			},
+		},
+		"memberOf": types.Attribute{
+			Name: "memberOf",
+			Values: []types.AttributeValue{
+				{
+					Value: "group1",
+				},
+			},
+		},
+	}
+	valuesEmptyMemberOf := saml2.Values{
+		"displayName": types.Attribute{
+			Name: "displayName",
+			Values: []types.AttributeValue{
+				{
+					Value: "Jane Doe",
+				},
+			},
+		},
+		"memberOf": types.Attribute{
+			Name:   "memberOf",
+			Values: []types.AttributeValue{},
+		},
+	}
+	valuesMissingMemberOf := saml2.Values{
+		"displayName": types.Attribute{
+			Name: "displayName",
+			Values: []types.AttributeValue{
+				{
+					Value: "Jane Doe",
+				},
+			},
+		},
+	}
 	var authnStatement types.AuthnStatement
 	err = xml.Unmarshal([]byte(`<AuthnStatement xmlns="urn:oasis:names:tc:SAML:2.0:assertion" SessionNotOnOrAfter="2020-11-13T17:06:00Z"/>`), &authnStatement)
 	if err != nil {
@@ -187,10 +229,10 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
 	}
 	for _, tc := range []testCase{
 		{
-			"without exp",
+			"without exp, with 2 memberOf",
 			&saml2.AssertionInfo{
 				NameID: "abc12345",
-				Values: values,
+				Values: valuesWith2MemberOf,
 			},
 			map[string]interface{}{
 				"sub": "abc12345",
@@ -204,15 +246,31 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
 			},
 		},
 		{
-			"with exp",
+			"without exp, with 1 memberOf",
+			&saml2.AssertionInfo{
+				NameID: "abc12345",
+				Values: valuesWith1MemberOf,
+			},
+			map[string]interface{}{
+				"sub": "abc12345",
+				"attributes": map[string]interface{}{
+					"displayName": "Jane Doe",
+					"memberOf": []string{
+						"group1",
+					},
+				},
+			},
+		},
+		{
+			"with exp, with memberOf",
 			&saml2.AssertionInfo{
 				NameID:              "abc12345",
 				SessionNotOnOrAfter: authnStatement.SessionNotOnOrAfter,
-				Values:              values,
+				Values:              valuesWith2MemberOf,
 			},
 			map[string]interface{}{
 				"sub": "abc12345",
-				"exp": 1605287160,
+				"exp": int64(1605287160),
 				"attributes": map[string]interface{}{
 					"displayName": "Jane Doe",
 					"memberOf": []string{
@@ -222,11 +280,39 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
 				},
 			},
 		},
+		{
+			"without exp, empty memberOf",
+			&saml2.AssertionInfo{
+				NameID: "abc12345",
+				Values: valuesEmptyMemberOf,
+			},
+			map[string]interface{}{
+				"sub": "abc12345",
+				"attributes": map[string]interface{}{
+					"displayName": "Jane Doe",
+					"memberOf":    []string{},
+				},
+			},
+		},
+		{
+			"without exp, without memberOf",
+			&saml2.AssertionInfo{
+				NameID: "abc12345",
+				Values: valuesMissingMemberOf,
+			},
+			map[string]interface{}{
+				"sub": "abc12345",
+				"attributes": map[string]interface{}{
+					"displayName": "Jane Doe",
+					"memberOf":    []string{},
+				},
+			},
+		},
 	} {
 		t.Run(tc.name, func(subT *testing.T) {
 			assertionData := sa.GetAssertionData(tc.assertionInfo)
-			if fmt.Sprint(assertionData) != fmt.Sprint(tc.want) {
-				subT.Errorf("%s: GetAssertionData() data = %v, want %v", tc.name, assertionData, tc.want)
+			if !cmp.Equal(tc.want, assertionData) {
+				subT.Errorf(cmp.Diff(tc.want, assertionData))
 			}
 		})
 	}

diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md
@@ -471,7 +471,7 @@ required _label_.
 | `idp_metadata_file` | string | - | File reference to the Identity Provider metadata XML file. | &#9888; required | - |
 | `sp_acs_url`        | string | - | The URL of the Service Provider's ACS endpoint. | &#9888; required. Relative URL references are resolved against the origin of the current request URL. The origin can be changed with the [`accept_forwarded_url`](#settings-block) attribute if Couper is running behind a proxy. | - |
 | `sp_entity_id`      | string | - | The Service Provider's entity ID. |&#9888; required | - |
-| `array_attributes`  | string | - | A list of assertion attributes that may have several values. | - | - |
+| `array_attributes`  | string | `[]` | A list of assertion attributes that may have several values. | Results in at least an empty array in `request.context.<label>.attributes.<name>` | `array_attributes = ["memberOf"]` |
 
 Some information from the assertion consumed at the ACS endpoint is provided in the context at `request.context.<label>`:
 

diff --git a/internal/seetie/convert.go b/internal/seetie/convert.go
@@ -85,9 +85,6 @@ func ValuesMapToValue(m url.Values) cty.Value {
 }
 
 func ListToValue(l []interface{}) cty.Value {
-	if len(l) == 0 {
-		return cty.NilVal
-	}
 	var list []cty.Value
 	for _, v := range l {
 		list = append(list, GoToValue(v))

diff --git a/server/http_integration_test.go b/server/http_integration_test.go
@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/dgrijalva/jwt-go/v4"
+	"github.com/google/go-cmp/cmp"
 	"github.com/sirupsen/logrus"
 	logrustest "github.com/sirupsen/logrus/hooks/test"
 
@@ -3160,13 +3161,14 @@ func TestJWTAccessControl_round(t *testing.T) {
 	defer shutdown()
 
 	type testCase struct {
-		name string
-		path string
+		name      string
+		path      string
+		expGroups []interface{}
 	}
 
 	for _, tc := range []testCase{
-		{"separate jwt_signing_profile/jwt", "/separate"},
-		{"self-signed jwt", "/self-signed"},
+		{"separate jwt_signing_profile/jwt", "/separate", []interface{}{"g1", "g2"}},
+		{"self-signed jwt", "/self-signed", []interface{}{}},
 	} {
 		t.Run(tc.path, func(subT *testing.T) {
 			helper := test.New(subT)
@@ -3217,6 +3219,17 @@ func TestJWTAccessControl_round(t *testing.T) {
 			if pidclaim != pid {
 				subT.Fatalf("%q: unexpected pid claim: %q", tc.name, pidclaim)
 			}
+			groupsclaim, ok := claims["groups"]
+			if !ok {
+				subT.Fatalf("%q: missing groups claim: %#v", tc.name, claims)
+			}
+			groupsclaimArray, ok := groupsclaim.([]interface{})
+			if !ok {
+				subT.Fatalf("%q: groups must be array: %#v", tc.name, groupsclaim)
+			}
+			if !cmp.Equal(tc.expGroups, groupsclaimArray) {
+				subT.Errorf(cmp.Diff(tc.expGroups, groupsclaimArray))
+			}
 		})
 	}
 }

diff --git a/server/testdata/integration/config/08_couper.hcl b/server/testdata/integration/config/08_couper.hcl
@@ -20,7 +20,7 @@ server "jwt" {
     endpoint "/{p}/create-jwt" {
       response {
         headers = {
-          x-jwt = jwt_sign("my_jwt", {})
+          x-jwt = jwt_sign("my_jwt", {groups = []})
         }
       }
     }
@@ -40,6 +40,7 @@ definitions {
     claims = {
       iss = "the_issuer"
       pid = request.path_params.p
+      groups = ["g1", "g2"]
     }
   }
   jwt "my_jwt" {