Skip to content

Commit

Permalink
fix: handled requests respond with 410 Gone and include redirect URL (o…
Browse files Browse the repository at this point in the history 
…ry#2473)

Closes ory#1569

BREAKING CHANGE: This patch makes it so that already handled consent/login/logout requests respond with 410 Gone instead of 409 Conflict. Additionally, a URL is included that the user should be redirected to!

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
  • Loading branch information
@svrakitin @aeneasr @mitar
2 people authored and mitar committed May 13, 2021
1 parent 63a9079 commit eb2c38a
Show file tree
Hide file tree
Showing 16 changed files with 295 additions and 117 deletions.
28 changes: 15 additions & 13 deletions consent/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (h *Handler) DeleteLoginSession(w http.ResponseWriter, r *http.Request, ps
// 200: loginRequest
// 400: genericError
// 404: genericError
// 409: genericError
// 410: requestWasHandledResponse
// 500: genericError
func (h *Handler) GetLoginRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
challenge := stringsx.Coalesce(
Expand All @@ -269,10 +269,10 @@ func (h *Handler) GetLoginRequest(w http.ResponseWriter, r *http.Request, ps htt
h.r.Writer().WriteError(w, r, err)
return
}

if request.WasHandled {
h.r.Writer().WriteError(w, r, x.ErrConflict.WithDebug("Login request has been used already"))
return
h.r.Writer().WriteCode(w, r, http.StatusGone, &RequestWasHandledResponse{
RedirectTo: request.RequestURL,
})
}

request.Client = sanitizeClient(request.Client)
Expand Down Expand Up @@ -476,7 +476,7 @@ func (h *Handler) RejectLoginRequest(w http.ResponseWriter, r *http.Request, ps
// Responses:
// 200: consentRequest
// 404: genericError
// 409: genericError
// 410: requestWasHandledResponse
// 500: genericError
func (h *Handler) GetConsentRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
challenge := stringsx.Coalesce(
Expand All @@ -494,8 +494,9 @@ func (h *Handler) GetConsentRequest(w http.ResponseWriter, r *http.Request, ps h
return
}
if request.WasHandled {
h.r.Writer().WriteError(w, r, x.ErrConflict.WithDebug("Consent request has been used already"))
return
h.r.Writer().WriteCode(w, r, http.StatusGone, &RequestWasHandledResponse{
RedirectTo: request.RequestURL,
})
}

if request.RequestedScope == nil {
Expand Down Expand Up @@ -751,23 +752,24 @@ func (h *Handler) RejectLogoutRequest(w http.ResponseWriter, r *http.Request, ps
// Responses:
// 200: logoutRequest
// 404: genericError
// 410: requestWasHandledResponse
// 500: genericError
func (h *Handler) GetLogoutRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
challenge := stringsx.Coalesce(
r.URL.Query().Get("logout_challenge"),
r.URL.Query().Get("challenge"),
)

c, err := h.r.ConsentManager().GetLogoutRequest(r.Context(), challenge)
request, err := h.r.ConsentManager().GetLogoutRequest(r.Context(), challenge)
if err != nil {
h.r.Writer().WriteError(w, r, err)
return
}

if c.WasUsed {
h.r.Writer().WriteError(w, r, x.ErrConflict.WithDebug("Logout request has been used already"))
return
if request.WasHandled {
h.r.Writer().WriteCode(w, r, http.StatusGone, &RequestWasHandledResponse{
RedirectTo: request.RequestURL,
})
}

h.r.Writer().Write(w, r, c)
h.r.Writer().Write(w, r, request)
}
24 changes: 12 additions & 12 deletions consent/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ import (

func TestGetLogoutRequest(t *testing.T) {
for k, tc := range []struct {
exists bool
used bool
status int
exists bool
handled bool
status int
}{
{false, false, http.StatusNotFound},
{true, false, http.StatusOK},
{true, true, http.StatusConflict},
{true, true, http.StatusGone},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
key := fmt.Sprint(k)
Expand All @@ -58,9 +58,9 @@ func TestGetLogoutRequest(t *testing.T) {
cl := &client.Client{OutfacingID: "client" + key}
require.NoError(t, reg.ClientManager().CreateClient(context.Background(), cl))
require.NoError(t, reg.ConsentManager().CreateLogoutRequest(context.TODO(), &LogoutRequest{
Client: cl,
ID: challenge,
WasUsed: tc.used,
Client: cl,
ID: challenge,
WasHandled: tc.handled,
}))
}

Expand All @@ -86,7 +86,7 @@ func TestGetLoginRequest(t *testing.T) {
}{
{false, false, http.StatusNotFound},
{true, false, http.StatusOK},
{true, true, http.StatusConflict},
{true, true, http.StatusGone},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
key := fmt.Sprint(k)
Expand All @@ -104,7 +104,7 @@ func TestGetLoginRequest(t *testing.T) {
}))

if tc.handled {
_, err := reg.ConsentManager().HandleLoginRequest(context.Background(), challenge, &HandledLoginRequest{ID: challenge, WasUsed: true})
_, err := reg.ConsentManager().HandleLoginRequest(context.Background(), challenge, &HandledLoginRequest{ID: challenge, WasHandled: true})
require.NoError(t, err)
}
}
Expand All @@ -131,7 +131,7 @@ func TestGetConsentRequest(t *testing.T) {
}{
{false, false, http.StatusNotFound},
{true, false, http.StatusOK},
{true, true, http.StatusConflict},
{true, true, http.StatusGone},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
key := fmt.Sprint(k)
Expand All @@ -150,8 +150,8 @@ func TestGetConsentRequest(t *testing.T) {

if tc.handled {
_, err := reg.ConsentManager().HandleConsentRequest(context.Background(), challenge, &HandledConsentRequest{
ID: challenge,
WasUsed: true,
ID: challenge,
WasHandled: true,
})
require.NoError(t, err)
}
Expand Down
4 changes: 2 additions & 2 deletions consent/janitor_consent_test_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func NewHandledLoginRequest(challenge string, hasError bool, requestedAt time.Ti
return &HandledLoginRequest{
ID: challenge,
Error: deniedErr,
WasUsed: true,
WasHandled: true,
RequestedAt: requestedAt,
AuthenticatedAt: authenticatedAt,
}
Expand All @@ -47,6 +47,6 @@ func NewHandledConsentRequest(challenge string, hasError bool, requestedAt time.
Error: deniedErr,
RequestedAt: requestedAt,
AuthenticatedAt: authenticatedAt,
WasUsed: true,
WasHandled: true,
}
}
12 changes: 6 additions & 6 deletions consent/manager_test_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func MockLogoutRequest(key string, withClient bool) (c *LogoutRequest) {
RPInitiated: true,
RequestURL: "http://request-me/",
PostLogoutRedirectURI: "http://redirect-me/",
WasUsed: false,
WasHandled: false,
Accepted: false,
Client: cl,
}
Expand Down Expand Up @@ -164,7 +164,7 @@ func MockAuthRequest(key string, authAt bool) (c *LoginRequest, h *HandledLoginR
Subject: c.Subject,
ACR: "acr",
ForceSubjectIdentifier: "forced-subject",
WasUsed: false,
WasHandled: false,
}

return c, h
Expand Down Expand Up @@ -193,7 +193,7 @@ func SaneMockHandleConsentRequest(t *testing.T, m Manager, c *ConsentRequest, au
GrantedScope: []string{"scopea", "scopeb"},
GrantedAudience: []string{"auda", "audb"},
Error: rde,
WasUsed: false,
WasHandled: false,
HandledAt: sqlxx.NullTime(time.Now().UTC().Add(-time.Minute)),
}

Expand Down Expand Up @@ -798,7 +798,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit

got2, err := m.GetLogoutRequest(context.Background(), "challenge"+tc.key)
require.NoError(t, err)
assert.False(t, got2.WasUsed)
assert.False(t, got2.WasHandled)
assert.False(t, got2.Accepted)
compareLogoutRequest(t, c, got2)

Expand All @@ -811,7 +811,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
got3, err := m.VerifyAndInvalidateLogoutRequest(context.Background(), "verifier"+tc.key)
require.NoError(t, err)
assert.True(t, got3.Accepted)
assert.True(t, got3.WasUsed)
assert.True(t, got3.WasHandled)
compareLogoutRequest(t, c, got3)

_, err = m.VerifyAndInvalidateLogoutRequest(context.Background(), "verifier"+tc.key)
Expand All @@ -820,7 +820,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager x.Fosit
got2, err = m.GetLogoutRequest(context.Background(), "challenge"+tc.key)
require.NoError(t, err)
compareLogoutRequest(t, got3, got2)
assert.True(t, got2.WasUsed)
assert.True(t, got2.WasHandled)
} else {
require.NoError(t, m.RejectLogoutRequest(context.Background(), "challenge"+tc.key))
_, err = m.GetLogoutRequest(context.Background(), "challenge"+tc.key)
Expand Down
52 changes: 46 additions & 6 deletions consent/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,16 @@ func (e *RequestDeniedError) Value() (driver.Value, error) {
return string(value), nil
}

// The response payload sent when there is an attempt to access already handled request.
//
// swagger:model requestWasHandledResponse
type RequestWasHandledResponse struct {
// Original request URL to which you should redirect the user if request was already handled.
//
// required: true
RedirectTo string `json:"redirect_to"`
}

// The request payload used to accept a consent request.
//
// swagger:model acceptConsentRequest
Expand Down Expand Up @@ -178,11 +188,16 @@ type HandledConsentRequest struct {
// HandledAt contains the timestamp the consent request was handled.
HandledAt sqlxx.NullTime `json:"handled_at" db:"handled_at"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_used"`

ConsentRequest *ConsentRequest `json:"-" db:"-"`
Error *RequestDeniedError `json:"-" db:"error"`
RequestedAt time.Time `json:"-" db:"requested_at"`
AuthenticatedAt sqlxx.NullTime `json:"-" db:"authenticated_at"`
WasUsed bool `json:"-" db:"was_used"`

SessionIDToken sqlxx.MapStringInterface `db:"session_id_token" json:"-"`
SessionAccessToken sqlxx.MapStringInterface `db:"session_access_token" json:"-"`
Expand Down Expand Up @@ -250,11 +265,16 @@ type PreviousConsentSession struct {
// HandledAt contains the timestamp the consent request was handled.
HandledAt sqlxx.NullTime `json:"handled_at" db:"handled_at"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_used"`

ConsentRequest *ConsentRequest `json:"consent_request" db:"-"`
Error *RequestDeniedError `json:"-" db:"error"`
RequestedAt time.Time `json:"-" db:"requested_at"`
AuthenticatedAt sqlxx.NullTime `json:"-" db:"authenticated_at"`
WasUsed bool `json:"-" db:"was_used"`

SessionIDToken sqlxx.MapStringInterface `db:"session_id_token" json:"-"`
SessionAccessToken sqlxx.MapStringInterface `db:"session_access_token" json:"-"`
Expand Down Expand Up @@ -309,11 +329,16 @@ type HandledLoginRequest struct {
// data.
Context sqlxx.JSONRawMessage `json:"context" db:"context"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_used"`

LoginRequest *LoginRequest `json:"-" db:"-"`
Error *RequestDeniedError `json:"-" db:"error"`
RequestedAt time.Time `json:"-" db:"requested_at"`
AuthenticatedAt sqlxx.NullTime `json:"-" db:"authenticated_at"`
WasUsed bool `json:"-" db:"was_used"`
}

func (_ HandledLoginRequest) TableName() string {
Expand Down Expand Up @@ -412,9 +437,14 @@ type LogoutRequest struct {
// RPInitiated is set to true if the request was initiated by a Relying Party (RP), also known as an OAuth 2.0 Client.
RPInitiated bool `json:"rp_initiated" db:"rp_initiated"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_used"`

Verifier string `json:"-" db:"verifier"`
PostLogoutRedirectURI string `json:"-" db:"redir_url"`
WasUsed bool `json:"-" db:"was_used"`
Accepted bool `json:"-" db:"accepted"`
Rejected bool `db:"rejected" json:"-"`
ClientID sql.NullString `json:"-" db:"client_id"`
Expand Down Expand Up @@ -510,13 +540,18 @@ type LoginRequest struct {
// channel logout. It's value can generally be used to associate consecutive login requests by a certain user.
SessionID sqlxx.NullString `json:"session_id" db:"login_session_id"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_handled,r"`

ForceSubjectIdentifier string `json:"-" db:"-"` // this is here but has no meaning apart from sql_helper working properly.
Verifier string `json:"-" db:"verifier"`
CSRF string `json:"-" db:"csrf"`

AuthenticatedAt sqlxx.NullTime `json:"-" db:"authenticated_at"`
RequestedAt time.Time `json:"-" db:"requested_at"`
WasHandled bool `json:"-" db:"was_handled,r"`
}

func (_ LoginRequest) TableName() string {
Expand Down Expand Up @@ -596,14 +631,19 @@ type ConsentRequest struct {
// Context contains arbitrary information set by the login endpoint or is empty if not set.
Context sqlxx.JSONRawMessage `json:"context,omitempty" db:"context"`

// If set to true means that the request was already handled. This
// can happen on form double-submit or other errors. If this is set
// we recommend redirecting the user to `request_url` to re-initiate
// the flow.
WasHandled bool `json:"-" db:"was_handled,r"`

// ForceSubjectIdentifier is the value from authentication (if set).
ForceSubjectIdentifier string `json:"-" db:"forced_subject_identifier"`
SubjectIdentifier string `json:"-" db:"-"`
Verifier string `json:"-" db:"verifier"`
CSRF string `json:"-" db:"csrf"`
AuthenticatedAt sqlxx.NullTime `json:"-" db:"authenticated_at"`
RequestedAt time.Time `json:"-" db:"requested_at"`
WasHandled bool `json:"-" db:"was_handled,r"`
}

func (_ ConsentRequest) TableName() string {
Expand Down
28 changes: 14 additions & 14 deletions internal/httpclient/client/admin/get_consent_request_responses.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit eb2c38a

Please sign in to comment.