Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: recovery with OAuth2 and 2FA #3834

Closed
wants to merge 1 commit into from
Closed

fix: recovery with OAuth2 and 2FA #3834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/client-go/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
Expand Down
18 changes: 5 additions & 13 deletions selfservice/flow/settings/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func TestHandler(t *testing.T) {
conf.MustSet(ctx, config.ViperKeySelfServiceSettingsRequiredAAL, config.HighestAvailableAAL)
res, body := initFlow(t, aal2Identity, true)
assert.Equalf(t, http.StatusForbidden, res.StatusCode, "%s", body)
assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied(publicTS.URL+"/self-service/login/browser?aal=aal2"), json.RawMessage(body))
assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied("/self-service/login/browser?aal=aal2"), json.RawMessage(body))
})
})

Expand Down Expand Up @@ -289,9 +289,7 @@ func TestHandler(t *testing.T) {
assert.Equal(t, http.StatusForbidden, res.StatusCode)

url := url.URL{
Scheme: conf.SelfPublicURL(ctx).Scheme,
Host: conf.SelfPublicURL(ctx).Host,
Path: login.RouteInitBrowserFlow,
Path: login.RouteInitBrowserFlow,
}
q := url.Query()
q.Add("aal", "aal2")
Expand Down Expand Up @@ -438,9 +436,7 @@ func TestHandler(t *testing.T) {
require.NoError(t, res.Body.Close())

url := url.URL{
Scheme: conf.SelfPublicURL(ctx).Scheme,
Host: conf.SelfPublicURL(ctx).Host,
Path: login.RouteInitBrowserFlow,
Path: login.RouteInitBrowserFlow,
}

returnTo := conf.SelfServiceFlowSettingsUI(context.Background())
Expand Down Expand Up @@ -494,9 +490,7 @@ func TestHandler(t *testing.T) {
assert.Equal(t, http.StatusForbidden, res.StatusCode)

url := url.URL{
Scheme: conf.SelfPublicURL(ctx).Scheme,
Host: conf.SelfPublicURL(ctx).Host,
Path: login.RouteInitBrowserFlow,
Path: login.RouteInitBrowserFlow,
}
q := url.Query()
q.Set("aal", "aal2")
Expand All @@ -522,9 +516,7 @@ func TestHandler(t *testing.T) {
assert.Equal(t, http.StatusForbidden, res.StatusCode)

url := url.URL{
Scheme: conf.SelfPublicURL(ctx).Scheme,
Host: conf.SelfPublicURL(ctx).Host,
Path: login.RouteInitBrowserFlow,
Path: login.RouteInitBrowserFlow,
}

q := url.Query()
Expand Down
2 changes: 1 addition & 1 deletion session/manager_http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func (s *ManagerHTTP) DoesSessionSatisfy(r *http.Request, sess *Session, request
return nil
}

loginURL := urlx.CopyWithQuery(urlx.AppendPaths(s.r.Config().SelfPublicURL(ctx), "/self-service/login/browser"), url.Values{"aal": {"aal2"}})
loginURL := &url.URL{Path: "/self-service/login/browser", RawQuery: "aal=aal2"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the difference to be honest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the idea is to not use the domain, then this won't work because the public url might have a path prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug I want to fix is that if Kratos runs on multiple domains / CNAMEs, redirecting to the public URL will cause the user to end up on a different domain than he started on.

If we redirect without a host, the current host will continue to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the idea is to not use the domain, then this won't work because the public url might have a path prefix

OK, so then we can just extract the path prefix from the public URL?


// return to the requestURL if it was set
if managerOpts.requestURL != "" {
Expand Down
23 changes: 14 additions & 9 deletions session/manager_http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@ import (
"testing"
"time"

"github.com/ory/nosurf"
"github.com/ory/x/urlx"

"github.com/ory/kratos/driver"
"github.com/ory/nosurf"

"github.com/julienschmidt/httprouter"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -562,11 +560,18 @@ func TestDoesSessionSatisfy(t *testing.T) {
amr: session.AuthenticationMethods{{Method: identity.CredentialsTypeOIDC, AAL: identity.AuthenticatorAssuranceLevel1}},
},
{
d: "has=aal1, requested=highest, available=aal1, credentials=password+webauthn_mfa, recovery with session manager options",
requested: config.HighestAvailableAAL,
creds: []identity.Credentials{password, mfaWebAuth},
amr: session.AuthenticationMethods{{Method: identity.CredentialsTypeRecoveryCode}},
err: session.NewErrAALNotSatisfied(urlx.CopyWithQuery(urlx.AppendPaths(conf.SelfPublicURL(context.Background()), "/self-service/login/browser"), url.Values{"aal": {"aal2"}, "return_to": {"https://myapp.com/settings?id=123"}}).String()),
d: "has=aal1, requested=highest, available=aal1, credentials=password+webauthn_mfa, recovery with session manager options",
requested: config.HighestAvailableAAL,
creds: []identity.Credentials{password, mfaWebAuth},
amr: session.AuthenticationMethods{{Method: identity.CredentialsTypeRecoveryCode}},
err: session.NewErrAALNotSatisfied(
(&url.URL{
Path: "/self-service/login/browser",
RawQuery: url.Values{
"aal": []string{"aal2"},
"return_to": {"https://myapp.com/settings?id=123"},
}.Encode(),
}).String()),
sessionManagerOptions: []session.ManagerOptions{session.WithRequestURL("https://myapp.com/settings?id=123")},
expectedFunc: func(t *testing.T, err error, tcError error) {
require.Contains(t, err.(*session.ErrAALNotSatisfied).RedirectTo, "myapp.com")
Expand All @@ -578,7 +583,7 @@ func TestDoesSessionSatisfy(t *testing.T) {
requested: config.HighestAvailableAAL,
creds: []identity.Credentials{password, mfaWebAuth},
amr: session.AuthenticationMethods{{Method: identity.CredentialsTypeRecoveryCode}},
err: session.NewErrAALNotSatisfied(urlx.CopyWithQuery(urlx.AppendPaths(conf.SelfPublicURL(context.Background()), "/self-service/login/browser"), url.Values{"aal": {"aal2"}}).String()),
err: session.NewErrAALNotSatisfied("/self-service/login/browser?aal=aal2"),
expectedFunc: func(t *testing.T, err error, tcError error) {
require.Equal(t, tcError.(*session.ErrAALNotSatisfied).RedirectTo, err.(*session.ErrAALNotSatisfied).RedirectTo)
},
Expand Down
Loading