From d9efc0ad476f0b619dfee1131b508f0f0c2b2be9 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 14 Jun 2023 12:26:04 +0200 Subject: [PATCH 1/4] fix: don't assume the login challenge to be a UUID For compatibility with https://github.com/ory/hydra/pull/3515, which now encodes the whole flow in the login challenge, we cannot further assume that the challenge is a UUID. --- hydra/fake.go | 14 ++--- hydra/hydra.go | 34 +++++------- hydra/hydra_test.go | 55 +++++++++---------- .../0bc96cc9-dda4-4700-9e42-35731f2af91e.json | 1 - .../1fb23c75-b809-42cc-8984-6ca2d0a1192f.json | 1 - .../202c1981-1e25-47f0-8764-75ad506c2bec.json | 1 - .../349c945a-60f8-436a-a301-7a42c92604f9.json | 1 - .../38caf592-b042-4551-b92f-8d5223c2a4e2.json | 1 - .../3a9ea34f-0f12-469b-9417-3ae5795a7baa.json | 1 - .../43c99182-bb67-47e1-b564-bb23bd8d4393.json | 1 - .../47edd3a8-0998-4779-9469-f4b8ee4430df.json | 1 - .../56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e.json | 1 - .../6d387820-f2f4-4f9f-9980-a90d89e7811f.json | 1 - .../916ded11-aa64-4a27-b06e-96e221a509d7.json | 1 - .../99974ce6-388c-4669-a95a-7757ee724020.json | 1 - .../b1fac7fb-d016-4a06-a7fe-e4eab2a0429f.json | 1 - .../d6aa1f23-88c9-4b9b-a850-392f48c7f9e8.json | 1 - .../05a7f09d-4ef3-41fb-958a-6ad74584b36a.json | 1 - .../22d58184-b97d-44a5-bbaf-0aa8b4000d81.json | 1 - .../2bf132e0-5d40-4df9-9a11-9106e5333735.json | 1 - .../696e7022-c466-44f6-89c6-8cf93c06a62a.json | 1 - .../87fa3f43-5155-42b4-a1ad-174c2595fdaf.json | 1 - .../8ef215a9-e8d5-43b3-9aa3-cb4333562e36.json | 1 - .../8f32efdc-f6fc-4c27-a3c2-579d109eff60.json | 1 - .../9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd.json | 1 - .../e2150cdc-23ac-4940-a240-6c79c27ab029.json | 1 - .../ef18b06e-4700-4021-9949-ef783cd86be8.json | 1 - .../f1b5ed18-113a-4a98-aae7-d4eba007199c.json | 1 - ...0000_hydra_login_challenge_format.down.sql | 5 ++ ...ydra_login_challenge_format.mysql.down.sql | 5 ++ ...ra_login_challenge_format.sqlite3.down.sql | 5 ++ ...000000_hydra_login_challenge_format.up.sql | 5 ++ selfservice/flow/login/flow.go | 6 +- selfservice/flow/login/flow_test.go | 2 +- selfservice/flow/login/handler.go | 33 ++++++----- selfservice/flow/login/hook.go | 6 +- selfservice/flow/registration/flow.go | 2 +- selfservice/flow/registration/flow_test.go | 2 +- selfservice/flow/registration/handler.go | 6 +- selfservice/flow/registration/hook.go | 6 +- .../strategy/password/op_login_test.go | 8 +-- ...ebauthn_login_is_invalid-type=browser.json | 1 - ...if_webauthn_login_is_invalid-type=spa.json | 1 - 43 files changed, 104 insertions(+), 117 deletions(-) create mode 100644 persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql create mode 100644 persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql create mode 100644 persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql create mode 100644 persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql diff --git a/hydra/fake.go b/hydra/fake.go index 02c908d0b0c6..1e4a71ffe2c4 100644 --- a/hydra/fake.go +++ b/hydra/fake.go @@ -7,8 +7,6 @@ import ( "context" "errors" - "github.com/gofrs/uuid" - hydraclientgo "github.com/ory/hydra-client-go/v2" "github.com/ory/kratos/session" ) @@ -29,19 +27,19 @@ func NewFake() *FakeHydra { return &FakeHydra{} } -func (h *FakeHydra) AcceptLoginRequest(_ context.Context, hlc uuid.UUID, _ string, _ session.AuthenticationMethods) (string, error) { - switch hlc.String() { + func (h *FakeHydra) AcceptLoginRequest(_ context.Context, loginChallenge string, _ string, _ session.AuthenticationMethods) (string, error) { + switch loginChallenge { case FakeInvalidLoginChallenge: return "", ErrFakeAcceptLoginRequestFailed case FakeValidLoginChallenge: return FakePostLoginURL, nil default: - panic("unknown fake login_challenge " + hlc.String()) + panic("unknown fake login_challenge " + loginChallenge) } } -func (h *FakeHydra) GetLoginRequest(_ context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) { - switch hlc.UUID.String() { +func (h *FakeHydra) GetLoginRequest(_ context.Context, loginChallenge string) (*hydraclientgo.OAuth2LoginRequest, error) { + switch loginChallenge { case FakeInvalidLoginChallenge: return &hydraclientgo.OAuth2LoginRequest{}, nil case FakeValidLoginChallenge: @@ -49,6 +47,6 @@ func (h *FakeHydra) GetLoginRequest(_ context.Context, hlc uuid.NullUUID) (*hydr RequestUrl: "https://www.ory.sh", }, nil default: - panic("unknown fake login_challenge " + hlc.UUID.String()) + panic("unknown fake login_challenge " + loginChallenge) } } diff --git a/hydra/hydra.go b/hydra/hydra.go index 4200555fc530..e26d32d77834 100644 --- a/hydra/hydra.go +++ b/hydra/hydra.go @@ -5,13 +5,12 @@ package hydra import ( "context" - "fmt" "net/http" "time" "github.com/ory/x/httpx" + "github.com/ory/x/sqlxx" - "github.com/gofrs/uuid" "github.com/pkg/errors" "github.com/ory/herodot" @@ -26,12 +25,12 @@ type ( config.Provider x.HTTPClientProvider } - HydraProvider interface { + Provider interface { Hydra() Hydra } Hydra interface { - AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, sub string, amr session.AuthenticationMethods) (string, error) - GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) + AcceptLoginRequest(ctx context.Context, loginChallenge string, subject string, amr session.AuthenticationMethods) (string, error) + GetLoginRequest(ctx context.Context, loginChallenge string) (*hydraclientgo.OAuth2LoginRequest, error) } DefaultHydra struct { d hydraDependencies @@ -44,19 +43,14 @@ func NewDefaultHydra(d hydraDependencies) *DefaultHydra { } } -func GetLoginChallengeID(conf *config.Config, r *http.Request) (uuid.NullUUID, error) { +func GetLoginChallengeID(conf *config.Config, r *http.Request) (sqlxx.NullString, error) { if !r.URL.Query().Has("login_challenge") { - return uuid.NullUUID{}, nil + return "", nil } else if conf.OAuth2ProviderURL(r.Context()) == nil { - return uuid.NullUUID{}, errors.WithStack(herodot.ErrInternalServerError.WithReason("refusing to parse login_challenge query parameter because " + config.ViperKeyOAuth2ProviderURL + " is invalid or unset")) + return "", errors.WithStack(herodot.ErrInternalServerError.WithReason("refusing to parse login_challenge query parameter because " + config.ViperKeyOAuth2ProviderURL + " is invalid or unset")) } - hlc, err := uuid.FromString(r.URL.Query().Get("login_challenge")) - if err != nil || hlc.IsNil() { - return uuid.NullUUID{}, errors.WithStack(herodot.ErrBadRequest.WithReason("the login_challenge parameter is present but invalid or zero UUID")) - } else { - return uuid.NullUUID{UUID: hlc, Valid: true}, nil - } + return sqlxx.NullString(r.URL.Query().Get("login_challenge")), nil } func (h *DefaultHydra) getAdminURL(ctx context.Context) (string, error) { @@ -85,11 +79,11 @@ func (h *DefaultHydra) getAdminAPIClient(ctx context.Context) (hydraclientgo.OAu return hydraclientgo.NewAPIClient(configuration).OAuth2Api, nil } -func (h *DefaultHydra) AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, sub string, amr session.AuthenticationMethods) (string, error) { +func (h *DefaultHydra) AcceptLoginRequest(ctx context.Context, loginChallenge string, subject string, amr session.AuthenticationMethods) (string, error) { remember := h.d.Config().SessionPersistentCookie(ctx) rememberFor := int64(h.d.Config().SessionLifespan(ctx) / time.Second) - alr := hydraclientgo.NewAcceptOAuth2LoginRequest(sub) + alr := hydraclientgo.NewAcceptOAuth2LoginRequest(subject) alr.Remember = &remember alr.RememberFor = &rememberFor alr.Amr = []string{} @@ -102,7 +96,7 @@ func (h *DefaultHydra) AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, su return "", err } - resp, r, err := aa.AcceptOAuth2LoginRequest(ctx).LoginChallenge(fmt.Sprintf("%x", hlc)).AcceptOAuth2LoginRequest(*alr).Execute() + resp, r, err := aa.AcceptOAuth2LoginRequest(ctx).LoginChallenge(loginChallenge).AcceptOAuth2LoginRequest(*alr).Execute() if err != nil { innerErr := herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to accept OAuth 2.0 Login Challenge.") if r != nil { @@ -126,8 +120,8 @@ func (h *DefaultHydra) AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, su return resp.RedirectTo, nil } -func (h *DefaultHydra) GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) { - if !hlc.Valid { +func (h *DefaultHydra) GetLoginRequest(ctx context.Context, loginChallenge string) (*hydraclientgo.OAuth2LoginRequest, error) { + if loginChallenge == "" { return nil, errors.WithStack(herodot.ErrBadRequest.WithReason("invalid login_challenge")) } @@ -136,7 +130,7 @@ func (h *DefaultHydra) GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) ( return nil, err } - hlr, r, err := aa.GetOAuth2LoginRequest(ctx).LoginChallenge(fmt.Sprintf("%x", hlc.UUID)).Execute() + hlr, r, err := aa.GetOAuth2LoginRequest(ctx).LoginChallenge(loginChallenge).Execute() if err != nil { innerErr := herodot.ErrInternalServerError.WithWrap(err).WithReasonf("Unable to get OAuth 2.0 Login Challenge.") if r != nil { diff --git a/hydra/hydra_test.go b/hydra/hydra_test.go index dcb489737d5c..49f99c38957e 100644 --- a/hydra/hydra_test.go +++ b/hydra/hydra_test.go @@ -6,21 +6,25 @@ package hydra_test import ( "net/http" "os" - "reflect" "testing" - "github.com/gofrs/uuid" + "github.com/stretchr/testify/assert" "github.com/ory/kratos/driver/config" "github.com/ory/kratos/hydra" "github.com/ory/x/configx" "github.com/ory/x/logrusx" + "github.com/ory/x/sqlxx" "github.com/ory/x/urlx" ) +func requestFromChallenge(s string) *http.Request { + return &http.Request{URL: urlx.ParseOrPanic("https://hydra?login_challenge=" + s)} +} + func TestGetLoginChallengeID(t *testing.T) { - validChallenge := "https://hydra?login_challenge=b346a452-e8fb-4828-8ef8-a4dbc98dc23a" - invalidChallenge := "https://hydra?login_challenge=invalid" + uuidChallenge := "b346a452-e8fb-4828-8ef8-a4dbc98dc23a" + blobChallenge := "1337deadbeefcafe" defaultConfig := config.MustNew(t, logrusx.New("", ""), os.Stderr, configx.SkipValidation()) configWithHydra := config.MustNew( t, @@ -37,10 +41,10 @@ func TestGetLoginChallengeID(t *testing.T) { r *http.Request } tests := []struct { - name string - args args - want uuid.NullUUID - wantErr bool + name string + args args + want string + assertErr assert.ErrorAssertionFunc }{ { name: "no login challenge; hydra is not configured", @@ -48,8 +52,8 @@ func TestGetLoginChallengeID(t *testing.T) { conf: defaultConfig, r: &http.Request{URL: urlx.ParseOrPanic("https://hydra")}, }, - want: uuid.NullUUID{Valid: false}, - wantErr: false, + want: "", + assertErr: assert.NoError, }, { name: "no login challenge; hydra is configured", @@ -57,47 +61,42 @@ func TestGetLoginChallengeID(t *testing.T) { conf: configWithHydra, r: &http.Request{URL: urlx.ParseOrPanic("https://hydra")}, }, - want: uuid.NullUUID{Valid: false}, - wantErr: false, + want: "", + assertErr: assert.NoError, }, { name: "login_challenge is present; Hydra is not configured", args: args{ conf: defaultConfig, - r: &http.Request{URL: urlx.ParseOrPanic(validChallenge)}, + r: requestFromChallenge(uuidChallenge), }, - want: uuid.NullUUID{Valid: false}, - wantErr: true, + want: "", + assertErr: assert.Error, }, { name: "login_challenge is present; hydra is configured", args: args{ conf: configWithHydra, - r: &http.Request{URL: urlx.ParseOrPanic(validChallenge)}, + r: requestFromChallenge(uuidChallenge), }, - want: uuid.NullUUID{Valid: true, UUID: uuid.FromStringOrNil("b346a452-e8fb-4828-8ef8-a4dbc98dc23a")}, - wantErr: false, + want: uuidChallenge, + assertErr: assert.NoError, }, { name: "login_challenge is invalid; hydra is configured", args: args{ conf: configWithHydra, - r: &http.Request{URL: urlx.ParseOrPanic(invalidChallenge)}, + r: requestFromChallenge(blobChallenge), }, - want: uuid.NullUUID{Valid: false}, - wantErr: true, + want: blobChallenge, + assertErr: assert.NoError, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := hydra.GetLoginChallengeID(tt.args.conf, tt.args.r) - if (err != nil) != tt.wantErr { - t.Errorf("GetLoginChallengeID() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetLoginChallengeID() = %v, want %v", got, tt.want) - } + tt.assertErr(t, err) + assert.Equal(t, sqlxx.NullString(tt.want), got) }) } } diff --git a/persistence/sql/migratest/fixtures/login_flow/0bc96cc9-dda4-4700-9e42-35731f2af91e.json b/persistence/sql/migratest/fixtures/login_flow/0bc96cc9-dda4-4700-9e42-35731f2af91e.json index ab5b570fb780..e48e54d97a6b 100644 --- a/persistence/sql/migratest/fixtures/login_flow/0bc96cc9-dda4-4700-9e42-35731f2af91e.json +++ b/persistence/sql/migratest/fixtures/login_flow/0bc96cc9-dda4-4700-9e42-35731f2af91e.json @@ -1,6 +1,5 @@ { "id": "0bc96cc9-dda4-4700-9e42-35731f2af91e", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/1fb23c75-b809-42cc-8984-6ca2d0a1192f.json b/persistence/sql/migratest/fixtures/login_flow/1fb23c75-b809-42cc-8984-6ca2d0a1192f.json index d955b7c462e6..5f63a7ec006a 100644 --- a/persistence/sql/migratest/fixtures/login_flow/1fb23c75-b809-42cc-8984-6ca2d0a1192f.json +++ b/persistence/sql/migratest/fixtures/login_flow/1fb23c75-b809-42cc-8984-6ca2d0a1192f.json @@ -1,6 +1,5 @@ { "id": "1fb23c75-b809-42cc-8984-6ca2d0a1192f", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/202c1981-1e25-47f0-8764-75ad506c2bec.json b/persistence/sql/migratest/fixtures/login_flow/202c1981-1e25-47f0-8764-75ad506c2bec.json index 3241be100fe9..efbd0740cdfb 100644 --- a/persistence/sql/migratest/fixtures/login_flow/202c1981-1e25-47f0-8764-75ad506c2bec.json +++ b/persistence/sql/migratest/fixtures/login_flow/202c1981-1e25-47f0-8764-75ad506c2bec.json @@ -1,6 +1,5 @@ { "id": "202c1981-1e25-47f0-8764-75ad506c2bec", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/349c945a-60f8-436a-a301-7a42c92604f9.json b/persistence/sql/migratest/fixtures/login_flow/349c945a-60f8-436a-a301-7a42c92604f9.json index 58c5df35b673..7586d19409ab 100644 --- a/persistence/sql/migratest/fixtures/login_flow/349c945a-60f8-436a-a301-7a42c92604f9.json +++ b/persistence/sql/migratest/fixtures/login_flow/349c945a-60f8-436a-a301-7a42c92604f9.json @@ -1,6 +1,5 @@ { "id": "349c945a-60f8-436a-a301-7a42c92604f9", - "oauth2_login_challenge": "3caddfd5-9903-4bce-83ff-cae36f42dff7", "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/38caf592-b042-4551-b92f-8d5223c2a4e2.json b/persistence/sql/migratest/fixtures/login_flow/38caf592-b042-4551-b92f-8d5223c2a4e2.json index aebfc35ddcbc..084b36a0c0b9 100644 --- a/persistence/sql/migratest/fixtures/login_flow/38caf592-b042-4551-b92f-8d5223c2a4e2.json +++ b/persistence/sql/migratest/fixtures/login_flow/38caf592-b042-4551-b92f-8d5223c2a4e2.json @@ -1,6 +1,5 @@ { "id": "38caf592-b042-4551-b92f-8d5223c2a4e2", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/3a9ea34f-0f12-469b-9417-3ae5795a7baa.json b/persistence/sql/migratest/fixtures/login_flow/3a9ea34f-0f12-469b-9417-3ae5795a7baa.json index 87ac8cbc5773..13dff119fce0 100644 --- a/persistence/sql/migratest/fixtures/login_flow/3a9ea34f-0f12-469b-9417-3ae5795a7baa.json +++ b/persistence/sql/migratest/fixtures/login_flow/3a9ea34f-0f12-469b-9417-3ae5795a7baa.json @@ -1,6 +1,5 @@ { "id": "3a9ea34f-0f12-469b-9417-3ae5795a7baa", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/43c99182-bb67-47e1-b564-bb23bd8d4393.json b/persistence/sql/migratest/fixtures/login_flow/43c99182-bb67-47e1-b564-bb23bd8d4393.json index cee695fcfea8..5f1529c393b3 100644 --- a/persistence/sql/migratest/fixtures/login_flow/43c99182-bb67-47e1-b564-bb23bd8d4393.json +++ b/persistence/sql/migratest/fixtures/login_flow/43c99182-bb67-47e1-b564-bb23bd8d4393.json @@ -1,6 +1,5 @@ { "id": "43c99182-bb67-47e1-b564-bb23bd8d4393", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/47edd3a8-0998-4779-9469-f4b8ee4430df.json b/persistence/sql/migratest/fixtures/login_flow/47edd3a8-0998-4779-9469-f4b8ee4430df.json index 698d637c4588..fe46265a6d2e 100644 --- a/persistence/sql/migratest/fixtures/login_flow/47edd3a8-0998-4779-9469-f4b8ee4430df.json +++ b/persistence/sql/migratest/fixtures/login_flow/47edd3a8-0998-4779-9469-f4b8ee4430df.json @@ -1,6 +1,5 @@ { "id": "47edd3a8-0998-4779-9469-f4b8ee4430df", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e.json b/persistence/sql/migratest/fixtures/login_flow/56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e.json index cca989cf7d1c..85156c189e4d 100644 --- a/persistence/sql/migratest/fixtures/login_flow/56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e.json +++ b/persistence/sql/migratest/fixtures/login_flow/56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e.json @@ -1,6 +1,5 @@ { "id": "56d94e8b-8a5d-4b7f-8a6e-3259d2b2903e", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/6d387820-f2f4-4f9f-9980-a90d89e7811f.json b/persistence/sql/migratest/fixtures/login_flow/6d387820-f2f4-4f9f-9980-a90d89e7811f.json index 87ce37ad7964..c38727386af7 100644 --- a/persistence/sql/migratest/fixtures/login_flow/6d387820-f2f4-4f9f-9980-a90d89e7811f.json +++ b/persistence/sql/migratest/fixtures/login_flow/6d387820-f2f4-4f9f-9980-a90d89e7811f.json @@ -1,6 +1,5 @@ { "id": "6d387820-f2f4-4f9f-9980-a90d89e7811f", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/916ded11-aa64-4a27-b06e-96e221a509d7.json b/persistence/sql/migratest/fixtures/login_flow/916ded11-aa64-4a27-b06e-96e221a509d7.json index 715afad13121..eb8ec21e0e31 100644 --- a/persistence/sql/migratest/fixtures/login_flow/916ded11-aa64-4a27-b06e-96e221a509d7.json +++ b/persistence/sql/migratest/fixtures/login_flow/916ded11-aa64-4a27-b06e-96e221a509d7.json @@ -1,6 +1,5 @@ { "id": "916ded11-aa64-4a27-b06e-96e221a509d7", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/99974ce6-388c-4669-a95a-7757ee724020.json b/persistence/sql/migratest/fixtures/login_flow/99974ce6-388c-4669-a95a-7757ee724020.json index 18ea717cf9cd..418e16ebe69b 100644 --- a/persistence/sql/migratest/fixtures/login_flow/99974ce6-388c-4669-a95a-7757ee724020.json +++ b/persistence/sql/migratest/fixtures/login_flow/99974ce6-388c-4669-a95a-7757ee724020.json @@ -1,6 +1,5 @@ { "id": "99974ce6-388c-4669-a95a-7757ee724020", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/b1fac7fb-d016-4a06-a7fe-e4eab2a0429f.json b/persistence/sql/migratest/fixtures/login_flow/b1fac7fb-d016-4a06-a7fe-e4eab2a0429f.json index 6909c4aa6502..84eda2f96615 100644 --- a/persistence/sql/migratest/fixtures/login_flow/b1fac7fb-d016-4a06-a7fe-e4eab2a0429f.json +++ b/persistence/sql/migratest/fixtures/login_flow/b1fac7fb-d016-4a06-a7fe-e4eab2a0429f.json @@ -1,6 +1,5 @@ { "id": "b1fac7fb-d016-4a06-a7fe-e4eab2a0429f", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/login_flow/d6aa1f23-88c9-4b9b-a850-392f48c7f9e8.json b/persistence/sql/migratest/fixtures/login_flow/d6aa1f23-88c9-4b9b-a850-392f48c7f9e8.json index d5dfa1ed1436..87ccb1d1dcd0 100644 --- a/persistence/sql/migratest/fixtures/login_flow/d6aa1f23-88c9-4b9b-a850-392f48c7f9e8.json +++ b/persistence/sql/migratest/fixtures/login_flow/d6aa1f23-88c9-4b9b-a850-392f48c7f9e8.json @@ -1,6 +1,5 @@ { "id": "d6aa1f23-88c9-4b9b-a850-392f48c7f9e8", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/05a7f09d-4ef3-41fb-958a-6ad74584b36a.json b/persistence/sql/migratest/fixtures/registration_flow/05a7f09d-4ef3-41fb-958a-6ad74584b36a.json index 2434aecce32b..1e649d64ad51 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/05a7f09d-4ef3-41fb-958a-6ad74584b36a.json +++ b/persistence/sql/migratest/fixtures/registration_flow/05a7f09d-4ef3-41fb-958a-6ad74584b36a.json @@ -1,6 +1,5 @@ { "id": "05a7f09d-4ef3-41fb-958a-6ad74584b36a", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/22d58184-b97d-44a5-bbaf-0aa8b4000d81.json b/persistence/sql/migratest/fixtures/registration_flow/22d58184-b97d-44a5-bbaf-0aa8b4000d81.json index 462728060917..7f90a694387d 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/22d58184-b97d-44a5-bbaf-0aa8b4000d81.json +++ b/persistence/sql/migratest/fixtures/registration_flow/22d58184-b97d-44a5-bbaf-0aa8b4000d81.json @@ -1,6 +1,5 @@ { "id": "22d58184-b97d-44a5-bbaf-0aa8b4000d81", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/2bf132e0-5d40-4df9-9a11-9106e5333735.json b/persistence/sql/migratest/fixtures/registration_flow/2bf132e0-5d40-4df9-9a11-9106e5333735.json index ac6f789af892..dbc832d2aa71 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/2bf132e0-5d40-4df9-9a11-9106e5333735.json +++ b/persistence/sql/migratest/fixtures/registration_flow/2bf132e0-5d40-4df9-9a11-9106e5333735.json @@ -1,6 +1,5 @@ { "id": "2bf132e0-5d40-4df9-9a11-9106e5333735", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/696e7022-c466-44f6-89c6-8cf93c06a62a.json b/persistence/sql/migratest/fixtures/registration_flow/696e7022-c466-44f6-89c6-8cf93c06a62a.json index 76048be0f173..6b627d7541f9 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/696e7022-c466-44f6-89c6-8cf93c06a62a.json +++ b/persistence/sql/migratest/fixtures/registration_flow/696e7022-c466-44f6-89c6-8cf93c06a62a.json @@ -1,6 +1,5 @@ { "id": "696e7022-c466-44f6-89c6-8cf93c06a62a", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/87fa3f43-5155-42b4-a1ad-174c2595fdaf.json b/persistence/sql/migratest/fixtures/registration_flow/87fa3f43-5155-42b4-a1ad-174c2595fdaf.json index 1a41a4488ca3..6a1dcdac29dd 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/87fa3f43-5155-42b4-a1ad-174c2595fdaf.json +++ b/persistence/sql/migratest/fixtures/registration_flow/87fa3f43-5155-42b4-a1ad-174c2595fdaf.json @@ -1,6 +1,5 @@ { "id": "87fa3f43-5155-42b4-a1ad-174c2595fdaf", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/8ef215a9-e8d5-43b3-9aa3-cb4333562e36.json b/persistence/sql/migratest/fixtures/registration_flow/8ef215a9-e8d5-43b3-9aa3-cb4333562e36.json index 91fffa409bae..ed2e8512fde1 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/8ef215a9-e8d5-43b3-9aa3-cb4333562e36.json +++ b/persistence/sql/migratest/fixtures/registration_flow/8ef215a9-e8d5-43b3-9aa3-cb4333562e36.json @@ -1,6 +1,5 @@ { "id": "8ef215a9-e8d5-43b3-9aa3-cb4333562e36", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/8f32efdc-f6fc-4c27-a3c2-579d109eff60.json b/persistence/sql/migratest/fixtures/registration_flow/8f32efdc-f6fc-4c27-a3c2-579d109eff60.json index e6333958d0d4..df3f9c392998 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/8f32efdc-f6fc-4c27-a3c2-579d109eff60.json +++ b/persistence/sql/migratest/fixtures/registration_flow/8f32efdc-f6fc-4c27-a3c2-579d109eff60.json @@ -1,6 +1,5 @@ { "id": "8f32efdc-f6fc-4c27-a3c2-579d109eff60", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd.json b/persistence/sql/migratest/fixtures/registration_flow/9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd.json index fb0dcdcb872a..2195263f1574 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd.json +++ b/persistence/sql/migratest/fixtures/registration_flow/9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd.json @@ -1,6 +1,5 @@ { "id": "9edcf051-1cd0-44cc-bd2f-6ac21f0c24dd", - "oauth2_login_challenge": null, "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/e2150cdc-23ac-4940-a240-6c79c27ab029.json b/persistence/sql/migratest/fixtures/registration_flow/e2150cdc-23ac-4940-a240-6c79c27ab029.json index 20aad6534059..497f88de81b2 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/e2150cdc-23ac-4940-a240-6c79c27ab029.json +++ b/persistence/sql/migratest/fixtures/registration_flow/e2150cdc-23ac-4940-a240-6c79c27ab029.json @@ -1,6 +1,5 @@ { "id": "e2150cdc-23ac-4940-a240-6c79c27ab029", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/ef18b06e-4700-4021-9949-ef783cd86be8.json b/persistence/sql/migratest/fixtures/registration_flow/ef18b06e-4700-4021-9949-ef783cd86be8.json index 350b0c9fcd20..6763bf5c63f5 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/ef18b06e-4700-4021-9949-ef783cd86be8.json +++ b/persistence/sql/migratest/fixtures/registration_flow/ef18b06e-4700-4021-9949-ef783cd86be8.json @@ -1,6 +1,5 @@ { "id": "ef18b06e-4700-4021-9949-ef783cd86be8", - "oauth2_login_challenge": "3caddfd5-9903-4bce-83ff-cae36f42dff7", "type": "browser", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migratest/fixtures/registration_flow/f1b5ed18-113a-4a98-aae7-d4eba007199c.json b/persistence/sql/migratest/fixtures/registration_flow/f1b5ed18-113a-4a98-aae7-d4eba007199c.json index 440430b3f825..d894073c5468 100644 --- a/persistence/sql/migratest/fixtures/registration_flow/f1b5ed18-113a-4a98-aae7-d4eba007199c.json +++ b/persistence/sql/migratest/fixtures/registration_flow/f1b5ed18-113a-4a98-aae7-d4eba007199c.json @@ -1,6 +1,5 @@ { "id": "f1b5ed18-113a-4a98-aae7-d4eba007199c", - "oauth2_login_challenge": null, "type": "api", "expires_at": "2013-10-07T08:23:19Z", "issued_at": "2013-10-07T08:23:19Z", diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql new file mode 100644 index 000000000000..d99fb9ee5a7d --- /dev/null +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql @@ -0,0 +1,5 @@ +ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge UUID NULL; +ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge UUID NULL; + +ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge_data; +ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge_data; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql new file mode 100644 index 000000000000..aa28372db6e3 --- /dev/null +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql @@ -0,0 +1,5 @@ +ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; +ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; + +ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge_data; +ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge_data; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql new file mode 100644 index 000000000000..ca4f960a18af --- /dev/null +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql @@ -0,0 +1,5 @@ +ALTER TABLE `selfservice_login_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; +ALTER TABLE `selfservice_registration_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; + +ALTER TABLE "selfservice_login_flows" DROP COLUMN "oauth2_login_challenge_data"; +ALTER TABLE "selfservice_registration_flows" DROP COLUMN "oauth2_login_challenge_data"; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql new file mode 100644 index 000000000000..aa535361b774 --- /dev/null +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql @@ -0,0 +1,5 @@ +ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge; +ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge; + +ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge_data TEXT NULL; +ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge_data TEXT NULL; diff --git a/selfservice/flow/login/flow.go b/selfservice/flow/login/flow.go index 2401a5256dd7..a0276175e2e4 100644 --- a/selfservice/flow/login/flow.go +++ b/selfservice/flow/login/flow.go @@ -57,7 +57,7 @@ type Flow struct { // // This value is set using the `login_challenge` query parameter of the registration and login endpoints. // If set will cooperate with Ory OAuth2 and OpenID to act as an OAuth2 server / OpenID Provider. - OAuth2LoginChallenge uuid.NullUUID `json:"oauth2_login_challenge,omitempty" faker:"-" db:"oauth2_login_challenge"` + OAuth2LoginChallenge sqlxx.NullString `json:"oauth2_login_challenge,omitempty" faker:"-" db:"oauth2_login_challenge_data"` // HydraLoginRequest is an optional field whose presence indicates that Kratos // is being used as an identity provider in a Hydra OAuth2 flow. Kratos @@ -142,14 +142,14 @@ func NewFlow(conf *config.Config, exp time.Duration, csrf string, r *http.Reques return nil, err } - hlc, err := hydra.GetLoginChallengeID(conf, r) + hydraLoginChallenge, err := hydra.GetLoginChallengeID(conf, r) if err != nil { return nil, err } return &Flow{ ID: id, - OAuth2LoginChallenge: hlc, + OAuth2LoginChallenge: hydraLoginChallenge, ExpiresAt: now.Add(exp), IssuedAt: now, UI: &container.Container{ diff --git a/selfservice/flow/login/flow_test.go b/selfservice/flow/login/flow_test.go index 46832ed2d3f5..1c7f1e200a53 100644 --- a/selfservice/flow/login/flow_test.go +++ b/selfservice/flow/login/flow_test.go @@ -124,7 +124,7 @@ func TestNewFlow(t *testing.T) { r, err := login.NewFlow(conf, 0, "csrf", &http.Request{URL: urlx.ParseOrPanic("https://ory.sh/?login_challenge=8aadcb8fc1334186a84c4da9813356d9"), Host: "ory.sh"}, flow.TypeBrowser) require.NoError(t, err) - assert.Equal(t, "8aadcb8f-c133-4186-a84c-4da9813356d9", r.OAuth2LoginChallenge.UUID.String()) + assert.Equal(t, "8aadcb8fc1334186a84c4da9813356d9", string(r.OAuth2LoginChallenge)) }) } diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index 0da9f5ae915a..5a775535549d 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -15,6 +15,7 @@ import ( "github.com/ory/kratos/hydra" "github.com/ory/kratos/selfservice/sessiontokenexchange" "github.com/ory/kratos/text" + "github.com/ory/x/sqlxx" "github.com/ory/x/stringsx" "github.com/ory/nosurf" @@ -49,7 +50,7 @@ type ( HookExecutorProvider FlowPersistenceProvider errorx.ManagementProvider - hydra.HydraProvider + hydra.Provider StrategyProvider session.HandlerProvider session.ManagementProvider @@ -403,24 +404,26 @@ type createBrowserLoginFlow struct { // 303: emptyResponse // 400: errorGeneric // default: errorGeneric -func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - var hlr *hydraclientgo.OAuth2LoginRequest - var hlc uuid.NullUUID +func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + var ( + hydraLoginRequest *hydraclientgo.OAuth2LoginRequest + hydraLoginChallenge sqlxx.NullString + ) if r.URL.Query().Has("login_challenge") { var err error - hlc, err = hydra.GetLoginChallengeID(h.d.Config(), r) + hydraLoginChallenge, err = hydra.GetLoginChallengeID(h.d.Config(), r) if err != nil { h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) return } - hlr, err = h.d.Hydra().GetLoginRequest(r.Context(), hlc) + hydraLoginRequest, err = h.d.Hydra().GetLoginRequest(r.Context(), string(hydraLoginChallenge)) if err != nil { h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, errors.WithStack(herodot.ErrInternalServerError.WithReason("Failed to retrieve OAuth 2.0 login request."))) return } - if !hlr.GetSkip() { + if !hydraLoginRequest.GetSkip() { q := r.URL.Query() q.Set("refresh", "true") r.URL.RawQuery = q.Encode() @@ -432,23 +435,23 @@ func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request, // different flows, such as login to registration and login to recovery. // After completing a complex flow, such as recovery, we want the user // to be redirected back to the original OAuth2 login flow. - if hlr.RequestUrl != "" && h.d.Config().OAuth2ProviderOverrideReturnTo(r.Context()) { + if hydraLoginRequest.RequestUrl != "" && h.d.Config().OAuth2ProviderOverrideReturnTo(r.Context()) { // replace the return_to query parameter q := r.URL.Query() - q.Set("return_to", hlr.RequestUrl) + q.Set("return_to", hydraLoginRequest.RequestUrl) r.URL.RawQuery = q.Encode() } } a, sess, err := h.NewLoginFlow(w, r, flow.TypeBrowser) if errors.Is(err, ErrAlreadyLoggedIn) { - if hlr != nil { - if !hlr.GetSkip() { + if hydraLoginRequest != nil { + if !hydraLoginRequest.GetSkip() { h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, errors.WithStack(herodot.ErrInternalServerError.WithReason("ErrAlreadyLoggedIn indicated we can skip login, but Hydra asked us to refresh"))) return } - rt, err := h.d.Hydra().AcceptLoginRequest(r.Context(), hlc.UUID, sess.IdentityID.String(), sess.AMR) + rt, err := h.d.Hydra().AcceptLoginRequest(r.Context(), string(hydraLoginChallenge), sess.IdentityID.String(), sess.AMR) if err != nil { h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) @@ -479,7 +482,7 @@ func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request, return } - a.HydraLoginRequest = hlr + a.HydraLoginRequest = hydraLoginRequest x.AcceptToRedirectOrJSON(w, r, h.d.Writer(), a, a.AppendTo(h.d.Config().SelfServiceFlowLoginUI(r.Context())).String()) } @@ -580,8 +583,8 @@ func (h *Handler) getLoginFlow(w http.ResponseWriter, r *http.Request, _ httprou return } - if ar.OAuth2LoginChallenge.Valid { - hlr, err := h.d.Hydra().GetLoginRequest(r.Context(), ar.OAuth2LoginChallenge) + if ar.OAuth2LoginChallenge != "" { + hlr, err := h.d.Hydra().GetLoginRequest(r.Context(), string(ar.OAuth2LoginChallenge)) if err != nil { // We don't redirect back to the third party on errors because Hydra doesn't // give us the 3rd party return_uri when it redirects to the login UI. diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 8405d6b7f04e..2d56b2655a6c 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -45,7 +45,7 @@ type ( type ( executorDependencies interface { config.Provider - hydra.HydraProvider + hydra.Provider session.ManagementProvider session.PersistenceProvider x.CSRFTokenGeneratorProvider @@ -261,8 +261,8 @@ func (e *HookExecutor) PostLoginHook( } finalReturnTo := returnTo.String() - if a.OAuth2LoginChallenge.Valid { - rt, err := e.d.Hydra().AcceptLoginRequest(r.Context(), a.OAuth2LoginChallenge.UUID, i.ID.String(), s.AMR) + if a.OAuth2LoginChallenge != "" { + rt, err := e.d.Hydra().AcceptLoginRequest(r.Context(), string(a.OAuth2LoginChallenge), i.ID.String(), s.AMR) if err != nil { return err } diff --git a/selfservice/flow/registration/flow.go b/selfservice/flow/registration/flow.go index da6eb07df18f..26c748760ce2 100644 --- a/selfservice/flow/registration/flow.go +++ b/selfservice/flow/registration/flow.go @@ -44,7 +44,7 @@ type Flow struct { // // This value is set using the `login_challenge` query parameter of the registration and login endpoints. // If set will cooperate with Ory OAuth2 and OpenID to act as an OAuth2 server / OpenID Provider. - OAuth2LoginChallenge uuid.NullUUID `json:"oauth2_login_challenge,omitempty" faker:"-" db:"oauth2_login_challenge"` + OAuth2LoginChallenge sqlxx.NullString `json:"oauth2_login_challenge,omitempty" faker:"-" db:"oauth2_login_challenge_data"` // HydraLoginRequest is an optional field whose presence indicates that Kratos // is being used as an identity provider in a Hydra OAuth2 flow. Kratos diff --git a/selfservice/flow/registration/flow_test.go b/selfservice/flow/registration/flow_test.go index 6a9c283541a5..d72e0c437921 100644 --- a/selfservice/flow/registration/flow_test.go +++ b/selfservice/flow/registration/flow_test.go @@ -91,7 +91,7 @@ func TestNewFlow(t *testing.T) { r, err := registration.NewFlow(conf, 0, "csrf", &http.Request{URL: urlx.ParseOrPanic("https://ory.sh/?login_challenge=8aadcb8fc1334186a84c4da9813356d9"), Host: "ory.sh"}, flow.TypeBrowser) require.NoError(t, err) - assert.Equal(t, "8aadcb8f-c133-4186-a84c-4da9813356d9", r.OAuth2LoginChallenge.UUID.String()) + assert.Equal(t, "8aadcb8fc1334186a84c4da9813356d9", string(r.OAuth2LoginChallenge)) }) } diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go index 8a76d02521e2..66c3f0fe9389 100644 --- a/selfservice/flow/registration/handler.go +++ b/selfservice/flow/registration/handler.go @@ -45,7 +45,7 @@ type ( handlerDependencies interface { config.Provider errorx.ManagementProvider - hydra.HydraProvider + hydra.Provider session.HandlerProvider session.ManagementProvider x.WriterProvider @@ -449,8 +449,8 @@ func (h *Handler) getRegistrationFlow(w http.ResponseWriter, r *http.Request, ps return } - if ar.OAuth2LoginChallenge.Valid { - hlr, err := h.d.Hydra().GetLoginRequest(r.Context(), ar.OAuth2LoginChallenge) + if ar.OAuth2LoginChallenge != "" { + hlr, err := h.d.Hydra().GetLoginRequest(r.Context(), string(ar.OAuth2LoginChallenge)) if err != nil { // We don't redirect back to the third party on errors because Hydra doesn't // give us the 3rd party return_uri when it redirects to the login UI. diff --git a/selfservice/flow/registration/hook.go b/selfservice/flow/registration/hook.go index c220e31a9871..78f9eb19fb3f 100644 --- a/selfservice/flow/registration/hook.go +++ b/selfservice/flow/registration/hook.go @@ -77,7 +77,7 @@ type ( session.PersistenceProvider session.ManagementProvider HooksProvider - hydra.HydraProvider + hydra.Provider x.CSRFTokenGeneratorProvider x.HTTPClientProvider x.LoggingProvider @@ -247,8 +247,8 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque } finalReturnTo := returnTo.String() - if a.OAuth2LoginChallenge.Valid { - cr, err := e.d.Hydra().AcceptLoginRequest(r.Context(), a.OAuth2LoginChallenge.UUID, i.ID.String(), s.AMR) + if a.OAuth2LoginChallenge != "" { + cr, err := e.d.Hydra().AcceptLoginRequest(r.Context(), string(a.OAuth2LoginChallenge), i.ID.String(), s.AMR) if err != nil { return err } diff --git a/selfservice/strategy/password/op_login_test.go b/selfservice/strategy/password/op_login_test.go index 35136a399b44..0c4c5991ecd1 100644 --- a/selfservice/strategy/password/op_login_test.go +++ b/selfservice/strategy/password/op_login_test.go @@ -340,11 +340,11 @@ type AcceptWrongSubject struct { h *hydra.DefaultHydra } -func (h *AcceptWrongSubject) AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, sub string, amr session.AuthenticationMethods) (string, error) { +func (h *AcceptWrongSubject) AcceptLoginRequest(ctx context.Context, loginChallenge string, subject string, amr session.AuthenticationMethods) (string, error) { hackerman := uuid.Must(uuid.NewV4()) - return h.h.AcceptLoginRequest(ctx, hlc, hackerman.String(), amr) + return h.h.AcceptLoginRequest(ctx, loginChallenge, hackerman.String(), amr) } -func (h *AcceptWrongSubject) GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) { - return h.h.GetLoginRequest(ctx, hlc) +func (h *AcceptWrongSubject) GetLoginRequest(ctx context.Context, loginChallenge string) (*hydraclientgo.OAuth2LoginRequest, error) { + return h.h.GetLoginRequest(ctx, loginChallenge) } diff --git a/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=browser.json b/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=browser.json index e8ec8c428d08..0a7494871340 100644 --- a/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=browser.json +++ b/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=browser.json @@ -1,5 +1,4 @@ { - "oauth2_login_challenge": null, "type": "browser", "ui": { "method": "POST", diff --git a/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=spa.json b/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=spa.json index e8ec8c428d08..0a7494871340 100644 --- a/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=spa.json +++ b/selfservice/strategy/webauthn/.snapshots/TestCompleteLogin-flow=passwordless-case=should_fail_if_webauthn_login_is_invalid-type=spa.json @@ -1,5 +1,4 @@ { - "oauth2_login_challenge": null, "type": "browser", "ui": { "method": "POST", From 794ac6a7b3d85c9980d9ad480f8aebbd5958d943 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Wed, 14 Jun 2023 21:05:55 +0200 Subject: [PATCH 2/4] fix: don't assume the login challenge to be a UUID For compatibility with https://github.com/ory/hydra/pull/3515, which now encodes the whole flow in the login challenge, we cannot further assume that the challenge is a UUID. --- hydra/fake.go | 2 +- hydra/hydra.go | 7 +++- hydra/hydra_test.go | 9 +++++ .../cccccccc-dda4-4700-9e42-35731f2af91e.json | 17 +++++++++ .../testdata/20230614205200_testdata.sql | 8 ++++ selfservice/flow/login/hook.go | 4 +- selfservice/flow/login/hook_test.go | 5 +-- .../profiles/oidc-provider/error.spec.ts | 37 ------------------- 8 files changed, 45 insertions(+), 44 deletions(-) create mode 100644 persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json create mode 100644 persistence/sql/migratest/testdata/20230614205200_testdata.sql delete mode 100644 test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts diff --git a/hydra/fake.go b/hydra/fake.go index 1e4a71ffe2c4..3d1bf852c97d 100644 --- a/hydra/fake.go +++ b/hydra/fake.go @@ -27,7 +27,7 @@ func NewFake() *FakeHydra { return &FakeHydra{} } - func (h *FakeHydra) AcceptLoginRequest(_ context.Context, loginChallenge string, _ string, _ session.AuthenticationMethods) (string, error) { +func (h *FakeHydra) AcceptLoginRequest(_ context.Context, loginChallenge string, _ string, _ session.AuthenticationMethods) (string, error) { switch loginChallenge { case FakeInvalidLoginChallenge: return "", ErrFakeAcceptLoginRequestFailed diff --git a/hydra/hydra.go b/hydra/hydra.go index e26d32d77834..695eeed55e91 100644 --- a/hydra/hydra.go +++ b/hydra/hydra.go @@ -50,7 +50,12 @@ func GetLoginChallengeID(conf *config.Config, r *http.Request) (sqlxx.NullString return "", errors.WithStack(herodot.ErrInternalServerError.WithReason("refusing to parse login_challenge query parameter because " + config.ViperKeyOAuth2ProviderURL + " is invalid or unset")) } - return sqlxx.NullString(r.URL.Query().Get("login_challenge")), nil + loginChallenge := r.URL.Query().Get("login_challenge") + if loginChallenge == "" { + return "", errors.WithStack(herodot.ErrBadRequest.WithReason("the login_challenge parameter is present but empty")) + } + + return sqlxx.NullString(loginChallenge), nil } func (h *DefaultHydra) getAdminURL(ctx context.Context) (string, error) { diff --git a/hydra/hydra_test.go b/hydra/hydra_test.go index 49f99c38957e..c00ec8bf2f4b 100644 --- a/hydra/hydra_test.go +++ b/hydra/hydra_test.go @@ -64,6 +64,15 @@ func TestGetLoginChallengeID(t *testing.T) { want: "", assertErr: assert.NoError, }, + { + name: "empty login challenge; hydra is configured", + args: args{ + conf: configWithHydra, + r: requestFromChallenge(""), + }, + want: "", + assertErr: assert.Error, + }, { name: "login_challenge is present; Hydra is not configured", args: args{ diff --git a/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json b/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json new file mode 100644 index 000000000000..438fb4005e14 --- /dev/null +++ b/persistence/sql/migratest/fixtures/login_flow/cccccccc-dda4-4700-9e42-35731f2af91e.json @@ -0,0 +1,17 @@ +{ + "id": "cccccccc-dda4-4700-9e42-35731f2af91e", + "oauth2_login_challenge": "challenge data", + "type": "api", + "expires_at": "2013-10-07T08:23:19Z", + "issued_at": "2013-10-07T08:23:19Z", + "request_url": "http://kratos:4433/self-service/browser/flows/login", + "ui": { + "action": "", + "method": "", + "nodes": null + }, + "created_at": "2013-10-07T08:23:19Z", + "updated_at": "2013-10-07T08:23:19Z", + "refresh": false, + "requested_aal": "aal1" +} diff --git a/persistence/sql/migratest/testdata/20230614205200_testdata.sql b/persistence/sql/migratest/testdata/20230614205200_testdata.sql new file mode 100644 index 000000000000..c047c59a3115 --- /dev/null +++ b/persistence/sql/migratest/testdata/20230614205200_testdata.sql @@ -0,0 +1,8 @@ +INSERT INTO selfservice_login_flows (id, nid, request_url, issued_at, expires_at, active_method, csrf_token, created_at, + updated_at, forced, type, ui, internal_context, oauth2_login_challenge_data) +VALUES ('cccccccc-dda4-4700-9e42-35731f2af91e', + '884f556e-eb3a-4b9f-bee3-11345642c6c0', + 'http://kratos:4433/self-service/browser/flows/login', + '2013-10-07 08:23:19', '2013-10-07 08:23:19', '', + 'fpeVSZ9ZH7YvUkhXsOVEIssxbfauh5lcoQSYxTcN0XkMneg1L42h+HtvisjlNjBF4ElcD2jApCHoJYq2u9sVWg==', + '2013-10-07 08:23:19', '2013-10-07 08:23:19', false, 'api', '{}', '{"foo":"bar"}', 'challenge data'); diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index 2d56b2655a6c..4dff5b989b86 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -233,8 +233,8 @@ func (e *HookExecutor) PostLoginHook( // If Kratos is used as a Hydra login provider, we need to redirect back to Hydra by returning a 422 status // with the post login challenge URL as the body. - if a.OAuth2LoginChallenge.Valid { - postChallengeURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(), a.OAuth2LoginChallenge.UUID, i.ID.String(), s.AMR) + if a.OAuth2LoginChallenge != "" { + postChallengeURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(), string(a.OAuth2LoginChallenge), i.ID.String(), s.AMR) if err != nil { return err } diff --git a/selfservice/flow/login/hook_test.go b/selfservice/flow/login/hook_test.go index a47c594fc18e..cfffc34701f0 100644 --- a/selfservice/flow/login/hook_test.go +++ b/selfservice/flow/login/hook_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/gofrs/uuid" "github.com/stretchr/testify/require" "github.com/ory/kratos/hydra" @@ -167,7 +166,7 @@ func TestLoginExecutor(t *testing.T) { t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) withOAuthChallenge := func(f *login.Flow) { - f.OAuth2LoginChallenge = uuid.NullUUID{Valid: true, UUID: uuid.Must(uuid.FromString(hydra.FakeValidLoginChallenge))} + f.OAuth2LoginChallenge = hydra.FakeValidLoginChallenge } res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil, withOAuthChallenge), true, url.Values{}) assert.EqualValues(t, http.StatusUnprocessableEntity, res.StatusCode) @@ -178,7 +177,7 @@ func TestLoginExecutor(t *testing.T) { t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) withOAuthChallenge := func(f *login.Flow) { - f.OAuth2LoginChallenge = uuid.NullUUID{Valid: true, UUID: uuid.Must(uuid.FromString(hydra.FakeInvalidLoginChallenge))} + f.OAuth2LoginChallenge = hydra.FakeInvalidLoginChallenge } res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil, withOAuthChallenge), true, url.Values{}) assert.EqualValues(t, http.StatusInternalServerError, res.StatusCode) diff --git a/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts b/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts deleted file mode 100644 index ec5a6321718f..000000000000 --- a/test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright © 2023 Ory Corp -// SPDX-License-Identifier: Apache-2.0 - -import { routes as express } from "../../../helpers/express" - -context("OpenID Provider", () => { - before(() => { - cy.useConfigProfile("oidc-provider") - cy.proxy("express") - }) - it("should fail with invalid login_challenge", () => { - cy.visit(express.login + "?login_challenge=not-a-uuid", { - failOnStatusCode: false, - }).then((d) => { - cy.get(`[data-testid="ui/error/message"]`).then((c) => { - cy.wrap(c[0].textContent).should( - "contain", - "the login_challenge parameter is present but invalid or zero UUID", - ) - }) - }) - }) - - it("should fail with zero login_challenge", () => { - cy.visit( - express.login + "?login_challenge=00000000-0000-0000-0000-000000000000", - { failOnStatusCode: false }, - ).then((d) => { - cy.get(`[data-testid="ui/error/message"]`).then((c) => { - cy.wrap(c[0].textContent).should( - "contain", - "the login_challenge parameter is present but invalid or zero UUID", - ) - }) - }) - }) -}) From c368567b2c21e677882193153d7f0f6eb2cf8866 Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Sat, 17 Jun 2023 10:36:36 +0200 Subject: [PATCH 3/4] Update hydra/hydra_test.go Co-authored-by: Patrik --- hydra/hydra_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hydra/hydra_test.go b/hydra/hydra_test.go index c00ec8bf2f4b..b2e252be5b18 100644 --- a/hydra/hydra_test.go +++ b/hydra/hydra_test.go @@ -92,7 +92,7 @@ func TestGetLoginChallengeID(t *testing.T) { assertErr: assert.NoError, }, { - name: "login_challenge is invalid; hydra is configured", + name: "login_challenge is present & non-uuid; hydra is configured", args: args{ conf: configWithHydra, r: requestFromChallenge(blobChallenge), From 5871f812e06b9c19edff47d437196dca813d9bc6 Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Mon, 19 Jun 2023 10:41:09 +0200 Subject: [PATCH 4/4] fix: defer dropping the old column --- ...0230614000001000000_hydra_login_challenge_format.down.sql | 4 ++-- ...4000001000000_hydra_login_challenge_format.mysql.down.sql | 4 ++-- ...00001000000_hydra_login_challenge_format.sqlite3.down.sql | 4 ++-- .../20230614000001000000_hydra_login_challenge_format.up.sql | 5 +++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql index d99fb9ee5a7d..e4207a2c71bd 100644 --- a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.down.sql @@ -1,5 +1,5 @@ -ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge UUID NULL; -ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge UUID NULL; +-- ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge UUID NULL; +-- ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge UUID NULL; ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge_data; ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge_data; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql index aa28372db6e3..d3d8b4f24a94 100644 --- a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.mysql.down.sql @@ -1,5 +1,5 @@ -ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; -ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; +-- ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; +-- ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge CHAR(36) NULL; ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge_data; ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge_data; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql index ca4f960a18af..1a251ee051ba 100644 --- a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.sqlite3.down.sql @@ -1,5 +1,5 @@ -ALTER TABLE `selfservice_login_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; -ALTER TABLE `selfservice_registration_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; +-- ALTER TABLE `selfservice_login_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; +-- ALTER TABLE `selfservice_registration_flows` ADD COLUMN `oauth2_login_challenge` CHAR(36) NULL; ALTER TABLE "selfservice_login_flows" DROP COLUMN "oauth2_login_challenge_data"; ALTER TABLE "selfservice_registration_flows" DROP COLUMN "oauth2_login_challenge_data"; diff --git a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql index aa535361b774..70785b738a52 100644 --- a/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql +++ b/persistence/sql/migrations/sql/20230614000001000000_hydra_login_challenge_format.up.sql @@ -1,5 +1,6 @@ -ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge; -ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge; +-- Drop columns in a later migration. +-- ALTER TABLE selfservice_login_flows DROP COLUMN oauth2_login_challenge; +-- ALTER TABLE selfservice_registration_flows DROP COLUMN oauth2_login_challenge; ALTER TABLE selfservice_login_flows ADD COLUMN oauth2_login_challenge_data TEXT NULL; ALTER TABLE selfservice_registration_flows ADD COLUMN oauth2_login_challenge_data TEXT NULL;