Skip to content

Commit

Permalink
feat: remove flow cookie
Browse files Browse the repository at this point in the history
  • Loading branch information
@hperl
hperl committed Sep 29, 2023
1 parent 89b1b1b commit 16fe681
Show file tree
Hide file tree
Showing 10 changed files with 387 additions and 147 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
43 changes: 13 additions & 30 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,18 @@ 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)

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 +630,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 +649,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 +748,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 +920,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
45 changes: 10 additions & 35 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 has already been used, has not been granted, or 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 @@ -511,10 +505,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 +620,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,19 +645,20 @@ 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)
session, err := s.r.ConsentManager().VerifyAndInvalidateConsentRequest(ctx, verifier)
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 {
Expand Down Expand Up @@ -700,10 +688,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 +1171,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
10 changes: 1 addition & 9 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 @@ -1057,18 +1053,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 16fe681

Please sign in to comment.