Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Respond with Redirect URL for already handled requests #2473

Merged
merged 6 commits into from
Apr 23, 2021
Merged

fix: Respond with Redirect URL for already handled requests #2473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a31e64e
fix: Allow access to already handled requests instead of raising conf…
svrakitin Apr 16, 2021
5baa201
Update comment
svrakitin Apr 22, 2021
0b771ad
Add redirect_to to request was handled response
svrakitin Apr 22, 2021
afa8088
Merge branch 'master' into allow-get-handled-requests
aeneasr Apr 23, 2021
6a33a7a
Use 410 Gone instead of 409 Conflict
svrakitin Apr 23, 2021
c06c70c
Merge branch 'master' into allow-get-handled-requests
aeneasr Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions consent/handler.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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

Loading