Limit concurrent user sessions #27655

joanlopez · 2020-09-18T12:45:05Z

What this PR does / why we need it:

It introduces an extensible way to limit the maximum number of concurrent sessions per user.

Special notes for your reviewer:

Still WIP since I'm looking for DELETE statement improvements, but completely open to feedback.
Manually tested for user & SAML auth mechanisms (cc/ @mjseaman).

marefr

See comments

marefr · 2020-09-21T09:10:06Z

pkg/services/auth/auth_token.go

+)
+
+func (s *UserAuthTokenService) isConcurrentSessionLimitEnabled() bool {
+	return s.Cfg.FeatureToggles[concurrentSessionLimitKey]


Why feature toggle and not a setting in [auth]?

I'm not really sure about this. Maybe @mjseaman can give us with more details.
The issue definition specifies it as such, I suppose it's somehow related with customer requirements.

Feature toggles are only used for temporary features that eventually will be enabled/included per default. Usually used when developing new experimental features. Adding it as a separate setting is basically a toggle.

Agreed with @marefr, we don't need feature toggle in this case, this is not an experimental feature.

marefr · 2020-09-21T09:10:46Z

pkg/services/auth/auth_token.go

@@ -404,6 +410,35 @@ func (s *UserAuthTokenService) rotatedAfterParam() int64 {
 	return getTime().Add(-s.Cfg.LoginMaxInactiveLifetime).Unix()
 }

+const (
+	concurrentSessionLimit    = 3


Would rather have this as a setting in [auth]. WDYT?

Let's wait for Mitch to address this.

But yup, I agree that moving this constant into a configuration variable could be a good idea 👌

I think we shouldn't change default behavior when introducing new features. So I'd prefer to have two config options: one for enabling user session limit and second for setting limit, like:

[auth] enable_user_session_limit = false user_session_limit = 10

Or maybe even just one config option user_session_limit which is 0 by default and disables limit.

Based on earlier setting like login_cookie_name, login_maximum_inactive_lifetime_duration I would suggest login_maximum_concurrent_user_sessions that default to 0 (disabled).

marefr · 2020-09-21T09:12:49Z

pkg/services/auth/auth_token.go

+	}
+
+	return s.SQLStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error {
+		result, err := dbSession.Exec("DELETE FROM user_auth_token WHERE id NOT IN (SELECT id FROM user_auth_token WHERE seen_at <> 0 ORDER BY seen_at DESC LIMIT ?) AND seen_at <> 0;", concurrentSessionLimit-1)


Should filter by user id here. Couldn't this query be written with only delete clause/without in(...), maybe not possible with order by then?

Completely agree, I already figured out the filter by user id missing but I was waiting for feedback to do it. Anyway, thanks for pointing it, it helped me to do not forget it.

Regarding the query, I cannot see a simpler way to do it. Probably, the subquery might be replaced by a JOIN clause but not really sure if in a simpler / clearer way. Feel free to write down an example if you have something in mind, I'd really appreciate it.

Thanks!

Related with the query ☝️ I've already been exploring the approach using something like DELETE ... LIMIT ... OFFSET which might be enough for our case and also seem to be supported by SQLite (also by PosgreSQL & MySQL I guess), but it didn't seem to be supported by the Go client.

… the SQL query)

alexanderzobnin · 2020-09-21T12:30:45Z

pkg/services/auth/auth_token.go

@@ -404,6 +410,35 @@ func (s *UserAuthTokenService) rotatedAfterParam() int64 {
 	return getTime().Add(-s.Cfg.LoginMaxInactiveLifetime).Unix()
 }

+const (
+	concurrentSessionLimit    = 3


I think we shouldn't change default behavior when introducing new features. So I'd prefer to have two config options: one for enabling user session limit and second for setting limit, like:

[auth] enable_user_session_limit = false user_session_limit = 10

Or maybe even just one config option user_session_limit which is 0 by default and disables limit.

alexanderzobnin · 2020-09-21T12:31:51Z

pkg/services/auth/auth_token.go

+)
+
+func (s *UserAuthTokenService) isConcurrentSessionLimitEnabled() bool {
+	return s.Cfg.FeatureToggles[concurrentSessionLimitKey]


Agreed with @marefr, we don't need feature toggle in this case, this is not an experimental feature.

alexanderzobnin · 2020-09-21T12:34:11Z

pkg/services/auth/auth_token.go

+}
+
+func (s *UserAuthTokenService) revokeOldUserTokens(ctx context.Context, userId int64) error {
+	if !s.isConcurrentSessionLimitEnabled() {


Isn't better to check this in CreateToken() function? In that case it looks more straightforward and clear I think.

joanlopez · 2020-09-23T07:23:45Z

After discussing it with @xlson, the initial approach has been changed.

So, the concurrent user session limits are finally implemented as an extensible mechanism that has not limits by default but it can be overwritten when needed.

sakjur

Logic looks good, but I think we're missing an index for user_id on the user_auth_token table.

pkg/services/auth/auth_token.go

sakjur · 2020-09-25T08:38:51Z

pkg/services/auth/auth_token.go

+			(
+			  SELECT id
+			  FROM user_auth_token
+			  WHERE user_id = ?


We don't have an index for user_id in

grafana/pkg/services/sqlstore/migrations/user_auth_token_mig.go

Lines 8 to 27 in b0df728

userAuthTokenV1 := Table{

Name: "user_auth_token",

Columns: []*Column{

{Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true},

{Name: "user_id", Type: DB_BigInt, Nullable: false},

{Name: "auth_token", Type: DB_NVarchar, Length: 100, Nullable: false},

{Name: "prev_auth_token", Type: DB_NVarchar, Length: 100, Nullable: false},

{Name: "user_agent", Type: DB_NVarchar, Length: 255, Nullable: false},

{Name: "client_ip", Type: DB_NVarchar, Length: 255, Nullable: false},

{Name: "auth_token_seen", Type: DB_Bool, Nullable: false},

{Name: "seen_at", Type: DB_Int, Nullable: true},

{Name: "rotated_at", Type: DB_Int, Nullable: false},

{Name: "created_at", Type: DB_Int, Nullable: false},

{Name: "updated_at", Type: DB_Int, Nullable: false},

},

Indices: []*Index{

{Cols: []string{"auth_token"}, Type: UniqueIndex},

{Cols: []string{"prev_auth_token"}, Type: UniqueIndex},

},

}

@marefr wdyt, should we add an index here or is there a better way to go about this?

Interesting one! Let's wait for @marefr point of view, but it might have sense since filtering by user_id will still be required, I guess 🤔

Thanks!

PR significantly changed

marefr · 2020-09-25T09:35:14Z

pkg/services/auth/auth_token.go

+			  ORDER BY seen_at DESC
+			  LIMIT ?
+			)
+		AND user_id = ?
+		AND seen_at != 0;`


Not sure about the logic for seen_at. Maybe moving this above inserting a new token would simplify things? Doing that could do a query to receive all auth tokens for user and then iterate and find the oldest (based on created_at or rotated_at) above maxConcurrentSessions-1 token and finally delete from user_auth_token where id in(<list of id's>).

Doing this you could potentially move all of this logic to enterprise by wrapping and overriding the UserTokenService in similar manner as done with the data source cache service using registry.RegisterOverride (in enterprise). @joanlopez @xlson thoughts?

Yup! I also thought about it, but I did it before just to avoid removing an existing session with the possibility of having issues while creating the new one. But this might be something insignificant for this case. So, if it helps us to simplify the code, I agree we can do it the other way round.

Doing this you could potentially move all of this logic to enterprise by wrapping and overriding the UserTokenService ª in similar manner as done with the data source cache service using registry.RegisterOverride (in enterprise). @joanlopez @xlson thoughts?

This approach produces inject errors because the models.UserTokenService interface is injected into some other services and once both implementations are present it does not know which one should be injected.

Any idea? 🤔 cc/ @marefr @xlson @sakjur

Thank you so much!!! 🙏

joanlopez · 2020-10-06T15:12:11Z

I proceed to close it since it's outdated and no longer valid.

The major outcome from this discussion was #27908.

Thanks guys!!! 👍

joanlopez added 3 commits September 18, 2020 14:29

Add feature toggle to enable the user concurrent sessions limits

ebb4d44

Add some tests for the user concurrent sessions limits feature

4b1dc59

Fix typo

1f7539e

joanlopez added the area/backend/user label Sep 18, 2020

joanlopez requested review from xlson, marefr, alexanderzobnin and AgnesToulet September 18, 2020 12:45

joanlopez requested a review from a team as a code owner September 18, 2020 12:45

joanlopez self-assigned this Sep 18, 2020

joanlopez requested review from papagian and removed request for a team September 18, 2020 12:45

Fix lint

9738294

marefr previously requested changes Sep 21, 2020

View reviewed changes

joanlopez added 2 commits September 21, 2020 12:57

Revoke old user tokens for the given user id (WHERE clause added into…

87bf12b

… the SQL query)

Merge branch 'master' into limit-concurrent-user-sessions

9fd202a

alexanderzobnin previously requested changes Sep 21, 2020

View reviewed changes

grafana deleted a comment from mjseaman Sep 21, 2020

Add extensible mechanism for limiting concurrent user sessions

a174c9f

joanlopez requested review from marefr and alexanderzobnin September 23, 2020 07:24

Merge branch 'master' into limit-concurrent-user-sessions

4f71dd2

sakjur self-requested a review September 25, 2020 07:46

sakjur suggested changes Sep 25, 2020

View reviewed changes

marefr reviewed Sep 25, 2020

View reviewed changes

joanlopez closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit concurrent user sessions #27655

Limit concurrent user sessions #27655

joanlopez commented Sep 18, 2020 •

edited

marefr left a comment

marefr Sep 21, 2020

joanlopez Sep 21, 2020 •

edited by AgnesToulet

marefr Sep 21, 2020

alexanderzobnin Sep 21, 2020

marefr Sep 21, 2020

joanlopez Sep 21, 2020 •

edited

alexanderzobnin Sep 21, 2020

marefr Sep 21, 2020

marefr Sep 21, 2020

joanlopez Sep 21, 2020

joanlopez Sep 21, 2020 •

edited

alexanderzobnin Sep 21, 2020

alexanderzobnin Sep 21, 2020

alexanderzobnin Sep 21, 2020

joanlopez commented Sep 23, 2020 •

edited

sakjur left a comment

sakjur Sep 25, 2020

joanlopez Sep 25, 2020

marefr Sep 25, 2020

joanlopez Sep 25, 2020

joanlopez Sep 29, 2020 •

edited

joanlopez commented Oct 6, 2020

	userAuthTokenV1 := Table{
	Name: "user_auth_token",
	Columns: []*Column{
	{Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true},
	{Name: "user_id", Type: DB_BigInt, Nullable: false},
	{Name: "auth_token", Type: DB_NVarchar, Length: 100, Nullable: false},
	{Name: "prev_auth_token", Type: DB_NVarchar, Length: 100, Nullable: false},
	{Name: "user_agent", Type: DB_NVarchar, Length: 255, Nullable: false},
	{Name: "client_ip", Type: DB_NVarchar, Length: 255, Nullable: false},
	{Name: "auth_token_seen", Type: DB_Bool, Nullable: false},
	{Name: "seen_at", Type: DB_Int, Nullable: true},
	{Name: "rotated_at", Type: DB_Int, Nullable: false},
	{Name: "created_at", Type: DB_Int, Nullable: false},
	{Name: "updated_at", Type: DB_Int, Nullable: false},
	},
	Indices: []*Index{
	{Cols: []string{"auth_token"}, Type: UniqueIndex},
	{Cols: []string{"prev_auth_token"}, Type: UniqueIndex},
	},
	}

Limit concurrent user sessions #27655

Limit concurrent user sessions #27655

Conversation

joanlopez commented Sep 18, 2020 • edited

marefr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanlopez Sep 21, 2020 • edited by AgnesToulet

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanlopez Sep 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanlopez Sep 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanlopez commented Sep 23, 2020 • edited

sakjur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanlopez Sep 29, 2020 • edited

Choose a reason for hiding this comment

joanlopez commented Oct 6, 2020

joanlopez commented Sep 18, 2020 •

edited

joanlopez Sep 21, 2020 •

edited by AgnesToulet

joanlopez Sep 21, 2020 •

edited

joanlopez Sep 21, 2020 •

edited

joanlopez commented Sep 23, 2020 •

edited

joanlopez Sep 29, 2020 •

edited