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

Created on 10 April 2024, 3 months ago
Updated 10 July 2024, 3 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:
    core/modules/file/tests
    • src/Kernel/SaveTest.php

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

MR to commit

MR 8651

πŸ“Œ Task
Status

RTBC

Version

11.0 πŸ”₯

Component
File moduleΒ  β†’

Last updated 1 day 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
  • First commit to issue fork.
  • Merge request !7428Issue #3439836: Fix fiel save tests β†’ (Open) created by thebumik
  • Status changed to Needs review 3 months ago
  • πŸ‡²πŸ‡©Moldova thebumik

    Fixed test, and removed deprecations.

  • Pipeline finished with Failed
    3 months ago
    Total: 961s
    #143105
  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Appears to have some test failures.

  • First commit to issue fork.
  • Merge request !8651issue/3439836: fix file test β†’ (Open) created by pooja_sharma
  • Pipeline finished with Canceled
    10 days ago
    Total: 190s
    #215027
  • Pipeline finished with Success
    10 days ago
    #215029
  • Pipeline finished with Success
    10 days ago
    Total: 513s
    #215043
  • Addressed the superuser code needs to remove & added respective code.

    Please review MR 8651, move NR

  • Status changed to Needs review 10 days ago
  • Pipeline finished with Success
    10 days ago
    Total: 560s
    #215052
  • Addressed the superuser code needs to remove & added relevant code for test.

    Please review MR 8651, move NR

  • Status changed to RTBC 5 days ago
  • πŸ‡©πŸ‡ͺGermany FeyP

    Thanks for working on this issue!

    Okay, so this was interesting. The test fails, because we validate the file in two cases and the default user created in the base test with the uid 1 is not active (status != 1), so it can't be referenced, which triggers an extra violation that we're not expecting. The UserCreationTrait on the other hand always creates the user with status = 1, so it is not affected by this.

    Of course it is probably not a good idea to just add status = 1 to the base test. So I think creating a new active user as it is implemented in the MR is the correct fix. Technically, it would have been enough to just use this new user id for the two files where we call validate() later and leave the rest as is, but I think it doesn't hurt here to use the active users to create the other files as well, so I won't block on this minor nit.

    The IS looks good. Updating remaining tasks and adding MR to commit as we have multiple. The issue scope seems appropriate. The issue title could be improved, but it seems acceptable as a git message and I guess it makes sense to keep that in line with the other similar issues that have already been committed, so going along with that.

    Code review looks good. Tests pass with the changes. No further tests requiring super user in file module left.

    The issue is RTBC πŸŽ‰.

  • πŸ‡©πŸ‡ͺGermany FeyP

    Forgot to add the tag, sorry for the noise.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    smustgrave β†’ changed the visibility of the branch 3439836-fix-file-tests to hidden.

  • Pipeline finished with Success
    3 days ago
    Total: 470s
    #220439
  • Rebased MR with latest code, seems working fine.

  • Status changed to Needs work 3 days ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?

    ❯ grep -nri "'uid' => 1"
    tests/src/Kernel/Migrate/d6/MigrateUploadTest.php:43:        'uid' => 1,
    tests/src/Kernel/Validation/FileValidatorTestBase.php:48:      'uid' => 1,
    tests/src/Kernel/FileManagedUnitTestBase.php:40:    $user = User::create(['uid' => 1, 'name' => $this->randomMachineName()]);
    tests/src/Kernel/FileManagedUnitTestBase.php:170:      'uid' => 1,
    tests/src/Kernel/FileManagedAccessTest.php:50:      'uid' => 1,
    tests/src/Kernel/FileManagedAccessTest.php:65:      'uid' => 1,
    tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:33:        'uid' => 1,
    tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:43:        'uid' => 1,
    tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:53:        'uid' => 1,
    tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:68:        'uid' => 1,
    tests/src/Kernel/Plugin/migrate/source/d6/FileTest.php:78:        'uid' => 1,
    tests/src/Kernel/FileUriItemTest.php:27:      'uid' => 1,
    tests/src/Functional/FileManagedTestBase.php:157:      'uid' => 1,
    tests/src/Functional/FileListingTest.php:235:      'uid' => 1,
    tests/src/Functional/FileListingTest.php:268:      'uid' => 1,
    
  • This issue is specifically related to the remove the superuser dependency code, so there is only one file detect where with superuser code exist:
    protected bool $usesSuperUserAccessPolicy = TRUE;

    After removing the superuser code, this further leads to test failure for uid attribute in File::create() API, so updated uid variable only in the : file/tests/src/Kernel/SaveTest.php

    setting back to RTBC

  • Status changed to Needs review 3 days ago
  • πŸ‡©πŸ‡ͺGermany FeyP

    I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?

    Without using the super user policy, these tests don't use any special behavior so the UID does not need to be changed. It is then just a regular user without any special access. That's also why those tests didn't have the flag added in the first place and why I said in my review in #13 that it would have been enough to change the user in just one place basically to complete this issue.

    So yes, we could change the UID for the sake of not using UID 1, but it is not required.

    Setting back to Needs review, because the MR was rebased.

  • Status changed to RTBC 3 days ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    OK thanks for clarifying @FeyP. Going to mark this RTBC.

  • πŸ‡©πŸ‡ͺGermany FeyP

    @kim.pepper Thanks for taking the extra review off my list, I appreciate it :)

    @pooja_sharma Unless I'm missing something here, I think the last rebase was not really required. So for next time: When you rebase an MR, the issue needs to go back to Needs Review. A second review is usually quicker than the first one, because the reviewer is already familiar with the issue and your changes, but it still takes time for you to do the rebase and for the reviewer to (re-)review. So once an issue is RTBC (or even Needs Review, you don't want to rebase while someone is in the middle of reviewing your issue and then they have to start all over again from scratch) you want to rebase only, if it is really necessary. As long as there are no merge conflicts, GitLab will do a rebase automatically, once a core committer merges the MR. A merge conflict usually happens, if there are more recent changes in the main branch that modify one or more of the same files that you are modifying in the MR. GitLab will usually also show you, if there is a merge conflict and a rebase is required. You can see this by looking at the overview page of the MR. So if you are unsure whether a rebase is really needed, it is usually enough to open the overview page of the MR in GitLab and it will tell you: Instead of "Ready to merge by members who can write to the target branch." there will be a message "Merge blocked: 1 check failed" and "Merge conflicts must be resolved.". Note that you must have access to the issue fork for this to work, so if you don't have access yet, you need to request it via the issue.

  • @FeyP Thanks for reviewing, there is already displaying conflicts errors on MR with error message: need to resolve conflicts locally, due to which I have resolved the conflicts & updated MR as well

  • πŸ‡©πŸ‡ͺGermany FeyP

    @pooja_sharma Alright. Then I missed something in the upstream changes that caused the conflict. Sorry for that. But why did you now update the remaining tasks again? You completed the update for this file, so this is not a remaining task, it is done now. Can you please change back the IS?

Production build 0.69.0 2024