In #540008: Add a container parameter that can remove the special behavior of UID#1 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:
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:
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:
Commit
MR 8637
Show commandsStart 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:
changes, plain diff MR !8637changes, plain diff MR !7589
Comments
Comment #2
vensiresComment #3
vensiresComment #7
thebumik commentedApplied necessary changes to fix these tests.
Comment #8
smustgrave commentedLeft some comments but appears to have test failures.
Comment #11
pooja_sharma commentedAddressed the superuser code needs to remove & added respective permissions.
Please review MR 8637, move NR
Comment #12
pooja_sharma commentedComment #13
smustgrave commentedLeft a comment on MR.
Comment #14
pooja_sharma commentedLeft a comment for suggestion, Please review, move NR
Comment #15
smustgrave commentedSee response
Comment #16
pooja_sharma commentedI quickly tried to replicate title missing issue on local & able to recall it, It was fixed on local however forgot to remove
$this->drupalPlaceBlock('page_title_block');from MR.So Removed the unrelated change , Please review , moving NR.
Comment #17
pooja_sharma commentedComment #18
smustgrave commentedLooks much better thanks.
Comment #19
quietone commentedThe 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.
Comment #20
pooja_sharma commentedApplied the suggestions & updated code
Comment #21
pooja_sharma commentedComment #22
pooja_sharma commentedAddressed the mentioned changes & updated the MR
Please review , moving NR
Comment #23
feyp commentedThanks for working on this issue. I think there are even more permissions that can be removed. Left some comments inline.
Comment #24
feyp commentedAfter conferring with @smustgrave, I filed #3460654: Merge test methods in FieldUIRouteTest for better performance for a small performance optimization as a follow-up issue. For now postponed on this one so that we can get this done first.
Comment #25
pooja_sharma commentedThanks for reviewing it, Applied the mentioned suggestions
Please review , moving NR
Comment #26
pooja_sharma commentedComment #27
feyp commentedThanks @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?Comment #28
pooja_sharma commented@FeyP There is follow-up already created for it, may be we can keep here only minimal changes required to remove super-user dependency code w.r.t. to issue & can cover these changes in other follow up. Any thoughts/concerns over it?
Comment #29
feyp commentedNo, the follow-up is to merge the two 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
FieldUIRouteTestdoesn'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.Comment #30
pooja_sharma commentedI 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.
Comment #31
feyp commentedThanks @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! 🎉
Comment #33
nod_Comment #37
nod_Committed and pushed 139bf72968 to 11.x and 35ee84ae5f to 11.0.x and 554f305ce5 to 10.4.x and 911c200988 to 10.3.x. Thanks!