From de65cc45c5b7dce371320fc39e5dad342fc78e8a Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Thu, 3 Sep 2026 05:59:48 -0400 Subject: [PATCH 1/2] Scope Asset Picker uploads to the field that opened the picker - The OS file dialog, drag-and-drop, and the Asset/File prompt all ignored the mimetype restriction already used to filter browsing, so an Image field or a Story Block video/audio node could upload any file type, producing assets that then vanished from the filtered list. - Add a pure `upload-restriction.ts` module (matcher, `accept` builder, label resolver) driven by the picker's existing `config.mimeTypes`, wire it into the hidden file input, a pre-upload guard on every upload route, and scoped copy in the Asset/File prompt, while leaving Content Drive and the unrestricted File/browse modes unchanged. --- .../dot-content-drive-shell.component.spec.ts | 10 + .../dot-asset-picker.component.html | 11 +- .../dot-asset-picker.component.spec.ts | 211 ++++++++++++++- .../dot-asset-picker.component.ts | 77 ++++++ .../upload-restriction.spec.ts | 164 ++++++++++++ .../dot-asset-picker/upload-restriction.ts | 112 ++++++++ .../dot-upload-type-selector/constants.ts | 6 + .../dot-upload-type-selector.component.html | 8 +- ...dot-upload-type-selector.component.spec.ts | 65 ++++- .../dot-upload-type-selector.component.ts | 12 + .../WEB-INF/messages/Language.properties | 7 + .../contracts/upload-restriction.contract.md | 189 +++++++++++++ .../data-model.md | 149 +++++++++++ specs/37365-asset-picker-upload-scope/spec.md | 252 ++++++++++++++++++ 14 files changed, 1268 insertions(+), 5 deletions(-) create mode 100644 core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.spec.ts create mode 100644 core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.ts create mode 100644 specs/37365-asset-picker-upload-scope/contracts/upload-restriction.contract.md create mode 100644 specs/37365-asset-picker-upload-scope/data-model.md create mode 100644 specs/37365-asset-picker-upload-scope/spec.md diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts index ef8677f1c5e8..10b288abcf27 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts @@ -1285,6 +1285,16 @@ describe('DotContentDriveShellComponent', () => { spectator.detectChanges(); }; + it('should leave the upload restricted to nothing', () => { + // Content Drive shares the selector and the dropzone with the Asset Picker, which + // scopes uploads to the field that opened it (#37365). Content Drive has no such field: + // it must keep accepting every file type, with today's wording. An empty + // `restrictionLabel` is what keeps the default descriptions rendering. + openViaButton(TARGET_FOLDER_DATA); + + expect(spectator.query(DotUploadTypeSelectorComponent).$restrictionLabel()).toBe(''); + }); + it('should open the upload menu with the selected folder when the upload button is clicked', () => { openViaButton(TARGET_FOLDER_DATA); diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html index 381e4ba7e4f9..9605d1f01a25 100644 --- a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html @@ -96,6 +96,7 @@ data-testid="asset-picker-upload-selector" [targetFolder]="payload.targetFolder" [files]="payload.files" + [restrictionLabel]="$uploadRestrictionLabel()" (selectUploadType)="onUploadTypeSelected($event)" /> } @@ -124,6 +125,14 @@ - + + diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.spec.ts b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.spec.ts index bb7ec7d2cb71..bfd1bbb9f286 100644 --- a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.spec.ts +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.spec.ts @@ -37,6 +37,7 @@ import { DotDialogHeaderComponent } from '../dot-dialog'; import { DotToastComponent } from '../dot-toast/dot-toast.component'; +import { DotUploadTypeSelectorComponent } from '../dot-upload-type-selector/dot-upload-type-selector.component'; /** * What every `overrideComponent({ set: { imports } })` below has to keep real. @@ -61,7 +62,8 @@ const PICKER_REAL_IMPORTS = [ DotDialogComponent, DotDialogHeaderComponent, DotDialogContentComponent, - DotDialogFooterComponent + DotDialogFooterComponent, + DotUploadTypeSelectorComponent ]; const SITE: DotSite = { @@ -187,7 +189,12 @@ describe('DotAssetPickerComponent', () => { provide: DotMessageService, useValue: new MockDotMessageService({ 'dot.common.dialog.accept': 'Add', - 'dot.common.dialog.reject': 'Cancel' + 'dot.common.dialog.reject': 'Cancel', + 'dot.asset.picker.upload.rejected': "Can't upload this file", + 'dot.asset.picker.upload.rejected.detail': 'Only {0} can be uploaded here.', + 'dot.asset.picker.upload.types.image': 'images', + 'dot.asset.picker.upload.types.video': 'video files', + 'dot.asset.picker.upload.types.audio': 'audio files' }) }, { provide: DynamicDialogConfig, useValue: { data: CONFIG } } @@ -593,6 +600,206 @@ describe('DotAssetPickerComponent', () => { expect(store.loadItems).not.toHaveBeenCalled(); }); }); + + describe('upload restriction', () => { + const messageService = () => spectator.inject(MessageService, true); + + /** `new File()` defaults `type` to `''`, which the restriction deliberately allows. */ + const fileList = (type: string, name = 'asset.bin'): FileList => { + const files = [new File([''], name, { type })] as unknown as FileList; + Object.defineProperty(files, 'length', { value: 1 }); + + return files; + }; + + /** Puts the picker in a media mode, the way an Image field opens it. */ + const restrictToImages = () => { + store.config.set({ ...CONFIG, mimeTypes: ['image/*'] }); + spectator.detectChanges(); + }; + + describe('in a media mode', () => { + beforeEach(() => restrictToImages()); + + it('should refuse a dropped file outside the allowed types', () => { + const spyAdd = jest.spyOn(messageService(), 'add'); + + spectator.component['onRequestUpload']({ + files: fileList('application/pdf', 'report.pdf'), + targetFolder: PINNED_FOLDER + }); + + expect(uploadService.uploadFileByBaseType).not.toHaveBeenCalled(); + expect(spyAdd).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'error', + detail: 'Only images can be uploaded here.' + }) + ); + }); + + it('should not open the Asset/File prompt for a refused drop', () => { + // Without the early gate the user is asked to choose a storage type and only then + // told the file was never eligible. + spectator.component['onRequestUpload']({ + files: fileList('application/pdf', 'report.pdf') + }); + + expect(spectator.component.$uploadSelectorPayload()).toBeUndefined(); + expect(spectator.component.$uploadModalVisible()).toBe(false); + }); + + it('should refuse a file chosen through the OS dialog', () => { + // `accept` is a hint the user can override from the dialog's own filter, so the + // pre-upload check has to stand on its own. + spectator.component.$activeSelection.set({ + baseType: DotCMSBaseTypesContentTypes.DOTASSET + }); + + spectator.component['onFileChange']({ + target: { files: fileList('application/pdf', 'report.pdf'), value: 'x' } + } as unknown as Event); + + expect(uploadService.uploadFileByBaseType).not.toHaveBeenCalled(); + }); + + it('should refuse a file after the Asset/File prompt is answered', () => { + spectator.component['onUploadTypeSelected']({ + baseType: DotCMSBaseTypesContentTypes.DOTASSET, + files: fileList('application/pdf', 'report.pdf') + }); + + expect(uploadService.uploadFileByBaseType).not.toHaveBeenCalled(); + }); + + it('should refuse a drop into a folder that pins a base type', () => { + // This route skips the prompt entirely — the one most easily left unguarded. + spectator.component['onRequestUpload']({ + files: fileList('audio/mpeg', 'song.mp3'), + targetFolder: PINNED_FOLDER + }); + + expect(uploadService.uploadFileByBaseType).not.toHaveBeenCalled(); + }); + + it('should refuse a button upload into a folder that pins a base type', () => { + store.selectedNode.set({ data: PINNED_FOLDER }); + spectator.detectChanges(); + + spectator.component['onUpload'](new MouseEvent('click')); + spectator.component['onFileChange']({ + target: { files: fileList('application/pdf', 'report.pdf'), value: 'x' } + } as unknown as Event); + + expect(uploadService.uploadFileByBaseType).not.toHaveBeenCalled(); + }); + + it('should allow a file whose type the browser does not report', () => { + // AC-010: the server stays the authority rather than blocking a file we cannot + // classify. + spectator.component['onRequestUpload']({ + files: fileList('', 'mystery.dat'), + targetFolder: PINNED_FOLDER + }); + + expect(uploadService.uploadFileByBaseType).toHaveBeenCalled(); + }); + + it('should upload an allowed file and refresh the list with the restriction intact', () => { + store.$request.set({ mimeTypes: ['image/*'] }); + + spectator.component['onRequestUpload']({ + files: fileList('image/png', 'logo.png'), + targetFolder: PINNED_FOLDER + }); + + expect(uploadService.uploadFileByBaseType).toHaveBeenCalled(); + expect(store.loadItems).toHaveBeenCalledWith({ mimeTypes: ['image/*'] }); + }); + }); + + describe('the hidden file input', () => { + const fileInput = () => + spectator.query('input[type="file"]') as HTMLInputElement | null; + + it('should filter the OS dialog to the restricted family', () => { + restrictToImages(); + + expect(fileInput()?.getAttribute('accept')).toBe('image/*'); + }); + + it('should carry every pattern a browse caller asked for', () => { + store.config.set({ ...CONFIG, mimeTypes: ['image/*', 'video/*'] }); + spectator.detectChanges(); + + expect(fileInput()?.getAttribute('accept')).toBe('image/*,video/*'); + }); + + it('should carry no accept attribute at all when nothing is restricted', () => { + // Absence, not `accept=""` — an empty value is a different thing to the browser, + // and a test asserting `''` would pass against a broken implementation. + expect(fileInput()?.hasAttribute('accept')).toBe(false); + }); + }); + + describe('the Asset/File prompt', () => { + const selector = () => spectator.query(DotUploadTypeSelectorComponent); + + it('should hand the restriction label to the selector in a media mode', () => { + restrictToImages(); + + spectator.component['onUpload'](new MouseEvent('click')); + spectator.detectChanges(); + + expect(selector()?.$restrictionLabel()).toBe('images'); + }); + + it('should hand the selector no label when nothing is restricted', () => { + spectator.component['onUpload'](new MouseEvent('click')); + spectator.detectChanges(); + + expect(selector()?.$restrictionLabel()).toBe(''); + }); + }); + + describe('in the File field, which restricts nothing', () => { + // The over-reach guard. CONFIG carries no `mimeTypes`, exactly as a File field opens. + it('should upload a dropped PDF', () => { + const spyAdd = jest.spyOn(messageService(), 'add'); + + spectator.component['onRequestUpload']({ + files: fileList('application/pdf', 'report.pdf'), + targetFolder: PINNED_FOLDER + }); + + expect(uploadService.uploadFileByBaseType).toHaveBeenCalled(); + expect(spyAdd).not.toHaveBeenCalledWith( + expect.objectContaining({ severity: 'error' }) + ); + }); + + it('should upload a PDF chosen through the OS dialog', () => { + spectator.component.$activeSelection.set({ + baseType: DotCMSBaseTypesContentTypes.DOTASSET + }); + + spectator.component['onFileChange']({ + target: { files: fileList('application/zip', 'bundle.zip'), value: 'x' } + } as unknown as Event); + + expect(uploadService.uploadFileByBaseType).toHaveBeenCalled(); + }); + + it('should upload a PDF after the Asset/File prompt is answered', () => { + spectator.component['onUploadTypeSelected']({ + baseType: DotCMSBaseTypesContentTypes.FILEASSET, + files: fileList('application/pdf', 'report.pdf') + }); + + expect(uploadService.uploadFileByBaseType).toHaveBeenCalled(); + }); + }); + }); }); describe('DotAssetPickerComponent — opened without dialog data', () => { diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts index 07322a48dc08..2d4999e81a76 100644 --- a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts @@ -50,6 +50,11 @@ import { import { DotAssetPickerLocation, writeLastAssetLocation } from './last-asset-path'; import { DotAssetPickerStore } from './store/dot-asset-picker.store'; import { DotAssetPickerConfig } from './store/models'; +import { + buildUploadAccept, + isUploadAllowed, + resolveUploadRestrictionLabel +} from './upload-restriction'; import { DIALOG_SIZE_TRANSITION, MAXIMIZED_DIALOG_CLASS } from '../../dialog/fullscreen-dialog'; import { DotMessagePipe } from '../../dot-message/dot-message.pipe'; @@ -202,6 +207,28 @@ export class DotAssetPickerComponent implements OnInit { /** Holds the chosen type while the OS file picker is open (Upload-button flow only). */ readonly $activeSelection = signal(undefined); + /** + * Pre-filters the OS file dialog to what the entry point can hold. + * + * `null` in the unrestricted modes, which removes the attribute — the File field and `browse` + * must keep offering every file. A hint only: the dialog lets the user switch back to "all + * files", which is why `#refuseDisallowedUpload` still has to stand behind it. + */ + protected readonly $uploadAccept = computed(() => + buildUploadAccept(this.store.config()?.mimeTypes) + ); + + /** + * What the restriction is called, for the Asset/File prompt and the refusal toast. Empty when + * nothing is restricted, which is what makes the prompt render its default copy. + */ + protected readonly $uploadRestrictionLabel = computed( + () => + resolveUploadRestrictionLabel(this.store.config()?.mimeTypes, (key) => + this.#dotMessageService.get(key) + ) ?? '' + ); + ngOnInit(): void { const config = this.#dialogConfig?.data; @@ -410,6 +437,13 @@ export class DotAssetPickerComponent implements OnInit { /** Drag-and-drop: the files are already known, so a pinned base type uploads immediately. */ protected onRequestUpload({ files, targetFolder }: DotUploadFiles): void { + // Judged here as well as at `#resolveFilesUpload`, which is the actual guarantee. Without + // this the user would be asked to choose a storage type for a file that was never eligible, + // and only then be refused. + if (this.#refuseDisallowedUpload(files)) { + return; + } + const baseType = this.#resolvePreferredBaseType(targetFolder); if (baseType) { @@ -506,11 +540,54 @@ export class DotAssetPickerComponent implements OnInit { } } + /** + * Refuses a file the entry point cannot hold, and says which types it can. + * + * The restriction is `config.mimeTypes` — the same value that narrows what the list shows, so + * an Image field cannot upload something it would then be unable to display. Returns whether + * the upload was refused. + * + * This is the guarantee, not the filter: `accept` on the file input only *suggests* a type to + * the OS dialog, and the user can switch it off from the dialog itself. Judged on the file that + * would actually be uploaded, since a multi-file selection already warns and uploads only the + * first. + */ + #refuseDisallowedUpload(files?: FileList | null): boolean { + const mimeTypes = this.store.config()?.mimeTypes; + const file = files?.[0]; + + if (!file || isUploadAllowed(file, mimeTypes)) { + return false; + } + + const allowed = resolveUploadRestrictionLabel(mimeTypes, (key) => + this.#dotMessageService.get(key) + ); + + this.#messageService.add({ + severity: 'error', + summary: this.#dotMessageService.get('dot.asset.picker.upload.rejected'), + detail: this.#dotMessageService.get( + 'dot.asset.picker.upload.rejected.detail', + allowed ?? '' + ), + life: ERROR_MESSAGE_LIFE + }); + + return true; + } + #resolveFilesUpload({ files, targetFolder, baseType }: DotUploadSelection): void { if (!files?.length) { return; } + // The one gate every upload route converges on — the Upload button, drag-and-drop, and a + // folder whose settings pin a base type and skip the prompt entirely. + if (this.#refuseDisallowedUpload(files)) { + return; + } + if (files.length > 1) { this.#messageService.add({ severity: 'warn', diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.spec.ts b/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.spec.ts new file mode 100644 index 000000000000..8e7837a91d42 --- /dev/null +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.spec.ts @@ -0,0 +1,164 @@ +import { describe, expect, it } from '@jest/globals'; + +import { + buildUploadAccept, + isUploadAllowed, + resolveUploadRestrictionLabel +} from './upload-restriction'; + +/** `new File()` defaults `type` to `''`, so pass it explicitly wherever the type is the subject. */ +const fileOfType = (type: string, name = 'asset.bin') => new File([''], name, { type }); + +/** Stands in for `DotMessageService.get` — the module takes the lookup as a parameter. */ +const translate = (key: string) => + ({ + 'dot.asset.picker.upload.types.image': 'images', + 'dot.asset.picker.upload.types.video': 'video files', + 'dot.asset.picker.upload.types.audio': 'audio files' + })[key] ?? key; + +describe('upload-restriction', () => { + describe('isUploadAllowed', () => { + describe('when there is no restriction', () => { + // The File field and the browse entry point both arrive here. Absence is the + // unrestricted state — a reader that defaults to refusing breaks them silently. + it('should allow anything when mimeTypes is undefined', () => { + expect(isUploadAllowed(fileOfType('application/pdf'), undefined)).toBe(true); + }); + + it('should allow anything when mimeTypes is empty', () => { + expect(isUploadAllowed(fileOfType('application/pdf'), [])).toBe(true); + }); + }); + + describe('when the browser reports no type', () => { + // AC-010: an unclassifiable file is allowed through and the server stays the + // authority. Refusing it would block a legitimate image behind a message saying only + // images are allowed. + it('should allow a file whose type is empty', () => { + expect(isUploadAllowed(fileOfType(''), ['image/*'])).toBe(true); + }); + }); + + describe('wildcard patterns', () => { + it('should allow a file in the restricted family', () => { + expect(isUploadAllowed(fileOfType('image/png'), ['image/*'])).toBe(true); + }); + + it('should reject a file outside the restricted family', () => { + expect(isUploadAllowed(fileOfType('application/pdf'), ['image/*'])).toBe(false); + }); + + it('should reject a media file of the wrong family', () => { + expect(isUploadAllowed(fileOfType('audio/mpeg'), ['video/*'])).toBe(false); + }); + + it('should not match on a family prefix', () => { + // `x-image/foo` contains `image/`; a substring match would wrongly allow it. + expect(isUploadAllowed(fileOfType('x-image/foo'), ['image/*'])).toBe(false); + }); + }); + + describe('exact patterns', () => { + // Only reachable through `browse`, whose caller supplies its own list. + it('should allow an exact match', () => { + expect(isUploadAllowed(fileOfType('application/pdf'), ['application/pdf'])).toBe( + true + ); + }); + + it('should reject a different type', () => { + expect(isUploadAllowed(fileOfType('application/zip'), ['application/pdf'])).toBe( + false + ); + }); + + it('should compare case-insensitively', () => { + expect(isUploadAllowed(fileOfType('IMAGE/PNG'), ['image/*'])).toBe(true); + expect(isUploadAllowed(fileOfType('application/PDF'), ['APPLICATION/pdf'])).toBe( + true + ); + }); + }); + + describe('several patterns', () => { + it('should allow a file matching any of them', () => { + expect(isUploadAllowed(fileOfType('video/mp4'), ['image/*', 'video/*'])).toBe(true); + }); + + it('should reject a file matching none of them', () => { + expect(isUploadAllowed(fileOfType('application/pdf'), ['image/*', 'video/*'])).toBe( + false + ); + }); + }); + + describe('the filename', () => { + // AC-002: the restriction comes from `config.mimeTypes` alone. An extension fallback + // would be a second, hand-maintained list of types. + it('should never be consulted — a mislabelled name does not rescue a rejected type', () => { + expect( + isUploadAllowed(fileOfType('application/pdf', 'photo.png'), ['image/*']) + ).toBe(false); + }); + + it('should never be consulted — a wrong extension does not condemn an allowed type', () => { + expect(isUploadAllowed(fileOfType('image/png', 'report.pdf'), ['image/*'])).toBe( + true + ); + }); + }); + }); + + describe('buildUploadAccept', () => { + it('should pass a single pattern through verbatim', () => { + // `accept` takes the same `type/*` syntax the browse filter already uses, so no + // translation layer is needed. + expect(buildUploadAccept(['image/*'])).toBe('image/*'); + }); + + it('should join several patterns with a comma', () => { + expect(buildUploadAccept(['image/*', 'video/*'])).toBe('image/*,video/*'); + }); + + it('should return null when there is no restriction', () => { + // Null, not empty string: the binding has to *remove* the attribute. An empty `accept` + // is not the same thing to the browser as no `accept`. + expect(buildUploadAccept(undefined)).toBeNull(); + expect(buildUploadAccept([])).toBeNull(); + }); + }); + + describe('resolveUploadRestrictionLabel', () => { + it('should resolve the label for each known media family', () => { + expect(resolveUploadRestrictionLabel(['image/*'], translate)).toBe('images'); + expect(resolveUploadRestrictionLabel(['video/*'], translate)).toBe('video files'); + expect(resolveUploadRestrictionLabel(['audio/*'], translate)).toBe('audio files'); + }); + + it('should return undefined when there is no restriction', () => { + expect(resolveUploadRestrictionLabel(undefined, translate)).toBeUndefined(); + expect(resolveUploadRestrictionLabel([], translate)).toBeUndefined(); + }); + + it('should fall back to the raw patterns for an unknown family', () => { + // A `browse` caller may pass anything. The message has to stay correct rather than + // rendering "Only can be uploaded here." + expect(resolveUploadRestrictionLabel(['application/pdf'], translate)).toBe( + 'application/pdf' + ); + }); + + it('should join several resolved families', () => { + expect(resolveUploadRestrictionLabel(['image/*', 'video/*'], translate)).toBe( + 'images, video files' + ); + }); + + it('should not repeat a family listed twice', () => { + expect(resolveUploadRestrictionLabel(['image/png', 'image/jpeg'], translate)).toBe( + 'images' + ); + }); + }); +}); diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.ts b/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.ts new file mode 100644 index 000000000000..1d8f42b8092b --- /dev/null +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.ts @@ -0,0 +1,112 @@ +import { ASSET_PICKER_MIME_TYPES } from './asset-picker-config'; + +/** + * Applies the picker's existing mimetype narrowing to uploads. + * + * The restriction is not a new concept: `config.mimeTypes` already decides what the editor may + * *browse*, and this is the same value deciding what they may *add*. Nothing here knows about an + * "Image field" or a "video node" — presence of a restriction is the whole input, which is what + * keeps the File field and the browse entry point unrestricted without a single `mode === 'x'`. + * + * Pure on purpose, like {@link ./asset-picker-config} and {@link ./last-asset-path}: the message + * lookup arrives as a parameter rather than an injected `DotMessageService`, so this is testable + * without a component harness. Not exported from the library barrel — it has no consumer outside + * the picker. + */ + +const MIME_LABEL_KEY_PREFIX = 'dot.asset.picker.upload.types.'; + +/** + * The type families that have a human label. + * + * Derived from {@link ASSET_PICKER_MIME_TYPES} rather than written out again, so adding a media + * mode there is the only edit needed — the family becomes labellable and only its message key has + * to follow. A `browse` caller may pass anything else; those fall back to their raw pattern. + */ +const LABELLED_FAMILIES: ReadonlySet = new Set(Object.keys(ASSET_PICKER_MIME_TYPES)); + +/** `image/*` and `image/png` are both the `image` family. */ +const familyOf = (pattern: string): string => pattern.trim().toLowerCase().split('/')[0]; + +/** + * Whether a file may be uploaded under the given restriction. + * + * Two things are deliberately permissive, and both are load-bearing: + * + * - **No restriction accepts everything.** Absence, not a sentinel, is the unrestricted state — + * the File field and `browse` arrive here with nothing, and a guard that defaulted to refusing + * would break them silently. + * - **A file the browser reports no type for is accepted.** The server remains the authority; + * refusing would occasionally block a legitimate image behind a message saying only images are + * allowed, with no way forward. (`DotDropZoneComponent.typeMatch` rejects in that case — the + * divergence is intentional.) + * + * The filename is never consulted. Matching on extensions would mean maintaining a second list of + * types alongside the one the browse filter already uses, which is the thing this whole module + * exists to avoid. + */ +export function isUploadAllowed(file: File, mimeTypes?: string[]): boolean { + if (!mimeTypes?.length) { + return true; + } + + const type = file.type?.trim().toLowerCase(); + + if (!type) { + return true; + } + + return mimeTypes.some((pattern) => { + const candidate = pattern.trim().toLowerCase(); + + // `image/*` matches the family, and only as a prefix — a substring test would also let + // `x-image/foo` through. + return candidate.endsWith('/*') + ? type.startsWith(candidate.slice(0, -1)) + : type === candidate; + }); +} + +/** + * The `accept` value for the hidden file input, or `null` when nothing is restricted. + * + * The patterns pass through verbatim: `accept` takes the same `type/*` syntax the browse filter + * already uses, so there is nothing to translate. + * + * `null` rather than `''` so the binding *removes* the attribute — an empty `accept` is not the + * same thing to the browser as no `accept`. + */ +export function buildUploadAccept(mimeTypes?: string[]): string | null { + return mimeTypes?.length ? mimeTypes.join(',') : null; +} + +/** + * Names the restriction in words an author can read — "images", not `image/*`. + * + * Falls back to the raw pattern for a family with no label, so a `browse` caller passing something + * exotic still produces a correct message instead of "Only can be uploaded here." + */ +export function resolveUploadRestrictionLabel( + mimeTypes: string[] | undefined, + translate: (key: string) => string +): string | undefined { + if (!mimeTypes?.length) { + return undefined; + } + + const labels: string[] = []; + + for (const pattern of mimeTypes) { + const family = familyOf(pattern); + const label = LABELLED_FAMILIES.has(family) + ? translate(`${MIME_LABEL_KEY_PREFIX}${family}`) + : pattern.trim(); + + // Two patterns in the same family resolve to one label; say it once. + if (!labels.includes(label)) { + labels.push(label); + } + } + + return labels.join(', '); +} diff --git a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/constants.ts b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/constants.ts index 5d3ef5f85e33..8b1eda27207e 100644 --- a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/constants.ts +++ b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/constants.ts @@ -3,6 +3,10 @@ import { DotCMSBaseTypesContentTypes } from '@dotcms/dotcms-models'; /** * The two ways an upload can be stored. Order is the display order; `recommended` flags the one the * product steers users toward. + * + * Each option carries two descriptions. `descriptionKey` is the general one; `scopedDescriptionKey` + * takes the host's restriction as `{0}`, so a picker opened for video does not offer "images, + * documents, and media". Hosts with no restriction never reach the scoped key. */ export const UPLOAD_SELECTOR_OPTIONS = [ { @@ -10,6 +14,7 @@ export const UPLOAD_SELECTOR_OPTIONS = [ icon: 'image', labelKey: 'content-drive.dialog.upload-selector.asset', descriptionKey: 'content-drive.dialog.upload-selector.asset.description', + scopedDescriptionKey: 'content-drive.dialog.upload-selector.asset.description.scoped', recommended: true }, { @@ -17,6 +22,7 @@ export const UPLOAD_SELECTOR_OPTIONS = [ icon: 'code_blocks', labelKey: 'content-drive.dialog.upload-selector.file', descriptionKey: 'content-drive.dialog.upload-selector.file.description', + scopedDescriptionKey: 'content-drive.dialog.upload-selector.file.description.scoped', recommended: false } ] as const; diff --git a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.html b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.html index a3d2bc00e7e4..7c372204b898 100644 --- a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.html +++ b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.html @@ -1,4 +1,6 @@
+ @let restrictionLabel = $restrictionLabel(); + @for (option of options; track option.baseType) { diff --git a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.spec.ts b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.spec.ts index 9d5cd6929e44..36365bf7f960 100644 --- a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.spec.ts +++ b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.spec.ts @@ -30,7 +30,11 @@ describe('DotUploadTypeSelectorComponent', () => { 'content-drive.dialog.upload-selector.file.description': 'For code', 'content-drive.dialog.upload-selector.recommended': 'Recommended', 'content-drive.dialog.upload-selector.settings-hint': - 'Set your default upload type in the Folder Settings.' + 'Set your default upload type in the Folder Settings.', + 'content-drive.dialog.upload-selector.asset.description.scoped': + 'For {0} used in your content', + 'content-drive.dialog.upload-selector.file.description.scoped': + 'For {0} that need predictable URLs' }) } ], @@ -71,6 +75,65 @@ describe('DotUploadTypeSelectorComponent', () => { }); }); + describe('copy scoped to a restriction', () => { + const descriptionOf = (baseType: string) => + spectator + .query(byTestId(`upload-selector-option-${baseType}`)) + ?.textContent?.replace(/\s+/g, ' ') + .trim(); + + describe('when the host restricts what may be uploaded', () => { + beforeEach(() => { + spectator.setInput('restrictionLabel', 'video files'); + spectator.detectChanges(); + }); + + it('should still offer both storage options', () => { + // The list is never filtered. An Asset and a File can each hold a video, and a + // folder's pinned default can be either, so removing one takes away a real choice. + expect(spectator.query(byTestId('upload-selector-option-DOTASSET'))).toBeTruthy(); + expect(spectator.query(byTestId('upload-selector-option-FILEASSET'))).toBeTruthy(); + }); + + it('should describe the Asset option in terms of the restriction', () => { + expect(descriptionOf('DOTASSET')).toContain('For video files used in your content'); + }); + + it('should describe the File option in terms of the restriction', () => { + expect(descriptionOf('FILEASSET')).toContain( + 'For video files that need predictable URLs' + ); + }); + + it('should not promise types the restriction excludes', () => { + // The defect's most visible symptom: "For images, documents, and media" offered + // inside a video-only picker. + expect(descriptionOf('DOTASSET')).not.toContain('For images'); + expect(descriptionOf('FILEASSET')).not.toContain('For code'); + }); + }); + + describe('when the host restricts nothing', () => { + // Content Drive passes no label, so its rendered copy must be exactly today's. This is + // the AC-008 guarantee — assert it rather than assume it. + it('should keep the default Asset description', () => { + expect(descriptionOf('DOTASSET')).toContain('For images'); + }); + + it('should keep the default File description', () => { + expect(descriptionOf('FILEASSET')).toContain('For code'); + }); + + it('should keep the default descriptions for an empty label', () => { + spectator.setInput('restrictionLabel', ''); + spectator.detectChanges(); + + expect(descriptionOf('DOTASSET')).toContain('For images'); + expect(descriptionOf('FILEASSET')).toContain('For code'); + }); + }); + }); + describe('selection', () => { it('should emit the DOTASSET selection with the folder and files when Asset is clicked', () => { const files = { length: 0 } as FileList; diff --git a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.ts b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.ts index 7c7084e1dd11..c4d8b9f2699d 100644 --- a/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.ts +++ b/core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.ts @@ -30,6 +30,18 @@ export class DotUploadTypeSelectorComponent { /** Files to upload — present for the drag-and-drop flow, absent for the Upload-button flow. */ $files = input(undefined, { alias: 'files' }); + /** + * What the host allows, already translated (e.g. `"images"`) — empty when it allows everything. + * + * Set, the options describe themselves in terms of it instead of promising types the host would + * refuse. The option list itself is never filtered: an Asset and a File can each hold an image, + * a video or an audio file, and a folder's pinned default can be either. + * + * Deliberately a translated string rather than a mode or a message key — this component stays + * generic, and the host remains the one place that knows what its own restriction is called. + */ + $restrictionLabel = input('', { alias: 'restrictionLabel' }); + /** Emits the chosen base type plus the upload context when the user picks an option. */ selectUploadType = output(); diff --git a/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties b/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties index 4db77ad34d28..4dd323dae7d1 100644 --- a/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties +++ b/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties @@ -7283,6 +7283,8 @@ content-drive.dialog.upload-selector.asset=Asset content-drive.dialog.upload-selector.asset.description=For images, documents, and media used in your content content-drive.dialog.upload-selector.file=File content-drive.dialog.upload-selector.file.description=For code, templates, and developer files that need predictable URLs +content-drive.dialog.upload-selector.asset.description.scoped=For {0} used in your content +content-drive.dialog.upload-selector.file.description.scoped=For {0} that need predictable URLs content-drive.dialog.upload-selector.settings-hint=Set your default upload type in the Folder Settings. content-drive.dialog.folder.header=Create Folder content-drive.dialog.folder.header.edit=Folder Settings @@ -8207,6 +8209,11 @@ dot.asset.picker.error.assets=Couldn't load the assets dot.asset.picker.error.folders=Couldn't load the folders dot.asset.picker.confirm.error=Couldn't add the asset dot.asset.picker.confirm.error.detail=We couldn't load the selected asset. It may have been deleted or you may no longer have access to it. +dot.asset.picker.upload.rejected=Can't upload this file +dot.asset.picker.upload.rejected.detail=Only {0} can be uploaded here. +dot.asset.picker.upload.types.image=images +dot.asset.picker.upload.types.video=video files +dot.asset.picker.upload.types.audio=audio files ## dotAuth Portlet dotauth.header=dotAuth diff --git a/specs/37365-asset-picker-upload-scope/contracts/upload-restriction.contract.md b/specs/37365-asset-picker-upload-scope/contracts/upload-restriction.contract.md new file mode 100644 index 000000000000..d210c6b2300a --- /dev/null +++ b/specs/37365-asset-picker-upload-scope/contracts/upload-restriction.contract.md @@ -0,0 +1,189 @@ +# Contract: Asset Picker upload restriction + +**Feature**: `specs/37365-asset-picker-upload-scope` | **Date**: 2026-09-03 + +The picker exposes no REST endpoint. Its contracts are the **UI surfaces** other code and other +teams depend on: a shared component's inputs, a rendered DOM attribute, and a message bundle. Those +are what this document pins down, because breaking one of them is how AC-007 and AC-008 get +violated by accident. + +--- + +## C1 — `DotUploadTypeSelectorComponent` public API + +The only public API change in this fix. + +### Existing inputs (unchanged) + +| Input | Type | Default | +|---|---|---| +| `targetFolder` | `TreeNodeData \| undefined` | `undefined` | +| `files` | `FileList \| undefined` | `undefined` | + +### New input + +| Input | Type | Default | Meaning | +|---|---|---|---| +| `restrictionLabel` | `string` | `''` | The already-translated human name for what the host allows (e.g. `"images"`). Empty means unrestricted. | + +**Contract rules**: + +1. **Absent or empty ⇒ today's behavior exactly.** Both options render, with + `content-drive.dialog.upload-selector.asset.description` and + `...file.description`. Content Drive passes nothing, so its output is unchanged. *This is the + AC-008 guarantee and must be asserted, not assumed.* +2. **Set ⇒ both options still render.** The option list is never filtered. Per the Q1 decision, both + `DOTASSET` and `FILEASSET` can legitimately hold an image, a video or an audio file, and the + per-folder default upload-type preference can pin either. +3. **Set ⇒ the scoped description keys are used**, parameterized with the label (C3). +4. The component does **not** translate. It receives a translated string and interpolates it. Adding + `DotMessageService` lookups for the restriction inside this shared component is out of contract. +5. `selectUploadType` emits the same `DotUploadSelection` shape regardless. The restriction changes + what is *said*, never what is *emitted*. + +### Unchanged by contract + +`DotUploadDropzoneComponent` gains no input and changes no behavior. Its spec file should need no +edit; if it does, the design in [research.md R3](../research.md#r3--refusing-a-dropped-file-without-touching-the-shared-dropzone) +was not followed. + +--- + +## C2 — Rendered DOM contract + +### The hidden file input + +`dot-asset-picker.component.html`, currently ``. + +| Picker state | Rendered | +|---|---| +| Media mode (`image`) | `accept="image/*"` | +| Media mode (`video` / `audio`) | `accept="video/*"` / `accept="audio/*"` | +| `browse` with caller-supplied types | `accept` = those types, joined with `,` | +| `file` mode | **no `accept` attribute at all** | +| `browse` with no caller types | **no `accept` attribute at all** | + +**Contract rule**: in the unrestricted cases the attribute is *absent*, not empty. `accept=""` is a +different thing to the browser, and a test asserting `toBe('')` would pass against a broken +implementation. Assert absence. + +### Test hooks + +Existing `data-testid`s are relied on and must not be renamed: +`asset-picker-dropzone`, `asset-picker-upload-selector`, `asset-picker-upload-popover`, +`asset-picker-upload-modal`, `upload-selector-option-`, +`upload-selector-recommended`, `upload-selector-settings-hint`. + +--- + +## C3 — Message bundle contract + +New keys in `dotCMS/src/main/webapp/WEB-INF/messages/Language.properties`. The picker's own keys use +the `dot.asset.picker.*` namespace (existing convention, line ~8193); the two scoped option +descriptions extend the shared `content-drive.dialog.upload-selector.*` family they belong to. + +### Refusal toast (AC-004) + +```properties +dot.asset.picker.upload.rejected=Can't upload this file +dot.asset.picker.upload.rejected.detail=Only {0} can be uploaded here. +``` + +`{0}` is the restriction label from C4. + +### Family labels (AC-004, AC-005) + +```properties +dot.asset.picker.upload.types.image=images +dot.asset.picker.upload.types.video=video files +dot.asset.picker.upload.types.audio=audio files +``` + +Lower-case: these are interpolated mid-sentence, never used as a standalone heading. + +### Scoped option descriptions (AC-005) + +```properties +content-drive.dialog.upload-selector.asset.description.scoped=For {0} used in your content +content-drive.dialog.upload-selector.file.description.scoped=For {0} that need predictable URLs +``` + +`{0}` is the same restriction label. These are used **only** when `restrictionLabel` is set. + +### Unchanged keys + +These keep their current wording and remain the default. Content Drive renders them (AC-008): + +```properties +content-drive.dialog.upload-selector.asset.description=For images, documents, and media used in your content +content-drive.dialog.upload-selector.file.description=For code, templates, and developer files that need predictable URLs +``` + +Also unchanged: `content-drive.dialog.upload-selector.header`, `.recommended`, `.asset`, `.file`, +`.settings-hint`, and every existing upload toast key. + +--- + +## C4 — `upload-restriction.ts` module contract + +A new pure module, colocated with `asset-picker-config.ts` and `last-asset-path.ts` — the +established shape for the picker's non-component logic. No Angular imports, no injection, directly +unit-testable. + +### Exported behavior + +| Export | Input | Output | +|---|---|---| +| Matcher | a `File` (or its `type`) + the restriction list | `boolean` — may this file be uploaded | +| `accept` builder | the restriction list | the joined attribute value, or `null` when unrestricted | +| Label resolver | the restriction list + a translate function | the translated human label, or `undefined` when unrestricted | + +### Behavioral contract + +Full rule table in [data-model.md §2](../data-model.md#2-validation-rules). The three that are easy +to get wrong, and are therefore the ones to write tests for first: + +1. **Empty restriction ⇒ accept.** Never default to refusing when the value is missing — that breaks + the File field (AC-007). +2. **Empty `file.type` ⇒ accept.** The AC-010 decision. The browser sometimes reports no type; the + server remains the authority. This is the opposite of what + `DotDropZoneComponent.typeMatch()` does, and the divergence is deliberate — see + [research.md R5](../research.md#r5--matching-a-file-against-image-and-the-unclassifiable-case). +3. **Never consult the filename extension.** An extension fallback is a hand-maintained type list, + forbidden by AC-002. + +Comparisons are case-insensitive. `family/*` matches on the family; anything else matches in full. + +### Label resolution + +The translate function is injected as a parameter rather than the module importing +`DotMessageService` — that is what keeps it pure and its spec harness-free. + +| Restriction | Label | +|---|---| +| `['image/*']` | *images* | +| `['video/*']` | *video files* | +| `['audio/*']` | *audio files* | +| Unknown family (e.g. `['application/pdf']`) | fall back to the raw patterns | +| Several families | the resolved labels, joined | +| `undefined` / `[]` | `undefined` | + +The fallback matters: a `browse` caller may pass anything, and a missing label must still produce a +correct message rather than *"Only can be uploaded here."* + +--- + +## C5 — What this fix must NOT change + +Explicit non-contract, since these are the AC-007 / AC-008 regression lines: + +| Surface | Guarantee | +|---|---| +| `file` mode | Every file type accepted, through all four upload routes. No `accept`. No toast. | +| `browse` mode, no caller types | Unrestricted, exactly as today. | +| Content Drive uploads | Unrestricted; same options, same copy, same toasts. | +| `DotUploadDropzoneComponent` | No API or behavior change. | +| `DotAssetPickerConfig` | No new field. | +| `DotUploadFileService` | Untouched — the guard lives in the picker, not the service. | +| Multi-file handling | The existing warn-and-upload-the-first behavior stands (tracked in #37166). | +| Server-side validation | Unchanged. This is a UX guard, not an enforcement boundary. | diff --git a/specs/37365-asset-picker-upload-scope/data-model.md b/specs/37365-asset-picker-upload-scope/data-model.md new file mode 100644 index 000000000000..48c61e358284 --- /dev/null +++ b/specs/37365-asset-picker-upload-scope/data-model.md @@ -0,0 +1,149 @@ +# Phase 1 Data Model: Asset Picker upload restriction + +**Feature**: `specs/37365-asset-picker-upload-scope` | **Date**: 2026-09-03 + +No persisted entity, no API payload and no store slice changes. The "data model" here is the +in-memory shape of the restriction as it travels from the entry point to each upload surface, plus +the message keys that describe it to the user. + +--- + +## 1. The restriction value + +### `mimeTypes: string[] | undefined` + +Already present on `DotAssetPickerConfig` +(`core-web/libs/ui/src/lib/components/dot-asset-picker/store/models.ts`). **No shape change.** This +plan only adds readers. + +| Property | Value | +|---|---| +| Origin | `buildAssetPickerConfig()` — from `ASSET_PICKER_MIME_TYPES[mode]`, or caller-supplied for `browse` | +| Read from | `store.config()?.mimeTypes` | +| Absent means | No restriction. `file` and `browse` (without caller-supplied types) produce `undefined` | +| Values today | `['image/*']`, `['video/*']`, `['audio/*']`, or arbitrary caller strings in `browse` | + +**Invariant**: absence, not a sentinel, is the unrestricted state. Every new reader must treat +`undefined` and `[]` identically and permissively. A reader that defaults to "restrict everything" +when the value is missing would break the File field (AC-007). + +### Derived: `acceptAttribute: string | null` + +| | | +|---|---| +| Derivation | `mimeTypes.join(',')`, or `null` when there is no restriction | +| Consumer | `[attr.accept]` on the hidden `` | +| Why `null`, not `''` | `[attr.accept]="null"` removes the attribute; `''` leaves an empty `accept`, which is not the same thing to the browser | + +### Derived: `restrictionLabel: string | undefined` + +The human name for the restriction, already translated. + +| | | +|---|---| +| Derivation | family (segment before `/`) of the `mimeTypes` entries → message key → translated string | +| Known families | `image`, `video`, `audio` | +| Unknown family | Fall back to listing the raw patterns, so the message is still correct if less friendly | +| Mixed families | Join the resolved labels; the picker produces single-family restrictions today, but a `browse` caller may pass several | +| Absent when | There is no restriction | +| Consumers | The refusal toast (AC-004), and the Asset/File prompt's scoped descriptions (AC-005) | + +--- + +## 2. Validation rules + +Applied by the pure matcher in `upload-restriction.ts`. Sourced from the spec's AC-004 and AC-010 +and the Q2 decision. + +| # | Input | Result | Source | +|---|---|---|---| +| V1 | No restriction (`undefined` / `[]`) | **Accept** | AC-007 | +| V2 | `file.type` empty or missing | **Accept** — the server remains the authority | AC-010 | +| V3 | Pattern `family/*`, `file.type` in that family | **Accept** | AC-001 | +| V4 | Pattern `family/*`, `file.type` in another family | **Reject** | AC-001 | +| V5 | Exact pattern (`application/pdf`), equal ignoring case | **Accept** | `browse` callers | +| V6 | Exact pattern, not equal | **Reject** | `browse` callers | +| V7 | Several patterns | **Accept** if any matches | — | + +**Deliberately not a rule**: the filename extension is never consulted. Adding an extension fallback +would reintroduce the hand-maintained list AC-002 forbids — see +[research.md R5](./research.md#r5--matching-a-file-against-image-and-the-unclassifiable-case). + +**Scope of the check**: the file that would actually be uploaded. The picker warns on a multi-file +selection and uploads only the first (unchanged, tracked separately in #37166), so the guard judges +that file. See spec Assumptions. + +--- + +## 3. State transitions — where the guard sits + +All four routes converge on `#resolveFilesUpload()`, which is the mandatory gate. `onRequestUpload()` +carries an early copy so a dropped file is refused before the user is asked to pick a storage type. + +```text + Upload button Drag and drop + │ │ + ▼ ▼ + onUpload() onRequestUpload() ◀── EARLY GUARD (AC-003) + │ │ + folder pins a base type? folder pins a base type? + │ │ + yes ├── fileInput.click() yes ──┼─────────────┐ + │ │ │ │ + no │ │ no │ │ + ▼ │ ▼ │ + Asset/File popover Asset/File modal │ + │ │ │ │ + ▼ │ ▼ │ + onUploadTypeSelected() ────────────────────┤ │ + │ │ │ │ + ├── fileInput.click() │ │ + │ │ │ │ + ▼ ▼ ▼ ▼ + onFileChange() ──────────────▶ #resolveFilesUpload() ◀── MANDATORY GUARD + │ (AC-004, AC-009) + ▼ + #uploadByBaseType() +``` + +**Rejected transition**: guard fails → no upload request, an error toast naming the allowed types, +and the picker stays open on the same folder. Nothing else is mutated: no `$activeSelection` left +dangling, no prompt opened, no list refresh. + +**Why both gates**: `#resolveFilesUpload()` alone would satisfy AC-004 and AC-009 for all four +routes, but a dropped PDF would first open the prompt and make the user choose a storage type before +being refused. The early gate is UX, not correctness — the mandatory one is the guarantee. + +--- + +## 4. Component contract deltas + +| Component | Change | Compatibility | +|---|---|---| +| `DotAssetPickerComponent` | Internal only — a computed `accept`, a computed label, a private guard | None public | +| `DotUploadTypeSelectorComponent` | **One new optional input**: the already-translated restriction label | Absent ⇒ today's exact option list and description keys. Content Drive passes nothing (AC-008) | +| `DotUploadDropzoneComponent` | **None** | Untouched by design ([R3](./research.md#r3--refusing-a-dropped-file-without-touching-the-shared-dropzone)) | +| `DotAssetPickerConfig` | **None** | No new field — the value already exists | + +The selector takes an already-translated string rather than a mode or a key so that +`DotMessageService` stays out of the shared component's new branch, and the picker remains the only +place that knows what its own restriction is called. + +--- + +## 5. Message keys + +New keys in `dotCMS/src/main/webapp/WEB-INF/messages/Language.properties`. Existing keys are **not** +edited — Content Drive keeps rendering them (AC-008). + +| Purpose | Count | Notes | +|---|---|---| +| Refusal toast summary + detail | 2 | Detail takes the restriction label as a parameter | +| Family labels (`image`, `video`, `audio`) | 3 | The human name; also feeds the scoped descriptions | +| Scoped option descriptions (Asset, File) | 2 | Parameterized with the family label; used only when the label input is set | + +Unchanged and still the default: `content-drive.dialog.upload-selector.asset.description`, +`content-drive.dialog.upload-selector.file.description`, and the existing upload toasts. + +Exact key names and wording are settled in +[contracts/upload-restriction.contract.md](./contracts/upload-restriction.contract.md). diff --git a/specs/37365-asset-picker-upload-scope/spec.md b/specs/37365-asset-picker-upload-scope/spec.md new file mode 100644 index 000000000000..43f7a755ef47 --- /dev/null +++ b/specs/37365-asset-picker-upload-scope/spec.md @@ -0,0 +1,252 @@ +# Issue Resolution Specification: Asset Picker — the upload flow ignores the field that opened the picker + +**Feature Branch**: `nicobytes/37365-asset-picker-scope-the-upload-flow-to-the-field-that-opened-the-picker` + +**Created**: 2026-09-03 + +**Status**: Draft + +**Type**: Issue / Bug Resolution + +**Related GitHub Issue**: dotCMS/core#37365 (split from dotCMS/core#37174, finding 8; parent epic dotCMS/core#36702) + +**Input**: User description: "https://github.com/dotCMS/core/issues/37365 — The Asset Picker already restricts what you can *browse* to the field that opened it, but not what you can *upload*. An Image field, or a Story Block `dotVideo` node, lets you upload any file type from inside the picker." + +## Problem Statement *(mandatory)* + +The Asset Picker is opened from a specific place: an Image field, a File field, a Story Block +image / video / audio node, or the generic browse entry point. It already knows which one, and it +uses that knowledge to narrow **what the editor can see** — an Image field lists only images, a +video node only video. + +It does not use that knowledge for **what the editor can add**. Every upload route inside the +picker is unrestricted: + +- the OS file dialog opens with no filter, so it offers every file on the machine; +- a file dragged onto the list is accepted whatever it is; +- the Asset / File prompt offers the same two options with the same wording in every mode, one of + which describes itself as being "for images, documents, and media" even when the picker was + opened for video only; +- nothing checks the file before the upload request is sent. + +The result is that an Image field can be made to hold a PDF and a video node an mp3 — the exact +outcome the browse-side restriction exists to prevent. Worse, the upload succeeds and then the new +asset **disappears**: the list is filtered by the mode, so the file the editor just uploaded is not +shown and cannot be selected. From the editor's point of view the upload silently did nothing, +while a stray file has in fact been written into the site's folder tree. + +**Severity / Impact**: Medium. Affects every author who uploads from inside the picker in a +media-scoped context — Image fields in the new Edit Content, and the Story Block image / video / +audio nodes. It is deterministic, not intermittent, and it is reachable by the ordinary +upload path, not an edge case. Two distinct harms: content typed as an image can end up pointing at +a non-image, and authors lose work to an upload that appears to do nothing. It also leaves orphan +files in the folder tree that nobody sees from the picker that created them. + +## Reproduction *(mandatory)* + +**Environment**: dotCMS `main` (includes PR #36848, merged as `8c725747c0`). New Edit Content UI, +any browser. No special configuration; a site the user can add content to. + +**Steps to Reproduce**: + +*Path A — Image field, OS file dialog:* + +1. Open a content type that has an **Image** field in the new Edit Content editor. +2. Open that field's Asset Picker. +3. Click **Upload** → the Asset / File prompt appears, with both options and their generic copy. +4. Pick either option → the OS file dialog opens with **no file-type filter**; every file on the + machine is selectable. +5. Choose a PDF (or a `.zip`) and confirm. + +*Path B — Image field, drag and drop:* + +1. Steps 1–2 above. +2. Drag a PDF from the desktop onto the picker's asset list and drop it. + +*Path C — Story Block media node:* + +1. In a Story Block field, type `/video` to open the video node's Asset Picker. +2. Repeat step 3–5 of Path A with an mp3 or a PDF. + +**Expected Behavior**: + +The picker offers only what the field can hold. In an Image field the OS dialog lists images only, +a dropped PDF is refused with a message saying which types are allowed, and no upload request is +sent for it. The Asset / File prompt describes the choice in terms that are true for the mode it +was opened in. Uploading an allowed file works as it does today, and the new asset appears in the +list ready to select. + +**Actual Behavior**: + +The OS dialog offers every file type; a dropped PDF is accepted; the upload request is sent and +succeeds; a success toast is shown; the list refreshes and the uploaded file is **not** in it, +because the browse filter excludes it. The file remains in the folder, unreferenced by the field +that created it. The generic **File** field behaves correctly, since it intentionally has no +restriction. + +**Reproducibility**: Always, in every media-scoped mode (`image`, `video`, `audio`), through all +three upload routes (Upload button → OS dialog, drag and drop, and a folder whose settings pin an +upload type and so skip the prompt). + +## Scope of Investigation *(mandatory)* + +- **Affected area**: Content authoring UI — the Asset Picker's upload flow, reached from the new + Edit Content Image / File fields and from the Story Block image / video / audio nodes. The + picker's shared upload building blocks (the drop zone and the Asset / File prompt) are also used + by Content Drive, which is a different host with no mode restriction. +- **Suspected surface**: Modern frontend only (`core-web`, `libs/ui` Asset Picker plus the shared + upload components, and the Story Block / Edit Content hosts that open the picker). No backend + change is expected: the server-side upload contract is unchanged, and this defect is about what + the UI offers and permits before the request is made. Confirmed during planning. +- **Related known decisions**: The restriction must come from the configuration the picker already + carries for browsing — the same per-mode mimetype narrowing — and not from a second, + independently maintained list of file types. This is the stated decision from refinement and is + binding on the fix. The plan formally consults `dotCMS/platform-adrs`. + +## Root-Cause Hypothesis + +The picker's entry point is translated into a browse restriction and nothing else. The +configuration built when the picker opens carries a per-mode mimetype narrowing, and every browse +request applies it — but no upload surface reads it: + +- the hidden file input that opens the OS dialog carries no `accept` attribute, so the dialog is + unfiltered; +- the drop zone is presentational and emits whatever was dropped, with no notion of allowed types; +- the Asset / File prompt renders a fixed, mode-independent list of options and copy; +- the upload handler goes straight from "files chosen" to "send the request", with no type check + in between. + +In other words the restriction exists in exactly one of the four places it needs to exist. The fix +is to make the same configured restriction the single source for all of them. + +A secondary consequence explains the "upload vanished" symptom: because the list is filtered on +the same restriction, any file that gets through the unrestricted upload is invisible in the list +that refreshes right after it. + +## Fix Scope & Non-Goals *(mandatory)* + +**In scope**: + +- Scoping every upload route inside the Asset Picker to the mode that opened it, derived from the + restriction the picker already carries for browsing: + - the OS file dialog is pre-filtered to the allowed types; + - a dropped file outside the allowed types is refused, with a message that says why; + - a file that reaches the upload handler anyway is refused before the request is sent, with a + message naming the allowed types; + - the Asset / File prompt keeps both storage options but describes them in wording that is true + for the mode it was opened in. +- Keeping the unrestricted modes unrestricted: the generic File field and the browse entry point + continue to accept every file type, unless the caller explicitly asked for a narrowing. +- Regression coverage for both halves — restriction applied per media mode, and no restriction + where none is configured. +- Any new user-facing wording added as translatable message keys. + +**Explicitly out of scope / non-goals**: + +- Server-side enforcement of the field's type. The upload endpoint keeps its current behavior; + this fix is about the picker offering and permitting only what the field can hold. Server-side + validation is a separate, larger change. +- Content Drive's own upload flow. It shares the drop zone and the Asset / File prompt, and must + keep accepting every file type; the shared components gain an optional restriction that Content + Drive does not set. +- The multi-file upload limitation. Today the picker warns and uploads only the first file of a + multi-file selection; that behavior is unchanged here and is covered by #37166. +- Repairing files already uploaded into the wrong place by this defect. No data migration. +- Widening or changing which base types the picker may browse, or the per-folder default + upload-type preference. +- The `accept`-style narrowing of any other upload surface in the product. + +## Regression Risk *(mandatory)* + +- **Blast radius**: + - The drop zone and the Asset / File prompt are shared with **Content Drive**. Any restriction + they gain must be opt-in, so Content Drive's uploads stay unrestricted. + - The picker's own **File field** and **browse** modes must keep accepting everything — an + over-reaching fix would block legitimate uploads of code, templates and documents. + - The **per-folder default upload type** path skips the Asset / File prompt entirely and goes + straight to the OS dialog; the restriction has to apply there too, or one of the three routes + stays open. + - Browse-mode callers that pass their own explicit mimetype narrowing (the legacy custom-field + browser entry point) would newly have that narrowing applied to uploads as well. +- **Backward compatibility**: No API, content, or persisted-state change. The picker's public + configuration shape gains no required field. Existing call sites that pass no restriction keep + today's behavior exactly. Not rollback-unsafe. +- **Data considerations**: None. Files already uploaded through the defect stay where they are and + remain reachable from Content Drive and the File field; nothing is moved or deleted. + +## Acceptance & Verification *(mandatory)* + +- **AC-001**: The reproduction steps above no longer produce the actual behavior. In an Image + field, the OS file dialog offers only image files; in a Story Block `video` node only video; in + an `audio` node only audio. +- **AC-002**: The restriction is derived from the configuration the picker already applies to + browsing. Adding a new media mode to that configuration restricts its uploads with no second + list to update, and no upload surface carries its own hard-coded list of types. +- **AC-003**: Dragging a file outside the allowed types onto the picker is refused: no upload + request is sent, and the user is told why, naming what is allowed. +- **AC-004**: A file that reaches the upload handler despite the dialog filter — the filter being a + hint the OS may let the user override — is refused before the upload request is sent, with a + message naming the allowed types. +- **AC-005**: The Asset / File prompt keeps offering both storage options in every mode, but its + wording reflects the mode it was opened in — no option's description promises types the mode does + not allow. In a video-only context, nothing in the prompt says "images, documents, and media". +- **AC-006**: After a successful upload in a media mode, the new asset appears in the list and can + be selected without reopening the picker. +- **AC-007** *(regression — no over-reach)*: The generic File field's picker still offers and + accepts every file type through all three upload routes, and the browse entry point is unchanged + unless its caller asked for a narrowing. +- **AC-008** *(regression — shared components)*: Content Drive's upload flow — Upload button, drag + and drop, and the Asset / File prompt — still accepts every file type and shows its current + wording. +- **AC-009**: The restriction applies on all three routes into an upload, including a folder whose + settings pin a default upload type and therefore skip the Asset / File prompt. +- **AC-010**: A file whose type the browser does not report is allowed through rather than refused + — the upload proceeds and the server remains the authority. The picker never blocks a file it + cannot classify. + +**Verification method**: + +- Frontend unit/component specs (Jest + Spectator) in the Asset Picker and the shared upload + components, covering per media mode: the pre-filtered dialog, the refused drop, the refused + pre-upload file, the prompt's mode-dependent copy, and the pinned-folder route + (AC-001 → AC-006, AC-009, AC-010). +- Frontend specs asserting the unrestricted modes and the Content Drive host are untouched + (AC-007, AC-008). +- Manual verification of the three reproduction paths above, plus the same three paths in a File + field and in Content Drive to confirm no over-reach. +- No backend test change expected; if planning finds a server-side element, integration coverage is + added then. + +## Assumptions + +- **Frontend-only fix.** The defect is that the UI offers what the field cannot hold. The upload + endpoint's behavior is treated as correct-as-is and out of scope; the picker is not made the + place where server-side type enforcement is introduced. +- **The three media modes are the whole restricted set.** `image`, `video` and `audio` are the + modes that carry a browse restriction today; `file` and `browse` carry none and stay + unrestricted. The fix keys off "does this mode carry a restriction", not off a list of mode + names, so a future media mode is covered automatically. +- **Browse-mode explicit narrowing also scopes uploads.** When the legacy custom-field browser + entry point asks for specific types, the same restriction applies to its uploads. This follows + from deriving the restriction from one configuration value rather than from the mode name, and is + the behavior a caller asking for "images only" would expect. +- **Multi-file drops are judged on what would actually be uploaded.** The picker already warns and + uploads only the first file of a multi-file selection. A drop whose uploaded file is outside the + allowed types is refused; the existing multi-file warning is unchanged. +- **The refusal message names the allowed types in user terms** (for example "images"), not raw + mimetype patterns, and is added as a translatable message key. +- **The restriction is expressed as the same broad type families the browse filter uses** + (images / video / audio), not an enumerated extension list — the issue explicitly rules out a + second hand-maintained list. +- **Both storage options stay on offer in every mode** *(decided during specification)*. Nothing + about this fix changes which base type an upload is stored as; it changes which *files* may be + uploaded. An Asset and a File can each legitimately hold an image, a video or an audio file, so + removing an option would take away a real choice. What changes in the prompt is the wording, not + the option list — the fix is scoped to correcting descriptions that over-promise, keeping it out + of the way of the per-folder default upload-type preference. +- **An unclassifiable file is allowed, not blocked** *(decided during specification)*. When the + browser reports no type for a file, the picker lets the upload proceed. The alternative — refusing + it — would occasionally block a legitimate image behind a message saying only images are allowed, + with no way forward; and closing the gap by inspecting filename extensions would reintroduce the + hand-maintained list the issue rules out. The residual exposure is narrow and the server remains + the authority. From 6d7ee824467edc56e5ecb91c8bb5444b124202d5 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Mon, 7 Sep 2026 09:25:12 -0400 Subject: [PATCH 2/2] Refactor DotContentDriveShellComponent tests to use a stub for $sidePanel signal - Updated test cases to utilize a stub function for the $sidePanel signal, improving clarity and reducing redundancy. - Enhanced type safety by explicitly casting the component for the spyOn method. - Added comments to clarify the purpose of the stub and its integration with the test logic. --- .../dot-content-drive-shell.component.spec.ts | 36 +++++++++++-------- .../dot-asset-picker.component.ts | 5 ++- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts index 10b288abcf27..f94a60ef151e 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts @@ -11,7 +11,7 @@ import { of, throwError } from 'rxjs'; import { Location } from '@angular/common'; import { provideHttpClient } from '@angular/common/http'; import { provideHttpClientTesting } from '@angular/common/http/testing'; -import { signal, WritableSignal } from '@angular/core'; +import { signal, Signal, WritableSignal } from '@angular/core'; import { By } from '@angular/platform-browser'; import { ActivatedRoute, Router } from '@angular/router'; @@ -2883,11 +2883,26 @@ describe('DotContentDriveShellComponent', () => { url: string; }) => void; - it('routes Back through the panel close guard (does not discard silently)', () => { + // The panel lives behind `@defer`, so the view child is not resolved synchronously — + // the tests stub the signal instead. `$sidePanel` is protected, so it is absent from + // the public type `jest.spyOn` infers its keys from; cast to the shape being stubbed. + const stubSidePanel = () => { const requestClose = jest.fn(); - jest.spyOn(spectator.component, '$sidePanel').mockReturnValue({ + + jest.spyOn( + spectator.component as unknown as { + $sidePanel: Signal; + }, + '$sidePanel' + ).mockReturnValue({ requestClose } as unknown as DotEditContentSidePanelComponent); + + return requestClose; + }; + + it('routes Back through the panel close guard (does not discard silently)', () => { + const requestClose = stubSidePanel(); setPanelRequest(EDIT_REQUEST); getPopstateHandler()({ url: '/c/content-drive?path=/foo' }); @@ -2900,10 +2915,7 @@ describe('DotContentDriveShellComponent', () => { }); it('keeps the panel open when Back preserves the same editContent param', () => { - const requestClose = jest.fn(); - jest.spyOn(spectator.component, '$sidePanel').mockReturnValue({ - requestClose - } as unknown as DotEditContentSidePanelComponent); + const requestClose = stubSidePanel(); setPanelRequest(EDIT_REQUEST); getPopstateHandler()({ url: '/c/content-drive?editContent=id-1' }); @@ -2913,10 +2925,7 @@ describe('DotContentDriveShellComponent', () => { }); it('routes Back through the guard for an open new-mode panel too (AC8)', () => { - const requestClose = jest.fn(); - jest.spyOn(spectator.component, '$sidePanel').mockReturnValue({ - requestClose - } as unknown as DotEditContentSidePanelComponent); + const requestClose = stubSidePanel(); setPanelRequest({ mode: 'new', contentTypeId: 'ct-1', title: 'New content' }); // Back removed the `new` marker entirely — the popstate handler must still close @@ -2929,10 +2938,7 @@ describe('DotContentDriveShellComponent', () => { }); it('keeps a new-mode panel open when Back preserves the editContent=new marker', () => { - const requestClose = jest.fn(); - jest.spyOn(spectator.component, '$sidePanel').mockReturnValue({ - requestClose - } as unknown as DotEditContentSidePanelComponent); + const requestClose = stubSidePanel(); setPanelRequest({ mode: 'new', contentTypeId: 'ct-1', title: 'New content' }); getPopstateHandler()({ url: '/c/content-drive?editContent=new' }); diff --git a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts index 2d4999e81a76..7d77053bd167 100644 --- a/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts +++ b/core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts @@ -20,6 +20,7 @@ import { Dialog, DialogModule } from 'primeng/dialog'; import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; import { Popover, PopoverModule } from 'primeng/popover'; import { SplitterModule } from 'primeng/splitter'; +import type { SplitterPassThrough } from 'primeng/types/splitter'; import { DotContentletService, @@ -154,8 +155,10 @@ export class DotAssetPickerComponent implements OnInit { * The legacy theme gives `.p-splitter` a gray border and a radius, which read as a stray box * inside a dialog that already has its own chrome. The gutter keeps its own styling. */ - protected readonly splitterPt = { + protected readonly splitterPt: SplitterPassThrough = { root: { class: 'border-0! rounded-none!' }, + // PrimeNG types `panel` as required, so it has to be listed even with nothing to pass. + panel: {}, gutterHandle: { 'aria-label': this.#dotMessageService.get('dot.asset.picker.splitter.aria') }