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 15, 2024
1 parent 4f81698 commit b9e7e12
Show file tree
Hide file tree
Showing 27 changed files with 351 additions and 229 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
]
}
5 changes: 2 additions & 3 deletions oauth2/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,18 +753,18 @@ func (h *Handler) performOAuth2DeviceVerificationFlow(w http.ResponseWriter, r *
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)
if err != nil {
Expand Down Expand Up @@ -862,7 +862,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
17 changes: 9 additions & 8 deletions oauth2/oauth2_device_code_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,15 @@ func TestDeviceTokenRequest(t *testing.T) {

testCases := []struct {
description string
setUp func(signature string)
setUp func(signature, userCodeSignature string)
check func(t *testing.T, token *oauth2.Token, err error)
cleanUp func()
}{
{
description: "should pass with refresh token",
setUp: func(signature string) {
setUp: func(signature, userCodeSignature string) {
authreq := &fosite.DeviceRequest{
UserCodeState: fosite.UserCodeAccepted,
Request: fosite.Request{
Client: &fosite.DefaultClient{
ID: c.GetID(),
Expand All @@ -153,13 +154,12 @@ func TestDeviceTokenRequest(t *testing.T) {
fosite.DeviceCode: time.Now().Add(time.Hour).UTC(),
},
},
BrowserFlowCompleted: true,
},
RequestedAt: time.Now(),
},
}

require.NoError(t, reg.OAuth2Storage().CreateDeviceCodeSession(context.TODO(), signature, authreq))
require.NoError(t, reg.OAuth2Storage().CreateDeviceAuthSession(context.TODO(), signature, userCodeSignature, authreq))
},
check: func(t *testing.T, token *oauth2.Token, err error) {
assert.NotEmpty(t, token.AccessToken)
Expand All @@ -168,8 +168,9 @@ func TestDeviceTokenRequest(t *testing.T) {
},
{
description: "should pass with ID token",
setUp: func(signature string) {
setUp: func(signature, userCodeSignature string) {
authreq := &fosite.DeviceRequest{
UserCodeState: fosite.UserCodeAccepted,
Request: fosite.Request{
Client: &fosite.DefaultClient{
ID: c.GetID(),
Expand All @@ -186,13 +187,12 @@ func TestDeviceTokenRequest(t *testing.T) {
fosite.DeviceCode: time.Now().Add(time.Hour).UTC(),
},
},
BrowserFlowCompleted: true,
},
RequestedAt: time.Now(),
},
}

require.NoError(t, reg.OAuth2Storage().CreateDeviceCodeSession(context.TODO(), signature, authreq))
require.NoError(t, reg.OAuth2Storage().CreateDeviceAuthSession(context.TODO(), signature, userCodeSignature, authreq))
require.NoError(t, reg.OAuth2Storage().CreateOpenIDConnectSession(context.TODO(), signature, authreq))
},
check: func(t *testing.T, token *oauth2.Token, err error) {
Expand All @@ -206,10 +206,11 @@ func TestDeviceTokenRequest(t *testing.T) {
for _, testCase := range testCases {
t.Run("case="+testCase.description, func(t *testing.T) {
code, signature, err := reg.RFC8628HMACStrategy().GenerateDeviceCode(context.TODO())
_, userCodeSignature, err := reg.RFC8628HMACStrategy().GenerateUserCode(context.TODO())
require.NoError(t, err)

if testCase.setUp != nil {
testCase.setUp(signature)
testCase.setUp(signature, userCodeSignature)
}

var token *oauth2.Token
Expand Down
Loading

0 comments on commit b9e7e12

Please sign in to comment.