feat: return to oauth flow after switching from login to other flows (#…

…3212) * feat: return to oauth flow after switching from login to other flows * feat(e2e): flows should have return_to set to hydra request_url * u * fix: override return_to URL on OAuth flows * style: format * fix: TestOAuth2Provider * feat: config to opt into using OAuth request url as return_to * chore: cleanup * fix(e2e): oauth2 login flow switching to recovery * feat(test): oauth2 login flow to recovery through oidc provider * fix(e2e): oidc-provider registration * chore: rename `oauth2_provider.return_to_enabled` to `oauth2_provider.override_return_to` * style: format * chore: nit config description Co-authored-by: Jonas Hungershausen <[email protected]> --------- Co-authored-by: Jonas Hungershausen <[email protected]>
ory · May 3, 2023 · a1fea6c · a1fea6c
1 parent 45485c3
commit a1fea6c
Show file tree

Hide file tree

Showing 14 changed files with 318 additions and 12 deletions.
diff --git a/driver/config/config.go b/driver/config/config.go
@@ -180,6 +180,7 @@ const (
 	ViperKeyWebAuthnPasswordless                             = "selfservice.methods.webauthn.config.passwordless"
 	ViperKeyOAuth2ProviderURL                                = "oauth2_provider.url"
 	ViperKeyOAuth2ProviderHeader                             = "oauth2_provider.headers"
+	ViperKeyOAuth2ProviderOverrideReturnTo                   = "oauth2_provider.override_return_to"
 	ViperKeyClientHTTPNoPrivateIPRanges                      = "clients.http.disallow_private_ip_ranges"
 	ViperKeyClientHTTPPrivateIPExceptionURLs                 = "clients.http.private_ip_exception_urls"
 	ViperKeyVersion                                          = "version"
@@ -884,6 +885,10 @@ func (p *Config) OAuth2ProviderHeader(ctx context.Context) http.Header {
 	return h
 }
 
+func (p *Config) OAuth2ProviderOverrideReturnTo(ctx context.Context) bool {
+	return p.GetProvider(ctx).Bool(ViperKeyOAuth2ProviderOverrideReturnTo)
+}
+
 func (p *Config) OAuth2ProviderURL(ctx context.Context) *url.URL {
 	k := ViperKeyOAuth2ProviderURL
 	v := p.GetProvider(ctx).String(k)

diff --git a/driver/config/config_test.go b/driver/config/config_test.go
@@ -1166,12 +1166,14 @@ func TestOAuth2Provider(t *testing.T) {
 			configx.WithConfigFiles("stub/.kratos.oauth2_provider.yaml"), configx.SkipValidation())
 		assert.Equal(t, "https://oauth2_provider/", conf.OAuth2ProviderURL(ctx).String())
 		assert.Equal(t, http.Header{"Authorization": {"Basic"}}, conf.OAuth2ProviderHeader(ctx))
+		assert.True(t, conf.OAuth2ProviderOverrideReturnTo(ctx))
 	})
 
 	t.Run("case=defaults", func(t *testing.T) {
 		conf, _ := config.New(ctx, logrusx.New("", ""), os.Stderr, configx.SkipValidation())
 		assert.Empty(t, conf.OAuth2ProviderURL(ctx))
 		assert.Empty(t, conf.OAuth2ProviderHeader(ctx))
+		assert.False(t, conf.OAuth2ProviderOverrideReturnTo(ctx))
 	})
 }
 

diff --git a/driver/config/stub/.kratos.oauth2_provider.yaml b/driver/config/stub/.kratos.oauth2_provider.yaml
@@ -2,3 +2,4 @@ oauth2_provider:
   url: https://oauth2_provider/
   headers:
     Authorization: Basic
+  override_return_to: true
diff --git a/embedx/config.schema.json b/embedx/config.schema.json
@@ -1901,6 +1901,12 @@
               "Authorization": "Bearer some-token"
             }
           ]
+        },
+        "override_return_to": {
+          "title":"Persist OAuth2 request between flows",
+          "type":"boolean",
+          "default":false,
+          "description":"Override the return_to query parameter with the OAuth2 provider request URL when perfoming an OAuth2 login flow."
         }
       },
       "additionalProperties": false

diff --git a/hydra/fake.go b/hydra/fake.go
@@ -41,7 +41,9 @@ func (h *FakeHydra) GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) (*hy
 	case FAKE_ACCEPT_REQUEST_FAIL:
 		return &hydraclientgo.OAuth2LoginRequest{}, nil
 	case FAKE_SUCCESS:
-		return &hydraclientgo.OAuth2LoginRequest{}, nil
+		return &hydraclientgo.OAuth2LoginRequest{
+			RequestUrl: "https://www.ory.sh",
+		}, nil
 	default:
 		panic("unknown fake login_challenge " + hlc.UUID.String())
 	}

diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go
@@ -425,6 +425,19 @@ func (h *Handler) createBrowserLoginFlow(w http.ResponseWriter, r *http.Request,
 			q.Set("refresh", "true")
 			r.URL.RawQuery = q.Encode()
 		}
+
+		// 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 hlr.RequestUrl != "" && h.d.Config().OAuth2ProviderOverrideReturnTo(r.Context()) {
+			// replace the return_to query parameter
+			q := r.URL.Query()
+			q.Set("return_to", hlr.RequestUrl)
+			r.URL.RawQuery = q.Encode()
+		}
 	}
 
 	a, sess, err := h.NewLoginFlow(w, r, flow.TypeBrowser)

diff --git a/selfservice/flow/login/handler_test.go b/selfservice/flow/login/handler_test.go
@@ -14,6 +14,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pkg/errors"
+
 	"github.com/ory/x/urlx"
 
 	"github.com/ory/x/sqlxx"
@@ -57,6 +59,7 @@ func TestFlowLifecycle(t *testing.T) {
 
 	errorTS := testhelpers.NewErrorTestServer(t, reg)
 	conf.MustSet(ctx, config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh")
+
 	testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/password.schema.json")
 
 	assertion := func(body []byte, isForced, isApi bool) {
@@ -84,6 +87,24 @@ func TestFlowLifecycle(t *testing.T) {
 		return res, body
 	}
 
+	initUnauthenticatedFlow := func(t *testing.T, extQuery url.Values, isAPI bool) (*http.Response, []byte) {
+		route := login.RouteInitBrowserFlow
+		if isAPI {
+			route = login.RouteInitAPIFlow
+		}
+		client := ts.Client()
+		req := x.NewTestHTTPRequest(t, "GET", ts.URL+route, nil)
+
+		req.URL.RawQuery = extQuery.Encode()
+		res, err := client.Do(req)
+		require.NoError(t, errors.WithStack(err))
+
+		body, err := io.ReadAll(res.Body)
+		require.NoError(t, errors.WithStack(err))
+		require.NoError(t, res.Body.Close())
+		return res, body
+	}
+
 	initFlowWithAccept := func(t *testing.T, query url.Values, isAPI bool, accept string) (*http.Response, []byte) {
 		route := login.RouteInitBrowserFlow
 		if isAPI {
@@ -578,6 +599,36 @@ func TestFlowLifecycle(t *testing.T) {
 
 			conf.MustSet(ctx, config.ViperKeyOAuth2ProviderURL, "https://fake-hydra")
 
+			t.Run("case=oauth2 flow init should override return_to to the oauth2 request_url", func(t *testing.T) {
+				conf.MustSet(ctx, config.ViperKeyURLsAllowedReturnToDomains, []string{"https://www.ory.sh", "https://example.com"})
+				conf.MustSet(ctx, config.ViperKeyOAuth2ProviderOverrideReturnTo, true)
+
+				t.Cleanup(func() {
+					conf.MustSet(ctx, config.ViperKeyOAuth2ProviderOverrideReturnTo, false)
+				})
+
+				res, _ := initUnauthenticatedFlow(t, url.Values{
+					"return_to":       {"https://example.com"},
+					"login_challenge": {hydra.FAKE_SUCCESS},
+				}, false)
+				require.Equal(t, http.StatusOK, res.StatusCode)
+				require.Contains(t, res.Request.URL.String(), loginTS.URL)
+
+				c := ts.Client()
+				req := x.NewTestHTTPRequest(t, "GET", ts.URL+login.RouteGetFlow, nil)
+				req.URL.RawQuery = url.Values{"id": {res.Request.URL.Query().Get("flow")}}.Encode()
+
+				res, err := c.Do(req)
+				require.NoError(t, err)
+
+				body, err := io.ReadAll(res.Body)
+				require.NoError(t, errors.WithStack(err))
+
+				require.NoError(t, res.Body.Close())
+
+				assert.Equal(t, "https://www.ory.sh", gjson.GetBytes(body, "return_to").Value())
+			})
+
 			t.Run("case=oauth2 flow init succeeds", func(t *testing.T) {
 				res, _ := initAuthenticatedFlow(t, url.Values{"login_challenge": {hydra.FAKE_SUCCESS}}, false)
 				require.Contains(t, res.Request.URL.String(), loginTS.URL)

diff --git a/selfservice/strategy/password/op_login_test.go b/selfservice/strategy/password/op_login_test.go
@@ -163,6 +163,7 @@ func TestOAuth2Provider(t *testing.T) {
 		config.ViperKeySelfServiceStrategyConfig+"."+string(identity.CredentialsTypePassword),
 		map[string]interface{}{"enabled": true},
 	)
+
 	router := x.NewRouterPublic()
 	kratosPublicTS, _ := testhelpers.NewKratosServerWithRouters(t, reg, router, x.NewRouterAdmin())
 
@@ -238,6 +239,7 @@ func TestOAuth2Provider(t *testing.T) {
 
 	hydraAdmin, hydraPublic := newHydra(t, uiTS.URL, uiTS.URL)
 	conf.MustSet(ctx, config.ViperKeyOAuth2ProviderURL, hydraAdmin)
+
 	hydraAdminClient = createHydraOAuth2ApiClient(hydraAdmin)
 	clientID := createOAuth2Client(t, ctx, hydraAdminClient, []string{clientAppTS.URL}, "profile email")