From 8a67511d2c219b9a76ace84e4a756ce39f27ecc9 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Thu, 14 Sep 2023 18:27:53 +0100 Subject: [PATCH 1/4] [twgit] Init feature 'feature-PRESIDECMS-2708_better-cache-headers-for-assets'. From 0f2094057e24ea16959da4ff0d9111ec65ef2aac Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Thu, 14 Sep 2023 18:29:12 +0100 Subject: [PATCH 2/4] PRESIDECMS-2708 add more nuanced cache-control headers with asset downloads. For restricted assets, we should use cache-control: private to avoid proxies or any other shared caches from caching the response. Public assets can explicitly be labelled as such. In addition, provide configurable timeouts for the caches. --- system/config/Config.cfc | 4 ++ system/handlers/core/AssetDownload.cfc | 81 ++++++++++++++------------ 2 files changed, 49 insertions(+), 36 deletions(-) diff --git a/system/config/Config.cfc b/system/config/Config.cfc index 1730676806..e75af021f9 100644 --- a/system/config/Config.cfc +++ b/system/config/Config.cfc @@ -791,6 +791,10 @@ component { , trash = ( settings.env[ "assetmanager.storage.trash" ] ?: settings.uploads_directory & "/.trash" ) , publicUrl = ( settings.env[ "assetmanager.storage.publicUrl" ] ?: "" ) } + , cacheExpiry = { + public = Val( settings.env.ASSET_CACHE_EXPIRY_PUBLIC ?: 31536000 ) // one year + , private = Val( settings.env.ASSET_CACHE_EXPIRY_PRIVATE ?: 86400 ) // one day + } }; settings.assetManager.allowedExtensions = _typesToExtensions( settings.assetManager.types ); settings.assetManager.types.document.append( { tiff = { serveAsAttachment = true, mimeType="image/tiff" } } ); diff --git a/system/handlers/core/AssetDownload.cfc b/system/handlers/core/AssetDownload.cfc index 4a6933a6d9..c1684fd136 100644 --- a/system/handlers/core/AssetDownload.cfc +++ b/system/handlers/core/AssetDownload.cfc @@ -5,12 +5,13 @@ component { property name="websiteUserActionService" inject="websiteUserActionService"; property name="rulesEngineWebRequestService" inject="rulesEngineWebRequestService"; property name="queueMaxWaitAttempts" inject="coldbox:setting:assetManager.queue.downloadWaitSeconds"; + property name="publicCacheAge" inject="coldbox:setting:assetManager.cacheExpiry.public"; + property name="privateCacheAge" inject="coldbox:setting:assetManager.cacheExpiry.private"; public function asset( event, rc, prc ) output=false { announceInterception( "preDownloadAsset" ); - _checkDownloadPermissions( argumentCollection=arguments ); - + var isRestricted = _checkDownloadPermissions( argumentCollection=arguments ); var assetId = rc.assetId ?: ""; var versionId = rc.versionId ?: ""; var derivativeName = rc.derivativeId ?: ""; @@ -137,8 +138,13 @@ component { ); } + header name="etag" value=etag; - header name="cache-control" value="max-age=31536000"; + if ( isRestricted ) { + header name="cache-control" value="private, max-age=#privateCacheAge#"; + } else { + header name="cache-control" value="public, max-age=#publicCacheAge#"; + } if ( IsBinary( assetFilePathOrBinary ) ) { content @@ -178,54 +184,55 @@ component { return ReReplace( arguments.assetTitle, "\.#arguments.extension#$", "" ) & "." & arguments.extension; } - private void function _checkDownloadPermissions( event, rc, prc ) output=false { + private boolean function _checkDownloadPermissions( event, rc, prc ) output=false { var assetId = rc.assetId ?: ""; var derivativeName = rc.derivativeId ?: ""; if ( Len( Trim( derivativeName ) ) && assetManagerService.isDerivativePubliclyAccessible( derivativeName ) ) { - return; + return false; } var permissionSettings = assetManagerService.getAssetPermissioningSettings( assetId ); + if ( !permissionSettings.restricted ) { + return false; + } + if ( !event.isAdminUser() ) { - if ( permissionSettings.restricted ) { - if ( Len( Trim( permissionSettings.conditionId ) ) ) { - var conditionIsTrue = rulesEngineWebRequestService.evaluateCondition( permissionSettings.conditionId ); - - if ( !conditionIsTrue ) { - if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { - event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); - } else { - event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); - } + if ( Len( Trim( permissionSettings.conditionId ) ) ) { + var conditionIsTrue = rulesEngineWebRequestService.evaluateCondition( permissionSettings.conditionId ); + + if ( !conditionIsTrue ) { + if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { + event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); + } else { + event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); } - return; } - var hasPerm = event.isAdminUser() && hasCmsPermission( - permissionKey = "assetmanager.assets.download" - , context = "assetmanagerfolder" - , contextKeys = permissionSettings.contextTree - , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers - ); - if ( hasPerm ) { return; } + return true; + } + var hasPerm = event.isAdminUser() && hasCmsPermission( + permissionKey = "assetmanager.assets.download" + , context = "assetmanagerfolder" + , contextKeys = permissionSettings.contextTree + , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers + ); + if ( hasPerm ) { return true; } - if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { - event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); - } + if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { + event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); + } - hasPerm = hasWebsitePermission( - permissionKey = "assets.access" - , context = "asset" - , contextKeys = permissionSettings.contextTree - , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers - ) - if ( !hasPerm ) { - event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); - } + hasPerm = hasWebsitePermission( + permissionKey = "assets.access" + , context = "asset" + , contextKeys = permissionSettings.contextTree + , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers + ) + if ( !hasPerm ) { + event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); } } else { - hasPerm = hasCmsPermission( permissionKey = "assetmanager.assets.download" , context = "assetmanagerfolder" @@ -235,5 +242,7 @@ component { event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); } } + + return true; } } \ No newline at end of file From 73741cde919f89a626f5a7d49089aa23ce4a3a81 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Fri, 15 Sep 2023 11:26:26 +0100 Subject: [PATCH 3/4] PRESIDECMS-2708 tidy up methods + fix lack of var on variable --- system/handlers/core/AssetDownload.cfc | 93 +++++++++++++------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/system/handlers/core/AssetDownload.cfc b/system/handlers/core/AssetDownload.cfc index c1684fd136..5ac26b7b96 100644 --- a/system/handlers/core/AssetDownload.cfc +++ b/system/handlers/core/AssetDownload.cfc @@ -8,10 +8,14 @@ component { property name="publicCacheAge" inject="coldbox:setting:assetManager.cacheExpiry.public"; property name="privateCacheAge" inject="coldbox:setting:assetManager.cacheExpiry.private"; - public function asset( event, rc, prc ) output=false { + public function asset( event, rc, prc ) { announceInterception( "preDownloadAsset" ); - var isRestricted = _checkDownloadPermissions( argumentCollection=arguments ); + var permissionSettings = _getPermissionSettings( argumentCollection=arguments ); + if ( permissionSettings.restricted ) { + _checkDownloadPermissions( argumentCollection=arguments, permissionSettings=permissionSettings ); + } + var assetId = rc.assetId ?: ""; var versionId = rc.versionId ?: ""; var derivativeName = rc.derivativeId ?: ""; @@ -138,9 +142,8 @@ component { ); } - header name="etag" value=etag; - if ( isRestricted ) { + if ( permissionSettings.restricted ) { header name="cache-control" value="private, max-age=#privateCacheAge#"; } else { header name="cache-control" value="public, max-age=#publicCacheAge#"; @@ -173,7 +176,7 @@ component { } // private helpers - private string function _doBrowserEtagLookup( required string etag ) output=false { + private string function _doBrowserEtagLookup( required string etag ) { if ( ( cgi.http_if_none_match ?: "" ) == arguments.etag ) { announceInterception( "onReturnAsset304", { etag = arguments.etag } ); content reset=true;header statuscode=304 statustext="Not Modified";abort; @@ -184,7 +187,7 @@ component { return ReReplace( arguments.assetTitle, "\.#arguments.extension#$", "" ) & "." & arguments.extension; } - private boolean function _checkDownloadPermissions( event, rc, prc ) output=false { + private struct function _getPermissionSettings( event, rc, prc ) { var assetId = rc.assetId ?: ""; var derivativeName = rc.derivativeId ?: ""; @@ -192,57 +195,53 @@ component { return false; } - var permissionSettings = assetManagerService.getAssetPermissioningSettings( assetId ); - - if ( !permissionSettings.restricted ) { - return false; - } - - if ( !event.isAdminUser() ) { - if ( Len( Trim( permissionSettings.conditionId ) ) ) { - var conditionIsTrue = rulesEngineWebRequestService.evaluateCondition( permissionSettings.conditionId ); - - if ( !conditionIsTrue ) { - if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { - event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); - } else { - event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); - } - } - return true; - } - var hasPerm = event.isAdminUser() && hasCmsPermission( - permissionKey = "assetmanager.assets.download" - , context = "assetmanagerfolder" - , contextKeys = permissionSettings.contextTree - , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers - ); - if ( hasPerm ) { return true; } + return assetManagerService.getAssetPermissioningSettings( assetId ); + } - if ( !isLoggedIn() || ( permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { - event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); - } + private void function _checkDownloadPermissions( event, rc, prc, permissionSettings ) { + var assetId = rc.assetId ?: ""; + var derivativeName = rc.derivativeId ?: ""; + var hasPerm = false; - hasPerm = hasWebsitePermission( - permissionKey = "assets.access" - , context = "asset" - , contextKeys = permissionSettings.contextTree - , forceGrantByDefault = IsBoolean( permissionSettings.grantAcessToAllLoggedInUsers ) && permissionSettings.grantAcessToAllLoggedInUsers - ) - if ( !hasPerm ) { - event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); - } - } else { + if ( event.isAdminUser() ) { hasPerm = hasCmsPermission( permissionKey = "assetmanager.assets.download" , context = "assetmanagerfolder" - , contextKeys = permissionSettings.contextTree ?: [] + , contextKeys = arguments.permissionSettings.contextTree ?: [] ); if ( !hasPerm ) { event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); } + return; } - return true; + if ( Len( Trim( arguments.permissionSettings.conditionId ) ) ) { + var conditionIsTrue = rulesEngineWebRequestService.evaluateCondition( arguments.permissionSettings.conditionId ); + + if ( conditionIsTrue ) { + return; + } + + if ( !isLoggedIn() || ( arguments.permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { + event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); + } else { + event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); + } + } + + if ( !isLoggedIn() || ( arguments.permissionSettings.fullLoginRequired && isAutoLoggedIn() ) ) { + event.accessDenied( reason="LOGIN_REQUIRED", postLoginUrl=( cgi.http_referer ?: "" ) ); + } + + hasPerm = hasWebsitePermission( + permissionKey = "assets.access" + , context = "asset" + , contextKeys = arguments.permissionSettings.contextTree + , forceGrantByDefault = IsBoolean( arguments.permissionSettings.grantAcessToAllLoggedInUsers ) && arguments.permissionSettings.grantAcessToAllLoggedInUsers + ); + + if ( !hasPerm ) { + event.accessDenied( reason="INSUFFICIENT_PRIVILEGES" ); + } } } \ No newline at end of file From be825f7549d3abe22a258ea693d75b069632ab69 Mon Sep 17 00:00:00 2001 From: Dominic Watson Date: Fri, 15 Sep 2023 11:31:48 +0100 Subject: [PATCH 4/4] PRESIDECMS-2708 ensure we return expected format in all scenarios here --- system/handlers/core/AssetDownload.cfc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/handlers/core/AssetDownload.cfc b/system/handlers/core/AssetDownload.cfc index 5ac26b7b96..06b7598e02 100644 --- a/system/handlers/core/AssetDownload.cfc +++ b/system/handlers/core/AssetDownload.cfc @@ -192,7 +192,7 @@ component { var derivativeName = rc.derivativeId ?: ""; if ( Len( Trim( derivativeName ) ) && assetManagerService.isDerivativePubliclyAccessible( derivativeName ) ) { - return false; + return { restricted=false }; } return assetManagerService.getAssetPermissioningSettings( assetId );