Skip to content

Commit

Permalink
consent: Handles auth time across login & consent flow
Browse files Browse the repository at this point in the history 
This patch improves the handling of auth_time and thus resolves issues with prompt & max_age handling within fosite.
  • Loading branch information
arekkas committed May 17, 2018
1 parent ac1f0ce commit 2ec3f61
Show file tree
Hide file tree
Showing 8 changed files with 281 additions and 118 deletions.
14 changes: 11 additions & 3 deletions consent/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ import (
)

type Handler struct {
H herodot.Writer
M Manager
H herodot.Writer
M Manager
RequestMaxAge time.Duration
}

func NewHandler(
Expand Down Expand Up @@ -134,7 +135,8 @@ func (h *Handler) AcceptLoginRequest(w http.ResponseWriter, r *http.Request, ps
p.Challenge = ps.ByName("challenge")
p.RequestedAt = time.Now().UTC()

if ar, err := h.M.GetAuthenticationRequest(ps.ByName("challenge")); err != nil {
ar, err := h.M.GetAuthenticationRequest(ps.ByName("challenge"))
if err != nil {
h.H.WriteError(w, r, err)
return
} else if ar.Subject != "" && p.Subject != ar.Subject {
Expand All @@ -145,6 +147,12 @@ func (h *Handler) AcceptLoginRequest(w http.ResponseWriter, r *http.Request, ps
return
}

if !ar.Skip {
p.AuthenticatedAt = time.Now().UTC()
} else {
p.AuthenticatedAt = ar.AuthenticatedAt
}

request, err := h.M.HandleAuthenticationRequest(ps.ByName("challenge"), &p)
if err != nil {
h.H.WriteError(w, r, errors.WithStack(err))
Expand Down
14 changes: 8 additions & 6 deletions consent/manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,13 @@ func mockConsentRequest(key string, remember bool, rememberFor int, hasError boo
}

h = &HandledConsentRequest{
ConsentRequest: c,
RememberFor: rememberFor,
Remember: remember,
Challenge: "challenge" + key,
RequestedAt: time.Now().UTC().Add(-time.Minute),
Error: err,
ConsentRequest: c,
RememberFor: rememberFor,
Remember: remember,
Challenge: "challenge" + key,
RequestedAt: time.Now().UTC().Add(-time.Minute),
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
Error: err,
}

return c, h
Expand Down Expand Up @@ -102,6 +103,7 @@ func mockAuthRequest(key string) (c *AuthenticationRequest, h *HandledAuthentica
Remember: true,
Challenge: "challenge" + key,
RequestedAt: time.Now().UTC().Add(-time.Minute),
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
Error: err,
Subject: c.Subject,
ACR: "acr",
Expand Down
138 changes: 85 additions & 53 deletions consent/sql_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,29 +37,31 @@ var migrations = &migrate.MemoryMigrationSource{
Id: "1",
Up: []string{
`CREATE TABLE hydra_oauth2_consent_request (
challenge varchar(40) NOT NULL PRIMARY KEY,
verifier varchar(40) NOT NULL,
client_id varchar(255) NOT NULL,
subject varchar(255) NOT NULL,
request_url text NOT NULL,
skip bool NOT NULL,
requested_scope text NOT NULL,
csrf varchar(40) NOT NULL,
oidc_context text NOT NULL
challenge varchar(40) NOT NULL PRIMARY KEY,
verifier varchar(40) NOT NULL,
client_id varchar(255) NOT NULL,
subject varchar(255) NOT NULL,
request_url text NOT NULL,
skip bool NOT NULL,
requested_scope text NOT NULL,
csrf varchar(40) NOT NULL,
authenticated_at timestamp NOT NULL DEFAULT now(),
oidc_context text NOT NULL
)`,
// It would probably make sense here to have a FK relation to clients, but it increases testing complexity and might also
// purge important audit data when a client is deleted. Also, stale data does not have a negative impact here
// FOREIGN KEY (client_id) REFERENCES hydra_client (id) ON DELETE CASCADE
`CREATE TABLE hydra_oauth2_authentication_request (
challenge varchar(40) NOT NULL PRIMARY KEY,
requested_scope text NOT NULL,
verifier varchar(40) NOT NULL,
csrf varchar(40) NOT NULL,
subject varchar(255) NOT NULL,
request_url text NOT NULL,
skip bool NOT NULL,
client_id varchar(255) NOT NULL,
oidc_context text NOT NULL
challenge varchar(40) NOT NULL PRIMARY KEY,
requested_scope text NOT NULL,
verifier varchar(40) NOT NULL,
csrf varchar(40) NOT NULL,
subject varchar(255) NOT NULL,
request_url text NOT NULL,
skip bool NOT NULL,
client_id varchar(255) NOT NULL,
authenticated_at timestamp NOT NULL DEFAULT now(),
oidc_context text NOT NULL
)`,
// It would probably make sense here to have a FK relation to clients, but it increases testing complexity and might also
// purge important audit data when a client is deleted. Also, stale data does not have a negative impact here
Expand All @@ -78,17 +80,19 @@ var migrations = &migrate.MemoryMigrationSource{
requested_at timestamp NOT NULL DEFAULT now(),
session_access_token text NOT NULL,
session_id_token text NOT NULL,
authenticated_at timestamp NOT NULL DEFAULT now(),
was_used bool NOT NULL
)`,
`CREATE TABLE hydra_oauth2_authentication_request_handled (
challenge varchar(40) NOT NULL PRIMARY KEY,
subject varchar(255) NOT NULL,
remember bool NOT NULL,
remember_for int NOT NULL,
error text NOT NULL,
acr text NOT NULL,
requested_at timestamp NOT NULL DEFAULT now(),
was_used bool NOT NULL
challenge varchar(40) NOT NULL PRIMARY KEY,
subject varchar(255) NOT NULL,
remember bool NOT NULL,
remember_for int NOT NULL,
error text NOT NULL,
acr text NOT NULL,
requested_at timestamp NOT NULL DEFAULT now(),
authenticated_at timestamp NOT NULL DEFAULT now(),
was_used bool NOT NULL
)`,
},
Down: []string{
Expand All @@ -109,6 +113,7 @@ var sqlParamsAuthenticationRequestHandled = []string{
"remember_for",
"error",
"requested_at",
"authenticated_at",
"acr",
"was_used",
}
Expand All @@ -121,6 +126,7 @@ var sqlParamsRequest = []string{
"request_url",
"skip",
"requested_scope",
"authenticated_at",
"csrf",
"oidc_context",
}
Expand All @@ -129,6 +135,7 @@ var sqlParamsConsentRequestHandled = []string{
"granted_scope",
"remember",
"remember_for",
"authenticated_at",
"error",
"requested_at",
"session_access_token",
Expand All @@ -142,15 +149,32 @@ var sqlParamsAuthSession = []string{
}

type sqlRequest struct {
OpenIDConnectContext string `db:"oidc_context"`
Client string `db:"client_id"`
Subject string `db:"subject"`
RequestURL string `db:"request_url"`
Skip bool `db:"skip"`
Challenge string `db:"challenge"`
RequestedScope string `db:"requested_scope"`
Verifier string `db:"verifier"`
CSRF string `db:"csrf"`
OpenIDConnectContext string `db:"oidc_context"`
Client string `db:"client_id"`
Subject string `db:"subject"`
RequestURL string `db:"request_url"`
Skip bool `db:"skip"`
Challenge string `db:"challenge"`
RequestedScope string `db:"requested_scope"`
Verifier string `db:"verifier"`
CSRF string `db:"csrf"`
AuthenticatedAt time.Time `db:"authenticated_at"`
}

// Ugly hack to prevent mySql from going berzerk with: Received unexpected error Error 1292: Incorrect datetime value: '0000-00-00' for column 'authenticated_at' at row 1
var zeroDate = time.Unix(1, 0).UTC()

func toMySQLDateHack(t time.Time) time.Time {
if t.IsZero() {
return zeroDate
}
return t
}
func fromMySQLDateHack(t time.Time) time.Time {
if t == zeroDate {
return time.Time{}
}
return t
}

func newSQLConsentRequest(c *ConsentRequest) (*sqlRequest, error) {
Expand All @@ -169,6 +193,7 @@ func newSQLConsentRequest(c *ConsentRequest) (*sqlRequest, error) {
RequestedScope: strings.Join(c.RequestedScope, "|"),
Verifier: c.Verifier,
CSRF: c.CSRF,
AuthenticatedAt: toMySQLDateHack(c.AuthenticatedAt),
}, nil
}

Expand Down Expand Up @@ -205,6 +230,7 @@ func (s *sqlRequest) toConsentRequest(client *client.Client) (*ConsentRequest, e
RequestedScope: stringsx.Splitx(s.RequestedScope, "|"),
Verifier: s.Verifier,
CSRF: s.CSRF,
AuthenticatedAt: fromMySQLDateHack(s.AuthenticatedAt),
}, nil
}

Expand All @@ -218,6 +244,7 @@ type sqlHandledConsentRequest struct {
Challenge string `db:"challenge"`
RequestedAt time.Time `db:"requested_at"`
WasUsed bool `db:"was_used"`
AuthenticatedAt time.Time `db:"authenticated_at"`
}

func newSQLHandledConsentRequest(c *HandledConsentRequest) (*sqlHandledConsentRequest, error) {
Expand Down Expand Up @@ -261,6 +288,7 @@ func newSQLHandledConsentRequest(c *HandledConsentRequest) (*sqlHandledConsentRe
Challenge: c.Challenge,
RequestedAt: c.RequestedAt,
WasUsed: c.WasUsed,
AuthenticatedAt: toMySQLDateHack(c.AuthenticatedAt),
}, nil
}

Expand Down Expand Up @@ -294,20 +322,22 @@ func (s *sqlHandledConsentRequest) toHandledConsentRequest(r *ConsentRequest) (*
IDToken: idt,
AccessToken: at,
},
Error: e,
ConsentRequest: r,
Error: e,
ConsentRequest: r,
AuthenticatedAt: fromMySQLDateHack(s.AuthenticatedAt),
}, nil
}

type sqlHandledAuthenticationRequest struct {
Remember bool `db:"remember"`
RememberFor int `db:"remember_for"`
ACR string `db:"acr"`
Subject string `db:"subject"`
Error string `db:"error"`
Challenge string `db:"challenge"`
RequestedAt time.Time `db:"requested_at"`
WasUsed bool `db:"was_used"`
Remember bool `db:"remember"`
RememberFor int `db:"remember_for"`
ACR string `db:"acr"`
Subject string `db:"subject"`
Error string `db:"error"`
Challenge string `db:"challenge"`
RequestedAt time.Time `db:"requested_at"`
WasUsed bool `db:"was_used"`
AuthenticatedAt time.Time `db:"authenticated_at"`
}

func newSQLHandledAuthenticationRequest(c *HandledAuthenticationRequest) (*sqlHandledAuthenticationRequest, error) {
Expand All @@ -322,14 +352,15 @@ func newSQLHandledAuthenticationRequest(c *HandledAuthenticationRequest) (*sqlHa
}

return &sqlHandledAuthenticationRequest{
ACR: c.ACR,
Subject: c.Subject,
Remember: c.Remember,
RememberFor: c.RememberFor,
Error: e,
Challenge: c.Challenge,
RequestedAt: c.RequestedAt,
WasUsed: c.WasUsed,
ACR: c.ACR,
Subject: c.Subject,
Remember: c.Remember,
RememberFor: c.RememberFor,
Error: e,
Challenge: c.Challenge,
RequestedAt: c.RequestedAt,
WasUsed: c.WasUsed,
AuthenticatedAt: toMySQLDateHack(c.AuthenticatedAt),
}, nil
}

Expand All @@ -353,5 +384,6 @@ func (s *sqlHandledAuthenticationRequest) toHandledAuthenticationRequest(a *Auth
Error: e,
AuthenticationRequest: a,
Subject: s.Subject,
AuthenticatedAt: fromMySQLDateHack(s.AuthenticatedAt),
}, nil
}
52 changes: 31 additions & 21 deletions consent/sql_helper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,28 @@ import (
"github.com/stretchr/testify/require"
)

func TestMySQLHack(t *testing.T) {
now := time.Now().UTC()
assert.EqualValues(t, now, fromMySQLDateHack(toMySQLDateHack(now)))
assert.EqualValues(t, time.Time{}, fromMySQLDateHack(toMySQLDateHack(time.Time{})))
}

func TestSQLAuthenticationConverter(t *testing.T) {
a := &AuthenticationRequest{
OpenIDConnectContext: &OpenIDConnectContext{
ACRValues: []string{"1", "2"},
UILocales: []string{"fr", "de"},
Display: "popup",
},
Client: &client.Client{ID: "client"},
Subject: "subject",
RequestURL: "https://request-url/path",
Skip: true,
Challenge: "challenge",
RequestedScope: []string{"scopea", "scopeb"},
Verifier: "verifier",
CSRF: "csrf",
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
Client: &client.Client{ID: "client"},
Subject: "subject",
RequestURL: "https://request-url/path",
Skip: true,
Challenge: "challenge",
RequestedScope: []string{"scopea", "scopeb"},
Verifier: "verifier",
CSRF: "csrf",
}

b := &HandledAuthenticationRequest{
Expand All @@ -52,6 +59,7 @@ func TestSQLAuthenticationConverter(t *testing.T) {
Remember: true,
Challenge: "challenge",
RequestedAt: time.Now().UTC().Add(-time.Minute),
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
Error: &RequestDeniedError{
Name: "error_name",
Description: "error_description",
Expand Down Expand Up @@ -87,22 +95,24 @@ func TestSQLConsentConverter(t *testing.T) {
UILocales: []string{"fr", "de"},
Display: "popup",
},
Client: &client.Client{ID: "client"},
Subject: "subject",
RequestURL: "https://request-url/path",
Skip: true,
Challenge: "challenge",
RequestedScope: []string{"scopea", "scopeb"},
Verifier: "verifier",
CSRF: "csrf",
Client: &client.Client{ID: "client"},
Subject: "subject",
RequestURL: "https://request-url/path",
Skip: true,
Challenge: "challenge",
RequestedScope: []string{"scopea", "scopeb"},
Verifier: "verifier",
CSRF: "csrf",
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
}

b := &HandledConsentRequest{
ConsentRequest: a,
RememberFor: 10,
Remember: true,
GrantedScope: []string{"asdf", "fdsa"},
Challenge: "challenge",
ConsentRequest: a,
RememberFor: 10,
Remember: true,
GrantedScope: []string{"asdf", "fdsa"},
AuthenticatedAt: time.Now().UTC().Add(-time.Minute),
Challenge: "challenge",
Session: &ConsentRequestSessionData{
AccessToken: map[string]interface{}{"asdf": "fdsa"},
IDToken: map[string]interface{}{"foo": "fab"},
Expand Down

0 comments on commit 2ec3f61

Please sign in to comment.