diff --git a/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs b/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs index 686433c42984..94047b46c8e0 100644 --- a/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs +++ b/src/Umbraco.Cms.Api.Management/Services/Entities/UserStartNodeEntitiesService.cs @@ -142,7 +142,8 @@ public IEnumerable ChildUserAccessEntities(IEnumerable path.StartsWith(child.Path))) + // Note: Add trailing comma to prevent false matches (e.g., path "-1,100" should not match "-1,1001") + if (userStartNodePaths.Any(path => path.StartsWith($"{child.Path},"))) { return new UserAccessEntity(child, false); } @@ -220,5 +221,7 @@ public IEnumerable UserAccessEntities(IEnumerable => entities.Select(entity => new UserAccessEntity(entity, IsDescendantOrSelf(entity, userStartNodePaths))).ToArray(); private static bool IsDescendantOrSelf(IEntitySlim child, string[] userStartNodePaths) - => userStartNodePaths.Any(path => child.Path.StartsWith(path)); + // Note: Add trailing commas to both paths to prevent false matches (e.g., path "-1,100" should not match "-1,1001") + // This matches the pattern used in lines 92 and 192 of this file + => userStartNodePaths.Any(path => $"{child.Path},".StartsWith($"{path},")); } diff --git a/src/Umbraco.Web.UI.Client/src/mocks/data/document/data/permissions-test.data.ts b/src/Umbraco.Web.UI.Client/src/mocks/data/document/data/permissions-test.data.ts index 5bb1d07ec635..89f34cf86941 100644 --- a/src/Umbraco.Web.UI.Client/src/mocks/data/document/data/permissions-test.data.ts +++ b/src/Umbraco.Web.UI.Client/src/mocks/data/document/data/permissions-test.data.ts @@ -40,6 +40,34 @@ const permissionsTestDocument = { export const data: Array = [ permissionsTestDocument, + { + ...permissionsTestDocument, + ancestors: [{ id: 'permissions-document-id' }], + hasChildren: false, + id: 'permissions-0-document-id', + parent: { id: 'permissions-document-id' }, + variants: permissionsTestDocument.variants.map((variant) => ({ + ...variant, + name: 'Permissions 0 - No Access', + id: 'permissions-0', + })), + flags: [], + noAccess: true, + }, + { + ...permissionsTestDocument, + ancestors: [{ id: 'permissions-0-document-id' }], + hasChildren: false, + id: 'permissions-0-1-document-id', + parent: { id: 'permissions-0-document-id' }, + variants: permissionsTestDocument.variants.map((variant) => ({ + ...variant, + name: 'Permissions 0.1 - Has access', + id: 'permissions-0-1', + })), + flags: [], + noAccess: false, + }, { ...permissionsTestDocument, ancestors: [{ id: 'permissions-document-id' }], diff --git a/src/Umbraco.Web.UI.Client/src/mocks/data/media/media.data.ts b/src/Umbraco.Web.UI.Client/src/mocks/data/media/media.data.ts index 6b8519e6d1db..7b2fb3282fac 100644 --- a/src/Umbraco.Web.UI.Client/src/mocks/data/media/media.data.ts +++ b/src/Umbraco.Web.UI.Client/src/mocks/data/media/media.data.ts @@ -44,6 +44,43 @@ export const data: Array = [ ], flags: [], }, + { + hasChildren: false, + id: 'f2f81a40-c989-4b6b-84e2-057cecd3grd4', + createDate: '2023-02-06T15:32:05.350038', + parent: null, + noAccess: true, + isTrashed: false, + mediaType: { + id: 'media-type-1-id', + icon: 'icon-picture', + }, + values: [ + { + editorAlias: 'Umbraco.UploadField', + alias: 'mediaPicker', + value: { + src: '/umbraco/backoffice/assets/installer-illustration.svg', + }, + }, + { + editorAlias: 'Umbraco.TextBox', + alias: 'mediaType1Property1', + value: 'The daily life at Umbraco HQ', + }, + ], + variants: [ + { + publishDate: '2023-02-06T15:31:51.354764', + culture: null, + segment: null, + name: 'Permissions - No Access', + createDate: '2023-02-06T15:31:46.876902', + updateDate: '2023-02-06T15:31:51.354764', + }, + ], + flags: [], + }, { hasChildren: false, id: '69431027-8867-45bf-a93b-72bbdabfb177', diff --git a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts index 296409029889..0b283bda7232 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/core/tree/tree-item/tree-item-base/tree-item-element-base.ts @@ -34,6 +34,29 @@ export abstract class UmbTreeItemElementBase< } protected _item?: TreeItemModelType; + /** + * @internal + * Indicates whether the user has no access to this tree item. + * This property is reflected as an attribute for styling purposes. + * + * **Usage Pattern (opt-in):** + * Child classes that support access restrictions should observe their context's `noAccess` observable + * and update this property. The base class provides the property, styling, and interaction prevention, + * but does not subscribe to the observable to avoid forcing all tree item types to implement it. + * + * **Example (in child class api setter):** + * ```typescript + * this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess)); + * ``` + * + * **Why not in the base interface?** + * Adding `noAccess` to `UmbTreeItemContext` would be a breaking change, forcing all tree item + * implementations (users, members, data types, etc.) to provide this property even when access + * restrictions don't apply to them. + */ + @property({ type: Boolean, reflect: true, attribute: 'no-access' }) + protected _noAccess = false; + /** * @param item - The item from which to extract flags. * @description This method is called whenever the `item` property is set. It extracts the flags from the item and assigns them to the `_flags` state property. @@ -205,7 +228,7 @@ export abstract class UmbTreeItemElementBase< @selected=${this._handleSelectedItem} @deselected=${this._handleDeselectedItem} ?active=${this._isActive} - ?disabled=${this._isSelectableContext && !this._isSelectable} + ?disabled=${(this._isSelectableContext && !this._isSelectable) || this._noAccess} ?selectable=${this._isSelectable} ?selected=${this._isSelected} .loading=${this._isLoading} @@ -213,7 +236,7 @@ export abstract class UmbTreeItemElementBase< .showChildren=${this._isOpen} .caretLabel=${this.localize.term(caretLabelKey) + ' ' + this._label} label=${ifDefined(this._label)} - href=${ifDefined(this._isSelectableContext ? undefined : this._href)} + href=${ifDefined(this._isSelectableContext || this._noAccess ? undefined : this._href)} .renderExpandSymbol=${this._renderExpandSymbol}> ${this.#renderLoadPrevButton()} ${this.renderIconContainer()} ${this.renderLabel()} ${this.#renderActions()} ${this.#renderChildItems()} diff --git a/src/Umbraco.Web.UI.Client/src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts b/src/Umbraco.Web.UI.Client/src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts index 31abdd7b3e44..958ceef2ae11 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/documents/documents/tree/tree-item/document-tree-item.element.ts @@ -11,16 +11,12 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase< #api: UmbDocumentTreeItemContext | undefined; @property({ type: Object, attribute: false }) - public override get api(): UmbDocumentTreeItemContext | undefined { - return this.#api; - } public override set api(value: UmbDocumentTreeItemContext | undefined) { this.#api = value; if (this.#api) { this.observe(this.#api.name, (name) => (this._name = name || '')); this.observe(this.#api.isDraft, (isDraft) => (this._isDraft = isDraft || false)); - this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess || false)); this.observe(this.#api.hasCollection, (has) => { const oldValue = this._forceShowExpand; this._forceShowExpand = has; @@ -28,10 +24,16 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase< }); this.observe(this.#api.icon, (icon) => (this.#icon = icon || '')); this.observe(this.#api.flags, (flags) => (this._flags = flags || '')); + // Observe noAccess from context and update base class property (_noAccess). + // This enables access restriction behavior (click prevention) and styling from the base class. + this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess)); } super.api = value; } + public override get api(): UmbDocumentTreeItemContext | undefined { + return this.#api; + } @state() private _name = ''; @@ -43,27 +45,8 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase< @property({ type: Boolean, reflect: true, attribute: 'draft' }) protected _isDraft = false; - /** - * @internal - * Indicates whether the user has no access to this document, this is controlled internally but present as an attribute as it affects styling. - */ - @property({ type: Boolean, reflect: true, attribute: 'no-access' }) - protected _noAccess = false; - #icon: string | null | undefined; - constructor() { - super(); - this.addEventListener('click', this.#handleClick); - } - - #handleClick = (event: MouseEvent) => { - if (this._noAccess) { - event.preventDefault(); - event.stopPropagation(); - } - }; - // eslint-disable-next-line @typescript-eslint/no-unused-vars protected override _extractFlags(item: UmbDocumentTreeItemModel | undefined) { // Empty on purpose and NOT calling super to prevent doing what the base does. [NL] @@ -85,7 +68,9 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase< }; override renderLabel() { - return html`${this._name} `; + return html` + ${this._name} + `; } static override styles = [ @@ -97,16 +82,6 @@ export class UmbDocumentTreeItemElement extends UmbTreeItemElementBase< :host([draft]) umb-icon { opacity: 0.6; } - :host([no-access]) { - cursor: not-allowed; - } - :host([no-access]) #label { - opacity: 0.6; - font-style: italic; - } - :host([no-access]) umb-icon { - opacity: 0.6; - } `, ]; } diff --git a/src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts b/src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts index a751cbb36cf9..b106c310439f 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/documents/documents/workspace/document-workspace-editor.element.ts @@ -72,6 +72,7 @@ export class UmbDocumentWorkspaceEditorElement extends UmbLitElement { #generateRoutes() { if (!this.#variants || this.#variants.length === 0 || !this.#appCulture) { this._routes = []; + this.#ensureForbiddenRoute(this._routes); return; } @@ -143,6 +144,17 @@ export class UmbDocumentWorkspaceEditorElement extends UmbLitElement { }); } + this.#ensureForbiddenRoute(routes); + + this._routes = routes; + } + + /** + * Ensure that there is a route to handle forbidden access. + * This route will display a forbidden message when the user does not have permission to access certain resources. + * Also handles not found routes. + */ + #ensureForbiddenRoute(routes: Array = []) { routes.push({ path: '**', component: async () => { @@ -150,8 +162,6 @@ export class UmbDocumentWorkspaceEditorElement extends UmbLitElement { return this.#isForbidden ? router.UmbRouteForbiddenElement : router.UmbRouteNotFoundElement; }, }); - - this._routes = routes; } private _gotWorkspaceRoute = (e: UmbRouterSlotInitEvent) => { diff --git a/src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts b/src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts index b38900f860ec..34311c7106bc 100644 --- a/src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts +++ b/src/Umbraco.Web.UI.Client/src/packages/media/media/tree/tree-item/media-tree-item.element.ts @@ -1,47 +1,18 @@ import type { UmbMediaTreeItemModel } from '../types.js'; import type { UmbMediaTreeItemContext } from './media-tree-item.context.js'; -import { css, html, customElement, nothing, property } from '@umbraco-cms/backoffice/external/lit'; -import { UmbTextStyles } from '@umbraco-cms/backoffice/style'; +import { css, html, customElement, nothing, classMap } from '@umbraco-cms/backoffice/external/lit'; import { UmbTreeItemElementBase } from '@umbraco-cms/backoffice/tree'; const elementName = 'umb-media-tree-item'; @customElement(elementName) export class UmbMediaTreeItemElement extends UmbTreeItemElementBase { - #api: UmbMediaTreeItemContext | undefined; - - @property({ type: Object, attribute: false }) - public override get api(): UmbMediaTreeItemContext | undefined { - return this.#api; - } public override set api(value: UmbMediaTreeItemContext | undefined) { - this.#api = value; - - if (this.#api) { - this.observe(this.#api.noAccess, (noAccess) => (this._noAccess = noAccess || false)); - } - + // Observe noAccess from context and update base class property (_noAccess). + // This enables access restriction behavior (click prevention) and styling from the base class. + this.observe(value?.noAccess, (noAccess) => (this._noAccess = noAccess ?? false)); super.api = value; } - /** - * @internal - * Indicates whether the user has no access to this media item, this is controlled internally but present as an attribute as it affects styling. - */ - @property({ type: Boolean, reflect: true, attribute: 'no-access' }) - protected _noAccess = false; - - constructor() { - super(); - this.addEventListener('click', this.#handleClick); - } - - #handleClick = (event: MouseEvent) => { - if (this._noAccess) { - event.preventDefault(); - event.stopPropagation(); - } - }; - override renderIconContainer() { const icon = this.item?.mediaType.icon; @@ -58,7 +29,9 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase${this._item?.variants[0].name} `; + return html` + ${this._item?.variants[0].name} + `; } #renderStateIcon() { @@ -74,7 +47,7 @@ export class UmbMediaTreeItemElement extends UmbTreeItemElementBase { +test.beforeEach(async ({ umbracoApi }) => { await umbracoApi.documentType.ensureNameNotExists(rootDocumentTypeName); await umbracoApi.documentType.ensureNameNotExists(childDocumentTypeOneName); await umbracoApi.documentType.ensureNameNotExists(childDocumentTypeTwoName); await umbracoApi.user.ensureNameNotExists(testUser.name); await umbracoApi.userGroup.ensureNameNotExists(userGroupName); - childDocumentTypeOneId = await umbracoApi.documentType.createDefaultDocumentType(childDocumentTypeOneName); - const childDocumentTypeTwoId = await umbracoApi.documentType.createDefaultDocumentType(childDocumentTypeTwoName); - rootDocumentTypeId = await umbracoApi.documentType.createDocumentTypeWithAllowedTwoChildNodes(rootDocumentTypeName, childDocumentTypeOneId, childDocumentTypeTwoId); - rootDocumentId = await umbracoApi.document.createDefaultDocument(rootDocumentName, rootDocumentTypeId); - childDocumentOneId = await umbracoApi.document.createDefaultDocumentWithParent(childDocumentOneName, childDocumentTypeOneId, rootDocumentId); - await umbracoApi.document.createDefaultDocumentWithParent(childDocumentTwoName, childDocumentTypeTwoId, rootDocumentId); - userGroupId = await umbracoApi.userGroup.createSimpleUserGroupWithContentSection(userGroupName); + childDocumentTypeOneId = + await umbracoApi.documentType.createDefaultDocumentType( + childDocumentTypeOneName + ); + const childDocumentTypeTwoId = + await umbracoApi.documentType.createDefaultDocumentType( + childDocumentTypeTwoName + ); + rootDocumentTypeId = + await umbracoApi.documentType.createDocumentTypeWithAllowedTwoChildNodes( + rootDocumentTypeName, + childDocumentTypeOneId, + childDocumentTypeTwoId + ); + rootDocumentId = await umbracoApi.document.createDefaultDocument( + rootDocumentName, + rootDocumentTypeId + ); + childDocumentOneId = + await umbracoApi.document.createDefaultDocumentWithParent( + childDocumentOneName, + childDocumentTypeOneId, + rootDocumentId + ); + await umbracoApi.document.createDefaultDocumentWithParent( + childDocumentTwoName, + childDocumentTypeTwoId, + rootDocumentId + ); + userGroupId = + await umbracoApi.userGroup.createSimpleUserGroupWithContentSection( + userGroupName + ); }); -test.afterEach(async ({umbracoApi}) => { +test.afterEach(async ({ umbracoApi }) => { // Ensure we are logged in to admin - await umbracoApi.loginToAdminUser(testUserCookieAndToken.cookie, testUserCookieAndToken.accessToken, testUserCookieAndToken.refreshToken); + await umbracoApi.loginToAdminUser( + testUserCookieAndToken.cookie, + testUserCookieAndToken.accessToken, + testUserCookieAndToken.refreshToken + ); await umbracoApi.documentType.ensureNameNotExists(rootDocumentTypeName); await umbracoApi.documentType.ensureNameNotExists(childDocumentTypeOneName); await umbracoApi.documentType.ensureNameNotExists(childDocumentTypeTwoName); await umbracoApi.userGroup.ensureNameNotExists(userGroupName); }); -test('can see root start node and children', async ({umbracoApi, umbracoUi}) => { +test("can see root start node and children", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId, [rootDocumentId]); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId, + [rootDocumentId] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act @@ -53,33 +97,111 @@ test('can see root start node and children', async ({umbracoApi, umbracoUi}) => // Assert await umbracoUi.content.isContentInTreeVisible(rootDocumentName); await umbracoUi.content.openContentCaretButtonForName(rootDocumentName); - await umbracoUi.content.isChildContentInTreeVisible(rootDocumentName, childDocumentOneName); - await umbracoUi.content.isChildContentInTreeVisible(rootDocumentName, childDocumentTwoName); + await umbracoUi.content.isChildContentInTreeVisible( + rootDocumentName, + childDocumentOneName + ); + await umbracoUi.content.isChildContentInTreeVisible( + rootDocumentName, + childDocumentTwoName + ); }); -// Skip this test due to this issue: https://github.com/umbraco/Umbraco-CMS/issues/20505 -test.skip('can see parent of start node but not access it', async ({umbracoApi, umbracoUi}) => { +test("can see parent of start node but not access it", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId, [childDocumentOneId]); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId, + [childDocumentOneId] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act await umbracoUi.user.goToSection(ConstantHelper.sections.content, false); // Assert + + // Get initial URL (should be on content section) + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + const initialUrl = umbracoUi.page.url(); + await umbracoUi.content.isContentInTreeVisible(rootDocumentName); await umbracoUi.content.goToContentWithName(rootDocumentName); - await umbracoUi.content.doesDocumentWorkspaceHaveText('Access denied'); + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + + // Assert - URL should not have changed (no navigation occurred) + const currentUrl = umbracoUi.page.url(); + expect(currentUrl).toBe(initialUrl); + await umbracoUi.content.openContentCaretButtonForName(rootDocumentName); - await umbracoUi.content.isChildContentInTreeVisible(rootDocumentName, childDocumentOneName); - await umbracoUi.content.isChildContentInTreeVisible(rootDocumentName, childDocumentTwoName, false); + await umbracoUi.content.isChildContentInTreeVisible( + rootDocumentName, + childDocumentOneName + ); + await umbracoUi.content.isChildContentInTreeVisible( + rootDocumentName, + childDocumentTwoName, + false + ); +}); + +test("see no-access view when deep-linking to restricted document", async ({ + umbracoApi, + umbracoUi, +}) => { + // Arrange + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId, + [childDocumentOneId] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); + await umbracoUi.goToBackOffice(); + + // Act + await umbracoUi.user.goToSection(ConstantHelper.sections.content, false); + + // Assert + await umbracoUi.content.isContentInTreeVisible(rootDocumentName); + await umbracoUi.page.goto( + `${umbracoUi.page.url()}/workspace/document/edit/${rootDocumentId!}` + ); + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + await umbracoUi.content.doesDocumentWorkspaceHaveText("Access denied"); }); -test('can not see any content when no start nodes specified', async ({umbracoApi, umbracoUi}) => { +test("can not see any content when no start nodes specified", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act diff --git a/tests/Umbraco.Tests.AcceptanceTest/tests/DefaultConfig/Users/Permissions/User/MediaStartNodes.spec.ts b/tests/Umbraco.Tests.AcceptanceTest/tests/DefaultConfig/Users/Permissions/User/MediaStartNodes.spec.ts index ae7949b647e2..d026e9222c15 100644 --- a/tests/Umbraco.Tests.AcceptanceTest/tests/DefaultConfig/Users/Permissions/User/MediaStartNodes.spec.ts +++ b/tests/Umbraco.Tests.AcceptanceTest/tests/DefaultConfig/Users/Permissions/User/MediaStartNodes.spec.ts @@ -1,32 +1,48 @@ -import {ConstantHelper, test} from '@umbraco/playwright-testhelpers'; +import { expect } from "@playwright/test"; +import { ConstantHelper, test } from "@umbraco/playwright-testhelpers"; const testUser = ConstantHelper.testUserCredentials; -let testUserCookieAndToken = {cookie: "", accessToken: "", refreshToken: ""}; +let testUserCookieAndToken = { cookie: "", accessToken: "", refreshToken: "" }; -const userGroupName = 'TestUserGroup'; +const userGroupName = "TestUserGroup"; let userGroupId = null; let rootFolderId = null; let childFolderOneId = null; -const rootFolderName = 'RootFolder'; -const childFolderOneName = 'ChildFolderOne'; -const childFolderTwoName = 'ChildFolderTwo'; +const rootFolderName = "RootFolder"; +const childFolderOneName = "ChildFolderOne"; +const childFolderTwoName = "ChildFolderTwo"; -test.beforeEach(async ({umbracoApi}) => { +test.beforeEach(async ({ umbracoApi }) => { await umbracoApi.user.ensureNameNotExists(testUser.name); await umbracoApi.userGroup.ensureNameNotExists(userGroupName); await umbracoApi.media.ensureNameNotExists(rootFolderName); await umbracoApi.media.ensureNameNotExists(childFolderOneName); await umbracoApi.media.ensureNameNotExists(childFolderTwoName); - rootFolderId = await umbracoApi.media.createDefaultMediaFolder(rootFolderName); - childFolderOneId = await umbracoApi.media.createDefaultMediaFolderAndParentId(childFolderOneName, rootFolderId); - await umbracoApi.media.createDefaultMediaFolderAndParentId(childFolderTwoName, rootFolderId); - userGroupId = await umbracoApi.userGroup.createSimpleUserGroupWithMediaSection(userGroupName); + rootFolderId = await umbracoApi.media.createDefaultMediaFolder( + rootFolderName + ); + childFolderOneId = await umbracoApi.media.createDefaultMediaFolderAndParentId( + childFolderOneName, + rootFolderId + ); + await umbracoApi.media.createDefaultMediaFolderAndParentId( + childFolderTwoName, + rootFolderId + ); + userGroupId = + await umbracoApi.userGroup.createSimpleUserGroupWithMediaSection( + userGroupName + ); }); -test.afterEach(async ({umbracoApi}) => { +test.afterEach(async ({ umbracoApi }) => { // Ensure we are logged in to admin - await umbracoApi.loginToAdminUser(testUserCookieAndToken.cookie, testUserCookieAndToken.accessToken, testUserCookieAndToken.refreshToken); + await umbracoApi.loginToAdminUser( + testUserCookieAndToken.cookie, + testUserCookieAndToken.accessToken, + testUserCookieAndToken.refreshToken + ); await umbracoApi.user.ensureNameNotExists(testUser.name); await umbracoApi.userGroup.ensureNameNotExists(userGroupName); await umbracoApi.media.ensureNameNotExists(rootFolderName); @@ -34,10 +50,25 @@ test.afterEach(async ({umbracoApi}) => { await umbracoApi.media.ensureNameNotExists(childFolderTwoName); }); -test('can see root media start node and children', async ({umbracoApi, umbracoUi}) => { +test("can see root media start node and children", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId, [], false, [rootFolderId]); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId, + [], + false, + [rootFolderId] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act @@ -50,30 +81,102 @@ test('can see root media start node and children', async ({umbracoApi, umbracoUi await umbracoUi.media.isChildMediaVisible(rootFolderName, childFolderTwoName); }); -// Skip this test due to this issue: https://github.com/umbraco/Umbraco-CMS/issues/20505 -test.skip('can see parent of start node but not access it', async ({umbracoApi, umbracoUi}) => { +test("can see parent of start node but not access it", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId, [], false, [childFolderOneId]); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId!, + [], + false, + [childFolderOneId!] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act await umbracoUi.user.goToSection(ConstantHelper.sections.media, false); // Assert + + // Get initial URL (should be on media section) + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + const initialUrl = umbracoUi.page.url(); + await umbracoUi.media.isMediaTreeItemVisible(rootFolderName); await umbracoUi.media.goToMediaWithName(rootFolderName); - await umbracoUi.waitForTimeout(ConstantHelper.wait.short); // Wait for workspace to load - await umbracoUi.media.doesMediaWorkspaceHaveText('Access denied'); + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + + // Assert - URL should not have changed (no navigation occurred) + const currentUrl = umbracoUi.page.url(); + expect(currentUrl).toBe(initialUrl); + await umbracoUi.media.openMediaCaretButtonForName(rootFolderName); await umbracoUi.media.isChildMediaVisible(rootFolderName, childFolderOneName); - await umbracoUi.media.isChildMediaVisible(rootFolderName, childFolderTwoName, false); + await umbracoUi.media.isChildMediaVisible( + rootFolderName, + childFolderTwoName, + false + ); +}); + +test("see no-access view when deep-linking to restricted media", async ({ + umbracoApi, + umbracoUi, +}) => { + // Arrange + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId!, + [], + false, + [childFolderOneId!] + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); + await umbracoUi.goToBackOffice(); + + // Act + await umbracoUi.user.goToSection(ConstantHelper.sections.media, false); + + // Assert + await umbracoUi.media.isMediaTreeItemVisible(rootFolderName); + await umbracoUi.page.goto( + `${umbracoUi.page.url()}/workspace/media/edit/${rootFolderId!}` + ); + await umbracoUi.waitForTimeout(100); // Wait for workspace to load + await umbracoUi.media.doesMediaWorkspaceHaveText("Access denied"); }); -test('can not see any media when no media start nodes specified', async ({umbracoApi, umbracoUi}) => { +test("can not see any media when no media start nodes specified", async ({ + umbracoApi, + umbracoUi, +}) => { // Arrange - await umbracoApi.user.setUserPermissions(testUser.name, testUser.email, testUser.password, userGroupId); - testUserCookieAndToken = await umbracoApi.user.loginToUser(testUser.name, testUser.email, testUser.password); + await umbracoApi.user.setUserPermissions( + testUser.name, + testUser.email, + testUser.password, + userGroupId + ); + testUserCookieAndToken = await umbracoApi.user.loginToUser( + testUser.name, + testUser.email, + testUser.password + ); await umbracoUi.goToBackOffice(); // Act diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Services/UserStartNodePathComparisonTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Services/UserStartNodePathComparisonTests.cs new file mode 100644 index 000000000000..63c5cf1d09e5 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Management/Services/UserStartNodePathComparisonTests.cs @@ -0,0 +1,151 @@ +using NUnit.Framework; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Cms.Api.Management.Services; + +/// +/// Tests for the path comparison logic used in UserStartNodeEntitiesService. +/// These tests document the fix for the path comparison bug where node IDs +/// that are numeric prefixes of other IDs (e.g., 100 vs 1001) would incorrectly match. +/// +[TestFixture] +public class UserStartNodePathComparisonTests +{ + /// + /// Tests the IsDescendantOrSelf path comparison logic with trailing commas. + /// This prevents false matches when one ID is a numeric prefix of another. + /// + /// The fix: Add trailing commas to both paths before comparison: + /// - Without fix: "-1,100".StartsWith("-1,10") = true ❌ (incorrect match) + /// - With fix: "-1,100,".StartsWith("-1,10,") = false ✅ (correct) + /// + [Test] + [TestCase( + "-1,100", + "-1,1001", + ExpectedResult = false, + TestName = "Should not match when child ID 100 is prefix of start node ID 1001")] + [TestCase( + "-1,1001", + "-1,100", + ExpectedResult = false, + TestName = "Should not match when start node ID 100 is prefix of child ID 1001")] + [TestCase( + "-1,10", + "-1,100", + ExpectedResult = false, + TestName = "Should not match when child ID 10 is prefix of start node ID 100")] + [TestCase( + "-1,100", + "-1,10", + ExpectedResult = false, + TestName = "Should not match when start node ID 10 is prefix of child ID 100")] + [TestCase( + "-1,1", + "-1,10", + ExpectedResult = false, + TestName = "Should not match when child ID 1 is prefix of start node ID 10")] + [TestCase( + "-1,10", + "-1,1", + ExpectedResult = false, + TestName = "Should not match when start node ID 1 is prefix of child ID 10")] + [TestCase( + "-1,1", + "-1,100", + ExpectedResult = false, + TestName = "Should not match when child ID 1 is prefix of start node ID 100")] + [TestCase( + "-1,100", + "-1,100", + ExpectedResult = true, + TestName = "Should match when paths are identical (self case)")] + [TestCase( + "-1,100,200", + "-1,100", + ExpectedResult = true, + TestName = "Should match when child is descendant of start node")] + [TestCase( + "-1,100,200,300", + "-1,100", + ExpectedResult = true, + TestName = "Should match when child is deep descendant of start node")] + [TestCase( + "-1,100", + "-1,100,200", + ExpectedResult = false, + TestName = "Should not match when child is ancestor of start node (reversed relationship)")] + [TestCase( + "-1,100", + "-1,100,200,300", + ExpectedResult = false, + TestName = "Should not match when child is deep ancestor of start node (reversed relationship)")] + public bool IsDescendantOrSelf_WithTrailingCommas_PreventsNumericPrefixMatches(string childPath, string startNodePath) + { + // This implements the same logic as UserStartNodeEntitiesService.IsDescendantOrSelf + // Adding trailing commas to both paths ensures accurate comparison + var childPathWithComma = $"{childPath},"; + var startNodePathWithComma = $"{startNodePath},"; + + return childPathWithComma.StartsWith(startNodePathWithComma); + } + + /// + /// Demonstrates the bug that existed before the fix. + /// Without trailing commas, numeric prefix IDs incorrectly match. + /// + [Test] + [TestCase( + "-1,100", + "-1,10", + ExpectedResult = true, + TestName = "BUG: Without trailing commas, 100 incorrectly matches 10")] + [TestCase( + "-1,1001", + "-1,100", + ExpectedResult = true, + TestName = "BUG: Without trailing commas, 1001 incorrectly matches 100")] + [TestCase( + "-1,10", + "-1,1", + ExpectedResult = true, + TestName = "BUG: Without trailing commas, 10 incorrectly matches 1")] + public bool WithoutTrailingCommas_CausesFalseMatches_DocumentsBug(string childPath, string startNodePath) + { + // This demonstrates the bug: without trailing commas, we get false matches + // These tests document WHY the fix with trailing commas is necessary + return childPath.StartsWith(startNodePath); + } + + /// + /// Tests edge cases with multi-level paths to ensure the fix works + /// at different tree depths. + /// + [Test] + [TestCase( + "-1,100,200,300", + "-1,10,20,30", + ExpectedResult = false, + TestName = "Deep paths with prefix IDs should not match")] + [TestCase( + "-1,1,10,100,1000", + "-1,1,10,100", + ExpectedResult = true, + TestName = "Should match when child is descendant despite multiple prefix IDs")] + [TestCase( + "-1,1,10,100", + "-1,1,10,100", + ExpectedResult = true, + TestName = "Deep paths that are identical should match (self case)")] + [TestCase( + "-1,1,10,100", + "-1,1,10,100,1000", + ExpectedResult = false, + TestName = "Should not match when child is ancestor of start node")] + public bool DeepPaths_WorkCorrectly_WithTrailingCommas(string childPath, string startNodePath) + { + var childPathWithComma = $"{childPath},"; + var startNodePathWithComma = $"{startNodePath},"; + + return childPathWithComma.StartsWith(startNodePathWithComma); + } +}