Skip to content

Commit

Permalink
fix: redirect to verification URL even if login_challenge is set (#3412)
Browse files Browse the repository at this point in the history 
Fixes ory/network#320
  • Loading branch information
@hperl
hperl authored Aug 2, 2023
1 parent 60e9a36 commit cd9e6a0
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 34 deletions.
14 changes: 11 additions & 3 deletions identity/credentials.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,14 @@ const (
CredentialsTypeWebAuthn CredentialsType = "webauthn"
)

var AllCredentialTypes = []CredentialsType{
CredentialsTypePassword,
CredentialsTypeOIDC,
CredentialsTypeTOTP,
CredentialsTypeLookup,
CredentialsTypeWebAuthn,
}

const (
// CredentialsTypeRecoveryLink is a special credential type linked to the link strategy (recovery flow).
// It is not used within the credentials object itself.
Expand Down Expand Up @@ -163,7 +171,7 @@ type Credentials struct {
NID uuid.UUID `json:"-" faker:"-" db:"nid"`
}

func (c Credentials) TableName(ctx context.Context) string {
func (c Credentials) TableName(context.Context) string {
return "identity_credentials"
}

Expand Down Expand Up @@ -202,11 +210,11 @@ type (
}
)

func (c CredentialsTypeTable) TableName(ctx context.Context) string {
func (c CredentialsTypeTable) TableName(context.Context) string {
return "identity_credential_types"
}

func (c CredentialIdentifier) TableName(ctx context.Context) string {
func (c CredentialIdentifier) TableName(context.Context) string {
return "identity_credential_identifiers"
}

Expand Down
8 changes: 1 addition & 7 deletions selfservice/flow/login/hook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,7 @@ func TestLoginExecutor(t *testing.T) {
t.Parallel()
ctx := context.Background()

for _, strategy := range []identity.CredentialsType{
identity.CredentialsTypePassword,
identity.CredentialsTypeOIDC,
identity.CredentialsTypeTOTP,
identity.CredentialsTypeWebAuthn,
identity.CredentialsTypeLookup,
} {
for _, strategy := range identity.AllCredentialTypes {
strategy := strategy

t.Run("strategy="+strategy.String(), func(t *testing.T) {
Expand Down
18 changes: 16 additions & 2 deletions selfservice/flow/registration/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"fmt"
"net/http"
"net/url"
"time"

"go.opentelemetry.io/otel/trace"
Expand Down Expand Up @@ -248,11 +249,24 @@ func (e *HookExecutor) PostRegistrationHook(w http.ResponseWriter, r *http.Reque

finalReturnTo := returnTo.String()
if a.OAuth2LoginChallenge != "" {
cr, err := e.d.Hydra().AcceptLoginRequest(r.Context(), string(a.OAuth2LoginChallenge), i.ID.String(), s.AMR)
callbackURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(), string(a.OAuth2LoginChallenge), i.ID.String(), s.AMR)
if err != nil {
return err
}
finalReturnTo = cr
if a.ReturnToVerification != "" {
// Special case: If Kratos is used as a login UI *and* we want to show the verification UI,
// redirect to the verification URL first and then return to Hydra.
verificationURL, err := url.Parse(a.ReturnToVerification)
if err != nil {
return err
}
q := verificationURL.Query()
q.Set("return_to", callbackURL)
verificationURL.RawQuery = q.Encode()
finalReturnTo = verificationURL.String()
} else {
finalReturnTo = callbackURL
}
} else if a.ReturnToVerification != "" {
finalReturnTo = a.ReturnToVerification
}
Expand Down
80 changes: 62 additions & 18 deletions selfservice/flow/registration/hook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/tidwall/gjson"

"github.com/ory/kratos/driver/config"
"github.com/ory/kratos/hydra"
"github.com/ory/kratos/identity"
"github.com/ory/kratos/internal"
"github.com/ory/kratos/internal/testhelpers"
Expand All @@ -27,20 +28,23 @@ import (
)

func TestRegistrationExecutor(t *testing.T) {
t.Parallel()
ctx := context.Background()
for _, strategy := range []string{
identity.CredentialsTypePassword.String(),
identity.CredentialsTypeOIDC.String(),
identity.CredentialsTypeTOTP.String(),
identity.CredentialsTypeWebAuthn.String(),
} {

for _, strategy := range identity.AllCredentialTypes {
strategy := strategy.String()

t.Run("strategy="+strategy, func(t *testing.T) {
t.Parallel()

conf, reg := internal.NewFastRegistryWithMocks(t)
reg.WithHydra(hydra.NewFake())
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/registration.schema.json")
conf.MustSet(ctx, config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh/")

newServer := func(t *testing.T, i *identity.Identity, ft flow.Type) *httptest.Server {
newServer := func(t *testing.T, i *identity.Identity, ft flow.Type, flowCallbacks ...func(*registration.Flow)) *httptest.Server {
router := httprouter.New()

handleErr := testhelpers.SelfServiceHookRegistrationErrorHandler
router.GET("/registration/pre", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
f, err := registration.NewFlow(conf, time.Minute, x.FakeCSRFToken, r, ft)
Expand All @@ -54,10 +58,13 @@ func TestRegistrationExecutor(t *testing.T) {
if i == nil {
i = testhelpers.SelfServiceHookFakeIdentity(t)
}
a, err := registration.NewFlow(conf, time.Minute, x.FakeCSRFToken, r, ft)
regFlow, err := registration.NewFlow(conf, time.Minute, x.FakeCSRFToken, r, ft)
require.NoError(t, err)
a.RequestURL = x.RequestURL(r).String()
_ = handleErr(t, w, r, reg.RegistrationHookExecutor().PostRegistrationHook(w, r, identity.CredentialsType(strategy), "", a, i))
regFlow.RequestURL = x.RequestURL(r).String()
for _, callback := range flowCallbacks {
callback(regFlow)
}
_ = handleErr(t, w, r, reg.RegistrationHookExecutor().PostRegistrationHook(w, r, identity.CredentialsType(strategy), "", regFlow, i))
})

ts := httptest.NewServer(router)
Expand Down Expand Up @@ -161,11 +168,11 @@ func TestRegistrationExecutor(t *testing.T) {
assert.Empty(t, gjson.Get(body, "session_token"))
})

t.Run("case=should redirect to verification ui if show_verification_ui hook is set", func(t *testing.T) {
t.Run("case=should redirect to verification UI if show_verification_ui hook is set", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.Set(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.Set(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerificationUI,
},
Expand All @@ -179,11 +186,48 @@ func TestRegistrationExecutor(t *testing.T) {
assert.NotEmpty(t, res.Request.URL.Query().Get("flow"))
})

t.Run("case=should redirect to first verification ui if show_verification_ui hook is set and multiple verifiable addresses", func(t *testing.T) {
t.Run("case=should redirect to verification UI if there is a login_challenge", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{{
"hook": hook.KeyVerificationUI,
}})
i := testhelpers.SelfServiceHookFakeIdentity(t)
i.Traits = identity.Traits(`{"email": "[email protected]"}`)

withOAuthChallenge := func(f *registration.Flow) {
f.OAuth2LoginChallenge = hydra.FakeValidLoginChallenge
}
res, _ := makeRequestPost(t, newServer(t, i, flow.TypeBrowser, withOAuthChallenge), false, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.Contains(t, res.Request.URL.String(), verificationTS.URL)
assert.NotEmpty(t, res.Request.URL.Query().Get("flow"))
assert.Equal(t, hydra.FakePostLoginURL, res.Request.URL.Query().Get("return_to"))
})

t.Run("case=should not redirect to verification UI if the login_challenge is invalid", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{{
"hook": hook.KeyVerificationUI,
}})
i := testhelpers.SelfServiceHookFakeIdentity(t)
i.Traits = identity.Traits(`{"email": "[email protected]"}`)

withOAuthChallenge := func(f *registration.Flow) {
f.OAuth2LoginChallenge = hydra.FakeInvalidLoginChallenge
}
res, body := makeRequestPost(t, newServer(t, i, flow.TypeBrowser, withOAuthChallenge), false, url.Values{})
assert.EqualValues(t, http.StatusInternalServerError, res.StatusCode)
assert.Equal(t, hydra.ErrFakeAcceptLoginRequestFailed.Error(), body, "%s", body)
})

t.Run("case=should redirect to first verification UI if show_verification_ui hook is set and multiple verifiable addresses", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.Set(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.Set(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerificationUI,
},
Expand All @@ -202,8 +246,8 @@ func TestRegistrationExecutor(t *testing.T) {
t.Run("case=should still sent session if show_verification_ui is set after session hook", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.Set(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.Set(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerificationUI,
},
Expand Down
8 changes: 4 additions & 4 deletions selfservice/hook/show_verification_ui.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
)

var (
_ registration.PostHookPostPersistExecutor = new(SessionIssuer)
_ registration.PostHookPostPersistExecutor = new(ShowVerificationUIHook)
)

type (
Expand All @@ -41,13 +41,13 @@ func NewShowVerificationUIHook(d showVerificationUIDependencies) *ShowVerificati

// ExecutePostRegistrationPostPersistHook adds redirect headers and status code if the request is a browser request.
// If the request is not a browser request, this hook does nothing.
func (e *ShowVerificationUIHook) ExecutePostRegistrationPostPersistHook(w http.ResponseWriter, r *http.Request, f *registration.Flow, s *session.Session) error {
func (e *ShowVerificationUIHook) ExecutePostRegistrationPostPersistHook(_ http.ResponseWriter, r *http.Request, f *registration.Flow, _ *session.Session) error {
return otelx.WithSpan(r.Context(), "selfservice.hook.SessionIssuer.ExecutePostRegistrationPostPersistHook", func(ctx context.Context) error {
return e.execute(w, r.WithContext(ctx), f, s)
return e.execute(r.WithContext(ctx), f)
})
}

func (e *ShowVerificationUIHook) execute(w http.ResponseWriter, r *http.Request, f *registration.Flow, s *session.Session) error {
func (e *ShowVerificationUIHook) execute(r *http.Request, f *registration.Flow) error {
if !x.IsBrowserRequest(r) {
// this hook is only intended to be used by browsers, as it redirects to the verification ui
// JSON API clients should use the `continue_with` field to continue the flow
Expand Down

0 comments on commit cd9e6a0

Please sign in to comment.