Skip to content

Commit feb95a9

Browse files
nsklikasnsklikas
committed
chore: address review comments
1 parent c4789ab commit feb95a9

35 files changed

+78
-94
lines changed

‎.schema/config.schema.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1181,7 +1181,7 @@
11811181
}
11821182
],
11831183
"default": "5s",
1184-
"description": "configure how often a non-interactive device should poll the device token endpoint",
1184+
"description": "Configures how often a non-interactive device should poll the device token endpoint, this is a purely informational configuration and does not enforce rate-limiting.",
11851185
"examples": ["5s", "15s", "1m"]
11861186
},
11871187
"user_code_entropy": {

‎consent/handler_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func TestGetLoginRequest(t *testing.T) {
108108
if tc.exists {
109109
cl := &client.Client{ID: "client" + key}
110110
require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
111-
f, err := reg.ConsentManager().CreateLoginRequest(context.Background(), nil, &flow.LoginRequest{
111+
f, err := reg.ConsentManager().CreateLoginRequest(context.Background(), &flow.LoginRequest{
112112
Client: cl,
113113
ID: challenge,
114114
RequestURL: requestURL,
@@ -180,7 +180,7 @@ func TestGetConsentRequest(t *testing.T) {
180180
RequestURL: requestURL,
181181
RequestedAt: time.Now(),
182182
}
183-
f, err := reg.ConsentManager().CreateLoginRequest(ctx, nil, lr)
183+
f, err := reg.ConsentManager().CreateLoginRequest(ctx, lr)
184184
require.NoError(t, err)
185185
challenge, err = f.ToLoginChallenge(ctx, reg)
186186
require.NoError(t, err)
@@ -247,7 +247,7 @@ func TestGetLoginRequestWithDuplicateAccept(t *testing.T) {
247247

248248
cl := &client.Client{ID: "client"}
249249
require.NoError(t, reg.ClientManager().CreateClient(ctx, cl))
250-
f, err := reg.ConsentManager().CreateLoginRequest(ctx, nil, &flow.LoginRequest{
250+
f, err := reg.ConsentManager().CreateLoginRequest(ctx, &flow.LoginRequest{
251251
Client: cl,
252252
ID: challenge,
253253
RequestURL: requestURL,

‎consent/manager.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ type (
4545
RevokeSubjectLoginSession(ctx context.Context, user string) error
4646
ConfirmLoginSession(ctx context.Context, loginSession *flow.LoginSession) error
4747

48-
CreateLoginRequest(ctx context.Context, f *flow.Flow, req *flow.LoginRequest) (*flow.Flow, error)
48+
CreateLoginRequest(ctx context.Context, req *flow.LoginRequest) (*flow.Flow, error)
49+
CreateLoginRequestFromDeviceRequest(ctx context.Context, f *flow.Flow, req *flow.LoginRequest) (*flow.Flow, error)
4950
GetLoginRequest(ctx context.Context, challenge string) (*flow.LoginRequest, error)
5051
HandleLoginRequest(ctx context.Context, f *flow.Flow, challenge string, r *flow.HandledLoginRequest) (*flow.LoginRequest, error)
5152
VerifyAndInvalidateLoginRequest(ctx context.Context, verifier string) (*flow.HandledLoginRequest, error)

‎consent/strategy_default.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,11 +279,12 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(
279279
LoginHint: ar.GetRequestForm().Get("login_hint"),
280280
},
281281
}
282-
f, err := s.r.ConsentManager().CreateLoginRequest(
283-
ctx,
284-
f,
285-
loginRequest,
286-
)
282+
var err error
283+
if f == nil {
284+
f, err = s.r.ConsentManager().CreateLoginRequest(ctx, loginRequest)
285+
} else {
286+
f, err = s.r.ConsentManager().CreateLoginRequestFromDeviceRequest(ctx, f, loginRequest)
287+
}
287288
if err != nil {
288289
return errorsx.WithStack(err)
289290
}
@@ -1293,7 +1294,12 @@ func (s *DefaultStrategy) forwardDeviceRequest(ctx context.Context, w http.Respo
12931294

12941295
// Generate the request URL
12951296
iu := s.getDeviceVerificationPath(ctx)
1296-
iu.RawQuery = r.URL.RawQuery
1297+
// We don't want the user_code persisted in the database
1298+
q := r.URL.Query()
1299+
if q.Has("user_code") {
1300+
q.Set("user_code", "****")
1301+
}
1302+
iu.RawQuery = q.Encode()
12971303

12981304
f, err := s.r.ConsentManager().CreateDeviceUserAuthRequest(
12991305
r.Context(),

‎consent/test/manager_test_helpers.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ func SaneMockAuthRequest(t *testing.T, m consent.Manager, ls *flow.LoginSession,
301301
ID: uuid.New().String(),
302302
Verifier: uuid.New().String(),
303303
}
304-
_, err := m.CreateLoginRequest(context.Background(), nil, c)
304+
_, err := m.CreateLoginRequest(context.Background(), c)
305305
require.NoError(t, err)
306306
return c
307307
}
@@ -346,9 +346,9 @@ func TestHelperNID(r interface {
346346
require.Error(t, t2InvalidNID.CreateLoginSession(ctx, &testLS))
347347
require.NoError(t, t1ValidNID.CreateLoginSession(ctx, &testLS))
348348

349-
_, err := t2InvalidNID.CreateLoginRequest(ctx, nil, &testLR)
349+
_, err := t2InvalidNID.CreateLoginRequest(ctx, &testLR)
350350
require.Error(t, err)
351-
f, err := t1ValidNID.CreateLoginRequest(ctx, nil, &testLR)
351+
f, err := t1ValidNID.CreateLoginRequest(ctx, &testLR)
352352
require.NoError(t, err)
353353

354354
testLR.ID = x.Must(f.ToLoginChallenge(ctx, r))
@@ -406,7 +406,7 @@ func ManagerTests(deps Deps, m consent.Manager, clientManager client.Manager, fo
406406
RequestedAt: time.Now(),
407407
}
408408

409-
_, err := m.CreateLoginRequest(ctx, nil, lr[k])
409+
_, err := m.CreateLoginRequest(ctx, lr[k])
410410
require.NoError(t, err)
411411
}
412412
})
@@ -581,7 +581,7 @@ func ManagerTests(deps Deps, m consent.Manager, clientManager client.Manager, fo
581581
_, err := m.GetLoginRequest(ctx, loginChallenge)
582582
require.Error(t, err)
583583

584-
f, err = m.CreateLoginRequest(ctx, nil, c)
584+
f, err = m.CreateLoginRequest(ctx, c)
585585
require.NoError(t, err)
586586

587587
loginChallenge = x.Must(f.ToLoginChallenge(ctx, deps))
@@ -852,9 +852,9 @@ func ManagerTests(deps Deps, m consent.Manager, clientManager client.Manager, fo
852852
})
853853

854854
t.Run("case=list-used-consent-requests", func(t *testing.T) {
855-
f1, err := m.CreateLoginRequest(ctx, nil, lr["rv1"])
855+
f1, err := m.CreateLoginRequest(ctx, lr["rv1"])
856856
require.NoError(t, err)
857-
f2, err := m.CreateLoginRequest(ctx, nil, lr["rv2"])
857+
f2, err := m.CreateLoginRequest(ctx, lr["rv2"])
858858
require.NoError(t, err)
859859

860860
cr1, hcr1, _ := MockConsentRequest("rv1", true, 0, false, false, false, "fk-login-challenge", network)
@@ -1174,7 +1174,7 @@ func ManagerTests(deps Deps, m consent.Manager, clientManager client.Manager, fo
11741174
SessionID: sqlxx.NullString(s.ID),
11751175
}
11761176

1177-
f, err := m.CreateLoginRequest(ctx, nil, lr)
1177+
f, err := m.CreateLoginRequest(ctx, lr)
11781178
require.NoError(t, err)
11791179
expected := &flow.OAuth2ConsentRequest{
11801180
ID: uuid.NewString(),

‎flow/flow.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,8 +230,6 @@ type Flow struct {
230230
DeviceVerifier sqlxx.NullString `db:"device_verifier" json:"dv,omitempty"`
231231
// DeviceVerifier is the device request's CSRF
232232
DeviceCSRF sqlxx.NullString `db:"device_csrf" json:"dc,omitempty"`
233-
// DeviceUserCodeAcceptedAt is the time when device user_code was accepted
234-
DeviceUserCodeAcceptedAt sqlxx.NullTime `db:"device_user_code_accepted_at" json:"da,omitempty"`
235233
// DeviceWasUsed set to true means that the device request was already handled
236234
DeviceWasUsed sqlxx.NullBool `db:"device_was_used" json:"du,omitempty"`
237235
// DeviceHandledAt contains the timestamp the device user_code verification request was handled

‎internal/testhelpers/janitor_test_helper.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ func (j *JanitorConsentTestHelper) LoginRejectionSetup(ctx context.Context, reg
192192
// Create login requests
193193
for _, r := range j.flushLoginRequests {
194194
require.NoError(t, cl.CreateClient(ctx, r.Client))
195-
f, err := cm.CreateLoginRequest(ctx, nil, r)
195+
f, err := cm.CreateLoginRequest(ctx, r)
196196
require.NoError(t, err)
197197

198198
f.RequestedAt = time.Now() // we won't handle expired flows
@@ -246,7 +246,7 @@ func (j *JanitorConsentTestHelper) LimitSetup(ctx context.Context, reg interface
246246
// Create login requests
247247
for _, r := range j.flushLoginRequests {
248248
require.NoError(t, cl.CreateClient(ctx, r.Client))
249-
f, err = cm.CreateLoginRequest(ctx, nil, r)
249+
f, err = cm.CreateLoginRequest(ctx, r)
250250
require.NoError(t, err)
251251

252252
// Reject each request
@@ -290,7 +290,7 @@ func (j *JanitorConsentTestHelper) ConsentRejectionSetup(ctx context.Context, re
290290
// Create login requests
291291
for i, loginRequest := range j.flushLoginRequests {
292292
require.NoError(t, cl.CreateClient(ctx, loginRequest.Client))
293-
f, err = cm.CreateLoginRequest(ctx, nil, loginRequest)
293+
f, err = cm.CreateLoginRequest(ctx, loginRequest)
294294
require.NoError(t, err)
295295

296296
// Create consent requests
@@ -345,7 +345,7 @@ func (j *JanitorConsentTestHelper) LoginTimeoutSetup(ctx context.Context, reg in
345345
// Create login requests
346346
for i, loginRequest := range j.flushLoginRequests {
347347
require.NoError(t, cl.CreateClient(ctx, loginRequest.Client))
348-
f, err = cm.CreateLoginRequest(ctx, nil, loginRequest)
348+
f, err = cm.CreateLoginRequest(ctx, loginRequest)
349349
require.NoError(t, err)
350350

351351
if i == 0 {
@@ -386,7 +386,7 @@ func (j *JanitorConsentTestHelper) ConsentTimeoutSetup(ctx context.Context, reg
386386
// Let's reset and accept all login requests to test the consent requests
387387
for i, loginRequest := range j.flushLoginRequests {
388388
require.NoError(t, cl.CreateClient(ctx, loginRequest.Client))
389-
f, err := cm.CreateLoginRequest(ctx, nil, loginRequest)
389+
f, err := cm.CreateLoginRequest(ctx, loginRequest)
390390
require.NoError(t, err)
391391
f.RequestedAt = time.Now() // we won't handle expired flows
392392
challenge := x.Must(f.ToLoginChallenge(ctx, reg))
@@ -438,7 +438,7 @@ func (j *JanitorConsentTestHelper) LoginConsentNotAfterSetup(ctx context.Context
438438
)
439439
for _, r := range j.flushLoginRequests {
440440
require.NoError(t, cl.CreateClient(ctx, r.Client))
441-
f, err = cm.CreateLoginRequest(ctx, nil, r)
441+
f, err = cm.CreateLoginRequest(ctx, r)
442442
require.NoError(t, err)
443443
}
444444

@@ -470,7 +470,7 @@ func (j *JanitorConsentTestHelper) LoginConsentNotAfterValidate(
470470
t.Logf("login flush check:\nNotAfter: %s\nLoginRequest: %s\nis expired: %v\n%+v\n",
471471
notAfter.String(), consentRequestLifespan.String(), isExpired, r)
472472

473-
f = x.Must(reg.ConsentManager().CreateLoginRequest(ctx, nil, r))
473+
f = x.Must(reg.ConsentManager().CreateLoginRequest(ctx, r))
474474
loginChallenge := x.Must(f.ToLoginChallenge(ctx, reg))
475475

476476
_, err = reg.ConsentManager().GetLoginRequest(ctx, loginChallenge)

‎oauth2/fosite_store_helpers_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ func mockRequestForeignKey(t *testing.T, id string, x oauth2.InternalRegistry) {
126126
}
127127

128128
f, err := x.ConsentManager().CreateLoginRequest(
129-
ctx, nil, &flow.LoginRequest{
129+
ctx, &flow.LoginRequest{
130130
Client: cl,
131131
OpenIDConnectContext: new(flow.OAuth2ConsentRequestOpenIDConnectContext),
132132
ID: id,

‎oauth2/handler.go‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,16 @@ import (
4747
)
4848

4949
const (
50-
DefaultLoginPath = "/oauth2/fallbacks/login"
51-
DefaultConsentPath = "/oauth2/fallbacks/consent"
52-
DefaultPostLogoutPath = "/oauth2/fallbacks/logout/callback"
53-
DefaultPostDevicePath = "/oauth2/fallbacks/device/done"
54-
DefaultLogoutPath = "/oauth2/fallbacks/logout"
55-
DefaultErrorPath = "/oauth2/fallbacks/error"
56-
TokenPath = "/oauth2/token" // #nosec G101
57-
AuthPath = "/oauth2/auth"
58-
LogoutPath = "/oauth2/sessions/logout"
50+
DefaultLoginPath = "/oauth2/fallbacks/login"
51+
DefaultConsentPath = "/oauth2/fallbacks/consent"
52+
DefaultPostLogoutPath = "/oauth2/fallbacks/logout/callback"
53+
DefaultDeviceVerificationPath = "/oauth2/fallbacks/device"
54+
DefaultPostDevicePath = "/oauth2/fallbacks/device/done"
55+
DefaultLogoutPath = "/oauth2/fallbacks/logout"
56+
DefaultErrorPath = "/oauth2/fallbacks/error"
57+
TokenPath = "/oauth2/token" // #nosec G101
58+
AuthPath = "/oauth2/auth"
59+
LogoutPath = "/oauth2/sessions/logout"
5960

6061
VerifiableCredentialsPath = "/credentials"
6162
UserinfoPath = "/userinfo"
@@ -111,6 +112,7 @@ func (h *Handler) SetRoutes(admin *httprouterx.RouterAdmin, public *httprouterx.
111112
http.StatusOK,
112113
config.KeyLogoutRedirectURL,
113114
))
115+
public.GET(DefaultDeviceVerificationPath, h.fallbackHandler("", "", http.StatusOK, config.KeyDeviceVerificationURL))
114116
public.GET(DefaultPostDevicePath, h.fallbackHandler(
115117
"You successfully authenticated on your device!",
116118
"The Default Post Device URL is not set which is why you are seeing this fallback page. Your device login request however succeeded.",

‎oauth2/oauth2_device_code_test.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,6 @@ func TestDeviceCodeWithDefaultStrategy(t *testing.T) {
323323
assert.EqualValues(t, c.LogoURI, pointerx.Deref(rr.Client.LogoUri))
324324
assert.EqualValues(t, subject, pointerx.Deref(rr.Subject))
325325
assert.EqualValues(t, scopes, rr.RequestedScope)
326-
assert.EqualValues(t, r.URL.Query().Get("consent_challenge"), rr.Challenge)
327326
assert.Contains(t, *rr.RequestUrl, hydraoauth2.DeviceVerificationPath)
328327
if checkRequestPayload != nil {
329328
checkRequestPayload(rr)

0 commit comments

Comments
 (0)