From d878484c58f1791369a7c6638fb5f96297d0e903 Mon Sep 17 00:00:00 2001 From: "r.v.stupnikov" Date: Wed, 1 Jun 2022 17:59:25 +0400 Subject: [PATCH 1/6] feat: add a flag and a routine for flushing inactive login sessions --- cmd/cli/handler_janitor.go | 12 ++- cmd/cli/handler_janitor_test.go | 89 +++++++++++++++++++ cmd/janitor.go | 7 +- .../janitor_session_test_helper.go | 87 ++++++++++++++++++ persistence/sql/persister_consent.go | 43 +++++++-- x/fosite_storer.go | 2 + 6 files changed, 229 insertions(+), 11 deletions(-) create mode 100644 internal/testhelpers/janitor_session_test_helper.go diff --git a/cmd/cli/handler_janitor.go b/cmd/cli/handler_janitor.go index bec0401fe36..49d8e6ad380 100644 --- a/cmd/cli/handler_janitor.go +++ b/cmd/cli/handler_janitor.go @@ -33,6 +33,7 @@ const ( ConsentRequestLifespan = "consent-request-lifespan" OnlyTokens = "tokens" OnlyRequests = "requests" + OnlyLoginSessions = "login-sessions" OnlyGrants = "grants" ReadFromEnv = "read-from-env" Config = "config" @@ -64,7 +65,10 @@ func (_ *JanitorHandler) Args(cmd *cobra.Command, args []string) error { "- Using the config file with flag -c, --config") } - if !flagx.MustGetBool(cmd, OnlyTokens) && !flagx.MustGetBool(cmd, OnlyRequests) && !flagx.MustGetBool(cmd, OnlyGrants) { + if !flagx.MustGetBool(cmd, OnlyTokens) && + !flagx.MustGetBool(cmd, OnlyRequests) && + !flagx.MustGetBool(cmd, OnlyGrants) && + !flagx.MustGetBool(cmd, OnlyLoginSessions) { return fmt.Errorf("%s\n%s\n", cmd.UsageString(), "Janitor requires at least one of --tokens, --requests or --grants to be set") } @@ -154,6 +158,10 @@ func purge(cmd *cobra.Command, args []string, sl *servicelocatorx.Options, dOpts routineFlags = append(routineFlags, OnlyGrants) } + if flagx.MustGetBool(cmd, OnlyLoginSessions) { + routineFlags = append(routineFlags, OnlyLoginSessions) + } + return cleanupRun(cmd.Context(), notAfter, limit, batchSize, addRoutine(p, routineFlags...)...) } @@ -168,6 +176,8 @@ func addRoutine(p persistence.Persister, names ...string) []cleanupRoutine { routines = append(routines, cleanup(p.FlushInactiveLoginConsentRequests, "login-consent requests")) case OnlyGrants: routines = append(routines, cleanup(p.FlushInactiveGrants, "grants")) + case OnlyLoginSessions: + routines = append(routines, cleanup(p.FlushInactiveLoginSessions, "login sessions")) } } return routines diff --git a/cmd/cli/handler_janitor_test.go b/cmd/cli/handler_janitor_test.go index c99440c23b1..ce694ee914e 100644 --- a/cmd/cli/handler_janitor_test.go +++ b/cmd/cli/handler_janitor_test.go @@ -92,6 +92,95 @@ func TestJanitorHandler_PurgeLoginConsentNotAfter(t *testing.T) { } +func TestJanitorHandler_PurgeLoginSessions(t *testing.T) { + t.Run("case=cleanup-login-session", func(t *testing.T) { + t.Run("case=clean session without consent-request", func(t *testing.T) { + ctx := context.Background() + jt := testhelpers.NewConsentJanitorTestHelper(t.Name()) + reg, err := jt.GetRegistry(ctx, t.Name()) + require.NoError(t, err) + + const sessionID = "session_id" + + // setup + sessionHelper := testhelpers.NewJanitorSessionTestHelper(reg, t.Name()) + sessionHelper.CreateLoginSession(t, ctx, sessionID) + + // cleanup + t.Run("step=cleanup", func(t *testing.T) { + cmdx.ExecNoErr(t, newJanitorCmd(), + "janitor", + fmt.Sprintf("--%s", cli.OnlyLoginSessions), + jt.GetDSN(), + ) + }) + + // validate + sessionHelper.ValidateSessionNotExist(t, ctx, sessionID) + }) + + t.Run("case=clean sessions with consent-request", func(t *testing.T) { + ctx := context.Background() + jt := testhelpers.NewConsentJanitorTestHelper(t.Name()) + reg, err := jt.GetRegistry(ctx, t.Name()) + require.NoError(t, err) + + const ( + sessionID = "session_id_1" + ) + + //setup + sessionHelper := testhelpers.NewJanitorSessionTestHelper(reg, t.Name()) + sessionHelper.CreateLoginSession(t, ctx, sessionID) + sessionHelper.CreateEnvironmentForSession(t, ctx, sessionID) + + // cleanup + t.Run("step=cleanup", func(t *testing.T) { + cmdx.ExecNoErr(t, newJanitorCmd(), + "janitor", + fmt.Sprintf("--%s", cli.OnlyLoginSessions), + jt.GetDSN(), + ) + }) + + // validate + sessionHelper.ValidateSessionExist(t, ctx, sessionID) + }) + + t.Run("case=alive session and flush session", func(t *testing.T) { + ctx := context.Background() + jt := testhelpers.NewConsentJanitorTestHelper(t.Name()) + reg, err := jt.GetRegistry(ctx, t.Name()) + require.NoError(t, err) + + const ( + aliveSessionID = "session_id_1" + flushSessionID = "session_id_flush" + ) + + //setup + sessionHelper := testhelpers.NewJanitorSessionTestHelper(reg, t.Name()) + sessionHelper.CreateLoginSession(t, ctx, aliveSessionID) + sessionHelper.CreateEnvironmentForSession(t, ctx, aliveSessionID) + sessionHelper.CreateLoginSession(t, ctx, flushSessionID) + + // cleanup + t.Run("step=cleanup", func(t *testing.T) { + cmdx.ExecNoErr(t, newJanitorCmd(), + "janitor", + fmt.Sprintf("--%s", cli.OnlyLoginSessions), + jt.GetDSN(), + ) + }) + + // validate + sessionHelper.ValidateSessionExist(t, ctx, aliveSessionID) + sessionHelper.ValidateSessionNotExist(t, ctx, flushSessionID) + }) + + }) +} + func TestJanitorHandler_PurgeLoginConsent(t *testing.T) { /* Login and Consent also needs to be purged on two conditions besides the KeyConsentRequestMaxAge and notAfter time diff --git a/cmd/janitor.go b/cmd/janitor.go index 31ffd4e7a63..87b83d44971 100644 --- a/cmd/janitor.go +++ b/cmd/janitor.go @@ -71,9 +71,10 @@ Janitor can be used in several ways. 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.") cmd.Flags().Duration(cli.ConsentRequestLifespan, 0, "Set the login/consent request lifespan e.g. 1s, 1m, 1h") - cmd.Flags().Bool(cli.OnlyRequests, false, "This will only run the cleanup on requests and will skip token and trust relationships cleanup.") - cmd.Flags().Bool(cli.OnlyTokens, false, "This will only run the cleanup on tokens and will skip requests and trust relationships cleanup.") - cmd.Flags().Bool(cli.OnlyGrants, false, "This will only run the cleanup on trust relationships and will skip requests and token cleanup.") + cmd.Flags().Bool(cli.OnlyRequests, false, "This will only run the cleanup on requests and will skip token, login sessions and trust relationships cleanup.") + cmd.Flags().Bool(cli.OnlyTokens, false, "This will only run the cleanup on tokens and will skip requests, login sessions and trust relationships cleanup.") + cmd.Flags().Bool(cli.OnlyGrants, false, "This will only run the cleanup on trust relationships and will skip requests, login sessions and token cleanup.") + cmd.Flags().Bool(cli.OnlyLoginSessions, false, "This will only run the cleanup on login sessions and will skip requests, trust relationship and token cleanup.") cmd.Flags().BoolP(cli.ReadFromEnv, "e", false, "If set, reads the database connection string from the environment variable DSN or config file key dsn.") configx.RegisterFlags(cmd.PersistentFlags()) return cmd diff --git a/internal/testhelpers/janitor_session_test_helper.go b/internal/testhelpers/janitor_session_test_helper.go new file mode 100644 index 00000000000..8b9c0b8a463 --- /dev/null +++ b/internal/testhelpers/janitor_session_test_helper.go @@ -0,0 +1,87 @@ +package testhelpers + +import ( + "context" + "fmt" + "github.com/ory/hydra/client" + "github.com/ory/hydra/consent" + "github.com/ory/hydra/driver" + "github.com/ory/x/sqlxx" + "github.com/stretchr/testify/require" + "testing" + "time" +) + +type JanitorSessionTestHelper struct { + uniqueName string + reg driver.Registry +} + +func NewJanitorSessionTestHelper(reg driver.Registry, uniqueName string) *JanitorSessionTestHelper { + return &JanitorSessionTestHelper{reg: reg, uniqueName: uniqueName} +} + +func (h *JanitorSessionTestHelper) CreateEnvironmentForSession(t *testing.T, ctx context.Context, id string) { + clientDTO := h.CreateClient(t, ctx) + h.CreateLoginRequest(t, ctx, clientDTO, id) + h.CreateConsentRequest(t, ctx, id) +} + +func (h *JanitorSessionTestHelper) ValidateSessionExist(t *testing.T, ctx context.Context, id string) { + session, err := h.reg.ConsentManager().GetRememberedLoginSession(ctx, id) + require.NoError(t, err) + require.NotNil(t, session) + require.NotZero(t, session.ID) +} + +func (h *JanitorSessionTestHelper) ValidateSessionNotExist(t *testing.T, ctx context.Context, id string) { + session, err := h.reg.ConsentManager().GetRememberedLoginSession(ctx, id) + require.Error(t, err) + require.Nil(t, session) +} + +func (h *JanitorSessionTestHelper) CreateClient(t *testing.T, ctx context.Context) *client.Client { + clientReq := &client.Client{ + OutfacingID: fmt.Sprintf("%s_flush-login-consent-1", h.uniqueName), + RedirectURIs: []string{"http://redirect"}, + } + require.NoError(t, h.reg.ClientManager().CreateClient(ctx, clientReq)) + return clientReq +} + +func (h *JanitorSessionTestHelper) CreateLoginSession(t *testing.T, ctx context.Context, sessionID string) { + require.NoError(t, h.reg.ConsentManager().CreateLoginSession(ctx, &consent.LoginSession{ + ID: sessionID, + Subject: "sub", + Remember: true, + })) +} + +func (h *JanitorSessionTestHelper) CreateLoginRequest(t *testing.T, ctx context.Context, clientDTO *client.Client, sessionID string) { + require.NoError(t, h.reg.ConsentManager().CreateLoginRequest(ctx, &consent.LoginRequest{ + ID: fmt.Sprintf("%s_flush-login-1", h.uniqueName), + RequestedScope: []string{"foo", "bar"}, + Subject: fmt.Sprintf("%s_flush-login-1", h.uniqueName), + Client: clientDTO, + RequestURL: "http://redirect", + RequestedAt: time.Now().Round(time.Second), + AuthenticatedAt: sqlxx.NullTime(time.Now().Round(time.Second)), + Verifier: fmt.Sprintf("%s_flush-login-1", h.uniqueName), + SessionID: sqlxx.NullString(sessionID), + })) +} + +func (h *JanitorSessionTestHelper) CreateConsentRequest(t *testing.T, ctx context.Context, sessionID string) { + require.NoError(t, h.reg.ConsentManager().CreateConsentRequest(ctx, &consent.ConsentRequest{ + ID: fmt.Sprintf("%s_flush-consent-1", h.uniqueName), + RequestedScope: []string{"foo", "bar"}, + Subject: fmt.Sprintf("%s_flush-consent-1", h.uniqueName), + OpenIDConnectContext: nil, + ClientID: fmt.Sprintf("%s_flush-login-consent-1", h.uniqueName), + RequestURL: "http://redirect", + LoginChallenge: sqlxx.NullString(fmt.Sprintf("%s_flush-login-1", h.uniqueName)), + RequestedAt: time.Now().Round(time.Second), + Verifier: fmt.Sprintf("%s_flush-consent-1", h.uniqueName), + LoginSessionID: sqlxx.NullString(sessionID), + })) +} diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index d99d18a78d0..66761da7b8c 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -11,19 +11,15 @@ import ( "time" "github.com/gobuffalo/pop/v6" - - "github.com/ory/x/sqlxx" - - "github.com/ory/x/errorsx" - - "github.com/pkg/errors" - "github.com/ory/fosite" "github.com/ory/hydra/client" "github.com/ory/hydra/consent" "github.com/ory/hydra/flow" "github.com/ory/hydra/x" + "github.com/ory/x/errorsx" "github.com/ory/x/sqlcon" + "github.com/ory/x/sqlxx" + "github.com/pkg/errors" ) var _ consent.Manager = &Persister{} @@ -611,6 +607,39 @@ func (p *Persister) VerifyAndInvalidateLogoutRequest(ctx context.Context, verifi }) } +func (p *Persister) FlushInactiveLoginSessions(ctx context.Context, _ time.Time, limit, batchSize int) error { + return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error { + // "hydra_oauth2_authentication_request" + var lr consent.LoginRequest + + // "hydra_oauth2_consent_request" + var cr consent.ConsentRequest + + // "hydra_oauth2_authentication_session" + var ls consent.LoginSession + return p.Connection(ctx).RawQuery(fmt.Sprintf(` + DELETE + FROM %[1]s + WHERE NOT EXISTS + ( + SELECT NULL + FROM %[2]s + WHERE %[2]s.login_session_id = %[1]s.id + ) + AND NOT EXISTS + ( + SELECT NULL + FROM %[3]s + WHERE %[3]s.login_session_id = %[1]s.id + ) + `, + (&ls).TableName(), + (&lr).TableName(), + (&cr).TableName()), + ).Exec() + }) +} + func (p *Persister) FlushInactiveLoginConsentRequests(ctx context.Context, notAfter time.Time, limit int, batchSize int) error { ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.FlushInactiveLoginConsentRequests") defer span.End() diff --git a/x/fosite_storer.go b/x/fosite_storer.go index 1afec037710..aefec6e3594 100644 --- a/x/fosite_storer.go +++ b/x/fosite_storer.go @@ -38,6 +38,8 @@ type FositeStorer interface { FlushInactiveRefreshTokens(ctx context.Context, notAfter time.Time, limit int, batchSize int) error + FlushInactiveLoginSessions(ctx context.Context, notAfter time.Time, limit, batchSize int) error + // DeleteOpenIDConnectSession deletes an OpenID Connect session. // This is duplicated from Ory Fosite to help against deprecation linting errors. DeleteOpenIDConnectSession(ctx context.Context, authorizeCode string) error From df38f9096f6d502cb2bbb1c2f266dd49065910e4 Mon Sep 17 00:00:00 2001 From: "r.v.stupnikov" Date: Thu, 2 Jun 2022 17:05:15 +0400 Subject: [PATCH 2/6] feat: format code --- internal/testhelpers/janitor_session_test_helper.go | 8 +++++--- persistence/sql/persister_consent.go | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/testhelpers/janitor_session_test_helper.go b/internal/testhelpers/janitor_session_test_helper.go index 8b9c0b8a463..2d1b682bb81 100644 --- a/internal/testhelpers/janitor_session_test_helper.go +++ b/internal/testhelpers/janitor_session_test_helper.go @@ -3,13 +3,15 @@ package testhelpers import ( "context" "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + "github.com/ory/hydra/client" "github.com/ory/hydra/consent" "github.com/ory/hydra/driver" "github.com/ory/x/sqlxx" - "github.com/stretchr/testify/require" - "testing" - "time" ) type JanitorSessionTestHelper struct { diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 66761da7b8c..39d95c7b6d9 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -11,6 +11,8 @@ import ( "time" "github.com/gobuffalo/pop/v6" + "github.com/pkg/errors" + "github.com/ory/fosite" "github.com/ory/hydra/client" "github.com/ory/hydra/consent" @@ -19,7 +21,6 @@ import ( "github.com/ory/x/errorsx" "github.com/ory/x/sqlcon" "github.com/ory/x/sqlxx" - "github.com/pkg/errors" ) var _ consent.Manager = &Persister{} From b929252dd598bb5eedb7d064d13a1b9dac8a59d6 Mon Sep 17 00:00:00 2001 From: norand94 Date: Thu, 2 Jun 2022 18:27:44 +0400 Subject: [PATCH 3/6] feat: method JanitorSessionTestHelper.ValidateSessionNotExist checks status code --- internal/testhelpers/janitor_session_test_helper.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/testhelpers/janitor_session_test_helper.go b/internal/testhelpers/janitor_session_test_helper.go index 2d1b682bb81..23e74cadbae 100644 --- a/internal/testhelpers/janitor_session_test_helper.go +++ b/internal/testhelpers/janitor_session_test_helper.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/ory/fosite" + "github.com/stretchr/testify/require" "github.com/ory/hydra/client" @@ -39,6 +41,8 @@ func (h *JanitorSessionTestHelper) ValidateSessionExist(t *testing.T, ctx contex func (h *JanitorSessionTestHelper) ValidateSessionNotExist(t *testing.T, ctx context.Context, id string) { session, err := h.reg.ConsentManager().GetRememberedLoginSession(ctx, id) require.Error(t, err) + rpcErr := fosite.ErrorToRFC6749Error(err) + require.Equal(t, fosite.ErrNotFound.StatusCode(), rpcErr.StatusCode()) require.Nil(t, session) } From 01da891e1fdf63fc0394397c3e7729cdaf3acaac Mon Sep 17 00:00:00 2001 From: "r.v.stupnikov" Date: Mon, 4 Jul 2022 17:46:38 +0400 Subject: [PATCH 4/6] feat: method Persister.FlushInactiveLoginSessions supports for limit --- persistence/sql/persister_consent.go | 33 ++++++++++++++++------------ 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index 39d95c7b6d9..e5a12756c82 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -618,26 +618,31 @@ func (p *Persister) FlushInactiveLoginSessions(ctx context.Context, _ time.Time, // "hydra_oauth2_authentication_session" var ls consent.LoginSession - return p.Connection(ctx).RawQuery(fmt.Sprintf(` - DELETE + query := fmt.Sprintf(` + DELETE FROM %[1]s WHERE id in + (SELECT id FROM %[1]s WHERE NOT EXISTS - ( - SELECT NULL - FROM %[2]s - WHERE %[2]s.login_session_id = %[1]s.id - ) + ( + SELECT NULL + FROM %[2]s + WHERE %[2]s.login_session_id = %[1]s.id + ) AND NOT EXISTS - ( - SELECT NULL - FROM %[3]s - WHERE %[3]s.login_session_id = %[1]s.id - ) + ( + SELECT NULL + FROM %[3]s + WHERE %[3]s.login_session_id = %[1]s.id + ) + LIMIT %[4]d) `, (&ls).TableName(), (&lr).TableName(), - (&cr).TableName()), - ).Exec() + (&cr).TableName(), + limit, + ) + + return p.Connection(ctx).RawQuery(query).Exec() }) } From 7a7d55fba25db181c4027178bd8e2dc8553139b9 Mon Sep 17 00:00:00 2001 From: "r.v.stupnikov" Date: Mon, 4 Jul 2022 18:54:33 +0400 Subject: [PATCH 5/6] feat: fix by reviewer's comments --- cmd/cli/handler_janitor.go | 2 +- persistence/sql/persister_consent.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/cli/handler_janitor.go b/cmd/cli/handler_janitor.go index 49d8e6ad380..d18f01c4d18 100644 --- a/cmd/cli/handler_janitor.go +++ b/cmd/cli/handler_janitor.go @@ -70,7 +70,7 @@ func (_ *JanitorHandler) Args(cmd *cobra.Command, args []string) error { !flagx.MustGetBool(cmd, OnlyGrants) && !flagx.MustGetBool(cmd, OnlyLoginSessions) { return fmt.Errorf("%s\n%s\n", cmd.UsageString(), - "Janitor requires at least one of --tokens, --requests or --grants to be set") + "Janitor requires at least one of --tokens, --requests, --grants or --login-sessions to be set") } limit := flagx.MustGetInt(cmd, Limit) diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go index e5a12756c82..0c025113c2b 100644 --- a/persistence/sql/persister_consent.go +++ b/persistence/sql/persister_consent.go @@ -608,7 +608,7 @@ func (p *Persister) VerifyAndInvalidateLogoutRequest(ctx context.Context, verifi }) } -func (p *Persister) FlushInactiveLoginSessions(ctx context.Context, _ time.Time, limit, batchSize int) error { +func (p *Persister) FlushInactiveLoginSessions(ctx context.Context, _ time.Time, limit, _ int) error { return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error { // "hydra_oauth2_authentication_request" var lr consent.LoginRequest From 1acf0973f7d66fb9e29b7075c3307cf9bd0a58cf Mon Sep 17 00:00:00 2001 From: "r.v.stupnikov" Date: Mon, 4 Jul 2022 19:13:33 +0400 Subject: [PATCH 6/6] feat: fix unit test --- cmd/cli/handler_janitor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cli/handler_janitor_test.go b/cmd/cli/handler_janitor_test.go index ce694ee914e..92247523791 100644 --- a/cmd/cli/handler_janitor_test.go +++ b/cmd/cli/handler_janitor_test.go @@ -303,7 +303,7 @@ func TestJanitorHandler_Arguments(t *testing.T) { "janitor", "memory") require.Error(t, err) - require.Contains(t, err.Error(), "Janitor requires at least one of --tokens, --requests or --grants to be set") + require.Contains(t, err.Error(), "Janitor requires at least one of --tokens, --requests, --grants or --login-sessions to be set") cmdx.ExecNoErr(t, cmd.NewRootCmd(nil, nil, nil), "janitor",