From 5baea314929c219d468df7f515effbf0226198c1 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Wed, 19 May 2021 09:33:42 +0200 Subject: [PATCH 01/13] perf(janitor): improve delete queries Improve delete queries by separating the data extraction from actual delete. Extraction is made with a configurable limit, using --limit flag, then deletes are made in batch mode with a configurable batch size, using --batch-size flag. Default value for limit is 100000 records and default value for batch size is 100 records. --- cmd/cli/handler_janitor.go | 17 ++- cmd/janitor.go | 2 + internal/testhelpers/janitor_test_helper.go | 10 +- oauth2/fosite_store_helpers.go | 6 +- oauth2/handler.go | 4 +- persistence/sql/persister_consent.go | 140 ++++++++++---------- persistence/sql/persister_oauth2.go | 69 ++++++++-- x/fosite_storer.go | 6 +- 8 files changed, 156 insertions(+), 98 deletions(-) diff --git a/cmd/cli/handler_janitor.go b/cmd/cli/handler_janitor.go index a1c7005f496..05eee3d9455 100644 --- a/cmd/cli/handler_janitor.go +++ b/cmd/cli/handler_janitor.go @@ -20,6 +20,8 @@ import ( ) const ( + Limit = "limit" + BatchSize = "batch-size" KeepIfYounger = "keep-if-younger" AccessLifespan = "access-lifespan" RefreshLifespan = "refresh-lifespan" @@ -111,6 +113,9 @@ func purge(cmd *cobra.Command, args []string) error { p := d.Persister() + limit := flagx.MustGetInt(cmd, Limit) + batchSize := flagx.MustGetInt(cmd, BatchSize) + var routineFlags []string if flagx.MustGetBool(cmd, OnlyTokens) { @@ -121,7 +126,7 @@ func purge(cmd *cobra.Command, args []string) error { routineFlags = append(routineFlags, OnlyRequests) } - return cleanupRun(cmd.Context(), notAfter, addRoutine(p, routineFlags...)...) + return cleanupRun(cmd.Context(), notAfter, limit, batchSize, addRoutine(p, routineFlags...)...) } func addRoutine(p persistence.Persister, names ...string) []cleanupRoutine { @@ -138,11 +143,11 @@ func addRoutine(p persistence.Persister, names ...string) []cleanupRoutine { return routines } -type cleanupRoutine func(ctx context.Context, notAfter time.Time) error +type cleanupRoutine func(ctx context.Context, notAfter time.Time, limit int, batchSize int) error func cleanup(cr cleanupRoutine, routineName string) cleanupRoutine { - return func(ctx context.Context, notAfter time.Time) error { - if err := cr(ctx, notAfter); err != nil { + return func(ctx context.Context, notAfter time.Time, limit int, batchSize int) error { + if err := cr(ctx, notAfter, limit, batchSize); err != nil { return errors.Wrap(errorsx.WithStack(err), fmt.Sprintf("Could not cleanup inactive %s", routineName)) } fmt.Printf("Successfully completed Janitor run on %s\n", routineName) @@ -150,13 +155,13 @@ func cleanup(cr cleanupRoutine, routineName string) cleanupRoutine { } } -func cleanupRun(ctx context.Context, notAfter time.Time, routines ...cleanupRoutine) error { +func cleanupRun(ctx context.Context, notAfter time.Time, limit int, batchSize int, routines ...cleanupRoutine) error { if len(routines) == 0 { return errors.New("clean up run received 0 routines") } for _, r := range routines { - if err := r(ctx, notAfter); err != nil { + if err := r(ctx, notAfter, limit, batchSize); err != nil { return err } } diff --git a/cmd/janitor.go b/cmd/janitor.go index ea324dbda31..862d68aea53 100644 --- a/cmd/janitor.go +++ b/cmd/janitor.go @@ -52,6 +52,8 @@ Janitor can be used in several ways. RunE: cli.NewHandler().Janitor.RunE, Args: cli.NewHandler().Janitor.Args, } + cmd.Flags().Int(cli.Limit, 10000, "Limits the number of records retrieved from database for deletion.") + cmd.Flags().Int(cli.BatchSize, 100, "Limits the number of records retrieved from database for deletion.") cmd.Flags().Duration(cli.KeepIfYounger, 0, "Keep database records that are younger than a specified duration e.g. 1s, 1m, 1h.") cmd.Flags().Duration(cli.AccessLifespan, 0, "Set the access token lifespan e.g. 1s, 1m, 1h.") cmd.Flags().Duration(cli.RefreshLifespan, 0, "Set the refresh token lifespan e.g. 1s, 1m, 1h.") diff --git a/internal/testhelpers/janitor_test_helper.go b/internal/testhelpers/janitor_test_helper.go index 9b07fc75deb..0412cc9ac84 100644 --- a/internal/testhelpers/janitor_test_helper.go +++ b/internal/testhelpers/janitor_test_helper.go @@ -412,7 +412,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // run the cleanup routine t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, notAfter)) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, notAfter, 1000, 100)) }) // validate test @@ -431,7 +431,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // cleanup t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second))) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 1000, 100)) }) // validate @@ -446,7 +446,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // cleanup t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second))) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 1000, 100)) }) // validate @@ -465,7 +465,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // cleanup t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second))) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 1000, 100)) }) // validate @@ -482,7 +482,7 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM // cleanup t.Run("step=cleanup", func(t *testing.T) { - require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second))) + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 1000, 100)) }) // validate diff --git a/oauth2/fosite_store_helpers.go b/oauth2/fosite_store_helpers.go index 1def3c0d715..879f4296592 100644 --- a/oauth2/fosite_store_helpers.go +++ b/oauth2/fosite_store_helpers.go @@ -443,7 +443,7 @@ func testHelperFlushTokens(x InternalRegistry, lifespan time.Duration) func(t *t require.NoError(t, err) } - require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now().Add(-time.Hour*24))) + require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now().Add(-time.Hour*24), 100, 10)) _, err := m.GetAccessTokenSession(ctx, "flush-1", ds) require.NoError(t, err) _, err = m.GetAccessTokenSession(ctx, "flush-2", ds) @@ -451,7 +451,7 @@ func testHelperFlushTokens(x InternalRegistry, lifespan time.Duration) func(t *t _, err = m.GetAccessTokenSession(ctx, "flush-3", ds) require.NoError(t, err) - require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now().Add(-(lifespan+time.Hour/2)))) + require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now().Add(-(lifespan+time.Hour/2)), 100, 10)) _, err = m.GetAccessTokenSession(ctx, "flush-1", ds) require.NoError(t, err) _, err = m.GetAccessTokenSession(ctx, "flush-2", ds) @@ -459,7 +459,7 @@ func testHelperFlushTokens(x InternalRegistry, lifespan time.Duration) func(t *t _, err = m.GetAccessTokenSession(ctx, "flush-3", ds) require.Error(t, err) - require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now())) + require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now(), 100, 10)) _, err = m.GetAccessTokenSession(ctx, "flush-1", ds) require.NoError(t, err) _, err = m.GetAccessTokenSession(ctx, "flush-2", ds) diff --git a/oauth2/handler.go b/oauth2/handler.go index 694c75f236b..2340b712512 100644 --- a/oauth2/handler.go +++ b/oauth2/handler.go @@ -529,12 +529,12 @@ func (h *Handler) FlushHandler(w http.ResponseWriter, r *http.Request, _ httprou fr.NotAfter = time.Now() } - if err := h.r.OAuth2Storage().FlushInactiveAccessTokens(r.Context(), fr.NotAfter); err != nil { + if err := h.r.OAuth2Storage().FlushInactiveAccessTokens(r.Context(), fr.NotAfter, 1000, 100); err != nil { h.r.Writer().WriteError(w, r, err) return } - if err := h.r.OAuth2Storage().FlushInactiveRefreshTokens(r.Context(), fr.NotAfter); err != nil { + if err := h.r.OAuth2Storage().FlushInactiveRefreshTokens(r.Context(), fr.NotAfter, 1000, 100); err != nil { h.r.Writer().WriteError(w, r, err) return } diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 31af5a759ee..cc22639dcde 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -439,7 +439,7 @@ func (p *Persister) VerifyAndInvalidateLogoutRequest(ctx context.Context, verifi }) } -func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAfter time.Time) error { +func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAfter time.Time, limit int, batchSize int) error { /* #nosec G201 table is static */ var lr consent.LoginRequest var lrh consent.HandledLoginRequest @@ -447,76 +447,82 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf var cr consent.ConsentRequest var crh consent.HandledConsentRequest - return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error { + // The value of notAfter should be the minimum between input parameter and request max expire based on its configured age + requestMaxExpire := time.Now().Add(-p.config.ConsentRequestMaxAge()) + if requestMaxExpire.Before(notAfter) { + notAfter = requestMaxExpire + } + + p.r.Logger().Printf("Deleting rejected login and consent requests which are older than %s\n", notAfter) + + challenges := []string{} + + // Select challenges from all requests that can be safely deleted with limit + // where hydra_oauth2_authentication_request were rejected, so either of these is true + // - hydra_oauth2_authentication_request_handled has valid error + // - hydra_oauth2_consent_request_handled has valid error + // AND timed-out + // - hydra_oauth2_authentication_request.requested_at < ttl.login_consent_request + // - hydra_oauth2_authentication_request.requested_at < notAfter + q := p.Connection(ctx).RawQuery(fmt.Sprintf(` + SELECT challenge + FROM %[1]s + JOIN %[2]s + ON %[1]s.challenge = %[2]s.challenge + JOIN %[3]s + ON %[1]s.challenge = %[3]s.login_challenge + JOIN %[4]s + ON %[3]s.challenge = %[4]s.challenge + WHERE ( + (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') + OR (%[4]s.error IS NOT NULL AND %[4]s.error <> '{}' AND %[4]s.error <> '') + ) + AND %[1]s.requested_at < ? + `, + (&lr).TableName(), + (&lrh).TableName(), + (&cr).TableName(), + (&crh).TableName()), + notAfter) + if err := q.Limit(limit).All(&challenges); err == sql.ErrNoRows { + return errors.Wrap(fosite.ErrNotFound, "") + } + + p.r.Logger().Printf("Found challenges %+v\n", challenges) + + // Delete requests and their references in cascade in batch + var err error + for i := 0; i < len(challenges); i += batchSize { + j := i + batchSize + if j > len(challenges) { + j = len(challenges) + } + + p.r.Logger().Printf("Deleting batch %+v\n", challenges[i:j]) + // Delete from hydra_oauth2_authentication_request + err = p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error { + err := p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), + challenges[i:j], + ).Exec() + + if err != nil { + return sqlcon.HandleError(err) + } - // Delete all entries (and their FK) - // where hydra_oauth2_authentication_request were timed-out or rejected - // - hydra_oauth2_authentication_request_handled - // - hydra_oauth2_consent_request_handled - // AND - // - hydra_oauth2_authentication_request.requested_at < ttl.login_consent_request - // - hydra_oauth2_authentication_request.requested_at < notAfter - - // Using NOT EXISTS instead of LEFT JOIN or NOT IN due to - // LEFT JOIN not supported by Postgres and NOT IN will have performance hits with large tables. - // https://stackoverflow.com/questions/19363481/select-rows-which-are-not-present-in-other-table/19364694#19364694 - // Cannot use table aliasing in MYSQL, will work in Postgresql though... - - err := p.Connection(ctx).RawQuery(fmt.Sprintf(` - DELETE - FROM %[1]s - WHERE NOT EXISTS - ( - SELECT NULL - FROM %[2]s - WHERE %[1]s.challenge = %[2]s.challenge AND (%[2]s.error = '{}' OR %[2]s.error = '' OR %[2]s.error is NULL) - ) - AND NOT EXISTS - ( - SELECT NULL - FROM %[3]s - INNER JOIN %[4]s - ON %[3]s.challenge = %[4]s.challenge - WHERE %[1]s.challenge = %[3]s.login_challenge AND (%[4]s.error = '{}' OR %[4]s.error = '' OR %[4]s.error is NULL) - ) - AND requested_at < ? - AND requested_at < ? - `, - (&lr).TableName(), - (&lrh).TableName(), - (&cr).TableName(), - (&crh).TableName()), - time.Now().Add(-p.config.ConsentRequestMaxAge()), - notAfter).Exec() + // This query is needed due to the fact that the first query will not delete cascade to the consent tables + err = p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE login_challenge in (?)", (&cr).TableName()), + challenges[i:j], + ).Exec() + + return sqlcon.HandleError(err) + }) if err != nil { return sqlcon.HandleError(err) } + } - // This query is needed due to the fact that the first query will not delete cascade to the consent tables - // This cleans up the consent requests if requests have timed out or been rejected. - - // Using NOT EXISTS instead of LEFT JOIN or NOT IN due to - // LEFT JOIN not supported by Postgres and NOT IN will have performance hits with large tables. - // https://stackoverflow.com/questions/19363481/select-rows-which-are-not-present-in-other-table/19364694#19364694 - // Cannot use table aliasing in MYSQL, will work in Postgresql though... - err = p.Connection(ctx).RawQuery( - fmt.Sprintf(` - DELETE - FROM %[1]s - WHERE NOT EXISTS - ( - SELECT NULL - FROM %[2]s - WHERE %[1]s.challenge = %[2]s.challenge AND (%[2]s.error = '{}' OR %[2]s.error = '' OR %[2]s.error is NULL) - ) - AND requested_at < ? - AND requested_at < ?`, - (&cr).TableName(), - (&crh).TableName()), - time.Now().Add(-p.config.ConsentRequestMaxAge()), - notAfter).Exec() - - return sqlcon.HandleError(err) - }) + return sqlcon.HandleError(err) } diff --git a/persistence/sql/persister_oauth2.go b/persistence/sql/persister_oauth2.go index fdd859a710d..4ca5535c7f0 100644 --- a/persistence/sql/persister_oauth2.go +++ b/persistence/sql/persister_oauth2.go @@ -361,30 +361,75 @@ func (p *Persister) RevokeAccessToken(ctx context.Context, id string) error { return p.deleteSessionByRequestID(ctx, id, sqlTableAccess) } -func (p *Persister) FlushInactiveAccessTokens(ctx context.Context, notAfter time.Time) error { +func (p *Persister) FlushInactiveAccessTokens(ctx context.Context, notAfter time.Time, limit int, batchSize int) error { /* #nosec G201 table is static */ - err := p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE requested_at < ? AND requested_at < ?", OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), - time.Now().Add(-p.config.AccessTokenLifespan()), + // The value of notAfter should be the minimum between input parameter and access token max expire based on its configured age + requestMaxExpire := time.Now().Add(-p.config.AccessTokenLifespan()) + if requestMaxExpire.Before(notAfter) { + notAfter = requestMaxExpire + } + + signatures := []string{} + + // Select tokens' signatures with limit + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ?", + OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), notAfter, - ).Exec() - if err == sql.ErrNoRows { + ) + if err := q.Limit(limit).All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } + + // Delete tokens in batch + var err error + for i := 0; i < len(signatures); i += batchSize { + j := i + batchSize + if j > len(signatures) { + j = len(signatures) + } + + err = p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), + signatures[i:j], + ).Exec() + } return sqlcon.HandleError(err) } -func (p *Persister) FlushInactiveRefreshTokens(ctx context.Context, notAfter time.Time) error { +func (p *Persister) FlushInactiveRefreshTokens(ctx context.Context, notAfter time.Time, limit int, batchSize int) error { /* #nosec G201 table is static */ - err := p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE requested_at < ? AND requested_at < ?", OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), - time.Now().Add(-p.config.RefreshTokenLifespan()), + // The value of notAfter should be the minimum between input parameter and refresh token max expire based on its configured age + requestMaxExpire := time.Now().Add(-p.config.RefreshTokenLifespan()) + if requestMaxExpire.Before(notAfter) { + notAfter = requestMaxExpire + } + + signatures := []string{} + + // Select tokens' signatures with limit + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ?", + OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), notAfter, - ).Exec() - if err == sql.ErrNoRows { + ) + if err := q.Limit(limit).All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } + // Delete tokens in batch + var err error + for i := 0; i < len(signatures); i += batchSize { + j := i + batchSize + if j > len(signatures) { + j = len(signatures) + } + + err = p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), + signatures[i:j], + ).Exec() + } return sqlcon.HandleError(err) } diff --git a/x/fosite_storer.go b/x/fosite_storer.go index bc120affa3e..91d3f10fa16 100644 --- a/x/fosite_storer.go +++ b/x/fosite_storer.go @@ -42,14 +42,14 @@ type FositeStorer interface { // flush the access token requests from the database. // no data will be deleted after the 'notAfter' timeframe. - FlushInactiveAccessTokens(ctx context.Context, notAfter time.Time) error + FlushInactiveAccessTokens(ctx context.Context, notAfter time.Time, limit int, batchSize int) error // flush the login requests from the database. // this will address the database long-term growth issues discussed in https://github.com/ory/hydra/issues/1574. // no data will be deleted after the 'notAfter' timeframe. - FlushInactiveLoginConsentRequests(ctx context.Context, notAfter time.Time) error + FlushInactiveLoginConsentRequests(ctx context.Context, notAfter time.Time, limit int, batchSize int) error DeleteAccessTokens(ctx context.Context, clientID string) error - FlushInactiveRefreshTokens(ctx context.Context, notAfter time.Time) error + FlushInactiveRefreshTokens(ctx context.Context, notAfter time.Time, limit int, batchSize int) error } From 8b6cb9efe7ca7cf0ef5c5f67f617897afa546a71 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Wed, 19 May 2021 16:42:24 +0200 Subject: [PATCH 02/13] fix(janitor): include unhandled requests in login and consent cleanup This uses LEFT JOIN to select also login and consent requests which did not result in a complete authentication, i.e. user requested login but timed out or user logged in and timed out at consent. --- persistence/sql/persister_consent.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index cc22639dcde..b7e6eb4cdb1 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -465,16 +465,18 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_authentication_request.requested_at < ttl.login_consent_request // - hydra_oauth2_authentication_request.requested_at < notAfter q := p.Connection(ctx).RawQuery(fmt.Sprintf(` - SELECT challenge + SELECT %[1]s.challenge FROM %[1]s - JOIN %[2]s + LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge - JOIN %[3]s + LEFT JOIN %[3]s ON %[1]s.challenge = %[3]s.login_challenge - JOIN %[4]s + LEFT JOIN %[4]s ON %[3]s.challenge = %[4]s.challenge WHERE ( - (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') + (%[2]s.challenge IS NULL) + OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') + OR (%[4]s.challenge IS NULL) OR (%[4]s.error IS NOT NULL AND %[4]s.error <> '{}' AND %[4]s.error <> '') ) AND %[1]s.requested_at < ? From 6943655a60d4dc4a259eb8e31921d8b0b04981f2 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Thu, 20 May 2021 16:55:58 +0200 Subject: [PATCH 03/13] fix(janitor): split login and consent requests extraction This splits in two independent SELECTs the extraction of login and consent requests eligible for deletion. This solves a bug in the single SELECT causing deletion of consent requests where matching login requests were eligible for deletion and vice versa. With independent SELECTs we keep consent requests even if matching login request gets deleted and vice versa. --- persistence/sql/persister_consent.go | 94 +++++++++++++++++----------- 1 file changed, 56 insertions(+), 38 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index b7e6eb4cdb1..50e1f9f4f19 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -457,74 +457,92 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf challenges := []string{} - // Select challenges from all requests that can be safely deleted with limit - // where hydra_oauth2_authentication_request were rejected, so either of these is true - // - hydra_oauth2_authentication_request_handled has valid error - // - hydra_oauth2_consent_request_handled has valid error + // Select challenges from all consent requests that can be safely deleted with limit + // where hydra_oauth2_consent_request were unhandled or rejected, so either of these is true + // - hydra_oauth2_authentication_request_handled does not exist (unhandled) + // - hydra_oauth2_consent_request_handled has valid error (rejected) // AND timed-out - // - hydra_oauth2_authentication_request.requested_at < ttl.login_consent_request - // - hydra_oauth2_authentication_request.requested_at < notAfter + // - hydra_oauth2_consent_request.requested_at < minimum between ttl.login_consent_request and notAfter q := p.Connection(ctx).RawQuery(fmt.Sprintf(` SELECT %[1]s.challenge FROM %[1]s - LEFT JOIN %[2]s - ON %[1]s.challenge = %[2]s.challenge - LEFT JOIN %[3]s - ON %[1]s.challenge = %[3]s.login_challenge - LEFT JOIN %[4]s - ON %[3]s.challenge = %[4]s.challenge + LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge WHERE ( (%[2]s.challenge IS NULL) OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') - OR (%[4]s.challenge IS NULL) - OR (%[4]s.error IS NOT NULL AND %[4]s.error <> '{}' AND %[4]s.error <> '') ) AND %[1]s.requested_at < ? `, - (&lr).TableName(), - (&lrh).TableName(), (&cr).TableName(), (&crh).TableName()), notAfter) + if err := q.Limit(limit).All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } - p.r.Logger().Printf("Found challenges %+v\n", challenges) + p.r.Logger().Printf("Found login challenges %+v\n", challenges) - // Delete requests and their references in cascade in batch - var err error + // Delete in batch consent requests and their references in cascade for i := 0; i < len(challenges); i += batchSize { j := i + batchSize if j > len(challenges) { j = len(challenges) } - p.r.Logger().Printf("Deleting batch %+v\n", challenges[i:j]) - // Delete from hydra_oauth2_authentication_request - err = p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error { - err := p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), - challenges[i:j], - ).Exec() + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&cr).TableName()), + challenges[i:j], + ) - if err != nil { - return sqlcon.HandleError(err) - } + if err := q.Exec(); err != nil { + return sqlcon.HandleError(err) + } + } - // This query is needed due to the fact that the first query will not delete cascade to the consent tables - err = p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE login_challenge in (?)", (&cr).TableName()), - challenges[i:j], - ).Exec() + // Select challenges from all authentication requests that can be safely deleted with limit + // where hydra_oauth2_authentication_request were unhandled or rejected, so either of these is true + // - hydra_oauth2_authentication_request_handled does not exist (unhandled) + // - hydra_oauth2_authentication_request_handled has valid error (rejected) + // AND timed-out + // - hydra_oauth2_authentication_request.requested_at < minimum between ttl.login_consent_request and notAfter + q = p.Connection(ctx).RawQuery(fmt.Sprintf(` + SELECT %[1]s.challenge + FROM %[1]s + LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge + WHERE ( + (%[2]s.challenge IS NULL) + OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') + ) + AND %[1]s.requested_at < ? + `, + (&lr).TableName(), + (&lrh).TableName()), + notAfter) - return sqlcon.HandleError(err) - }) + if err := q.Limit(limit).All(&challenges); err == sql.ErrNoRows { + return errors.Wrap(fosite.ErrNotFound, "") + } - if err != nil { + p.r.Logger().Printf("Found consent challenges %+v\n", challenges) + + // Delete in batch authentication requests + for i := 0; i < len(challenges); i += batchSize { + j := i + batchSize + if j > len(challenges) { + j = len(challenges) + } + + p.r.Logger().Printf("Deleting batch %+v\n", challenges[i:j]) + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), + challenges[i:j], + ) + + if err := q.Exec(); err != nil { return sqlcon.HandleError(err) } } - return sqlcon.HandleError(err) + return nil } From d5bd25258ae809fe5a7c97de09ac2affeba86560 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Thu, 20 May 2021 17:11:14 +0200 Subject: [PATCH 04/13] refactor(janitor): remove unuseful logs --- persistence/sql/persister_consent.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 50e1f9f4f19..7e6f8a6966c 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -453,8 +453,6 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf notAfter = requestMaxExpire } - p.r.Logger().Printf("Deleting rejected login and consent requests which are older than %s\n", notAfter) - challenges := []string{} // Select challenges from all consent requests that can be safely deleted with limit @@ -481,8 +479,6 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf return errors.Wrap(fosite.ErrNotFound, "") } - p.r.Logger().Printf("Found login challenges %+v\n", challenges) - // Delete in batch consent requests and their references in cascade for i := 0; i < len(challenges); i += batchSize { j := i + batchSize @@ -524,8 +520,6 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf return errors.Wrap(fosite.ErrNotFound, "") } - p.r.Logger().Printf("Found consent challenges %+v\n", challenges) - // Delete in batch authentication requests for i := 0; i < len(challenges); i += batchSize { j := i + batchSize @@ -533,7 +527,6 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf j = len(challenges) } - p.r.Logger().Printf("Deleting batch %+v\n", challenges[i:j]) q := p.Connection(ctx).RawQuery( fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), challenges[i:j], From d55622bbbb532896df7a52b1788dde391df41ad6 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 21 May 2021 23:42:42 +0200 Subject: [PATCH 05/13] test: ensure that user is passing limit and batch size greater than 0 This adds a check in janitor command handler to ensure that user is not passing wrong values for limit anch batch flags. This also adds tests for these command line arguments. --- cmd/cli/handler_janitor.go | 5 +++++ cmd/cli/handler_janitor_test.go | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/cmd/cli/handler_janitor.go b/cmd/cli/handler_janitor.go index 05eee3d9455..375c6c1a561 100644 --- a/cmd/cli/handler_janitor.go +++ b/cmd/cli/handler_janitor.go @@ -55,6 +55,11 @@ func (_ *JanitorHandler) Args(cmd *cobra.Command, args []string) error { "Janitor requires either --tokens or --requests or both to be set") } + if flagx.MustGetInt(cmd, Limit) <= 0 || flagx.MustGetInt(cmd, BatchSize) <= 0 { + return fmt.Errorf("%s\n%s\n", cmd.UsageString(), + "Values for --limit and --batch-size should both be greater than 0") + } + return nil } diff --git a/cmd/cli/handler_janitor_test.go b/cmd/cli/handler_janitor_test.go index 49ac4c48f40..e6a5955868e 100644 --- a/cmd/cli/handler_janitor_test.go +++ b/cmd/cli/handler_janitor_test.go @@ -209,4 +209,44 @@ func TestJanitorHandler_Arguments(t *testing.T) { "memory") require.Error(t, err) require.Contains(t, err.Error(), "Janitor requires either --tokens or --requests or both to be set") + + cmdx.ExecNoErr(t, cmd.NewRootCmd(), + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.Limit, "1000"), + fmt.Sprintf("--%s=%s", cli.BatchSize, "100"), + "memory", + ) + + _, _, err = cmdx.ExecCtx(context.Background(), cmd.NewRootCmd(), nil, + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.Limit, "0"), + "memory") + require.Error(t, err) + require.Contains(t, err.Error(), "Values for --limit and --batch-size should both be greater than 0") + + _, _, err = cmdx.ExecCtx(context.Background(), cmd.NewRootCmd(), nil, + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.Limit, "-100"), + "memory") + require.Error(t, err) + require.Contains(t, err.Error(), "Values for --limit and --batch-size should both be greater than 0") + + _, _, err = cmdx.ExecCtx(context.Background(), cmd.NewRootCmd(), nil, + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.BatchSize, "0"), + "memory") + require.Error(t, err) + require.Contains(t, err.Error(), "Values for --limit and --batch-size should both be greater than 0") + + _, _, err = cmdx.ExecCtx(context.Background(), cmd.NewRootCmd(), nil, + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.BatchSize, "-100"), + "memory") + require.Error(t, err) + require.Contains(t, err.Error(), "Values for --limit and --batch-size should both be greater than 0") } From b5b0791f495eda98235fbd8cff79f432dfb21653 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Sat, 22 May 2021 00:17:24 +0200 Subject: [PATCH 06/13] docs(janitor): add info on limit and batch deletes --- cmd/janitor.go | 5 +++-- docs/docs/cli/hydra-janitor.md | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cmd/janitor.go b/cmd/janitor.go index 862d68aea53..a25a9a40930 100644 --- a/cmd/janitor.go +++ b/cmd/janitor.go @@ -12,6 +12,7 @@ func NewJanitorCmd() *cobra.Command { Use: "janitor []", Short: "Clean the database of old tokens and login/consent requests", Long: `This command will cleanup any expired oauth2 tokens as well as login/consent requests. +This will select records to delete with a limit and delete records in batch to ensure that no table locking issues arise in big production databases. ### Warning ### @@ -52,8 +53,8 @@ Janitor can be used in several ways. RunE: cli.NewHandler().Janitor.RunE, Args: cli.NewHandler().Janitor.Args, } - cmd.Flags().Int(cli.Limit, 10000, "Limits the number of records retrieved from database for deletion.") - cmd.Flags().Int(cli.BatchSize, 100, "Limits the number of records retrieved from database for deletion.") + cmd.Flags().Int(cli.Limit, 10000, "Limit the number of records retrieved from database for deletion.") + cmd.Flags().Int(cli.BatchSize, 100, "Define how many records are deleted with each iteration.") cmd.Flags().Duration(cli.KeepIfYounger, 0, "Keep database records that are younger than a specified duration e.g. 1s, 1m, 1h.") cmd.Flags().Duration(cli.AccessLifespan, 0, "Set the access token lifespan e.g. 1s, 1m, 1h.") cmd.Flags().Duration(cli.RefreshLifespan, 0, "Set the refresh token lifespan e.g. 1s, 1m, 1h.") diff --git a/docs/docs/cli/hydra-janitor.md b/docs/docs/cli/hydra-janitor.md index f26fc6e3d8a..12b3804269b 100644 --- a/docs/docs/cli/hydra-janitor.md +++ b/docs/docs/cli/hydra-janitor.md @@ -19,6 +19,8 @@ Clean the database of old tokens and login/consent requests This command will cleanup any expired oauth2 tokens as well as login/consent requests. +This will select records to delete with a limit and delete records in batch +to ensure that no table locking issues arise in big production databases. ### Warning @@ -69,6 +71,8 @@ hydra janitor [<database-url>] [flags] -c, --config strings Path to one or more .json, .yaml, .yml, .toml config files. Values are loaded in the order provided, meaning that the last config file overwrites values from the previous config file. --consent-request-lifespan duration Set the login/consent request lifespan e.g. 1s, 1m, 1h -h, --help help for janitor + --limit Limits the number of records retrieved from database for deletion. This is optional and default value is 10000 records. + --batch-size Define how many records are deleted with each iteration. This is optional and default value is 100 records. --keep-if-younger duration Keep database records that are younger than a specified duration e.g. 1s, 1m, 1h. -e, --read-from-env If set, reads the database connection string from the environment variable DSN or config file key dsn. --refresh-lifespan duration Set the refresh token lifespan e.g. 1s, 1m, 1h. From 254a271d61c915ff4052c9ff99b53b8ab665f0bf Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 25 Jun 2021 12:12:49 +0200 Subject: [PATCH 07/13] fix: --batch-size must be smaller than --limit --- cmd/cli/handler_janitor.go | 8 +++++++- cmd/cli/handler_janitor_test.go | 9 +++++++++ docs/docs/cli/hydra-janitor.md | 9 ++++----- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/cmd/cli/handler_janitor.go b/cmd/cli/handler_janitor.go index 375c6c1a561..944c28d29ff 100644 --- a/cmd/cli/handler_janitor.go +++ b/cmd/cli/handler_janitor.go @@ -55,10 +55,16 @@ func (_ *JanitorHandler) Args(cmd *cobra.Command, args []string) error { "Janitor requires either --tokens or --requests or both to be set") } - if flagx.MustGetInt(cmd, Limit) <= 0 || flagx.MustGetInt(cmd, BatchSize) <= 0 { + limit := flagx.MustGetInt(cmd, Limit) + batchSize := flagx.MustGetInt(cmd, BatchSize) + if limit <= 0 || batchSize <= 0 { return fmt.Errorf("%s\n%s\n", cmd.UsageString(), "Values for --limit and --batch-size should both be greater than 0") } + if batchSize > limit { + return fmt.Errorf("%s\n%s\n", cmd.UsageString(), + "Value for --batch-size must not be greater than value for --limit") + } return nil } diff --git a/cmd/cli/handler_janitor_test.go b/cmd/cli/handler_janitor_test.go index e6a5955868e..52c45149192 100644 --- a/cmd/cli/handler_janitor_test.go +++ b/cmd/cli/handler_janitor_test.go @@ -249,4 +249,13 @@ func TestJanitorHandler_Arguments(t *testing.T) { "memory") require.Error(t, err) require.Contains(t, err.Error(), "Values for --limit and --batch-size should both be greater than 0") + + _, _, err = cmdx.ExecCtx(context.Background(), cmd.NewRootCmd(), nil, + "janitor", + fmt.Sprintf("--%s", cli.OnlyRequests), + fmt.Sprintf("--%s=%s", cli.Limit, "100"), + fmt.Sprintf("--%s=%s", cli.BatchSize, "1000"), + "memory") + require.Error(t, err) + require.Contains(t, err.Error(), "Value for --batch-size must not be greater than value for --limit") } diff --git a/docs/docs/cli/hydra-janitor.md b/docs/docs/cli/hydra-janitor.md index 12b3804269b..14e9505eb3c 100644 --- a/docs/docs/cli/hydra-janitor.md +++ b/docs/docs/cli/hydra-janitor.md @@ -18,9 +18,8 @@ Clean the database of old tokens and login/consent requests ### Synopsis This command will cleanup any expired oauth2 tokens as well as login/consent -requests. -This will select records to delete with a limit and delete records in batch -to ensure that no table locking issues arise in big production databases. +requests. This will select records to delete with a limit and delete records in +batch to ensure that no table locking issues arise in big production databases. ### Warning @@ -71,8 +70,8 @@ hydra janitor [<database-url>] [flags] -c, --config strings Path to one or more .json, .yaml, .yml, .toml config files. Values are loaded in the order provided, meaning that the last config file overwrites values from the previous config file. --consent-request-lifespan duration Set the login/consent request lifespan e.g. 1s, 1m, 1h -h, --help help for janitor - --limit Limits the number of records retrieved from database for deletion. This is optional and default value is 10000 records. - --batch-size Define how many records are deleted with each iteration. This is optional and default value is 100 records. + --limit Limits the number of records retrieved from database for deletion. This is optional and default value is 10000 records. This value should be bigger than your database grow rate, this being calculated in the time interval between two consecutive janitor runs. + --batch-size Define how many records are deleted with each iteration. This is optional and default value is 100 records. This value must be smaller than the limit value. --keep-if-younger duration Keep database records that are younger than a specified duration e.g. 1s, 1m, 1h. -e, --read-from-env If set, reads the database connection string from the environment variable DSN or config file key dsn. --refresh-lifespan duration Set the refresh token lifespan e.g. 1s, 1m, 1h. From 744b1c5045ab4464f746c4a80816cddb528f8eeb Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 25 Jun 2021 12:31:23 +0200 Subject: [PATCH 08/13] fix(janitor): ensure that records to delete are selected in order This fix avoids unexpected behaviors if the janitor cli is executed in parallel. In that case, if we do not order records to delete, we might incur in duplicate deletes. --- persistence/sql/persister_consent.go | 4 ++-- persistence/sql/persister_oauth2.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 7e6f8a6966c..c505ebdcaf0 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -475,7 +475,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf (&crh).TableName()), notAfter) - if err := q.Limit(limit).All(&challenges); err == sql.ErrNoRows { + if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -516,7 +516,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf (&lrh).TableName()), notAfter) - if err := q.Limit(limit).All(&challenges); err == sql.ErrNoRows { + if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } diff --git a/persistence/sql/persister_oauth2.go b/persistence/sql/persister_oauth2.go index 4ca5535c7f0..a60b1ada007 100644 --- a/persistence/sql/persister_oauth2.go +++ b/persistence/sql/persister_oauth2.go @@ -377,7 +377,7 @@ func (p *Persister) FlushInactiveAccessTokens(ctx context.Context, notAfter time OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), notAfter, ) - if err := q.Limit(limit).All(&signatures); err == sql.ErrNoRows { + if err := q.Limit(limit).Order("signature").All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -413,7 +413,7 @@ func (p *Persister) FlushInactiveRefreshTokens(ctx context.Context, notAfter tim OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), notAfter, ) - if err := q.Limit(limit).All(&signatures); err == sql.ErrNoRows { + if err := q.Limit(limit).Order("signature").All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } From e77b6107e596346cd4829e7f01283b7254eaeed1 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 25 Jun 2021 12:43:03 +0200 Subject: [PATCH 09/13] refactor: generate a query multiple times with a single format string --- persistence/sql/persister_consent.go | 38 +++++++++------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index c505ebdcaf0..94b0d0877c3 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -454,6 +454,16 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf } challenges := []string{} + queryFormat := ` + SELECT %[1]s.challenge + FROM %[1]s + LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge + WHERE ( + (%[2]s.challenge IS NULL) + OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') + ) + AND %[1]s.requested_at < ? + ` // Select challenges from all consent requests that can be safely deleted with limit // where hydra_oauth2_consent_request were unhandled or rejected, so either of these is true @@ -461,19 +471,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_consent_request_handled has valid error (rejected) // AND timed-out // - hydra_oauth2_consent_request.requested_at < minimum between ttl.login_consent_request and notAfter - q := p.Connection(ctx).RawQuery(fmt.Sprintf(` - SELECT %[1]s.challenge - FROM %[1]s - LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge - WHERE ( - (%[2]s.challenge IS NULL) - OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') - ) - AND %[1]s.requested_at < ? - `, - (&cr).TableName(), - (&crh).TableName()), - notAfter) + q := p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&cr).TableName(), (&crh).TableName()), notAfter) if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") @@ -502,19 +500,7 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_authentication_request_handled has valid error (rejected) // AND timed-out // - hydra_oauth2_authentication_request.requested_at < minimum between ttl.login_consent_request and notAfter - q = p.Connection(ctx).RawQuery(fmt.Sprintf(` - SELECT %[1]s.challenge - FROM %[1]s - LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge - WHERE ( - (%[2]s.challenge IS NULL) - OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') - ) - AND %[1]s.requested_at < ? - `, - (&lr).TableName(), - (&lrh).TableName()), - notAfter) + q = p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&lr).TableName(), (&lrh).TableName()), notAfter) if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") From 84a7794b2b884902d63c7e0a7a4a828ac6dfffd1 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 25 Jun 2021 15:34:24 +0200 Subject: [PATCH 10/13] fix: run format on versions.json --- docs/versions.json | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/docs/versions.json b/docs/versions.json index 51494024aed..f583affc3e3 100644 --- a/docs/versions.json +++ b/docs/versions.json @@ -1,9 +1 @@ -[ - "v1.10", - "v1.9", - "v1.8", - "v1.7", - "v1.6", - "v1.5", - "v1.4" -] +["v1.10", "v1.9", "v1.8", "v1.7", "v1.6", "v1.5", "v1.4"] From 0ff8afed4fffc4cc8af09e13685826cfa35bc1c6 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Fri, 16 Jul 2021 18:16:06 +0200 Subject: [PATCH 11/13] fix: use only pop RawQuery and make batchSize=1 work --- internal/testhelpers/janitor_test_helper.go | 51 +++++++++++++++++++++ oauth2/fosite_store_helpers.go | 35 ++++++++++++++ persistence/sql/persister_consent.go | 38 ++++++++------- persistence/sql/persister_oauth2.go | 32 +++++++------ 4 files changed, 126 insertions(+), 30 deletions(-) diff --git a/internal/testhelpers/janitor_test_helper.go b/internal/testhelpers/janitor_test_helper.go index 0412cc9ac84..93d3fee9d91 100644 --- a/internal/testhelpers/janitor_test_helper.go +++ b/internal/testhelpers/janitor_test_helper.go @@ -178,6 +178,40 @@ func (j *JanitorConsentTestHelper) LoginRejectionValidate(ctx context.Context, c } } +func (j *JanitorConsentTestHelper) LimitSetup(ctx context.Context, cm consent.Manager, cl client.Manager) func(t *testing.T) { + return func(t *testing.T) { + var err error + + // Create login requests + for _, r := range j.flushLoginRequests { + require.NoError(t, cl.CreateClient(ctx, r.Client)) + require.NoError(t, cm.CreateLoginRequest(ctx, r)) + } + + // Reject each request + for _, r := range j.flushLoginRequests { + _, err = cm.HandleLoginRequest(ctx, r.ID, consent.NewHandledLoginRequest( + r.ID, true, r.RequestedAt, r.AuthenticatedAt)) + require.NoError(t, err) + } + } +} + +func (j *JanitorConsentTestHelper) LimitValidate(ctx context.Context, cm consent.Manager) func(t *testing.T) { + return func(t *testing.T) { + // flush-login-2 and 3 should be cleared now + for _, r := range j.flushLoginRequests { + t.Logf("check login: %s", r.ID) + _, err := cm.GetLoginRequest(ctx, r.ID) + if r.ID == j.flushLoginRequests[0].ID { + require.NoError(t, err) + } else { + require.Error(t, err) + } + } + } +} + func (j *JanitorConsentTestHelper) ConsentRejectionSetup(ctx context.Context, cm consent.Manager, cl client.Manager) func(t *testing.T) { return func(t *testing.T) { var err error @@ -422,6 +456,23 @@ func JanitorTests(conf *config.Provider, consentManager consent.Manager, clientM } }) + t.Run("case=flush-consent-request-limit", func(t *testing.T) { + jt := NewConsentJanitorTestHelper("limit") + + t.Run("case=limit", func(t *testing.T) { + // setup + t.Run("step=setup", jt.LimitSetup(ctx, consentManager, clientManager)) + + // cleanup + t.Run("step=cleanup", func(t *testing.T) { + require.NoError(t, fositeManager.FlushInactiveLoginConsentRequests(ctx, time.Now().Round(time.Second), 2, 1)) + }) + + // validate + t.Run("step=validate", jt.LimitValidate(ctx, consentManager)) + }) + }) + t.Run("case=flush-consent-request-rejection", func(t *testing.T) { jt := NewConsentJanitorTestHelper("loginRejection") diff --git a/oauth2/fosite_store_helpers.go b/oauth2/fosite_store_helpers.go index 879f4296592..577e237bdf8 100644 --- a/oauth2/fosite_store_helpers.go +++ b/oauth2/fosite_store_helpers.go @@ -175,6 +175,7 @@ func TestHelperRunner(t *testing.T, store InternalRegistry, k string) { t.Run(fmt.Sprintf("case=testHelperRevokeRefreshToken/db=%s", k), testHelperRevokeRefreshToken(store)) t.Run(fmt.Sprintf("case=testHelperCreateGetDeletePKCERequestSession/db=%s", k), testHelperCreateGetDeletePKCERequestSession(store)) t.Run(fmt.Sprintf("case=testHelperFlushTokens/db=%s", k), testHelperFlushTokens(store, time.Hour)) + t.Run(fmt.Sprintf("case=testHelperFlushTokensWithLimitAndBatchSize/db=%s", k), testHelperFlushTokensWithLimitAndBatchSize(store, 3, 2)) t.Run(fmt.Sprintf("case=testFositeStoreSetClientAssertionJWT/db=%s", k), testFositeStoreSetClientAssertionJWT(store)) t.Run(fmt.Sprintf("case=testFositeStoreClientAssertionJWTValid/db=%s", k), testFositeStoreClientAssertionJWTValid(store)) t.Run(fmt.Sprintf("case=testHelperDeleteAccessTokens/db=%s", k), testHelperDeleteAccessTokens(store)) @@ -469,6 +470,40 @@ func testHelperFlushTokens(x InternalRegistry, lifespan time.Duration) func(t *t } } +func testHelperFlushTokensWithLimitAndBatchSize(x InternalRegistry, limit int, batchSize int) func(t *testing.T) { + m := x.OAuth2Storage() + ds := &Session{} + + return func(t *testing.T) { + ctx := context.Background() + var requests []*fosite.Request + + // create five expired requests + id := uuid.New() + for i := 0; i < 5; i++ { + r := createTestRequest(fmt.Sprintf("%s-%d", id, i+1)) + r.RequestedAt = time.Now().Add(-2 * time.Hour) + mockRequestForeignKey(t, r.ID, x, false) + require.NoError(t, m.CreateAccessTokenSession(ctx, r.ID, r)) + _, err := m.GetAccessTokenSession(ctx, r.ID, ds) + require.NoError(t, err) + requests = append(requests, r) + } + + require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now(), limit, batchSize)) + _, err := m.GetAccessTokenSession(ctx, requests[0].ID, ds) + require.Error(t, err) + _, err = m.GetAccessTokenSession(ctx, requests[1].ID, ds) + require.Error(t, err) + _, err = m.GetAccessTokenSession(ctx, requests[2].ID, ds) + require.Error(t, err) + _, err = m.GetAccessTokenSession(ctx, requests[3].ID, ds) + require.NoError(t, err) + _, err = m.GetAccessTokenSession(ctx, requests[4].ID, ds) + require.NoError(t, err) + } +} + func testFositeSqlStoreTransactionCommitAccessToken(m InternalRegistry) func(t *testing.T) { return func(t *testing.T) { { diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 94b0d0877c3..b20dbaf7847 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -463,6 +463,8 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf OR (%[2]s.error IS NOT NULL AND %[2]s.error <> '{}' AND %[2]s.error <> '') ) AND %[1]s.requested_at < ? + ORDER BY %[1]s.challenge + LIMIT %[3]d ` // Select challenges from all consent requests that can be safely deleted with limit @@ -471,9 +473,9 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_consent_request_handled has valid error (rejected) // AND timed-out // - hydra_oauth2_consent_request.requested_at < minimum between ttl.login_consent_request and notAfter - q := p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&cr).TableName(), (&crh).TableName()), notAfter) + q := p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&cr).TableName(), (&crh).TableName(), limit), notAfter) - if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { + if err := q.All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -484,13 +486,15 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf j = len(challenges) } - q := p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&cr).TableName()), - challenges[i:j], - ) + if i != j { + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&cr).TableName()), + challenges[i:j], + ) - if err := q.Exec(); err != nil { - return sqlcon.HandleError(err) + if err := q.Exec(); err != nil { + return sqlcon.HandleError(err) + } } } @@ -500,9 +504,9 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf // - hydra_oauth2_authentication_request_handled has valid error (rejected) // AND timed-out // - hydra_oauth2_authentication_request.requested_at < minimum between ttl.login_consent_request and notAfter - q = p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&lr).TableName(), (&lrh).TableName()), notAfter) + q = p.Connection(ctx).RawQuery(fmt.Sprintf(queryFormat, (&lr).TableName(), (&lrh).TableName(), limit), notAfter) - if err := q.Limit(limit).Order("challenge").All(&challenges); err == sql.ErrNoRows { + if err := q.All(&challenges); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -513,13 +517,15 @@ func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAf j = len(challenges) } - q := p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), - challenges[i:j], - ) + if i != j { + q := p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE challenge in (?)", (&lr).TableName()), + challenges[i:j], + ) - if err := q.Exec(); err != nil { - return sqlcon.HandleError(err) + if err := q.Exec(); err != nil { + return sqlcon.HandleError(err) + } } } diff --git a/persistence/sql/persister_oauth2.go b/persistence/sql/persister_oauth2.go index a60b1ada007..1a1803597a8 100644 --- a/persistence/sql/persister_oauth2.go +++ b/persistence/sql/persister_oauth2.go @@ -373,11 +373,11 @@ func (p *Persister) FlushInactiveAccessTokens(ctx context.Context, notAfter time // Select tokens' signatures with limit q := p.Connection(ctx).RawQuery( - fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ?", - OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), + fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ? ORDER BY signature LIMIT %d", + OAuth2RequestSQL{Table: sqlTableAccess}.TableName(), limit), notAfter, ) - if err := q.Limit(limit).Order("signature").All(&signatures); err == sql.ErrNoRows { + if err := q.All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -389,10 +389,12 @@ func (p *Persister) FlushInactiveAccessTokens(ctx context.Context, notAfter time j = len(signatures) } - err = p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), - signatures[i:j], - ).Exec() + if i != j { + err = p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableAccess}.TableName()), + signatures[i:j], + ).Exec() + } } return sqlcon.HandleError(err) } @@ -409,11 +411,11 @@ func (p *Persister) FlushInactiveRefreshTokens(ctx context.Context, notAfter tim // Select tokens' signatures with limit q := p.Connection(ctx).RawQuery( - fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ?", - OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), + fmt.Sprintf("SELECT signature FROM %s WHERE requested_at < ? ORDER BY signature LIMIT %d", + OAuth2RequestSQL{Table: sqlTableRefresh}.TableName(), limit), notAfter, ) - if err := q.Limit(limit).Order("signature").All(&signatures); err == sql.ErrNoRows { + if err := q.All(&signatures); err == sql.ErrNoRows { return errors.Wrap(fosite.ErrNotFound, "") } @@ -425,10 +427,12 @@ func (p *Persister) FlushInactiveRefreshTokens(ctx context.Context, notAfter tim j = len(signatures) } - err = p.Connection(ctx).RawQuery( - fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), - signatures[i:j], - ).Exec() + if i != j { + err = p.Connection(ctx).RawQuery( + fmt.Sprintf("DELETE FROM %s WHERE signature in (?)", OAuth2RequestSQL{Table: sqlTableRefresh}.TableName()), + signatures[i:j], + ).Exec() + } } return sqlcon.HandleError(err) } From b54d3e4fd07af169dc7247b7a09f824fc2053d6c Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Sat, 17 Jul 2021 15:33:52 +0200 Subject: [PATCH 12/13] fix: upgrade github.com/gobuffalo/packr --- go.mod | 1 + go.sum | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/go.mod b/go.mod index e84c54ae83f..4adb30dfb2b 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/go-openapi/swag v0.19.13 github.com/go-openapi/validate v0.20.1 github.com/go-swagger/go-swagger v0.26.1 + github.com/gobuffalo/packr v1.30.1 // indirect github.com/gobuffalo/pop/v5 v5.3.4 github.com/gobuffalo/x v0.0.0-20181007152206-913e47c59ca7 github.com/gobwas/glob v0.2.3 diff --git a/go.sum b/go.sum index e2ae256948d..ed631d432a2 100644 --- a/go.sum +++ b/go.sum @@ -526,6 +526,8 @@ github.com/gobuffalo/packr v1.20.0/go.mod h1:JDytk1t2gP+my1ig7iI4NcVaXr886+N0ecU github.com/gobuffalo/packr v1.21.0/go.mod h1:H00jGfj1qFKxscFJSw8wcL4hpQtPe1PfU2wa6sg/SR0= github.com/gobuffalo/packr v1.22.0 h1:/YVd/GRGsu0QuoCJtlcWSVllobs4q3Xvx3nqxTvPyN0= github.com/gobuffalo/packr v1.22.0/go.mod h1:Qr3Wtxr3+HuQEwWqlLnNW4t1oTvK+7Gc/Rnoi/lDFvA= +github.com/gobuffalo/packr v1.30.1 h1:hu1fuVR3fXEZR7rXNW3h8rqSML8EVAf6KNm0NKO/wKg= +github.com/gobuffalo/packr v1.30.1/go.mod h1:ljMyFO2EcrnzsHsN99cvbq055Y9OhRrIaviy289eRuk= github.com/gobuffalo/packr/v2 v2.0.0-rc.8/go.mod h1:y60QCdzwuMwO2R49fdQhsjCPv7tLQFR0ayzxxla9zes= github.com/gobuffalo/packr/v2 v2.0.0-rc.9/go.mod h1:fQqADRfZpEsgkc7c/K7aMew3n4aF1Kji7+lIZeR98Fc= github.com/gobuffalo/packr/v2 v2.0.0-rc.10/go.mod h1:4CWWn4I5T3v4c1OsJ55HbHlUEKNWMITG5iIkdr4Px4w= @@ -537,6 +539,7 @@ github.com/gobuffalo/packr/v2 v2.0.0-rc.15/go.mod h1:IMe7H2nJvcKXSF90y4X1rjYIRlN github.com/gobuffalo/packr/v2 v2.0.9/go.mod h1:emmyGweYTm6Kdper+iywB6YK5YzuKchGtJQZ0Odn4pQ= github.com/gobuffalo/packr/v2 v2.2.0/go.mod h1:CaAwI0GPIAv+5wKLtv8Afwl+Cm78K/I/VCm/3ptBN+0= github.com/gobuffalo/packr/v2 v2.4.0/go.mod h1:ra341gygw9/61nSjAbfwcwh8IrYL4WmR4IsPkPBhQiY= +github.com/gobuffalo/packr/v2 v2.5.1/go.mod h1:8f9c96ITobJlPzI44jj+4tHnEKNt0xXWSVlXRN9X1Iw= github.com/gobuffalo/packr/v2 v2.5.2/go.mod h1:sgEE1xNZ6G0FNN5xn9pevVu4nywaxHvgup67xisti08= github.com/gobuffalo/packr/v2 v2.7.1/go.mod h1:qYEvAazPaVxy7Y7KR0W8qYEE+RymX74kETFqjFoFlOc= github.com/gobuffalo/packr/v2 v2.8.0 h1:IULGd15bQL59ijXLxEvA5wlMxsmx/ZkQv9T282zNVIY= @@ -1753,6 +1756,7 @@ golang.org/x/tools v0.0.0-20190613204242-ed0dc450797f/go.mod h1:/rFqwRUd4F7ZHNgw golang.org/x/tools v0.0.0-20190614205625-5aca471b1d59/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190617190820-da514acc4774/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190621195816-6e04913cbbac/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= +golang.org/x/tools v0.0.0-20190624180213-70d37148ca0c/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190624190245-7f2218787638/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190624222133-a101b041ded4/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= golang.org/x/tools v0.0.0-20190628153133-6cdbf07be9d0/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc= From 12661f2ed1304013be293cfeb6cc0c7f42e25419 Mon Sep 17 00:00:00 2001 From: Flavio Leggio Date: Sun, 18 Jul 2021 12:49:43 +0200 Subject: [PATCH 13/13] refactor: loop over requests in flush tokens validation --- oauth2/fosite_store_helpers.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/oauth2/fosite_store_helpers.go b/oauth2/fosite_store_helpers.go index 577e237bdf8..87b8168d3c3 100644 --- a/oauth2/fosite_store_helpers.go +++ b/oauth2/fosite_store_helpers.go @@ -491,16 +491,14 @@ func testHelperFlushTokensWithLimitAndBatchSize(x InternalRegistry, limit int, b } require.NoError(t, m.FlushInactiveAccessTokens(ctx, time.Now(), limit, batchSize)) - _, err := m.GetAccessTokenSession(ctx, requests[0].ID, ds) - require.Error(t, err) - _, err = m.GetAccessTokenSession(ctx, requests[1].ID, ds) - require.Error(t, err) - _, err = m.GetAccessTokenSession(ctx, requests[2].ID, ds) - require.Error(t, err) - _, err = m.GetAccessTokenSession(ctx, requests[3].ID, ds) - require.NoError(t, err) - _, err = m.GetAccessTokenSession(ctx, requests[4].ID, ds) - require.NoError(t, err) + for i := range requests { + _, err := m.GetAccessTokenSession(ctx, requests[i].ID, ds) + if i >= limit { + require.NoError(t, err) + } else { + require.Error(t, err) + } + } } }