diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 2f4d96571eb..545de311912 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -2902,6 +2902,10 @@ func createUserAccessToken(c *Context, w http.ResponseWriter, r *http.Request) { accessToken.UserId = c.Params.UserId accessToken.Token = "" + // TODO: remove once the API officially supports setting expires_at; until + // then, strip any client-supplied value so that JSON-decoded requests cannot + // set an arbitrary (or zero) expiry through the create-token endpoint. + accessToken.ExpiresAt = 0 token, err := c.App.CreateUserAccessToken(c.AppContext, &accessToken) if err != nil { diff --git a/server/channels/app/job.go b/server/channels/app/job.go index f5df4cb2ab5..63c7e2cf311 100644 --- a/server/channels/app/job.go +++ b/server/channels/app/job.go @@ -223,7 +223,8 @@ func (a *App) SessionHasPermissionToCreateJob(session model.Session, job *model. model.JobTypeExportProcess, model.JobTypeExportDelete, model.JobTypeCloud, - model.JobTypeExtractContent: + model.JobTypeExtractContent, + model.JobTypeCleanupExpiredAccessTokens: return a.SessionHasPermissionTo(session, model.PermissionManageJobs), model.PermissionManageJobs case model.JobTypeAccessControlSync: // Allow system admins to create access control sync jobs @@ -294,7 +295,8 @@ func (a *App) SessionHasPermissionToManageJob(session model.Session, job *model. model.JobTypeExportProcess, model.JobTypeExportDelete, model.JobTypeCloud, - model.JobTypeExtractContent: + model.JobTypeExtractContent, + model.JobTypeCleanupExpiredAccessTokens: permission = model.PermissionManageJobs case model.JobTypeAccessControlSync: permission = model.PermissionManageSystem @@ -331,7 +333,8 @@ func (a *App) SessionHasPermissionToReadJob(session model.Session, jobType strin model.JobTypeExportDelete, model.JobTypeCloud, model.JobTypeMobileSessionMetadata, - model.JobTypeExtractContent: + model.JobTypeExtractContent, + model.JobTypeCleanupExpiredAccessTokens: return a.SessionHasPermissionTo(session, model.PermissionReadJobs), model.PermissionReadJobs case model.JobTypeAccessControlSync: return a.SessionHasPermissionTo(session, model.PermissionManageSystem), model.PermissionManageSystem diff --git a/server/channels/app/server.go b/server/channels/app/server.go index 518fe6930a1..3515dad7a6d 100644 --- a/server/channels/app/server.go +++ b/server/channels/app/server.go @@ -41,6 +41,7 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/jobs" "github.com/mattermost/mattermost/server/v8/channels/jobs/active_users" "github.com/mattermost/mattermost/server/v8/channels/jobs/cleanup_desktop_tokens" + "github.com/mattermost/mattermost/server/v8/channels/jobs/cleanup_expired_access_tokens" "github.com/mattermost/mattermost/server/v8/channels/jobs/delete_dms_preferences_migration" "github.com/mattermost/mattermost/server/v8/channels/jobs/delete_empty_drafts_migration" "github.com/mattermost/mattermost/server/v8/channels/jobs/delete_expired_posts" @@ -1724,6 +1725,12 @@ func (s *Server) initJobs() { cleanup_desktop_tokens.MakeScheduler(s.Jobs), ) + s.Jobs.RegisterJobType( + model.JobTypeCleanupExpiredAccessTokens, + cleanup_expired_access_tokens.MakeWorker(s.Jobs, s.platform.ClearUserSessionCache), + cleanup_expired_access_tokens.MakeScheduler(s.Jobs), + ) + s.Jobs.RegisterJobType( model.JobTypeRefreshMaterializedViews, refresh_materialized_views.MakeWorker(s.Jobs, *s.platform.Config().SqlSettings.DriverName), diff --git a/server/channels/app/session.go b/server/channels/app/session.go index af6c46c3bcb..0068cd3e49e 100644 --- a/server/channels/app/session.go +++ b/server/channels/app/session.go @@ -369,6 +369,20 @@ func (a *App) GetSessionLengthInMillis(session *model.Session) int64 { return 0 } + // For PAT sessions with a fixed expiry, return the remaining lifetime so + // that ExtendSessionExpiryIfNeeded never pushes ExpiresAt past the token's + // own expiry. The elapsed threshold check collapses to zero, so extension + // is effectively a no-op for these sessions (correct: the expiry is fixed). + // PAT sessions with ExpiresAt == 0 (non-expiring) fall through to normal + // web-session behavior. + if session.Props[model.SessionPropType] == model.SessionTypeUserAccessToken && session.ExpiresAt > 0 { + remaining := session.ExpiresAt - model.GetMillis() + if remaining < 0 { + return 0 + } + return remaining + } + var hours int if session.IsMobileApp() { hours = *a.Config().ServiceSettings.SessionLengthMobileInHours @@ -451,6 +465,15 @@ func (a *App) createSessionForUserAccessToken(rctx request.CTX, tokenString stri return nil, model.NewAppError("createSessionForUserAccessToken", "app.user_access_token.invalid_or_missing", nil, "EnableUserAccessTokens=false", http.StatusUnauthorized) } + if token.IsExpired() { + auditRec := a.MakeAuditRecord(rctx, model.AuditEventRejectExpiredUserAccessToken, model.AuditStatusFail) + auditRec.AddMeta("token_id", token.Id) + auditRec.AddMeta("user_id", token.UserId) + auditRec.AddMeta("expires_at", token.ExpiresAt) + a.LogAuditRec(rctx, auditRec, nil) + return nil, model.NewAppError("createSessionForUserAccessToken", "app.user_access_token.expired", nil, "expired_token", http.StatusUnauthorized) + } + if user.DeleteAt != 0 { return nil, model.NewAppError("createSessionForUserAccessToken", "app.user_access_token.invalid_or_missing", nil, "inactive_user_id="+user.Id, http.StatusUnauthorized) } @@ -478,6 +501,12 @@ func (a *App) createSessionForUserAccessToken(rctx request.CTX, tokenString stri } a.ch.srv.platform.SetSessionExpireInHours(session, model.SessionUserAccessTokenExpiryHours) + // If the underlying PAT has a non-zero expiry, clamp the session expiry to + // the token's ExpiresAt so that cached sessions honor PAT expiry as well. + if token.ExpiresAt > 0 && (session.ExpiresAt == 0 || token.ExpiresAt < session.ExpiresAt) { + session.ExpiresAt = token.ExpiresAt + } + session, nErr = a.Srv().Store().Session().Save(rctx, session) if nErr != nil { var invErr *store.ErrInvalidInput diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index d85e7155fcd..b51b603797b 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -369,3 +369,7 @@ channels/db/migrations/postgres/000185_create_channel_guards.down.sql channels/db/migrations/postgres/000185_create_channel_guards.up.sql channels/db/migrations/postgres/000186_create_channel_guards_plugin_id_index.down.sql channels/db/migrations/postgres/000186_create_channel_guards_plugin_id_index.up.sql +channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.down.sql +channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.up.sql +channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.down.sql +channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.up.sql diff --git a/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.down.sql b/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.down.sql new file mode 100644 index 00000000000..b0d5ff83f6c --- /dev/null +++ b/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.down.sql @@ -0,0 +1 @@ +ALTER TABLE useraccesstokens DROP COLUMN IF EXISTS expiresat; diff --git a/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.up.sql b/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.up.sql new file mode 100644 index 00000000000..311cd6adb4d --- /dev/null +++ b/server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.up.sql @@ -0,0 +1 @@ +ALTER TABLE useraccesstokens ADD COLUMN IF NOT EXISTS expiresat bigint NOT NULL DEFAULT 0; diff --git a/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.down.sql b/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.down.sql new file mode 100644 index 00000000000..2534cd3ad1a --- /dev/null +++ b/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.down.sql @@ -0,0 +1,2 @@ +-- morph:nontransactional +DROP INDEX CONCURRENTLY IF EXISTS idx_useraccesstokens_expiresat; diff --git a/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.up.sql b/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.up.sql new file mode 100644 index 00000000000..a023f331725 --- /dev/null +++ b/server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.up.sql @@ -0,0 +1,4 @@ +-- morph:nontransactional +CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_useraccesstokens_expiresat + ON useraccesstokens (expiresat) + WHERE expiresat > 0; diff --git a/server/channels/jobs/cleanup_expired_access_tokens/scheduler.go b/server/channels/jobs/cleanup_expired_access_tokens/scheduler.go new file mode 100644 index 00000000000..8f0107c3f21 --- /dev/null +++ b/server/channels/jobs/cleanup_expired_access_tokens/scheduler.go @@ -0,0 +1,20 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package cleanup_expired_access_tokens + +import ( + "time" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/jobs" +) + +const schedFreq = 1 * time.Hour + +func MakeScheduler(jobServer *jobs.JobServer) *jobs.PeriodicScheduler { + isEnabled := func(cfg *model.Config) bool { + return *cfg.ServiceSettings.EnableUserAccessTokens + } + return jobs.NewPeriodicScheduler(jobServer, model.JobTypeCleanupExpiredAccessTokens, schedFreq, isEnabled) +} diff --git a/server/channels/jobs/cleanup_expired_access_tokens/worker.go b/server/channels/jobs/cleanup_expired_access_tokens/worker.go new file mode 100644 index 00000000000..71fa4f0f988 --- /dev/null +++ b/server/channels/jobs/cleanup_expired_access_tokens/worker.go @@ -0,0 +1,112 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package cleanup_expired_access_tokens + +import ( + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" + "github.com/mattermost/mattermost/server/v8/channels/jobs" +) + +const ( + workerName = "CleanupExpiredAccessTokens" + // batchLimit bounds both the number of rows fetched by GetExpiredBefore + // and the corresponding DeleteByIds call, keeping the transaction + // footprint bounded even when a large number of tokens expire at once. + batchLimit = 1000 + // maxBatches caps the number of iterations per job execution so that very + // large expiry backlogs are drained across multiple scheduled runs rather + // than a single unbounded loop. + maxBatches = 1000 +) + +// expiredTokenStore is the subset of UserAccessTokenStore used by the worker. +// Defined here rather than depending on the full store interface so the +// orchestration logic can be unit-tested with a small fake. +type expiredTokenStore interface { + GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) + DeleteByIds(tokenIDs []string) (int64, error) +} + +// MakeWorker creates a worker that periodically deletes personal access tokens +// whose ExpiresAt has passed, along with any sessions created from them. +// The work is done in batches to keep the transaction footprint bounded. +// +// clearSessionCache is called for each affected user after their tokens are +// deleted so that in-memory session caches don't serve stale sessions. +func MakeWorker(jobServer *jobs.JobServer, clearSessionCache func(userID string)) *jobs.SimpleWorker { + isEnabled := func(cfg *model.Config) bool { + return *cfg.ServiceSettings.EnableUserAccessTokens + } + + execute := func(logger mlog.LoggerIFace, job *model.Job) error { + defer jobServer.HandleJobPanic(logger, job) + return cleanupExpired( + logger, + jobServer.Store.UserAccessToken(), + clearSessionCache, + model.GetMillis(), + batchLimit, + maxBatches, + ) + } + + return jobs.NewSimpleWorker(workerName, jobServer, execute, isEnabled) +} + +// cleanupExpired drains expired personal access tokens in batches up to +// maxBatches iterations. It is extracted from MakeWorker so that the batching +// and error-propagation logic can be exercised by unit tests with a fake store. +// +// clearSessionCache is called for each unique user whose tokens were deleted so +// that in-memory session caches don't continue serving the removed sessions. +func cleanupExpired( + logger mlog.LoggerIFace, + store expiredTokenStore, + clearSessionCache func(userID string), + cutoff int64, + limit int, + maxIter int, +) error { + var totalDeleted int64 + + for range maxIter { + expired, err := store.GetExpiredBefore(cutoff, limit) + if err != nil { + return err + } + if len(expired) == 0 { + break + } + + ids := make([]string, len(expired)) + userIDs := make(map[string]struct{}, len(expired)) + for i, token := range expired { + ids[i] = token.Id + userIDs[token.UserId] = struct{}{} + } + + deleted, err := store.DeleteByIds(ids) + if err != nil { + return err + } + totalDeleted += deleted + + for userID := range userIDs { + clearSessionCache(userID) + } + + if len(expired) < limit { + break + } + } + + logger.Info( + "Cleaned up expired personal access tokens", + mlog.Int("deleted", int(totalDeleted)), + mlog.Int("cutoff", int(cutoff)), + ) + + return nil +} diff --git a/server/channels/jobs/cleanup_expired_access_tokens/worker_test.go b/server/channels/jobs/cleanup_expired_access_tokens/worker_test.go new file mode 100644 index 00000000000..f497a807e3c --- /dev/null +++ b/server/channels/jobs/cleanup_expired_access_tokens/worker_test.go @@ -0,0 +1,169 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package cleanup_expired_access_tokens + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" +) + +// fakeStore implements expiredTokenStore. Each call to GetExpiredBefore pops +// the next pre-programmed batch off batches, then returns the configured error +// (which can be nil). DeleteByIds returns deleteCount/deleteErr and records +// the ids it was called with. +type fakeStore struct { + batches [][]*model.UserAccessToken + getCalls int + getErrAt int // 1-based call index that returns getErr; 0 == no error + getErr error + deleteCnt int64 + deleteErr error + deletedIDs [][]string +} + +func (f *fakeStore) GetExpiredBefore(_ int64, _ int) ([]*model.UserAccessToken, error) { + f.getCalls++ + if f.getErrAt != 0 && f.getCalls == f.getErrAt { + return nil, f.getErr + } + if len(f.batches) == 0 { + return nil, nil + } + next := f.batches[0] + f.batches = f.batches[1:] + return next, nil +} + +func (f *fakeStore) DeleteByIds(ids []string) (int64, error) { + f.deletedIDs = append(f.deletedIDs, ids) + if f.deleteErr != nil { + return 0, f.deleteErr + } + if f.deleteCnt != 0 { + return f.deleteCnt, nil + } + return int64(len(ids)), nil +} + +func makeTokens(n int, base int64) []*model.UserAccessToken { + out := make([]*model.UserAccessToken, n) + for i := range n { + out[i] = &model.UserAccessToken{ + Id: model.NewId(), + UserId: model.NewId(), + ExpiresAt: base + int64(i), + IsActive: true, + } + } + return out +} + +func nopClearSession(_ string) {} + +func TestCleanupExpired(t *testing.T) { + logger := mlog.CreateConsoleTestLogger(t) + + t.Run("happy path single batch", func(t *testing.T) { + tokens := makeTokens(3, 1000) + store := &fakeStore{batches: [][]*model.UserAccessToken{tokens}} + + err := cleanupExpired(logger, store, nopClearSession, 9999, 1000, 10) + require.NoError(t, err) + + // Exactly one DeleteByIds call with the three token ids. A partial first + // batch (len < limit) must short-circuit the loop, so GetExpiredBefore is + // called exactly once. + require.Len(t, store.deletedIDs, 1) + require.Len(t, store.deletedIDs[0], 3) + require.Equal(t, 1, store.getCalls) + }) + + t.Run("empty result is no-op", func(t *testing.T) { + store := &fakeStore{} // no batches, no errors + + err := cleanupExpired(logger, store, nopClearSession, 9999, 1000, 10) + require.NoError(t, err) + + require.Equal(t, 1, store.getCalls) + require.Empty(t, store.deletedIDs) + }) + + t.Run("full batch triggers next iteration", func(t *testing.T) { + const limit = 5 + first := makeTokens(limit, 1000) // full batch -> loop continues + second := makeTokens(2, 2000) // partial batch -> loop stops + store := &fakeStore{batches: [][]*model.UserAccessToken{first, second}} + + err := cleanupExpired(logger, store, nopClearSession, 9999, limit, 10) + require.NoError(t, err) + + require.Equal(t, 2, store.getCalls) + require.Len(t, store.deletedIDs, 2) + require.Len(t, store.deletedIDs[0], limit) + require.Len(t, store.deletedIDs[1], 2) + }) + + t.Run("max iter cap", func(t *testing.T) { + const limit = 3 + const maxIter = 2 + store := &fakeStore{batches: [][]*model.UserAccessToken{ + makeTokens(limit, 1000), + makeTokens(limit, 2000), + makeTokens(limit, 3000), // never reached + }} + + err := cleanupExpired(logger, store, nopClearSession, 9999, limit, maxIter) + require.NoError(t, err) + + require.Equal(t, maxIter, store.getCalls, "loop must cap at maxIter") + require.Len(t, store.deletedIDs, maxIter) + }) + + t.Run("get error propagates", func(t *testing.T) { + wantErr := errors.New("get failed") + store := &fakeStore{ + batches: [][]*model.UserAccessToken{makeTokens(2, 1000)}, + getErrAt: 1, + getErr: wantErr, + } + + err := cleanupExpired(logger, store, nopClearSession, 9999, 1000, 10) + require.ErrorIs(t, err, wantErr) + require.Empty(t, store.deletedIDs, "delete must not run when get fails") + }) + + t.Run("delete error propagates", func(t *testing.T) { + wantErr := errors.New("delete failed") + store := &fakeStore{ + batches: [][]*model.UserAccessToken{makeTokens(2, 1000)}, + deleteErr: wantErr, + } + + err := cleanupExpired(logger, store, nopClearSession, 9999, 1000, 10) + require.ErrorIs(t, err, wantErr) + require.Len(t, store.deletedIDs, 1, "DeleteByIds was called once before failing") + }) + + t.Run("session cache cleared for each unique user after delete", func(t *testing.T) { + sharedUserID := model.NewId() + tokens := []*model.UserAccessToken{ + {Id: model.NewId(), UserId: sharedUserID, ExpiresAt: 1000, IsActive: true}, + {Id: model.NewId(), UserId: sharedUserID, ExpiresAt: 1001, IsActive: true}, + {Id: model.NewId(), UserId: model.NewId(), ExpiresAt: 1002, IsActive: true}, + } + store := &fakeStore{batches: [][]*model.UserAccessToken{tokens}} + + cleared := map[string]int{} + err := cleanupExpired(logger, store, func(userID string) { cleared[userID]++ }, 9999, 1000, 10) + require.NoError(t, err) + + require.Len(t, cleared, 2, "cache must be cleared for each unique user") + require.Equal(t, 1, cleared[sharedUserID], "each user cleared exactly once per batch") + }) +} diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 3267d96082c..23c5c02b429 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -17843,6 +17843,48 @@ func (s *RetryLayerUserAccessTokenStore) Delete(tokenID string) error { } +func (s *RetryLayerUserAccessTokenStore) DeleteByIds(tokenIDs []string) (int64, error) { + + tries := 0 + for { + result, err := s.UserAccessTokenStore.DeleteByIds(tokenIDs) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + +func (s *RetryLayerUserAccessTokenStore) GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) { + + tries := 0 + for { + result, err := s.UserAccessTokenStore.GetExpiredBefore(cutoff, limit) + if err == nil { + return result, nil + } + if !isRepeatableError(err) { + return result, err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return result, err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerUserAccessTokenStore) DeleteAllForUser(userID string) error { tries := 0 diff --git a/server/channels/store/sqlstore/user_access_token_store.go b/server/channels/store/sqlstore/user_access_token_store.go index 7a93d389ede..ba33898c35c 100644 --- a/server/channels/store/sqlstore/user_access_token_store.go +++ b/server/channels/store/sqlstore/user_access_token_store.go @@ -32,6 +32,7 @@ func newSqlUserAccessTokenStore(sqlStore *SqlStore) store.UserAccessTokenStore { "UserAccessTokens.UserId", "UserAccessTokens.Description", "UserAccessTokens.IsActive", + "UserAccessTokens.ExpiresAt", ). From("UserAccessTokens") @@ -46,8 +47,8 @@ func (s SqlUserAccessTokenStore) Save(token *model.UserAccessToken) (*model.User } query, args, err := s.getQueryBuilder().Insert("UserAccessTokens"). - Columns("Id", "Token", "UserId", "Description", "IsActive"). - Values(token.Id, token.Token, token.UserId, token.Description, token.IsActive). + Columns("Id", "Token", "UserId", "Description", "IsActive", "ExpiresAt"). + Values(token.Id, token.Token, token.UserId, token.Description, token.IsActive, token.ExpiresAt). ToSql() if err != nil { return nil, errors.Wrap(err, "UserAccessToken_tosql") @@ -231,6 +232,103 @@ func (s SqlUserAccessTokenStore) UpdateTokenDisable(tokenId string) (err error) return nil } +// GetExpiredBefore returns active tokens whose non-zero ExpiresAt is less than +// or equal to the provided cutoff (Unix milliseconds), up to the given limit. +// The secret Token column is intentionally NOT selected — callers use the +// returned rows for metadata (audit logging, deletion) only. +// +// A non-positive limit returns an empty slice without hitting the DB rather +// than relying on the int -> uint64 cast (which would otherwise wrap a +// negative value into an enormous unsigned limit and effectively disable the +// bound). +func (s SqlUserAccessTokenStore) GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) { + tokens := []*model.UserAccessToken{} + + if limit <= 0 { + return tokens, nil + } + + query := s.getQueryBuilder(). + Select( + "UserAccessTokens.Id", + "UserAccessTokens.UserId", + "UserAccessTokens.Description", + "UserAccessTokens.IsActive", + "UserAccessTokens.ExpiresAt", + ). + From("UserAccessTokens"). + Where(sq.Gt{"UserAccessTokens.ExpiresAt": 0}). + Where(sq.LtOrEq{"UserAccessTokens.ExpiresAt": cutoff}). + Where(sq.Eq{"UserAccessTokens.IsActive": true}). + OrderBy("UserAccessTokens.ExpiresAt ASC"). + Limit(uint64(limit)) + + if err := s.GetReplica().SelectBuilder(&tokens, query); err != nil { + return nil, errors.Wrap(err, "failed to find expired UserAccessTokens") + } + + return tokens, nil +} + +// DeleteByIds deletes the tokens identified by tokenIDs along with any sessions +// minted from those tokens, all within a single transaction. It returns the +// number of UserAccessTokens rows actually deleted. +func (s SqlUserAccessTokenStore) DeleteByIds(tokenIDs []string) (deleted int64, err error) { + if len(tokenIDs) == 0 { + return 0, nil + } + + transaction, beginErr := s.GetMaster().Begin() + if beginErr != nil { + err = errors.Wrap(beginErr, "begin_transaction") + return + } + defer finalizeTransactionX(transaction, &err) + + // Delete sessions whose Token matches any of the PAT tokens via subquery. + subSQL, subArgs, sqErr := s.getQueryBuilder(). + Select("Token"). + From("UserAccessTokens"). + Where(sq.Eq{"Id": tokenIDs}). + ToSql() + if sqErr != nil { + err = errors.Wrap(sqErr, "UserAccessToken_tosql") + return + } + if _, sErr := transaction.Exec("DELETE FROM Sessions WHERE Token IN ("+subSQL+")", subArgs...); sErr != nil { + err = errors.Wrap(sErr, "failed to delete Sessions for UserAccessTokens") + return + } + + tokenSQL, tokenArgs, sqErr := s.getQueryBuilder(). + Delete("UserAccessTokens"). + Where(sq.Eq{"Id": tokenIDs}). + ToSql() + if sqErr != nil { + err = errors.Wrap(sqErr, "UserAccessToken_tosql") + return + } + res, execErr := transaction.Exec(tokenSQL, tokenArgs...) + if execErr != nil { + err = errors.Wrap(execErr, "failed to delete UserAccessTokens") + return + } + + rowCount, rErr := res.RowsAffected() + if rErr != nil { + err = errors.Wrap(rErr, "failed to read RowsAffected for UserAccessTokens delete") + return + } + + if cErr := transaction.Commit(); cErr != nil { + err = errors.Wrap(cErr, "commit_transaction") + return + } + + deleted = rowCount + return +} + func (s SqlUserAccessTokenStore) deleteSessionsAndDisableToken(transaction *sqlxTxWrapper, tokenId string) error { query := "DELETE FROM Sessions s USING UserAccessTokens o WHERE o.Token = s.Token AND o.Id = ?" diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 816d69e330c..0fda03addb7 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -844,10 +844,12 @@ type UserAccessTokenStore interface { Save(token *model.UserAccessToken) (*model.UserAccessToken, error) DeleteAllForUser(userID string) error Delete(tokenID string) error + DeleteByIds(tokenIDs []string) (int64, error) Get(tokenID string) (*model.UserAccessToken, error) GetAll(offset int, limit int) ([]*model.UserAccessToken, error) GetByToken(tokenString string) (*model.UserAccessToken, error) GetByUser(userID string, page, perPage int) ([]*model.UserAccessToken, error) + GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) Search(term string) ([]*model.UserAccessToken, error) UpdateTokenEnable(tokenID string) error UpdateTokenDisable(tokenID string) error diff --git a/server/channels/store/storetest/mocks/UserAccessTokenStore.go b/server/channels/store/storetest/mocks/UserAccessTokenStore.go index c4e23f6fc4f..62fc003a65f 100644 --- a/server/channels/store/storetest/mocks/UserAccessTokenStore.go +++ b/server/channels/store/storetest/mocks/UserAccessTokenStore.go @@ -50,6 +50,64 @@ func (_m *UserAccessTokenStore) DeleteAllForUser(userID string) error { return r0 } +// DeleteByIds provides a mock function with given fields: tokenIDs +func (_m *UserAccessTokenStore) DeleteByIds(tokenIDs []string) (int64, error) { + ret := _m.Called(tokenIDs) + + if len(ret) == 0 { + panic("no return value specified for DeleteByIds") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func([]string) (int64, error)); ok { + return rf(tokenIDs) + } + if rf, ok := ret.Get(0).(func([]string) int64); ok { + r0 = rf(tokenIDs) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func([]string) error); ok { + r1 = rf(tokenIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetExpiredBefore provides a mock function with given fields: cutoff, limit +func (_m *UserAccessTokenStore) GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) { + ret := _m.Called(cutoff, limit) + + if len(ret) == 0 { + panic("no return value specified for GetExpiredBefore") + } + + var r0 []*model.UserAccessToken + var r1 error + if rf, ok := ret.Get(0).(func(int64, int) ([]*model.UserAccessToken, error)); ok { + return rf(cutoff, limit) + } + if rf, ok := ret.Get(0).(func(int64, int) []*model.UserAccessToken); ok { + r0 = rf(cutoff, limit) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.UserAccessToken) + } + } + + if rf, ok := ret.Get(1).(func(int64, int) error); ok { + r1 = rf(cutoff, limit) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // Get provides a mock function with given fields: tokenID func (_m *UserAccessTokenStore) Get(tokenID string) (*model.UserAccessToken, error) { ret := _m.Called(tokenID) diff --git a/server/channels/store/storetest/user_access_token_store.go b/server/channels/store/storetest/user_access_token_store.go index 61eb8459378..6f972384483 100644 --- a/server/channels/store/storetest/user_access_token_store.go +++ b/server/channels/store/storetest/user_access_token_store.go @@ -18,6 +18,7 @@ func TestUserAccessTokenStore(t *testing.T, rctx request.CTX, ss store.Store) { t.Run("UserAccessTokenDisableEnable", func(t *testing.T) { testUserAccessTokenDisableEnable(t, rctx, ss) }) t.Run("UserAccessTokenSearch", func(t *testing.T) { testUserAccessTokenSearch(t, rctx, ss) }) t.Run("UserAccessTokenPagination", func(t *testing.T) { testUserAccessTokenPagination(t, rctx, ss) }) + t.Run("UserAccessTokenExpiry", func(t *testing.T) { testUserAccessTokenExpiry(t, rctx, ss) }) } func testUserAccessTokenSaveGetDelete(t *testing.T, rctx request.CTX, ss store.Store) { @@ -245,3 +246,113 @@ func testUserAccessTokenPagination(t *testing.T, rctx request.CTX, ss store.Stor require.NoError(t, nErr) require.Len(t, result, 0, "Should return 0 tokens for non-existent user") } + +func testUserAccessTokenExpiry(t *testing.T, rctx request.CTX, ss store.Store) { + now := model.GetMillis() + + // Non-expiring token (ExpiresAt == 0) + nonExpiring := &model.UserAccessToken{ + Token: model.NewId(), + UserId: model.NewId(), + Description: "non-expiring", + } + _, err := ss.UserAccessToken().Save(nonExpiring) + require.NoError(t, err) + + // Token already expired + expired := &model.UserAccessToken{ + Token: model.NewId(), + UserId: model.NewId(), + Description: "expired", + ExpiresAt: now - 60*1000, + } + expiredSession := &model.Session{UserId: expired.UserId, Token: expired.Token} + _, sErr := ss.Session().Save(rctx, expiredSession) + require.NoError(t, sErr) + _, err = ss.UserAccessToken().Save(expired) + require.NoError(t, err) + + // Token expiring in the future + future := &model.UserAccessToken{ + Token: model.NewId(), + UserId: model.NewId(), + Description: "future", + ExpiresAt: now + 60*60*1000, + } + _, err = ss.UserAccessToken().Save(future) + require.NoError(t, err) + + t.Cleanup(func() { + // Delete all three fixtures (expired included) so the test stays + // isolated even on early exit before DeleteByIds runs. + _ = ss.UserAccessToken().Delete(nonExpiring.Id) + _ = ss.UserAccessToken().Delete(future.Id) + _ = ss.UserAccessToken().Delete(expired.Id) + }) + + // The stored value should be persisted and returned + stored, err := ss.UserAccessToken().Get(expired.Id) + require.NoError(t, err) + require.Equal(t, expired.ExpiresAt, stored.ExpiresAt) + + storedNonExpiring, err := ss.UserAccessToken().Get(nonExpiring.Id) + require.NoError(t, err) + require.Equal(t, int64(0), storedNonExpiring.ExpiresAt) + + // GetExpiredBefore should only return the expired token and must not leak + // the secret token value (the Token column is intentionally not selected). + expiredRows, err := ss.UserAccessToken().GetExpiredBefore(now, 100) + require.NoError(t, err) + found := false + for _, row := range expiredRows { + // The Token column is never selected by GetExpiredBefore, so no row — + // not just the matched expired one — should ever carry the secret. + require.Empty(t, row.Token, "GetExpiredBefore must never return the secret Token value") + if row.Id == expired.Id { + require.Equal(t, expired.ExpiresAt, row.ExpiresAt) + found = true + } + require.NotEqual(t, nonExpiring.Id, row.Id, "non-expiring token must not be returned") + require.NotEqual(t, future.Id, row.Id, "future token must not be returned") + } + require.True(t, found, "expired token should be present in GetExpiredBefore results") + + // Negative or zero limits short-circuit and return an empty slice without + // hitting the DB; verify the contract holds. + zeroLimit, err := ss.UserAccessToken().GetExpiredBefore(now, 0) + require.NoError(t, err) + require.Empty(t, zeroLimit) + negativeLimit, err := ss.UserAccessToken().GetExpiredBefore(now, -5) + require.NoError(t, err) + require.Empty(t, negativeLimit) + + // DeleteByIds on the expired token removes it and its session but leaves + // the other two tokens alone. + deleted, err := ss.UserAccessToken().DeleteByIds([]string{expired.Id}) + require.NoError(t, err) + require.Equal(t, int64(1), deleted) + + _, err = ss.UserAccessToken().Get(expired.Id) + require.Error(t, err, "expired token should be deleted") + + _, err = ss.Session().Get(rctx, expiredSession.Token) + require.Error(t, err, "session for expired token should be deleted") + + stillThere, err := ss.UserAccessToken().Get(nonExpiring.Id) + require.NoError(t, err) + require.Equal(t, nonExpiring.Id, stillThere.Id) + + stillThere, err = ss.UserAccessToken().Get(future.Id) + require.NoError(t, err) + require.Equal(t, future.Id, stillThere.Id) + + // DeleteByIds with an empty slice is a no-op, and with a non-matching id + // returns 0 without error. + deleted, err = ss.UserAccessToken().DeleteByIds(nil) + require.NoError(t, err) + require.Equal(t, int64(0), deleted) + + deleted, err = ss.UserAccessToken().DeleteByIds([]string{model.NewId()}) + require.NoError(t, err) + require.Equal(t, int64(0), deleted) +} diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index db5c7249039..202bc98a7ce 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -14119,6 +14119,38 @@ func (s *TimerLayerUserAccessTokenStore) DeleteAllForUser(userID string) error { return err } +func (s *TimerLayerUserAccessTokenStore) DeleteByIds(tokenIDs []string) (int64, error) { + start := time.Now() + + result, err := s.UserAccessTokenStore.DeleteByIds(tokenIDs) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserAccessTokenStore.DeleteByIds", success, elapsed) + } + return result, err +} + +func (s *TimerLayerUserAccessTokenStore) GetExpiredBefore(cutoff int64, limit int) ([]*model.UserAccessToken, error) { + start := time.Now() + + result, err := s.UserAccessTokenStore.GetExpiredBefore(cutoff, limit) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("UserAccessTokenStore.GetExpiredBefore", success, elapsed) + } + return result, err +} + func (s *TimerLayerUserAccessTokenStore) Get(tokenID string) (*model.UserAccessToken, error) { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index dfe59e867ca..0631cff88e9 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -9522,6 +9522,10 @@ "id": "app.user_access_token.disabled", "translation": "Personal access tokens are disabled on this server. Please contact your system administrator for details." }, + { + "id": "app.user_access_token.expired", + "translation": "The personal access token has expired." + }, { "id": "app.user_access_token.get_all.app_error", "translation": "Unable to get all personal access tokens." @@ -12986,6 +12990,10 @@ "id": "model.user_access_token.is_valid.description.app_error", "translation": "Invalid description, must be 255 or less characters." }, + { + "id": "model.user_access_token.is_valid.expires_at.app_error", + "translation": "Invalid expires_at, must be zero or a positive Unix timestamp in milliseconds." + }, { "id": "model.user_access_token.is_valid.id.app_error", "translation": "Invalid value for id." diff --git a/server/public/model/audit_events.go b/server/public/model/audit_events.go index 489a4e1a3dd..16447f76cec 100644 --- a/server/public/model/audit_events.go +++ b/server/public/model/audit_events.go @@ -477,6 +477,7 @@ const ( AuditEventRevokeAllSessionsAllUsers = "revokeAllSessionsAllUsers" // revoke all active sessions for all users AuditEventRevokeAllSessionsForUser = "revokeAllSessionsForUser" // revoke all active sessions for specific user AuditEventRevokeSession = "revokeSession" // revoke specific user session + AuditEventRejectExpiredUserAccessToken = "rejectExpiredUserAccessToken" // rejected an API request because the personal access token has expired AuditEventRevokeUserAccessToken = "revokeUserAccessToken" // revoke user personal access token AuditEventSendPasswordReset = "sendPasswordReset" // send password reset email to user AuditEventSendVerificationEmail = "sendVerificationEmail" // send email verification link to user diff --git a/server/public/model/job.go b/server/public/model/job.go index 500a05e459e..77bfd7bfb1a 100644 --- a/server/public/model/job.go +++ b/server/public/model/job.go @@ -48,6 +48,7 @@ const ( JobTypeRecap = "recap" JobTypeDeleteExpiredPosts = "delete_expired_posts" JobTypeAutoTranslationRecovery = "autotranslation_recovery" + JobTypeCleanupExpiredAccessTokens = "cleanup_expired_access_tokens" JobStatusPending = "pending" JobStatusInProgress = "in_progress" @@ -78,6 +79,7 @@ var AllJobTypes = [...]string{ JobTypeLastAccessiblePost, JobTypeLastAccessibleFile, JobTypeCleanupDesktopTokens, + JobTypeCleanupExpiredAccessTokens, JobTypeRefreshMaterializedViews, JobTypeMobileSessionMetadata, } diff --git a/server/public/model/user_access_token.go b/server/public/model/user_access_token.go index dee31f1837a..9acccbfeab9 100644 --- a/server/public/model/user_access_token.go +++ b/server/public/model/user_access_token.go @@ -13,6 +13,11 @@ type UserAccessToken struct { UserId string `json:"user_id"` Description string `json:"description"` IsActive bool `json:"is_active"` + // ExpiresAt is the Unix timestamp in milliseconds at which the token + // expires. A value of 0 means the token does not expire. Tokens whose + // ExpiresAt is non-zero and in the past are considered expired and + // MUST be rejected at validation time. + ExpiresAt int64 `json:"expires_at"` } func (t *UserAccessToken) IsValid() *AppError { @@ -32,6 +37,10 @@ func (t *UserAccessToken) IsValid() *AppError { return NewAppError("UserAccessToken.IsValid", "model.user_access_token.is_valid.description.app_error", nil, "", http.StatusBadRequest) } + if t.ExpiresAt < 0 { + return NewAppError("UserAccessToken.IsValid", "model.user_access_token.is_valid.expires_at.app_error", nil, "", http.StatusBadRequest) + } + return nil } @@ -39,3 +48,13 @@ func (t *UserAccessToken) PreSave() { t.Id = NewId() t.IsActive = true } + +// IsExpired reports whether the token has a non-zero ExpiresAt in the past. +// Tokens with ExpiresAt == 0 are treated as non-expiring for backwards +// compatibility with tokens that existed before expiry was introduced. +func (t *UserAccessToken) IsExpired() bool { + if t.ExpiresAt <= 0 { + return false + } + return GetMillis() >= t.ExpiresAt +} diff --git a/server/public/model/user_access_token_test.go b/server/public/model/user_access_token_test.go index 7060430b474..762b08aa1bd 100644 --- a/server/public/model/user_access_token_test.go +++ b/server/public/model/user_access_token_test.go @@ -29,4 +29,37 @@ func TestUserAccessTokenIsValid(t *testing.T) { ad.Description = NewRandomString(256) appErr = ad.IsValid() require.False(t, appErr == nil || appErr.Id != "model.user_access_token.is_valid.description.app_error") + + ad.Description = NewRandomString(100) + ad.ExpiresAt = -1 + appErr = ad.IsValid() + require.NotNil(t, appErr) + require.Equal(t, "model.user_access_token.is_valid.expires_at.app_error", appErr.Id) + + ad.ExpiresAt = GetMillis() + 1000 + require.Nil(t, ad.IsValid()) +} + +func TestUserAccessTokenIsExpired(t *testing.T) { + now := GetMillis() + + t.Run("zero never expires", func(t *testing.T) { + tok := &UserAccessToken{ExpiresAt: 0} + require.False(t, tok.IsExpired()) + }) + + t.Run("negative never expires", func(t *testing.T) { + tok := &UserAccessToken{ExpiresAt: -1} + require.False(t, tok.IsExpired()) + }) + + t.Run("future not expired", func(t *testing.T) { + tok := &UserAccessToken{ExpiresAt: now + 60*1000} + require.False(t, tok.IsExpired()) + }) + + t.Run("past is expired", func(t *testing.T) { + tok := &UserAccessToken{ExpiresAt: now - 60*1000} + require.True(t, tok.IsExpired()) + }) }