From 5fa5a0c46aea36c11383a3a7a4121ca9e235b3da Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 17 Feb 2025 10:21:20 +1300 Subject: [PATCH 1/2] ENH Add a warning if allowed hosts is not set. (#11612) Adds ability to "wildcard" allow all hosts, which disables the logging. Adds much needed test coverage. --- .../Middleware/AllowedHostsMiddleware.php | 17 ++- src/Core/BaseKernel.php | 28 +++++ .../Middleware/AllowedHostsMiddlewareTest.php | 119 ++++++++++++++++++ tests/php/Core/KernelTest.php | 61 +++++++++ 4 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php diff --git a/src/Control/Middleware/AllowedHostsMiddleware.php b/src/Control/Middleware/AllowedHostsMiddleware.php index 8a7df32f4c7..7915dc8b139 100644 --- a/src/Control/Middleware/AllowedHostsMiddleware.php +++ b/src/Control/Middleware/AllowedHostsMiddleware.php @@ -2,6 +2,7 @@ namespace SilverStripe\Control\Middleware; +use InvalidArgumentException; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; @@ -12,14 +13,16 @@ class AllowedHostsMiddleware implements HTTPMiddleware { /** - * List of allowed hosts + * List of allowed hosts. + * Can be ['*'] to allow all hosts and disable the logged warning. * * @var array */ private $allowedHosts = []; /** - * @return array List of allowed Host header values + * @return array List of allowed Host header values. + * Note that both an empty array and ['*'] can be used to allow all hosts. */ public function getAllowedHosts() { @@ -30,14 +33,21 @@ public function getAllowedHosts() * Sets the list of allowed Host header values * Can also specify a comma separated list * + * Note that both an empty array and ['*'] can be used to allow all hosts. + * * @param array|string $allowedHosts * @return $this */ public function setAllowedHosts($allowedHosts) { - if (is_string($allowedHosts)) { + if ($allowedHosts === null) { + $allowedHosts = []; + } elseif (is_string($allowedHosts)) { $allowedHosts = preg_split('/ *, */', $allowedHosts ?? ''); } + if (count($allowedHosts) > 1 && in_array('*', $allowedHosts)) { + throw new InvalidArgumentException('The wildcard "*" cannot be used in conjunction with actual hosts.'); + } $this->allowedHosts = $allowedHosts; return $this; } @@ -51,6 +61,7 @@ public function process(HTTPRequest $request, callable $delegate) // check allowed hosts if ($allowedHosts + && $allowedHosts !== ['*'] && !Director::is_cli() && !in_array($request->getHeader('Host'), $allowedHosts ?? []) ) { diff --git a/src/Core/BaseKernel.php b/src/Core/BaseKernel.php index a1dbc503ba5..d3f96a0cb4e 100644 --- a/src/Core/BaseKernel.php +++ b/src/Core/BaseKernel.php @@ -27,6 +27,7 @@ use SilverStripe\View\ThemeManifest; use SilverStripe\View\ThemeResourceLoader; use Exception; +use SilverStripe\Control\Middleware\AllowedHostsMiddleware; use SilverStripe\Dev\Deprecation; /** @@ -222,6 +223,9 @@ protected function bootConfigs() { // After loading all other app manifests, include _config.php files $this->getModuleLoader()->getManifest()->activateConfig(); + + // Ensure everything is set up correctly + $this->validateConfiguration(); } /** @@ -388,6 +392,7 @@ public function activate() $this->getInjectorLoader() ->getManifest() ->registerService($this, Kernel::class); + return $this; } @@ -469,4 +474,27 @@ public function setThemeResourceLoader($themeResourceLoader) $this->themeResourceLoader = $themeResourceLoader; return $this; } + + /** + * Validate configuration of the application is in a good state, ready for use. + * + * This method can be used to warn developers of any misconfiguration, or configuration + * which is missing but should be set according to best practice. + * + * In some cases, this could be used to halt execution if configuration critical to operation + * has not been set. + */ + protected function validateConfiguration(): void + { + // Log a warning if allowed hosts hasn't been configured. + // This can include wildcard, but it must be explicitly set to ensure the developer is aware + // of the level of protection their application has against host header injection attacks. + $allowedHostsMiddleware = Injector::inst()->get(AllowedHostsMiddleware::class, true); + if (empty($allowedHostsMiddleware->getAllowedHosts())) { + Injector::inst()->get(LoggerInterface::class)->warning( + 'Allowed hosts has not been set. Your application could be vulnerable to host header injection attacks.' + . ' Either set the SS_ALLOWED_HOSTS environment variable or the AllowedHosts property on ' . AllowedHostsMiddleware::class + ); + } + } } diff --git a/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php b/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php new file mode 100644 index 00000000000..8da267c64af --- /dev/null +++ b/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php @@ -0,0 +1,119 @@ + [ + 'allowedHosts' => [], + 'isCli' => true, + 'allowed' => true, + ], + 'cli ignores config' => [ + 'allowedHosts' => ['example.org'], + 'isCli' => true, + 'allowed' => true, + ], + 'HTTP allow all' => [ + 'allowedHosts' => [], + 'isCli' => false, + 'allowed' => true, + ], + 'HTTP allow all explicit' => [ + 'allowedHosts' => ['*'], + 'isCli' => false, + 'allowed' => true, + ], + 'HTTP allow explicit host' => [ + 'allowedHosts' => ['www.example.com'], + 'isCli' => false, + 'allowed' => true, + ], + 'HTTP allow explicit host multiple values' => [ + 'allowedHosts' => ['example.com', 'example.org', 'ww.example.org', 'www.example.com'], + 'isCli' => false, + 'allowed' => true, + ], + 'HTTP allow explicit host string' => [ + 'allowedHosts' => 'example.com,example.org,ww.example.org,www.example.com', + 'isCli' => false, + 'allowed' => true, + ], + 'HTTP host mismatch (missing subdomain)' => [ + 'allowedHosts' => ['example.com'], + 'isCli' => false, + 'allowed' => false, + ], + 'HTTP host mismatch (different tld)' => [ + 'allowedHosts' => ['example.org'], + 'isCli' => false, + 'allowed' => false, + ], + 'HTTP host mismatch multiple' => [ + 'allowedHosts' => ['example.org', 'www.example.org', 'example.com'], + 'isCli' => false, + 'allowed' => false, + ], + 'HTTP host mismatch string' => [ + 'allowedHosts' => 'example.org,www.example.org,example.com', + 'isCli' => false, + 'allowed' => false, + ], + ]; + } + + /** + * @dataProvider provideProcess + */ + public function testProcess(string|array $allowedHosts, bool $isCli, bool $allowed): void + { + $reflectionEnvironment = new ReflectionClass(Environment::class); + $origIsCli = $reflectionEnvironment->getStaticPropertyValue('isCliOverride'); + $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $isCli); + + try { + $middleware = new AllowedHostsMiddleware(); + $middleware->setAllowedHosts($allowedHosts); + $request = new HTTPRequest('GET', '/'); + $request->addHeader('host', 'www.example.com'); + $defaultResponse = new HTTPResponse(); + + $result = $middleware->process($request, function () use ($defaultResponse) { + return $defaultResponse; + }); + + if ($allowed) { + $this->assertSame(200, $result->getStatusCode()); + $this->assertSame($defaultResponse, $result); + } else { + $this->assertSame(400, $result->getStatusCode()); + $this->assertNotSame($defaultResponse, $result); + } + } finally { + $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $origIsCli); + } + } + + public function testProcessInvalidConfig(): void + { + $middleware = new AllowedHostsMiddleware(); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('The wildcard "*" cannot be used in conjunction with actual hosts.'); + + $middleware->setAllowedHosts(['*', 'www.example.com']); + } +} diff --git a/tests/php/Core/KernelTest.php b/tests/php/Core/KernelTest.php index 42f05c2714e..35ed9b06892 100644 --- a/tests/php/Core/KernelTest.php +++ b/tests/php/Core/KernelTest.php @@ -3,10 +3,16 @@ namespace SilverStripe\Core\Tests; use BadMethodCallException; +use Exception; +use Monolog\Logger; +use Psr\Log\LoggerInterface; +use ReflectionClass; use SilverStripe\Control\Director; +use SilverStripe\Control\Middleware\AllowedHostsMiddleware; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\ConfigLoader; use SilverStripe\Core\CoreKernel; +use SilverStripe\Core\Environment; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Injector\InjectorLoader; use SilverStripe\Core\Kernel; @@ -81,4 +87,59 @@ public function testInvalidConfigDetection() $kernel->getConfigLoader()->getManifest(); } + + public function provideAllowedHostsWarning(): array + { + $scenarios = [ + [ + 'config' => [], + 'isCli' => true, + 'shouldLog' => true, + ], + [ + 'config' => ['*'], + 'isCli' => true, + 'shouldLog' => false, + ], + [ + 'config' => ['www.example.com', 'example.org'], + 'isCli' => true, + 'shouldLog' => false, + ], + ]; + // Test both in CLI and non-CLI context + foreach ($scenarios as $name => $scenario) { + $scenario['isCli'] = false; + $scenarios[$name . ' (non-CLI)'] = $scenario; + } + return $scenarios; + } + + /** + * @dataProvider provideAllowedHostsWarning + */ + public function testAllowedHostsWarning(array $config, bool $isCli, bool $shouldLog): void + { + // Prepare mock to check if a warning is logged or not + $mockLogger = $this->getMockBuilder(Logger::class)->setConstructorArgs(['testLogger'])->getMock(); + $expectLog = $shouldLog ? $this->once() : $this->never(); + $mockLogger->expects($expectLog)->method('warning'); + Injector::inst()->registerService($mockLogger, LoggerInterface::class); + + // Set the config in our middleware + $middleware = Injector::inst()->get(AllowedHostsMiddleware::class, true); + $middleware->setAllowedHosts($config); + + $reflectionEnvironment = new ReflectionClass(Environment::class); + $origIsCli = $reflectionEnvironment->getStaticPropertyValue('isCliOverride'); + $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $isCli); + + try { + $kernel = Injector::inst()->get(Kernel::class); + $kernel->nest(); // $kernel is no longer current kernel + $kernel->boot(); + } finally { + $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $origIsCli); + } + } } From 671b1e1c01044897c0eee900afe92d4ea78dad87 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 20 Feb 2025 12:33:35 +1300 Subject: [PATCH 2/2] NEW Form sudo mode --- composer.json | 1 + src/Forms/Form.php | 45 +++++++++++++++++ src/Forms/GridField/GridField.php | 35 ++++++++++++++ .../GridFieldDetailForm_ItemRequest.php | 8 ++++ src/Forms/GridField/GridFieldViewButton.php | 19 +++++++- src/Forms/SudoModePasswordField.php | 29 +++++++++++ src/Security/Group.php | 2 + src/Security/Member.php | 2 + .../SessionAuthenticationHandler.php | 5 -- src/Security/Permission.php | 2 + src/Security/PermissionRole.php | 2 + src/Security/PermissionRoleCode.php | 2 + src/Security/SudoMode/SudoModeService.php | 23 ++++++--- src/View/ViewableData.php | 15 ++++++ tests/php/Forms/FormTest.php | 48 +++++++++++++++++++ .../Security/SudoMode/SudoModeServiceTest.php | 6 +-- 16 files changed, 228 insertions(+), 16 deletions(-) create mode 100644 src/Forms/SudoModePasswordField.php diff --git a/composer.json b/composer.json index 97c5664e612..75c8e9be42c 100644 --- a/composer.json +++ b/composer.json @@ -72,6 +72,7 @@ }, "conflict": { "silverstripe/behat-extension": "<5.5", + "silverstripe/mfa": "<5.4", "egulias/email-validator": "^2", "oscarotero/html-parser": "<0.1.7", "symfony/process": "<5.3.7" diff --git a/src/Forms/Form.php b/src/Forms/Form.php index 31493df977a..d34a9cb18bf 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -3,6 +3,7 @@ namespace SilverStripe\Forms; use BadMethodCallException; +use SilverStripe\Admin\LeftAndMain; use SilverStripe\Control\Controller; use SilverStripe\Control\HasRequestHandler; use SilverStripe\Control\HTTPRequest; @@ -21,6 +22,10 @@ use SilverStripe\View\SSViewer; use SilverStripe\View\ViewableData; use SilverStripe\Dev\Deprecation; +use SilverStripe\Security\SudoMode\SudoModeServiceInterface; +use SilverStripe\Forms\GridField\GridFieldDetailForm_ItemRequest; +use SilverStripe\Forms\GridField\GridField; +use SilverStripe\Forms\GridField\GridFieldViewButton; /** * Base class for all forms. @@ -317,6 +322,46 @@ public function __construct( $this->setupDefaultClasses(); } + /** + * Make the form require sudo mode, which will make the form readonly and add a sudo mode password field + * unless the current user previously activated sudo mode. + * + * Note that if the parent request handler for this form isn't LeftAndMain or GridFieldDetailForm_ItemRequest, + * sudo mode will not be required by this form. + */ + public function requireSudoMode(): void + { + // Check that the current request handler for the form is one that's used + // in an admin context where sudo mode makes sense + $classes = [ + LeftAndMain::class, + GridFieldDetailForm_ItemRequest::class, + ]; + $enableSudoMode = false; + foreach ($classes as $class) { + if (is_a($this->getController(), $class)) { + $enableSudoMode = true; + break; + } + } + if (!$enableSudoMode) { + return; + } + // Check if sudo mode is currently enabled + $service = Injector::inst()->get(SudoModeServiceInterface::class); + $session = $this->getRequest()->getSession(); + if ($service->check($session)) { + return; + } + // If sudo mode is not active, make the form readonly and add a sudo mode password field + $this->makeReadonly(); + $field = SudoModePasswordField::create(SudoModePasswordField::FIELD_NAME); + // Manually call setForm() to the field as the field list is being updated after the + // form is created, which is when setForm() is normally being created + $field->setForm($this); + $this->Fields()->unshift($field); + } + /** * @return bool */ diff --git a/src/Forms/GridField/GridField.php b/src/Forms/GridField/GridField.php index 7745978887b..0d99b432861 100644 --- a/src/Forms/GridField/GridField.php +++ b/src/Forms/GridField/GridField.php @@ -28,6 +28,9 @@ use SilverStripe\ORM\SS_List; use SilverStripe\View\HTML; use SilverStripe\View\ViewableData; +use SilverStripe\Security\SudoMode\SudoModeServiceInterface; +use SilverStripe\ORM\DataObject; +use SilverStripe\Forms\GridField\GridFieldViewButton; /** * Displays a {@link SS_List} in a grid format. @@ -521,6 +524,31 @@ public function FieldHolder($properties = []) { $this->extend('onBeforeRenderHolder', $this, $properties); + // Set GridField to read-only if sudo mode is required for the DataObject being managed + // and sudo mode is not active + $sudoModeTransformation = false; + $modelClass = null; + try { + $modelClass = $this->getModelClass(); + } catch (LogicException) { + // noop - it's possible to have a gridfield with custom components that don't rely on columns + // from the records in the list. + } + if (is_a($modelClass, DataObject::class, true)) { + /** @var DataObject $obj */ + $obj = Injector::inst()->create($modelClass); + if ($obj->getRequireSudoMode()) { + $session = Controller::curr()?->getRequest()?->getSession(); + if ($session) { + $service = Injector::inst()->get(SudoModeServiceInterface::class); + if (!$service->check($session)) { + $this->performReadonlyTransformation(); + $sudoModeTransformation = true; + } + } + } + } + $columns = $this->getColumns(); $list = $this->getManipulatedList(); @@ -552,6 +580,13 @@ public function FieldHolder($properties = []) if ($item instanceof GridFieldPaginator) { $total = $item->getTotalItems(); } + if ($sudoModeTransformation) { + // Modify the GridFieldViewButton on any GridFields so that it doesn't suffix the view URL with 'view' + // This allows us to gracefully reload a form in readonly mode when sudo mode is activated + if ($item instanceof GridFieldViewButton) { + $item->setSuffixViewToUrl(false); + } + } } foreach ($content as $contentKey => $contentValue) { diff --git a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php index 5421edc8b1a..0632c315cf0 100644 --- a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php +++ b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php @@ -322,7 +322,15 @@ public function ItemEditForm() if ($cb) { $cb($form, $this); } + $this->extend("updateItemEditForm", $form); + + // Check if the the record is a DataObject and if that DataObject requires sudo mode + // If so then require sudo mode for the item edit form + if (is_a($this->record, DataObject::class) && $this->record->getRequireSudoMode()) { + $form->requireSudoMode(); + } + return $form; } diff --git a/src/Forms/GridField/GridFieldViewButton.php b/src/Forms/GridField/GridFieldViewButton.php index a0efccd6a67..d31738eb099 100644 --- a/src/Forms/GridField/GridFieldViewButton.php +++ b/src/Forms/GridField/GridFieldViewButton.php @@ -13,6 +13,19 @@ */ class GridFieldViewButton extends AbstractGridFieldComponent implements GridField_ColumnProvider, GridField_ActionMenuLink { + private bool $suffixViewToUrl = true; + + public function getSuffixViewToUrl(): bool + { + return $this->suffixViewToUrl; + } + + public function setSuffixViewToUrl(bool $suffixViewToUrl): static + { + $this->suffixViewToUrl = $suffixViewToUrl; + return $this; + } + /** * @inheritdoc */ @@ -44,7 +57,11 @@ public function getExtraData($gridField, $record, $columnName) */ public function getUrl($gridField, $record, $columnName) { - $link = Controller::join_links($gridField->Link('item'), $record->ID, 'view'); + $parts = [$gridField->Link('item'), $record->ID]; + if ($this->getSuffixViewToUrl()) { + $parts[] = 'view'; + } + $link = Controller::join_links(...$parts); return $gridField->addAllStateToUrl($link); } diff --git a/src/Forms/SudoModePasswordField.php b/src/Forms/SudoModePasswordField.php new file mode 100644 index 00000000000..66f79e8317b --- /dev/null +++ b/src/Forms/SudoModePasswordField.php @@ -0,0 +1,29 @@ +setTitle(''); + $this->addExtraClass('SudoModePasswordField'); + } + + public function performReadonlyTransformation() + { + // Readonly transformation should not be applied to this field + // as this field is intended to be used on a form that has been set to read only mode + return $this; + } +} diff --git a/src/Security/Group.php b/src/Security/Group.php index dff5e81b84e..c708674186b 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -90,6 +90,8 @@ class Group extends DataObject 'Code' => true, 'Sort' => true, ]; + + private static bool $require_sudo_mode = true; public function getAllChildren() { diff --git a/src/Security/Member.php b/src/Security/Member.php index 217c474727f..e8573230f52 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -108,6 +108,8 @@ class Member extends DataObject //'AutoLoginHash' => Array('type'=>'unique', 'value'=>'AutoLoginHash', 'ignoreNulls'=>true) ]; + private static bool $require_sudo_mode = true; + /** * @config * @var boolean diff --git a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php index 252bb749315..d18658adf23 100644 --- a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php +++ b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php @@ -82,11 +82,6 @@ public function logIn(Member $member, $persistent = false, HTTPRequest $request if (Member::config()->get('login_marker_cookie')) { Cookie::set(Member::config()->get('login_marker_cookie'), 1, 0); } - - // Activate sudo mode on login so the user doesn't have to reauthenticate for sudo - // actions until the sudo mode timeout expires - $service = Injector::inst()->get(SudoModeServiceInterface::class); - $service->activate($session); } /** diff --git a/src/Security/Permission.php b/src/Security/Permission.php index ce57f81bbf3..d0e804c5a3b 100644 --- a/src/Security/Permission.php +++ b/src/Security/Permission.php @@ -100,6 +100,8 @@ class Permission extends DataObject implements TemplateGlobalProvider, Resettabl 'EDIT_PERMISSIONS' ]; + private static bool $require_sudo_mode = true; + /** * Check that the current member has the given permission. * diff --git a/src/Security/PermissionRole.php b/src/Security/PermissionRole.php index 5f96f877024..93c30304ca9 100644 --- a/src/Security/PermissionRole.php +++ b/src/Security/PermissionRole.php @@ -46,6 +46,8 @@ class PermissionRole extends DataObject private static $plural_name = 'Roles'; + private static bool $require_sudo_mode = true; + public function getCMSFields() { $fields = parent::getCMSFields(); diff --git a/src/Security/PermissionRoleCode.php b/src/Security/PermissionRoleCode.php index 8d7e2979bd0..4011785abed 100644 --- a/src/Security/PermissionRoleCode.php +++ b/src/Security/PermissionRoleCode.php @@ -23,6 +23,8 @@ class PermissionRoleCode extends DataObject ]; private static $table_name = "PermissionRoleCode"; + + private static bool $require_sudo_mode = true; private static $indexes = [ "Code" => true, diff --git a/src/Security/SudoMode/SudoModeService.php b/src/Security/SudoMode/SudoModeService.php index 74fa8547fe4..6a0e158f65b 100644 --- a/src/Security/SudoMode/SudoModeService.php +++ b/src/Security/SudoMode/SudoModeService.php @@ -4,11 +4,13 @@ use SilverStripe\Control\Session; use SilverStripe\Core\Config\Configurable; +use SilverStripe\Core\Extensible; use SilverStripe\ORM\FieldType\DBDatetime; class SudoModeService implements SudoModeServiceInterface { use Configurable; + use Extensible; /** * The lifetime that sudo mode authorization lasts for, in minutes. @@ -27,16 +29,18 @@ class SudoModeService implements SudoModeServiceInterface public function check(Session $session): bool { + $active = true; $lastActivated = $session->get(SudoModeService::SUDO_MODE_SESSION_KEY); - // Not activated at all if (!$lastActivated) { - return false; + // Not activated at all + $active = false; + } else { + // Activated within the last "lifetime" window + $nowTimestamp = DBDatetime::now()->getTimestamp(); + $active = $lastActivated > ($nowTimestamp - $this->getLifetime() * 60); } - - // Activated within the last "lifetime" window - $nowTimestamp = DBDatetime::now()->getTimestamp(); - - return $lastActivated > ($nowTimestamp - $this->getLifetime() * 60); + $this->extend('updateCheck', $active, $session); + return $active; } public function activate(Session $session): bool @@ -45,6 +49,11 @@ public function activate(Session $session): bool return true; } + public function deactivate(Session $session): void + { + $session->set(SudoModeService::SUDO_MODE_SESSION_KEY, null); + } + public function getLifetime(): int { return (int) static::config()->get('lifetime_minutes'); diff --git a/src/View/ViewableData.php b/src/View/ViewableData.php index 655b1c8f266..b53d6a3a6ae 100644 --- a/src/View/ViewableData.php +++ b/src/View/ViewableData.php @@ -78,6 +78,13 @@ class ViewableData implements IteratorAggregate */ private array $dynamicData = []; + /** + * Config of whether the model requires sudo mode to be active in order to be modified in admin + * Sudo mode is a security feature that requires the user to re-enter their password before + * making changes to the database. + */ + private static bool $require_sudo_mode = false; + // ----------------------------------------------------------------------------------------------------------------- /** @@ -261,6 +268,14 @@ public function hasDynamicData(string $field): bool return array_key_exists($field, $this->dynamicData); } + /** + * Whether the model requires sudo mode to be active in order to be modified in admin + */ + public function getRequireSudoMode(): bool + { + return static::config()->get('require_sudo_mode'); + } + /** * Returns true if a method exists for the current class which isn't private. * Also returns true for private methods if $this is ViewableData (not a subclass) diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index 05ad1f2055f..80defa8a41f 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -33,6 +33,13 @@ use SilverStripe\Security\SecurityToken; use SilverStripe\View\ArrayData; use SilverStripe\View\SSViewer; +use SilverStripe\Forms\GridField\GridFieldDetailForm; +use SilverStripe\Forms\GridField\GridFieldDetailForm_ItemRequest; +use SilverStripe\Security\MemberAuthenticator\LostPasswordHandler; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\Forms\SudoModePasswordField; +use SilverStripe\Security\SudoMode\SudoModeServiceInterface; +use SilverStripe\Forms\ReadonlyField; class FormTest extends FunctionalTest { @@ -1263,6 +1270,47 @@ public function testRestoreFromState() ); } + public static function provideRequireSudoMode(): array + { + return [ + 'sudo-protected' => [ + 'class' => GridFieldDetailForm_ItemRequest::class, + 'expected' => true, + ], + 'not-sudo-protected' => [ + 'class' => LostPasswordHandler::class, + 'expected' => false, + ], + ]; + } + + /** + * @dataProvider provideRequireSudoMode + */ + public function testRequireSudoMode(string $class, bool $expected): void + { + $request = Controller::curr()->getRequest(); + if ($class === GridFieldDetailForm_ItemRequest::class) { + $handler = new GridFieldDetailForm_ItemRequest(null, null, null, null, null); + } elseif ($class === LostPasswordHandler::class) { + $handler = new LostPasswordHandler(''); + } + $handler->setRequest($request); + $form = new Form($handler, 'MyForm', new FieldList(new TextField('MyTextField'))); + $form->requireSudoMode(); + $fieldList = $form->Fields(); + $last = $fieldList->last(); + $this->assertSame('MyTextField', $last->getName()); + if ($expected) { + $this->assertSame(2, $fieldList->count()); + $this->assertSame(SudoModePasswordField::class, get_class($fieldList->first())); + $this->assertSame(ReadonlyField::class, get_class($last)); + } else { + $this->assertSame(1, $fieldList->count()); + $this->assertSame(TextField::class, get_class($last)); + } + } + protected function getStubForm() { return new Form( diff --git a/tests/php/Security/SudoMode/SudoModeServiceTest.php b/tests/php/Security/SudoMode/SudoModeServiceTest.php index 0dca89f6023..cadd478bee9 100644 --- a/tests/php/Security/SudoMode/SudoModeServiceTest.php +++ b/tests/php/Security/SudoMode/SudoModeServiceTest.php @@ -61,7 +61,7 @@ public function testActivateAndCheckImmediately() $this->assertTrue($this->service->check($this->session)); } - public function testSudoModeActivatesOnLogin() + public function testSudoModeNotActiveOnLogin() { // Sometimes being logged in carries over from other tests $this->logOut(); @@ -73,9 +73,9 @@ public function testSudoModeActivatesOnLogin() // Sudo mode should not be enabled automagically when nobody is logged in $this->assertFalse($service->check($session)); - // Ensure sudo mode is activated on login + // Ensure sudo mode is not automatically activated on login $this->logInWithPermission(); - $this->assertTrue($service->check($session)); + $this->assertFalse($service->check($session)); // Ensure sudo mode is not active after logging out $this->logOut();