From aa03fae744f73030357531df853f5c2eef75b852 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 4 Jun 2026 11:45:00 +0200 Subject: [PATCH 1/4] [MM-69026] Add zoom and pan to the image file preview (#36775) * [MM-69026] Add zoom and pan to the image file preview Enables the existing file-preview-modal zoom controls for image previews (previously PDF-only) and adds cursor-aware wheel zoom, drag-to-pan, keyboard shortcuts, and overflow clipping so the panned image can't escape behind the modal header. - Per-file default scale (1.0 for images, 1.75 stays for PDFs). - Translate state alongside scale; auto-snaps to the origin at default scale. - Native non-passive wheel listener so preventDefault actually fires. - Wheel step scaled by deltaY magnitude for trackpad pinch. - Keyboard: +/=, -, 0; skipped when an input/textarea/contentEditable is focused. Co-Authored-By: Claude Opus 4.7 (1M context) * Fix eslint no-mixed-operators and lines-around-comment Parenthesise mixed +/* and -/ arithmetic to satisfy eslint(no-mixed-operators) and add the blank line before the inline-input guard comment for lines-around-comment. Co-Authored-By: Claude Opus 4.7 (1M context) * Cap image zoom at 2x and harden lifecycle / drag-pan - Add ZoomSettings.MAX_SCALE_IMAGE = 2.0 and route image-path clamp sites through a new FilePreviewModal.getMaxScaleForFile. PDFs keep the original 3.0 ceiling. - Reconcile scale/translate in getDerivedStateFromProps when props.fileInfos changes so newly appearing indexes get seeded with the file's default instead of reading as undefined. - Gate drag-to-pan on currentScale > defaultScale so dragging at default scale (image fits the viewport) doesn't slide the image around in empty space. - Tighten the e2e style-match regex so it only accepts scale values strictly greater than 1. Co-Authored-By: Claude Opus 4.7 (1M context) * Reset image zoom state on same-length file swaps A websocket post update can replace an attachment at the same index without changing the array length, which previously bypassed the length-based reconciliation in getDerivedStateFromProps and let the old file's zoom/translate state apply to the new file. Track a per-index identity (file id, falling back to link) and trigger the same reconciliation when any identity differs at its index; indexes whose identity is unchanged keep their existing scale/translate. Co-Authored-By: Claude Opus 4.7 (1M context) * Add unit tests for zoom-related instance behavior and helpers Covers the gaps flagged by Tests/analysis: - Static helpers getDefaultScaleForFile, getMaxScaleForFile, and getFileIdentity (incl. namespace separation for file vs link identities). - handleKeyDown: +/= zoom in, - zoom out, 0 reset, modifier-key bailout, and the INPUT-focus guard. - handleImageMouseDown drag gate: ignored at default scale and on non-left-button mousedowns; sets isDragging when zoomed. - handleImageWheel clamping at MAX_SCALE_IMAGE (2.0) and MIN_SCALE (0.25), plus deltaY=0 short-circuit. - getDerivedStateFromProps identity reconciliation: same-length swap resets the affected index while preserving others; list growth seeds the new index with the file's default. 68 tests pass (was 50). Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../file_preview_image_spec.js | 55 +++ .../file_preview_modal.test.tsx.snap | 12 + .../__snapshots__/image_preview.test.tsx.snap | 26 +- .../file_preview_modal.scss | 1 + .../file_preview_modal.test.tsx | 341 +++++++++++++++++- .../file_preview_modal/file_preview_modal.tsx | 284 ++++++++++++++- .../file_preview_modal/image_preview.scss | 19 + .../file_preview_modal/image_preview.test.tsx | 36 ++ .../file_preview_modal/image_preview.tsx | 91 ++++- .../popover_bar/popover_bar.tsx | 10 +- webapp/channels/src/utils/constants.tsx | 2 + 11 files changed, 849 insertions(+), 28 deletions(-) diff --git a/e2e-tests/cypress/tests/integration/channels/files_and_attachments/file_preview_image_spec.js b/e2e-tests/cypress/tests/integration/channels/files_and_attachments/file_preview_image_spec.js index 2f66beeaf1b..fa461f76aa8 100644 --- a/e2e-tests/cypress/tests/integration/channels/files_and_attachments/file_preview_image_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/files_and_attachments/file_preview_image_spec.js @@ -104,6 +104,61 @@ describe('Upload Files - Image', () => { testImage(properties); }); + + it('MM-69026 - Image preview supports zoom controls and keyboard shortcut', () => { + // Use a unique filename so we can find the thumbnail unambiguously after + // earlier tests in this spec have already uploaded PNG.png/JPG.jpg/etc. + const fileName = 'mm-69026-zoom.png'; + const properties = { + filePath: 'mm_file_testing/Images/PNG.png', + fileName, + originalWidth: 400, + originalHeight: 479, + mimeType: 'image/png', + }; + + // # Post any message + cy.postMessage(fileName); + + // # Post an image in center channel + attachFile(properties); + + // # Wait until file upload is complete then submit + waitUntilUploadComplete(); + cy.uiGetPostTextBox().clear().type('{enter}'); + cy.wait(TIMEOUTS.FIVE_SEC); + + // # Open file preview + cy.uiGetFileThumbnail(fileName).click(); + + // * Verify preview modal opened + cy.uiGetFilePreviewModal().as('filePreviewModal'); + + // * Zoom controls are visible for images + cy.get('@filePreviewModal').find('.file-preview-modal__zoom-bar .modal-zoom-btn').should('have.length.at.least', 1); + + // # Click zoom-in + cy.get('@filePreviewModal').find('.modal-zoom-btn .icon-plus').click(); + + // * Image is scaled up — match only scale values strictly greater than 1 + // (e.g. scale(1.25), scale(2)), so a transform of scale(1) or no + // scale at all would fail this assertion. + cy.get('@filePreviewModal').find('[data-testid="imagePreview"]'). + should('have.attr', 'style'). + and('match', /scale\((?:1\.\d+|[2-9]\d*(?:\.\d+)?)\)/); + + // # Press '0' to reset + cy.get('body').type('0'); + + // * Image transform clears (no scale applied at default 1.0) + cy.get('@filePreviewModal').find('[data-testid="imagePreview"]').then(($img) => { + const style = $img.attr('style') || ''; + expect(style).to.not.match(/scale\([0-9.]+\)/); + }); + + // # Close modal + cy.uiCloseFilePreviewModal(); + }); }); function testImage(properties) { diff --git a/webapp/channels/src/components/file_preview_modal/__snapshots__/file_preview_modal.test.tsx.snap b/webapp/channels/src/components/file_preview_modal/__snapshots__/file_preview_modal.test.tsx.snap index 8127eba41ec..a05ad4c5ec3 100644 --- a/webapp/channels/src/components/file_preview_modal/__snapshots__/file_preview_modal.test.tsx.snap +++ b/webapp/channels/src/components/file_preview_modal/__snapshots__/file_preview_modal.test.tsx.snap @@ -130,6 +130,9 @@ exports[`components/FilePreviewModal should match snapshot, loaded 1`] = `
File Preview Modal Header
+
+ Popover Bar +
File Preview Modal Header
+
+ Popover Bar +
File Preview Modal Header
+
+ Popover Bar +
File Preview Modal Header
+
+ Popover Bar +
@@ -19,9 +20,15 @@ exports[`components/view_image/ImagePreview should match snapshot, with preview exports[`components/view_image/ImagePreview should match snapshot, with preview, cannot download 1`] = `
- + + +
`; @@ -35,6 +42,7 @@ exports[`components/view_image/ImagePreview should match snapshot, without previ alt="preview url image" class="image_preview__image" data-testid="imagePreview" + draggable="false" loading="lazy" src="/api/v4/files/file_id?download=1" /> @@ -44,8 +52,14 @@ exports[`components/view_image/ImagePreview should match snapshot, without previ exports[`components/view_image/ImagePreview should match snapshot, without preview, cannot download 1`] = `
- + + +
`; diff --git a/webapp/channels/src/components/file_preview_modal/file_preview_modal.scss b/webapp/channels/src/components/file_preview_modal/file_preview_modal.scss index b9e215aad7f..a1d95a7ec5a 100644 --- a/webapp/channels/src/components/file_preview_modal/file_preview_modal.scss +++ b/webapp/channels/src/components/file_preview_modal/file_preview_modal.scss @@ -19,6 +19,7 @@ &__content { display: flex; + overflow: hidden; // clip transformed image previews so panning can't escape behind the header height: calc(100vh - 72px); align-items: center; justify-content: center; diff --git a/webapp/channels/src/components/file_preview_modal/file_preview_modal.test.tsx b/webapp/channels/src/components/file_preview_modal/file_preview_modal.test.tsx index 785a019dbc1..f1d2ef2bc2a 100644 --- a/webapp/channels/src/components/file_preview_modal/file_preview_modal.test.tsx +++ b/webapp/channels/src/components/file_preview_modal/file_preview_modal.test.tsx @@ -3,7 +3,7 @@ import React from 'react'; -import FilePreviewModal from 'components/file_preview_modal/file_preview_modal'; +import FilePreviewModal, {computeZoomAtCursor} from 'components/file_preview_modal/file_preview_modal'; import {act, render} from 'tests/react_testing_utils'; import Constants from 'utils/constants'; @@ -401,3 +401,342 @@ describe('components/FilePreviewModal', () => { expect(container).toMatchSnapshot(); }); }); + +describe('computeZoomAtCursor', () => { + test('cursor at center yields zero translate when starting from zero', () => { + const result = computeZoomAtCursor(1, {x: 0, y: 0}, 0, 0, 2); + expect(result).toEqual({x: 0, y: 0}); + }); + + test('zooming in at an off-center cursor shifts translate opposite to the cursor', () => { + // Cursor 100px right of center, zooming 1 -> 2. The pixel under the cursor + // must remain under the cursor, so translate must move by -100px on x. + const result = computeZoomAtCursor(1, {x: 0, y: 0}, 100, 50, 2); + expect(result).toEqual({x: -100, y: -50}); + }); + + test('zooming back to the same scale is a no-op on translate', () => { + const start = {x: 30, y: -40}; + const result = computeZoomAtCursor(2, start, 75, 25, 2); + expect(result).toEqual(start); + }); + + test('preserves the pixel under the cursor across a zoom step', () => { + // The image pixel under the cursor (in local image-center coords) is + // p = (cursor - translate) / scale + // After the new scale + new translate, applying the same transform to p + // should land back at the cursor position. + const oldScale = 1.5; + const oldT = {x: 20, y: -10}; + const cursor = {x: 60, y: 80}; + const newScale = 2.25; + const px = (cursor.x - oldT.x) / oldScale; + const py = (cursor.y - oldT.y) / oldScale; + + const newT = computeZoomAtCursor(oldScale, oldT, cursor.x, cursor.y, newScale); + + expect(newT.x + (newScale * px)).toBeCloseTo(cursor.x); + expect(newT.y + (newScale * py)).toBeCloseTo(cursor.y); + }); + + test('returns the input translate when oldScale is zero (guard)', () => { + const t = {x: 5, y: 7}; + expect(computeZoomAtCursor(0, t, 10, 10, 2)).toBe(t); + }); +}); + +describe('FilePreviewModal static helpers', () => { + test('getDefaultScaleForFile returns the image default for image extensions', () => { + const png = TestHelper.getFileInfoMock({extension: 'png'}); + expect(FilePreviewModal.getDefaultScaleForFile(png)).toBe(1.0); + }); + + test('getDefaultScaleForFile returns the image default for SVG', () => { + const svg = TestHelper.getFileInfoMock({extension: 'svg'}); + expect(FilePreviewModal.getDefaultScaleForFile(svg)).toBe(1.0); + }); + + test('getDefaultScaleForFile returns the (PDF) default for other file types', () => { + const pdf = TestHelper.getFileInfoMock({extension: 'pdf'}); + expect(FilePreviewModal.getDefaultScaleForFile(pdf)).toBe(1.75); + }); + + test('getMaxScaleForFile caps images at 2.0 and other files at 3.0', () => { + expect(FilePreviewModal.getMaxScaleForFile(TestHelper.getFileInfoMock({extension: 'png'}))).toBe(2.0); + expect(FilePreviewModal.getMaxScaleForFile(TestHelper.getFileInfoMock({extension: 'svg'}))).toBe(2.0); + expect(FilePreviewModal.getMaxScaleForFile(TestHelper.getFileInfoMock({extension: 'pdf'}))).toBe(3.0); + }); + + test('getFileIdentity distinguishes FileInfo by id and LinkInfo by link', () => { + const fileInfo = TestHelper.getFileInfoMock({id: 'abc-123', extension: 'png'}); + const linkInfo = {link: 'https://example.com/x.png', extension: 'png', name: 'x.png'}; + expect(FilePreviewModal.getFileIdentity(fileInfo)).toBe('f:abc-123'); + expect(FilePreviewModal.getFileIdentity(linkInfo as any)).toBe('l:https://example.com/x.png'); + }); + + test('getFileIdentity namespaces file ids vs links so a literal collision never compares equal', () => { + const fileInfo = TestHelper.getFileInfoMock({id: 'shared-value', extension: 'png'}); + const linkInfo = {link: 'shared-value', extension: 'png', name: 'x.png'}; + expect(FilePreviewModal.getFileIdentity(fileInfo)).not.toBe(FilePreviewModal.getFileIdentity(linkInfo as any)); + }); +}); + +describe('FilePreviewModal instance behavior', () => { + const imageProps = { + fileInfos: [TestHelper.getFileInfoMock({id: 'img_1', extension: 'png'})], + startIndex: 0, + canDownloadFiles: true, + enablePublicLink: true, + isMobileView: false, + post: TestHelper.getPostMock(), + onExited: jest.fn(), + }; + + const mountModal = (props = imageProps) => { + const ref = React.createRef(); + const utils = render( + , + ); + return {ref, ...utils}; + }; + + describe('handleKeyDown', () => { + test('"+" zooms in and "-" zooms back out for an image', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}}); + }); + + act(() => { + document.dispatchEvent(new KeyboardEvent('keydown', {key: '+'})); + }); + expect(ref.current?.state.scale[0]).toBeCloseTo(1.25); + + act(() => { + document.dispatchEvent(new KeyboardEvent('keydown', {key: '-'})); + }); + expect(ref.current?.state.scale[0]).toBeCloseTo(1.0); + }); + + test('"0" resets a previously zoomed image to default scale', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 1.5}}); + }); + + act(() => { + document.dispatchEvent(new KeyboardEvent('keydown', {key: '0'})); + }); + expect(ref.current?.state.scale[0]).toBe(1.0); + }); + + test('ignores zoom keys when a Ctrl/Cmd modifier is held', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}}); + }); + + act(() => { + document.dispatchEvent(new KeyboardEvent('keydown', {key: '+', ctrlKey: true})); + document.dispatchEvent(new KeyboardEvent('keydown', {key: '+', metaKey: true})); + }); + expect(ref.current?.state.scale[0]).toBe(1.0); + }); + + test('ignores zoom keys when an input element has focus', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}}); + }); + + const input = document.createElement('input'); + document.body.appendChild(input); + input.focus(); + + act(() => { + input.dispatchEvent(new KeyboardEvent('keydown', {key: '+', bubbles: true})); + }); + expect(ref.current?.state.scale[0]).toBe(1.0); + + document.body.removeChild(input); + }); + }); + + describe('handleImageMouseDown drag gate', () => { + // The drag handler requires DOM-level event listeners to set up; we + // assert behavior via the resulting React state rather than the + // private dragState field. + test('does not start a drag when the image is at default scale', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}}); + }); + + const mockEvent = {button: 0, clientX: 0, clientY: 0, preventDefault: jest.fn()} as any; + act(() => { + ref.current?.handleImageMouseDown(mockEvent); + }); + expect(ref.current?.state.isDragging).toBe(false); + }); + + test('starts a drag when the image is zoomed past default scale', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 1.5}}); + }); + + const mockEvent = {button: 0, clientX: 0, clientY: 0, preventDefault: jest.fn()} as any; + act(() => { + ref.current?.handleImageMouseDown(mockEvent); + }); + expect(ref.current?.state.isDragging).toBe(true); + + // Cleanup: simulate mouseup so the document listeners don't leak + // between tests. + act(() => { + document.dispatchEvent(new MouseEvent('mouseup')); + }); + }); + + test('ignores non-left-button mousedowns', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 1.5}}); + }); + + const mockEvent = {button: 2, clientX: 0, clientY: 0, preventDefault: jest.fn()} as any; + act(() => { + ref.current?.handleImageMouseDown(mockEvent); + }); + expect(ref.current?.state.isDragging).toBe(false); + }); + }); + + describe('handleImageWheel scale clamping', () => { + // Use a stable rect for getBoundingClientRect across these tests so the + // cursor-offset math is predictable. + const wheelEventOn = (target: HTMLElement, deltaY: number) => { + const evt = new WheelEvent('wheel', {deltaY, clientX: 100, clientY: 100, cancelable: true}); + Object.defineProperty(evt, 'currentTarget', {value: target, configurable: true}); + return evt; + }; + + test('clamps zoom-in at MAX_SCALE_IMAGE (2.0)', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 1.9}}); + }); + const dummy = document.createElement('div'); + jest.spyOn(dummy, 'getBoundingClientRect').mockReturnValue({left: 0, top: 0, width: 200, height: 200, right: 200, bottom: 200, x: 0, y: 0, toJSON: () => ({})}); + + act(() => { + ref.current?.handleImageWheel(wheelEventOn(dummy, -100)); + + // A second zoom-in should not push past the cap. + ref.current?.handleImageWheel(wheelEventOn(dummy, -100)); + }); + expect(ref.current?.state.scale[0]).toBeLessThanOrEqual(2.0); + expect(ref.current?.state.scale[0]).toBeCloseTo(2.0); + }); + + test('clamps zoom-out at MIN_SCALE (0.25)', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 0.3}}); + }); + const dummy = document.createElement('div'); + jest.spyOn(dummy, 'getBoundingClientRect').mockReturnValue({left: 0, top: 0, width: 200, height: 200, right: 200, bottom: 200, x: 0, y: 0, toJSON: () => ({})}); + + act(() => { + ref.current?.handleImageWheel(wheelEventOn(dummy, 100)); + ref.current?.handleImageWheel(wheelEventOn(dummy, 100)); + }); + expect(ref.current?.state.scale[0]).toBeGreaterThanOrEqual(0.25); + expect(ref.current?.state.scale[0]).toBeCloseTo(0.25); + }); + + test('no-op when deltaY is exactly zero', () => { + const {ref} = mountModal(); + act(() => { + ref.current?.setState({showZoomControls: true, loaded: {0: true}, scale: {0: 1.0}}); + }); + const dummy = document.createElement('div'); + jest.spyOn(dummy, 'getBoundingClientRect').mockReturnValue({left: 0, top: 0, width: 200, height: 200, right: 200, bottom: 200, x: 0, y: 0, toJSON: () => ({})}); + + act(() => { + ref.current?.handleImageWheel(wheelEventOn(dummy, 0)); + }); + expect(ref.current?.state.scale[0]).toBe(1.0); + }); + }); + + describe('getDerivedStateFromProps identity reconciliation', () => { + test('resets scale/translate at an index where the file identity changed (same length)', () => { + const initial = { + ...imageProps, + fileInfos: [ + TestHelper.getFileInfoMock({id: 'a', extension: 'png'}), + TestHelper.getFileInfoMock({id: 'b', extension: 'png'}), + ], + }; + const {ref, rerender} = mountModal(initial); + + // Simulate the user zooming both images. + act(() => { + ref.current?.setState({ + scale: {0: 1.5, 1: 1.75}, + translate: {0: {x: 10, y: 10}, 1: {x: 20, y: 20}}, + }); + }); + + // Swap the file at index 1 for a different one; index 0 keeps its identity. + const swapped = { + ...initial, + fileInfos: [ + initial.fileInfos[0], + TestHelper.getFileInfoMock({id: 'c', extension: 'png'}), + ], + }; + rerender( + , + ); + + // Index 0 (unchanged identity) keeps its zoom; index 1 (new identity) resets. + expect(ref.current?.state.scale[0]).toBe(1.5); + expect(ref.current?.state.translate[0]).toEqual({x: 10, y: 10}); + expect(ref.current?.state.scale[1]).toBe(1.0); + expect(ref.current?.state.translate[1]).toEqual({x: 0, y: 0}); + }); + + test('seeds default scale/translate for brand-new indexes when the list grows', () => { + const {ref, rerender} = mountModal(); + act(() => { + ref.current?.setState({scale: {0: 1.5}, translate: {0: {x: 5, y: 5}}}); + }); + + const grown = { + ...imageProps, + fileInfos: [ + imageProps.fileInfos[0], + TestHelper.getFileInfoMock({id: 'new', extension: 'pdf'}), + ], + }; + rerender( + , + ); + + expect(ref.current?.state.scale[0]).toBe(1.5); + expect(ref.current?.state.scale[1]).toBe(1.75); // pdf default + expect(ref.current?.state.translate[1]).toEqual({x: 0, y: 0}); + }); + }); +}); diff --git a/webapp/channels/src/components/file_preview_modal/file_preview_modal.tsx b/webapp/channels/src/components/file_preview_modal/file_preview_modal.tsx index 6dbfe274e7c..8c20b0f4b05 100644 --- a/webapp/channels/src/components/file_preview_modal/file_preview_modal.tsx +++ b/webapp/channels/src/components/file_preview_modal/file_preview_modal.tsx @@ -62,19 +62,44 @@ export type Props = { startIndex: number; } +type Translate = {x: number; y: number}; + type State = { show: boolean; imageIndex: number; imageHeight: number | string; loaded: Record; prevFileInfosCount: number; + prevFileIds: string[]; progress: Record; showCloseBtn: boolean; showZoomControls: boolean; scale: Record; + translate: Record; + isDragging: boolean; content: string; } +// Pure helper for cursor-aware zoom math. Given current scale + translate, +// the cursor position relative to the wrapper's center, and the new scale, +// returns the new translate so the image pixel under the cursor stays fixed. +export function computeZoomAtCursor( + oldScale: number, + oldTranslate: Translate, + cursorOffsetX: number, + cursorOffsetY: number, + newScale: number, +): Translate { + if (oldScale === 0) { + return oldTranslate; + } + const ratio = newScale / oldScale; + return { + x: (cursorOffsetX * (1 - ratio)) + (oldTranslate.x * ratio), + y: (cursorOffsetY * (1 - ratio)) + (oldTranslate.y * ratio), + }; +} + export default class FilePreviewModal extends React.PureComponent { static defaultProps = { fileInfos: [], @@ -91,14 +116,53 @@ export default class FilePreviewModal extends React.PureComponent imageHeight: '100%', loaded: Utils.fillRecord(false, this.props.fileInfos.length), prevFileInfosCount: 0, + prevFileIds: this.props.fileInfos.map(FilePreviewModal.getFileIdentity), progress: Utils.fillRecord(0, this.props.fileInfos.length), showCloseBtn: false, showZoomControls: false, - scale: Utils.fillRecord(ZoomSettings.DEFAULT_SCALE, this.props.fileInfos.length), + scale: this.props.fileInfos.reduce>((acc, fileInfo, index) => { + acc[index] = FilePreviewModal.getDefaultScaleForFile(fileInfo); + return acc; + }, {}), + translate: this.props.fileInfos.reduce>((acc, _fileInfo, index) => { + acc[index] = {x: 0, y: 0}; + return acc; + }, {}), + isDragging: false, content: '', }; } + // Stable identity for a file used to detect same-length swaps in the + // fileInfos prop (a websocket post update could replace one attachment + // without changing the array length). FileInfo has an `id`, LinkInfo + // has a `link`; fall back to extension to keep this total. + static getFileIdentity(fileInfo: FileInfo | LinkInfo): string { + if (isFileInfo(fileInfo)) { + return `f:${fileInfo.id}`; + } + return `l:${fileInfo.link ?? fileInfo.extension ?? ''}`; + } + + static getDefaultScaleForFile(fileInfo: FileInfo | LinkInfo): number { + const fileType = Utils.getFileType(fileInfo.extension || ''); + if (fileType === FileTypes.IMAGE || fileType === FileTypes.SVG) { + return ZoomSettings.DEFAULT_SCALE_IMAGE; + } + return ZoomSettings.DEFAULT_SCALE; + } + + // Images get a lower zoom-in ceiling than PDFs: beyond ~2x the image + // exceeds the modal viewport in both dimensions and panning becomes + // tedious. PDFs keep the original 3.0 ceiling. + static getMaxScaleForFile(fileInfo: FileInfo | LinkInfo): number { + const fileType = Utils.getFileType(fileInfo.extension || ''); + if (fileType === FileTypes.IMAGE || fileType === FileTypes.SVG) { + return ZoomSettings.MAX_SCALE_IMAGE; + } + return ZoomSettings.MAX_SCALE; + } + handleNext = () => { let id = this.state.imageIndex + 1; if (id > this.props.fileInfos.length - 1) { @@ -123,27 +187,94 @@ export default class FilePreviewModal extends React.PureComponent } }; + // Zoom keyboard shortcuts. Bound on keydown so they fire on press (not release) + // and feel snappy when held. Modifier keys are required to be absent so we + // don't shadow text-editing shortcuts in any focused input. + handleKeyDown = (e: KeyboardEvent) => { + if (!this.state.show || !this.state.showZoomControls || e.ctrlKey || e.metaKey || e.altKey) { + return; + } + + // Don't hijack '+'/'='/'-'/'0' while the user is typing in an input. + const target = e.target as HTMLElement | null; + if (target && (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable)) { + return; + } + switch (e.key) { + case '+': + case '=': + e.preventDefault(); + this.handleZoomIn(); + break; + case '-': + e.preventDefault(); + this.handleZoomOut(); + break; + case '0': + e.preventDefault(); + this.handleZoomReset(); + break; + } + }; + componentDidMount() { document.addEventListener('keyup', this.handleKeyPress); + document.addEventListener('keydown', this.handleKeyDown); this.showImage(this.props.startIndex); } componentWillUnmount() { document.removeEventListener('keyup', this.handleKeyPress); + document.removeEventListener('keydown', this.handleKeyDown); + document.removeEventListener('mousemove', this.handleDocumentMouseMove); + document.removeEventListener('mouseup', this.handleDocumentMouseUp); + window.removeEventListener('blur', this.endDrag); } static getDerivedStateFromProps(props: Props, state: State) { const updatedState: Partial = {}; - if (props.fileInfos[state.imageIndex] && props.fileInfos[state.imageIndex].extension === FileTypes.PDF) { - updatedState.showZoomControls = true; + const currentFile = props.fileInfos[state.imageIndex]; + if (currentFile) { + const extension = currentFile.extension; + const fileType = Utils.getFileType(extension || ''); + updatedState.showZoomControls = ( + extension === FileTypes.PDF || + fileType === FileTypes.IMAGE || + fileType === FileTypes.SVG + ); } else { updatedState.showZoomControls = false; } - if (props.fileInfos.length !== state.prevFileInfosCount) { + + // Detect any change to the file list — either the length, or a + // same-index identity swap (e.g. a websocket post update replacing + // attachment N without changing array length). + const nextFileIds = props.fileInfos.map(FilePreviewModal.getFileIdentity); + const lengthChanged = props.fileInfos.length !== state.prevFileInfosCount; + const identitiesChanged = !lengthChanged && nextFileIds.some((id, i) => id !== state.prevFileIds[i]); + if (lengthChanged || identitiesChanged) { updatedState.loaded = Utils.fillRecord(false, props.fileInfos.length); updatedState.progress = Utils.fillRecord(0, props.fileInfos.length); updatedState.prevFileInfosCount = props.fileInfos.length; + updatedState.prevFileIds = nextFileIds; + + // Reconcile scale/translate per index: preserve entries whose + // file identity is unchanged; reset to the new file's default + // when the identity at that index changed (or is brand new). + const nextScale: Record = {}; + const nextTranslate: Record = {}; + for (let i = 0; i < props.fileInfos.length; i++) { + const idUnchanged = state.prevFileIds[i] === nextFileIds[i]; + nextScale[i] = idUnchanged && state.scale[i] !== undefined ? + state.scale[i] : + FilePreviewModal.getDefaultScaleForFile(props.fileInfos[i]); + nextTranslate[i] = idUnchanged && state.translate[i] !== undefined ? + state.translate[i] : + {x: 0, y: 0}; + } + updatedState.scale = nextScale; + updatedState.translate = nextTranslate; } return Object.keys(updatedState).length ? updatedState : null; } @@ -248,20 +379,32 @@ export default class FilePreviewModal extends React.PureComponent this.setState({showCloseBtn: false}); }; - setScale = (index: number, scale: number) => { + // Apply a scale update for the current image. Auto-snaps translate to the + // origin when the new scale equals the file's default (so the image is + // re-centered when fully zoomed out via the reset/zoom buttons). + setScale = (index: number, scale: number, translate?: Translate) => { + const fileInfo = this.props.fileInfos[index]; + const defaultScale = FilePreviewModal.getDefaultScaleForFile(fileInfo); this.setState((prevState) => { + const snappedTranslate = scale === defaultScale ? {x: 0, y: 0} : (translate ?? prevState.translate[index] ?? {x: 0, y: 0}); return { scale: { ...prevState.scale, [index]: scale, }, + translate: { + ...prevState.translate, + [index]: snappedTranslate, + }, }; }); }; handleZoomIn = () => { + const fileInfo = this.props.fileInfos[this.state.imageIndex]; + const maxScale = FilePreviewModal.getMaxScaleForFile(fileInfo); let newScale = this.state.scale[this.state.imageIndex]; - newScale = Math.min(newScale + ZoomSettings.SCALE_DELTA, ZoomSettings.MAX_SCALE); + newScale = Math.min(newScale + ZoomSettings.SCALE_DELTA, maxScale); this.setScale(this.state.imageIndex, newScale); }; @@ -272,7 +415,114 @@ export default class FilePreviewModal extends React.PureComponent }; handleZoomReset = () => { - this.setScale(this.state.imageIndex, ZoomSettings.DEFAULT_SCALE); + const fileInfo = this.props.fileInfos[this.state.imageIndex]; + this.setScale(this.state.imageIndex, FilePreviewModal.getDefaultScaleForFile(fileInfo)); + }; + + // Native (non-passive) wheel handler so preventDefault actually suppresses + // the page-scroll default. Bound by ImagePreview via addEventListener. + handleImageWheel = (e: WheelEvent) => { + if (!this.state.showZoomControls || e.deltaY === 0) { + return; + } + e.preventDefault(); + + const target = e.currentTarget as HTMLElement | null; + if (!target) { + return; + } + const rect = target.getBoundingClientRect(); + const cursorOffsetX = e.clientX - rect.left - (rect.width / 2); + const cursorOffsetY = e.clientY - rect.top - (rect.height / 2); + + // Trackpad pinch / smooth wheel emit many small deltaY events. Scale + // the step by deltaY magnitude (capped at 1) so trackpad zoom doesn't + // sprint past max scale in a few frames. + const direction = e.deltaY < 0 ? 1 : -1; + const stepMagnitude = Math.min(Math.abs(e.deltaY) / 100, 1) * ZoomSettings.SCALE_DELTA; + + this.setState((prev) => { + const idx = prev.imageIndex; + const fileInfo = this.props.fileInfos[idx]; + const defaultScale = FilePreviewModal.getDefaultScaleForFile(fileInfo); + const maxScale = FilePreviewModal.getMaxScaleForFile(fileInfo); + const oldScale = prev.scale[idx] ?? defaultScale; + const newScale = direction > 0 ? + Math.min(oldScale + stepMagnitude, maxScale) : + Math.max(oldScale - stepMagnitude, ZoomSettings.MIN_SCALE); + if (newScale === oldScale) { + return null; + } + const oldTranslate = prev.translate[idx] ?? {x: 0, y: 0}; + const newTranslate = newScale === defaultScale ? + {x: 0, y: 0} : + computeZoomAtCursor(oldScale, oldTranslate, cursorOffsetX, cursorOffsetY, newScale); + return { + scale: {...prev.scale, [idx]: newScale}, + translate: {...prev.translate, [idx]: newTranslate}, + }; + }); + }; + + // Drag state lives outside React state — only the resulting translate needs + // to render, not the in-flight drag metadata. + private dragState: {startX: number; startY: number; startTx: number; startTy: number; index: number} | null = null; + + handleImageMouseDown = (e: React.MouseEvent) => { + if (e.button !== 0 || !this.state.showZoomControls) { + return; + } + const idx = this.state.imageIndex; + const fileInfo = this.props.fileInfos[idx]; + const defaultScale = FilePreviewModal.getDefaultScaleForFile(fileInfo); + const currentScale = this.state.scale[idx] ?? defaultScale; + + // Only allow drag-to-pan when the image is actually zoomed in. At + // default scale the image fits the viewport, so dragging would just + // slide it around in empty space. + if (currentScale <= defaultScale) { + return; + } + const current = this.state.translate[idx] ?? {x: 0, y: 0}; + this.dragState = { + startX: e.clientX, + startY: e.clientY, + startTx: current.x, + startTy: current.y, + index: idx, + }; + document.addEventListener('mousemove', this.handleDocumentMouseMove); + document.addEventListener('mouseup', this.handleDocumentMouseUp); + window.addEventListener('blur', this.endDrag); + this.setState({isDragging: true}); + e.preventDefault(); // suppress text selection / link drag-ghost + }; + + private handleDocumentMouseMove = (e: MouseEvent) => { + if (!this.dragState) { + return; + } + const {startX, startY, startTx, startTy, index} = this.dragState; + const newTx = startTx + (e.clientX - startX); + const newTy = startTy + (e.clientY - startY); + this.setState((prev) => ({ + translate: {...prev.translate, [index]: {x: newTx, y: newTy}}, + })); + }; + + private handleDocumentMouseUp = () => { + this.endDrag(); + }; + + private endDrag = () => { + if (!this.dragState) { + return; + } + this.dragState = null; + document.removeEventListener('mousemove', this.handleDocumentMouseMove); + document.removeEventListener('mouseup', this.handleDocumentMouseUp); + window.removeEventListener('blur', this.endDrag); + this.setState({isDragging: false}); }; handleModalClose = () => { @@ -335,10 +585,30 @@ export default class FilePreviewModal extends React.PureComponent if (!isFileInfo(fileInfo) || !fileInfo.archived) { if (this.state.loaded[this.state.imageIndex]) { if (fileType === FileTypes.IMAGE || fileType === FileTypes.SVG) { + const currentScale = this.state.scale[this.state.imageIndex]; + const currentTranslate = this.state.translate[this.state.imageIndex] ?? {x: 0, y: 0}; + const defaultScale = FilePreviewModal.getDefaultScaleForFile(fileInfo); content = ( + ); + zoomBar = ( + ); } else if (fileType === FileTypes.VIDEO || fileType === FileTypes.AUDIO) { diff --git a/webapp/channels/src/components/file_preview_modal/image_preview.scss b/webapp/channels/src/components/file_preview_modal/image_preview.scss index fea64a3d5ea..c5ff143c209 100644 --- a/webapp/channels/src/components/file_preview_modal/image_preview.scss +++ b/webapp/channels/src/components/file_preview_modal/image_preview.scss @@ -1,11 +1,30 @@ .image_preview { + user-select: none; + &__image { max-height: calc(100vh - 168px); border-radius: 8px; cursor: default; + transform-origin: center center; @media screen and (max-width: 768px) { border-radius: 0; } + + // Only the zoomed state pays for the transform compositor layer; at + // rest the image renders like before. + &--zoomed { + transition: transform 0.1s ease-out; + will-change: transform; + } + } + + // The drag handler adds `image_preview--dragging` while a pan is in + // progress; suppress the transition then so the cursor stays locked to + // the image (the cursor-leaves-anchor case the previous :active trick + // didn't handle). + &--dragging &__image { + cursor: grabbing; + transition: none; } } diff --git a/webapp/channels/src/components/file_preview_modal/image_preview.test.tsx b/webapp/channels/src/components/file_preview_modal/image_preview.test.tsx index 31aac4156ea..1fc72c2a244 100644 --- a/webapp/channels/src/components/file_preview_modal/image_preview.test.tsx +++ b/webapp/channels/src/components/file_preview_modal/image_preview.test.tsx @@ -1,6 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import {fireEvent} from '@testing-library/react'; import React from 'react'; import ImagePreview from 'components/file_preview_modal/image_preview'; @@ -71,6 +72,41 @@ describe('components/view_image/ImagePreview', () => { expect(container).toMatchSnapshot(); }); + test('should apply transform when scale is provided and not 1', () => { + const props = { + ...baseProps, + scale: 2, + }; + + render(); + + expect(screen.getByTestId('imagePreview')).toHaveStyle('transform: scale(2)'); + }); + + test('should not apply transform when scale is 1', () => { + const props = { + ...baseProps, + scale: 1, + }; + + render(); + + expect(screen.getByTestId('imagePreview').getAttribute('style') || '').not.toContain('transform'); + }); + + test('should call onWheel handler when wheel event fires on image', () => { + const onWheel = jest.fn(); + const props = { + ...baseProps, + onWheel, + }; + + render(); + + fireEvent.wheel(screen.getByRole('link'), {deltaY: -100}); + expect(onWheel).toHaveBeenCalledTimes(1); + }); + test('should not download link for external file', () => { fileInfo1.link = 'https://example.com/image.png'; const props = { diff --git a/webapp/channels/src/components/file_preview_modal/image_preview.tsx b/webapp/channels/src/components/file_preview_modal/image_preview.tsx index f4a6d600590..62239fc22a7 100644 --- a/webapp/channels/src/components/file_preview_modal/image_preview.tsx +++ b/webapp/channels/src/components/file_preview_modal/image_preview.tsx @@ -1,7 +1,8 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import classNames from 'classnames'; +import React, {useEffect, useRef} from 'react'; import type {FileInfo} from '@mattermost/types/files'; @@ -15,11 +16,46 @@ import './image_preview.scss'; interface Props { fileInfo: FileInfo; canDownloadFiles: boolean; + scale?: number; + translate?: {x: number; y: number}; + isZoomed?: boolean; + isDragging?: boolean; + onWheel?: (e: WheelEvent) => void; + onMouseDown?: (e: React.MouseEvent) => void; } -export default function ImagePreview({fileInfo, canDownloadFiles}: Props) { +function buildTransform(scale?: number, translate?: {x: number; y: number}): string | undefined { + const hasScale = scale !== undefined && scale !== 1; + const hasTranslate = translate && (translate.x !== 0 || translate.y !== 0); + if (!hasScale && !hasTranslate) { + return undefined; + } + const parts: string[] = []; + if (hasTranslate) { + parts.push(`translate(${translate.x}px, ${translate.y}px)`); + } + if (hasScale) { + parts.push(`scale(${scale})`); + } + return parts.join(' '); +} + +export default function ImagePreview({fileInfo, canDownloadFiles, scale, translate, isZoomed, isDragging, onWheel, onMouseDown}: Props) { const isExternalFile = !fileInfo.id; + // React's synthetic wheel events are passive by default in modern React, so + // e.preventDefault() inside them is a no-op. Bind a native non-passive + // listener via ref so the page can't scroll while zooming the image. + const wrapperRef = useRef(null); + useEffect(() => { + const node = wrapperRef.current; + if (!node || !onWheel) { + return undefined; + } + node.addEventListener('wheel', onWheel, {passive: false}); + return () => node.removeEventListener('wheel', onWheel); + }, [onWheel]); + let fileUrl; let previewUrl; if (isExternalFile) { @@ -30,30 +66,63 @@ export default function ImagePreview({fileInfo, canDownloadFiles}: Props) { previewUrl = fileInfo.has_preview_image ? getFilePreviewUrl(fileInfo.id) : fileUrl; } + const transform = buildTransform(scale, translate); + const imgStyle: React.CSSProperties = {}; + if (transform) { + imgStyle.transform = transform; + } + if (isZoomed) { + imgStyle.cursor = 'grab'; + } + const imgClassName = classNames('image_preview__image', { + 'image_preview__image--zoomed': isZoomed, + }); + const wrapperClassName = classNames('image_preview', { + 'image_preview--dragging': isDragging, + }); + if (!canDownloadFiles) { - return ; + return ( + } + className={wrapperClassName} + onMouseDown={onMouseDown} + > + + + ); } - let conditionalSVGStyleAttribute; + const finalImgStyle: React.CSSProperties = {...imgStyle}; if (getFileType(fileInfo.extension) === FileTypes.SVG) { - conditionalSVGStyleAttribute = { - width: fileInfo.width, - height: 'auto', - }; + finalImgStyle.width = fileInfo.width; + finalImgStyle.height = 'auto'; } + const preventLinkNav = (e: React.SyntheticEvent) => e.preventDefault(); + return ( } + className={wrapperClassName} href='#' + onMouseDown={onMouseDown} + onClick={preventLinkNav} + onDragStart={preventLinkNav} > {'preview ); diff --git a/webapp/channels/src/components/file_preview_modal/popover_bar/popover_bar.tsx b/webapp/channels/src/components/file_preview_modal/popover_bar/popover_bar.tsx index 97b00625077..ec23117cf27 100644 --- a/webapp/channels/src/components/file_preview_modal/popover_bar/popover_bar.tsx +++ b/webapp/channels/src/components/file_preview_modal/popover_bar/popover_bar.tsx @@ -11,6 +11,8 @@ import {ZoomSettings} from 'utils/constants'; export interface Props { scale?: number; + defaultScale?: number; + maxScale?: number; showZoomControls?: boolean; handleZoomIn?: () => void; handleZoomOut?: () => void; @@ -19,6 +21,8 @@ export interface Props { export default class PopoverBar extends React.PureComponent { render() { + const defaultScale = this.props.defaultScale ?? ZoomSettings.DEFAULT_SCALE; + const maxScale = this.props.maxScale ?? ZoomSettings.MAX_SCALE; const zoomControls: React.ReactNode[] = []; let wrappedZoomControls: React.ReactNode = null; if (this.props.showZoomControls) { @@ -55,7 +59,7 @@ export default class PopoverBar extends React.PureComponent { , ); - if (this.props.scale && this.props.scale > ZoomSettings.DEFAULT_SCALE) { + if (this.props.scale && this.props.scale > defaultScale) { zoomResetButton = ( @@ -63,7 +67,7 @@ export default class PopoverBar extends React.PureComponent { ); - } else if (this.props.scale && this.props.scale < ZoomSettings.DEFAULT_SCALE) { + } else if (this.props.scale && this.props.scale < defaultScale) { zoomResetButton = ( @@ -92,7 +96,7 @@ export default class PopoverBar extends React.PureComponent { , ); - if (this.props.scale && this.props.scale < ZoomSettings.MAX_SCALE) { + if (this.props.scale && this.props.scale < maxScale) { zoomInButton = ( diff --git a/webapp/channels/src/utils/constants.tsx b/webapp/channels/src/utils/constants.tsx index 6769f3ad018..bc1fc917eb3 100644 --- a/webapp/channels/src/utils/constants.tsx +++ b/webapp/channels/src/utils/constants.tsx @@ -1428,9 +1428,11 @@ export const CacheTypes = { export const ZoomSettings = { DEFAULT_SCALE: 1.75, + DEFAULT_SCALE_IMAGE: 1.0, SCALE_DELTA: 0.25, MIN_SCALE: 0.25, MAX_SCALE: 3.0, + MAX_SCALE_IMAGE: 2.0, }; export const DataSpillagePropertyNames = { From 85dae1b8841cf7f64334937f5399c49e390c6b6e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 4 Jun 2026 14:21:35 +0200 Subject: [PATCH 2/4] MM-68417, MM-68420: API support for PAT expiry and admin policy settings (#36706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * MM-68417, MM-68420: API support for PAT expiry and admin policy settings POST /users/{id}/tokens now accepts a client-supplied expires_at (previously stripped per the TODO in api4/user.go), and the create app method enforces two new ServiceSettings: - EnforcePersonalAccessTokenExpiry (bool, default false): when on, rejects creates with expires_at == 0 - MaximumPersonalAccessTokenLifetimeDays (int, default 0 = unlimited): caps how far in the future expires_at may be Rejections return distinct app error ids so clients can disambiguate: expires_at_required, expires_at_in_past, expires_at_too_far. GET /users/{id}/tokens already serializes expires_at via the model's JSON tag added in MM-68419; clients derive token status (active / expired / inactive) from is_active + expires_at without a separate server-side field, keeping the response shape minimal. The Client4 helper CreateUserAccessToken gained an optional variadic expiresAt parameter (and the mmctl Client interface + mock match) rather than introducing a parallel WithExpiry method. Refs: MM-68417, MM-68420 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68417: exempt bot accounts from PAT expiry enforcement Mirrors the existing EnableUserAccessTokens bypass at session.go:453, where bot tokens are allowed even when human PATs are disabled. Bots are programmatic clients that typically need long-lived credentials, and integrations that provision them would otherwise break the moment an admin enables EnforcePersonalAccessTokenExpiry — turning a settings toggle into a footgun. The expiry policy now applies only to human users; bots can still be given a future expires_at by callers that want it, but the server won't require one. Locked in by a new TestCreateUserAccessToken/bot_tokens_are_exempt subtest that enables enforcement plus a 30-day cap, creates a bot, and asserts a non-expiring token is accepted. Refs: MM-68417 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68420: bound MaximumPersonalAccessTokenLifetimeDays in isValid Negative values silently meant "unlimited" (the runtime check is `> 0`), and very large values overflow int64 when computing `now + days*86_400_000` at token-creation time, producing a wrap-around that either rejects all reasonable expiries or accepts past timestamps as valid. Bound the setting in ServiceSettings.isValid to [0, MaxPersonalAccess TokenLifetimeDays] where the cap is 36500 (100 years) — past any realistic operational use and well clear of int64 overflow. Surfaces as a config validation error rather than a silently-broken runtime check. New TestServiceSettingsIsValid cases lock in zero, negative, upper-bound, and above-upper-bound behavior. Refs: MM-68420 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68417: reject multiple expiresAt values in Client4 helper CodeRabbit caught that the variadic CreateUserAccessToken silently used expiresAt[0] when callers passed more than one value, masking a mis-call instead of failing fast. Return an error in that case so the misuse is visible at the call site rather than producing a token with the wrong (or right-but-coincidental) expiry. Refs: MM-68417 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68417: make expiresAt a required parameter on Client4.CreateUserAccessToken Drop the variadic in favor of a regular int64 parameter. The variadic form silently dropped extra values and made a misuse undetectable at the call site (per CodeRabbit review on PR 36706); the previous fix guarded against >1 values at runtime, but a required parameter is strictly better — the compiler now refuses the misuse and every caller is forced to make a deliberate decision about expiry. Existing callers that want the old behavior pass 0 (== never expires). Refs: MM-68417 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68417: add --expires-in flag to mmctl user token generate Adds an --expires-in duration flag so operators can create expiring PATs via the CLI. Accepts the standard Go duration syntax plus a trailing 'd' for days (the common case for token lifetimes), e.g. --expires-in 90d, --expires-in 12h, --expires-in 1h30m. Empty (the default) means no expiry — matching prior behavior. Without this flag the command was unusable once an admin enables EnforcePersonalAccessTokenExpiry, since every create would fail with app.user_access_token.expires_at_required.app_error. Flag parsing now happens before the user-lookup API call so the command fails fast on invalid input. Regenerated mmctl docs reflect the new flag. Refs: MM-68417 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68417: cap --expires-in day count to prevent time.Duration overflow parseExpiresIn returns time.Duration(days) * 24 * time.Hour, which is int64 nanoseconds and overflows past ~106751 days (CodeRabbit caught this). Cap at model.MaxPersonalAccessTokenLifetimeDays (36500) so the CLI rejects values the server would reject anyway, well below the int64-overflow point. Adds two test cases (at-cap and beyond-cap) to TestParseExpiresIn. Refs: MM-68417 Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68420: collapse PAT expiry settings into a single max-lifetime setting Remove ServiceSettings.EnforcePersonalAccessTokenExpiry and fold the policy onto MaximumPersonalAccessTokenLifetimeDays: 0 means no policy (never-expiring tokens allowed, no cap), while a value > 0 requires every new token to expire within that many days. The two-setting design let an admin set a maximum but leave enforcement off, silently allowing never-expiring tokens to sidestep the cap; the only combination the boolean added (require expiry, no upper bound) has little practical value. The removed field was introduced on this branch and never released, so this is not a breaking change. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- server/channels/api4/bot_test.go | 2 +- server/channels/api4/user.go | 4 - server/channels/api4/user_test.go | 206 ++++++++++++++---- server/channels/app/session.go | 53 +++++ server/cmd/mmctl/client/client.go | 2 +- server/cmd/mmctl/commands/bot_test.go | 2 +- server/cmd/mmctl/commands/token.go | 68 +++++- server/cmd/mmctl/commands/token_test.go | 99 ++++++++- .../cmd/mmctl/docs/mmctl_token_generate.rst | 7 +- server/cmd/mmctl/mocks/client_mock.go | 8 +- server/config/client.go | 1 + server/i18n/en.json | 16 ++ server/public/model/client4.go | 8 +- server/public/model/config.go | 103 +++++---- server/public/model/config_test.go | 24 ++ 15 files changed, 493 insertions(+), 110 deletions(-) diff --git a/server/channels/api4/bot_test.go b/server/channels/api4/bot_test.go index 109f53ea241..a65eebb7785 100644 --- a/server/channels/api4/bot_test.go +++ b/server/channels/api4/bot_test.go @@ -129,7 +129,7 @@ func TestCreateBot(t *testing.T) { _, appErr = th.App.UpdateUserRoles(th.Context, bot.UserId, model.TeamUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) assert.Nil(t, appErr) - rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), bot.UserId, "test token") + rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), bot.UserId, "test token", 0) require.NoError(t, err) th.Client.AuthToken = rtoken.Token diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 5df095ff0a8..fa26c7b7fae 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -2912,10 +2912,6 @@ 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/api4/user_test.go b/server/channels/api4/user_test.go index a0f64f046f0..c85f74c880d 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -5696,7 +5696,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5708,7 +5708,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) { - rtoken, _, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + rtoken, _, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assert.Equal(t, th.BasicUser.Id, rtoken.UserId, "wrong user id") @@ -5727,7 +5727,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { - _, resp, err := client.CreateUserAccessToken(context.Background(), "notarealuserid", "test token") + _, resp, err := client.CreateUserAccessToken(context.Background(), "notarealuserid", "test token", 0) require.Error(t, err) CheckBadRequestStatus(t, resp) }) @@ -5740,7 +5740,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { - _, resp, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "") + _, resp, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "", 0) require.Error(t, err) CheckBadRequestStatus(t, resp) }) @@ -5755,7 +5755,7 @@ func TestCreateUserAccessToken(t *testing.T) { require.Nil(t, appErr) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { - _, resp, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + _, resp, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.Error(t, err) CheckNotImplementedStatus(t, resp) }) @@ -5769,7 +5769,7 @@ func TestCreateUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assert.Equal(t, th.BasicUser.Id, rtoken.UserId, "wrong user id") @@ -5787,7 +5787,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token") + _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5801,7 +5801,7 @@ func TestCreateUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserManagerRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token") + rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token", 0) require.NoError(t, err) assert.Equal(t, th.BasicUser2.Id, rtoken.UserId) @@ -5820,7 +5820,7 @@ func TestCreateUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserManagerRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.SystemAdminUser.Id, "test token") + _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.SystemAdminUser.Id, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5831,7 +5831,7 @@ func TestCreateUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assert.Equal(t, th.BasicUser.Id, rtoken.UserId) @@ -5856,7 +5856,7 @@ func TestCreateUserAccessToken(t *testing.T) { }) require.Nil(t, appErr) - _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), remoteUser.Id, "test token") + _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), remoteUser.Id, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) // remote users are not allowed to have access tokens }) @@ -5871,7 +5871,7 @@ func TestCreateUserAccessToken(t *testing.T) { session.IsOAuth = true th.App.AddSessionToCache(session) - _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + _, resp, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5907,7 +5907,7 @@ func TestCreateUserAccessToken(t *testing.T) { t.Run("without MANAGE_BOT permission", func(t *testing.T) { th.RemovePermissionFromRole(t, model.PermissionManageBots.Id, model.TeamUserRoleId) - _, resp, err = th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + _, resp, err = th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5915,7 +5915,7 @@ func TestCreateUserAccessToken(t *testing.T) { t.Run("with MANAGE_BOTS permission", func(t *testing.T) { th.AddPermissionToRole(t, model.PermissionManageBots.Id, model.TeamUserRoleId) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) assert.Equal(t, createdBot.UserId, token.UserId) assertToken(t, th, token, createdBot.UserId) @@ -5952,7 +5952,7 @@ func TestCreateUserAccessToken(t *testing.T) { }() t.Run("only having MANAGE_BOTS permission", func(t *testing.T) { - _, resp, err = th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + _, resp, err = th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.Error(t, err) CheckForbiddenStatus(t, resp) }) @@ -5960,13 +5960,131 @@ func TestCreateUserAccessToken(t *testing.T) { t.Run("with MANAGE_OTHERS_BOTS permission", func(t *testing.T) { th.AddPermissionToRole(t, model.PermissionManageOthersBots.Id, model.TeamUserRoleId) - rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + rtoken, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) assert.Equal(t, createdBot.UserId, rtoken.UserId) assertToken(t, th, rtoken, createdBot.UserId) }) }) + + t.Run("create token with a future expires_at", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) + + expiresAt := model.GetMillis() + 7*24*60*60*1000 + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", expiresAt) + require.NoError(t, err) + assert.Equal(t, expiresAt, rtoken.ExpiresAt) + assert.True(t, rtoken.IsActive) + assertToken(t, th, rtoken, th.BasicUser.Id) + }) + + t.Run("create token with expires_at in the past is rejected", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) + + _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", model.GetMillis()-1000) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + CheckErrorID(t, err, "app.user_access_token.expires_at_in_past.app_error") + }) + + t.Run("create token without expires_at is allowed when no maximum lifetime is set", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + // MaximumPersonalAccessTokenLifetimeDays defaults to 0 (no policy), so a + // never-expiring token is accepted. + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) + + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) + require.NoError(t, err) + assert.Equal(t, int64(0), rtoken.ExpiresAt) + assert.True(t, rtoken.IsActive) + }) + + t.Run("create token without expires_at is rejected when a maximum lifetime is set", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserAccessTokens = true + *cfg.ServiceSettings.MaximumPersonalAccessTokenLifetimeDays = 30 + }) + + _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + CheckErrorID(t, err, "app.user_access_token.expires_at_required.app_error") + }) + + t.Run("create token with expires_at within the maximum lifetime is allowed", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserAccessTokens = true + *cfg.ServiceSettings.MaximumPersonalAccessTokenLifetimeDays = 30 + }) + + expiresAt := model.GetMillis() + 24*60*60*1000 + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", expiresAt) + require.NoError(t, err) + assert.Equal(t, expiresAt, rtoken.ExpiresAt) + }) + + t.Run("create token beyond maximum lifetime is rejected", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserAccessTokens = true + *cfg.ServiceSettings.MaximumPersonalAccessTokenLifetimeDays = 30 + }) + + // Request expiry 60 days out, well past the 30-day cap. + expiresAt := model.GetMillis() + 60*24*60*60*1000 + _, resp, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", expiresAt) + require.Error(t, err) + CheckBadRequestStatus(t, resp) + CheckErrorID(t, err, "app.user_access_token.expires_at_too_far.app_error") + }) + + t.Run("bot tokens are exempt from expiry enforcement", func(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableUserAccessTokens = true + *cfg.ServiceSettings.EnableBotAccountCreation = true + *cfg.ServiceSettings.MaximumPersonalAccessTokenLifetimeDays = 30 + }) + + createdBot, resp, err := th.SystemAdminClient.CreateBot(context.Background(), &model.Bot{ + Username: GenerateTestUsername(), + DisplayName: "a bot", + Description: "bot", + }) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + defer func() { + appErr := th.App.PermanentDeleteBot(th.Context, createdBot.UserId) + require.Nil(t, appErr) + }() + + // Bot is allowed a non-expiring token even though a max lifetime is + // configured — bots are programmatic clients and bypass the PAT expiry + // policy, matching the existing EnableUserAccessTokens bypass. + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test bot token", 0) + require.NoError(t, err) + assert.Equal(t, int64(0), rtoken.ExpiresAt) + assert.True(t, rtoken.IsActive) + }) } func TestGetUserAccessToken(t *testing.T) { @@ -6002,7 +6120,7 @@ func TestGetUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) rtoken, _, err := th.Client.GetUserAccessToken(context.Background(), token.Id) @@ -6023,7 +6141,7 @@ func TestGetUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) rtoken, _, err := th.SystemAdminClient.GetUserAccessToken(context.Background(), token.Id) @@ -6065,7 +6183,7 @@ func TestGetUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("without MANAGE_BOTS permission", func(t *testing.T) { @@ -6118,7 +6236,7 @@ func TestGetUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("only having MANAGE_BOTS permission", func(t *testing.T) { @@ -6152,10 +6270,10 @@ func TestGetUserAccessTokensForUser(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) - _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { @@ -6178,10 +6296,10 @@ func TestGetUserAccessTokensForUser(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) - _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { @@ -6222,10 +6340,10 @@ func TestGetUserAccessTokens(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) - _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) rtokens, _, err := th.SystemAdminClient.GetUserAccessTokens(context.Background(), 1, 1) @@ -6243,10 +6361,10 @@ func TestGetUserAccessTokens(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) - _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2") + _, _, err = th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token 2", 0) require.NoError(t, err) rtokens, _, err := th.SystemAdminClient.GetUserAccessTokens(context.Background(), 0, 2) @@ -6267,7 +6385,7 @@ func TestSearchUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription) + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription, 0) require.NoError(t, err) _, resp, err := th.Client.SearchUserAccessTokens(context.Background(), &model.UserAccessTokenSearch{Term: token.Id}) @@ -6307,7 +6425,7 @@ func TestRevokeUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { - token, _, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + token, _, err := client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assertToken(t, th, token, th.BasicUser.Id) @@ -6324,7 +6442,7 @@ func TestRevokeUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token", 0) require.NoError(t, err) resp, err := th.Client.RevokeUserAccessToken(context.Background(), token.Id) @@ -6362,7 +6480,7 @@ func TestRevokeUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("without MANAGE_BOTS permission", func(t *testing.T) { @@ -6411,7 +6529,7 @@ func TestRevokeUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("only having MANAGE_BOTS permission", func(t *testing.T) { @@ -6440,7 +6558,7 @@ func TestDisableUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assertToken(t, th, token, th.BasicUser.Id) @@ -6456,7 +6574,7 @@ func TestDisableUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token", 0) require.NoError(t, err) resp, err := th.Client.DisableUserAccessToken(context.Background(), token.Id) @@ -6494,7 +6612,7 @@ func TestDisableUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("without MANAGE_BOTS permission", func(t *testing.T) { @@ -6542,7 +6660,7 @@ func TestDisableUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) t.Run("only having MANAGE_BOTS permission", func(t *testing.T) { @@ -6570,7 +6688,7 @@ func TestEnableUserAccessToken(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, "test token", 0) require.NoError(t, err) assertToken(t, th, token, th.BasicUser.Id) @@ -6591,7 +6709,7 @@ func TestEnableUserAccessToken(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableUserAccessTokens = true }) - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), th.BasicUser2.Id, "test token", 0) require.NoError(t, err) _, err = th.SystemAdminClient.DisableUserAccessToken(context.Background(), token.Id) @@ -6632,7 +6750,7 @@ func TestEnableUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.Client.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) _, err = th.Client.DisableUserAccessToken(context.Background(), token.Id) @@ -6684,7 +6802,7 @@ func TestEnableUserAccessToken(t *testing.T) { require.Nil(t, appErr) }() - token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token") + token, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), createdBot.UserId, "test token", 0) require.NoError(t, err) _, err = th.SystemAdminClient.DisableUserAccessToken(context.Background(), token.Id) @@ -6716,7 +6834,7 @@ func TestUserAccessTokenInactiveUser(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription) + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription, 0) require.NoError(t, err) th.Client.AuthToken = token.Token @@ -6742,7 +6860,7 @@ func TestUserAccessTokenDisableConfig(t *testing.T) { _, appErr := th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.SystemUserRoleId+" "+model.SystemUserAccessTokenRoleId, false) require.Nil(t, appErr) - token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription) + token, _, err := th.Client.CreateUserAccessToken(context.Background(), th.BasicUser.Id, testDescription, 0) require.NoError(t, err) oldSessionToken := th.Client.AuthToken @@ -6779,7 +6897,7 @@ func TestUserAccessTokenDisableConfigBotsExcluded(t *testing.T) { require.NoError(t, err) CheckCreatedStatus(t, resp) - rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), bot.UserId, "test token") + rtoken, _, err := th.SystemAdminClient.CreateUserAccessToken(context.Background(), bot.UserId, "test token", 0) th.Client.AuthToken = rtoken.Token require.NoError(t, err) diff --git a/server/channels/app/session.go b/server/channels/app/session.go index 0068cd3e49e..815cb364c2b 100644 --- a/server/channels/app/session.go +++ b/server/channels/app/session.go @@ -401,6 +401,49 @@ func (a *App) SetSessionExpireInHours(session *model.Session, hours int) { a.ch.srv.platform.SetSessionExpireInHours(session, hours) } +// validateUserAccessTokenExpiry checks a token's ExpiresAt against +// ServiceSettings.MaximumPersonalAccessTokenLifetimeDays. 0 means no policy: +// never-expiring tokens are allowed. A value > 0 requires the token to expire +// within that many days, so ExpiresAt == 0 or an expiry beyond the cap is +// rejected. Only newly created tokens are checked; existing tokens are not +// re-validated. +func (a *App) validateUserAccessTokenExpiry(token *model.UserAccessToken) *model.AppError { + cfg := a.Config().ServiceSettings + + maxDays := int64(0) + if cfg.MaximumPersonalAccessTokenLifetimeDays != nil { + maxDays = int64(*cfg.MaximumPersonalAccessTokenLifetimeDays) + } + + if token.ExpiresAt == 0 { + // A configured maximum lifetime implies tokens must expire; never-expiring + // tokens are only allowed when no maximum is set. + if maxDays > 0 { + return model.NewAppError("CreateUserAccessToken", "app.user_access_token.expires_at_required.app_error", nil, "", http.StatusBadRequest) + } + return nil + } + + if token.ExpiresAt <= model.GetMillis() { + return model.NewAppError("CreateUserAccessToken", "app.user_access_token.expires_at_in_past.app_error", nil, "", http.StatusBadRequest) + } + + if maxDays > 0 { + maxExpiry := model.GetMillis() + maxDays*24*60*60*1000 + if token.ExpiresAt > maxExpiry { + return model.NewAppError( + "CreateUserAccessToken", + "app.user_access_token.expires_at_too_far.app_error", + map[string]any{"Days": maxDays}, + "", + http.StatusBadRequest, + ) + } + } + + return nil +} + func (a *App) CreateUserAccessToken(rctx request.CTX, token *model.UserAccessToken) (*model.UserAccessToken, *model.AppError) { user, nErr := a.ch.srv.userService.GetUser(token.UserId) if nErr != nil { @@ -417,6 +460,16 @@ func (a *App) CreateUserAccessToken(rctx request.CTX, token *model.UserAccessTok return nil, model.NewAppError("CreateUserAccessToken", "app.user_access_token.disabled", nil, "", http.StatusNotImplemented) } + // Bot accounts are exempt from the PAT expiry policy, matching the existing + // EnableUserAccessTokens bypass above: bots are programmatic clients that + // typically need long-lived credentials, and integrations that provision + // them would otherwise break the moment an admin enables enforcement. + if !user.IsBot { + if err := a.validateUserAccessTokenExpiry(token); err != nil { + return nil, err + } + } + token.Token = model.NewId() token, nErr = a.Srv().Store().UserAccessToken().Save(token) diff --git a/server/cmd/mmctl/client/client.go b/server/cmd/mmctl/client/client.go index 4f157884068..1b8df0bea34 100644 --- a/server/cmd/mmctl/client/client.go +++ b/server/cmd/mmctl/client/client.go @@ -82,7 +82,7 @@ type Client interface { UpdateUserMfa(ctx context.Context, userID, code string, activate bool) (*model.Response, error) UpdateUserPassword(ctx context.Context, userID, currentPassword, newPassword string) (*model.Response, error) UpdateUserHashedPassword(ctx context.Context, userID, newHashedPassword string) (*model.Response, error) - CreateUserAccessToken(ctx context.Context, userID, description string) (*model.UserAccessToken, *model.Response, error) + CreateUserAccessToken(ctx context.Context, userID, description string, expiresAt int64) (*model.UserAccessToken, *model.Response, error) RevokeUserAccessToken(ctx context.Context, tokenID string) (*model.Response, error) GetUserAccessTokensForUser(ctx context.Context, userID string, page, perPage int) ([]*model.UserAccessToken, *model.Response, error) ConvertUserToBot(ctx context.Context, userID string) (*model.Bot, *model.Response, error) diff --git a/server/cmd/mmctl/commands/bot_test.go b/server/cmd/mmctl/commands/bot_test.go index 14bb4fb56e4..4eeb086e9be 100644 --- a/server/cmd/mmctl/commands/bot_test.go +++ b/server/cmd/mmctl/commands/bot_test.go @@ -64,7 +64,7 @@ func (s *MmctlUnitTestSuite) TestBotCreateCmd() { s.client. EXPECT(). - CreateUserAccessToken(context.TODO(), mockBot.UserId, "autogenerated"). + CreateUserAccessToken(context.TODO(), mockBot.UserId, "autogenerated", int64(0)). Return(&mockToken, &model.Response{}, nil). Times(1) diff --git a/server/cmd/mmctl/commands/token.go b/server/cmd/mmctl/commands/token.go index 8f80c62fb9a..b8d4c6adfe4 100644 --- a/server/cmd/mmctl/commands/token.go +++ b/server/cmd/mmctl/commands/token.go @@ -6,7 +6,11 @@ package commands import ( "context" "net/http" + "strconv" + "strings" + "time" + "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/v8/cmd/mmctl/client" "github.com/mattermost/mattermost/server/v8/cmd/mmctl/printer" @@ -20,12 +24,15 @@ var TokenCmd = &cobra.Command{ } var GenerateUserTokenCmd = &cobra.Command{ - Use: "generate [user] [description]", - Short: "Generate token for a user", - Long: "Generate token for a user", - Example: " generate testuser test-token", - RunE: withClient(generateTokenForAUserCmdF), - Args: cobra.ExactArgs(2), + Use: "generate [user] [description]", + Short: "Generate token for a user", + Long: "Generate token for a user. Use --expires-in to set an expiry, which may be required by " + + "the server's MaximumPersonalAccessTokenLifetimeDays setting.", + Example: ` generate testuser test-token + generate testuser ci-token --expires-in 90d + generate testuser short-lived --expires-in 12h`, + RunE: withClient(generateTokenForAUserCmdF), + Args: cobra.ExactArgs(2), } var RevokeUserTokenCmd = &cobra.Command{ @@ -47,6 +54,8 @@ var ListUserTokensCmd = &cobra.Command{ } func init() { + GenerateUserTokenCmd.Flags().String("expires-in", "", "Duration after which the token expires (e.g. 90d, 12h, 30m). Accepts the standard Go duration syntax plus a 'd' (days) suffix. If empty, the token does not expire.") + ListUserTokensCmd.Flags().Int("page", 0, "Page number to fetch for the list of users") ListUserTokensCmd.Flags().Int("per-page", DefaultPageSize, "Number of users to be fetched") ListUserTokensCmd.Flags().Bool("all", false, "Fetch all tokens. --page flag will be ignore if provided") @@ -65,13 +74,18 @@ func init() { } func generateTokenForAUserCmdF(c client.Client, command *cobra.Command, args []string) error { + expiresAt, err := resolveTokenExpiry(command) + if err != nil { + return err + } + userArg := args[0] user := getUserFromUserArg(c, userArg) if user == nil { return errors.Errorf("could not retrieve user information of %q", userArg) } - token, _, err := c.CreateUserAccessToken(context.TODO(), user.Id, args[1]) + token, _, err := c.CreateUserAccessToken(context.TODO(), user.Id, args[1], expiresAt) if err != nil { return errors.Errorf("could not create token for %q: %s", userArg, err.Error()) } @@ -80,6 +94,46 @@ func generateTokenForAUserCmdF(c client.Client, command *cobra.Command, args []s return nil } +// resolveTokenExpiry converts the --expires-in flag into a Unix-millis timestamp +// suitable for UserAccessToken.ExpiresAt. Returns 0 when the flag is empty, +// meaning the token does not expire (subject to server policy). +func resolveTokenExpiry(command *cobra.Command) (int64, error) { + raw, _ := command.Flags().GetString("expires-in") + raw = strings.TrimSpace(raw) + if raw == "" { + return 0, nil + } + d, err := parseExpiresIn(raw) + if err != nil { + return 0, errors.Wrap(err, "invalid --expires-in value") + } + if d <= 0 { + return 0, errors.Errorf("--expires-in must be positive, got %q", raw) + } + return time.Now().Add(d).UnixMilli(), nil +} + +// parseExpiresIn accepts a duration string. In addition to the standard Go +// duration syntax (e.g. "12h", "30m"), a trailing "d" is interpreted as days +// — the common case for token lifetimes which stdlib's time.ParseDuration +// does not support. +func parseExpiresIn(s string) (time.Duration, error) { + if prefix, ok := strings.CutSuffix(s, "d"); ok { + days, err := strconv.Atoi(prefix) + if err != nil { + return 0, errors.Errorf("%q is not a valid day count", s) + } + // time.Duration is int64 nanoseconds; days * 24h overflows past ~106751. + // Cap at the server-side bound so the CLI rejects values that the server + // would reject anyway, well below the int64-overflow point. + if days > model.MaxPersonalAccessTokenLifetimeDays { + return 0, errors.Errorf("%q exceeds maximum supported day count of %d", s, model.MaxPersonalAccessTokenLifetimeDays) + } + return time.Duration(days) * 24 * time.Hour, nil + } + return time.ParseDuration(s) +} + func listTokensOfAUserCmdF(c client.Client, command *cobra.Command, args []string) error { page, _ := command.Flags().GetInt("page") perPage, _ := command.Flags().GetInt("per-page") diff --git a/server/cmd/mmctl/commands/token_test.go b/server/cmd/mmctl/commands/token_test.go index 1778ae60f8d..8cff5ae6730 100644 --- a/server/cmd/mmctl/commands/token_test.go +++ b/server/cmd/mmctl/commands/token_test.go @@ -7,15 +7,48 @@ import ( "context" "fmt" "net/http" + "testing" + "time" + gomock "github.com/golang/mock/gomock" "github.com/mattermost/mattermost/server/public/model" "github.com/pkg/errors" + "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/v8/cmd/mmctl/printer" "github.com/spf13/cobra" ) +func TestParseExpiresIn(t *testing.T) { + for name, tc := range map[string]struct { + input string + want time.Duration + wantErr bool + }{ + "days": {input: "30d", want: 30 * 24 * time.Hour}, + "single day": {input: "1d", want: 24 * time.Hour}, + "hours": {input: "12h", want: 12 * time.Hour}, + "compound stdlib": {input: "1h30m", want: 90 * time.Minute}, + "minutes": {input: "45m", want: 45 * time.Minute}, + "non-numeric days": {input: "xd", wantErr: true}, + "non-numeric stdlib": {input: "abc", wantErr: true}, + "empty": {input: "", wantErr: true}, + "days beyond cap": {input: "36501d", wantErr: true}, + "days at cap accepted": {input: "36500d", want: 36500 * 24 * time.Hour}, + } { + t.Run(name, func(t *testing.T) { + got, err := parseExpiresIn(tc.input) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.want, got) + }) + } +} + func (s *MmctlUnitTestSuite) TestGenerateTokenForAUserCmd() { s.Run("Should generate a token for a user", func() { printer.Clean() @@ -38,7 +71,7 @@ func (s *MmctlUnitTestSuite) TestGenerateTokenForAUserCmd() { s.client. EXPECT(). - CreateUserAccessToken(context.TODO(), mockUser.Id, mockToken.Description). + CreateUserAccessToken(context.TODO(), mockUser.Id, mockToken.Description, int64(0)). Return(&mockToken, &model.Response{}, nil). Times(1) @@ -83,7 +116,7 @@ func (s *MmctlUnitTestSuite) TestGenerateTokenForAUserCmd() { s.client. EXPECT(). - CreateUserAccessToken(context.TODO(), mockUser.Id, "description"). + CreateUserAccessToken(context.TODO(), mockUser.Id, "description", int64(0)). Return(nil, &model.Response{}, errors.New("error-message")). Times(1) @@ -91,6 +124,68 @@ func (s *MmctlUnitTestSuite) TestGenerateTokenForAUserCmd() { s.Require().NotNil(err) s.Require().Contains(err.Error(), fmt.Sprintf("could not create token for %q:", "user1")) }) + + s.Run("Should pass a positive expires_at when --expires-in is set", func() { + printer.Clean() + + userArg := "userId1" + mockUser := model.User{Id: "userId1", Email: "user1@example.com", Username: "user1"} + mockToken := model.UserAccessToken{Token: "token-id", Description: "ci-token"} + + s.client. + EXPECT(). + GetUserByUsername(context.TODO(), userArg, ""). + Return(nil, &model.Response{}, errors.New("no user found with the given username")). + Times(1) + s.client. + EXPECT(). + GetUser(context.TODO(), userArg, ""). + Return(&mockUser, &model.Response{}, nil). + Times(1) + + now := time.Now() + s.client. + EXPECT(). + CreateUserAccessToken(context.TODO(), mockUser.Id, mockToken.Description, gomock.AssignableToTypeOf(int64(0))). + DoAndReturn(func(_ context.Context, _, _ string, expiresAt int64) (*model.UserAccessToken, *model.Response, error) { + // 90 days = 7_776_000_000 ms; allow generous slack for clock between Now() calls. + expected := now.Add(90 * 24 * time.Hour).UnixMilli() + s.Require().InDelta(expected, expiresAt, float64(time.Minute.Milliseconds())) + return &mockToken, &model.Response{}, nil + }). + Times(1) + + cmd := &cobra.Command{} + cmd.Flags().String("expires-in", "", "") + s.Require().NoError(cmd.Flags().Set("expires-in", "90d")) + + err := generateTokenForAUserCmdF(s.client, cmd, []string{mockUser.Id, mockToken.Description}) + s.Require().NoError(err) + }) + + s.Run("Should reject invalid --expires-in", func() { + printer.Clean() + + cmd := &cobra.Command{} + cmd.Flags().String("expires-in", "", "") + s.Require().NoError(cmd.Flags().Set("expires-in", "not-a-duration")) + + err := generateTokenForAUserCmdF(s.client, cmd, []string{"userId1", "desc"}) + s.Require().Error(err) + s.Require().Contains(err.Error(), "invalid --expires-in") + }) + + s.Run("Should reject non-positive --expires-in", func() { + printer.Clean() + + cmd := &cobra.Command{} + cmd.Flags().String("expires-in", "", "") + s.Require().NoError(cmd.Flags().Set("expires-in", "-5h")) + + err := generateTokenForAUserCmdF(s.client, cmd, []string{"userId1", "desc"}) + s.Require().Error(err) + s.Require().Contains(err.Error(), "must be positive") + }) } func (s *MmctlUnitTestSuite) TestListTokensOfAUserCmdF() { diff --git a/server/cmd/mmctl/docs/mmctl_token_generate.rst b/server/cmd/mmctl/docs/mmctl_token_generate.rst index cb31a4292b8..1224a11132e 100644 --- a/server/cmd/mmctl/docs/mmctl_token_generate.rst +++ b/server/cmd/mmctl/docs/mmctl_token_generate.rst @@ -9,7 +9,7 @@ Synopsis ~~~~~~~~ -Generate token for a user +Generate token for a user. Use --expires-in to set an expiry, which may be required by the server's MaximumPersonalAccessTokenLifetimeDays setting. :: @@ -21,13 +21,16 @@ Examples :: generate testuser test-token + generate testuser ci-token --expires-in 90d + generate testuser short-lived --expires-in 12h Options ~~~~~~~ :: - -h, --help help for generate + --expires-in string Duration after which the token expires (e.g. 90d, 12h, 30m). Accepts the standard Go duration syntax plus a 'd' (days) suffix. If empty, the token does not expire. + -h, --help help for generate Options inherited from parent commands ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/server/cmd/mmctl/mocks/client_mock.go b/server/cmd/mmctl/mocks/client_mock.go index de9182a593c..4b0f4dcb50e 100644 --- a/server/cmd/mmctl/mocks/client_mock.go +++ b/server/cmd/mmctl/mocks/client_mock.go @@ -344,9 +344,9 @@ func (mr *MockClientMockRecorder) CreateUser(arg0, arg1 interface{}) *gomock.Cal } // CreateUserAccessToken mocks base method. -func (m *MockClient) CreateUserAccessToken(arg0 context.Context, arg1, arg2 string) (*model.UserAccessToken, *model.Response, error) { +func (m *MockClient) CreateUserAccessToken(arg0 context.Context, arg1, arg2 string, arg3 int64) (*model.UserAccessToken, *model.Response, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateUserAccessToken", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "CreateUserAccessToken", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*model.UserAccessToken) ret1, _ := ret[1].(*model.Response) ret2, _ := ret[2].(error) @@ -354,9 +354,9 @@ func (m *MockClient) CreateUserAccessToken(arg0 context.Context, arg1, arg2 stri } // CreateUserAccessToken indicates an expected call of CreateUserAccessToken. -func (mr *MockClientMockRecorder) CreateUserAccessToken(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockClientMockRecorder) CreateUserAccessToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateUserAccessToken", reflect.TypeOf((*MockClient)(nil).CreateUserAccessToken), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateUserAccessToken", reflect.TypeOf((*MockClient)(nil).CreateUserAccessToken), arg0, arg1, arg2, arg3) } // DeleteCPAField mocks base method. diff --git a/server/config/client.go b/server/config/client.go index 5b255f1af97..d3027ec7c1b 100644 --- a/server/config/client.go +++ b/server/config/client.go @@ -37,6 +37,7 @@ func GenerateClientConfig(c *model.Config, telemetryID string, license *model.Li props["EnablePostUsernameOverride"] = strconv.FormatBool(*c.ServiceSettings.EnablePostUsernameOverride) props["EnablePostIconOverride"] = strconv.FormatBool(*c.ServiceSettings.EnablePostIconOverride) props["EnableUserAccessTokens"] = strconv.FormatBool(*c.ServiceSettings.EnableUserAccessTokens) + props["MaximumPersonalAccessTokenLifetimeDays"] = strconv.Itoa(*c.ServiceSettings.MaximumPersonalAccessTokenLifetimeDays) props["EnableLinkPreviews"] = strconv.FormatBool(*c.ServiceSettings.EnableLinkPreviews) props["EnablePermalinkPreviews"] = strconv.FormatBool(*c.ServiceSettings.EnablePermalinkPreviews) props["EnableTesting"] = strconv.FormatBool(*c.ServiceSettings.EnableTesting) diff --git a/server/i18n/en.json b/server/i18n/en.json index 32d54ef9d7b..e04562bfa22 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -9550,6 +9550,18 @@ "id": "app.user_access_token.expired", "translation": "The personal access token has expired." }, + { + "id": "app.user_access_token.expires_at_in_past.app_error", + "translation": "The provided expires_at is in the past." + }, + { + "id": "app.user_access_token.expires_at_required.app_error", + "translation": "Personal access tokens must include an expiry date because the server enforces a maximum token lifetime." + }, + { + "id": "app.user_access_token.expires_at_too_far.app_error", + "translation": "The provided expires_at exceeds the configured maximum personal access token lifetime ({{.Days}} days)." + }, { "id": "app.user_access_token.get_all.app_error", "translation": "Unable to get all personal access tokens." @@ -11774,6 +11786,10 @@ "id": "model.config.is_valid.max_payload_size.app_error", "translation": "Invalid max payload size for service settings. Must be a whole number greater than zero." }, + { + "id": "model.config.is_valid.max_personal_access_token_lifetime_days.app_error", + "translation": "Invalid MaximumPersonalAccessTokenLifetimeDays. Must be between 0 (unlimited) and {{.Max}} days." + }, { "id": "model.config.is_valid.max_url_length.app_error", "translation": "Invalid max URL length for service settings. Must be a whole number greater than zero." diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 38371edda01..fee1be13840 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -1891,8 +1891,12 @@ func (c *Client4) SetProfileImage(ctx context.Context, userId string, data []byt // of a session token to access the REST API. Must have the 'create_user_access_token' // permission and if generating for another user, must have the 'edit_other_users' // permission. A non-blank description is required. -func (c *Client4) CreateUserAccessToken(ctx context.Context, userId, description string) (*UserAccessToken, *Response, error) { - requestBody := map[string]string{"description": description} +// +// expiresAt is the Unix-millis expiry for the token; 0 means the token does not +// expire, subject to server policy (ServiceSettings.MaximumPersonalAccessTokenLifetimeDays: +// a value > 0 requires tokens to expire within that many days and rejects 0). +func (c *Client4) CreateUserAccessToken(ctx context.Context, userId, description string, expiresAt int64) (*UserAccessToken, *Response, error) { + requestBody := &UserAccessToken{Description: description, ExpiresAt: expiresAt} r, err := c.doAPIPostJSON(ctx, c.userRoute(userId).Join("tokens"), requestBody) if err != nil { return nil, BuildResponse(r), err diff --git a/server/public/model/config.go b/server/public/model/config.go index 38721d7e1bb..0bc6e4b09ea 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -319,6 +319,12 @@ const ( StorageClassGlacierIR = "GLACIER_IR" StorageClassSnow = "SNOW" StorageClassExpressOnezone = "EXPRESS_ONEZONE" + + // MaxPersonalAccessTokenLifetimeDays is the upper bound accepted for + // ServiceSettings.MaximumPersonalAccessTokenLifetimeDays. 100 years is well + // past any realistic operational use and leaves ample headroom against + // int64 overflow when computing token expiry millis. + MaxPersonalAccessTokenLifetimeDays = 36500 ) func GetDefaultAppCustomURLSchemes() []string { @@ -361,48 +367,49 @@ type ServiceSettings struct { TLSMinVer *string `access:"write_restrictable,cloud_restrictable"` // telemetry: none TLSStrictTransport *bool `access:"write_restrictable,cloud_restrictable"` // In seconds. - TLSStrictTransportMaxAge *int64 `access:"write_restrictable,cloud_restrictable"` // telemetry: none - TLSOverwriteCiphers []string `access:"write_restrictable,cloud_restrictable"` // telemetry: none - UseLetsEncrypt *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` - LetsEncryptCertificateCacheFile *string `access:"environment_web_server,write_restrictable,cloud_restrictable"` // telemetry: none - Forward80To443 *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` - TrustedProxyIPHeader []string `access:"write_restrictable,cloud_restrictable"` // telemetry: none - ReadTimeout *int `access:"environment_web_server,write_restrictable,cloud_restrictable"` - WriteTimeout *int `access:"environment_web_server,write_restrictable,cloud_restrictable"` - IdleTimeout *int `access:"write_restrictable,cloud_restrictable"` - MaximumLoginAttempts *int `access:"authentication_password,write_restrictable,cloud_restrictable"` - GoroutineHealthThreshold *int `access:"write_restrictable,cloud_restrictable"` // telemetry: none - EnableOAuthServiceProvider *bool `access:"integrations_integration_management"` - EnableDynamicClientRegistration *bool `access:"integrations_integration_management"` - DCRRedirectURIAllowlist []string `access:"integrations_integration_management"` - EnableIncomingWebhooks *bool `access:"integrations_integration_management"` - EnableOutgoingWebhooks *bool `access:"integrations_integration_management"` - EnableOutgoingOAuthConnections *bool `access:"integrations_integration_management"` - EnableCommands *bool `access:"integrations_integration_management"` - OutgoingIntegrationRequestsTimeout *int64 `access:"integrations_integration_management"` // In seconds. - EnablePostUsernameOverride *bool `access:"integrations_integration_management"` - EnablePostIconOverride *bool `access:"integrations_integration_management"` - GoogleDeveloperKey *string `access:"site_posts,write_restrictable,cloud_restrictable"` - EnableLinkPreviews *bool `access:"site_posts"` - EnablePermalinkPreviews *bool `access:"site_posts"` - RestrictLinkPreviews *string `access:"site_posts"` - EnableTesting *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` - EnableDeveloper *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` - DeveloperFlags *string `access:"environment_developer,cloud_restrictable"` - EnableClientPerformanceDebugging *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` - EnableSecurityFixAlert *bool `access:"environment_smtp,write_restrictable,cloud_restrictable"` - EnableInsecureOutgoingConnections *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` - AllowedUntrustedInternalConnections *string `access:"environment_web_server,write_restrictable,cloud_restrictable"` - EnableMultifactorAuthentication *bool `access:"authentication_mfa"` - EnforceMultifactorAuthentication *bool `access:"authentication_mfa"` - EnableUserAccessTokens *bool `access:"integrations_integration_management"` - AllowCorsFrom *string `access:"integrations_cors,write_restrictable,cloud_restrictable"` - CorsExposedHeaders *string `access:"integrations_cors,write_restrictable,cloud_restrictable"` - CorsAllowCredentials *bool `access:"integrations_cors,write_restrictable,cloud_restrictable"` - CorsDebug *bool `access:"integrations_cors,write_restrictable,cloud_restrictable"` - AllowCookiesForSubdomains *bool `access:"write_restrictable,cloud_restrictable"` - ExtendSessionLengthWithActivity *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` - TerminateSessionsOnPasswordChange *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` + TLSStrictTransportMaxAge *int64 `access:"write_restrictable,cloud_restrictable"` // telemetry: none + TLSOverwriteCiphers []string `access:"write_restrictable,cloud_restrictable"` // telemetry: none + UseLetsEncrypt *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` + LetsEncryptCertificateCacheFile *string `access:"environment_web_server,write_restrictable,cloud_restrictable"` // telemetry: none + Forward80To443 *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` + TrustedProxyIPHeader []string `access:"write_restrictable,cloud_restrictable"` // telemetry: none + ReadTimeout *int `access:"environment_web_server,write_restrictable,cloud_restrictable"` + WriteTimeout *int `access:"environment_web_server,write_restrictable,cloud_restrictable"` + IdleTimeout *int `access:"write_restrictable,cloud_restrictable"` + MaximumLoginAttempts *int `access:"authentication_password,write_restrictable,cloud_restrictable"` + GoroutineHealthThreshold *int `access:"write_restrictable,cloud_restrictable"` // telemetry: none + EnableOAuthServiceProvider *bool `access:"integrations_integration_management"` + EnableDynamicClientRegistration *bool `access:"integrations_integration_management"` + DCRRedirectURIAllowlist []string `access:"integrations_integration_management"` + EnableIncomingWebhooks *bool `access:"integrations_integration_management"` + EnableOutgoingWebhooks *bool `access:"integrations_integration_management"` + EnableOutgoingOAuthConnections *bool `access:"integrations_integration_management"` + EnableCommands *bool `access:"integrations_integration_management"` + OutgoingIntegrationRequestsTimeout *int64 `access:"integrations_integration_management"` // In seconds. + EnablePostUsernameOverride *bool `access:"integrations_integration_management"` + EnablePostIconOverride *bool `access:"integrations_integration_management"` + GoogleDeveloperKey *string `access:"site_posts,write_restrictable,cloud_restrictable"` + EnableLinkPreviews *bool `access:"site_posts"` + EnablePermalinkPreviews *bool `access:"site_posts"` + RestrictLinkPreviews *string `access:"site_posts"` + EnableTesting *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` + EnableDeveloper *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` + DeveloperFlags *string `access:"environment_developer,cloud_restrictable"` + EnableClientPerformanceDebugging *bool `access:"environment_developer,write_restrictable,cloud_restrictable"` + EnableSecurityFixAlert *bool `access:"environment_smtp,write_restrictable,cloud_restrictable"` + EnableInsecureOutgoingConnections *bool `access:"environment_web_server,write_restrictable,cloud_restrictable"` + AllowedUntrustedInternalConnections *string `access:"environment_web_server,write_restrictable,cloud_restrictable"` + EnableMultifactorAuthentication *bool `access:"authentication_mfa"` + EnforceMultifactorAuthentication *bool `access:"authentication_mfa"` + EnableUserAccessTokens *bool `access:"integrations_integration_management"` + MaximumPersonalAccessTokenLifetimeDays *int `access:"integrations_integration_management"` + AllowCorsFrom *string `access:"integrations_cors,write_restrictable,cloud_restrictable"` + CorsExposedHeaders *string `access:"integrations_cors,write_restrictable,cloud_restrictable"` + CorsAllowCredentials *bool `access:"integrations_cors,write_restrictable,cloud_restrictable"` + CorsDebug *bool `access:"integrations_cors,write_restrictable,cloud_restrictable"` + AllowCookiesForSubdomains *bool `access:"write_restrictable,cloud_restrictable"` + ExtendSessionLengthWithActivity *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` + TerminateSessionsOnPasswordChange *bool `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` // Deprecated SessionLengthWebInDays *int `access:"environment_session_lengths,write_restrictable,cloud_restrictable"` // telemetry: none @@ -569,6 +576,10 @@ func (s *ServiceSettings) SetDefaults(isUpdate bool) { s.EnableUserAccessTokens = new(false) } + if s.MaximumPersonalAccessTokenLifetimeDays == nil { + s.MaximumPersonalAccessTokenLifetimeDays = new(0) + } + if s.GoroutineHealthThreshold == nil { s.GoroutineHealthThreshold = new(-1) } @@ -4984,6 +4995,14 @@ func (s *ServiceSettings) isValid() *AppError { } } + // MaximumPersonalAccessTokenLifetimeDays: 0 means unlimited; negative is + // nonsensical; an upper bound of MaxPersonalAccessTokenLifetimeDays guards + // against int64 overflow when computing now + days*86_400_000 millis at + // token-creation time. + if *s.MaximumPersonalAccessTokenLifetimeDays < 0 || *s.MaximumPersonalAccessTokenLifetimeDays > MaxPersonalAccessTokenLifetimeDays { + return NewAppError("Config.IsValid", "model.config.is_valid.max_personal_access_token_lifetime_days.app_error", map[string]any{"Max": MaxPersonalAccessTokenLifetimeDays}, "", http.StatusBadRequest) + } + return nil } diff --git a/server/public/model/config_test.go b/server/public/model/config_test.go index fa34fa9cc66..52cca449ca5 100644 --- a/server/public/model/config_test.go +++ b/server/public/model/config_test.go @@ -148,6 +148,30 @@ func TestServiceSettingsIsValid(t *testing.T) { }, ExpectError: false, }, + "MaximumPersonalAccessTokenLifetimeDays zero (unlimited) is accepted": { + ServiceSettings: ServiceSettings{ + MaximumPersonalAccessTokenLifetimeDays: new(0), + }, + ExpectError: false, + }, + "MaximumPersonalAccessTokenLifetimeDays negative is rejected": { + ServiceSettings: ServiceSettings{ + MaximumPersonalAccessTokenLifetimeDays: new(-1), + }, + ExpectError: true, + }, + "MaximumPersonalAccessTokenLifetimeDays at upper bound is accepted": { + ServiceSettings: ServiceSettings{ + MaximumPersonalAccessTokenLifetimeDays: new(MaxPersonalAccessTokenLifetimeDays), + }, + ExpectError: false, + }, + "MaximumPersonalAccessTokenLifetimeDays beyond upper bound is rejected": { + ServiceSettings: ServiceSettings{ + MaximumPersonalAccessTokenLifetimeDays: new(MaxPersonalAccessTokenLifetimeDays + 1), + }, + ExpectError: true, + }, } { t.Run(name, func(t *testing.T) { test.ServiceSettings.SetDefaults(false) From ca19b0b834c1c929eb1731d5fe885b20996438fc Mon Sep 17 00:00:00 2001 From: M-ZubairAhmed Date: Thu, 4 Jun 2026 18:44:20 +0530 Subject: [PATCH 3/4] Remove dynamic-virtualized-list from ignoreDependencies in config.yaml (#36897) --- server/build/notice-file/config.yaml | 1 - .../dynamic_virtualized_list/README.md | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 webapp/channels/src/components/dynamic_virtualized_list/README.md diff --git a/server/build/notice-file/config.yaml b/server/build/notice-file/config.yaml index 72cbbbe2b02..83838ac586a 100644 --- a/server/build/notice-file/config.yaml +++ b/server/build/notice-file/config.yaml @@ -7,6 +7,5 @@ search: - "webapp/package.json" - "webapp/channels/package.json" ignoreDependencies: - - "@mattermost/dynamic-virtualized-list" - "@mattermost/shared" includeDevDependencies: false diff --git a/webapp/channels/src/components/dynamic_virtualized_list/README.md b/webapp/channels/src/components/dynamic_virtualized_list/README.md new file mode 100644 index 00000000000..326f8e12442 --- /dev/null +++ b/webapp/channels/src/components/dynamic_virtualized_list/README.md @@ -0,0 +1,17 @@ +# dynamic_virtualized_list + +A virtualized list that supports **dynamically-sized, unmeasured items** — rows whose +heights are not known ahead of time and are measured as they render. It is the rendering +engine behind Mattermost's long, variable-height message lists. + +## Origin + +This package was built from a fork of an alpha version of [`react-window`](https://github.com/bvaughn/react-window), +but took a substantially different approach — using relative rather than absolute positioning to render items whose sizes aren't known ahead of time. + +## Files + +| File | Purpose | +| --- | --- | +| `list_item.tsx` | Wrapper around each rendered row that reports its measured size back to the list. | +| `list_item_size_observer.ts` | Tracks rendered item sizes so the list can position rows without pre-measuring them. | From 3fc5b942927dede596ead4ebfec4f40085365f4b Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Thu, 4 Jun 2026 09:22:59 -0400 Subject: [PATCH 4/4] [MM-69115] Fixed issue where channels could end up in two categories (#36875) * [MM-69115] Fixed issue where channels could end up in two categories * Additional fix * PR feedback --- server/channels/app/channel.go | 10 ++- server/channels/app/channel_test.go | 96 +++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 10bc89acb81..769b2a8de40 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -4563,15 +4563,21 @@ func (a *App) addChannelToDefaultCategory(rctx request.CTX, userID string, chann // Find the original category if the channel is already in a category var originalCategory *model.SidebarCategoryWithChannels for _, category := range categories.Categories { - if category.Type == model.SidebarCategoryCustom && category.Channels != nil && slices.Contains(category.Channels, channel.Id) { + if category.Type != model.SidebarCategoryDirectMessages && slices.Contains(category.Channels, channel.Id) { originalCategory = category break } } + // The channel is already in the target category, so there's nothing to move. + if originalCategory != nil && originalCategory == targetCategory { + return + } + var categoriesToUpdate []*model.SidebarCategoryWithChannels if originalCategory != nil { - originalCategory.Channels = slices.Delete(originalCategory.Channels, slices.Index(originalCategory.Channels, channel.Id), 1) + idx := slices.Index(originalCategory.Channels, channel.Id) + originalCategory.Channels = slices.Delete(originalCategory.Channels, idx, idx+1) categoriesToUpdate = append(categoriesToUpdate, originalCategory) } diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index dce2f6e298c..b845a272620 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -3996,3 +3996,99 @@ func TestPatchChannelWithCategorySorting(t *testing.T) { require.Equal(t, "Disabled Category/Channel Name", patchedChannel.DisplayName) require.Equal(t, "New Category", patchedChannel.DefaultCategoryName) } + +func TestPatchChannelDefaultCategoryMovesOutOfChannels(t *testing.T) { + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.TeamSettings.EnableChannelCategorySorting = true + }) + + // Create several channels so the target sits at an index > 0 in the default Channels category, + // exercising the path that previously panicked when removing the channel from its original category. + var channels []*model.Channel + for range 3 { + c := th.createChannel(t, th.BasicTeam, model.ChannelTypeOpen) + channels = append(channels, c) + } + + categories, appErr := th.App.GetSidebarCategoriesForTeamForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + + var channelsCategory *model.SidebarCategoryWithChannels + for _, category := range categories.Categories { + if category.Type == model.SidebarCategoryChannels { + channelsCategory = category + break + } + } + require.NotNil(t, channelsCategory) + + created := make(map[string]*model.Channel, len(channels)) + for _, c := range channels { + created[c.Id] = c + } + + // Pick the created channel sitting at the highest index in the default Channels category so the + // removal path operates on an index > 1, which previously triggered an out-of-range panic. + var target *model.Channel + targetIndex := -1 + for i, channelID := range channelsCategory.Channels { + if c, ok := created[channelID]; ok && i > targetIndex { + target = c + targetIndex = i + } + } + require.NotNil(t, target) + require.Greater(t, targetIndex, 1, "expected a created channel beyond index 1 to exercise the removal path") + + patch := &model.ChannelPatch{DefaultCategoryName: new("Moved Category")} + _, appErr = th.App.PatchChannel(th.Context, target, patch, th.BasicUser.Id) + require.Nil(t, appErr) + + categories, appErr = th.App.GetSidebarCategoriesForTeamForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + + for _, category := range categories.Categories { + switch { + case category.DisplayName == "Moved Category": + assert.Contains(t, category.Channels, target.Id, "channel should be in the new default category") + case category.Type == model.SidebarCategoryChannels: + assert.NotContains(t, category.Channels, target.Id, "channel should no longer be in the default Channels category") + } + } +} + +func TestPatchChannelDefaultCategoryReapplyIsIdempotent(t *testing.T) { + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.TeamSettings.EnableChannelCategorySorting = true + }) + + channel := th.createChannel(t, th.BasicTeam, model.ChannelTypeOpen) + + patch := &model.ChannelPatch{DefaultCategoryName: new("Reapply Category")} + channel, appErr := th.App.PatchChannel(th.Context, channel, patch, th.BasicUser.Id) + require.Nil(t, appErr) + + // Patching again re-runs the default category logic while the channel is already in the target + // category. This must not error or list the channel twice in the category. + _, appErr = th.App.PatchChannel(th.Context, channel, &model.ChannelPatch{Header: new("updated header")}, th.BasicUser.Id) + require.Nil(t, appErr) + + categories, appErr := th.App.GetSidebarCategoriesForTeamForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + + for _, category := range categories.Categories { + if category.DisplayName == "Reapply Category" { + count := 0 + for _, channelID := range category.Channels { + if channelID == channel.Id { + count++ + } + } + assert.Equal(t, 1, count, "channel should appear exactly once in the default category") + } + } +}