Problem/Motivation

Even if you have $settings['skip_permission_hardening'] = TRUE; in your settings.php, SiteConfigureForm::buildForm() will still call drupal_verify_install_file() with the FILE_NOT_WRITABLE flag on settings.php and the settings directory. This is annoying for people deploying their settings.php with Git and defeats the purpose of the setting in the first place.

Proposed resolution

Add a check for Settings::get('skip_permissions_hardening') to SiteConfigureForm::buildForm()

CommentFileSizeAuthor
#30 2869916-30.patch4.36 KBtstoeckler
#30 2869916-30--tests-only.patch2.61 KBtstoeckler
#30 2869916-26-30-interdiff.txt4.01 KBtstoeckler
#26 869916-25.patch4.08 KBjohnwebdev
#25 interdiff_11-25.txt2.47 KBjohnwebdev
#25 869916-25-nopatch.patch2.33 KBjohnwebdev
#22 2869916-interdiff-13-17.txt798 byteststoeckler
#22 2869916-interdiff-9-13.txt743 byteststoeckler
#17 869916-11-nopatch.patch2.11 KBjohnwebdev
#17 869916-11.patch3.86 KBjohnwebdev
#13 869916-10-nopatch.patch2.11 KBjohnwebdev
#13 869916-10.patch3.86 KBjohnwebdev
#9 869916-9.patch3.81 KBjohnwebdev
#9 869916-9-nopatch.patch2.05 KBjohnwebdev
#2 2869916-2.patch1.75 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
FileSize
2869916-2.patch1.75 KB

Here we go.

I wondered whether - if you have this setting set - it makes sense to display the message seen in the patch context in the first place. But I guess it can't hurt, so this is the minimally invasive patch. I don't mind changing the check to skip the message in this case.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lpeabody’s picture

lpeabody CreditAttribution: lpeabody commented
Status: Needs review » Reviewed & tested by the community

This worked for me! Thanks.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

We need a test here, if you want it to stick around - thanks

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work

Setting to NW based on #5.

msankhala’s picture

msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented

There is no harm/misleading in displaying this message on SiteConfigureForm even if $settings['skip_permission_hardening'] = TRUE; is set for people deploying settings.php with version control. Because this message is only going to be displayed at the time of installation only Because SiteConfigureForm is only being used at the time of installation. We can improve this warning message and mention about $settings['skip_permission_hardening'] setting and its purpose in this message. Thoguhts?

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
FileSize
869916-9-nopatch.patch2.05 KB
869916-9.patch3.81 KB

Let's see if i got this right...

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs work » Needs review

The last submitted patch, 9: 869916-9-nopatch.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: 869916-9.patch, failed testing. View results

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
FileSize
869916-10.patch3.86 KB
869916-10-nopatch.patch2.11 KB
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs work » Needs review

The last submitted patch, 13: 869916-10.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 13: 869916-10-nopatch.patch, failed testing. View results

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
FileSize
869916-11.patch3.86 KB
869916-11-nopatch.patch2.11 KB

Ok jeez. Sorry about that. This should pass.

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: 869916-11-nopatch.patch, failed testing. View results

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs work » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Issue tags: -Needs tests

This test looks really simple, great work! Removing the needs tests tag. I think this is sufficient but not setting to rtbc as I'm not sure if adding a new method to the InstallerTestBase class is the best solution.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
FileSize
2869916-interdiff-9-13.txt743 bytes
2869916-interdiff-13-17.txt798 bytes

@johndevman Thanks for writing the test, it looks good!

In general it's a best practice to provide an interdiff when working on multiple versions of a patch. See https://www.drupal.org/documentation/git/interdiff for more information.

In order to push this issue forward and since I was curious I generated the interdiffs in this case, but just as a hint for next time ;-)

The test-only patch in #9 is an interdiff from #2.

Now looking at #21 whether postInstall() is in fact needed.

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented

@tstoeckler Thanks!

@borisson_ @tstoeckler

Regarding TestInstallerBase::postInstall the reason was because of:

      // After writing settings.php, the installer removes write permissions
      // from the site directory. To allow drupal_generate_test_ua() to write
      // a file containing the private key for drupal_valid_test_ua(), the site
      // directory has to be writable.
      // BrowserTestBase::tearDown() will delete the entire test site directory.
      // Not using File API; a potential error must trigger a PHP warning.
      chmod($this->container->get('app.root') . '/' . $this->siteDirectory, 0777);
      $this->kernel = DrupalKernel::createFromRequest($request, $class_loader, 'prod', FALSE);
      $this->kernel->prepareLegacyRequest($request);
      $this->container = $this->kernel->getContainer();

which chmods the folder before the actual tests are run. Perhaps it could be moved to the tearDown instead.

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs review » Needs work
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
FileSize
869916-25-nopatch.patch2.33 KB
interdiff_11-25.txt2.47 KB
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
FileSize
869916-25.patch4.08 KB
johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented
Status: Needs work » Needs review

The last submitted patch, 25: 869916-25-nopatch.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 26: 869916-25.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
2869916-26-30-interdiff.txt4.01 KB
2869916-30--tests-only.patch2.61 KB
2869916-30.patch4.36 KB

I was playing around with this as well. I think instead of adding an assertion after the end of the installation I think it makes more sense to add those right on the site configuration form which is where the changed code is run. InstallerTestBase already has the ::setupSite() method for exactly this purpose.

That allowed me to leave InstallerTestBase untouched. I also added the reverse assertions (i.e. that the files are not writable) to InstallerTest. Also since we are now testing right on the site where the "You can make the files non-writable now." message is shown, I added an assertion for that as well.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented

Oh right, and I also renamed the test file to *Test.php (The rename itself is not in the interdiff because PhpStorm was trying to be "smart" again...)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Related issues: +#2958228: HTML output recording does not work for InstallerTestBase tests

Oh and I also replaced the deprecated function calls with the newer methods.

I also found #2958228: HTML output recording does not work for InstallerTestBase tests by the way, wouldn't mind a review ;-)

The last submitted patch, 30: 2869916-30--tests-only.patch, failed testing. View results

johnwebdev’s picture

johnwebdev CreditAttribution: johnwebdev commented

That looks more sensible to me. Great thinking. Can this be RBTC now?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented

Thanks for the endorsement @johndevman! Maybe @borisson_ feels comfortable RTBCing now that there are no longer any changes in InstallerTestBase ;-)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yes, I do feel more comfortable now. Great work @johndevman @tstoeckler!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

Crediting @larowlan and @borisson_ for insisting on tests and making them clean.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 39183a2956 to 8.6.x and b8c3bcc7d2 to 8.5.x. Thanks!

  • alexpott committed 39183a2 on 8.6.x 
    Issue #2869916 by johndevman, tstoeckler, borisson_, larowlan:...

  • alexpott committed b8c3bcc on 8.5.x 
    Issue #2869916 by johndevman, tstoeckler, borisson_, larowlan:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.