Repository: zitadel/oidc
Pull request #407 (2023-06-09 11:43:43)

fix: don't error on invalid i18n tags in discovery
This changes the use of `[]language.Tag` to `oidc.Locales` in `DiscoveryConfig`. This should be compatible with callers that use the `[]language.Tag` .

Locales now implements the `json.Unmarshaler` interface. With support for json arrays or space seperated strings. The latter because `UnmarshalText` might have been implicitly called by the json library before we added `UnmarshalJSON`.

Fixes: #406

### Definition of Ready

- [x] I am happy with the code
- [x] Short description of the feature/issue is added in the pr description
- [x] PR is linked to the corresponding user story
- [x] Acceptance criteria are met
- [x] All open todos and follow ups are defined in a new ticket and justified
- [x] Deviations from the acceptance criteria and design are agreed with the PO and documented.
- [x] No debug or dead code
- [x] My code has no repetitions
- [x] Critical parts are tested automatically
- [ ] ~~Where possible E2E tests are implemented~~
- [x] Documentation/examples are up-to-date
- [x] All non-functional requirements are met
- [ ] ~~Functionality of the acceptance criteria is checked manually on the dev system.~~



codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zitadel/oidc/pull/407?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
> Merging [#407](https://app.codecov.io/gh/zitadel/oidc/pull/407?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (7570c9d) into [main](https://app.codecov.io/gh/zitadel/oidc/commit/77436a2ce74232ff5d889591099b9c4525dbd432?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (77436a2) will **increase** coverage by `0.20%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   52.08%   52.28%   +0.20%     
==========================================
  Files          74       74              
  Lines        5687     5711      +24     
==========================================
+ Hits         2962     2986      +24     
  Misses       2461     2461              
  Partials      264      264              
```


| [Impacted Files](https://app.codecov.io/gh/zitadel/oidc/pull/407?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Coverage Δ | |
|---|---|---|
| [pkg/oidc/types.go](https://app.codecov.io/gh/zitadel/oidc/pull/407?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29pZGMvdHlwZXMuZ28=) | `92.81% <100.00%> (+1.33%)` | :arrow_up: |



Patch:
diff --git a/pkg/oidc/discovery.go b/pkg/oidc/discovery.go
index 3574101..14fce5e 100644
--- a/pkg/oidc/discovery.go
+++ b/pkg/oidc/discovery.go
@@ -1,9 +1,5 @@
 package oidc
 
-import (
-	"golang.org/x/text/language"
-)
-
 const (
 	DiscoveryEndpoint = "/.well-known/openid-configuration"
 )
@@ -130,10 +126,10 @@ type DiscoveryConfiguration struct {
 	ServiceDocumentation string `json:"service_documentation,omitempty"`
 
 	// ClaimsLocalesSupported contains a list of BCP47 language tag values that the OP supports for values of Claims returned.
-	ClaimsLocalesSupported []language.Tag `json:"claims_locales_supported,omitempty"`
+	ClaimsLocalesSupported Locales `json:"claims_locales_supported,omitempty"`
 
 	// UILocalesSupported contains a list of BCP47 language tag values that the OP supports for the user interface.
-	UILocalesSupported []language.Tag `json:"ui_locales_supported,omitempty"`
+	UILocalesSupported Locales `json:"ui_locales_supported,omitempty"`
 
 	// RequestParameterSupported specifies whether the OP supports use of the `request` parameter. If omitted, the default value is false.
 	RequestParameterSupported bool `json:"request_parameter_supported,omitempty"`
diff --git a/pkg/oidc/types.go b/pkg/oidc/types.go
index 167f8b7..23367ef 100644
--- a/pkg/oidc/types.go
+++ b/pkg/oidc/types.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/gorilla/schema"
+	"github.com/muhlemmer/gu"
 	"golang.org/x/text/language"
 	"gopkg.in/square/go-jose.v2"
 )
@@ -81,14 +82,58 @@ func (l *Locale) UnmarshalJSON(data []byte) error {
 
 type Locales []language.Tag
 
-func (l *Locales) UnmarshalText(text []byte) error {
-	locales := strings.Split(string(text), " ")
+// ParseLocales parses a slice of strings into Locales.
+// If an entry causes a parse error or is undefined,
+// it is ignored and not set to Locales.
+func ParseLocales(locales []string) Locales {
+	out := make(Locales, 0, len(locales))
 	for _, locale := range locales {
 		tag, err := language.Parse(locale)
 		if err == nil && !tag.IsRoot() {
-			*l = append(*l, tag)
+			out = append(out, tag)
 		}
 	}
+	return out
+}
+
+// UnmarshalText implements the [encoding.TextUnmarshaler] interface.
+// It decodes an unquoted space seperated string into Locales.
+// Undefined language tags in the input are ignored and ommited from
+// the resulting Locales.
+func (l *Locales) UnmarshalText(text []byte) error {
+	*l = ParseLocales(
+		strings.Split(string(text), " "),
+	)
+	return nil
+}
+
+// UnmarshalJSON implements the [json.Unmarshaler] interface.
+// It decodes a json array or a space seperated string into Locales.
+// Undefined language tags in the input are ignored and ommited from
+// the resulting Locales.
+func (l *Locales) UnmarshalJSON(data []byte) error {
+	var dst any
+	if err := json.Unmarshal(data, &dst); err != nil {
+		return fmt.Errorf("oidc locales: %w", err)
+	}
+
+	// We catch the posibility of a space seperated string here,
+	// because UnmarshalText might have been implicetely called
+	// by the json library before we added UnmarshalJSON.
+	switch v := dst.(type) {
+	case nil:
+		*l = nil
+	case string:
+		*l = ParseLocales(strings.Split(v, " "))
+	case []any:
+		locales, err := gu.AssertInterfaces[string](v)
+		if err != nil {
+			return fmt.Errorf("oidc locales: %w", err)
+		}
+		*l = ParseLocales(locales)
+	default:
+		return fmt.Errorf("oidc locales: unsupported type: %T", v)
+	}
 	return nil
 }
 


Test patch:
diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go
new file mode 100644
index 0000000..02c408b
--- /dev/null
+++ b/pkg/client/client_test.go
@@ -0,0 +1,54 @@
+package client
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDiscover(t *testing.T) {
+	type wantFields struct {
+		UILocalesSupported bool
+	}
+
+	type args struct {
+		issuer       string
+		wellKnownUrl []string
+	}
+	tests := []struct {
+		name       string
+		args       args
+		wantFields *wantFields
+		wantErr    bool
+	}{
+		{
+			name: "spotify", // https://github.com/zitadel/oidc/issues/406
+			args: args{
+				issuer: "https://accounts.spotify.com",
+			},
+			wantFields: &wantFields{
+				UILocalesSupported: true,
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := Discover(tt.args.issuer, http.DefaultClient, tt.args.wellKnownUrl...)
+			if tt.wantErr {
+				assert.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			if tt.wantFields == nil {
+				return
+			}
+			assert.Equal(t, tt.args.issuer, got.Issuer)
+			if tt.wantFields.UILocalesSupported {
+				assert.NotEmpty(t, got.UILocalesSupported)
+			}
+		})
+	}
+}
diff --git a/pkg/oidc/types_test.go b/pkg/oidc/types_test.go
index 64f07f1..69540c2 100644
--- a/pkg/oidc/types_test.go
+++ b/pkg/oidc/types_test.go
@@ -224,6 +224,13 @@ func TestLocale_UnmarshalJSON(t *testing.T) {
 	assert.Equal(t, want, got)
 }
 
+func TestParseLocales(t *testing.T) {
+	in := []string{language.Afrikaans.String(), language.Danish.String(), "foobar", language.Und.String()}
+	want := Locales{language.Afrikaans, language.Danish}
+	got := ParseLocales(in)
+	assert.ElementsMatch(t, want, got)
+}
+
 func TestLocales_UnmarshalText(t *testing.T) {
 	type args struct {
 		text []byte
@@ -281,6 +288,80 @@ func TestLocales_UnmarshalText(t *testing.T) {
 	}
 }
 
+func TestLocales_UnmarshalJSON(t *testing.T) {
+	in := []string{language.Afrikaans.String(), language.Danish.String(), "foobar", language.Und.String()}
+	spaceSepStr := strconv.Quote(strings.Join(in, " "))
+	jsonArray, err := json.Marshal(in)
+	require.NoError(t, err)
+
+	out := Locales{language.Afrikaans, language.Danish}
+
+	type args struct {
+		data []byte
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    Locales
+		wantErr bool
+	}{
+		{
+			name: "invalid JSON",
+			args: args{
+				data: []byte("~~~"),
+			},
+			wantErr: true,
+		},
+		{
+			name: "null",
+			args: args{
+				data: []byte("null"),
+			},
+			want: nil,
+		},
+		{
+			name: "space seperated string",
+			args: args{
+				data: []byte(spaceSepStr),
+			},
+			want: out,
+		},
+		{
+			name: "json string array",
+			args: args{
+				data: jsonArray,
+			},
+			want: out,
+		},
+		{
+			name: "json invalid array",
+			args: args{
+				data: []byte(`[1,2,3]`),
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid type (float64)",
+			args: args{
+				data: []byte("22"),
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var got Locales
+			err := got.UnmarshalJSON([]byte(tt.args.data))
+			if tt.wantErr {
+				assert.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestScopes_UnmarshalText(t *testing.T) {
 	type args struct {
 		text []byte