Skip to content

Commit 5d8635c

Browse files
alnralnr
authored
fix: revoke by consent request ID (#3947)
This is a follow-up to #3932. This makes several improvements, chiefly restoring backwards compatibility to Hydra before #3932. We now return both the `challenge` as well as the `consent_request_id` from our APIs and distinguish between them clearly. Closes #3941
1 parent 5d2ca41 commit 5d8635c

33 files changed

+515
-357
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ quicktest-hsm:
9393

9494
.PHONY: test-refresh
9595
test-refresh:
96-
UPDATE_SNAPSHOTS=true go test -failfast -short -tags sqlite,sqlite_omit_load_extension ./...
96+
UPDATE_SNAPSHOTS=true go test -short -tags sqlite,sqlite_omit_load_extension ./...
9797
DOCKER_CONTENT_TRUST=1 docker build --progress=plain -f .docker/Dockerfile-test-hsm --target test-refresh-hsm -t oryd/hydra:${IMAGE_TAG} --target test-refresh-hsm .
9898

9999
authors: # updates the AUTHORS file

‎consent/handler.go‎

Lines changed: 26 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,12 @@ type revokeOAuth2ConsentSessions struct {
8888
// in: query
8989
Client string `json:"client"`
9090

91-
// Consent Challenge ID
91+
// Consent Request ID
9292
//
9393
// If set, revoke all token chains derived from this particular consent request ID.
9494
//
9595
// in: query
96-
ConsentChallengeID string `json:"consent_challenge_id"`
96+
ConsentRequestID string `json:"consent_request_id"`
9797

9898
// Revoke All Consent Sessions
9999
//
@@ -122,44 +122,37 @@ type revokeOAuth2ConsentSessions struct {
122122
// 204: emptyResponse
123123
// default: errorOAuth2
124124
func (h *Handler) revokeOAuth2ConsentSessions(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
125-
subject := r.URL.Query().Get("subject")
126-
client := r.URL.Query().Get("client")
127-
consentChallengeID := r.URL.Query().Get("consent_challenge_id")
128-
allClients := r.URL.Query().Get("all") == "true"
129-
if subject == "" && consentChallengeID == "" {
130-
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' or 'consent_challenge_id' are required.`)))
131-
return
132-
}
133-
if consentChallengeID != "" && subject != "" {
134-
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' and 'consent_challenge_id' cannot be set at the same time.`)))
135-
return
136-
}
137-
if consentChallengeID != "" && client != "" {
138-
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'client' and 'consent_challenge_id' cannot be set at the same time.`)))
139-
return
140-
}
125+
var (
126+
subject = r.URL.Query().Get("subject")
127+
client = r.URL.Query().Get("client")
128+
consentRequestID = r.URL.Query().Get("consent_request_id")
129+
allClients = r.URL.Query().Get("all") == "true"
130+
)
141131

142132
switch {
143-
case client != "":
133+
case consentRequestID != "" && subject == "" && client == "":
134+
if err := h.r.ConsentManager().RevokeConsentSessionByID(r.Context(), consentRequestID); err != nil && !errors.Is(err, x.ErrNotFound) {
135+
h.r.Writer().WriteError(w, r, err)
136+
return
137+
}
138+
events.Trace(r.Context(), events.ConsentRevoked, events.WithConsentRequestID(consentRequestID))
139+
140+
case consentRequestID == "" && subject != "" && client != "" && !allClients:
144141
if err := h.r.ConsentManager().RevokeSubjectClientConsentSession(r.Context(), subject, client); err != nil && !errors.Is(err, x.ErrNotFound) {
145142
h.r.Writer().WriteError(w, r, err)
146143
return
147144
}
148145
events.Trace(r.Context(), events.ConsentRevoked, events.WithSubject(subject), events.WithClientID(client))
149-
case allClients:
146+
147+
case consentRequestID == "" && subject != "" && client == "" && allClients:
150148
if err := h.r.ConsentManager().RevokeSubjectConsentSession(r.Context(), subject); err != nil && !errors.Is(err, x.ErrNotFound) {
151149
h.r.Writer().WriteError(w, r, err)
152150
return
153151
}
154152
events.Trace(r.Context(), events.ConsentRevoked, events.WithSubject(subject))
155-
case consentChallengeID != "":
156-
if err := h.r.ConsentManager().RevokeConsentSessionByID(r.Context(), consentChallengeID); err != nil && !errors.Is(err, x.ErrNotFound) {
157-
h.r.Writer().WriteError(w, r, err)
158-
return
159-
}
160-
return
153+
161154
default:
162-
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter both 'client' and 'all' is not defined but one of them should have been.`)))
155+
h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Invalid combination of query parameters.")))
163156
return
164157
}
165158

@@ -782,7 +775,7 @@ func (h *Handler) acceptOAuth2ConsentRequest(w http.ResponseWriter, r *http.Requ
782775
return
783776
}
784777

785-
p.ID = challenge
778+
p.ConsentRequestID = cr.ConsentRequestID
786779
p.RequestedAt = cr.RequestedAt
787780
p.HandledAt = sqlxx.NullTime(time.Now().UTC())
788781

@@ -899,12 +892,13 @@ func (h *Handler) rejectOAuth2ConsentRequest(w http.ResponseWriter, r *http.Requ
899892
h.r.Writer().WriteError(w, r, err)
900893
return
901894
}
895+
cr := f.GetConsentRequest(challenge)
902896

903897
request, err := h.r.ConsentManager().HandleConsentRequest(ctx, f, &flow.AcceptOAuth2ConsentRequest{
904-
Error: &p,
905-
ID: challenge,
906-
RequestedAt: hr.RequestedAt,
907-
HandledAt: sqlxx.NullTime(time.Now().UTC()),
898+
Error: &p,
899+
ConsentRequestID: cr.ConsentRequestID,
900+
RequestedAt: hr.RequestedAt,
901+
HandledAt: sqlxx.NullTime(time.Now().UTC()),
908902
})
909903
if err != nil {
910904
h.r.Writer().WriteError(w, r, errorsx.WithStack(err))

‎consent/handler_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -187,18 +187,18 @@ func TestGetConsentRequest(t *testing.T) {
187187
challenge, err = f.ToConsentChallenge(ctx, reg)
188188
require.NoError(t, err)
189189
require.NoError(t, reg.ConsentManager().CreateConsentRequest(ctx, f, &flow.OAuth2ConsentRequest{
190-
Client: cl,
191-
ID: challenge,
192-
Verifier: challenge,
193-
CSRF: challenge,
194-
LoginChallenge: sqlxx.NullString(lr.ID),
190+
Client: cl,
191+
ConsentRequestID: challenge,
192+
Verifier: challenge,
193+
CSRF: challenge,
194+
LoginChallenge: sqlxx.NullString(lr.ID),
195195
}))
196196

197197
if tc.handled {
198198
_, err := reg.ConsentManager().HandleConsentRequest(ctx, f, &flow.AcceptOAuth2ConsentRequest{
199-
ID: challenge,
200-
WasHandled: true,
201-
HandledAt: sqlxx.NullTime(time.Now()),
199+
ConsentRequestID: challenge,
200+
WasHandled: true,
201+
HandledAt: sqlxx.NullTime(time.Now()),
202202
})
203203
require.NoError(t, err)
204204
challenge, err = f.ToConsentChallenge(ctx, reg)

‎consent/helper_test.go‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -39,52 +39,52 @@ func TestSanitizeClient(t *testing.T) {
3939

4040
func TestMatchScopes(t *testing.T) {
4141
for k, tc := range []struct {
42-
granted []flow.AcceptOAuth2ConsentRequest
43-
requested []string
44-
expectChallenge string
42+
granted []flow.AcceptOAuth2ConsentRequest
43+
requested []string
44+
expectedID string
4545
}{
4646
{
47-
granted: []flow.AcceptOAuth2ConsentRequest{{ID: "1", GrantedScope: []string{"foo", "bar"}}},
48-
requested: []string{"foo", "bar"},
49-
expectChallenge: "1",
47+
granted: []flow.AcceptOAuth2ConsentRequest{{ConsentRequestID: "1", GrantedScope: []string{"foo", "bar"}}},
48+
requested: []string{"foo", "bar"},
49+
expectedID: "1",
5050
},
5151
{
52-
granted: []flow.AcceptOAuth2ConsentRequest{{ID: "1", GrantedScope: []string{"foo", "bar"}}},
53-
requested: []string{"foo", "bar", "baz"},
54-
expectChallenge: "",
52+
granted: []flow.AcceptOAuth2ConsentRequest{{ConsentRequestID: "1", GrantedScope: []string{"foo", "bar"}}},
53+
requested: []string{"foo", "bar", "baz"},
54+
expectedID: "",
5555
},
5656
{
5757
granted: []flow.AcceptOAuth2ConsentRequest{
58-
{ID: "1", GrantedScope: []string{"foo", "bar"}},
59-
{ID: "2", GrantedScope: []string{"foo", "bar"}},
58+
{ConsentRequestID: "1", GrantedScope: []string{"foo", "bar"}},
59+
{ConsentRequestID: "2", GrantedScope: []string{"foo", "bar"}},
6060
},
61-
requested: []string{"foo", "bar"},
62-
expectChallenge: "1",
61+
requested: []string{"foo", "bar"},
62+
expectedID: "1",
6363
},
6464
{
6565
granted: []flow.AcceptOAuth2ConsentRequest{
66-
{ID: "1", GrantedScope: []string{"foo", "bar"}},
67-
{ID: "2", GrantedScope: []string{"foo", "bar", "baz"}},
66+
{ConsentRequestID: "1", GrantedScope: []string{"foo", "bar"}},
67+
{ConsentRequestID: "2", GrantedScope: []string{"foo", "bar", "baz"}},
6868
},
69-
requested: []string{"foo", "bar", "baz"},
70-
expectChallenge: "2",
69+
requested: []string{"foo", "bar", "baz"},
70+
expectedID: "2",
7171
},
7272
{
7373
granted: []flow.AcceptOAuth2ConsentRequest{
74-
{ID: "1", GrantedScope: []string{"foo", "bar"}},
75-
{ID: "2", GrantedScope: []string{"foo", "bar", "baz"}},
74+
{ConsentRequestID: "1", GrantedScope: []string{"foo", "bar"}},
75+
{ConsentRequestID: "2", GrantedScope: []string{"foo", "bar", "baz"}},
7676
},
77-
requested: []string{"zab"},
78-
expectChallenge: "",
77+
requested: []string{"zab"},
78+
expectedID: "",
7979
},
8080
} {
8181
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
8282
got := matchScopes(fosite.ExactScopeStrategy, tc.granted, tc.requested)
83-
if tc.expectChallenge == "" {
83+
if tc.expectedID == "" {
8484
assert.Nil(t, got)
8585
return
8686
}
87-
assert.Equal(t, tc.expectChallenge, got.ID)
87+
assert.Equal(t, tc.expectedID, got.ConsentRequestID)
8888
})
8989
}
9090
}

‎consent/janitor_consent_test_helper.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func NewHandledLoginRequest(challenge string, hasError bool, requestedAt time.Ti
3232
}
3333
}
3434

35-
func NewHandledConsentRequest(challenge string, hasError bool, requestedAt time.Time, authenticatedAt sqlxx.NullTime) *flow.AcceptOAuth2ConsentRequest {
35+
func NewHandledConsentRequest(consentRequestID string, hasError bool, requestedAt time.Time, authenticatedAt sqlxx.NullTime) *flow.AcceptOAuth2ConsentRequest {
3636
var deniedErr *flow.RequestDeniedError
3737
if hasError {
3838
deniedErr = &flow.RequestDeniedError{
@@ -46,11 +46,11 @@ func NewHandledConsentRequest(challenge string, hasError bool, requestedAt time.
4646
}
4747

4848
return &flow.AcceptOAuth2ConsentRequest{
49-
ID: challenge,
50-
HandledAt: sqlxx.NullTime(time.Now().Round(time.Second)),
51-
Error: deniedErr,
52-
RequestedAt: requestedAt,
53-
AuthenticatedAt: authenticatedAt,
54-
WasHandled: true,
49+
ConsentRequestID: consentRequestID,
50+
HandledAt: sqlxx.NullTime(time.Now().Round(time.Second)),
51+
Error: deniedErr,
52+
RequestedAt: requestedAt,
53+
AuthenticatedAt: authenticatedAt,
54+
WasHandled: true,
5555
}
5656
}

‎consent/manager.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ type (
3030
HandleConsentRequest(ctx context.Context, f *flow.Flow, r *flow.AcceptOAuth2ConsentRequest) (*flow.OAuth2ConsentRequest, error)
3131
RevokeSubjectConsentSession(ctx context.Context, user string) error
3232
RevokeSubjectClientConsentSession(ctx context.Context, user, client string) error
33-
RevokeConsentSessionByID(ctx context.Context, consentChallengeID string) error
33+
RevokeConsentSessionByID(ctx context.Context, consentRequestID string) error
3434

3535
VerifyAndInvalidateConsentRequest(ctx context.Context, verifier string) (*flow.AcceptOAuth2ConsentRequest, error)
3636
FindGrantedAndRememberedConsentRequests(ctx context.Context, client, user string) ([]flow.AcceptOAuth2ConsentRequest, error)

‎consent/strategy_default.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -585,13 +585,13 @@ func (s *DefaultStrategy) forwardConsentRequest(
585585

586586
// Set up csrf/challenge/verifier values
587587
verifier := strings.Replace(uuid.New(), "-", "", -1)
588-
consentChallengeID := strings.Replace(uuid.New(), "-", "", -1)
588+
consentRequestID := strings.Replace(uuid.New(), "-", "", -1)
589589
csrf := strings.Replace(uuid.New(), "-", "", -1)
590590

591591
cl := sanitizeClientFromRequest(ar)
592592

593593
consentRequest := &flow.OAuth2ConsentRequest{
594-
ID: consentChallengeID,
594+
ConsentRequestID: consentRequestID,
595595
ACR: as.ACR,
596596
AMR: as.AMR,
597597
Verifier: verifier,

‎consent/strategy_default_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func checkAndAcceptConsentHandler(t *testing.T, apiClient *hydra.APIClient, cb f
4747
payload := cb(t, res, err)
4848

4949
v, _, err := apiClient.OAuth2API.AcceptOAuth2ConsentRequest(context.Background()).
50-
ConsentChallenge(r.URL.Query().Get("consent_challenge")).
50+
ConsentChallenge(res.Challenge).
5151
AcceptOAuth2ConsentRequest(payload).
5252
Execute()
5353
require.NoError(t, err)

‎consent/strategy_oauth_test.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,11 +207,12 @@ func TestStrategyLoginConsentNext(t *testing.T) {
207207

208208
testhelpers.NewLoginConsentUI(t, reg.Config(),
209209
func(w http.ResponseWriter, r *http.Request) {
210+
loginChallenge = r.URL.Query().Get("login_challenge")
210211
res, _, err := adminClient.OAuth2API.GetOAuth2LoginRequest(ctx).
211-
LoginChallenge(r.URL.Query().Get("login_challenge")).
212+
LoginChallenge(loginChallenge).
212213
Execute()
213214
require.NoError(t, err)
214-
loginChallenge = res.Challenge
215+
require.Equal(t, loginChallenge, res.Challenge)
215216

216217
v, _, err := adminClient.OAuth2API.AcceptOAuth2LoginRequest(ctx).
217218
LoginChallenge(loginChallenge).
@@ -223,10 +224,11 @@ func TestStrategyLoginConsentNext(t *testing.T) {
223224
},
224225
func(w http.ResponseWriter, r *http.Request) {
225226
consentChallenge = r.URL.Query().Get("consent_challenge")
226-
_, _, err := adminClient.OAuth2API.GetOAuth2ConsentRequest(ctx).
227+
res, _, err := adminClient.OAuth2API.GetOAuth2ConsentRequest(ctx).
227228
ConsentChallenge(consentChallenge).
228229
Execute()
229230
require.NoError(t, err)
231+
require.Equal(t, consentChallenge, res.Challenge)
230232

231233
v, _, err := adminClient.OAuth2API.AcceptOAuth2ConsentRequest(ctx).
232234
ConsentChallenge(consentChallenge).

0 commit comments

Comments
 (0)