RoleAccessCheck returns Allowed if user has no role

Novice?

Hi, I will work at this problem that you talked about @amjad1233 but I think that maybe we have to open another issue for the modification of web/modules/contrib/search_api/tests/src/Kernel/Processor/RoleAccessTest.php test as it is a file that belongs to a module that is not from the core. I understand that altering this file is a mean of testing that change about the role access, but I think it can not be sent in a same patch, so that's why I think you could open another issue and add here as a related issue.

So, I tried a lot of things at this issue and I couldn't be able to reproduce the problem. I will explain in topics my steps:

1 - I altered the test provider to look like this: (I also added an other parameter to the testRoleAccess function that is called "neutral_accounts" that reefers to accounts with no user roles. )

return [
      ['role_test_1', [$account_1, $account_12], [$account_2, $account_none],[]],
      ['role_test_2', [$account_2, $account_12], [$account_1, $account_none],[]],
      ['role_test_3', [$account_12], [$account_1, $account_2, $account_none],[]],
      ['role_test_4', [$account_12], [$account_1, $account_2, $account_none],[$account_none]],
      ['role_test_5', [$account_1, $account_2, $account_12], [], []],
      ['role_test_6', [$account_1, $account_2, $account_12], [], []],
      ['role_test_7', [], [], [$account_none]],
    ];

At 'role_test_4', I'm providing $account_none (an user with no roles) to be verify with the following code:

foreach($neutral_accounts as $account) {
      $message = sprintf('Access is neutral for user %s with no roles on path %s', $account->id(), implode(', ', $account->getRoles()), $path);
      $has_access = $role_access_check->access($collection->get($path), $account);
      $this->assertEquals(AccessResult::neutral()->addCacheContexts(['user.roles']), $has_access, $message);
    }

I noticed that this part that I did, it is just the same as the $deny_accounts' foreach.

That scenario did not return me any errors, the array_diff() function dos not return empty when it is comparing with a account with no roles, it return us the roles that the route allows. So, at the end, the following return is given:

return AccessResult::neutral()->addCacheContexts(['user.roles']);

Just like it had to be.

2 - I also tried a scenario where I created a route without roles, like this:

$route_collection->add('role_test_7', new Route('/role_test_7',
      [
        '_controller' => '\Drupal\router_test\TestControllers::test1',
      ],
    ));

And put in the provider the following line:
['role_test_7', [], [], [$account_none]],

So, this time, the result is also AccessResult::neutral().

I don't know if I'm missing something, but it could be nice if you could give me more information about that.

Thank you, and I'll submit a patch in case someone wants to verify the things I said before.

Fixed CS issues

Here is a little more context than the issue summary gives from Drupal\user\Access\RoleAccessCheck:

    $rid_string = $route->getRequirement('_role');

    $explode_and = array_filter(array_map('trim', explode(',', $rid_string)));
    if (count($explode_and) > 1) {
      $diff = array_diff($explode_and, $account->getRoles());
      if (empty($diff)) {
        return AccessResult::allowed()->addCacheContexts(['user.roles']);
      }
    }

So the referenced code is executed when the _role requirement has at least one comma. (More precisely, when there are at least two non-blank parts separated by a comma.) The issue summary states,

... if we have a user without a role following code in the class would return empty diff and user will be allowed in

There are various things that might count as "without a role": $account->getRoles() might be an empty array, boolean FALSE, or NULL, for example.

An empty array leads to the array diff being the same as $explode_and, which is not empty.

A value of NULL or FALSE leads to a PHP warning, but then array_diff() returns NULL.

Looking at \Drupal\Tests\Core\Route\RoleAccessCheckTest, we already have

    $account_none = new UserSession([
      'uid' => 1,
      'roles' => [],
    ]);

and that account is granted/denied access as expected. To expose the bug, we should add

    $account_null = new UserSession([
      'uid' => 1,
      'roles' => NULL,
    ]);

We do not need to add any routes to the test, since role_test_3 and role_test_4 already have commas in their _role requirements.

Add to the test that $account_null does not get access to those two routes. You may need to do something to suppress the PHP warnings.

Remember, the goal for now is to get a failing test, to prove the bug. The next step will be to fix the bug, so that the test passes.

By the way, this issue reminds me of a bug report that I opened a long time ago: #1873606: drupalCreateUser() creates a user with Anonymous AND Authenticated roles.. I wonder if that is still a problem.

I'll work on this

Couldn't make any progress :/

Hello, I'll try to work on this.

Hi! So, I have made this patch that applies the tests and that reproduces the bug we are having.

I still trying to fix the error but I'll try do a new patch to that.

Hi! Finally I think I got it!

I've made a new patch, that time with fixes in the tests and the fix to the bug :D if someone could review it will be very helpful!

Oh sorry! I forgot to do the coding standards. Assigning back to me to fix this.

Made it! A new patch, now following the coding standards!

Moving to needs review!

Hi!

I'll try to review.

Hello,

I applied the patch #23, run the test in \Drupal\Tests\Core\Route\RoleAccessCheckTest and all pass. No errors in the test and no phpcs errors either. So I will change the status for RTBC.

thanks!

+++ b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php
@@ -109,18 +108,47 @@ public function roleAccessProvider() {
+    $account_null = new UserSession([
+      'uid' => 5,
+      'roles' => NULL,
+    ]);

I'm not really convinced by this test. For one passing in NULL for roles is completely breaking the documentation. Secondly the moment we add property typehinting to \Drupal\Core\Session\UserSession::$roles then this bug becomes impossible. At the moment it is highly unlikely because UserSession is nearly always created from \Drupal\user\Authentication\Provider\Cookie::getUserFromSession() or new \Drupal\Core\Session\AnonymousUserSession() - both of which guarantee that UserSession::getRoles() returns an array.

Furthermore I'm not sure that the issue summary has the following correct.

However, with testing if we have a user without a role following code in the class would return empty diff and user will be allowed in.

<?php
$diff = array_diff($explode_and, $account->getRoles());
      if (empty($diff)) {
        return AccessResult::allowed()->addCacheContexts(['user.roles']);
      }
?>

To reach this code $explode_add will contain more than 1 rid and if $account->getRoles() returns an empty array then $diff can not be empty.

+++ b/core/modules/user/src/Access/RoleAccessCheck.php
@@ -33,14 +33,22 @@ public function access(Route $route, AccountInterface $account) {
+      if (!isset($roles)) {
+        return AccessResult::allowed()->addCacheContexts(['user.roles']);
+      }

And what's more this is granting access when the user has a null value for roles and the route is saying it needs multiple roles to have access. This is would be introducing a security bug.

Removing the novice tag because of #26 and #27

DrupalSouth contribution day task.

Just hiding patch files

Branch form forced forward to be based from current HEAD of main.
Patch updated to identify the error condition that should NEVER occur of an account with NO role defined, and to forbid access in this situation.

Seems that the force rebase may have reverted some previous changes.

Thanks.

Un-assigning per Assigning ownership of a Drupal core issue.

Thanks @quietone for fixing up LGTM.

I do not remember this issue, but in 2022 I left a comment on the test code, so I reviewed that.

I left a nit-level suggestion on the test. As far as I am concerned, feel free to set status back to RTBC if you accept the suggestion.

If I were updating the code, then I would use multi-line syntax for the arrays in the data provider and the $this->assertEquals() lines in the test, but that is too big a change to request at this late date.

@benjifisher, thanks for the review. I have applied the suggestion.

And I agree that the suggested code changes to the test would be nice and should not hold this up. Therefore, restoring the RTBC. I am assuming that tests will pass.

The failing test is a know random fail, core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php

The actual runtime code changes look good but one comment on the test changes.

I've addressed the feedback from catch.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Test failure in core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php. Only have time to add this.

@quietone:

That test passes when I run it locally. I looked at the test: it seems unrelated to the changes for this issue, and I do not see how it could fail. If you can re-run the pipeline, then please do.

@benjifisher, thanks for looking at this issue. I reran the test and it passed, so back to needs review.

Believe all feedback for this one has been addressed, least I didn't see anything new.

Sorry folks, I think dropping assert messages in a loop is bad DX - left a comment on the MR

I'd also like to see a follow up to deprecate RoleAccessCheck. We should never be checking access by role, only by permission.

Follow up created, #3616218: Deprecate RoleAccessCheck.

My error, I was reading one thing and thinking another.

I've added assertion messages.

I did a complete review of this issue since I haven't ever looked at it before. I became concerned about @alexpott's comment in #26 that the description of the bug in the issue summary may not be correct. We also don't have access to the old test result showing the bug as requested by @benjifisher in #13. I examined the RoleAccessCheck code and couldn't understand how this bug might occur in the first place.

I downloaded the MR and did a test-only run on my local to check the results. Note that the test expects AccessResult::forbidden(), so a test-only run will always fail. Per the issue summary we expect that it will fail with AccessResult::allowed(). It does not. On the initial run it failed for the user with an empty array of roles, $account_none, with AccessResult::neutral(). I removed $account_none from the test's data provider in order to get results for $account_null, then re-ran the test. This time the test failed repeatedly with a TypeError because you can't pass NULL to array_diff() or array_intersection().

In #13 back in 2022 @benjifisher said:

A value of NULL or FALSE leads to a PHP warning, but then array_diff() returns NULL.

This is no longer accurate. Was this a PHP 7 problem before the array functions got type-hinted parameters? Have we aged out of this issue?

This looks like a "won't fix" to me. As far as I can tell, the problem does not exist as described in the issue summary. If it is still a problem, then once again someone needs to provide a failing test to prove it. Also, the issue summary must be updated in that case. For one thing, the proposed resolution isn't even up-to-date because the fix was changed to return AccessResult::forbidden().

I have updated the issue summary. And it is clear now that this returns access forbidden for the unlikely case when an account has not roles.

It is true, that typehinting will help here, but we don't know when that will happen. So, any improvement should be done now. I am also aware that 2 security team members have reviewed this in the last 3 months and have not suggested this change is not worth making.

Back for reviews.