From 56b14060a1b1a83106ae731e4de09901587ca877 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Mon, 10 Aug 2026 12:43:45 -0600 Subject: [PATCH 1/3] fix(permissions): pattern-switch resolvePermissionType and stop it throwing on a typeless contentlet (#34154) resolvePermissionType was an if/else-if chain of thirteen instanceof tests, and the first condition alone cast the same reference three times: if (permissionable instanceof Host || (permissionable instanceof Contentlet && ((Contentlet) permissionable).getStructure() != null && ((Contentlet) permissionable).getStructure().getVelocityVarName() != null && ((Contentlet) permissionable).getStructure().getVelocityVarName().equals("Host"))) It is now a pattern switch: the type test binds the value once, and the extra conditions move into `when` guards. The nested chain for the Identifier fallback moves to its own method, and `case null` replaces the `if (perm != null)` wrapper. Behaviour change, deliberate: a contentlet with no resolvable content type no longer throws. Contentlet.getStructure() returns null when getContentType() is null. The first branch checked for that; the FILEASSET branch right after it did not, and read getStructure().getStructureType() directly. So a typeless contentlet failed the first guard correctly and then NPE'd on the second. Verified by running the original condition verbatim against such a contentlet: NullPointerException: Cannot invoke "Structure.getStructureType()" because the return value of "Contentlet.getStructure()" is null The null check is now in one shared helper, so both call sites get it. Two branches of the original chain are gone, as provably redundant rather than as a judgement call. They mapped a FILEASSET contentlet, and an Event, to Contentlet.class.getCanonicalName() - which is exactly what Contentlet.getPermissionType() already returns (Contentlet.java:1186), and no Contentlet subclass overrides it. Both inputs now reach `default` and produce the same string. The FILEASSET branch was also the one that could throw, so it was spending a crash on a value that was already correct. Case order is load bearing and is documented in the javadoc: Host extends Contentlet, so it has to match first, and the Host-content-type check has to precede the IHTMLPage one. Ordering the other way is not a silent bug - the compiler rejects a dominated case - but only for unguarded patterns, so the comment earns its place. resolvePermissionType widened from private to package-private @VisibleForTesting. It had no direct coverage before. Tests: 11 new unit tests in PermissionBitFactoryImplResolvePermissionTypeTest, covering every branch of the chain plus both trailing overrides. Assets are mocked because instantiating a real Contentlet runs a static initialiser that reaches CDI. Where the expected answer is "the asset's own declared type" the mock declares a sentinel, so a fall-through to `default` is distinguishable from a branch that happened to produce the same value - without that, the two redundancy tests would have asserted a string they themselves stubbed. Co-Authored-By: Claude Opus 5 (1M context) --- .../business/PermissionBitFactoryImpl.java | 177 ++++++++++---- ...tFactoryImplResolvePermissionTypeTest.java | 217 ++++++++++++++++++ 2 files changed, 351 insertions(+), 43 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java diff --git a/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java index 106d773d00e1..0b170db2ba1e 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java @@ -15,6 +15,7 @@ import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.contenttype.transform.contenttype.StructureTransformer; import com.dotcms.rendering.velocity.viewtools.navigation.NavResult; +import com.google.common.annotations.VisibleForTesting; import com.google.common.primitives.Ints; import com.dotcms.system.SimpleMapAppContext; import com.dotmarketing.beans.Host; @@ -1825,56 +1826,146 @@ private Tuple2> _loadParentPermissions(final Per } - private String resolvePermissionType(final Permissionable permissionable) { - // Need to determine who this asset should inherit from - String type = permissionable.getPermissionType(); - if (permissionable instanceof Host || (permissionable instanceof Contentlet && ((Contentlet) permissionable).getStructure() != null - && ((Contentlet) permissionable).getStructure().getVelocityVarName() != null - && ((Contentlet) permissionable).getStructure().getVelocityVarName().equals("Host"))) { - type = Host.class.getCanonicalName(); - } else if (permissionable instanceof Contentlet - && BaseContentType.FILEASSET.getType() == ((Contentlet) permissionable).getStructure().getStructureType()) { - type = Contentlet.class.getCanonicalName(); - } else if (permissionable instanceof IHTMLPage || (permissionable instanceof Contentlet - && BaseContentType.HTMLPAGE.getType() == ((Contentlet) permissionable).getStructure().getStructureType())) { - type = IHTMLPage.class.getCanonicalName(); - } else if (permissionable instanceof Event) { - type = Contentlet.class.getCanonicalName(); - } else if (permissionable instanceof Identifier) { - Permissionable perm = InodeFactory.getInode(permissionable.getPermissionId(), Inode.class); - Logger.error(this, - "PermissionBitFactoryImpl : loadPermissions Method : was passed an identifier. This is a problem. We will get inode as a fallback but this should be reported"); - if (perm != null) { - if (perm instanceof IHTMLPage || (perm instanceof Contentlet - && BaseContentType.HTMLPAGE.getType() == ((Contentlet) perm).getStructure().getStructureType())) { - type = IHTMLPage.class.getCanonicalName(); - } else if (perm instanceof Container) { - type = Container.class.getCanonicalName(); - } else if (perm instanceof Folder) { - type = Folder.class.getCanonicalName(); - } else if (perm instanceof Link) { - type = Link.class.getCanonicalName(); - } else if (perm instanceof Template) { - type = Template.class.getCanonicalName(); - } else if (perm instanceof Structure || perm instanceof ContentType) { - type = Structure.class.getCanonicalName(); - } else if (perm instanceof Contentlet || perm instanceof Event) { - type = Contentlet.class.getCanonicalName(); - } - } - } - - if (permissionable instanceof Template && UtilMethods.isSet(((Template) permissionable).isDrawed()) - && ((Template) permissionable).isDrawed()) { + /** + * Determines the permission type an asset inherits under — the key that + * {@code permission_reference} rows are stored and looked up against. + * + *

Two families of rule apply, in this order: the type the asset inherits by virtue of + * what it is ({@link #inheritablePermissionType(Permissionable)}), and then two overrides + * that depend on the asset's state rather than its type — a drawn {@link Template} inherits + * as a {@link TemplateLayout}, and a {@link NavResult} defers to whatever it encloses.

+ * + * @param permissionable the asset whose inheritance key is being resolved + * @return the fully qualified permission type name + */ + @VisibleForTesting + String resolvePermissionType(final Permissionable permissionable) { + + String type = inheritablePermissionType(permissionable); + + if (permissionable instanceof Template template + && UtilMethods.isSet(template.isDrawed()) + && template.isDrawed()) { type = TemplateLayout.class.getCanonicalName(); } - if (permissionable instanceof NavResult) { - type = ((NavResult) permissionable).getEnclosingPermissionClassName(); + if (permissionable instanceof NavResult navResult) { + type = navResult.getEnclosingPermissionClassName(); } + return type; } + /** + * The type an asset inherits permissions under, derived purely from what the asset is. + * + *

Case order is significant and mirrors the {@code if / else if} chain this + * replaced: {@link Host} extends {@link Contentlet}, so it has to be matched first, and the + * Host-content-type check has to precede the {@link IHTMLPage} one.

+ * + *

Two branches of the original chain are deliberately absent, because they were provably + * redundant: they mapped a {@code FILEASSET} contentlet, and an {@link Event}, to + * {@code Contentlet.class.getCanonicalName()} — which is exactly what + * {@link Contentlet#getPermissionType()} already returns, and no {@code Contentlet} subclass + * overrides it. Both now fall through to {@code default} and produce the same string. The + * {@code FILEASSET} branch was also the one that read {@code getStructure().getStructureType()} + * without the null check its neighbour performed, so it could throw while computing a value that + * was already correct.

+ * + * @param permissionable the asset whose inheritance key is being resolved + * @return the fully qualified permission type name, defaulting to the asset's own declared type + */ + private String inheritablePermissionType(final Permissionable permissionable) { + + return switch (permissionable) { + + case Host _ -> Host.class.getCanonicalName(); + + case Contentlet contentlet when isOfContentType(contentlet, Host.HOST_VELOCITY_VAR_NAME) + -> Host.class.getCanonicalName(); + + case IHTMLPage _ -> IHTMLPage.class.getCanonicalName(); + + case Contentlet contentlet when isOfBaseType(contentlet, BaseContentType.HTMLPAGE) + -> IHTMLPage.class.getCanonicalName(); + + case Identifier identifier -> permissionTypeOfBackingInode(identifier); + + default -> permissionable.getPermissionType(); + }; + } + + /** + * Resolves the permission type for an {@link Identifier}, which should never reach the permission + * loader in the first place — the caller logs that as a defect and falls back to the inode the + * identifier points at. + * + * @param identifier the identifier that was passed in by mistake + * @return the backing inode's permission type, or the identifier's own when the inode cannot be + * resolved or is of no recognised kind + */ + private String permissionTypeOfBackingInode(final Identifier identifier) { + + final Permissionable inode = InodeFactory.getInode(identifier.getPermissionId(), Inode.class); + + Logger.error(this, + "PermissionBitFactoryImpl : loadPermissions Method : was passed an identifier. This is a problem. We will get inode as a fallback but this should be reported"); + + return switch (inode) { + + case null -> identifier.getPermissionType(); + + case IHTMLPage _ -> IHTMLPage.class.getCanonicalName(); + + case Contentlet contentlet when isOfBaseType(contentlet, BaseContentType.HTMLPAGE) + -> IHTMLPage.class.getCanonicalName(); + + case Container _ -> Container.class.getCanonicalName(); + + case Folder _ -> Folder.class.getCanonicalName(); + + case Link _ -> Link.class.getCanonicalName(); + + case Template _ -> Template.class.getCanonicalName(); + + case Structure _, ContentType _ -> Structure.class.getCanonicalName(); + + case Contentlet _ -> Contentlet.class.getCanonicalName(); + + default -> identifier.getPermissionType(); + }; + } + + /** + * Whether the contentlet's content type is the one with the given velocity variable name. + * + * @param contentlet the contentlet to inspect + * @param variableName the velocity variable name to match + * @return {@code false} when the contentlet has no resolvable content type + */ + private static boolean isOfContentType(final Contentlet contentlet, final String variableName) { + + final Structure structure = contentlet.getStructure(); + return structure != null && variableName.equals(structure.getVelocityVarName()); + } + + /** + * Whether the contentlet's content type has the given base type. + * + *

{@link Contentlet#getStructure()} returns {@code null} when the contentlet has no resolvable + * content type, so the null check is load bearing — reading the structure type straight off it is + * what made the original chain throw.

+ * + * @param contentlet the contentlet to inspect + * @param baseType the base type to match + * @return {@code false} when the contentlet has no resolvable content type + */ + private static boolean isOfBaseType(final Contentlet contentlet, final BaseContentType baseType) { + + final Structure structure = contentlet.getStructure(); + return structure != null && baseType.getType() == structure.getStructureType(); + } + diff --git a/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java new file mode 100644 index 000000000000..12ea5a36dfde --- /dev/null +++ b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java @@ -0,0 +1,217 @@ +package com.dotmarketing.business; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.rendering.velocity.viewtools.navigation.NavResult; +import com.dotmarketing.beans.Host; +import com.dotmarketing.portlets.calendar.model.Event; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.htmlpageasset.model.IHTMLPage; +import com.dotmarketing.portlets.structure.model.Structure; +import com.dotmarketing.portlets.templates.design.bean.TemplateLayout; +import com.dotmarketing.portlets.templates.model.Template; +import org.junit.Test; + +/** + * Pins the behaviour of {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} — the + * permission-inheritance key an asset is stored and looked up under. + * + *

The method was an {@code if / else if} chain of thirteen {@code instanceof} tests with repeated + * casts; it is now a pattern {@code switch}. It had no direct coverage before, which is why this suite + * exists: it is the evidence the conversion preserved every mapping, and it pins the one behaviour that + * deliberately changed.

+ * + *

Assets are mocked rather than constructed: instantiating a real {@link Contentlet} runs a static + * initialiser that reaches CDI (`OSIndexAPIImpl`), which no plain unit test has. Mocking costs nothing + * here — every assertion is about which branch the resolver takes, not about how the asset was built.

+ * + *

Where the expected answer is "the asset's own declared type", the mock declares + * {@link #DECLARED_TYPE} instead of the real value. That keeps the test from asserting a string it + * itself stubbed: seeing the sentinel come back out proves the resolver fell through to + * {@code default} rather than matching a branch that happened to produce the same value.

+ * + * @author Fabrizio Araya + */ +public class PermissionBitFactoryImplResolvePermissionTypeTest { + + private static final String HOST_TYPE = Host.class.getCanonicalName(); + private static final String HTML_PAGE_TYPE = IHTMLPage.class.getCanonicalName(); + + /** Stands in for whatever the asset declares, so a fall-through is distinguishable from a match. */ + private static final String DECLARED_TYPE = "com.example.DeclaredByTheAssetItself"; + + private final PermissionBitFactoryImpl factory = + new PermissionBitFactoryImpl(mock(PermissionCache.class)); + + /** + * A contentlet whose content type resolves, with the given velocity variable name and base type. + */ + private Contentlet contentletOf(final String velocityVarName, final BaseContentType baseType) { + final Structure structure = mock(Structure.class); + when(structure.getVelocityVarName()).thenReturn(velocityVarName); + when(structure.getStructureType()).thenReturn(baseType.getType()); + + final Contentlet contentlet = mock(Contentlet.class); + when(contentlet.getStructure()).thenReturn(structure); + when(contentlet.getPermissionType()).thenReturn(DECLARED_TYPE); + return contentlet; + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a contentlet with no resolvable content type, so {@code getStructure()} is null. + * Expected result: it falls through to the declared type instead of throwing. + * + *

This is the behaviour that deliberately changed. The old chain read + * {@code getStructure().getStructureType()} in the FILEASSET branch without the null check its + * neighbouring branch performed, and {@link Contentlet#getStructure()} returns {@code null} when the + * contentlet has no resolvable content type — so this input threw a {@link NullPointerException}.

+ */ + @Test + public void test_contentletWithNoContentType_doesNotThrow() { + final Contentlet contentlet = mock(Contentlet.class); + when(contentlet.getStructure()).thenReturn(null); + when(contentlet.getPermissionType()).thenReturn(DECLARED_TYPE); + + assertEquals(DECLARED_TYPE, factory.resolvePermissionType(contentlet)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a {@link Host}, which is itself a {@link Contentlet} subclass. + * Expected result: resolves as a Host. Order-sensitive — Host has to be matched before Contentlet, + * or the guarded Contentlet cases would claim it first. + */ + @Test + public void test_host_resolvesAsHost() { + assertEquals(HOST_TYPE, factory.resolvePermissionType(mock(Host.class))); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a plain contentlet whose content type is the Host content type. + * Expected result: resolves as a Host, even though the object is not a {@link Host} instance. + */ + @Test + public void test_contentletOfHostContentType_resolvesAsHost() { + final Contentlet contentlet = + contentletOf(Host.HOST_VELOCITY_VAR_NAME, BaseContentType.CONTENT); + + assertEquals(HOST_TYPE, factory.resolvePermissionType(contentlet)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link IHTMLPage}. + * Expected result: resolves as an HTML page. + */ + @Test + public void test_htmlPage_resolvesAsHtmlPage() { + assertEquals(HTML_PAGE_TYPE, factory.resolvePermissionType(mock(IHTMLPage.class))); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a contentlet whose base type is HTMLPAGE but which is not an {@link IHTMLPage}. + * Expected result: resolves as an HTML page, through the guarded case rather than the type case. + */ + @Test + public void test_contentletOfHtmlPageBaseType_resolvesAsHtmlPage() { + final Contentlet contentlet = contentletOf("myPage", BaseContentType.HTMLPAGE); + + assertEquals(HTML_PAGE_TYPE, factory.resolvePermissionType(contentlet)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a contentlet whose base type is FILEASSET. + * Expected result: the declared type, by falling through to {@code default}. + * + *

The original chain had an explicit branch mapping this to + * {@code Contentlet.class.getCanonicalName()}, which is precisely what + * {@link Contentlet#getPermissionType()} already returns — and no {@code Contentlet} subclass + * overrides it. The branch was therefore assigning the value the variable already held, so it was + * dropped. The sentinel is what makes this test meaningful: it shows the resolver now reaches + * {@code default} rather than matching a branch.

+ */ + @Test + public void test_contentletOfFileAssetBaseType_fallsThroughToDeclaredType() { + final Contentlet contentlet = contentletOf("fileAsset", BaseContentType.FILEASSET); + + assertEquals(DECLARED_TYPE, factory.resolvePermissionType(contentlet)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Event}, which extends {@link Contentlet}. + * Expected result: the declared type. This is the second branch dropped for the same reason — it + * mapped Events to {@code Contentlet.class.getCanonicalName()}, which an Event already declares + * through the accessor it inherits from {@link Contentlet}. + */ + @Test + public void test_event_fallsThroughToDeclaredType() { + final Event event = mock(Event.class); + when(event.getPermissionType()).thenReturn(DECLARED_TYPE); + + assertEquals(DECLARED_TYPE, factory.resolvePermissionType(event)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an ordinary contentlet of a custom content type. + * Expected result: the declared type, via {@code default}. + */ + @Test + public void test_ordinaryContentlet_fallsThroughToDeclaredType() { + final Contentlet contentlet = contentletOf("myBlogPost", BaseContentType.CONTENT); + + assertEquals(DECLARED_TYPE, factory.resolvePermissionType(contentlet)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a template that has not been drawn. + * Expected result: the declared type — the {@link TemplateLayout} override must not fire. + */ + @Test + public void test_templateNotDrawn_fallsThroughToDeclaredType() { + final Template template = mock(Template.class); + when(template.isDrawed()).thenReturn(Boolean.FALSE); + when(template.getPermissionType()).thenReturn(DECLARED_TYPE); + + assertEquals(DECLARED_TYPE, factory.resolvePermissionType(template)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a drawn template. + * Expected result: resolves as a {@link TemplateLayout}. This override runs after the type-based + * resolution and replaces its result. + */ + @Test + public void test_templateDrawn_resolvesAsTemplateLayout() { + final Template template = mock(Template.class); + when(template.isDrawed()).thenReturn(Boolean.TRUE); + when(template.getPermissionType()).thenReturn(DECLARED_TYPE); + + assertEquals(TemplateLayout.class.getCanonicalName(), + factory.resolvePermissionType(template)); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: a {@link NavResult}. + * Expected result: it defers to whatever it encloses, overriding the type-based resolution. + */ + @Test + public void test_navResult_defersToEnclosingType() { + final NavResult navResult = mock(NavResult.class); + when(navResult.getPermissionType()).thenReturn(DECLARED_TYPE); + when(navResult.getEnclosingPermissionClassName()).thenReturn("com.example.Enclosing"); + + assertEquals("com.example.Enclosing", factory.resolvePermissionType(navResult)); + } +} From 312dc9ff2f228c201a370245376cafccc3124207 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Mon, 10 Aug 2026 15:34:45 -0600 Subject: [PATCH 2/3] refactor(permissions): drop the unreachable cases from the Identifier fallback (#34154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The switch in permissionTypeOfBackingInode held its subject as a Permissionable, but InodeFactory.getInode can only ever hand back an Inode. Widening the local was the only reason five cases compiled at all: Contentlet, Folder and ContentType are classes with no kinship to Inode, so no object could ever be both, and the Permissionable interface they share does not change that — Java has single class inheritance. Those branches were dead in the original if / else if chain too; the pattern switch is what made them visible. Typing the local as Inode is the part that lasts: the compiler now rejects a case that cannot match, instead of leaving it to an IDE inspection. The IHTMLPage branch stays as a defence — it is an interface, so a future Inode subclass could implement it — with a comment saying why it is currently unreachable. The null case folds into the default arm, which already returns the same value. Also reads the content type off Contentlet#getContentType() rather than the deprecated getStructure(), which merely wraps it in a StructureTransformer, and compares base types by enum identity instead of by int. The pinning test follows the accessor the code actually reads. Co-Authored-By: Claude Opus 5 (1M context) --- .../business/PermissionBitFactoryImpl.java | 38 +++++++++---------- ...tFactoryImplResolvePermissionTypeTest.java | 23 +++++------ 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java index 0b170db2ba1e..a7c66b1478d9 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java @@ -1900,39 +1900,41 @@ case Contentlet contentlet when isOfBaseType(contentlet, BaseContentType.HTMLPAG * loader in the first place — the caller logs that as a defect and falls back to the inode the * identifier points at. * + *

The lookup can only ever hand back an {@link Inode}, which is why the local is typed as one + * rather than as {@link Permissionable}: it lets the compiler reject a case that cannot match. + * The branches the original {@code if / else if} chain spent on {@link Contentlet}, + * {@link Folder} and {@link ContentType} are gone for exactly that reason — each is a class with + * no kinship to {@code Inode}, so no object could ever be both, and the shared + * {@code Permissionable} interface does not change that. {@link IHTMLPage} is kept as a defensive + * branch instead: it is an interface, so a future {@code Inode} subclass could implement it.

+ * * @param identifier the identifier that was passed in by mistake * @return the backing inode's permission type, or the identifier's own when the inode cannot be * resolved or is of no recognised kind */ private String permissionTypeOfBackingInode(final Identifier identifier) { - final Permissionable inode = InodeFactory.getInode(identifier.getPermissionId(), Inode.class); + final Inode inode = InodeFactory.getInode(identifier.getPermissionId(), Inode.class); Logger.error(this, "PermissionBitFactoryImpl : loadPermissions Method : was passed an identifier. This is a problem. We will get inode as a fallback but this should be reported"); return switch (inode) { - case null -> identifier.getPermissionType(); - + // No Inode implements IHTMLPage today — the only implementation, HTMLPageAsset, is a + // Contentlet — so this branch is unreachable. It is kept because IHTMLPage is an interface + // and a future Inode subclass could implement it, which is also why the compiler allows it. case IHTMLPage _ -> IHTMLPage.class.getCanonicalName(); - case Contentlet contentlet when isOfBaseType(contentlet, BaseContentType.HTMLPAGE) - -> IHTMLPage.class.getCanonicalName(); - case Container _ -> Container.class.getCanonicalName(); - case Folder _ -> Folder.class.getCanonicalName(); - case Link _ -> Link.class.getCanonicalName(); case Template _ -> Template.class.getCanonicalName(); - case Structure _, ContentType _ -> Structure.class.getCanonicalName(); - - case Contentlet _ -> Contentlet.class.getCanonicalName(); + case Structure _ -> Structure.class.getCanonicalName(); - default -> identifier.getPermissionType(); + case null, default -> identifier.getPermissionType(); }; } @@ -1944,15 +1946,14 @@ case Contentlet contentlet when isOfBaseType(contentlet, BaseContentType.HTMLPAG * @return {@code false} when the contentlet has no resolvable content type */ private static boolean isOfContentType(final Contentlet contentlet, final String variableName) { - - final Structure structure = contentlet.getStructure(); - return structure != null && variableName.equals(structure.getVelocityVarName()); + final ContentType contentType = contentlet.getContentType(); + return contentType != null && variableName.equals(contentType.variable()); } /** * Whether the contentlet's content type has the given base type. * - *

{@link Contentlet#getStructure()} returns {@code null} when the contentlet has no resolvable + *

{@link Contentlet#getContentType()} returns {@code null} when the contentlet has no resolvable * content type, so the null check is load bearing — reading the structure type straight off it is * what made the original chain throw.

* @@ -1961,9 +1962,8 @@ private static boolean isOfContentType(final Contentlet contentlet, final String * @return {@code false} when the contentlet has no resolvable content type */ private static boolean isOfBaseType(final Contentlet contentlet, final BaseContentType baseType) { - - final Structure structure = contentlet.getStructure(); - return structure != null && baseType.getType() == structure.getStructureType(); + final ContentType contentType = contentlet.getContentType(); + return contentType != null && baseType == contentType.baseType(); } diff --git a/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java index 12ea5a36dfde..7fe0aa2f5d2d 100644 --- a/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java +++ b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java @@ -5,12 +5,12 @@ import static org.mockito.Mockito.when; import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.rendering.velocity.viewtools.navigation.NavResult; import com.dotmarketing.beans.Host; import com.dotmarketing.portlets.calendar.model.Event; import com.dotmarketing.portlets.contentlet.model.Contentlet; import com.dotmarketing.portlets.htmlpageasset.model.IHTMLPage; -import com.dotmarketing.portlets.structure.model.Structure; import com.dotmarketing.portlets.templates.design.bean.TemplateLayout; import com.dotmarketing.portlets.templates.model.Template; import org.junit.Test; @@ -47,33 +47,34 @@ public class PermissionBitFactoryImplResolvePermissionTypeTest { new PermissionBitFactoryImpl(mock(PermissionCache.class)); /** - * A contentlet whose content type resolves, with the given velocity variable name and base type. + * A contentlet whose content type resolves, with the given variable name and base type. */ - private Contentlet contentletOf(final String velocityVarName, final BaseContentType baseType) { - final Structure structure = mock(Structure.class); - when(structure.getVelocityVarName()).thenReturn(velocityVarName); - when(structure.getStructureType()).thenReturn(baseType.getType()); + private Contentlet contentletOf(final String variableName, final BaseContentType baseType) { + final ContentType contentType = mock(ContentType.class); + when(contentType.variable()).thenReturn(variableName); + when(contentType.baseType()).thenReturn(baseType); final Contentlet contentlet = mock(Contentlet.class); - when(contentlet.getStructure()).thenReturn(structure); + when(contentlet.getContentType()).thenReturn(contentType); when(contentlet.getPermissionType()).thenReturn(DECLARED_TYPE); return contentlet; } /** * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} - * Given scenario: a contentlet with no resolvable content type, so {@code getStructure()} is null. + * Given scenario: a contentlet with no resolvable content type, so {@code getContentType()} is null. * Expected result: it falls through to the declared type instead of throwing. * *

This is the behaviour that deliberately changed. The old chain read * {@code getStructure().getStructureType()} in the FILEASSET branch without the null check its - * neighbouring branch performed, and {@link Contentlet#getStructure()} returns {@code null} when the - * contentlet has no resolvable content type — so this input threw a {@link NullPointerException}.

+ * neighbouring branch performed, and {@link Contentlet#getContentType()} returns {@code null} when + * the contentlet has no resolvable content type — so this input threw a + * {@link NullPointerException}.

*/ @Test public void test_contentletWithNoContentType_doesNotThrow() { final Contentlet contentlet = mock(Contentlet.class); - when(contentlet.getStructure()).thenReturn(null); + when(contentlet.getContentType()).thenReturn(null); when(contentlet.getPermissionType()).thenReturn(DECLARED_TYPE); assertEquals(DECLARED_TYPE, factory.resolvePermissionType(contentlet)); From ad7b1db72b2de9ff032cbe33c82ab6edff6a21fc Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Mon, 10 Aug 2026 16:11:26 -0600 Subject: [PATCH 3/3] test(permissions): cover the Identifier fallback of resolvePermissionType (#34154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nested chain that resolves an Identifier through the inode it points at had no coverage at all: reaching it means going through the static InodeFactory.getInode, so every claim about those branches came from reading the code rather than running it. Seven tests intercept that lookup with mockStatic and pin each arm. Two are worth calling out. The IHTMLPage arm is the branch kept as a defence rather than deleted, and no Inode implements that interface today — the mock is built with extraInterfaces to produce the combination at all, which is precisely the shape a future subclass would take. The null arm pins that `case null, default` is load bearing: a switch over patterns throws on a null selector unless a case null says otherwise, and getInode returns null when the lookup fails. The Template and Structure arms are unreachable in production, because InodeFactory throws for those row types instead of returning them. Their tests say so, and pin the mapping for the day that changes. Verified the coverage is not vacuous: removing the IHTMLPage arm fails exactly one test, and the sentinel in the failure message shows the resolver fell through to default. Co-Authored-By: Claude Opus 5 (1M context) --- ...tFactoryImplResolvePermissionTypeTest.java | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java index 7fe0aa2f5d2d..e98fc6353409 100644 --- a/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java +++ b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java @@ -2,18 +2,27 @@ import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; import com.dotcms.contenttype.model.type.BaseContentType; import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.rendering.velocity.viewtools.navigation.NavResult; import com.dotmarketing.beans.Host; +import com.dotmarketing.beans.Identifier; +import com.dotmarketing.beans.Inode; +import com.dotmarketing.factories.InodeFactory; import com.dotmarketing.portlets.calendar.model.Event; +import com.dotmarketing.portlets.containers.model.Container; import com.dotmarketing.portlets.contentlet.model.Contentlet; import com.dotmarketing.portlets.htmlpageasset.model.IHTMLPage; +import com.dotmarketing.portlets.links.model.Link; +import com.dotmarketing.portlets.structure.model.Structure; import com.dotmarketing.portlets.templates.design.bean.TemplateLayout; import com.dotmarketing.portlets.templates.model.Template; import org.junit.Test; +import org.mockito.MockedStatic; /** * Pins the behaviour of {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} — the @@ -215,4 +224,121 @@ public void test_navResult_defersToEnclosingType() { assertEquals("com.example.Enclosing", factory.resolvePermissionType(navResult)); } + + // --- The Identifier fallback ------------------------------------------------------------- + // + // An Identifier should never reach the permission loader; when one does, the resolver logs it + // as a defect and resolves the inode it points at instead. Reaching that code means going + // through the static InodeFactory.getInode, so these tests intercept it. + + private static final String BACKING_INODE_ID = "e7b4c0de-0000-0000-0000-000000000001"; + + /** + * Resolves an identifier whose permission id is backed by the given inode, with + * {@link InodeFactory} intercepted so no database is involved. + * + * @param backingInode what the lookup hands back, {@code null} for an id that resolves to nothing + * @param expected the permission type the resolver is expected to produce + */ + private void assertIdentifierBackedBy(final Inode backingInode, final String expected) { + final Identifier identifier = mock(Identifier.class); + when(identifier.getPermissionId()).thenReturn(BACKING_INODE_ID); + when(identifier.getPermissionType()).thenReturn(DECLARED_TYPE); + + try (MockedStatic inodeFactory = mockStatic(InodeFactory.class)) { + inodeFactory.when(() -> InodeFactory.getInode(BACKING_INODE_ID, Inode.class)) + .thenReturn(backingInode); + + assertEquals(expected, factory.resolvePermissionType(identifier)); + } + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at a {@link Container}. + * Expected result: resolves as a Container. + */ + @Test + public void test_identifierBackedByContainer_resolvesAsContainer() { + assertIdentifierBackedBy(mock(Container.class), Container.class.getCanonicalName()); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at a {@link Link}. + * Expected result: resolves as a Link. + */ + @Test + public void test_identifierBackedByLink_resolvesAsLink() { + assertIdentifierBackedBy(mock(Link.class), Link.class.getCanonicalName()); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at a {@link Template}. + * Expected result: resolves as a Template. + * + *

Production cannot currently reach this branch: {@link InodeFactory} throws for a + * {@code template} row rather than returning one. The mapping is pinned anyway, so the branch is + * covered for the day that changes.

+ */ + @Test + public void test_identifierBackedByTemplate_resolvesAsTemplate() { + assertIdentifierBackedBy(mock(Template.class), Template.class.getCanonicalName()); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at a {@link Structure}. + * Expected result: resolves as a Structure. Unreachable in production for the same reason as + * the Template case above. + */ + @Test + public void test_identifierBackedByStructure_resolvesAsStructure() { + assertIdentifierBackedBy(mock(Structure.class), Structure.class.getCanonicalName()); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at an inode that also implements + * {@link IHTMLPage}. + * Expected result: resolves as an HTML page. + * + *

This is the branch kept as a defence rather than deleted. No {@code Inode} implements + * {@link IHTMLPage} today — the only implementation, {@code HTMLPageAsset}, is a + * {@link Contentlet} — so the mock has to be built with the extra interface to produce the + * combination at all. That is exactly the shape a future subclass would take, and the test shows + * the branch does its job if one ever appears.

+ */ + @Test + public void test_identifierBackedByHtmlPageInode_resolvesAsHtmlPage() { + final Inode htmlPageInode = mock(Inode.class, withSettings().extraInterfaces(IHTMLPage.class)); + + assertIdentifierBackedBy(htmlPageInode, HTML_PAGE_TYPE); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} pointing at an inode of no recognised kind. + * Expected result: the identifier's own declared type, via {@code default}. + */ + @Test + public void test_identifierBackedByUnrecognisedInode_fallsThroughToDeclaredType() { + assertIdentifierBackedBy(mock(Inode.class), DECLARED_TYPE); + } + + /** + * Method to test: {@link PermissionBitFactoryImpl#resolvePermissionType(Permissionable)} + * Given scenario: an {@link Identifier} whose permission id resolves to no inode at all. + * Expected result: the identifier's own declared type, without throwing. + * + *

{@link InodeFactory#getInode(String, Class)} returns {@code null} when the lookup fails, and + * a {@code switch} over patterns throws on a null selector unless a {@code case null} says + * otherwise — so this pins that the {@code case null, default} arm is load bearing, not + * decorative.

+ */ + @Test + public void test_identifierWithNoBackingInode_fallsThroughToDeclaredType() { + assertIdentifierBackedBy(null, DECLARED_TYPE); + } }