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.

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

feat: remove flow cookie #3639

Merged
merged 5 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions consent/manager.go
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
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)
Copy link
Member

Choose a reason for hiding this comment

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

Should this assertion not still be here? The flow was handled so it should be true right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we persisted the flow in the database, so VerifyAndInvalidateLoginRequest set WasHandled to true in the DB. Since we now store the flow in the login verifier, there is no place to store information about whether the flow was already handled.

This is already true even without this patch.

If we want double-submit protection, we should build a separate NonceService (new creates a new nonce, use uses a nonce once) to make sure that the verifiers are only used once.

Copy link
Member

Choose a reason for hiding this comment

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

I see - do we protect against double submit for the consent verifier? Or can I generate as many authorization codes as I want with the same consent verifier?

Can I log in as often as I want to using the same login verifier?

Copy link
Contributor Author

@hperl hperl Oct 11, 2023

Choose a reason for hiding this comment

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

You can log in as often as you want, but the double-submit will be caught at the consent verification step.

I added a test case for consent verifier double submit here: f482f4e

})
}
})
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)
Copy link
Member

Choose a reason for hiding this comment

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

Why is it ok to remove this assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we can't detect double submit any more.

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
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
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."))
hperl marked this conversation as resolved.
Show resolved Hide resolved
}
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
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
Loading