Skip to content

Commit

Permalink
fix: don't assume the login challenge to be a UUID
Browse files Browse the repository at this point in the history
For compatibility with ory/hydra#3515, which
now encodes the whole flow in the login challenge, we cannot further
assume that the challenge is a UUID.
  • Loading branch information
hperl committed Jun 15, 2023
1 parent 84d14ed commit d716e20
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 18 deletions.
7 changes: 6 additions & 1 deletion hydra/hydra.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@ func GetLoginChallengeID(conf *config.Config, r *http.Request) (sqlxx.NullString
return "", errors.WithStack(herodot.ErrInternalServerError.WithReason("refusing to parse login_challenge query parameter because " + config.ViperKeyOAuth2ProviderURL + " is invalid or unset"))
}

return sqlxx.NullString(r.URL.Query().Get("login_challenge")), nil
loginChallenge := r.URL.Query().Get("login_challenge")
if loginChallenge == "" {
return "", errors.WithStack(herodot.ErrBadRequest.WithReason("the login_challenge parameter is present but empty"))
}

return sqlxx.NullString(loginChallenge), nil
}

func (h *DefaultHydra) getAdminURL(ctx context.Context) (string, error) {
Expand Down
9 changes: 9 additions & 0 deletions hydra/hydra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,15 @@ func TestGetLoginChallengeID(t *testing.T) {
want: "",
assertErr: assert.NoError,
},
{
name: "empty login challenge; hydra is configured",
args: args{
conf: configWithHydra,
r: requestFromChallenge(""),
},
want: "",
assertErr: assert.Error,
},
{
name: "login_challenge is present; Hydra is not configured",
args: args{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"id": "cccccccc-dda4-4700-9e42-35731f2af91e",
"oauth2_login_challenge": "challenge data",
"type": "api",
"expires_at": "2013-10-07T08:23:19Z",
"issued_at": "2013-10-07T08:23:19Z",
"request_url": "http://kratos:4433/self-service/browser/flows/login",
"ui": {
"action": "",
"method": "",
"nodes": null
},
"created_at": "2013-10-07T08:23:19Z",
"updated_at": "2013-10-07T08:23:19Z",
"refresh": false,
"requested_aal": "aal1"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
INSERT INTO selfservice_login_flows (id, nid, request_url, issued_at, expires_at, active_method, csrf_token, created_at,
updated_at, forced, type, ui, internal_context, oauth2_login_challenge_data)
VALUES ('cccccccc-dda4-4700-9e42-35731f2af91e',
'884f556e-eb3a-4b9f-bee3-11345642c6c0',
'http://kratos:4433/self-service/browser/flows/login',
'2013-10-07 08:23:19', '2013-10-07 08:23:19', '',
'fpeVSZ9ZH7YvUkhXsOVEIssxbfauh5lcoQSYxTcN0XkMneg1L42h+HtvisjlNjBF4ElcD2jApCHoJYq2u9sVWg==',
'2013-10-07 08:23:19', '2013-10-07 08:23:19', false, 'api', '{}', '{"foo":"bar"}', 'challenge data');
20 changes: 3 additions & 17 deletions test/e2e/cypress/integration/profiles/oidc-provider/error.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,14 @@ context("OpenID Provider", () => {
cy.useConfigProfile("oidc-provider")
cy.proxy("express")
})
it("should fail with invalid login_challenge", () => {
cy.visit(express.login + "?login_challenge=not-a-uuid", {
it("should fail with empty login_challenge", () => {
cy.visit(express.login + "?login_challenge=", {
failOnStatusCode: false,
}).then((d) => {
cy.get(`[data-testid="ui/error/message"]`).then((c) => {
cy.wrap(c[0].textContent).should(
"contain",
"the login_challenge parameter is present but invalid or zero UUID",
)
})
})
})

it("should fail with zero login_challenge", () => {
cy.visit(
express.login + "?login_challenge=00000000-0000-0000-0000-000000000000",
{ failOnStatusCode: false },
).then((d) => {
cy.get(`[data-testid="ui/error/message"]`).then((c) => {
cy.wrap(c[0].textContent).should(
"contain",
"the login_challenge parameter is present but invalid or zero UUID",
"the login_challenge parameter is present but empty",
)
})
})
Expand Down

0 comments on commit d716e20

Please sign in to comment.