Problem/Motivation

In #3100386: Create contrib update module test cases that use semantic versioning @dww pointed out the administer themes is not a needed for permission for the tests to pass.
Since code was just being copied in that issue it was out of scope.

But we should review the functional tests and determine if we have unneeded permissions. After that issue `UpdateSemverTestBase` will be the class to look at but we should look at all Update module functional tests.

Proposed resolution

See if we can get rid of some of the permissions given to user accounts for the tests.

Remaining tasks

User interface changes

NOne

API changes

NOne

Data model changes

None

Release notes snippet

None

Issue fork drupal-3205909

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Postponed

Postpone on #3205909: Ensure only needed permissions are used for Update module functional tests

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Postponed » Active

whoops in #3 I postponed this on itself 😂

I assume I meant #3100386: Create contrib update module test cases that use semantic versioning which is now fixed. Marking as active

kunal.sachdev made their first commit to this issue’s fork.

kunal.sachdev opened merge request !1345

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs work
kunal.sachdev’s picture

kunal.sachdev CreditAttribution: kunal.sachdev at Acquia commented
Status: Needs work » Needs review

I checked all the functional tests to remove any unnecessary permissions. This method \Drupal\Tests\update\Functional\UpdateSemverCoreTest::testModulePageSecurityUpdate was failing without the 'administer themes' permission. core/modules/update/tests/src/Functional/FileTransferAuthorizeFormTest.php had a unnecessary permission 'administer site configuration'.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: Ensure only needed permission are used for Update module functional tests » Ensure only needed permissions are used for Update module functional tests
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

The merge request looks pretty good to me, but I'm not clear on the issue scope. Are we intending to fix this problem for all of Update's functional tests, or just the ones in the merge request? Looking at other functional tests in the Update module, like UpdateUploadTest, I'm seeing some permissions (like administer site configuration) that may or may not be needed.

So I guess my thoughts are: if we're doing this for UpdateSemverTestBase and its descendants, then this merge request seems good to me (although we should probably re-title the issue and adjust the summary). If we want to check this for all of Update's tests, then we might need to go through all of them and try cutting out permissions to verify that the ones that are there, are actually needed.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Assigned: Unassigned » phenaproxima

for review

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

I did a thorough review of this by running all of the Update module's functional tests locally, in turn, with permissions disabled one-by-one to see what kinds of failures they cause. It would appear we've pretty much got everything covered here, except for one suggestion.

I'm leaving this at "needs review" so that @kunal.sachdev and/or @tedbow can implement or reject that suggestion at their discretion. Otherwise, I consider this very much RTBC.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks great. RTBC on green.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 20f3c89d02f to 10.0.x and 2a5f29ee579 to 9.4.x and 00e48f5b3ed to 9.3.x. Thanks!

  • alexpott committed 20f3c89 on 10.0.x 
    Issue #3205909 by kunal.sachdev, tedbow, phenaproxima: Ensure only...

  • alexpott committed 2a5f29e on 9.4.x 
    Issue #3205909 by kunal.sachdev, tedbow, phenaproxima: Ensure only...

  • alexpott committed 00e48f5 on 9.3.x 
    Issue #3205909 by kunal.sachdev, tedbow, phenaproxima: Ensure only...

Status: Fixed » Closed (fixed)

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