Skip to content

Commit

Permalink
fix: OIDC login in browser JSON flow
Browse files Browse the repository at this point in the history
  • Loading branch information
@hperl
hperl committed Jun 13, 2023
1 parent d3f3be3 commit 940e9b0
Show file tree
Hide file tree
Showing 6 changed files with 144 additions and 31 deletions.
24 changes: 14 additions & 10 deletions hydra/fake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,37 @@ import (
)

const (
FAKE_GET_LOGIN_REQUEST_RETURN_NIL_NIL = "b805f2d9-2f6d-4745-9d68-a17f48e25774"
FAKE_ACCEPT_REQUEST_FAIL = "2e98454e-031b-4870-9ad6-8517df1ce604"
FAKE_SUCCESS = "5ff59a39-ecc5-467e-bb10-26644c0700ee"
FakeInvalidLoginChallenge = "2e98454e-031b-4870-9ad6-8517df1ce604"
FakeValidLoginChallenge = "5ff59a39-ecc5-467e-bb10-26644c0700ee"
FakePostLoginURL = "https://www.ory.sh/fake-post-login"
)

var ErrFakeAcceptLoginRequestFailed = errors.New("failed to accept login request")

type FakeHydra struct{}

var _ Hydra = &FakeHydra{}

func NewFakeHydra() *FakeHydra {
func NewFake() *FakeHydra {
return &FakeHydra{}
}

func (h *FakeHydra) AcceptLoginRequest(ctx context.Context, hlc uuid.UUID, sub string, amr session.AuthenticationMethods) (string, error) {
func (h *FakeHydra) AcceptLoginRequest(_ context.Context, hlc uuid.UUID, _ string, _ session.AuthenticationMethods) (string, error) {
switch hlc.String() {
case FAKE_ACCEPT_REQUEST_FAIL:
return "", errors.New("failed to accept login request")
case FakeInvalidLoginChallenge:
return "", ErrFakeAcceptLoginRequestFailed
case FakeValidLoginChallenge:
return FakePostLoginURL, nil
default:
panic("unknown fake login_challenge " + hlc.String())
}
}

func (h *FakeHydra) GetLoginRequest(ctx context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) {
func (h *FakeHydra) GetLoginRequest(_ context.Context, hlc uuid.NullUUID) (*hydraclientgo.OAuth2LoginRequest, error) {
switch hlc.UUID.String() {
case FAKE_ACCEPT_REQUEST_FAIL:
case FakeInvalidLoginChallenge:
return &hydraclientgo.OAuth2LoginRequest{}, nil
case FAKE_SUCCESS:
case FakeValidLoginChallenge:
return &hydraclientgo.OAuth2LoginRequest{
RequestUrl: "https://www.ory.sh",
}, nil
Expand Down
2 changes: 1 addition & 1 deletion internal/testhelpers/selfservice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func SelfServiceHookSettingsErrorHandler(t *testing.T, w http.ResponseWriter, r
return SelfServiceHookErrorHandler(t, w, r, settings.ErrHookAbortFlow, err)
}

func SelfServiceHookErrorHandler(t *testing.T, w http.ResponseWriter, r *http.Request, abortErr error, actualErr error) bool {
func SelfServiceHookErrorHandler(t *testing.T, w http.ResponseWriter, _ *http.Request, abortErr error, actualErr error) bool {
if actualErr != nil {
t.Logf("received error: %+v", actualErr)
if errors.Is(actualErr, abortErr) {
Expand Down
10 changes: 5 additions & 5 deletions selfservice/flow/login/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func init() {
func TestFlowLifecycle(t *testing.T) {
ctx := context.Background()
conf, reg := internal.NewFastRegistryWithMocks(t)
reg.WithHydra(hydra.NewFakeHydra())
reg.WithHydra(hydra.NewFake())
router := x.NewRouterPublic()
ts, _ := testhelpers.NewKratosServerWithRouters(t, reg, router, x.NewRouterAdmin())
loginTS := testhelpers.NewLoginUIFlowEchoServer(t, reg)
Expand Down Expand Up @@ -592,7 +592,7 @@ func TestFlowLifecycle(t *testing.T) {
})

t.Run("case=refuses to parse oauth2 login challenge when Hydra is not configured", func(t *testing.T) {
res, body := initAuthenticatedFlow(t, url.Values{"login_challenge": {hydra.FAKE_GET_LOGIN_REQUEST_RETURN_NIL_NIL}}, false)
res, body := initAuthenticatedFlow(t, url.Values{"login_challenge": {hydra.FakeValidLoginChallenge}}, false)
require.Contains(t, res.Request.URL.String(), errorTS.URL)
require.Contains(t, string(body), "refusing to parse")
})
Expand All @@ -609,7 +609,7 @@ func TestFlowLifecycle(t *testing.T) {

res, _ := initUnauthenticatedFlow(t, url.Values{
"return_to": {"https://example.com"},
"login_challenge": {hydra.FAKE_SUCCESS},
"login_challenge": {hydra.FakeValidLoginChallenge},
}, false)
require.Equal(t, http.StatusOK, res.StatusCode)
require.Contains(t, res.Request.URL.String(), loginTS.URL)
Expand All @@ -630,12 +630,12 @@ func TestFlowLifecycle(t *testing.T) {
})

t.Run("case=oauth2 flow init succeeds", func(t *testing.T) {
res, _ := initAuthenticatedFlow(t, url.Values{"login_challenge": {hydra.FAKE_SUCCESS}}, false)
res, _ := initAuthenticatedFlow(t, url.Values{"login_challenge": {hydra.FakeValidLoginChallenge}}, false)
require.Contains(t, res.Request.URL.String(), loginTS.URL)
})

t.Run("case=oauth2 flow init adds oauth2_login_request field", func(t *testing.T) {
res, body := initSPAFlow(t, url.Values{"login_challenge": {hydra.FAKE_SUCCESS}})
res, body := initSPAFlow(t, url.Values{"login_challenge": {hydra.FakeValidLoginChallenge}})
assert.NotContains(t, res.Request.URL.String(), loginTS.URL)

assert.NotEmpty(t, gjson.GetBytes(body, "oauth2_login_request").Value(), "%s", body)
Expand Down
11 changes: 11 additions & 0 deletions selfservice/flow/login/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,17 @@ func (e *HookExecutor) PostLoginHook(w http.ResponseWriter, r *http.Request, g n
// Browser flows rely on cookies. Adding tokens in the mix will confuse consumers.
s.Token = ""

// If Kratos is used as a Hydra login provider, we need to redirect back to Hydra by returning a 422 status
// with the post login challenge URL as the body.
if a.OAuth2LoginChallenge.Valid {
postChallengeURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(), a.OAuth2LoginChallenge.UUID, i.ID.String(), s.AMR)
if err != nil {
return err
}
e.d.Writer().WriteError(w, r, flow.NewBrowserLocationChangeRequiredError(postChallengeURL))
return nil
}

response := &APIFlowResponse{Session: s}
if required, _ := e.requiresAAL2(r, s, a); required {
// If AAL is not satisfied, we omit the identity to preserve the user's privacy in case of a phishing attack.
Expand Down
105 changes: 98 additions & 7 deletions selfservice/flow/login/hook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ import (
"testing"
"time"

"github.com/gofrs/uuid"
"github.com/stretchr/testify/require"

"github.com/ory/kratos/hydra"
"github.com/ory/kratos/session"

"github.com/gobuffalo/httptest"
Expand All @@ -29,43 +31,54 @@ import (
)

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,
} {
strategy := strategy

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

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

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

router.GET("/login/pre", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
f, err := login.NewFlow(conf, time.Minute, "", r, ft)
loginFlow, err := login.NewFlow(conf, time.Minute, "", r, ft)
require.NoError(t, err)
if testhelpers.SelfServiceHookLoginErrorHandler(t, w, r, reg.LoginHookExecutor().PreLoginHook(w, r, f)) {
if testhelpers.SelfServiceHookLoginErrorHandler(t, w, r, reg.LoginHookExecutor().PreLoginHook(w, r, loginFlow)) {
_, _ = w.Write([]byte("ok"))
}
})

router.GET("/login/post", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
a, err := login.NewFlow(conf, time.Minute, "", r, ft)
loginFlow, err := login.NewFlow(conf, time.Minute, "", r, ft)
require.NoError(t, err)
a.Active = strategy
a.RequestURL = x.RequestURL(r).String()
loginFlow.Active = strategy
loginFlow.RequestURL = x.RequestURL(r).String()
for _, cb := range flowCallback {
cb(loginFlow)
}

sess := session.NewInactiveSession()
sess.CompletedLoginFor(identity.CredentialsTypePassword, identity.AuthenticatorAssuranceLevel1)
if useIdentity == nil {
useIdentity = testhelpers.SelfServiceHookCreateFakeIdentity(t, reg)
}

testhelpers.SelfServiceHookLoginErrorHandler(t, w, r,
reg.LoginHookExecutor().PostLoginHook(w, r, strategy.ToUiNodeGroup(), a, useIdentity, sess, ""))
reg.LoginHookExecutor().PostLoginHook(w, r, strategy.ToUiNodeGroup(), loginFlow, useIdentity, sess, ""))
})

ts := httptest.NewServer(router)
Expand Down Expand Up @@ -149,6 +162,84 @@ func TestLoginExecutor(t *testing.T) {
assert.NotEmpty(t, gjson.Get(body, "session.identity.id").String())
})

t.Run("suite=handle login challenge with browser and application/json", func(t *testing.T) {
t.Run("case=includes the return_to address for a valid challenge", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))

withOAuthChallenge := func(f *login.Flow) {
f.OAuth2LoginChallenge = uuid.NullUUID{Valid: true, UUID: uuid.Must(uuid.FromString(hydra.FakeValidLoginChallenge))}
}
res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil, withOAuthChallenge), true, url.Values{})
assert.EqualValues(t, http.StatusUnprocessableEntity, res.StatusCode)
assert.Equal(t, hydra.FakePostLoginURL, gjson.Get(body, "redirect_browser_to").String(), "%s", body)
})

t.Run("case=returns an error for an invalid challenge", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))

withOAuthChallenge := func(f *login.Flow) {
f.OAuth2LoginChallenge = uuid.NullUUID{Valid: true, UUID: uuid.Must(uuid.FromString(hydra.FakeInvalidLoginChallenge))}
}
res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil, withOAuthChallenge), true, url.Values{})
assert.EqualValues(t, http.StatusInternalServerError, res.StatusCode)
assert.Equal(t, hydra.ErrFakeAcceptLoginRequestFailed.Error(), body, "%s", body)
})
})

t.Run("case=fail if hooks fail", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
viperSetPost(t, conf, strategy.String(), []config.SelfServiceHook{{Name: "err", Config: []byte(`{"ExecuteLoginPostHook": "abort"}`)}})

res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.Equal(t, "", body)
})

t.Run("case=use return_to value", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeyURLsAllowedReturnToDomains, []string{"https://www.ory.sh/"})

res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{"return_to": {"https://www.ory.sh/kratos/"}})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.EqualValues(t, "https://www.ory.sh/kratos/", res.Request.URL.String())
})

t.Run("case=use nested config value", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceLoginAfter+"."+config.DefaultBrowserReturnURL, "https://www.ory.sh/kratos")

res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.EqualValues(t, "https://www.ory.sh/kratos/", res.Request.URL.String())
})

t.Run("case=use nested config value", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
testhelpers.SelfServiceHookLoginSetDefaultRedirectTo(t, conf, "https://www.ory.sh/not-kratos")
testhelpers.SelfServiceHookLoginSetDefaultRedirectToStrategy(t, conf, strategy.String(), "https://www.ory.sh/kratos")

res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.EqualValues(t, "https://www.ory.sh/kratos/", res.Request.URL.String())
})

t.Run("case=pass if hooks pass", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
viperSetPost(t, conf, strategy.String(), []config.SelfServiceHook{{Name: "err", Config: []byte(`{}`)}})

res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.EqualValues(t, "https://www.ory.sh/", res.Request.URL.String())
})

t.Run("case=send a json response for API clients", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))

res, body := makeRequestPost(t, newServer(t, flow.TypeAPI, nil), true, url.Values{})
assert.EqualValues(t, http.StatusOK, res.StatusCode)
assert.NotEmpty(t, gjson.Get(body, "session.identity.id").String())
})

t.Run("case=pass without hooks for browser flow with application/json", func(t *testing.T) {
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))

Expand Down
23 changes: 15 additions & 8 deletions selfservice/strategy/password/op_login_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func newHydra(t *testing.T, loginUI string, consentUI string) (hydraAdmin string
pool, err := dockertest.NewPool("")
require.NoError(t, err)

hydra, err := pool.RunWithOptions(&dockertest.RunOptions{
hydraResource, err := pool.RunWithOptions(&dockertest.RunOptions{
Repository: "oryd/hydra",
Tag: "v2.0.0",
Env: []string{
Expand All @@ -110,18 +110,25 @@ func newHydra(t *testing.T, loginUI string, consentUI string) (hydraAdmin string
})
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, hydra.Close())
require.NoError(t, hydraResource.Close())
})

require.NoError(t, hydra.Expire(uint(60*5)))
require.NoError(t, hydraResource.Expire(uint(60*5)))

require.NotEmpty(t, hydra.GetPort("4444/tcp"), "%+v", hydra.Container.NetworkSettings.Ports)
require.NotEmpty(t, hydra.GetPort("4445/tcp"), "%+v", hydra.Container)
require.NotEmpty(t, hydraResource.GetPort("4444/tcp"), "%+v", hydraResource.Container.NetworkSettings.Ports)
require.NotEmpty(t, hydraResource.GetPort("4445/tcp"), "%+v", hydraResource.Container)

hydraPublic = "http://127.0.0.1:" + hydra.GetPort("4444/tcp")
hydraAdmin = "http://127.0.0.1:" + hydra.GetPort("4445/tcp")
hydraPublic = "http://127.0.0.1:" + hydraResource.GetPort("4444/tcp")
hydraAdmin = "http://127.0.0.1:" + hydraResource.GetPort("4445/tcp")

go pool.Client.Logs(docker.LogsOptions{ErrorStream: TestLogWriter{T: t, streamName: "hydra-stderr"}, OutputStream: TestLogWriter{T: t, streamName: "hydra-stdout"}, Stdout: true, Stderr: true, Follow: true, Container: hydra.Container.ID})
go pool.Client.Logs(docker.LogsOptions{
ErrorStream: TestLogWriter{T: t, streamName: "hydra-stderr"},
OutputStream: TestLogWriter{T: t, streamName: "hydra-stdout"},
Stdout: true,
Stderr: true,
Follow: true,
Container: hydraResource.Container.ID,
})
hl := logrusx.New("hydra-ready-check", "hydra-ready-check")
err = resilience.Retry(hl, time.Second*1, time.Second*5, func() error {
pr := hydraPublic + "/health/ready"
Expand Down

0 comments on commit 940e9b0

Please sign in to comment.