fix: don't assume the login challenge to be a UUID (#3317)

For compatibility with ory/hydra#3515, which now encodes the whole flow in the login challenge, we cannot further assume that the challenge is a UUID.
ory · Jun 19, 2023 · 3172862 · 3172862
1 parent 3485204
commit 3172862
Show file tree

Hide file tree

Showing 47 changed files with 148 additions and 159 deletions.
diff --git 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,26 +27,26 @@ 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:
 		return &hydraclientgo.OAuth2LoginRequest{
 			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
@@ -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,19 @@ 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
+	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) {
@@ -85,11 +84,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 +101,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 +125,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 +135,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
@@ -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,67 +41,71 @@ 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",
 			args: args{
 				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",
 			args: args{
 				conf: configWithHydra,
 				r:    &http.Request{URL: urlx.ParseOrPanic("https://hydra")},
 			},
-			want:    uuid.NullUUID{Valid: false},
-			wantErr: false,
+			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{
 				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",
+			name: "login_challenge is present & non-uuid; 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/cccccccc-dda4-4700-9e42-35731f2af91e.json 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/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/...stence/sql/migratest/fixtures/registration_flow/05a7f09d-4ef3-41fb-958a-6ad74584b36a.json b/...stence/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",