Fix Field UI tests that rely on UID1's super user behavior

Created on 10 April 2024, 3 months ago
Updated 11 July 2024, 8 days 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.

Files that need to be fixed:

  • core/modules/field(_ui)/tests/src/
    • Functional/EntityReference/EntityReferenceXSSTest.php
    • Functional/FieldDefaultValueCallbackTest.php
    • Functional/FieldUIRouteTest.php

Remaining tasks

Commit

MR to commit

MR 8637

๐Ÿ“Œ Task
Status

RTBC

Version

11.0 ๐Ÿ”ฅ

Component
Field UIย  โ†’

Last updated 6 days ago

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
  • ๐Ÿ‡ฌ๐Ÿ‡ทGreece vensires
  • ๐Ÿ‡ฌ๐Ÿ‡ทGreece vensires
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia pradhumanjainOSL

    pradhumanjain2311 โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    3 months ago
    Total: 991s
    #150346
  • First commit to issue fork.
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฒ๐Ÿ‡ฉMoldova thebumik

    Applied necessary changes to fix these tests.

  • Pipeline finished with Failed
    3 months ago
    Total: 1078s
    #153069
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Left some comments but appears to have test failures.

  • First commit to issue fork.
  • Merge request !8637Resolve #3439835 "Fix field ui tests" โ†’ (Open) created by pooja_sharma
  • Pipeline finished with Canceled
    16 days ago
    Total: 846s
    #214184
  • Pipeline finished with Failed
    16 days ago
    Total: 164s
    #214195
  • Pipeline finished with Success
    16 days ago
    #214199
  • Pipeline finished with Success
    16 days ago
    #214210
  • Pipeline finished with Success
    16 days ago
    #214220
  • Addressed the superuser code needs to remove & added respective permissions.

    Please review MR 8637, move NR

  • Status changed to Needs review 16 days ago
  • Status changed to Needs work 10 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Left a comment on MR.

  • Status changed to Needs review 10 days ago
  • Left a comment for suggestion, Please review, move NR

  • Status changed to Needs work 10 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    See response

  • Pipeline finished with Success
    9 days ago
    Total: 672s
    #219897
  • I quickly tried to replicate title missing issue on local & able to recall it, I have fixed that title missing issue by adding 'access content' permission, however forgot to remove $this->drupalPlaceBlock('page_title_block'); from MR.

    So Removed the unrelated change , Please review , moving NR.

  • Status changed to Needs review 9 days ago
  • Status changed to RTBC 9 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Looks much better thanks.

  • Status changed to Needs work 9 days ago
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    The FieldUIRouteTest is not accessing content so why does the user need 'access content' permission? And EntityReferenceXSSTest is not editing content so it shouldn't need 'edit own article content'. Setting to needs work for checking the permissions.

  • Pipeline finished with Canceled
    9 days ago
    Total: 138s
    #220719
  • Pipeline finished with Success
    9 days ago
    Total: 464s
    #220724
  • Addressed the mentioned changes & updated the MR

    Please review , moving NR

  • Status changed to Needs review 9 days ago
  • Pipeline finished with Success
    9 days ago
    #220745
  • Addressed the mentioned changes & updated the MR

    Please review , moving NR

  • Status changed to Needs work 8 days ago
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    Thanks for working on this issue. I think there are even more permissions that can be removed. Left some comments inline.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    After conferring with @smustgrave, I filed #3460654 for a small performance optimization as a follow-up issue. For now postponed on this one so that we can get this done first.

  • Pipeline finished with Success
    8 days ago
    Total: 542s
    #221505
  • Pipeline finished with Success
    8 days ago
    Total: 466s
    #221521
  • Thanks for reviewing it, Applied the mentioned suggestions

    Please review , moving NR

  • Status changed to Needs review 8 days ago
  • Status changed to Needs work 8 days ago
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    Thanks @pooja_sharma. As far as I can see, the login in FieldUIRouteTest is still done in the setup method. Can we please move this to testFieldUIRoutes(), because I don't think it is needed for the other test in that file? Or is there a specific reason why you think it shouldn't be done?

  • @FeyP There is follow-up already created for it, may be we can keep here only minimal changes & cover these changes other follow up. Any thoughts/concerns over it?

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    There is follow-up already created for it

    No, the follow-up is to merge the to test methods, which is definitely out of scope for this issue.

    But my understanding is, also from looking at the reviews by others in this issue and other similar issues, who are a part of the same meta, that the intention is to use minimal required permissions only. And the other test in FieldUIRouteTest doesn't need any permissions at all to pass. This is also underlined by an earlier review comment from @smustgrave "The move to setup() is probably fine since this is the only test in this file." In this case we have two tests, so it is different. So I think we should move the login method into the test where it is needed as part of this issue so that the other test runs without any elevated permissions. But if you want, I can ask for a second opinion. Or wait for others to RTBC this.

  • Pipeline finished with Success
    8 days ago
    Total: 534s
    #221881
  • Pipeline finished with Success
    8 days ago
    Total: 448s
    #221893
  • Status changed to Needs review 8 days ago
  • I have applied the suggestion, there was bit confusion earlier thought of that specific 'll handle in other followup thanks for clarity got your concerns as well.

    Please review, moving NR.

  • Status changed to RTBC 8 days ago
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    Thanks @pooja_sharma, the final state is exactly what I asked for.

    Alright, I verified yesterday that the permissions now in use are the minimum required permissions. I also verified that previous review comments had been addressed. Then I looked specifically at the places where we assert that something is not on the page to make sure that this doesn't pass now just because we are missing some permissions now. Luckily those very only few places, because we mostly only assert what's there. So the code review looks good now.

    There are no super user flags left to be removed within tests in the scope of this issue. The tests pass with the changes. Rest of the pipeline also looks good.

    The issue is scoped appropriately. I'm gonna move the files from the remaining tasks section to the proposed resolution and then add the MR to commit, because we have multiple. Then the IS is fine. The issue title could be improved, but is okay as a git commit message and it makes sense to keep this similar to the other issues already committed as part of this meta. Follow up for performance optimization is filed.

    Thanks again @pooja_sharma for staying on top of this and for understanding my concerns.

    I think the issue is RTBC! โ€ž๐ŸŽ‰

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany FeyP

    FeyP โ†’ changed the visibility of the branch 3439835-fix-field-ui to hidden.

Production build 0.69.0 2024