Permissions: Only check for document permissions when verifying entity actions for a document (closes #20097)

[AndyButland]

Prerequisites

• I have added steps to test this contribution in the description below

Addresses: #20097

Description

Although I haven't been able to replicate the reported issue, I can see the proposed solution is sensible, as it allows us to by-pass complex logic for verifying document specific permissions, when we aren't working with a document.

So I've applied that here.

Testing

To verify, use a set up such as:

• Create a user in only the "Editors" group
• Add a document specific permission to a document that removes some permissions that the editor normally has.
• Verify when expanding the entity actions menu that the default editor permissions appear on documents without the document specific permission, but that the restricted set appears on those that have it defined
• Verify that default editor permissions appear on all entity action lists that aren't based on documents (e.g. media, members, dictionary).

[Copilot]

Pull Request Overview

This PR fixes a bug where document-specific permissions were being checked for all entity types, not just documents. The fix ensures that complex document permission logic is only executed when the entity type is actually a document.

Key Changes

• Added entity type check before evaluating document permissions
• Simplified the code structure by removing an unnecessary conditional block
• Fixed comment capitalization for consistency

[nielslyngsoe]

Just a short update on the status: I made this one Draft based on a Conversation between me & Mads. We think the issue is mistreading the intention of the Condition. Mads is working on providing the right fix and will return to the conversation when its ready.

For now the concern is that the Document Permission Condition is targeted for Documents, hence the name, and therefore we should not make that more flexible. The name Document is in the name and we would hope that would make the use-case clear, but we properly have to ensure that it fails in other cases to avoid this misunderstanding.

Instead, we should provide a Generic User Entity Permission Condition ( or similar name) that will resolve this use case.

[ronaldbarendse]

Regardless of whether the actual issue can be reproduced, these changes do make sense: only check document-specific permissions if the entity type is actually a document 😄

I can see why making the condition more 'flexible' might be a concern, but the alternative would be to conditionally apply conditions based on entity type 🙃

Take the following 'Queue for transfer' entity action that's registered by Deploy:

const entityAction: ManifestEntityAction = {
  type: "entityAction",
  kind: "default",
  alias: "Deploy.EntityAction.Queue",
  name: "Deploy Queue Entity Action",
  weight: 95,
  api: () => import("./queue.action.js"),
  forEntityTypes: [
    "document-root",
    "document",
    "document-blueprint-root",
    "document-blueprint",
    "media-root",
    "media",
    "member-root",
    "member",
    "member-group-root",
    "member-group",
  ],
  meta: {
    icon: "icon-cloud-upload",
    label: "#deploy_queueForTransfer_label",
  },
  conditions: [
    {
      alias: UMB_DOCUMENT_USER_PERMISSION_CONDITION_ALIAS,
      allOf: [DEPLOY_USER_PERMISSION_QUEUE_FOR_TRANSFER],
    },
    { alias: DEPLOY_QUEUE_CONDITION },
  ],
};

This entity action requires the user to have the correct permission and the environment to support transferring items to a next environment. For documents, this can be a document-specific permission (but can fallback to the generic ones); all other entity types should always use the generic fallback permissions.

However, if we want to add e.g. entity-specific permissions for forms (or folders) in Umbraco Forms Deploy, this would already break if you don't have the generic fallback permission. So maybe instead of using the fallback permissions for non-document entity types, it should actually completely skip the check and have another condition handle the entity-specific form/folder permission check? For backwards compatibility though, this behavior might need to be configurable (default to using fallback permissions, but allow skipping for non-document entity types)...

[madsrasmussen]

Hi,

I have opened a PR with an alternative solution that does more or less what @ronaldbarendse mentions in his comment. We need to split the condition into two separate conditions. The Document condition is meant for Documents 😃

See more details here: #20097 (comment)

[AndyButland]

Closing now since superseded by #20224.