Fix System tests that rely on UID1's super user behavior

Created on 10 April 2024, almost 2 years ago
Updated 30 April 2024, over 1 year ago

Problem/Motivation

In 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed an approach was taken where we can simply flag tests that are failing if we turn off user 1's super user powers, so that they can be taken care of in a followup. This issue is to collect all of these followups.

The goal is to have no tests in Drupal core that rely on UID1's special privileges so that we:

  1. Know these tests are correctly assigning the necessary permissions to run
  2. Can turn off the super user access policy in D11, knowing it won't break core
  3. Can remove the super user access policy in D12, providing an admin account recovery tool to replace it

Steps to reproduce

Go into any of the tests flagged with:

  /**
   * {@inheritdoc}
   *
   * @todo Remove and fix test to not rely on super user.
   * @see https://www.drupal.org/project/drupal/issues/3437620
   */

And:

  1. Remove the code below that sets the usesSuperUserAccessPolicy to TRUE.
  2. Run the test to see which test methods are failing

Proposed resolution

Assign the right permissions to make the test go green without the super user access policy. Those few tests that specifically test said policy can obviously stay, but will be removed along with the policy in D12.

Remaining tasks

  1. core/modules/system/tests/src/
    • Functional/Bootstrap/DrupalMessengerServiceTest.php
    • Functional/Entity/EntityReferenceFieldCreationTest.php
    • Functional/File/FileSaveHtaccessLoggingTest.php
    • Functional/Menu/LocalTasksTest.php
    • Functional/Module/ClassLoaderTest.php
    • Functional/Module/GenericModuleTestBase.php
    • Functional/System/DateFormatsLockedTest.php
    • Functional/Theme/MaintenanceThemeUpdateRegistryTest.php
    • Functional/UpdateSystem/UpdateScriptTest.php
    • Kernel/DateFormatAccessControlHandlerTest.php
    • Kernel/Entity/EntityReferenceSelection/EntityReferenceSelectionAccessTest.php
    • Kernel/MenuAccessControlHandlerTest.php
📌 Task
Status

Fixed

Version

10.3

Component
System 

Last updated 5 months ago

No maintainer
Created by

🇬🇷Greece vensires

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @vensires
  • First commit to issue fork.
  • Pipeline finished with Failed
    almost 2 years ago
    Total: 990s
    #150345
  • First commit to issue fork.
  • Pipeline finished with Failed
    over 1 year ago
    Total: 178s
    #153162
  • Pipeline finished with Failed
    over 1 year ago
    #153172
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1102s
    #153211
  • Pipeline finished with Success
    over 1 year ago
    Total: 990s
    #153340
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Left comments on MR.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 1109s
    #153446
  • Pipeline finished with Success
    over 1 year ago
    Total: 993s
    #153475
  • Pipeline finished with Success
    over 1 year ago
    Total: 1698s
    #153898
  • Status changed to Needs review over 1 year ago
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Left more feedback.

  • 🇺🇸United States smustgrave

    Also @pradhumanjain2311 why did you just remove $usesSuperUserAccessPolicy but not try and fix any of the tests? Not saving credit for that. Please if going to work on these try and fix them.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 175s
    #155990
  • Pipeline finished with Success
    over 1 year ago
    Total: 992s
    #155998
  • Pipeline finished with Failed
    over 1 year ago
    Total: 1080s
    #156010
  • Pipeline finished with Failed
    over 1 year ago
    Total: 961s
    #156023
  • Pipeline finished with Success
    over 1 year ago
    Total: 1909s
    #156033
  • Status changed to Needs review over 1 year ago
  • 🇮🇳India sukr_s

    comments responded / resolved

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    The scope of the tickets are to just add permissions for the logged in users.

    Adding additional assertions is out of scope and definitely shouldn’t be updating the performance test as nothing here should impact that.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 8254s
    #157028
  • Pipeline finished with Canceled
    over 1 year ago
    Total: 770s
    #157151
  • Pipeline finished with Success
    over 1 year ago
    Total: 988s
    #157175
  • Status changed to Needs review over 1 year ago
  • 🇮🇳India sukr_s

    - reverted performance tests. worked now. looks like it was a random error in test the first time
    - removed one extra assert
    - keeping one assertion. If this assert is removed and the permission 'administer account settings', is also removed, the test passes but it would not be correct since the page that opens is access denied which doesn't have local tasks.

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    A follow up should then be opened for expanding test coverage. Should stick within the scope of the ticket please

  • Pipeline finished with Success
    over 1 year ago
    Total: 1531s
    #157492
  • Pipeline finished with Success
    over 1 year ago
    Total: 1022s
    #157668
  • Status changed to Needs review over 1 year ago
  • Assigned to smustgrave
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Will fix later changing to permissionless seems to be changing what's tested.

  • Pipeline finished with Success
    over 1 year ago
    Total: 1110s
    #157718
  • Issue was unassigned.
  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    All I did was some cleanup, still green

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Now that user 1 is no longer special we can make the kernel tests much nicer here...

  • Pipeline finished with Success
    over 1 year ago
    Total: 1041s
    #160329
  • Status changed to RTBC over 1 year ago
  • 🇮🇳India sukr_s

    suggestions applied

  • Status changed to Fixed over 1 year ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Committed and pushed 8aeebfb32e to 11.x and 4ed57d454b to 10.3.x. Thanks!

    • alexpott committed 4ed57d45 on 10.3.x
      Issue #3439907 by sukr_s, smustgrave, vensires, alexpott: Fix System...
    • alexpott committed 8aeebfb3 on 11.x
      Issue #3439907 by sukr_s, smustgrave, vensires, alexpott: Fix System...
Production build 0.71.5 2024