[!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * #929

timohund · 2017-01-30T14:09:23Z

This PR changes the behaviour of query.allowedSites; the behavior * has been changed:

Before: * was the same as __all, which means all sites in the system
After: __all is still handled as __all sites in the system, but * now means every site (same as no check at all)

Implementation Details

During the implementation the following things have been done

Move SiteHash related logic from Util* to SiteHashService* and make them non static (See: http://wiki.c2.com/?AbuseOfUtilityClasses)
Mark SiteHash related method in Util deprecated and announce removal
Added Tests for Query and SiteHashService modifications
If variants should be used across multiple instances the variantId needs to be unique by system (not by site!). As of now the variantId only contains (type/uid), to have a unique variantId across multiple systems, a system hash was added.

Impact & Migration

If you use * for query.allowedSites please use __all now unless you want to disable the siteHash check
If you use variants, it is recommended to reindex your pages, since the hash was changed.

Fixes: #862

thomashohn · 2017-01-30T16:35:47Z

Classes/Domain/Site/SiteHashService.php

+     */
+    public function getAllowedSitesForPageIdAndAllowedSitesConfiguration($pageId, $allowedSitesConfiguration)
+    {
+        if ($allowedSitesConfiguration == '__all') {


Use strict ===

thomashohn · 2017-01-30T16:53:15Z

Classes/Domain/Site/SiteHashService.php

+            return $siteHashes[$domain];
+        }
+
+        $siteHashes[$domain] = sha1($domain . $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] . 'tx_solr');


Just moved, the hash should be the same as before

thomashohn · 2017-01-30T16:53:53Z

Classes/Domain/Variants/IdBuilder.php

+/***************************************************************
+ *  Copyright notice
+ *
+ *  (c) 2010-2017 Timo Hund <timo.hund@dkd.de


If new file 2017- and missing >

thomashohn · 2017-01-30T16:54:22Z

Classes/Domain/Variants/IdModifier.php

+/***************************************************************
+ *  Copyright notice
+ *
+ *  (c) 2010-2017 Timo Hund <timo.hund@dkd.de


If new file 2017 and agin missing >

thomashohn · 2017-01-30T16:58:23Z

Classes/Domain/Site/SiteHashService.php

+    }
+
+    /**
+     * Retrieves the domain of the site that belongs to the passed pageId and replaces ther markers __solr_current_site


thomashohn · 2017-01-30T16:58:50Z

Classes/Domain/Site/SiteHashService.php

+     *
+     * @param $pageId
+     * @param $allowedSitesConfiguration
+     * @return mixed


Is it mixed or string as return type?

thomashohn · 2017-01-30T17:01:00Z

Classes/Domain/Variants/IdBuilder.php

+     * @param integer $uid
+     * @return string
+     */
+    protected function applyHook($variantId, $systemHash, $type, $uid)


Maybe it would be better to use the new Signal concept in TYPO3? - I think hooks are only kept for compatibility

signals don't work for every scenario. If you want to change data, you still need hooks.

Hi @irnnr and @timohund

Not convinced - see https://usetypo3.com/signals-and-hooks-in-typo3.html and https://github.com/TYPO3/TYPO3.CMS/blob/master/typo3/sysext/core/Documentation/Changelog/master/Feature-79387-AddSignalToExcludeTablesFromReferenceIndex.rst - or have I missed something in this method?

I think signals and slots are more like "events" and the intension is normally a one way direction. It is possible to return data, but only by passing a variable by reference (what is not optimal i would say). I think with hooks you can directly return a value, what is needed in this case (Add btw. in the changes of the core also new hooks are added).

thomashohn · 2017-01-30T17:08:20Z

Classes/Domain/Variants/IdBuilder.php

+     */
+    protected function getSystemHash()
+    {
+        $siteName = isset($GLOBALS['TYPO3_CONF_VARS']['SYS']['sitename']) ? $GLOBALS['TYPO3_CONF_VARS']['SYS']['sitename'] : 'none';


Im not sure TYPO3 works with this $GLOBALS['TYPO3_CONF_VARS']['SYS']['sitename'] - but I think I would throw an exception instead

I'll propose to throw an exception when sitename is not set.

irnnr

LGTM, pending minor changes.

irnnr · 2017-01-30T23:23:25Z

Classes/Domain/Site/SiteHashService.php

+/***************************************************************
+ *  Copyright notice
+ *
+ *  (c) 2016 Timo Hund <timo.hund@dkd.de>


irnnr · 2017-01-30T23:30:18Z

Tests/Unit/Domain/Site/SiteHashServiceTest.php

+        $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] = 'testKey';
+
+        $service = new SiteHashService();
+        $hash1 = $service->getSiteHashForDomain('www.test.de');


Please use www.example.com for such use-cases. test.de is Stiftung Warentest.

…ites to * This PR changes the behaviour of query.allowedSites the previos setting * behaviour was changed: * Before: * was the same as __all, which means all sites in the system * After: __all is still handled as __all sites in the system, but * now means every site (same as no check at all) Migration: When you are using * for query.allowedSites change the setting to __all. During the implementation the following things have been done * Move SiteHash related logic from Util* to SiteHashService* and make them non static (See: http://wiki.c2.com/?AbuseOfUtilityClasses) * Mark SiteHash related method in Util deprecated and announce removal * Added Tests for Query and SiteHashService modifications * When variants should be used accross multiple instances the variantId needs to be unique by system (not by site!). By now the variantId only contains (type/uid), to have a unique variantId accros multiple systems, a system hash was added. Impact: * If you use * for query.allowedSites please use __all now unless you want to disable the siteHash check * If you use variants, it is recommended to reindex your pages, since the hash was changed. Fixes: TYPO3-Solr#862

timohund added this to the 6.1 - Get ready for 8 LTS milestone Jan 30, 2017

timohund requested a review from dkd-friedrich January 30, 2017 14:09

timohund changed the title [!!!][TASK] Allow to disable siteHash chech by setting query.allowedSites to * [!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * Jan 30, 2017

timohund force-pushed the task/master/862-allow-to-disable-siteHash branch from ab09be1 to 4bbdd8e Compare January 30, 2017 16:22

thomashohn reviewed Jan 30, 2017

View reviewed changes

irnnr approved these changes Jan 30, 2017

View reviewed changes

timohund force-pushed the task/master/862-allow-to-disable-siteHash branch 2 times, most recently from 9a6c08b to c1e535b Compare January 31, 2017 09:57

timohund force-pushed the task/master/862-allow-to-disable-siteHash branch from c1e535b to 2d2c141 Compare January 31, 2017 10:27

timohund merged commit 034cdd6 into TYPO3-Solr:master Jan 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * #929

[!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * #929

timohund commented Jan 30, 2017 •

edited

thomashohn Jan 30, 2017

timohund Jan 31, 2017

thomashohn Jan 30, 2017

timohund Jan 31, 2017

thomashohn Jan 30, 2017 •

edited

timohund Jan 31, 2017

thomashohn Jan 30, 2017

timohund Jan 31, 2017

thomashohn Jan 30, 2017

thomashohn Jan 30, 2017

thomashohn Jan 30, 2017

irnnr Jan 30, 2017

thomashohn Jan 31, 2017

timohund Jan 31, 2017

thomashohn Jan 30, 2017

timohund Jan 31, 2017

irnnr left a comment

irnnr Jan 30, 2017

timohund Jan 31, 2017

irnnr Jan 30, 2017

timohund Jan 31, 2017

[!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * #929

[!!!][TASK] Allow to disable siteHash check by setting query.allowedSites to * #929

Conversation

timohund commented Jan 30, 2017 • edited

Implementation Details

Impact & Migration

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomashohn Jan 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irnnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timohund commented Jan 30, 2017 •

edited

thomashohn Jan 30, 2017 •

edited