Skip to content

Commit

Permalink
Merge 606c405 into a0c06ec
Browse files Browse the repository at this point in the history
  • Loading branch information
@hperl
hperl authored Oct 16, 2023
2 parents a0c06ec + 606c405 commit 1a65ee3
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 1a65ee3

Please sign in to comment.