From a1fea6c353768bbf154900766fbbe51f2a148554 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Wed, 3 May 2023 17:16:50 +0200 Subject: [PATCH] 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 --------- Co-authored-by: Jonas Hungershausen --- driver/config/config.go | 5 + driver/config/config_test.go | 2 + .../config/stub/.kratos.oauth2_provider.yaml | 1 + embedx/config.schema.json | 6 + hydra/fake.go | 4 +- selfservice/flow/login/handler.go | 13 ++ selfservice/flow/login/handler_test.go | 51 +++++ .../strategy/password/op_login_test.go | 2 + .../profiles/oidc-provider/login.spec.ts | 205 +++++++++++++++++- .../oidc-provider/registration.spec.ts | 3 +- test/e2e/cypress/support/config.d.ts | 1 + test/e2e/profiles/oidc-provider/.kratos.yml | 19 ++ test/e2e/profiles/oidc-provider/hydra.jsonnet | 13 ++ .../oidc-provider/identity.traits.schema.json | 5 +- 14 files changed, 318 insertions(+), 12 deletions(-) create mode 100644 test/e2e/profiles/oidc-provider/hydra.jsonnet diff --git a/driver/config/config.go b/driver/config/config.go index 62a94baed82..cc72c2f7512 100644 --- 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 index c1b581c5cfb..dace0ee5d0d 100644 --- 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 index e7548822aba..9c009e294d9 100644 --- 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 index f7567a5143b..e79adc2a386 100644 --- 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 index 8287a88080c..b476418d8c4 100644 --- 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 index 3e4be7d2221..932cd680131 100644 --- 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 index fdf3d951871..9665c77a848 100644 --- 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 index 71010e33038..35a2ea57e32 100644 --- 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") diff --git a/test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts b/test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts index 5c1b9679a5e..66a4a91f17b 100644 --- a/test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts @@ -2,14 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 import { gen } from "../../../helpers" -import * as oauth2 from "../../../helpers/oauth2" import * as httpbin from "../../../helpers/httpbin" +import * as oauth2 from "../../../helpers/oauth2" context("OpenID Provider", () => { - before(() => { - cy.useConfigProfile("oidc-provider") - cy.proxy("express") - }) const client = { auth_endpoint: "http://localhost:4744/oauth2/auth", token_endpoint: "http://localhost:4744/oauth2/token", @@ -25,6 +21,12 @@ context("OpenID Provider", () => { ], } + before(() => { + cy.deleteMail() + cy.useConfigProfile("oidc-provider") + cy.proxy("express") + }) + it("login", () => { const email = gen.email() const password = gen.password() @@ -41,7 +43,7 @@ context("OpenID Provider", () => { // kratos login ui cy.get("[name=identifier]").type(email) cy.get("[name=password]").type(password) - cy.get("[type=submit]").click() + cy.get("[type='submit'][value='password']").click() // consent ui cy.get("#openid").click() @@ -79,7 +81,7 @@ context("OpenID Provider", () => { // kratos login ui cy.get("[name=identifier]").type(email) cy.get("[name=password]").type(password) - cy.get("[type=submit]").click() + cy.get("[type='submit'][value='password']").click() // consent ui cy.get("#accept").click() @@ -111,8 +113,7 @@ context("OpenID Provider", () => { // kratos login ui cy.get("[name=identifier]").type(email) cy.get("[name=password]").type(password) - cy.get("[type=submit]").click() - + cy.get("[type='submit'][value='password']").click() // consent ui cy.get("#accept").click() } @@ -146,3 +147,189 @@ context("OpenID Provider", () => { cy.getCookie("ory_hydra_session_dev").should("be.null") }) }) + +context("OpenID Provider - change between flows", () => { + const client = { + auth_endpoint: "http://localhost:4744/oauth2/auth", + token_endpoint: "http://localhost:4744/oauth2/token", + id: Cypress.env("OIDC_DUMMY_CLIENT_ID"), + secret: Cypress.env("OIDC_DUMMY_CLIENT_SECRET"), + token_endpoint_auth_method: "client_secret_basic", + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code", "id_token"], + scopes: ["openid", "offline", "email", "website"], + callbacks: [ + "http://localhost:5555/callback", + "https://httpbin.org/anything", + ], + } + + function doConsent(amrs?: string[]) { + cy.url().should("contain", "/consent") + + // consent ui + cy.get("#openid").click() + cy.get("#offline").click() + cy.get("#accept").click() + + const scope = ["offline", "openid"] + httpbin.checkToken(client, scope, (token: any) => { + expect(token).to.have.property("access_token") + expect(token).to.have.property("id_token") + expect(token).to.have.property("refresh_token") + expect(token).to.have.property("token_type") + expect(token).to.have.property("expires_in") + expect(token.scope).to.equal("offline openid") + let idToken = JSON.parse( + decodeURIComponent(escape(window.atob(token.id_token.split(".")[1]))), + ) + expect(idToken).to.have.property("amr") + expect(idToken.amr).to.deep.equal(amrs || ["password"]) + }) + } + + before(() => { + cy.useConfigProfile("oidc-provider") + cy.updateConfigFile((config) => { + config.selfservice.allowed_return_urls = [ + oauth2.getDefaultAuthorizeURL(client), + ] + config.oauth2_provider.override_return_to = true + return config + }) + cy.proxy("express") + }) + + it("switch to registration flow", () => { + const identity = gen.identityWithWebsite() + + const url = oauth2.getDefaultAuthorizeURL(client) + + cy.visit(url) + cy.get("[href*='/registration']").click() + cy.url().should("contain", "/registration") + + cy.get("[name='traits.email']").type(identity.email) + cy.get("[name='password']").type(identity.password) + cy.get("[name='traits.website']").type(identity.fields["traits.website"]) + cy.get("[type='submit'][value='password']").click() + + doConsent() + }) + + it("switch to recovery flow with password reset", () => { + cy.deleteMail() + cy.longRecoveryLifespan() + cy.longLinkLifespan() + cy.disableVerification() + cy.enableRecovery() + cy.useRecoveryStrategy("code") + cy.notifyUnknownRecipients("recovery", false) + cy.longPrivilegedSessionTime() + + const identity = gen.identityWithWebsite() + cy.registerApi(identity) + + const url = oauth2.getDefaultAuthorizeURL(client) + cy.visit(url) + cy.get("[href*='/recovery']").click() + + cy.get("input[name='email']").type(identity.email) + cy.get("button[value='code']").click() + cy.get('[data-testid="ui/message/1060003"]').should( + "have.text", + "An email containing a recovery code has been sent to the email address you provided. If you have not received an email, check the spelling of the address and make sure to use the address you registered with.", + ) + + cy.recoveryEmailWithCode({ expect: { email: identity.email } }) + cy.get("button[value='code']").click() + + cy.get('[data-testid="ui/message/1060001"]', { timeout: 30000 }).should( + "contain.text", + "You successfully recovered your account. ", + ) + + cy.getSession() + cy.location("pathname").should("eq", "/settings") + // do a password change + const newPassword = gen.password() + cy.get('input[name="password"]').clear().type(newPassword) + cy.get('button[value="password"]').click() + + // since we aren't skipping the consent, Kratos will ask us to login again + cy.get('input[name="password"]').clear().type(newPassword) + cy.get('button[value="password"]').click() + + // we should now end up on the consent screen + doConsent(["code_recovery", "password"]) + }) + + it("switch to recovery flow with oidc link", () => { + cy.deleteMail() + cy.longRecoveryLifespan() + cy.longLinkLifespan() + cy.enableRecovery() + cy.useRecoveryStrategy("code") + cy.notifyUnknownRecipients("recovery", false) + cy.longPrivilegedSessionTime() + + const fakeOidcFlow = (identity: identityWithWebsite) => { + cy.get("input[name='username']").type(identity.email) + cy.get("button[name='action'][value='accept']").click() + // consent screen for the 'fake' oidc provider + cy.url().should("contain", "/consent") + // consent ui for 3rd party integration to Kratos (sign in with Google) + // it will look the same as Hydra consent ui since we are reusing it as + // a 'fake' oidc provider + cy.get("#openid").click() + cy.get("#offline").click() + cy.get("#accept").click() + } + + const identity = gen.identityWithWebsite() + cy.registerApi(identity) + + const url = oauth2.getDefaultAuthorizeURL(client) + cy.visit(url) + + cy.get("[href*='/recovery']").click() + cy.get("input[name='email']").type(identity.email) + cy.get("button[value='code']").click() + + cy.get('[data-testid="ui/message/1060003"]').should( + "have.text", + "An email containing a recovery code has been sent to the email address you provided. If you have not received an email, check the spelling of the address and make sure to use the address you registered with.", + ) + + cy.recoveryEmailWithCode({ expect: { email: identity.email } }) + cy.get("button[value='code']").click() + + cy.get('[data-testid="ui/message/1060001"]', { timeout: 30000 }).should( + "contain.text", + "You successfully recovered your account. ", + ) + + cy.getSession() + cy.location("pathname").should("eq", "/settings") + + // (OAauth2+Kratos provider '/settings' -> Fake OAuth2 '/login') + + // we are reusing Hydra as an OIDC provider for Kratos and doing an OIDC account + // linking process here + // this is to check if the user can recover their account through OIDC linking + // and still be redirected back to the original request + cy.get("button[value='google']").click() + cy.location("pathname").should("eq", "/login") + fakeOidcFlow(identity) + + // it should return us back to the initial OAauth2 login flow + // sign in through the 'fake' oidc provider + cy.get("button[type='submit'][value='google']").click() + + // login again at the 'fake' oidc provider + fakeOidcFlow(identity) + + // complete the consent screen + doConsent(["oidc"]) + }) +}) diff --git a/test/e2e/cypress/integration/profiles/oidc-provider/registration.spec.ts b/test/e2e/cypress/integration/profiles/oidc-provider/registration.spec.ts index 8f980335291..6c13f158893 100644 --- a/test/e2e/cypress/integration/profiles/oidc-provider/registration.spec.ts +++ b/test/e2e/cypress/integration/profiles/oidc-provider/registration.spec.ts @@ -44,7 +44,8 @@ context("OpenID Provider", () => { cy.get('input[type=checkbox][name="traits.newsletter"]').click({ force: true, }) - cy.get("[type=submit]").click() + + cy.get("[type='submit'][value='password']").click() cy.get("#openid").click() cy.get("#offline").click() diff --git a/test/e2e/cypress/support/config.d.ts b/test/e2e/cypress/support/config.d.ts index cb5ad83b823..3ee8e8657b9 100644 --- a/test/e2e/cypress/support/config.d.ts +++ b/test/e2e/cypress/support/config.d.ts @@ -971,6 +971,7 @@ export interface WebHookAuthBasicAuthProperties { export interface OAuth2ProviderConfiguration { url?: OAuth20ProviderURL headers?: HTTPRequestHeaders + override_return_to?: boolean } /** * These headers will be passed in HTTP request to the OAuth2 Provider. diff --git a/test/e2e/profiles/oidc-provider/.kratos.yml b/test/e2e/profiles/oidc-provider/.kratos.yml index 43d3700a1da..4850e71a380 100644 --- a/test/e2e/profiles/oidc-provider/.kratos.yml +++ b/test/e2e/profiles/oidc-provider/.kratos.yml @@ -4,6 +4,23 @@ clients: oauth2_provider: url: "http://localhost:4745" selfservice: + methods: + password: + enabled: true + code: + enabled: true + oidc: + enabled: true + config: + providers: + - id: google + provider: generic + client_id: ${OIDC_GOOGLE_CLIENT_ID} + client_secret: ${OIDC_GOOGLE_CLIENT_SECRET} + issuer_url: http://localhost:4444/ + scope: + - offline + mapper_url: file://test/e2e/profiles/oidc-provider/hydra.jsonnet flows: settings: privileged_session_max_age: 5m @@ -30,6 +47,8 @@ selfservice: ui_url: http://localhost:4455/verify recovery: ui_url: http://localhost:4455/recovery + enabled: true + lifespan: 5m identity: schemas: diff --git a/test/e2e/profiles/oidc-provider/hydra.jsonnet b/test/e2e/profiles/oidc-provider/hydra.jsonnet new file mode 100644 index 00000000000..a0e1425f816 --- /dev/null +++ b/test/e2e/profiles/oidc-provider/hydra.jsonnet @@ -0,0 +1,13 @@ +local claims = std.extVar('claims'); + +if std.length(claims.sub) == 0 then + error 'claim sub not set' +else + { + identity: { + traits: { + email: claims.sub, + [if "website" in claims then "website" else null]: claims.website + }, + }, + } diff --git a/test/e2e/profiles/oidc-provider/identity.traits.schema.json b/test/e2e/profiles/oidc-provider/identity.traits.schema.json index e88020fa60a..8b6cd437ee9 100644 --- a/test/e2e/profiles/oidc-provider/identity.traits.schema.json +++ b/test/e2e/profiles/oidc-provider/identity.traits.schema.json @@ -20,6 +20,9 @@ "webauthn": { "identifier": true } + }, + "recovery": { + "via": "email" } } }, @@ -54,4 +57,4 @@ "additionalProperties": false } } -} +} \ No newline at end of file