Fix/allowedtypes picker enforcement - #59
Open
wpelczar wants to merge 2 commits into
Open
Conversation
…oint 7) The 2.1.0 change for issue Geta#56 point 7 did not take effect. Two separate defects: 1. The property's [AllowedTypes] never reached the editor on the [Categories] attribute path. CMS's ExtensibleMetadataProvider.ApplyMetadataAwareAttributes builds the DisplayMetadataProviderContext from ModelAttributes.GetAttributesForType(metadata.GetType()) - the attributes of the ExtendedMetadata class, not of the property - so context.Attributes never contained AllowedTypesAttribute and ResolveAllowedTypes silently fell back to "all category types". RestrictedTypes was affected the same way. Attributes are now read from ExtendedMetadata.Attributes, which is the property's own set on both wiring paths. 2. Delegating the allow-filter to the shell store's typeIdentifiers parameter cannot work for a nested category tree. That parameter filters rather than marks, so narrowing it removes intermediate container categories and makes every allowed category nested underneath unreachable - for a root -> container -> allowed category hierarchy the picker rendered completely empty. There is no value that satisfies both goals either: including an ancestor type so the container survives re-admits every sibling type, because the store matches via the item's whole descriptor ancestor chain. Allowed and restricted types are now evaluated per node by the Selectable endpoint, and disallowed nodes are disabled but still shown and expandable - the same "grey, don't hide" approach the native content picker takes (ContentTree + disableRestrictedTypes). Type matching goes through UIDescriptorRegistry.HasMatchedTypeIdentifier, so a base type matches all its subtypes, the same semantics as AllowedTypesAttribute's publish-time validation. As a side effect the allow-list now also applies to search results, which were never type-filtered at all: SearchResultStore does not bind a typeIdentifiers parameter, so the values the client sent were silently discarded. Also: - Selectable accepts allowedTypes / restrictedTypes and returns a reason (notSelectable / typeNotAllowed) so the picker can explain why a row is disabled. - Restricted types are disabled rather than removed from the tree and search, for the same hierarchy reason as above. - EditorConfiguration["AllowedTypes"] / ["AllowedDndTypes"] now hold type identifier strings instead of System.Type instances, matching the CMS convention for those keys. Verified against a real CMS 13 / .NET 10 solution using Optimizely Identity, with both [Categories] and [UIHint(CategoryUIHint.Categories)]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The example hierarchy referenced concrete type names from the solution the fix was verified against. Describe the roles instead — it reads better for anyone who does not have that model in front of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #58.
Makes
[AllowedTypes]/RestrictedTypesactually take effect in the category picker. The 2.1.0 change for #56 point 7 (PR #57) did not work in practice — #58 has the full diagnosis; the short version is two independent defects.What was wrong
1. The attribute never reached the editor on the
[Categories]path.CategoryEditorMetadataread it from thepropertyAttributeshanded toCategoriesAttribute.CreateDisplayMetadata, i.e.context.Attributes. CMS builds that context inExtensibleMetadataProvider.ApplyMetadataAwareAttributesas:— the attributes of the
ExtendedMetadataclass, not of the property. SoAllowedTypesAttributewas never present,ResolveAllowedTypessilently returned itstypeof(CategoryData)fallback, and the picker offered every category type.RestrictedTypeswas lost the same way.This was path-specific:
CategoriesEditorDescriptor(the[UIHint]path) is anIMetadataExtender, invoked withpropertyMetadata.Attributes, so it did receive the real set. The broken path is the one the 2.0.0 README and CHANGELOG recommend.2. Delegating the allow-filter to the shell store cannot work for a nested tree.
typeIdentifiersoncontentstructurefilters rather than marks:GetChildrenQueryrestricts children to the requested type, so intermediate "container" categories are removed and every allowed category nested beneath them becomes unreachable. On a tree shapedroot -> grouping category -> allowed category, the picker renders completely empty.There is no value that satisfies both goals. Including an ancestor type so the container survives re-admits every sibling type, because the store matches via the item's whole descriptor ancestor chain (
DefaultContentQueryHelper.FilterTypeIdentifier→UIDescriptorRegistry.HasMatchedTypeIdentifier). Restricting the leaves and exposing the containers are mutually exclusive through that parameter.So fixing 1 alone turns a permissive picker into an empty one. Both had to move together.
What this changes
CategoryEditorMetadata.ResolvePropertyAttributesfalls back toExtendedMetadata.Attributeswhen the supplied set carries noAllowedTypesAttribute. That is the property's own attribute list on both wiring paths, and the same source CMS's ownAllowedTypesMetadataExtenderuses.getChildrenalways requests the base category type, so the tree stays complete and navigable. Allowed/restricted types are evaluated per node by the existingSelectableendpoint, and disallowed nodes are disabled but still shown and expandable — the same "grey, don't hide" approach as the native picker (ContentTree+disableRestrictedTypes). A disallowed container is therefore still a path to the allowed categories under it.UIDescriptorRegistry.HasMatchedTypeIdentifier— the same call CMS makes itself. A base type matches all its subtypes, which lines up withAllowedTypesAttribute's publish-time validation, and the package does not have to model the type hierarchy in JavaScript. (Doing it in the client would need the polymorphic match too: for atypeof(...)-declared type,AllowedTypesMetadataExtenderemits only the lowercasedFullName, so string equality would wrongly reject subtypes.)SearchResultStorebinds notypeIdentifiersparameter, so the values the client sent were silently discarded. Search hits now go through the sameSelectablepass as tree nodes, so the request no longer sends type identifiers that cannot be honoured.Behaviour changes worth a reviewer's opinion
typeIdentifiersdoes — but it is a visible change from 2.1.0, so flagging it explicitly.EditorConfiguration["AllowedTypes"]/["AllowedDndTypes"]now hold type identifier strings instead ofSystem.Typeinstances, matching the CMS convention for those keys. For properties that declare[AllowedTypes], CMS's own extender overwrote these anyway; for a property relying on the package default, they were previously serialisedSystem.Typeobjects.API surface
CategoryEditorControllerconstructor gains aUIDescriptorRegistryparameter — binary-breaking for anything constructing it directly, though as an MVC controller it is resolved by DI.Selectable(string ids)→Selectable(string ids, string allowedTypes = null, string restrictedTypes = null). Source-compatible for existing callers, binary-breaking.CategorySelectableResponsegainsReason(notSelectable/typeNotAllowed, exposed asCategoryEditorController.NotSelectableReason/TypeNotAllowedReason) so the picker can explain why a row is disabled. Additive.The CHANGELOG entry is under
## [Unreleased]— move it under whichever version you plan to ship.Tests
27 pass (17 before, 10 new):
ResolvePropertyAttributes— the regression guard for defect 1, covering the fallback, precedence when both sets carry the attribute, the empty case, andRestrictedTypessurviving the fallback.Selectable— the type gate: allowed vs. disallowed, restricted taking precedence over allowed,IsSelectabletaking precedence over the type check, no registry call when no allow-list is supplied, and unresolvable ids staying selectable.The registry is stubbed rather than built from real
UIDescriptorinstances, becauseUIDescriptor's public constructor resolvesViewConfiguration/IContentTypeRepositorythroughServiceLocator. The polymorphic matching itself is CMS's contract, so the tests cover the controller's own logic: which gate wins, and which reason is reported.Note I could not run the full-solution build locally — the
sub/geta-foundation-coresubmodule is not part of my checkout — so the sandbox is unverified on my side. CI checks out submodules recursively, and this change touches neither the sandbox nor the submodule.Verification
Manually verified against a large production solution currently being migrated to CMS 13 / .NET 10 under Optimizely Identity (management UI at
/ui), on a category tree of ~25 category types nested behind non-selectable container categories, with 56[Categories]properties of which 20 declare[AllowedTypes]. Tested via a locally packed build of this branch, so the module zip andbuildTransitivedelivery path were exercised too, not just the assembly.Confirmed:
[Categories]property with no[AllowedTypes]still offers every category type, selectable.[Categories]and[UIHint(CategoryUIHint.Categories)]behave identically — the point of the fix.Attribution
The investigation and the code were done with Claude Code. The root cause was found by decompiling the CMS 13.1.0 assemblies to trace how
typeIdentifiersand the metadata pipeline actually behave, then confirmed empirically rather than inferred — thewidgetSettingscapture in #58 is the direct evidence. Every claim about CMS internals in #58 and in the code comments is checkable against the shipped assemblies.Happy to reshape any of this — particularly the disable-vs-hide decision — if you would rather solve it a different way.