MigrateControllerTest requires root user, remove @todo

I closed the MR so that anyone willing to tackle this issue may start the process from the very beginning.
To be honest though, I did try to find the comment described in the issue summary but couldn't find it at all in the code. Maybe something is off here or was it just me?

Working on this at Mentored Contribution at Atlanta 2025

Working on this at Mentored Contribution at Atlanta 2025

Reviewed at DrupalCon Atlanta 2025. This looks like it's ready to go.

I am a little confused. The @todo comment mentions to remove the usage of the super user, but it looks like this is still being used in the test. Does the variable also need to be removed?

For those working on this issue, can you explain why this wasn't removed too?

@mradclffe. As I read the issue is was to remove the todo comment from the test as the test required superuser access.

So what needs to be removed is that variable. Which will break the test

What needs to happen is in the setUp() the user needs to be created with necessary permissions

You can use https://www.drupal.org/project/drupal/issues/3437620 as good references

Actually I may be wrong. Will take a deeper look

Per the issue: "migrations through the UI must be run as the root user." Has that changed? Nothing in this issue mentioned changing the test. The issue was the presence of the todo, not the test itself.

I could have a go at that, but if the test __has__ to be run as superuser I think a change to another user would break the test.

Compromise lets add a small comment as to why are keeping

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Working on this.

Hello @quietone, I have the required changes. Please have a look and let me know if we require any other changes.

Thanks for making the change @koustav_mondal. The MigrateControllerTest is now failing. Running that test in isolation to debug it would be the next step here to resolving.

As part of the meta issue, #3437620: [Meta] Fix all tests that rely on UID1's super user behavior, we need to “Assign the right permissions to make the test go green without the super user access policy.”

The test is failing with an access denied error accessing /admin/reports/upgrade.

Honestly, I am not sure why that report uses the MigrateAccessCheck rather than a permission as the controller does not run migrations, it redirects to watchdog with the query parameter "type" set to "migrate_drupal_ui". The only permission that is needed is "access site reports", which is not restricted to user 1.

Maybe fixing migrate_drupal_ui.log route to use that permission instead would make sense because an administrative user with "access site reports" already has access to it?

Hi, could you please review this MR !11645?

I've restored the usesSuperUserAccessPolicy = TRUE property and removed only the outdated @todo, as discussed.
This change reflects the current behaviour where Migrate UI still requires user 1.

Please feel free to review, and if anything needs to be adjusted or clarified, please let me know!
Thank you!

I think we should answer the question as to why this test needs super user access when the test is not asserting anything about migrations and why the reports needs the special migrate access check prior to reverting the commit.

Hi @mradcliffe, thanks for highlighting that.

To clarify why this test still requires super user access:

Although the test doesn't perform migration actions itself, it accesses the /admin/reports/upgrade route. That route — as defined in migrate_drupal_ui.routing.yml — uses a _custom_access requirement pointing to \Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess().

This access check explicitly allows only UID 1:
return AccessResultAllowed::allowedIf((int) $account->id() === 1);

So, even just viewing the upgrade report (MigrateController::showLog) fails unless the test runs as the root user. Removing usesSuperUserAccessPolicy = TRUE causes an access denied error.

That’s why in the MR, we restored the flag and removed only the outdated @todo. A broader solution (e.g., updating that route to rely on a permission like access site reports) would be ideal, but it's out of scope for this issue.

Happy to help push that discussion further if needed.

Thank you!

Yes, but why do we even need it to have the special migrate access check?

If the point is to reduce the tests that depend on this special case, then maybe fixing migrate_drupal_ui.log route to use that permission instead would make sense because an administrative user with "access site reports" already has access to it?

The whole point of these issues is to make sure core no longer hard-codes anything based on the idea that user 1 is special. There are plans to remove user 1's special behavior in a future major Drupal release. So we really can't leave this test to rely on UID 1.

This access check explicitly allows only UID 1:

So that is what we need to change: Introduce a new permission and make the route check for said permission. Admins (including UID1) will automatically get the new permission, so a simple change record detailing that non-admins who want access need to be assigned the permission should do.

I don't think we need a new permission, "access site reports" permission fits because the report is already accessible by users who have access to dblog (or syslog, jsonlog) as MigrateController is a redirect.

If we added a new permission, then a non-admin user would need both that permission and "access site reports" to access a filtered list of migrate_drupal_ui.

Having it check for the "access site reports" permission would also be an acceptable solution to me. The essence is that the UID 1 check needs to go and preferably be replaced with a permission check.

I updated the issue summary to include how to resolve and removed the tag. I left the Novice tag because I think we have some consensus on the issue.

Thanks so much @mradcliffe and @kristiaanvandeneynde — that clears things up perfectly!

I’ll make the adjustments now based on the updated direction: using access site reports, removing the UID 1 check, and updating the test.

Really appreciate your help — I’ll update the MR soon!

Hi @mradcliffe
I've made the requested adjustments:

• Replaced the _custom_access on the migrate_drupal_ui.log route with _permission: 'access site reports'.
• I updated MigrateControllerTest to create and log in a user with the proper permissions (access site reports and administer views) instead of relying on $this->rootUser.
• Removed the outdated @todo and the usesSuperUserAccessPolicy flag.

All tests are now passing successfully.
I'll ask someone to test these changes tomorrow during our Contrib Day.

Thank you all for the helpful guidance throughout this issue!

Thank you for making the changes and fixes, @brandonlira. I reviewed the merge request and confirmed the changes.

LGTM.

But now we need a follow-up as I wanted to comment here that we should also remove MigrateAccessCheck then, but realized it has more implementations. So we need a follow-up where MigrateAccessCheck is removed, because it still runs UID == 1 checks.

Follow-up can be a child issue of this one: #3444429: Replace direct comparisons to user 1 in core.

Committed/pushed to 11.x, thanks!