Skip to content

Commit

Permalink
feat: remove flow cookie (#3639)
Browse files Browse the repository at this point in the history 
This patch removes the flow cookie. All information is already tracked in the request query parameters as part of the {login|consent}_{challenge|verifier}.
  • Loading branch information
@hperl
hperl authored Oct 16, 2023
1 parent a0c06ec commit cde3a30
Show file tree
Hide file tree
Showing 12 changed files with 626 additions and 322 deletions.
4 changes: 2 additions & 2 deletions consent/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type (
RevokeSubjectConsentSession(ctx context.Context, user string) error
RevokeSubjectClientConsentSession(ctx context.Context, user, client string) error

VerifyAndInvalidateConsentRequest(ctx context.Context, f *flow.Flow, verifier string) (*flow.AcceptOAuth2ConsentRequest, error)
VerifyAndInvalidateConsentRequest(ctx context.Context, verifier string) (*flow.AcceptOAuth2ConsentRequest, error)
FindGrantedAndRememberedConsentRequests(ctx context.Context, client, user string) ([]flow.AcceptOAuth2ConsentRequest, error)
FindSubjectsGrantedConsentRequests(ctx context.Context, user string, limit, offset int) ([]flow.AcceptOAuth2ConsentRequest, error)
FindSubjectsSessionGrantedConsentRequests(ctx context.Context, user, sid string, limit, offset int) ([]flow.AcceptOAuth2ConsentRequest, error)
Expand All @@ -48,7 +48,7 @@ type (
CreateLoginRequest(ctx context.Context, req *flow.LoginRequest) (*flow.Flow, error)
GetLoginRequest(ctx context.Context, challenge string) (*flow.LoginRequest, error)
HandleLoginRequest(ctx context.Context, f *flow.Flow, challenge string, r *flow.HandledLoginRequest) (*flow.LoginRequest, error)
VerifyAndInvalidateLoginRequest(ctx context.Context, f *flow.Flow, verifier string) (*flow.HandledLoginRequest, error)
VerifyAndInvalidateLoginRequest(ctx context.Context, verifier string) (*flow.HandledLoginRequest, error)

CreateForcedObfuscatedLoginSession(ctx context.Context, session *ForcedObfuscatedLoginSession) error
GetForcedObfuscatedLoginSession(ctx context.Context, client, obfuscated string) (*ForcedObfuscatedLoginSession, error)
Expand Down
46 changes: 17 additions & 29 deletions consent/manager_test_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,17 +480,13 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana

loginVerifier := x.Must(f.ToLoginVerifier(ctx, deps))

got2, err := m.VerifyAndInvalidateLoginRequest(ctx, f, loginVerifier)
got2, err := m.VerifyAndInvalidateLoginRequest(ctx, loginVerifier)
require.NoError(t, err)
compareAuthenticationRequest(t, c, got2.LoginRequest)

_, err = m.VerifyAndInvalidateLoginRequest(ctx, nil, loginVerifier)
require.Error(t, err)

loginChallenge = x.Must(f.ToLoginChallenge(ctx, deps))
got1, err = m.GetLoginRequest(ctx, loginChallenge)
require.NoError(t, err)
assert.True(t, got1.WasHandled)
})
}
})
Expand Down Expand Up @@ -544,32 +540,23 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana

consentVerifier := x.Must(f.ToConsentVerifier(ctx, deps))

got2, err := m.VerifyAndInvalidateConsentRequest(ctx, f, consentVerifier)
got2, err := m.VerifyAndInvalidateConsentRequest(ctx, consentVerifier)
require.NoError(t, err)
consentRequest.ID = f.ConsentChallengeID.String()
consentRequest.ID = got2.ID
compareConsentRequest(t, consentRequest, got2.ConsentRequest)
assert.Equal(t, consentRequest.ID, got2.ID)
assert.Equal(t, h.GrantedAudience, got2.GrantedAudience)

// Trying to update this again should return an error because the consent request was used.
h.GrantedAudience = sqlxx.StringSliceJSONFormat{"new-audience", "new-audience-2"}
_, err = m.HandleConsentRequest(ctx, f, h)
require.Error(t, err)
t.Run("sub=detect double-submit for consent verifier", func(t *testing.T) {
_, err := m.VerifyAndInvalidateConsentRequest(ctx, consentVerifier)
require.Error(t, err)
})

if tc.hasError {
assert.True(t, got2.HasError())
}
assert.Equal(t, tc.remember, got2.Remember)
assert.Equal(t, tc.rememberFor, got2.RememberFor)

_, err = m.VerifyAndInvalidateConsentRequest(ctx, f, makeID("verifier", network, tc.key))
require.Error(t, err)

// Because we don't persist the flow any more, we can't check for this.
//got1, err = m.GetConsentRequest(ctx, consentChallenge)
//require.NoError(t, err)
//assert.True(t, got1.WasHandled)

})
}

Expand Down Expand Up @@ -648,6 +635,7 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
challengerv1 := makeID("challenge", network, "rv1")
challengerv2 := makeID("challenge", network, "rv2")
t.Run("case=revoke-used-consent-request", func(t *testing.T) {

cr1, hcr1, f1 := MockConsentRequest("rv1", false, 0, false, false, false, "fk-login-challenge", network)
cr2, hcr2, f2 := MockConsentRequest("rv2", false, 0, false, false, false, "fk-login-challenge", network)
f1.NID = deps.Contextualizer().Network(context.Background(), gofrsuuid.Nil)
Expand All @@ -666,30 +654,30 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
_, err = m.HandleConsentRequest(ctx, f2, hcr2)
require.NoError(t, err)

_, err = m.VerifyAndInvalidateConsentRequest(ctx, f1, x.Must(f1.ToConsentVerifier(ctx, deps)))
crr1, err := m.VerifyAndInvalidateConsentRequest(ctx, x.Must(f1.ToConsentVerifier(ctx, deps)))
require.NoError(t, err)
_, err = m.VerifyAndInvalidateConsentRequest(ctx, f2, x.Must(f2.ToConsentVerifier(ctx, deps)))
crr2, err := m.VerifyAndInvalidateConsentRequest(ctx, x.Must(f2.ToConsentVerifier(ctx, deps)))
require.NoError(t, err)

require.NoError(t, fositeManager.CreateAccessTokenSession(
ctx,
makeID("", network, "trva1"),
&fosite.Request{Client: cr1.Client, ID: f1.ConsentChallengeID.String(), RequestedAt: time.Now()},
&fosite.Request{Client: cr1.Client, ID: crr1.ID, RequestedAt: time.Now()},
))
require.NoError(t, fositeManager.CreateRefreshTokenSession(
ctx,
makeID("", network, "rrva1"),
&fosite.Request{Client: cr1.Client, ID: f1.ConsentChallengeID.String(), RequestedAt: time.Now()},
&fosite.Request{Client: cr1.Client, ID: crr1.ID, RequestedAt: time.Now()},
))
require.NoError(t, fositeManager.CreateAccessTokenSession(
ctx,
makeID("", network, "trva2"),
&fosite.Request{Client: cr2.Client, ID: f2.ConsentChallengeID.String(), RequestedAt: time.Now()},
&fosite.Request{Client: cr2.Client, ID: crr2.ID, RequestedAt: time.Now()},
))
require.NoError(t, fositeManager.CreateRefreshTokenSession(
ctx,
makeID("", network, "rrva2"),
&fosite.Request{Client: cr2.Client, ID: f2.ConsentChallengeID.String(), RequestedAt: time.Now()},
&fosite.Request{Client: cr2.Client, ID: crr2.ID, RequestedAt: time.Now()},
))

for i, tc := range []struct {
Expand Down Expand Up @@ -765,9 +753,9 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
require.NoError(t, err)
_, err = m.HandleConsentRequest(ctx, f2, hcr2)
require.NoError(t, err)
handledConsentRequest1, err := m.VerifyAndInvalidateConsentRequest(ctx, f1, x.Must(f1.ToConsentVerifier(ctx, deps)))
handledConsentRequest1, err := m.VerifyAndInvalidateConsentRequest(ctx, x.Must(f1.ToConsentVerifier(ctx, deps)))
require.NoError(t, err)
handledConsentRequest2, err := m.VerifyAndInvalidateConsentRequest(ctx, f2, x.Must(f2.ToConsentVerifier(ctx, deps)))
handledConsentRequest2, err := m.VerifyAndInvalidateConsentRequest(ctx, x.Must(f2.ToConsentVerifier(ctx, deps)))
require.NoError(t, err)

for i, tc := range []struct {
Expand Down Expand Up @@ -937,7 +925,7 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
f.NID = deps.Contextualizer().Network(ctx, gofrsuuid.Nil)
cr := SaneMockConsentRequest(t, m, f, false)
_ = SaneMockHandleConsentRequest(t, m, f, cr, time.Time{}, 0, false, false)
_, err = m.VerifyAndInvalidateConsentRequest(ctx, f, x.Must(f.ToConsentVerifier(ctx, deps)))
_, err = m.VerifyAndInvalidateConsentRequest(ctx, x.Must(f.ToConsentVerifier(ctx, deps)))
require.NoError(t, err)

sessions[k] = *ls
Expand Down
4 changes: 2 additions & 2 deletions consent/sdk_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ func TestSDK(t *testing.T) {
_, err = m.HandleConsentRequest(context.Background(), cr4Flow, hcr4)
require.NoError(t, err)

_, err = m.VerifyAndInvalidateConsentRequest(context.Background(), cr3Flow, consentVerifier(cr3Flow))
_, err = m.VerifyAndInvalidateConsentRequest(context.Background(), consentVerifier(cr3Flow))
require.NoError(t, err)
_, err = m.VerifyAndInvalidateConsentRequest(context.Background(), cr4Flow, consentVerifier(cr4Flow))
_, err = m.VerifyAndInvalidateConsentRequest(context.Background(), consentVerifier(cr4Flow))
require.NoError(t, err)

lur1 := MockLogoutRequest("testsdk-1", true, network)
Expand Down
52 changes: 16 additions & 36 deletions consent/strategy_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,6 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(ctx context.Context, w ht
return errorsx.WithStack(err)
}

if err := flowctx.SetCookie(ctx, w, s.r, flowctx.FlowCookie(cl), f); err != nil {
return err
}

store, err := s.r.CookieStore(ctx)
if err != nil {
return err
Expand Down Expand Up @@ -356,15 +352,13 @@ func (s *DefaultStrategy) verifyAuthentication(
ctx, span := trace.SpanFromContext(ctx).TracerProvider().Tracer("").Start(ctx, "DefaultStrategy.verifyAuthentication")
defer otelx.End(span, &err)

f, err := s.flowFromCookie(r)
// We decode the flow from the cookie again because VerifyAndInvalidateLoginRequest does not return the flow
f, err := flowctx.Decode[flow.Flow](ctx, s.r.FlowCipher(), verifier, flowctx.AsLoginVerifier)
if err != nil {
return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The flow cookie is missing in the request."))
}
if f.Client.GetID() != req.GetClient().GetID() {
return nil, errorsx.WithStack(fosite.ErrInvalidClient.WithHint("The flow cookie client id does not match the authorize request client id."))
return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The login verifier is invalid."))
}

session, err := s.r.ConsentManager().VerifyAndInvalidateLoginRequest(ctx, f, verifier)
session, err := s.r.ConsentManager().VerifyAndInvalidateLoginRequest(ctx, verifier)
if errors.Is(err, sqlcon.ErrNoRows) {
return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The login verifier has already been used, has not been granted, or is invalid."))
} else if err != nil {
Expand Down Expand Up @@ -470,6 +464,9 @@ func (s *DefaultStrategy) verifyAuthentication(

loginSession.IdentityProviderSessionID = sqlxx.NullString(session.IdentityProviderSessionID)
if err := s.r.ConsentManager().ConfirmLoginSession(ctx, loginSession, sessionID, time.Time(session.AuthenticatedAt), session.Subject, session.Remember); err != nil {
if errors.Is(err, sqlcon.ErrUniqueViolation) {
return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The login verifier has already been used."))
}
return nil, err
}
}
Expand Down Expand Up @@ -511,10 +508,6 @@ func (s *DefaultStrategy) verifyAuthentication(
"cookie_secure": s.c.CookieSecure(ctx),
}).Debug("Authentication session cookie was set.")

if err = flowctx.SetCookie(ctx, w, s.r, flowctx.FlowCookie(flowctx.SuffixForClient(req.GetClient())), f); err != nil {
return nil, errorsx.WithStack(err)
}

return f, nil
}

Expand Down Expand Up @@ -630,9 +623,6 @@ func (s *DefaultStrategy) forwardConsentRequest(
return errorsx.WithStack(err)
}

if err := flowctx.SetCookie(ctx, w, s.r, flowctx.FlowCookie(cl), f); err != nil {
return err
}
consentChallenge, err := f.ToConsentChallenge(ctx, s.r)
if err != nil {
return err
Expand All @@ -658,20 +648,23 @@ func (s *DefaultStrategy) forwardConsentRequest(
return errorsx.WithStack(ErrAbortOAuth2Request)
}

func (s *DefaultStrategy) verifyConsent(ctx context.Context, w http.ResponseWriter, r *http.Request, verifier string) (_ *flow.AcceptOAuth2ConsentRequest, _ *flow.Flow, err error) {
func (s *DefaultStrategy) verifyConsent(ctx context.Context, _ http.ResponseWriter, r *http.Request, verifier string) (_ *flow.AcceptOAuth2ConsentRequest, _ *flow.Flow, err error) {
ctx, span := trace.SpanFromContext(ctx).TracerProvider().Tracer("").Start(ctx, "DefaultStrategy.verifyConsent")
defer otelx.End(span, &err)

f, err := s.flowFromCookie(r)
// We decode the flow here once again because VerifyAndInvalidateConsentRequest does not return the flow
f, err := flowctx.Decode[flow.Flow](ctx, s.r.FlowCipher(), verifier, flowctx.AsConsentVerifier)
if err != nil {
return nil, nil, err
return nil, nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The consent verifier has already been used, has not been granted, or is invalid."))
}
if f.Client.GetID() != r.URL.Query().Get("client_id") {
return nil, nil, errorsx.WithStack(fosite.ErrInvalidClient.WithHint("The flow cookie client id does not match the authorize request client id."))
return nil, nil, errorsx.WithStack(fosite.ErrInvalidClient.WithHint("The flow client id does not match the authorize request client id."))
}

session, err := s.r.ConsentManager().VerifyAndInvalidateConsentRequest(ctx, f, verifier)
if errors.Is(err, sqlcon.ErrNoRows) {
session, err := s.r.ConsentManager().VerifyAndInvalidateConsentRequest(ctx, verifier)
if errors.Is(err, sqlcon.ErrUniqueViolation) {
return nil, nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The consent verifier has already been used."))
} else if errors.Is(err, sqlcon.ErrNoRows) {
return nil, nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The consent verifier has already been used, has not been granted, or is invalid."))
} else if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -700,10 +693,6 @@ func (s *DefaultStrategy) verifyConsent(ctx context.Context, w http.ResponseWrit
return nil, nil, err
}

if err = flowctx.DeleteCookie(ctx, w, s.r, flowctx.FlowCookie(f.Client)); err != nil {
return nil, nil, err
}

if session.Session == nil {
session.Session = flow.NewConsentRequestSessionData()
}
Expand Down Expand Up @@ -1187,15 +1176,6 @@ func (s *DefaultStrategy) ObfuscateSubjectIdentifier(ctx context.Context, cl fos
return subject, nil
}

func (s *DefaultStrategy) flowFromCookie(r *http.Request) (*flow.Flow, error) {
clientID := r.URL.Query().Get("client_id")
if clientID == "" {
return nil, errors.WithStack(fosite.ErrInvalidClient)
}

return flowctx.FromCookie[flow.Flow](r.Context(), r, s.r.FlowCipher(), flowctx.FlowCookie(flowctx.SuffixFromStatic(clientID)))
}

func (s *DefaultStrategy) loginSessionFromCookie(r *http.Request) *flow.LoginSession {
clientID := r.URL.Query().Get("client_id")
if clientID == "" {
Expand Down
82 changes: 72 additions & 10 deletions consent/strategy_oauth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ import (

"github.com/ory/hydra/v2/aead"
"github.com/ory/hydra/v2/consent"
"github.com/ory/hydra/v2/flow"
"github.com/ory/hydra/v2/oauth2/flowctx"
"github.com/ory/hydra/v2/x"

"github.com/ory/x/pointerx"

"github.com/tidwall/gjson"
Expand Down Expand Up @@ -122,7 +118,7 @@ func TestStrategyLoginConsentNext(t *testing.T) {

makeRequestAndExpectError(
t, hc, c, url.Values{"login_verifier": {"does-not-exist"}},
"The login verifier has already been used, has not been granted, or is invalid.",
"The resource owner or authorization server denied the request. The login verifier is invalid",
)
})

Expand Down Expand Up @@ -206,6 +202,76 @@ func TestStrategyLoginConsentNext(t *testing.T) {
makeRequestAndExpectError(t, nil, c, url.Values{}, "expect-reject-consent")
})

t.Run("suite=double-submit", func(t *testing.T) {
ctx := context.Background()
c := createDefaultClient(t)
hc := testhelpers.NewEmptyJarClient(t)
var loginChallenge, consentChallenge string

testhelpers.NewLoginConsentUI(t, reg.Config(),
func(w http.ResponseWriter, r *http.Request) {
res, _, err := adminClient.OAuth2Api.GetOAuth2LoginRequest(ctx).
LoginChallenge(r.URL.Query().Get("login_challenge")).
Execute()
require.NoError(t, err)
loginChallenge = res.Challenge

v, _, err := adminClient.OAuth2Api.AcceptOAuth2LoginRequest(ctx).
LoginChallenge(loginChallenge).
AcceptOAuth2LoginRequest(hydra.AcceptOAuth2LoginRequest{Subject: "aeneas-rekkas"}).
Execute()
require.NoError(t, err)
require.NotEmpty(t, v.RedirectTo)
http.Redirect(w, r, v.RedirectTo, http.StatusFound)
},
func(w http.ResponseWriter, r *http.Request) {
res, _, err := adminClient.OAuth2Api.GetOAuth2ConsentRequest(ctx).
ConsentChallenge(r.URL.Query().Get("consent_challenge")).
Execute()
require.NoError(t, err)
consentChallenge = res.Challenge

v, _, err := adminClient.OAuth2Api.AcceptOAuth2ConsentRequest(ctx).
ConsentChallenge(consentChallenge).
AcceptOAuth2ConsentRequest(hydra.AcceptOAuth2ConsentRequest{}).
Execute()
require.NoError(t, err)
require.NotEmpty(t, v.RedirectTo)
http.Redirect(w, r, v.RedirectTo, http.StatusFound)
})

makeRequestAndExpectCode(t, hc, c, url.Values{})

t.Run("case=double-submit login verifier", func(t *testing.T) {
v, _, err := adminClient.OAuth2Api.AcceptOAuth2LoginRequest(ctx).
LoginChallenge(loginChallenge).
AcceptOAuth2LoginRequest(hydra.AcceptOAuth2LoginRequest{Subject: "aeneas-rekkas"}).
Execute()
require.NoError(t, err)
res, err := hc.Get(v.RedirectTo)
require.NoError(t, err)
q := res.Request.URL.Query()
assert.Equal(t,
"The resource owner or authorization server denied the request. The login verifier has already been used.",
q.Get("error_description"), q)
})

t.Run("case=double-submit consent verifier", func(t *testing.T) {
v, _, err := adminClient.OAuth2Api.AcceptOAuth2ConsentRequest(ctx).
ConsentChallenge(consentChallenge).
AcceptOAuth2ConsentRequest(hydra.AcceptOAuth2ConsentRequest{}).
Execute()
require.NoError(t, err)
res, err := hc.Get(v.RedirectTo)
require.NoError(t, err)
q := res.Request.URL.Query()
assert.Equal(t,
"The resource owner or authorization server denied the request. The consent verifier has already been used.",
q.Get("error_description"), q)
})

})

t.Run("case=should pass and set acr values properly", func(t *testing.T) {
c := createDefaultClient(t)
testhelpers.NewLoginConsentUI(t, reg.Config(),
Expand Down Expand Up @@ -1057,18 +1123,14 @@ func (d *dropCSRFCookieJar) Cookies(u *url.URL) []*http.Cookie {
return d.jar.Cookies(u)
}

// TODO(hperl): rename
func newHTTPClientWithFlowCookie(t *testing.T, ctx context.Context, reg interface {
ConsentManager() consent.Manager
Config() *config.DefaultProvider
FlowCipher() *aead.XChaCha20Poly1305
}, c *client.Client) *http.Client {
f, err := reg.ConsentManager().CreateLoginRequest(ctx, &flow.LoginRequest{Client: c})
require.NoError(t, err)

hc := testhelpers.NewEmptyJarClient(t)
hc.Jar.SetCookies(reg.Config().OAuth2AuthURL(ctx), []*http.Cookie{
{Name: flowctx.FlowCookie(c), Value: x.Must(flowctx.Encode(ctx, reg.FlowCipher(), f))},
})

return hc
}
Loading

0 comments on commit cde3a30

Please sign in to comment.