fix: registration should accept hydra login (#3592)

* fix: registration should accept hydra login * fix: oauth2 registration flow with session * wip: registration oauth flow tests * wip: refactor oauth flows test * wip: refactor op_registration_test * wip: oauth provider registration test * wip: refactor oauth flows test * fix(test): oauth provider login * style: format
ory · Oct 30, 2023 · 7a47827 · 7a47827
1 parent 985474c
commit 7a47827
Show file tree

Hide file tree

Showing 10 changed files with 1,986 additions and 465 deletions.
diff --git a/hydra/fake.go b/hydra/fake.go
@@ -19,12 +19,17 @@ const (
 
 var ErrFakeAcceptLoginRequestFailed = errors.New("failed to accept login request")
 
-type FakeHydra struct{}
+type FakeHydra struct {
+	Skip       bool
+	RequestURL string
+}
 
 var _ Hydra = &FakeHydra{}
 
 func NewFake() *FakeHydra {
-	return &FakeHydra{}
+	return &FakeHydra{
+		RequestURL: "https://www.ory.sh",
+	}
 }
 
 func (h *FakeHydra) AcceptLoginRequest(_ context.Context, params AcceptLoginRequestParams) (string, error) {
@@ -47,7 +52,8 @@ func (h *FakeHydra) GetLoginRequest(_ context.Context, loginChallenge string) (*
 		return nil, herodot.ErrBadRequest.WithReasonf("Unable to get OAuth 2.0 Login Challenge.")
 	case FakeValidLoginChallenge:
 		return &hydraclientgo.OAuth2LoginRequest{
-			RequestUrl: "https://www.ory.sh",
+			RequestUrl: h.RequestURL,
+			Skip:       h.Skip,
 		}, nil
 	default:
 		panic("unknown fake login_challenge " + loginChallenge)

diff --git a/internal/testhelpers/selfservice_login.go b/internal/testhelpers/selfservice_login.go
@@ -193,6 +193,13 @@ func LoginMakeRequest(
 	return string(ioutilx.MustReadAll(res.Body)), res
 }
 
+func GetLoginFlow(t *testing.T, client *http.Client, ts *httptest.Server, flowID string) *kratos.LoginFlow {
+	publicClient := NewSDKCustomClient(ts, client)
+	rs, _, err := publicClient.FrontendApi.GetLoginFlow(context.Background()).Id(flowID).Execute()
+	require.NoError(t, err)
+	return rs
+}
+
 // SubmitLoginForm initiates a login flow (for Browser and API!), fills out the form and modifies
 // the form values with `withValues`, and submits the form. Returns the body and checks for expectedStatusCode and
 // expectedURL on completion

diff --git a/internal/testhelpers/selfservice_registration.go b/internal/testhelpers/selfservice_registration.go
@@ -83,6 +83,13 @@ func InitializeRegistrationFlowViaAPI(t *testing.T, client *http.Client, ts *htt
 	return rs
 }
 
+func GetRegistrationFlow(t *testing.T, client *http.Client, ts *httptest.Server, flowID string) *kratos.RegistrationFlow {
+	rs, _, err := NewSDKCustomClient(ts, client).FrontendApi.GetRegistrationFlow(context.Background()).Id(flowID).Execute()
+	require.NoError(t, err)
+	assert.Empty(t, rs.Active)
+	return rs
+}
+
 func RegistrationMakeRequest(
 	t *testing.T,
 	isAPI bool,

diff --git a/selfservice/flow/registration/flow_test.go b/selfservice/flow/registration/flow_test.go
@@ -68,7 +68,8 @@ func TestNewFlow(t *testing.T) {
 	t.Run("case=1", func(t *testing.T) {
 		r, err := registration.NewFlow(conf, 0, "csrf", &http.Request{
 			URL:  urlx.ParseOrPanic("/?refresh=true"),
-			Host: "ory.sh"}, flow.TypeAPI)
+			Host: "ory.sh",
+		}, flow.TypeAPI)
 		require.NoError(t, err)
 		assert.Equal(t, r.IssuedAt, r.ExpiresAt)
 		assert.Equal(t, flow.TypeAPI, r.Type)
@@ -78,7 +79,8 @@ func TestNewFlow(t *testing.T) {
 	t.Run("case=2", func(t *testing.T) {
 		r, err := registration.NewFlow(conf, 0, "csrf", &http.Request{
 			URL:  urlx.ParseOrPanic("https://ory.sh/"),
-			Host: "ory.sh"}, flow.TypeBrowser)
+			Host: "ory.sh",
+		}, flow.TypeBrowser)
 		require.NoError(t, err)
 		assert.Equal(t, "https://ory.sh/", r.RequestURL)
 	})

diff --git a/selfservice/flow/registration/handler.go b/selfservice/flow/registration/handler.go
@@ -27,10 +27,10 @@ import (
 	"github.com/ory/x/sqlxx"
 	"github.com/ory/x/urlx"
 
+	hydraclientgo "github.com/ory/hydra-client-go/v2"
 	"github.com/ory/kratos/driver/config"
 	"github.com/ory/kratos/selfservice/errorx"
 	"github.com/ory/kratos/selfservice/flow"
-	"github.com/ory/kratos/selfservice/flow/logout"
 	"github.com/ory/kratos/session"
 	"github.com/ory/kratos/x"
 )
@@ -318,27 +318,78 @@ type createBrowserRegistrationFlow struct {
 //	  303: emptyResponse
 //	  default: errorGeneric
 func (h *Handler) createBrowserRegistrationFlow(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
+	ctx := r.Context()
+
 	a, err := h.NewRegistrationFlow(w, r, flow.TypeBrowser)
 	if err != nil {
-		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+		h.d.SelfServiceErrorManager().Forward(ctx, w, r, err)
 		return
 	}
 
-	if sess, err := h.d.SessionManager().FetchFromRequest(r.Context(), r); err == nil {
-		if r.URL.Query().Has("login_challenge") {
-			logoutUrl := urlx.AppendPaths(h.d.Config().SelfPublicURL(r.Context()), logout.RouteSubmitFlow)
-			self := urlx.CopyWithQuery(
-				urlx.AppendPaths(h.d.Config().SelfPublicURL(r.Context()), RouteInitBrowserFlow),
-				r.URL.Query(),
-			).String()
+	var (
+		hydraLoginRequest   *hydraclientgo.OAuth2LoginRequest
+		hydraLoginChallenge sqlxx.NullString
+	)
+	if r.URL.Query().Has("login_challenge") {
+		var err error
+		hydraLoginChallenge, err = hydra.GetLoginChallengeID(h.d.Config(), r)
+		if err != nil {
+			h.d.SelfServiceErrorManager().Forward(ctx, w, r, err)
+			return
+		}
+
+		hydraLoginRequest, err = h.d.Hydra().GetLoginRequest(ctx, hydraLoginChallenge.String())
+		if err != nil {
+			h.d.SelfServiceErrorManager().Forward(ctx, w, r, err)
+			return
+		}
+
+		// on OAuth2 flows, we need to use the RequestURL
+		// as the ReturnTo URL.
+		// This is because a user might want to switch between
+		// 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 hydraLoginRequest.RequestUrl != "" && h.d.Config().OAuth2ProviderOverrideReturnTo(r.Context()) {
+			q := r.URL.Query()
+			// replace the return_to query parameter
+			q.Set("return_to", hydraLoginRequest.RequestUrl)
+			r.URL.RawQuery = q.Encode()
+		}
+	}
 
+	if sess, err := h.d.SessionManager().FetchFromRequest(ctx, r); err == nil {
+		if hydraLoginRequest != nil {
+			if hydraLoginRequest.GetSkip() {
+				rt, err := h.d.Hydra().AcceptLoginRequest(r.Context(),
+					hydra.AcceptLoginRequestParams{
+						LoginChallenge:        string(hydraLoginChallenge),
+						IdentityID:            sess.IdentityID.String(),
+						SessionID:             sess.ID.String(),
+						AuthenticationMethods: sess.AMR,
+					})
+				if err != nil {
+					h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+					return
+				}
+				returnTo, err := url.Parse(rt)
+				if err != nil {
+					h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to parse URL: %s", rt)))
+					return
+				}
+				x.AcceptToRedirectOrJSON(w, r, h.d.Writer(), err, returnTo.String())
+				return
+			}
+
+			// hydra indicates that we cannot skip the login request
+			// so we must perform the login flow.
+			// we directly go to the login handler from here
+			// copy over any query parameters, such as `return_to` and `login_challenge`
+			loginURL := urlx.CopyWithQuery(urlx.AppendPaths(h.d.Config().SelfPublicURL(ctx), "/self-service/login/browser"), x.RequestURL(r).Query())
 			http.Redirect(
 				w,
 				r,
-				urlx.CopyWithQuery(logoutUrl, url.Values{
-					"token":     {sess.LogoutToken},
-					"return_to": {self},
-				}).String(),
+				loginURL.String(),
 				http.StatusFound,
 			)
 			return
@@ -349,20 +400,20 @@ func (h *Handler) createBrowserRegistrationFlow(w http.ResponseWriter, r *http.R
 			return
 		}
 
-		returnTo, redirErr := x.SecureRedirectTo(r, h.d.Config().SelfServiceBrowserDefaultReturnTo(r.Context()),
-			x.SecureRedirectAllowSelfServiceURLs(h.d.Config().SelfPublicURL(r.Context())),
-			x.SecureRedirectAllowURLs(h.d.Config().SelfServiceBrowserAllowedReturnToDomains(r.Context())),
+		returnTo, redirErr := x.SecureRedirectTo(r, h.d.Config().SelfServiceBrowserDefaultReturnTo(ctx),
+			x.SecureRedirectAllowSelfServiceURLs(h.d.Config().SelfPublicURL(ctx)),
+			x.SecureRedirectAllowURLs(h.d.Config().SelfServiceBrowserAllowedReturnToDomains(ctx)),
 		)
 		if redirErr != nil {
-			h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, redirErr)
+			h.d.SelfServiceErrorManager().Forward(ctx, w, r, redirErr)
 			return
 		}
 
 		http.Redirect(w, r, returnTo.String(), http.StatusSeeOther)
 		return
 	}
 
-	redirTo := a.AppendTo(h.d.Config().SelfServiceFlowRegistrationUI(r.Context())).String()
+	redirTo := a.AppendTo(h.d.Config().SelfServiceFlowRegistrationUI(ctx)).String()
 	x.AcceptToRedirectOrJSON(w, r, h.d.Writer(), a, redirTo)
 }
 

diff --git a/selfservice/flow/registration/handler_test.go b/selfservice/flow/registration/handler_test.go
@@ -20,6 +20,7 @@ import (
 	"github.com/gofrs/uuid"
 
 	"github.com/ory/kratos/corpx"
+	"github.com/ory/kratos/hydra"
 	"github.com/ory/x/urlx"
 
 	"github.com/stretchr/testify/assert"
@@ -32,6 +33,7 @@ import (
 	"github.com/ory/kratos/identity"
 	"github.com/ory/kratos/internal"
 	"github.com/ory/kratos/internal/testhelpers"
+	"github.com/ory/kratos/selfservice/flow/login"
 	"github.com/ory/kratos/selfservice/flow/registration"
 	"github.com/ory/kratos/selfservice/strategy/oidc"
 	"github.com/ory/kratos/selfservice/strategy/password"
@@ -45,6 +47,8 @@ func init() {
 func TestHandlerRedirectOnAuthenticated(t *testing.T) {
 	ctx := context.Background()
 	conf, reg := internal.NewFastRegistryWithMocks(t)
+	fakeHydra := hydra.NewFake()
+	reg.WithHydra(fakeHydra)
 
 	router := x.NewRouterPublic()
 	ts, _ := testhelpers.NewKratosServerWithRouters(t, reg, router, x.NewRouterAdmin())
@@ -74,6 +78,34 @@ func TestHandlerRedirectOnAuthenticated(t *testing.T) {
 		assert.Contains(t, res.Request.URL.String(), returnToTS.URL)
 		assert.EqualValues(t, "return_to", string(body))
 	})
+
+	t.Run("oauth2 with session and skip=false is redirected to login", func(t *testing.T) {
+		conf.MustSet(ctx, config.ViperKeyOAuth2ProviderURL, "https://fake-hydra")
+
+		fakeHydra.RequestURL = "https://www.ory.sh/oauth2/auth?audience=&client_id=foo&login_verifier="
+		fakeHydra.Skip = false
+
+		client := testhelpers.NewClientWithCookies(t)
+		client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		}
+		_, res := testhelpers.MockMakeAuthenticatedRequestWithClient(t, reg, conf, router.Router, testhelpers.NewTestHTTPRequest(t, "GET", ts.URL+registration.RouteInitBrowserFlow+"?login_challenge="+hydra.FakeValidLoginChallenge, nil), client)
+		assert.Contains(t, res.Header.Get("location"), login.RouteInitBrowserFlow)
+	})
+
+	t.Run("oauth2 with session and skip=true is accepted", func(t *testing.T) {
+		conf.MustSet(ctx, config.ViperKeyOAuth2ProviderURL, "https://fake-hydra")
+
+		fakeHydra.Skip = true
+		fakeHydra.RequestURL = "https://www.ory.sh/oauth2/auth?audience=&client_id=foo&login_verifier="
+
+		client := testhelpers.NewClientWithCookies(t)
+		client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		}
+		_, res := testhelpers.MockMakeAuthenticatedRequestWithClient(t, reg, conf, router.Router, testhelpers.NewTestHTTPRequest(t, "GET", ts.URL+registration.RouteInitBrowserFlow+"?login_challenge="+hydra.FakeValidLoginChallenge, nil), client)
+		assert.Contains(t, res.Header.Get("location"), hydra.FakePostLoginURL)
+	})
 }
 
 func TestInitFlow(t *testing.T) {