diff --git a/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java index 106d773d00e..a7c66b1478d 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. + * + *

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 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) { + + // 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 Container _ -> Container.class.getCanonicalName(); + + case Link _ -> Link.class.getCanonicalName(); + + case Template _ -> Template.class.getCanonicalName(); + + case Structure _ -> Structure.class.getCanonicalName(); + + case null, 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 ContentType contentType = contentlet.getContentType(); + return contentType != null && variableName.equals(contentType.variable()); + } + + /** + * Whether the contentlet's content type has the given base type. + * + *

{@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.

+ * + * @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 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 new file mode 100644 index 00000000000..e98fc635340 --- /dev/null +++ b/dotCMS/src/test/java/com/dotmarketing/business/PermissionBitFactoryImplResolvePermissionTypeTest.java @@ -0,0 +1,344 @@ +package com.dotmarketing.business; + +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 + * 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 variable name and base type. + */ + 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.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 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#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.getContentType()).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)); + } + + // --- 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); + } +}