Skip to content

Commit

Permalink
refactor: update device session persistence logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@nsklikas
nsklikas committed Nov 18, 2024
1 parent 93e49bf commit 0e55f5c
Show file tree
Hide file tree
Showing 27 changed files with 367 additions and 232 deletions.
21 changes: 9 additions & 12 deletions consent/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,12 +340,13 @@ func TestAcceptDeviceRequest(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
_, deviceCodesig, err := reg.RFC8628HMACStrategy().GenerateDeviceCode(ctx)
require.NoError(t, err)
userCode, sig, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
require.NoError(t, err)
reg.OAuth2Storage().CreateUserCodeSession(ctx, sig, deviceRequest)
reg.OAuth2Storage().CreateDeviceAuthSession(ctx, deviceCodesig, sig, deviceRequest)
require.NoError(t, err)

acceptUserCode := &hydra.AcceptDeviceUserCodeRequest{UserCode: &userCode}
Expand Down Expand Up @@ -408,12 +409,13 @@ func TestAcceptDuplicateDeviceRequest(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
_, deviceCodesig, err := reg.RFC8628HMACStrategy().GenerateDeviceCode(ctx)
require.NoError(t, err)
userCode, sig, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
require.NoError(t, err)
reg.OAuth2Storage().CreateUserCodeSession(ctx, sig, deviceRequest)
reg.OAuth2Storage().CreateDeviceAuthSession(ctx, deviceCodesig, sig, deviceRequest)
require.NoError(t, err)

acceptUserCode := &hydra.AcceptDeviceUserCodeRequest{UserCode: &userCode}
Expand Down Expand Up @@ -499,7 +501,6 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
userCode, _, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
Expand All @@ -523,7 +524,6 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
userCode := ""
Expand All @@ -546,7 +546,6 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
userCode, _, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
Expand All @@ -570,7 +569,6 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
userCode, _, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
Expand All @@ -594,22 +592,22 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
_, deviceCodesig, err := reg.RFC8628HMACStrategy().GenerateDeviceCode(ctx)
require.NoError(t, err)
userCode, sig, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
require.NoError(t, err)
deviceRequest.SetSession(
&oauth2.Session{
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
exp := time.Now().UTC()
deviceRequest.Session.SetExpiresAt(fosite.UserCode, exp)
err = reg.OAuth2Storage().CreateUserCodeSession(ctx, sig, deviceRequest)
err = reg.OAuth2Storage().CreateDeviceAuthSession(ctx, deviceCodesig, sig, deviceRequest)
require.NoError(t, err)
return json.Marshal(&hydra.AcceptDeviceUserCodeRequest{UserCode: &userCode})
},
Expand All @@ -633,7 +631,6 @@ func TestAcceptCodeDeviceRequestFailure(t *testing.T) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
},
)
userCode, _, err := reg.RFC8628HMACStrategy().GenerateUserCode(ctx)
Expand Down
3 changes: 0 additions & 3 deletions consent/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package consent
import (
"context"

"github.com/gobuffalo/pop/v6"
"github.com/gofrs/uuid"

"github.com/ory/hydra/v2/client"
Expand Down Expand Up @@ -66,8 +65,6 @@ type (
GetDeviceUserAuthRequest(ctx context.Context, challenge string) (*flow.DeviceUserAuthRequest, error)
HandleDeviceUserAuthRequest(ctx context.Context, f *flow.Flow, challenge string, r *flow.HandledDeviceUserAuthRequest) (*flow.DeviceUserAuthRequest, error)
VerifyAndInvalidateDeviceUserAuthRequest(ctx context.Context, verifier string) (*flow.HandledDeviceUserAuthRequest, error)

Transaction(context.Context, func(ctx context.Context, c *pop.Connection) error) error
}

ManagerProvider interface {
Expand Down
14 changes: 4 additions & 10 deletions consent/strategy_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"strings"
"time"

"github.com/gobuffalo/pop/v6"
"github.com/gorilla/sessions"
"github.com/hashicorp/go-retryablehttp"
"github.com/pborman/uuid"
Expand Down Expand Up @@ -1243,15 +1242,10 @@ func (s *DefaultStrategy) HandleOAuth2DeviceAuthorizationRequest(
var consentSession *flow.AcceptOAuth2ConsentRequest
var f *flow.Flow

err = s.r.ConsentManager().Transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
consentSession, f, err = s.verifyConsent(ctx, w, r, consentVerifier)
if err != nil {
return err
}
err = s.r.OAuth2Storage().UpdateAndInvalidateUserCodeSessionByRequestID(ctx, string(f.DeviceCodeRequestID), f.ID)

return err
})
consentSession, f, err = s.verifyConsent(ctx, w, r, consentVerifier)
if err != nil {
return nil, nil, err
}

return consentSession, f, err
}
Expand Down
3 changes: 1 addition & 2 deletions ..._request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ..._age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...thentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ..._request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ..._age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ...thentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=legacy.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"requester": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions ..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"consent_challenge": "",
"exclude_not_before_claim": false,
"allowed_top_level_claims": [],
"mirror_top_level_claims": true,
"browser_flow_completed": false
"mirror_top_level_claims": true
},
"request": {
"client_id": "app-client",
Expand Down
3 changes: 1 addition & 2 deletions oauth2/.snapshots/TestUnmarshalSession-v1.11.8.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,5 @@
"zone",
"login_session_id"
],
"mirror_top_level_claims": false,
"browser_flow_completed": false
"mirror_top_level_claims": false
}
3 changes: 1 addition & 2 deletions oauth2/.snapshots/TestUnmarshalSession-v1.11.9.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,5 @@
"zone",
"login_session_id"
],
"mirror_top_level_claims": false,
"browser_flow_completed": false
"mirror_top_level_claims": false
}
3 changes: 1 addition & 2 deletions oauth2/fixtures/v1.11.8-session.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,5 @@
"market",
"zone",
"login_session_id"
],
"BrowserFlowCompleted": false
]
}
3 changes: 1 addition & 2 deletions oauth2/fixtures/v1.11.9-session.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,5 @@
"market",
"zone",
"login_session_id"
],
"browser_flow_completed": false
]
}
20 changes: 14 additions & 6 deletions oauth2/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,15 @@ func (h *Handler) getOidcUserInfo(w http.ResponseWriter, r *http.Request) {
func (h *Handler) performOAuth2DeviceVerificationFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
ctx := r.Context()

// When this endpoint is called with a valid consent_verifier (meaning that the login flow completed successfully)
// there are 3 writes happening to the database:
// - The flow is created
// - The device auth session is updated (user_code is marked as accepted)
// - The OpenID session is created
// If there were multiple flows created for the same user_code then we may end up with multiple flow objects
// persisted to the database, while only one of them was actually used to validate the user_code
// (see https://github.com/ory/hydra/pull/3851#discussion_r1843678761)
// TODO: We should wrap these queries in a transaction
consentSession, f, err := h.r.ConsentStrategy().HandleOAuth2DeviceAuthorizationRequest(ctx, w, r)
if errors.Is(err, consent.ErrAbortOAuth2Request) {
x.LogAudit(r, nil, h.r.AuditLogger())
Expand All @@ -747,26 +756,26 @@ func (h *Handler) performOAuth2DeviceVerificationFlow(w http.ResponseWriter, r *
return
}

req, err := h.r.OAuth2Storage().GetDeviceCodeSessionByRequestID(ctx, f.DeviceCodeRequestID.String(), &Session{})
req, sig, err := h.r.OAuth2Storage().GetDeviceCodeSessionByRequestID(ctx, f.DeviceCodeRequestID.String(), &Session{})
if err != nil {
x.LogError(r, err, h.r.Logger())
h.r.Writer().WriteError(w, r, err)
return
}
req.SetUserCodeState(fosite.UserCodeAccepted)
session, err := h.updateSessionWithRequest(ctx, consentSession, f, r, req, req.GetSession().(*Session))
if err != nil {
h.r.Writer().WriteError(w, r, err)
return
}
session.SetBrowserFlowCompleted(true)

req.SetSession(session)
// Update the device code session with
// - the claims for which the user gave consent
// - the granted scopes
// - the granted audiences
// - the user_code_state set to accepted
// This marks it as ready to be used for the token exchange endpoint.
err = h.r.OAuth2Storage().UpdateDeviceCodeSessionByRequestID(ctx, f.DeviceCodeRequestID.String(), req)
err = h.r.OAuth2Storage().UpdateDeviceCodeSessionBySignature(ctx, sig, req)
if err != nil {
x.LogError(r, err, h.r.Logger())
h.r.Writer().WriteError(w, r, err)
Expand All @@ -775,7 +784,7 @@ func (h *Handler) performOAuth2DeviceVerificationFlow(w http.ResponseWriter, r *

// Update the OpenID Connect session if "openid" scope is granted
if req.GetGrantedScopes().Has("openid") {
err = h.r.OAuth2Storage().CreateOpenIDConnectSession(ctx, req.GetID(), req.Sanitize([]string{"grant_type",
err = h.r.OAuth2Storage().CreateOpenIDConnectSession(ctx, sig, req.Sanitize([]string{"grant_type",
"max_age",
"prompt",
"acr_values",
Expand Down Expand Up @@ -868,7 +877,6 @@ func (h *Handler) oAuth2DeviceFlow(w http.ResponseWriter, r *http.Request) {
DefaultSession: &openid.DefaultSession{
Headers: &jwt.Headers{},
},
BrowserFlowCompleted: false,
}

resp, err := h.r.OAuth2Provider().NewDeviceResponse(ctx, request, session)
Expand Down
Loading

0 comments on commit 0e55f5c

Please sign in to comment.