From 209606f15b2aefd95c80c8b0db1ba7d3b9d5eba5 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Fri, 22 May 2026 10:30:30 +0200 Subject: [PATCH] MM-68419: Add expires_at to PAT data model and enforce expiry at token validation (#36243) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add expires_at to PAT data model and enforce expiry at token validation Adds an ExpiresAt field (int64 millis, 0 = never expires) to the UserAccessToken model and DB table, enforces expiry when a PAT is used to create a session, clamps the resulting session's ExpiresAt to the token's expiry so cached sessions also honor it, ships a background job (cleanup_expired_access_tokens) that periodically deletes expired tokens along with any sessions minted from them, and emits audit events for rejected and reaped expired tokens. Refs: MM-68419 Co-authored-by: Ben Schumacher * Fix govet shadow warnings in DeleteExpired Co-authored-by: Ben Schumacher * Stabilize expired PAT test by persisting an already-expired token The previous variant created a live token, used it to mint a session, then backdated the row and revoked the cached session to force a re-validation. That flow was race-prone under parallel test execution and was flagged as flaky in CI. Replace it with a direct store write that persists the PAT with ExpiresAt already in the past, so createSessionForUserAccessToken is exercised deterministically on the first HTTP call and no session cache races are possible. Co-authored-by: Ben Schumacher * Address PR review: batched cleanup, consistent filters, audit ordering - server.go: initialize s.Audit before initJobs() so the cleanup worker never captures a nil audit logger. - Replace DeleteExpired(cutoff) with DeleteByIds([]string) on UserAccessTokenStore. The worker now fetches a batch via GetExpiredBefore, emits one audit record per token, then deletes exactly that batch by id — guaranteeing 1:1 audit/delete pairing and eliminating the IsActive-filter mismatch between reads and deletes. The worker loops up to maxBatches (=1000) x batchLimit (=1000) rows per run and stops when GetExpiredBefore returns less than batchLimit or zero rows. - GetExpiredBefore now selects an explicit column set that omits the secret Token column, so the PAT secret never travels from DB to app. - DeleteByIds surfaces an error from RowsAffected instead of silently returning 0. - Remove dead job.Data initialization in the worker. - api4 test: set IsActive: true explicitly, walk the AppError chain and assert the specific Id app.user_access_token.expired so future 401 regressions are caught. Co-authored-by: Ben Schumacher * Use named returns in DeleteByIds and clean up expired fixture in test - DeleteByIds now declares (deleted int64, err error) and uses bare returns on every error path so finalizeTransactionX can append a rollback failure to the returned error via merror.Append. Previously early returns short-circuited the deferred rollback's error contribution. - Add the expired token to the test cleanup so all three fixtures are removed even on early test exit. Co-authored-by: Ben Schumacher * Guard GetExpiredBefore against non-positive limit + tighten store test - GetExpiredBefore now short-circuits when limit <= 0 and returns an empty slice without hitting the DB. This prevents the int -> uint64 cast on a negative value from wrapping into an effectively unbounded query. - Store test now asserts row.Token is empty for every row returned by GetExpiredBefore (not just the matched one) to catch any future query change that accidentally re-introduces the secret column. - Added store-level coverage for the limit=0 and limit<0 short-circuit contract. Co-authored-by: Ben Schumacher * Add session-clamping and worker tests; bump migration to 172 Address PR test-coverage analysis (issuecomment-4336010565): - api4/user_test.go: add two subtests covering session.ExpiresAt behavior — clamped to token.ExpiresAt when the PAT has a non-zero ExpiresAt, and untouched (long-lived) when the PAT has no expiry. - cleanup_expired_access_tokens/worker.go: extract the batching/audit/ error orchestration into a package-private cleanupExpired() taking small interfaces (expiredTokenStore, auditRecorder) so it can be unit-tested without spinning up a job server. - cleanup_expired_access_tokens/worker_test.go (new): seven unit tests cover happy path, empty result, full-batch -> next iteration, maxIter cap, GetExpiredBefore error propagation, DeleteByIds error propagation, and nil auditLogger guard. - Bump migration 000170_add_expiresat_to_user_access_tokens to 000172 to slot in behind the master-side 000170 (property_groups_version) and 000171 (drop_property_fields_protected_index). Co-authored-by: Ben Schumacher * Fix PAT expiry audit ordering and cleanup scheduler gate - Move IsExpired() check after EnableUserAccessTokens gate in createSessionForUserAccessToken so the AuditEventRejectExpiredUserAccessToken event only fires when PATs are active for the user, not when the feature is globally disabled. - Tie the cleanup_expired_access_tokens scheduler to EnableUserAccessTokens so the hourly job does not schedule on servers where PATs are disabled. Co-Authored-By: Claude Sonnet 4.6 * Remove per-token audit logging from expired PAT cleanup job Background system jobs do not emit audit events in this codebase — only user/admin-initiated actions do. The cleanup worker's per-token AuditEventExpireUserAccessToken records were inconsistent with that pattern (cleanup_desktop_tokens and other session jobs log nothing). Also removes the early s.Audit init in NewServer that existed solely to supply a non-nil logger to the worker. The AuditEventRejectExpiredUserAccessToken event (emitted by createSessionForUserAccessToken when a live request is rejected) is unchanged — that is an auth gate firing in response to a request and warrants auditing. Co-Authored-By: Claude Sonnet 4.6 * Replace hand-rolled IN-clause helpers with squirrel query builder Remove placeholders() and idsToArgs() from DeleteByIds — squirrel's sq.Eq{"column": slice} generates the IN clause and argument list automatically, matching the pattern used throughout the sqlstore package. Also restructures the sessions delete from a PostgreSQL-specific USING join to a portable subquery, keeping both statements expressible via the query builder. Co-Authored-By: Claude Sonnet 4.6 * Drop redundant logger.Error calls in cleanup worker SimpleWorker already logs any error returned from execute at the Error level (base_workers.go:86). The extra logger.Error calls before return were double-logging every failure. Co-Authored-By: Claude Sonnet 4.6 * Use mlog.CreateConsoleTestLogger in cleanup worker tests Replaces the hand-rolled newTestLogger helper with the established mlog.CreateConsoleTestLogger(t) pattern used by other job tests in this package (jobs_test.go, recap/worker_test.go). It wires cleanup and test-runner output automatically. Co-Authored-By: Claude Sonnet 4.6 * Consolidate cleanup worker tests into subtests Groups the six top-level TestCleanupExpiredXxx functions under a single TestCleanupExpired parent with t.Run subtests. One shared logger is created at the parent level; each subtest gets its own fakeStore. Co-Authored-By: Claude Sonnet 4.6 * Revert incidental configureAudit restructure in server.go The separation of s.Audit init from configureAudit was an unintended side effect of an earlier commit. Restore the original pattern where configureAudit is only called when s.Audit was nil at startup. Co-Authored-By: Claude Sonnet 4.6 * Remove api4 PAT expiry tests until API endpoint exists The three tests (expired token rejected, session clamped, no-expiry default) bypass the API to inject ExpiresAt via the store directly, since no API endpoint exists yet to create tokens with an expiry. They belong in the PR that adds that endpoint. The same behaviors are covered at the appropriate layer by storetest/user_access_token_store.go and model/user_access_token_test.go. Co-Authored-By: Claude Sonnet 4.6 * Prevent ExtendSessionExpiryIfNeeded from overriding PAT session expiry PAT-authenticated sessions have their ExpiresAt clamped to the token's ExpiresAt in createSessionForUserAccessToken. However, ExtendSessionExpiryIfNeeded was resetting that expiry to now+SessionLengthWebInHours on the first subsequent request, effectively bypassing PAT expiry for cached sessions. Guard the extension to skip SessionTypeUserAccessToken sessions until GetSessionLengthInMillis learns to return a length bounded by token.ExpiresAt. Co-Authored-By: Claude Sonnet 4.6 * Strip ExpiresAt in create-token handler until API officially supports it The JSON decoder populates the full accessToken struct from the request body, and only UserId and Token were being overwritten before the store call. This allowed clients to set an arbitrary expires_at (including 0 for non-expiring) through the existing endpoint, contradicting the PR description. Co-Authored-By: Claude Sonnet 4.6 * Clear session cache for affected users after DeleteByIds in cleanup worker DeleteByIds removes sessions from the DB but did not invalidate the in-memory session cache. This left stale sessions readable from cache until eviction, inconsistent with the RevokeSession path. Thread a clearSessionCache callback through MakeWorker and cleanupExpired. After each successful batch delete, call it for each unique UserId in the batch. The callback is deduplicated per batch to avoid redundant cache invalidations when a user has multiple expired tokens. Co-Authored-By: Claude Sonnet 4.6 * Add partial index on useraccesstokens.expiresat The cleanup job queries expiresat on every scheduled run (hourly). Without an index this is a full sequential scan. Add a partial index WHERE expiresat > 0 to match the query's filter, keeping the index small since most tokens have no expiry set. Mirrors the idx_sessions_expires_at pattern on the sessions table. Co-Authored-By: Claude Sonnet 4.6 * Register JobTypeCleanupExpiredAccessTokens in job permission switches Without entries in SessionHasPermissionToReadJob, SessionHasPermissionToCreateJob, and SessionHasPermissionToManageJob, the job type falls through to (false, nil), which API handlers treat as HTTP 400. This made the cleanup job invisible to System Console and unmanageable via API (list, cancel, manual trigger all 400). Add the job type to the PermissionManageJobs / PermissionReadJobs groups in all three switches, matching how other internal jobs like JobTypeMigrations are handled. Co-Authored-By: Claude Sonnet 4.6 * Teach GetSessionLengthInMillis to honor PAT ExpiresAt Replace the blunt guard in ExtendSessionExpiryIfNeeded with proper logic in GetSessionLengthInMillis: for PAT sessions with a fixed ExpiresAt, return the remaining lifetime instead of the configured web-session hours. This means newExpiry = now + (ExpiresAt - now) = ExpiresAt, so extension never pushes the session past the token's own expiry. The elapsed threshold collapses to zero for such sessions, so no spurious DB writes occur either. Non-expiring PAT sessions (ExpiresAt == 0) continue to use normal web-session extension, which is correct behavior. Co-Authored-By: Claude Sonnet 4.6 * Split expiresat index into separate non-transactional migration (000175) CREATE INDEX CONCURRENTLY cannot run inside a transaction block. The morph migration runner wraps each file in a transaction by default, causing the combined migration to fail. Split the index creation out of 000174 into a new 000175 migration file with the -- morph:nontransactional directive, following the same pattern used by 000135, 000155, 000173 and others. The 174 down migration no longer needs to drop the index since 175 owns it. Co-Authored-By: Claude Sonnet 4.6 * Update Beginx call to renamed Begin (sqlx wrapper API change on master) --------- Co-authored-by: Cursor Agent Co-authored-by: Ben Schumacher Co-authored-by: Mattermost Build Co-authored-by: Claude Sonnet 4.6 --- server/channels/api4/user.go | 4 + server/channels/app/job.go | 9 +- server/channels/app/server.go | 7 + server/channels/app/session.go | 29 +++ server/channels/db/migrations/migrations.list | 4 + ...d_expiresat_to_user_access_tokens.down.sql | 1 + ...add_expiresat_to_user_access_tokens.up.sql | 1 + ...resat_index_to_user_access_tokens.down.sql | 2 + ...piresat_index_to_user_access_tokens.up.sql | 4 + .../scheduler.go | 20 +++ .../cleanup_expired_access_tokens/worker.go | 112 ++++++++++++ .../worker_test.go | 169 ++++++++++++++++++ .../channels/store/retrylayer/retrylayer.go | 42 +++++ .../store/sqlstore/user_access_token_store.go | 102 ++++++++++- server/channels/store/store.go | 2 + .../storetest/mocks/UserAccessTokenStore.go | 58 ++++++ .../storetest/user_access_token_store.go | 111 ++++++++++++ .../channels/store/timerlayer/timerlayer.go | 32 ++++ server/i18n/en.json | 8 + server/public/model/audit_events.go | 1 + server/public/model/job.go | 2 + server/public/model/user_access_token.go | 19 ++ server/public/model/user_access_token_test.go | 33 ++++ 23 files changed, 767 insertions(+), 5 deletions(-) create mode 100644 server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.down.sql create mode 100644 server/channels/db/migrations/postgres/000187_add_expiresat_to_user_access_tokens.up.sql create mode 100644 server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.down.sql create mode 100644 server/channels/db/migrations/postgres/000188_add_expiresat_index_to_user_access_tokens.up.sql create mode 100644 server/channels/jobs/cleanup_expired_access_tokens/scheduler.go create mode 100644 server/channels/jobs/cleanup_expired_access_tokens/worker.go create mode 100644 server/channels/jobs/cleanup_expired_access_tokens/worker_test.go 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()) + }) }