consent: Add error response if consent or login challenge is expired (#…

…1098)

Closes #1056 

Signed-off-by: Kostya Lepa <const.lepa@gmail.com>

consent/handler.go

@@ -31,6 +31,7 @@ import (
 	"github.com/ory/fosite"
 	"github.com/ory/go-convenience/urlx"
 	"github.com/ory/herodot"
+	"github.com/ory/hydra/pkg"
 	"github.com/ory/x/pagination"
 	"github.com/pkg/errors"
 )
@@ -250,13 +251,18 @@ func (h *Handler) DeleteLoginSession(w http.ResponseWriter, r *http.Request, ps
 //     Responses:
 //       200: loginRequest
 //       401: genericError
+//       409: genericError
 //       500: genericError
 func (h *Handler) GetLoginRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
 	request, err := h.M.GetAuthenticationRequest(r.Context(), ps.ByName("challenge"))
 	if err != nil {
 		h.H.WriteError(w, r, err)
 		return
 	}
+	if request.WasHandled {
+		h.H.WriteError(w, r, pkg.ErrConflict.WithDebug("Login request has been handled already"))
+		return
+	}
 
 	request.Client = sanitizeClient(request.Client)
 
@@ -430,13 +436,18 @@ func (h *Handler) RejectLoginRequest(w http.ResponseWriter, r *http.Request, ps
 //     Responses:
 //       200: consentRequest
 //       401: genericError
+//       409: genericError
 //       500: genericError
 func (h *Handler) GetConsentRequest(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
 	request, err := h.M.GetConsentRequest(r.Context(), ps.ByName("challenge"))
 	if err != nil {
 		h.H.WriteError(w, r, err)
 		return
 	}
+	if request.WasHandled {
+		h.H.WriteError(w, r, pkg.ErrConflict.WithDebug("Consent request has been handled already"))
+		return
+	}
 
 	request.Client = sanitizeClient(request.Client)
 

consent/handler_test.go

@@ -21,6 +21,7 @@
 package consent
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/cookiejar"
 	"net/http/httptest"
@@ -33,6 +34,7 @@ import (
 	"github.com/gorilla/sessions"
 	"github.com/julienschmidt/httprouter"
 	"github.com/ory/herodot"
+	"github.com/ory/hydra/client"
 	"github.com/pborman/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -91,3 +93,83 @@ func TestLogout(t *testing.T) {
 	assert.Len(t, cj.Cookies(u), 0)
 	assert.EqualValues(t, ts.URL+"/logout", resp.Request.URL.String())
 }
+
+func TestGetLoginRequest(t *testing.T) {
+	for k, tc := range []struct {
+		exists  bool
+		handled bool
+		status  int
+	}{
+		{false, false, http.StatusNotFound},
+		{true, false, http.StatusOK},
+		{true, true, http.StatusConflict},
+	} {
+		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
+			key := fmt.Sprint(k)
+			challenge := "challenge" + key
+			m := NewMemoryManager(nil)
+			if tc.exists {
+				require.NoError(t, m.CreateAuthenticationRequest(context.TODO(), &AuthenticationRequest{
+					Client:     &client.Client{ClientID: "client" + key},
+					Challenge:  challenge,
+					WasHandled: tc.handled,
+				}))
+			}
+			r := httprouter.New()
+			h := NewHandler(
+				herodot.NewJSONWriter(nil),
+				m,
+				nil,
+				"https://www.ory.sh",
+			)
+			h.SetRoutes(r, r)
+			ts := httptest.NewServer(r)
+			defer ts.Close()
+
+			c := &http.Client{}
+			resp, err := c.Get(ts.URL + LoginPath + "/" + challenge)
+			require.NoError(t, err)
+			require.EqualValues(t, tc.status, resp.StatusCode)
+		})
+	}
+}
+
+func TestGetConsentRequest(t *testing.T) {
+	for k, tc := range []struct {
+		exists  bool
+		handled bool
+		status  int
+	}{
+		{false, false, http.StatusNotFound},
+		{true, false, http.StatusOK},
+		{true, true, http.StatusConflict},
+	} {
+		t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
+			key := fmt.Sprint(k)
+			challenge := "challenge" + key
+			m := NewMemoryManager(nil)
+			if tc.exists {
+				require.NoError(t, m.CreateConsentRequest(context.TODO(), &ConsentRequest{
+					Client:     &client.Client{ClientID: "client" + key},
+					Challenge:  challenge,
+					WasHandled: tc.handled,
+				}))
+			}
+			r := httprouter.New()
+			h := NewHandler(
+				herodot.NewJSONWriter(nil),
+				m,
+				nil,
+				"https://www.ory.sh",
+			)
+			h.SetRoutes(r, r)
+			ts := httptest.NewServer(r)
+			defer ts.Close()
+
+			c := &http.Client{}
+			resp, err := c.Get(ts.URL + ConsentPath + "/" + challenge)
+			require.NoError(t, err)
+			require.EqualValues(t, tc.status, resp.StatusCode)
+		})
+	}
+}

consent/manager_memory.go

@@ -150,11 +150,19 @@ func (m *MemoryManager) CreateConsentRequest(ctx context.Context, c *ConsentRequ
 func (m *MemoryManager) GetConsentRequest(ctx context.Context, challenge string) (*ConsentRequest, error) {
 	m.m["consentRequests"].RLock()
 	defer m.m["consentRequests"].RUnlock()
-	if c, ok := m.consentRequests[challenge]; ok {
-		c.Client.ClientID = c.Client.GetID()
-		return &c, nil
+
+	c, ok := m.consentRequests[challenge]
+	if !ok {
+		return nil, errors.WithStack(pkg.ErrNotFound)
 	}
-	return nil, errors.WithStack(pkg.ErrNotFound)
+
+	for _, h := range m.handledConsentRequests {
+		if h.Challenge == c.Challenge {
+			c.WasHandled = h.WasUsed
+		}
+	}
+	c.Client.ClientID = c.Client.GetID()
+	return &c, nil
 }
 
 func (m *MemoryManager) HandleConsentRequest(ctx context.Context, challenge string, r *HandledConsentRequest) (*ConsentRequest, error) {
@@ -294,11 +302,19 @@ func (m *MemoryManager) CreateAuthenticationRequest(ctx context.Context, a *Auth
 func (m *MemoryManager) GetAuthenticationRequest(ctx context.Context, challenge string) (*AuthenticationRequest, error) {
 	m.m["authRequests"].RLock()
 	defer m.m["authRequests"].RUnlock()
-	if c, ok := m.authRequests[challenge]; ok {
-		c.Client.ClientID = c.Client.GetID()
-		return &c, nil
+
+	c, ok := m.authRequests[challenge]
+	if !ok {
+		return nil, errors.WithStack(pkg.ErrNotFound)
 	}
-	return nil, errors.WithStack(pkg.ErrNotFound)
+
+	for _, h := range m.handledAuthRequests {
+		if h.Challenge == c.Challenge {
+			c.WasHandled = h.WasUsed
+		}
+	}
+	c.Client.ClientID = c.Client.GetID()
+	return &c, nil
 }
 
 func (m *MemoryManager) HandleAuthenticationRequest(ctx context.Context, challenge string, r *HandledAuthenticationRequest) (*AuthenticationRequest, error) {

consent/manager_sql.go

@@ -219,8 +219,9 @@ func (m *SQLManager) CreateConsentRequest(ctx context.Context, c *ConsentRequest
 
 func (m *SQLManager) GetConsentRequest(ctx context.Context, challenge string) (*ConsentRequest, error) {
 	var d sqlConsentRequest
-
-	if err := m.db.GetContext(ctx, &d, m.db.Rebind("SELECT * FROM hydra_oauth2_consent_request WHERE challenge=?"), challenge); err != nil {
+	err := m.db.GetContext(ctx, &d, m.db.Rebind("SELECT r.*, COALESCE(hr.was_used, false) as was_handled FROM hydra_oauth2_consent_request r "+
+		"LEFT JOIN hydra_oauth2_consent_request_handled hr ON r.challenge = hr.challenge WHERE r.challenge=?"), challenge)
+	if err != nil {
 		if err == sql.ErrNoRows {
 			return nil, errors.WithStack(pkg.ErrNotFound)
 		}
@@ -253,9 +254,10 @@ func (m *SQLManager) CreateAuthenticationRequest(ctx context.Context, c *Authent
 }
 
 func (m *SQLManager) GetAuthenticationRequest(ctx context.Context, challenge string) (*AuthenticationRequest, error) {
-	var d sqlConsentRequest
-
-	if err := m.db.GetContext(ctx, &d, m.db.Rebind("SELECT * FROM hydra_oauth2_authentication_request WHERE challenge=?"), challenge); err != nil {
+	var d sqlAuthenticationRequest
+	err := m.db.GetContext(ctx, &d, m.db.Rebind("SELECT r.*, COALESCE(hr.was_used, false) as was_handled FROM hydra_oauth2_authentication_request r "+
+		"LEFT JOIN hydra_oauth2_authentication_request_handled hr ON r.challenge = hr.challenge WHERE r.challenge=?"), challenge)
+	if err != nil {
 		if err == sql.ErrNoRows {
 			return nil, errors.WithStack(pkg.ErrNotFound)
 		}
@@ -349,7 +351,7 @@ func (m *SQLManager) VerifyAndInvalidateAuthenticationRequest(ctx context.Contex
 	}
 
 	if d.WasUsed {
-		return nil, errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Consent verifier has been used already"))
+		return nil, errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Authentication verifier has been used already"))
 	}
 
 	if _, err := m.db.ExecContext(ctx, m.db.Rebind("UPDATE hydra_oauth2_authentication_request_handled SET was_used=true WHERE challenge=?"), challenge); err != nil {

consent/manager_test_helpers.go

@@ -217,6 +217,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager pkg.Fos
 					got1, err := m.GetConsentRequest(context.TODO(), "challenge"+tc.key)
 					require.NoError(t, err)
 					compareConsentRequest(t, c, got1)
+					assert.False(t, got1.WasHandled)
 
 					got1, err = m.HandleConsentRequest(context.TODO(), "challenge"+tc.key, h)
 					require.NoError(t, err)
@@ -229,6 +230,10 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager pkg.Fos
 
 					_, err = m.VerifyAndInvalidateConsentRequest(context.TODO(), "verifier"+tc.key)
 					require.Error(t, err)
+
+					got1, err = m.GetConsentRequest(context.TODO(), "challenge"+tc.key)
+					require.NoError(t, err)
+					assert.True(t, got1.WasHandled)
 				})
 			}
 
@@ -281,6 +286,7 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager pkg.Fos
 
 					got1, err := m.GetAuthenticationRequest(context.TODO(), "challenge"+tc.key)
 					require.NoError(t, err)
+					assert.False(t, got1.WasHandled)
 					compareAuthenticationRequest(t, c, got1)
 
 					got1, err = m.HandleAuthenticationRequest(context.TODO(), "challenge"+tc.key, h)
@@ -294,6 +300,10 @@ func ManagerTests(m Manager, clientManager client.Manager, fositeManager pkg.Fos
 
 					_, err = m.VerifyAndInvalidateAuthenticationRequest(context.TODO(), "verifier"+tc.key)
 					require.Error(t, err)
+
+					got1, err = m.GetAuthenticationRequest(context.TODO(), "challenge"+tc.key)
+					require.NoError(t, err)
+					assert.True(t, got1.WasHandled)
 				})
 			}
 		})

consent/sql_helper.go

@@ -201,6 +201,7 @@ type sqlAuthenticationRequest struct {
 	AuthenticatedAt      *time.Time `db:"authenticated_at"`
 	RequestedAt          time.Time  `db:"requested_at"`
 	SessionID            string     `db:"login_session_id"`
+	WasHandled           bool       `db:"was_handled"`
 }
 
 type sqlConsentRequest struct {
@@ -291,6 +292,7 @@ func (s *sqlAuthenticationRequest) toAuthenticationRequest(client *client.Client
 		CSRF:                 s.CSRF,
 		AuthenticatedAt:      fromMySQLDateHack(s.AuthenticatedAt),
 		RequestedAt:          s.RequestedAt,
+		WasHandled:           s.WasHandled,
 	}, nil
 }
 
@@ -313,6 +315,7 @@ func (s *sqlConsentRequest) toConsentRequest(client *client.Client) (*ConsentReq
 		AuthenticatedAt:        fromMySQLDateHack(s.AuthenticatedAt),
 		ForceSubjectIdentifier: s.ForcedSubjectIdentifier,
 		RequestedAt:            s.RequestedAt,
+		WasHandled:             s.WasHandled,
 	}, nil
 }
 

consent/types.go

@@ -262,6 +262,7 @@ type AuthenticationRequest struct {
 	CSRF                   string    `json:"-"`
 	AuthenticatedAt        time.Time `json:"-"`
 	RequestedAt            time.Time `json:"-"`
+	WasHandled             bool      `json:"-"`
 }
 
 // Contains information on an ongoing consent request.
@@ -311,6 +312,7 @@ type ConsentRequest struct {
 	CSRF                   string    `json:"-"`
 	AuthenticatedAt        time.Time `json:"-"`
 	RequestedAt            time.Time `json:"-"`
+	WasHandled             bool      `json:"-"`
 }
 
 // Used to pass session data to a consent request.

pkg/errors.go

@@ -35,6 +35,11 @@ var (
 		Name:        http.StatusText(http.StatusNotFound),
 		Description: "Unable to located the requested resource",
 	}
+	ErrConflict = &fosite.RFC6749Error{
+		Code:        http.StatusConflict,
+		Name:        http.StatusText(http.StatusConflict),
+		Description: "Unable to process the requested resource because of conflict in the current state",
+	}
 )
 
 type stackTracer interface {