From d08fb02d1a8931976d4ae47b3a7c9168d2606c34 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Tue, 10 Jan 2023 14:25:44 +0100 Subject: [PATCH 1/3] fix: resilient social sign in --- go.mod | 2 +- go.sum | 4 +- identity/credentials_oidc.go | 8 + identity/credentials_oidc_test.go | 19 + selfservice/strategy/oidc/error.go | 24 +- selfservice/strategy/oidc/provider.go | 15 + selfservice/strategy/oidc/provider_auth0.go | 5 +- ...h0_test.go => provider_auth0_unit_test.go} | 0 .../strategy/oidc/provider_dingtalk.go | 8 + .../strategy/oidc/provider_facebook.go | 4 + .../strategy/oidc/provider_facebook_test.go | 93 ----- selfservice/strategy/oidc/provider_gitlab.go | 5 +- .../strategy/oidc/provider_microsoft.go | 5 +- selfservice/strategy/oidc/provider_netid.go | 4 + selfservice/strategy/oidc/provider_test.go | 20 ++ .../strategy/oidc/provider_userinfo_test.go | 330 ++++++++++++++++++ selfservice/strategy/oidc/provider_vk.go | 15 +- selfservice/strategy/oidc/provider_yandex.go | 4 + selfservice/strategy/oidc/strategy.go | 5 + 19 files changed, 462 insertions(+), 108 deletions(-) create mode 100644 identity/credentials_oidc_test.go rename selfservice/strategy/oidc/{provider_auth0_test.go => provider_auth0_unit_test.go} (100%) delete mode 100644 selfservice/strategy/oidc/provider_facebook_test.go create mode 100644 selfservice/strategy/oidc/provider_test.go create mode 100644 selfservice/strategy/oidc/provider_userinfo_test.go diff --git a/go.mod b/go.mod index cd0b1a33109a..ab49bf87390c 100644 --- a/go.mod +++ b/go.mod @@ -78,7 +78,7 @@ require ( github.com/ory/jsonschema/v3 v3.0.7 github.com/ory/mail/v3 v3.0.0 github.com/ory/nosurf v1.2.7 - github.com/ory/x v0.0.527 + github.com/ory/x v0.0.531 github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2 github.com/pkg/errors v0.9.1 github.com/pquerna/otp v1.4.0 diff --git a/go.sum b/go.sum index 83d74c2ff8ab..c029b296e8d0 100644 --- a/go.sum +++ b/go.sum @@ -1140,8 +1140,8 @@ github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2 h1:zm6sDvHy/U9XrGpi github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM= github.com/ory/viper v1.7.5 h1:+xVdq7SU3e1vNaCsk/ixsfxE4zylk1TJUiJrY647jUE= github.com/ory/viper v1.7.5/go.mod h1:ypOuyJmEUb3oENywQZRgeAMwqgOyDqwboO1tj3DjTaM= -github.com/ory/x v0.0.527 h1:K6MmsYqT1NMb8VQ4hhn9q6NnrnecwNQJXc1bEoixQ8Y= -github.com/ory/x v0.0.527/go.mod h1:XBqhPZRppPHTxtsE0l0oI/B2Onf1QJtMRGPh3CpEpA0= +github.com/ory/x v0.0.531 h1:ndkhfzgX7y1ChNnYPS5ioqVuvyRENXKUBrNnkmoPOFQ= +github.com/ory/x v0.0.531/go.mod h1:XBqhPZRppPHTxtsE0l0oI/B2Onf1QJtMRGPh3CpEpA0= github.com/otiai10/copy v1.2.0/go.mod h1:rrF5dJ5F0t/EWSYODDu4j9/vEeYHMkc8jt0zJChqQWw= github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95/go.mod h1:9qAhocn7zKJG+0mI8eUu6xqkFDYS2kb2saOteoSB3cE= github.com/otiai10/curr v1.0.0/go.mod h1:LskTG5wDwr8Rs+nNQ+1LlxRjAtTZZjtJW4rMXl6j4vs= diff --git a/identity/credentials_oidc.go b/identity/credentials_oidc.go index 3121fed93c18..1ad0ada209c1 100644 --- a/identity/credentials_oidc.go +++ b/identity/credentials_oidc.go @@ -33,6 +33,14 @@ type CredentialsOIDCProvider struct { // NewCredentialsOIDC creates a new OIDC credential. func NewCredentialsOIDC(idToken, accessToken, refreshToken, provider, subject string) (*Credentials, error) { + if provider == "" { + return nil, errors.New("received empty provider in oidc credentials") + } + + if subject == "" { + return nil, errors.New("received empty provider in oidc credentials") + } + var b bytes.Buffer if err := json.NewEncoder(&b).Encode(CredentialsOIDC{ Providers: []CredentialsOIDCProvider{ diff --git a/identity/credentials_oidc_test.go b/identity/credentials_oidc_test.go new file mode 100644 index 000000000000..b916220f1b9b --- /dev/null +++ b/identity/credentials_oidc_test.go @@ -0,0 +1,19 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package identity + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNewCredentialsOIDC(t *testing.T) { + _, err := NewCredentialsOIDC("", "", "", "", "not-empty") + require.Error(t, err) + _, err = NewCredentialsOIDC("", "", "", "not-empty", "") + require.Error(t, err) + _, err = NewCredentialsOIDC("", "", "", "not-empty", "not-empty") + require.NoError(t, err) +} diff --git a/selfservice/strategy/oidc/error.go b/selfservice/strategy/oidc/error.go index 1b1f98751ee6..1a0cccdfae31 100644 --- a/selfservice/strategy/oidc/error.go +++ b/selfservice/strategy/oidc/error.go @@ -3,7 +3,15 @@ package oidc -import "github.com/ory/herodot" +import ( + "io" + "net/http" + + "github.com/pkg/errors" + + "github.com/ory/herodot" + "github.com/ory/x/logrusx" +) var ( ErrScopeMissing = herodot.ErrBadRequest. @@ -17,3 +25,17 @@ var ( ErrAPIFlowNotSupported = herodot.ErrBadRequest.WithError("API-based flows are not supported for this method"). WithReasonf("Social Sign In and OpenID Connect are only supported for flows initiated using the Browser endpoint.") ) + +func logUpstreamError(l *logrusx.Logger, resp *http.Response) error { + if resp.StatusCode == http.StatusOK { + return nil + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + body = []byte(err.Error()) + } + + l.WithField("response_code", resp.StatusCode).WithField("response_body", string(body)).Error("The upstream OIDC provider returned a non 200 status code.") + return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("OpenID Connect provider returned a %d status code but 200 is expected.", resp.StatusCode)) +} diff --git a/selfservice/strategy/oidc/provider.go b/selfservice/strategy/oidc/provider.go index 450e056cb14f..a63fd69ce517 100644 --- a/selfservice/strategy/oidc/provider.go +++ b/selfservice/strategy/oidc/provider.go @@ -7,6 +7,10 @@ import ( "context" "net/url" + "github.com/pkg/errors" + + "github.com/ory/herodot" + "golang.org/x/oauth2" "github.com/ory/kratos/x" @@ -50,3 +54,14 @@ type Claims struct { Team string `json:"team,omitempty"` RawClaims map[string]interface{} `json:"raw_claims,omitempty"` } + +// Validate checks if the claims are valid. +func (c *Claims) Validate() error { + if c.Subject == "" { + return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("provider did not return a subject")) + } + if c.Issuer == "" { + return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("issuer not set in claims")) + } + return nil +} diff --git a/selfservice/strategy/oidc/provider_auth0.go b/selfservice/strategy/oidc/provider_auth0.go index 3dcd204e9c74..4a0e727ff771 100644 --- a/selfservice/strategy/oidc/provider_auth0.go +++ b/selfservice/strategy/oidc/provider_auth0.go @@ -87,7 +87,6 @@ func (g *ProviderAuth0) Claims(ctx context.Context, exchange *oauth2.Token, quer return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err)) } - req.Header.Add("Authorization", "Bearer: "+exchange.AccessToken) req.Header.Add("Content-Type", "application/json") resp, err := client.Do(req) @@ -96,6 +95,10 @@ func (g *ProviderAuth0) Claims(ctx context.Context, exchange *oauth2.Token, quer } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + // Once auth0 fixes this bug, all this workaround can be removed. b, err := io.ReadAll(resp.Body) if err != nil { diff --git a/selfservice/strategy/oidc/provider_auth0_test.go b/selfservice/strategy/oidc/provider_auth0_unit_test.go similarity index 100% rename from selfservice/strategy/oidc/provider_auth0_test.go rename to selfservice/strategy/oidc/provider_auth0_unit_test.go diff --git a/selfservice/strategy/oidc/provider_dingtalk.go b/selfservice/strategy/oidc/provider_dingtalk.go index 1c8bccbbfcd4..71ae65c870cc 100644 --- a/selfservice/strategy/oidc/provider_dingtalk.go +++ b/selfservice/strategy/oidc/provider_dingtalk.go @@ -96,6 +96,10 @@ func (g *ProviderDingTalk) Exchange(ctx context.Context, code string, opts ...oa } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + var dToken struct { ErrCode int `json:"code"` ErrMsg string `json:"message"` @@ -135,6 +139,10 @@ func (g *ProviderDingTalk) Claims(ctx context.Context, exchange *oauth2.Token, _ } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + var user struct { Nick string `json:"nick"` OpenId string `json:"openId"` diff --git a/selfservice/strategy/oidc/provider_facebook.go b/selfservice/strategy/oidc/provider_facebook.go index fe40c6fe15a5..70e7df99b749 100644 --- a/selfservice/strategy/oidc/provider_facebook.go +++ b/selfservice/strategy/oidc/provider_facebook.go @@ -86,6 +86,10 @@ func (g *ProviderFacebook) Claims(ctx context.Context, exchange *oauth2.Token, q } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + var user struct { Id string `json:"id,omitempty"` Name string `json:"name,omitempty"` diff --git a/selfservice/strategy/oidc/provider_facebook_test.go b/selfservice/strategy/oidc/provider_facebook_test.go deleted file mode 100644 index a27412c2fa66..000000000000 --- a/selfservice/strategy/oidc/provider_facebook_test.go +++ /dev/null @@ -1,93 +0,0 @@ -// Copyright © 2023 Ory Corp -// SPDX-License-Identifier: Apache-2.0 - -package oidc_test - -import ( - "context" - "net/http" - "net/url" - "testing" - "time" - - "github.com/jarcoal/httpmock" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "golang.org/x/oauth2" - - "github.com/ory/kratos/internal" - "github.com/ory/kratos/selfservice/strategy/oidc" -) - -const fakeIDToken = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjk5OTk5OTk5OTksImF1ZCI6ImFiY2QiLCJpc3MiOiJodHRwczovL3Jhdy5naXRodWJ1c2VyY29udGVudC5jb20vYWVuZWFzci9wcml2YXRlLW9pZGMvbWFzdGVyL3Rva2VuIn0.G9v8pJXJrEOgdJ5ecE6sIIcTH_p-RKkBaImfZY5DDVCl7h5GEis1n3GKKYbL_O3fj8Fu-WzI2mquI8S8BOVCQ6wN0XtrqJv22iX_nzeVHc4V_JWV1q7hg2gPpoFFcnF3KKtxZLvDOA8ujsDbAXmoBu0fEBdwCN56xLOOKQDzULyfijuAa8hrCwespZ9HaqcHzD3iHf_Utd4nHqlTM-6upWpKIMkplS_NGcxrfIRIWusZ0wob6ryy8jECD9QeZpdTGUozq-YM64lZfMOZzuLuqichH_PCMKFyB_tOZb6lDIiiSX4Irz7_YF-DP-LmfxgIW4934RqTCeFGGIP64h4xAA" - -func TestProviderFacebook_Claims(t *testing.T) { - httpmock.Activate() - defer httpmock.DeactivateAndReset() - - httpmock.RegisterResponder("GET", "https://graph.facebook.com/me", - func(req *http.Request) (*http.Response, error) { - if _, ok := req.URL.Query()["appsecret_proof"]; !ok { - resp, err := httpmock.NewJsonResponse(400, map[string]interface{}{ - "error": map[string]interface{}{ - "message": "API calls from the server require an appsecret_proof argument", - "type": "GraphMethodException", - "code": 100, - "fbtrace_id": "Ay8LR3n5BsHm809VYpJ3eDM", - }, - }) - return resp, err - } - resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ - "id": "123456789012345", - "name": "John Doe", - "first_name": "John", - "last_name": "Doe", - "email": "john.doe@example.com", - "birthday": "01/01/1990", - }) - return resp, err - }, - ) - - httpmock.RegisterResponder("GET", "https://www.facebook.com/.well-known/openid-configuration", - func(req *http.Request) (*http.Response, error) { - resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ - "issuer": "https://www.facebook.com", - }) - return resp, err - }, - ) - - _, reg := internal.NewFastRegistryWithMocks(t) - c := &oidc.Configuration{ - ID: "facebook", - Provider: "facebook", - ClientID: "abcd", - ClientSecret: "secret", - Mapper: "file://./stub/oidc.facebook.jsonnet", - Scope: []string{"email"}, - } - facebook := oidc.NewProviderFacebook(c, reg) - - actual, err := facebook.Claims( - context.Background(), - (&oauth2.Token{AccessToken: "foo", Expiry: time.Now().Add(time.Hour)}).WithExtra(map[string]interface{}{"id_token": fakeIDToken}), - url.Values{}, - ) - require.NoError(t, err) - - assert.Equal(t, &oidc.Claims{ - Issuer: "https://graph.facebook.com/me?fields=id,name,first_name,last_name,middle_name,email,picture,birthday,gender&appsecret_proof=773ba44693c7553d6ee20f61ea5d2757a9a4f4a44d2841ae4e95b52e4cd62db4", - Subject: "123456789012345", - Name: "John Doe", - GivenName: "John", - FamilyName: "Doe", - Nickname: "John Doe", - PreferredUsername: "John Doe", - Email: "john.doe@example.com", - EmailVerified: true, - Birthdate: "01/01/1990", - }, actual) -} diff --git a/selfservice/strategy/oidc/provider_gitlab.go b/selfservice/strategy/oidc/provider_gitlab.go index ee6338ea37de..58fa106dbcab 100644 --- a/selfservice/strategy/oidc/provider_gitlab.go +++ b/selfservice/strategy/oidc/provider_gitlab.go @@ -6,7 +6,6 @@ package oidc import ( "context" "encoding/json" - "net/http" "net/url" "path" @@ -93,8 +92,8 @@ func (g *ProviderGitLab) Claims(ctx context.Context, exchange *oauth2.Token, que } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Expected the GitLab userinfo endpoint to return a 200 OK response but got %d instead.", resp.StatusCode)) + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err } var claims Claims diff --git a/selfservice/strategy/oidc/provider_microsoft.go b/selfservice/strategy/oidc/provider_microsoft.go index 55a48f1c5873..e5c4a8ec68ff 100644 --- a/selfservice/strategy/oidc/provider_microsoft.go +++ b/selfservice/strategy/oidc/provider_microsoft.go @@ -6,7 +6,6 @@ package oidc import ( "context" "encoding/json" - "net/http" "net/url" "strings" @@ -103,8 +102,8 @@ func (m *ProviderMicrosoft) updateSubject(ctx context.Context, claims *Claims, e } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to fetch from `https://graph.microsoft.com/v1.0/me: Got Status %s", resp.Status)) + if err := logUpstreamError(m.reg.Logger(), resp); err != nil { + return nil, err } var user struct { diff --git a/selfservice/strategy/oidc/provider_netid.go b/selfservice/strategy/oidc/provider_netid.go index bce2c5a18cd0..660537288f38 100644 --- a/selfservice/strategy/oidc/provider_netid.go +++ b/selfservice/strategy/oidc/provider_netid.go @@ -86,6 +86,10 @@ func (n *ProviderNetID) Claims(ctx context.Context, exchange *oauth2.Token, _ ur } defer resp.Body.Close() + if err := logUpstreamError(n.reg.Logger(), resp); err != nil { + return nil, err + } + var claims Claims if err := json.NewDecoder(resp.Body).Decode(&claims); err != nil { return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err)) diff --git a/selfservice/strategy/oidc/provider_test.go b/selfservice/strategy/oidc/provider_test.go new file mode 100644 index 000000000000..8fda4a753d4b --- /dev/null +++ b/selfservice/strategy/oidc/provider_test.go @@ -0,0 +1,20 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestClaimsValidate(t *testing.T) { + require.Error(t, new(Claims).Validate()) + require.Error(t, (&Claims{Issuer: "not-empty"}).Validate()) + require.Error(t, (&Claims{Issuer: "not-empty"}).Validate()) + require.Error(t, (&Claims{Subject: "not-empty"}).Validate()) + require.Error(t, (&Claims{Subject: "not-empty"}).Validate()) + require.NoError(t, (&Claims{Issuer: "not-empty", Subject: "not-empty"}).Validate()) + require.NoError(t, (&Claims{Issuer: "not-empty", Subject: "not-empty"}).Validate()) +} diff --git a/selfservice/strategy/oidc/provider_userinfo_test.go b/selfservice/strategy/oidc/provider_userinfo_test.go new file mode 100644 index 000000000000..a59cc74bb7a3 --- /dev/null +++ b/selfservice/strategy/oidc/provider_userinfo_test.go @@ -0,0 +1,330 @@ +// Copyright © 2023 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package oidc_test + +import ( + "context" + "encoding/json" + "net/http" + "net/url" + "testing" + "time" + + "github.com/jarcoal/httpmock" + "golang.org/x/oauth2" + + "github.com/ory/herodot" + "github.com/ory/kratos/driver/config" + "github.com/ory/kratos/internal" + "github.com/ory/kratos/selfservice/strategy/oidc" + "github.com/ory/x/otelx" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestProviderClaimsRespectsErrorCodes(t *testing.T) { + conf, reg := internal.NewFastRegistryWithMocks(t) + require.NoError(t, conf.Set(context.Background(), config.ViperKeyClientHTTPNoPrivateIPRanges, true)) + reg.SetTracer(otelx.NewNoop(nil, nil)) + + ctx := context.Background() + token := &oauth2.Token{AccessToken: "foo", Expiry: time.Now().Add(time.Hour)} + + expectedClaims := &oidc.Claims{ + Issuer: "ignore-me", + Subject: "123456789012345", + Name: "John Doe", + GivenName: "John", + FamilyName: "Doe", + Nickname: "John Doe", + PreferredUsername: "John Doe", + Email: "john.doe@example.com", + EmailVerified: true, + Birthdate: "01/01/1990", + } + + defaultUserinfoHandler := func(req *http.Request) (*http.Response, error) { + if head := req.Header.Get("Authorization"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, expectedClaims) + return resp, err + } + + for _, tc := range []struct { + name string + issuer string + userInfoEndpoint string + config *oidc.Configuration + provider oidc.Provider + userInfoHandler func(req *http.Request) (*http.Response, error) + expectedClaims *oidc.Claims + useToken *oauth2.Token + hook func(t *testing.T) + }{ + { + name: "auth0", + userInfoHandler: defaultUserinfoHandler, + userInfoEndpoint: "https://www.auth0.com/userinfo", + provider: oidc.NewProviderAuth0(&oidc.Configuration{ + IssuerURL: "https://www.auth0.com", + ID: "auth0", + Provider: "auth0", + }, reg), + }, + { + name: "netid", + userInfoHandler: defaultUserinfoHandler, + userInfoEndpoint: "https://broker.netid.de/userinfo", + provider: oidc.NewProviderNetID(&oidc.Configuration{ + IssuerURL: "https://broker.netid.de", + ID: "netid", + Provider: "netid", + }, reg), + }, + { + name: "vk", + userInfoEndpoint: "https://api.vk.com/method/users.get", + provider: oidc.NewProviderVK(&oidc.Configuration{ + IssuerURL: "https://oauth.vk.com", + ID: "vk", + Provider: "vk", + }, reg), + useToken: token.WithExtra(map[string]interface{}{"email": "john.doe@example.com"}), + userInfoHandler: func(req *http.Request) (*http.Response, error) { + if head := req.URL.Query().Get("access_token"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ + "response": []map[string]interface{}{{"id": 123456789012345}}, + }) + return resp, err + }, + + expectedClaims: &oidc.Claims{ + Issuer: "https://api.vk.com/method/users.get", + Subject: "123456789012345", + Email: "john.doe@example.com", + }, + }, + { + name: "yandex", + userInfoEndpoint: "https://login.yandex.ru/info", + provider: oidc.NewProviderYandex(&oidc.Configuration{ + IssuerURL: "https://oauth.yandex.com", + ID: "vk", + Provider: "vk", + }, reg), + useToken: token.WithExtra(map[string]interface{}{"email": "john.doe@example.com"}), + userInfoHandler: func(req *http.Request) (*http.Response, error) { + if head := req.URL.Query().Get("oauth_token"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ + "id": "123456789012345", + "default_email": "john.doe@example.com", + }) + return resp, err + }, + + expectedClaims: &oidc.Claims{ + Issuer: "https://login.yandex.ru/info", + Subject: "123456789012345", + Email: "john.doe@example.com", + Picture: "https://avatars.yandex.net/get-yapic//islands-200", + }, + }, + { + name: "facebook", + hook: func(t *testing.T) { + httpmock.RegisterResponder("GET", "https://www.facebook.com/.well-known/openid-configuration", + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ + "issuer": "https://www.facebook.com", + }) + return resp, err + }, + ) + }, + userInfoEndpoint: "https://graph.facebook.com/me", + provider: oidc.NewProviderFacebook(&oidc.Configuration{ + ID: "facebook", + Provider: "facebook", + }, reg), + useToken: token, + userInfoHandler: func(req *http.Request) (*http.Response, error) { + if head := req.Header.Get("Authorization"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + if _, ok := req.URL.Query()["appsecret_proof"]; !ok { + resp, err := httpmock.NewJsonResponse(400, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ + "id": "123456789012345", + "name": "John Doe", + "first_name": "John", + "last_name": "Doe", + "email": "john.doe@example.com", + "birthday": "01/01/1990", + }) + return resp, err + }, + expectedClaims: &oidc.Claims{ + Issuer: "https://graph.facebook.com/me?fields=id,name,first_name,last_name,middle_name,email,picture,birthday,gender&appsecret_proof=0c0d98f7e3d9d45e72e8877bc1b104327efb9c07b18f2ffeced76d81307f1fff", + Subject: "123456789012345", + Name: "John Doe", + GivenName: "John", + FamilyName: "Doe", + Nickname: "John Doe", + PreferredUsername: "John Doe", + Email: "john.doe@example.com", + EmailVerified: true, + Birthdate: "01/01/1990", + }, + }, + { + name: "gitlab", + userInfoHandler: defaultUserinfoHandler, + userInfoEndpoint: "https://www.gitlab.com/oauth/userinfo", + provider: oidc.NewProviderGitLab(&oidc.Configuration{ + IssuerURL: "https://www.gitlab.com", + ID: "gitlab", + Provider: "gitlab", + }, reg), + }, + // Microsoft is a more complicated set up because it actually verifies the ID Token before using the userinfo endpoint. + { + name: "microsoft", + userInfoHandler: func(req *http.Request) (*http.Response, error) { + if head := req.Header.Get("Authorization"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, json.RawMessage(`{"id":"new-id"}`)) + return resp, err + + }, + userInfoEndpoint: "https://graph.microsoft.com/v1.0/me", + provider: oidc.NewProviderMicrosoft(&oidc.Configuration{ + IssuerURL: "https://login.microsoftonline.com/", + ID: "microsoft", + Provider: "microsoft", + Tenant: "a9b86385-f32c-4803-afc8-4b2312fbdf24", + ClientID: "foo", + ClientSecret: "bar", + SubjectSource: "me", + }, reg), + useToken: token.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiZW1haWwiOiJqb2huLmRvZUBleGFtcGxlLmNvbSIsImlhdCI6MTUxNjIzOTAyMiwidGlkIjoiYTliODYzODUtZjMyYy00ODAzLWFmYzgtNGIyMzEyZmJkZjI0IiwiaXNzIjoiaHR0cHM6Ly9sb2dpbi5taWNyb3NvZnRvbmxpbmUuY29tL2E5Yjg2Mzg1LWYzMmMtNDgwMy1hZmM4LTRiMjMxMmZiZGYyNC92Mi4wIiwiYXVkIjpbImZvbyJdLCJleHAiOjQwNzE3Mjg1MDR9.LRgolO5_-26uMrBo89NCzUfi87a8jf7rXlWgZVYnpfowzqn-U0JhNGVzEOXOACoPdX9HsEtYj4hZxYgYcd6z7yqgmOZXE-y58L5BHYZU1kk37O1Dl_VnN-BmeCWs_JZiXF2KEnu7BW8btYnb26qCnc3_8RGbyJwI4UU6ynbJzAmLCPUPgoMZ2Jpahx_K8vqLe-4rveLyUvVVHMoUAV16I-Wg08GcuA5cY0_91-QpA5Kq0AA58wbrUbAOEAOYOpa_63QqYcCcZLbnX_w09Z1YCXGGVSbbpzjr3cJ8EFA6doloBKRLWtNtEnGk4hyjHyp_ls89ZYqJ1ngy95AEswzwJQ"}), + hook: func(t *testing.T) { + httpmock.RegisterResponder("GET", "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0/.well-known/openid-configuration", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, map[string]interface{}{ + "issuer": "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0", + "jwks_uri": "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0/.well-known/jwks.json", + }) + }, + ) + httpmock.RegisterResponder("GET", "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0/.well-known/jwks.json", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, json.RawMessage(`{ + "keys": [ + { + "kty": "RSA", + "e": "AQAB", + "use": "sig", + "alg": "RS256", + "n": "xGnYx5u9S7LCrtINTI1Kav6pqzFj72JhYunqrDhnG18Iu-MJdCSmd26xiiZFY6UsJoG-te_DcUE_YHvhx6vwC0tJO-Z2uxwKRxWsMiyW8v3V0Bfbtu7jlf0kpQYeAeAIAHih7GU_v5jtxaGLNR9JIbmENUGOoIfydfTWLKHtwI0MB8tNVwjqP8e6ZSr6DNIjsKKuz98BeRlZ576jyH2GcmM9DguLZv73qU1m1OLoHFW5rAPH70-nhr9V67TTR1A1is9v85uLlKBnZgKp-EdYtBEvKRWSpw5vxCnKggJjEYATOB2H5m_c8F1e1tsJLAehaXKMKZYI-VlWWxj7KixDKw" + } + ] +}`)) + }, + ) + }, + expectedClaims: &oidc.Claims{Issuer: "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0", Subject: "new-id", Name: "John Doe", Email: "john.doe@example.com", + RawClaims: map[string]interface{}{"aud": []interface{}{"foo"}, "exp": 4.071728504e+09, "iat": 1.516239022e+09, "iss": "https://login.microsoftonline.com/a9b86385-f32c-4803-afc8-4b2312fbdf24/v2.0", "email": "john.doe@example.com", "name": "John Doe", "sub": "1234567890", "tid": "a9b86385-f32c-4803-afc8-4b2312fbdf24"}}, + }, + { + name: "dingtalk", + userInfoHandler: func(req *http.Request) (*http.Response, error) { + if head := req.Header.Get("x-acs-dingtalk-access-token"); len(head) == 0 { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{"error": ""}) + return resp, err + } + + resp, err := httpmock.NewJsonResponse(200, map[string]interface{}{ + "openId": "123456789012345", + "email": "john.doe@example.com", + }) + return resp, err + }, + userInfoEndpoint: "https://api.dingtalk.com/v1.0/contact/users/me", + provider: oidc.NewProviderDingTalk(&oidc.Configuration{ + IssuerURL: "https://www.dingtalk.com", + ID: "dingtalk", + Provider: "dingtalk", + }, reg), + expectedClaims: &oidc.Claims{ + Issuer: "https://api.dingtalk.com/v1.0/contact/users/me", + Subject: "123456789012345", + Email: "john.doe@example.com", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + token := token + if tc.useToken != nil { + token = tc.useToken + } + + t.Run("http error is respected", func(t *testing.T) { + httpmock.Activate() + t.Cleanup(httpmock.DeactivateAndReset) + + if tc.hook != nil { + tc.hook(t) + } + + httpmock.RegisterResponder("GET", tc.userInfoEndpoint, func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.NewJsonResponse(401, map[string]interface{}{}) + return resp, err + }) + + _, err := tc.provider.Claims(ctx, token, url.Values{}) + var he *herodot.DefaultError + require.ErrorAs(t, err, &he) + assert.Equal(t, "OpenID Connect provider returned a 401 status code but 200 is expected.", he.Reason()) + }) + + t.Run("call is successful", func(t *testing.T) { + httpmock.Activate() + t.Cleanup(httpmock.DeactivateAndReset) + + if tc.hook != nil { + tc.hook(t) + } + + httpmock.RegisterResponder("GET", tc.userInfoEndpoint, tc.userInfoHandler) + + claims, err := tc.provider.Claims(ctx, token, url.Values{}) + require.NoError(t, err) + if tc.expectedClaims == nil { + assert.Equal(t, expectedClaims, claims) + } else { + assert.Equal(t, tc.expectedClaims, claims) + } + }) + }) + } +} diff --git a/selfservice/strategy/oidc/provider_vk.go b/selfservice/strategy/oidc/provider_vk.go index ff2f5a223061..602a8574f999 100644 --- a/selfservice/strategy/oidc/provider_vk.go +++ b/selfservice/strategy/oidc/provider_vk.go @@ -60,7 +60,6 @@ func (g *ProviderVK) OAuth2(ctx context.Context) (*oauth2.Config, error) { } func (g *ProviderVK) Claims(ctx context.Context, exchange *oauth2.Token, query url.Values) (*Claims, error) { - o, err := g.OAuth2(ctx) if err != nil { return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err)) @@ -78,13 +77,17 @@ func (g *ProviderVK) Claims(ctx context.Context, exchange *oauth2.Token, query u } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + type User struct { Id int `json:"id,omitempty"` FirstName string `json:"first_name,omitempty"` LastName string `json:"last_name,omitempty"` Nickname string `json:"nickname,omitempty"` Picture string `json:"photo_200,omitempty"` - Email string + Email string `json:"-"` Gender int `json:"sex,omitempty"` BirthDay string `json:"bdate,omitempty"` } @@ -97,10 +100,14 @@ func (g *ProviderVK) Claims(ctx context.Context, exchange *oauth2.Token, query u return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err)) } + if len(response.Result) == 0 { + return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("VK did not return a user in the userinfo request.")) + } + user := response.Result[0] - if email := exchange.Extra("email"); email != nil { - user.Email = email.(string) + if email, ok := exchange.Extra("email").(string); ok { + user.Email = email } gender := "" diff --git a/selfservice/strategy/oidc/provider_yandex.go b/selfservice/strategy/oidc/provider_yandex.go index 699918f3a570..12a845185b91 100644 --- a/selfservice/strategy/oidc/provider_yandex.go +++ b/selfservice/strategy/oidc/provider_yandex.go @@ -75,6 +75,10 @@ func (g *ProviderYandex) Claims(ctx context.Context, exchange *oauth2.Token, que } defer resp.Body.Close() + if err := logUpstreamError(g.reg.Logger(), resp); err != nil { + return nil, err + } + var user struct { Id string `json:"id,omitempty"` FirstName string `json:"first_name,omitempty"` diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 2c2cfebb2847..373e56c5fe00 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -350,6 +350,11 @@ func (s *Strategy) handleCallback(w http.ResponseWriter, r *http.Request, ps htt return } + if err := claims.Validate(); err != nil { + s.forwardError(w, r, req, s.handleError(w, r, req, pid, nil, err)) + return + } + switch a := req.(type) { case *login.Flow: if ff, err := s.processLogin(w, r, a, token, claims, provider, cntnr); err != nil { From ee62834293833f5e77e66744b066fc0c609ce82b Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Tue, 10 Jan 2023 14:50:56 +0100 Subject: [PATCH 2/3] Update selfservice/strategy/oidc/provider_test.go --- selfservice/strategy/oidc/provider_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/selfservice/strategy/oidc/provider_test.go b/selfservice/strategy/oidc/provider_test.go index 8fda4a753d4b..1b464bd2eec5 100644 --- a/selfservice/strategy/oidc/provider_test.go +++ b/selfservice/strategy/oidc/provider_test.go @@ -16,5 +16,4 @@ func TestClaimsValidate(t *testing.T) { require.Error(t, (&Claims{Subject: "not-empty"}).Validate()) require.Error(t, (&Claims{Subject: "not-empty"}).Validate()) require.NoError(t, (&Claims{Issuer: "not-empty", Subject: "not-empty"}).Validate()) - require.NoError(t, (&Claims{Issuer: "not-empty", Subject: "not-empty"}).Validate()) } From fc258f807e1dd9c53b7b4ca9b7be92de536605d8 Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Tue, 10 Jan 2023 15:26:14 +0100 Subject: [PATCH 3/3] Update selfservice/strategy/oidc/error.go Co-authored-by: Patrik --- selfservice/strategy/oidc/error.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/selfservice/strategy/oidc/error.go b/selfservice/strategy/oidc/error.go index 1a0cccdfae31..d901d474194d 100644 --- a/selfservice/strategy/oidc/error.go +++ b/selfservice/strategy/oidc/error.go @@ -33,7 +33,7 @@ func logUpstreamError(l *logrusx.Logger, resp *http.Response) error { body, err := io.ReadAll(resp.Body) if err != nil { - body = []byte(err.Error()) + l = l.WithError(err) } l.WithField("response_code", resp.StatusCode).WithField("response_body", string(body)).Error("The upstream OIDC provider returned a non 200 status code.")