XWIKI-24125: The spaces property for the TemplateProviderClass should not list non-accessible page references

[vmassol]

Jira URL

https://jira.xwiki.org/browse/XWIKI-24125

Changes

Description

1. TemplateProviderClass.xml — Changed the spaces field from checkbox display to input (suggest picker) with size=30. Updated SQL queries in creationRestrictions, spaces, and visibilityRestrictions to select distinct doc.fullName, doc.web from XWikiDocument doc where doc.name = 'WebHome' order by doc.web. The doc.fullName column is used for view-right filtering while doc.web is the actual displayed value.

2. ExplicitlyAllowedValuesDBListQueryBuilder.java — Added the viewableAllowedDBListPropertyValue query filter when the SQL contains doc.fullName, following the same convention as ImplicitlyAllowedValuesDBListQueryBuilder. This ensures only spaces the user can view are suggested.

3. ExplicitlyAllowedValuesDBListQueryBuilderTest.java — Added viewableAllowedDBListPropertyValue filter mock, verified existing tests confirm the filter is NOT applied when SQL lacks doc.fullName, and added a new test buildWithDocFullNameAppliesViewableFilter confirming it IS applied when doc.fullName is present.

Executed Tests

Unit test + manual verification in XS.

Expected merging strategy

• Prefers squash: Yes

[surli]

It means that the template provider will be restricted to nested page only, isn't it a regression? We could have a template provided by a terminal page no?

[vmassol]

The spaces property lists only spaces. See https://extensions.xwiki.org/xwiki/bin/view/Extension/Administration%20Application#HPageTemplates where it explains it:

Before 8.3M2 there was a single spaces property specifying both the places where the template was visible and also where the template could be used to create pages, but it was split into the 2 above described restrictions to allow more flexibility and visibility of the templates feature. We are still supporting old (pre 8.3M2) template providers by reading from their spaces property, but if both the new restrictions and the spaces property are used, the new restriction properties will have priority.

[michitux]

What if the query actually wanted to use doc.fullName as value? Isn't this a breaking change then?

[vmassol]

If the query wanted to use doc.fullName, shouldn't it always have the permission to view the pages it returns?

[michitux]

The comment suggests that the doc.fullName column would be ignored when getting the values. But I haven't checked if this is actually the case.

[vmassol]

Valid point, I'm checking.

[vmassol]

You're right: the doc.fullName column is removed in the results by QueryFilter("viewableAllowedDBListPropertyValue").

So the code has 2 problems:

1. The "doc.fullName" string could be used in WHERE/ORDER BY. That's relatively easy to fix
2. The DBList HQL query could want to return "doc.fullName" (Actually, at https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/DataModel/DatabaseList/#HDisplayingcustomproperties it's even listed as the correct form to use, even if only the value is displayed. Not sure why). That one is harder to fix and the solution I've implemented won't work for it. It works for ImplicitlyAllowedValuesDBListQueryBuilder because it constructs the HQL query based on the metadata entered by the user but here we're in a more advanced use case where the query is controlled by the user fully. I don't know if the use case really exists but it's possible and it would be a regression for sure.

I need to find a different solution. If you have an idea, don't hesitate.

[vmassol]

ok but we could use this same strategy and use some other value, like select distinct doc.fullName as permissionCheck ....

PS: I wonder if select distinct doc.fullName, doc.fullName as permissionCheck ... would work in case it's needed.

[michitux]

Revert the ExplicitlyAllowedValuesDBListQueryBuilder heuristic from contains("doc.fullName") to contains("unfilterable0"). The alias unfilterable0 is already the convention used by ImplicitlyAllowedValuesDBListQueryBuilder (line 254) — it was explicitly chosen to signal "this column is dropped after
rights checking".

I don't know how you/your coding agent arrived at that conclusion,

xwiki-platform/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/objects/classes/ImplicitlyAllowedValuesDBListQueryBuilder.java

Lines 252 to 254 in 0fb0f46

	// We need to select the document in order to be able to check access rights on the results. At the same time we
	// want to skip this column when filtering the Database List property values.
	selectClause.add("doc.fullName as unfilterable0");

is pretty explicit that this is used to control which values are used for filtering (based on user input).

[michitux]

I've just checked the code a bit. I think having an unfilterable document reference might actually not be a realistic use case as it would break getting the display value as this also uses a text filter - see https://github.com/xwiki/xwiki-platform/blob/b0b884fda74c5664c001015715d4533bab58b1b2/xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/classes/DBListClassPropertyValuesProvider.java and parent classes. So we could take an unfilterable document reference column as sign, though I would prefer something more explicit like unfilterableRightCheck.

However, we have legacy code in particular in suggest.vm but also in DBListClass that uses the structure of the query and expects a select with one or two values. This would possibly be broken by this change and it also wouldn't benefit from the fix. It is not clear to me how much of this legacy code is actually dead code. The suggest edit displayer in DBListClass for example is essentially dead because of https://github.com/xwiki/xwiki-platform/blob/b0b884fda74c5664c001015715d4533bab58b1b2/xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-templates/src/main/resources/templates/displayer_dblist.vm

Another idea: could we improve support for implicit queries to cover this use case so we could switch those properties to use implicit queries?

[michitux]

Another note, unless I missed something, DBListClass retrieves the full map of all values using this code that you're touching here to get the display value in displayView. I've recently limited this to 1k items. But still, with your change, XWiki will now check rights on up to 1k pages every time a single value of the property is displayed. In a live data this happens once per row.

I don't want to argue against a right check, but we need to take the potential performance impact into account and maybe improve the efficiency of getting the display value to avoid a serious performance regression.

[michitux]

Actually, at https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/DataModel/DatabaseList/#HDisplayingcustomproperties it's even listed as the correct form to use, even if only the value is displayed. Not sure why

Because the displayed value should be updated automatically when the respective value is updated on the selected page. There is a stored and a displayed value. The user selects based on the value of the property while we store the document reference. Take the example application of movies and directors. When the name of a director is updated, e.g., to correct a mistake or because the director changed name, all movies by that director will display the new name.

Of course, the assumption here is that the displayed value is unique among all pages, this might create problems in practice and it would be nice to offer the actually stored value as differentiator in edit mode. But that's a different topic, in principle I think the feature makes sense.

[vmassol]

I've just checked the code a bit

Thx for checking!

So we could take an unfilterable document reference column as sign, though I would prefer something more explicit like unfilterableRightCheck.

I assume that you mean what I've proposed at #5388 (comment) , right?

PS: I wonder if select distinct doc.fullName, doc.fullName as permissionCheck ... would work in case it's needed.

This works fine, so I think it's a good solution for now until we refactor more deeply.

However, we have legacy code in particular in suggest.vm but also in DBListClass that uses the structure of the query and expects a select with one or two values.

Note that I had tested the suggest (since I moved the field from a checkbox to a suggest input) and it works fine, so there doesn't seem to be some real issue.

This would possibly be broken by this change and it also wouldn't benefit from the fix.

The goal of this PR is not to fix all the places where we have right check issues ;) What's important is that we have some tools (the implicit and explicit query builders) that can be used to refactor existing code to be safe.

Another note, unless I missed something, DBListClass retrieves the full map of all values using this code that you're touching here to get the display value in displayView. I've recently limited this to 1k items. But still, with your change, XWiki will now check rights on up to 1k pages every time a single value of the property is displayed. In a live data this happens once per row.

This is also the case for ImplicitlyAllowedValuesDBListQueryBuilder . I'd even argue that it's worse for ImplicitlyAllowedValuesDBListQueryBuilder since that's the most common use case (vs a custom HQL query). Not saying the problem is not real but that we need to fix it globally.

All in all, I feel that we should go with it and then think about what to make it even better in the future, by providing some more global refactorings in XWiki.

Another idea: could we improve support for implicit queries to cover this use case so we could switch those properties to use implicit queries?

We could but it's a much bigger change (to be able to express that we want only spaces), and if we want to do it, we should do it in a second step. Also there'll always be cases when we need the custom query and we need a solution for that.

Thx for the analysis.

[michitux]

This is also the case for ImplicitlyAllowedValuesDBListQueryBuilder . I'd even argue that it's worse for ImplicitlyAllowedValuesDBListQueryBuilder since that's the most common use case (vs a custom HQL query). Not saying the problem is not real but that we need to fix it globally.

My point is that frequently, you only have a couple of values that are selectable. However, in the case of the properties that you are modifying here, you always have the full list of all spaces of the wiki.

[michitux]

Note that I had tested the suggest (since I moved the field from a checkbox to a suggest input) and it works fine, so there doesn't seem to be some real issue.

The suggest input that you've tested shouldn't be using suggest.vm, I think that part of suggest.vm is dead (but easily exploitable) code. The tag editing feature at the bottom of the page is the only place I'm aware of that is using suggest.vm - but I've never searched exhaustively.

I assume that you mean what I've proposed at #5388 (comment) , right?

The alias should use unfilterable as prefix so it isn't targeted by the text filter. Otherwise, when the user types for example "web", it would match the "WebHome" in every document reference which would be a regression as it would make it close to impossible to search for space names that contain "web".

[michitux]

The alias should use unfilterable as prefix so it isn't targeted by the text filter. Otherwise, when the user types for example "web", it would match the "WebHome" in every document reference which would be regression as it would make it close to impossible to search for space names that contain "web".

Alternatively, the text filter could be modified to also skip columns that have an alias named permissionCheck but I'm not a fan of the text filter being aware of the permissionCheck alias convention.

[vmassol]

I've pushed the change. We can now decide if we want to merge it or not.

Noter that my original fix was "ok" too (it actually had an issue if "doc.fullName" was used in WHERE/ORDER clause without being the first column) and to answer you original question:

What if the query actually wanted to use doc.fullName as value? Isn't this a breaking change then?

The answer would have been to use select doc.fullName, doc.fullName, .... But it's a bit magical which is why I've pushed the select doc.fullName as permissionCheck, doc.fullName, ... syntax instead.

[michitux]

But it's a bit magical which is why I've pushed the select doc.fullName as permissionCheck, doc.fullName, ... syntax instead.

Could you check what happens now if you search, e.g., for "web" if that matches all spaces now instead of just those containing web, at least in the HQL query? Client-side filtering might still filter them out, but that means that if the space containing web isn't among the first 10 (or 20?), it won't be shown. As I've written above, I think an alias like unfilterablePermissionCheck could work around this by also triggering the exclusion in the text filter.