Permission to access upgrade paths should not be validated only against the user id 1

Created on 8 February 2024, 9 months ago
Updated 9 May 2024, 6 months ago

Problem/Motivation

I tried to upgrade a Drupal site using migration_ui and the url https://mydruplasite.com/upgrade, but I always received an Access Denied Error.

On the error logs I saw:

Uncaught PHP Exception Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException: "" at AccessAwareRouter.php

After researching and checking all permissions, rebuild cache, registry and so on I couldn't get it working.

I decided to debug step by step the access to the path and I found the problem in the file: core/modules/migrate_drupal_ui/src/MigrateAccessCheck.php:

The follow function checks the user using the user id:

 public function checkAccess(AccountInterface $account) {
    // The access result is uncacheable because it is just limiting access to
    // the migrate UI which is not worth caching.
    return AccessResultAllowed::allowedIf((int) $account->id() === 677)->mergeCacheMaxAge(0);
  }

Since I'm an external worker on the current project I'm working on, I have received an user with the role "Administrator" but obviously it doesn't have the ID 1. So the fact is that I can't execute the upgrades although my user is an admin user.

I am sure, I am not an isolated case. So I think, it would be better to validate the access checking if the user has id 1 or if the user has a permission for it.

Steps to reproduce

Use an admin user with an id other than 1 in the backend and try to access yoursiteurl/upgrade.php
You should receive an "Access Denied" error.

Proposed resolution

Validate the access checking if the user has a permission for it.

Remaining tasks

N/A

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

โœจ Feature request
Status

Closed: works as designed

Version

11.0 ๐Ÿ”ฅ

Component
Migrationย  โ†’

Last updated 1 day ago

Created by

๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Issue created by @juagarc4
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4
  • First commit to issue fork.
  • Merge request !7937Resolve #3420021 "Permission to access" โ†’ (Open) created by sukr_s
  • Pipeline finished with Failed
    6 months ago
    Total: 578s
    #166173
  • Pipeline finished with Failed
    6 months ago
    Total: 529s
    #166184
  • Pipeline finished with Success
    6 months ago
    Total: 595s
    #166204
  • Status changed to Needs review 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sukr_s
  • Status changed to Needs work 6 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Adding title update as it doesn't seem to match the change

    Tagging for issue summary as proposed solution don't believe matches the change and there are some sections missing from the standard issue template. Would this not be considered an API change? Would add the full template back.

    Tagging for sub-maintainer but having a clear issue summary usually helps them.

  • Status changed to Needs review 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sukr_s

    IS updated

  • Status changed to Needs work 6 months ago
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4

    Hi smustgrave,

    I don't understand why the ticket is not clear enough.

    The summary uses the standard template. I only removed the elements not involved in the problem. I can get them back again.
    I described the problem I found and ask is there is a good reason for not changing the current implementation by my proposed one.

    I don't think, the committed solution is the best way to solve the problem either. So I don't understanb what the title and the summary must be adapted to match this solution.

    Would this not be considered an API change? I don't think, it's an API change

    Maybe I haven't understood you.

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4
  • Status changed to Needs review 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sukr_s

    Changing the proposed solution to check only for permission and not for uid 1 since there is an initiative to remove all dependency / hardcoding on uid 1.

    Current MR is based only on permission, so setting back to NR

  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany juagarc4

    Hi sukr_s,

    Oh, I haven't read about the initiative you mention to remove hardcoded uid 1. But it's for me ok only to check for the permission.
    I tried your solutions and now it's working fine if nthe user that access have the permissions assigned.

  • Status changed to Needs work 6 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Now this ticket seems to be doing 2 things. Either the changes for the user stuff should be reverted or the changes for the migration code.

    If it sticks to just user letโ€™s just stick with adding permissions and not additional assertions or test updates

  • Status changed to Needs review 6 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sukr_s

    I'm not sure I understand. After introduction of the permission, the tests have to be updated otherwise the tests will fail. There is no change in the migration code.

    Entity count change for action has changed, not sure why that is so. If you check the history of changes, the action count has been changed multiple times without change in test data. May be subsystem owner can comment on this.

    User count will change since we have created new user with the appropriate permission.

    Since a new user has been created a conflict will occur in migration and thus an extra step is required.

    Let me know if something is not addressed.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Still believe additional assertions are out of scope of this issue

    Title and summary should still be updated though as the other issues were about removing the admin flag for user tests but this is changing the underlying functionality.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mikelutz Michigan, USA

    I need to have a conversation with the fms on this to see what we should do exactly. Yes, the new access policy means we shouldn't make uid 1 special anymore, but migrate is a special case. There cannot be any other users in the system prior to a UI upgrade, so the check is actually reasonable in this case, and with migrate_drupal_ui being deprecated and removed from core in the 11.x cycle, the issue may be moot anyway.

    Beyond the nonsense of trying to run the UI migrations without a clean database with no entities, we at least know that coming from d7, uid 1 will be an admin, as uid 1 was special in the database we are migrating from. We can't guarantee this on any other users, so the uid 1 check is appropriate in this situation regardless of the new access policy stuff.

  • Status changed to Closed: works as designed 6 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mikelutz Michigan, USA

    The check against uid 1 is appropriate here, as you cannot run the upgrade path if there are any entities in the system beyond the default ones that exist on install. You can't run the upgrade path as any other user, as your user could be overwritten with a migrated user that didn't have administrator privs, or that wasn't even your account. If migrate_drupal were staying in core in d12, we would have to do some work here to ensure that we were both UID 1 and that UID 1 had an administrator account somehow, which would be difficult as we also couldn't guarantee an administrator role we create wouldn't be overwritten on migration either. Thankfully, we don't have to worry about it since this is all going away, but the use case stated in the original issue summary is invalid.

    Since I'm an external worker on the current project I'm working on, I have received an user with the role "Administrator" but obviously it doesn't have the ID 1. So the fact is that I can't execute the upgrades although my user is an admin user.

    For this to be true, the system would have to have an additional user created, which means you can't run the upgrades anyway, so the issue is moot.

Production build 0.71.5 2024