From 19d57dbdb55d4cb8acbdecd5bf9b0467a6df82a4 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Wed, 27 Sep 2023 20:57:46 +0100 Subject: [PATCH 1/5] [twgit] Init feature 'feature-PRESIDECMS-2613_feature-check-expressions'. From 048715be48366f4a68ba9de3653a5ebb2048f1b5 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Wed, 27 Sep 2023 21:09:50 +0100 Subject: [PATCH 2/5] PRESIDECMS-2613 add complex feature enabled evaluation In many scenarios, we need to be able to check whether or not multiple different features are enabled, or whether one feature is not enabled and another is, etc. This change adds support for using operators in our feature string to achieve this wherever we have feature specifications. e.g. in a form, you could now do: ```xml ``` --- system/services/features/FeatureService.cfc | 36 ++++++++++++++++++- .../api/features/FeatureServiceTest.cfc | 21 +++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/system/services/features/FeatureService.cfc b/system/services/features/FeatureService.cfc index 0036a3c1d4..ddf8716b7a 100644 --- a/system/services/features/FeatureService.cfc +++ b/system/services/features/FeatureService.cfc @@ -20,10 +20,13 @@ component singleton=true autodoc=true displayName="Feature service" { /** * Returns whether or not the passed feature is currently enabled * - * @feature.hint name of the feature to check + * @feature.hint name of the feature to check, or logical expression containing features to check, e.g. "(featurex || featurey) && featurez" * @siteTemplate.hint current active site template - can be used to check features that can be site template specific */ public boolean function isFeatureEnabled( required string feature, string siteTemplate ) autodoc=true { + if ( _isComplexExpression( Trim( arguments.feature ) ) ) { + return _processComplexExpression( arguments.feature ); + } var features = _getConfiguredFeatures(); var isEnabled = IsBoolean( features[ arguments.feature ].enabled ?: "" ) && features[ arguments.feature ].enabled; @@ -74,6 +77,37 @@ component singleton=true autodoc=true displayName="Feature service" { return ""; } +// PRIVATE HELPERS + private boolean function _isComplexExpression( filter ) { + var evalChars = "|&\(\)!\s"; + var featureNameChars = "a-zA-Z0-9_\-"; + + // must contain one ore more special evaluation chars + // and must ONLY contain feature name chars + evaluation chars + return ReFind( "[#evalChars#]+", arguments.filter ) && ReFind( "^[#evalChars##featureNameChars#]+$", arguments.filter ); + } + + private boolean function _processComplexExpression( filter, siteTemplate ) { + var compiled = arguments.filter; + var features = ReMatch( "\b[a-zA-Z0-9_\-]+\b", arguments.filter ); + + ArraySort( features, function( a, b ){ + var lena = Len( arguments.a ); + var lenb = Len( arguments.b ); + return lena == lenb ? 0 : ( lena < lenb ? 1 : -1 ); + } ); + + for( var feature in features ) { + compiled = ReplaceNoCase( compiled, feature, isFeatureEnabled( feature, arguments.siteTemplate ) ? "true" : "false", "all" ); + } + + try { + return Evaluate( compiled ); + } catch( any e ) { + throw( type="preside.feature.bad.expression", message="The feature name expression, [#arguments.filter#], could not be evaluated. Compiler error: [#e.message#]." ); + } + } + // GETTERS AND SETTERS private struct function _getConfiguredFeatures() { return _configuredFeatures; diff --git a/tests/integration/api/features/FeatureServiceTest.cfc b/tests/integration/api/features/FeatureServiceTest.cfc index 0f8e6bbd9b..e31cf9463d 100644 --- a/tests/integration/api/features/FeatureServiceTest.cfc +++ b/tests/integration/api/features/FeatureServiceTest.cfc @@ -32,6 +32,27 @@ component extends="testbox.system.BaseSpec"{ expect( svc.isFeatureEnabled( feature="sites", siteTemplate="" ) ) .toBeTrue(); } ); + it( "should multi-evaluate feature expressions using parenthesis and && and || characters", function(){ + var svc = _getService(); + + expect( svc.isFeatureEnabled( feature="sites || websiteUsers", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="sites && assetManager", siteTemplate="" ) ).toBeFalse(); + expect( svc.isFeatureEnabled( feature="sites && websiteUsers", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="(sites || what) && websiteusers", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="((sites && what) && websiteusers)", siteTemplate="" ) ).toBeFalse(); + expect( svc.isFeatureEnabled( feature="((sites && !what) && websiteusers)", siteTemplate="" ) ).toBeTrue(); + } ); + + it( "should not attempt dynamic lucee execution with malicious input", function(){ + var svc = _getService(); + + expect( svc.isFeatureEnabled( feature="http url='https://www.google.com';", siteTemplate="" ) ).toBeFalse(); + expect( svc.isFeatureEnabled( feature="int( 45.0 )", siteTemplate="" ) ).toBeFalse(); + expect( function(){ + svc.isFeatureEnabled( feature="floor( something )", siteTemplate="" ) + } ).toThrow( "preside.feature.bad.expression" ) ; + } ); + } ); describe( "isFeatureDefined()", function(){ From 0a1b879a95a29d11c31514018bc6348bb573d7ad Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Wed, 27 Sep 2023 21:24:07 +0100 Subject: [PATCH 3/5] PRESIDECMS-2613 Add support for and, not, or to help with writing expressions in xml --- system/services/features/FeatureService.cfc | 4 +++- tests/integration/api/features/FeatureServiceTest.cfc | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/system/services/features/FeatureService.cfc b/system/services/features/FeatureService.cfc index ddf8716b7a..5958e90866 100644 --- a/system/services/features/FeatureService.cfc +++ b/system/services/features/FeatureService.cfc @@ -98,7 +98,9 @@ component singleton=true autodoc=true displayName="Feature service" { } ); for( var feature in features ) { - compiled = ReplaceNoCase( compiled, feature, isFeatureEnabled( feature, arguments.siteTemplate ) ? "true" : "false", "all" ); + if ( feature != "not" && feature != "and" && feature != "or" ) { + compiled = ReplaceNoCase( compiled, feature, isFeatureEnabled( feature, arguments.siteTemplate ) ? "true" : "false", "all" ); + } } try { diff --git a/tests/integration/api/features/FeatureServiceTest.cfc b/tests/integration/api/features/FeatureServiceTest.cfc index e31cf9463d..12893cc006 100644 --- a/tests/integration/api/features/FeatureServiceTest.cfc +++ b/tests/integration/api/features/FeatureServiceTest.cfc @@ -36,8 +36,9 @@ component extends="testbox.system.BaseSpec"{ var svc = _getService(); expect( svc.isFeatureEnabled( feature="sites || websiteUsers", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="sites or websiteUsers", siteTemplate="" ) ).toBeTrue(); expect( svc.isFeatureEnabled( feature="sites && assetManager", siteTemplate="" ) ).toBeFalse(); - expect( svc.isFeatureEnabled( feature="sites && websiteUsers", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="sites and websiteUsers", siteTemplate="" ) ).toBeTrue(); expect( svc.isFeatureEnabled( feature="(sites || what) && websiteusers", siteTemplate="" ) ).toBeTrue(); expect( svc.isFeatureEnabled( feature="((sites && what) && websiteusers)", siteTemplate="" ) ).toBeFalse(); expect( svc.isFeatureEnabled( feature="((sites && !what) && websiteusers)", siteTemplate="" ) ).toBeTrue(); From 2e66f5dc72b3dc874f9a20ed25c650dde993bd29 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Wed, 27 Sep 2023 21:26:27 +0100 Subject: [PATCH 4/5] PRESIDECMS-2613 add test case for use of 'not' --- tests/integration/api/features/FeatureServiceTest.cfc | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/api/features/FeatureServiceTest.cfc b/tests/integration/api/features/FeatureServiceTest.cfc index 12893cc006..888aaa2f14 100644 --- a/tests/integration/api/features/FeatureServiceTest.cfc +++ b/tests/integration/api/features/FeatureServiceTest.cfc @@ -42,6 +42,7 @@ component extends="testbox.system.BaseSpec"{ expect( svc.isFeatureEnabled( feature="(sites || what) && websiteusers", siteTemplate="" ) ).toBeTrue(); expect( svc.isFeatureEnabled( feature="((sites && what) && websiteusers)", siteTemplate="" ) ).toBeFalse(); expect( svc.isFeatureEnabled( feature="((sites && !what) && websiteusers)", siteTemplate="" ) ).toBeTrue(); + expect( svc.isFeatureEnabled( feature="((sites and not what) and websiteusers)", siteTemplate="" ) ).toBeTrue(); } ); it( "should not attempt dynamic lucee execution with malicious input", function(){ From 56c01630308a832ad7d412f852dba1dbaa3bd11f Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Wed, 27 Sep 2023 21:27:53 +0100 Subject: [PATCH 5/5] PRESIDECMS-2613 add missing site template argument --- system/services/features/FeatureService.cfc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/services/features/FeatureService.cfc b/system/services/features/FeatureService.cfc index 5958e90866..14794d13ac 100644 --- a/system/services/features/FeatureService.cfc +++ b/system/services/features/FeatureService.cfc @@ -25,7 +25,7 @@ component singleton=true autodoc=true displayName="Feature service" { */ public boolean function isFeatureEnabled( required string feature, string siteTemplate ) autodoc=true { if ( _isComplexExpression( Trim( arguments.feature ) ) ) { - return _processComplexExpression( arguments.feature ); + return _processComplexExpression( arguments.feature, arguments.siteTemplate ); } var features = _getConfiguredFeatures(); var isEnabled = IsBoolean( features[ arguments.feature ].enabled ?: "" ) && features[ arguments.feature ].enabled;