mirror of
https://github.com/mattermost/mattermost.git
synced 2026-09-19 02:06:37 +08:00
209606f15b
* 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 <hanzei@users.noreply.github.com> * Fix govet shadow warnings in DeleteExpired Co-authored-by: Ben Schumacher <hanzei@users.noreply.github.com> * 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 <hanzei@users.noreply.github.com> * 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 <hanzei@users.noreply.github.com> * 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 <hanzei@users.noreply.github.com> * 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 <hanzei@users.noreply.github.com> * 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 <hanzei@users.noreply.github.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * Update Beginx call to renamed Begin (sqlx wrapper API change on master) --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Ben Schumacher <hanzei@users.noreply.github.com> Co-authored-by: Mattermost Build <build@mattermost.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>