RoleAccessCheck should provide a reason when denying access

Updated this to be a task.
Updated the issue summary to use the template

From the initial looks it looks like : web/core/lib/Drupal/Core/Access/AccessManager.php

public function check(...) {
    if (!empty($checks)) {
      $arguments_resolver = $this->argumentsResolverFactory->getArgumentsResolver($route_match, $account, $request);
      $result = AccessResult::allowed();
      foreach ($checks as $service_id) {
       // Note the andIf() here. 
       $result = $result->andIf($this->performCheck($service_id, $arguments_resolver));
      }
    }

Which triggers RoleAccessCheck at core/modules/user/src/Access/RoleAccessCheck.php L:50.

    // If there is no allowed role, give other access checks a chance.
    return AccessResult::neutral()->addCacheContexts(['user.roles']);

Which is returning AccessResult::neutral(**NO_REASON**) without a reason and thus we not getting anything in the logs.

So I will be digging further to see where it denies we could add $reason or something. So in other words we don't know if it's yet denied.

I think it would be worth trying to put a value there in place of *NO REASON* and see if that helps

@larowlan Could be a good candidate for tomorrow's code sprint hoping to resolve this one.

💯🙌

Adding $reason certainly did help as I was suspecting. Just don't know if it's a good practice to add $reason in Neutral access requests.

I've stepped through the process of testing the proposed change.

1. Set up local environment
2. Create authenticated user without access
3. Create content
4. Attempt to edit content as authenticated user without access
5. Observe error message, log out
6. Log in as admin and observe message log.

Looks great.

Added screenshot of test outcomes.

Left a code review. Will chat with Amjad on zoom about test coverage.

Crediting quietone who was also on the zoom call at the sprint

Some great suggestions from longwave

it needs to update second screenshot as messages are about roles not permissions)

fixed feedback

Left a question

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

MR should be updated to 10.1
There are also a number of open threads on 1407 that should be addressed

This seems like a super helpful feature. Is there any security implication?

Needs new patch or maybe @amjad1233 can change target branch to 10.1.x

Great suggestion let me get my hands dirty in this one.

Around to review again if needed. Just wanted to double check that there’s no security issue announcing permissions?

re-roll and fix for #19

Open questions are

1. is secure exposing roles machine names
2. suggestion to add protected method to generate the reason

@andypost Thanks for the re-roll. 💪💪💪

I tried and patch applies cleanly and site also builds up fine.

patching file 'core/modules/user/src/Access/RoleAccessCheck.php'
patching file 'core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php'

Regarding Question 1 : I believe we are doing that already at certain places, but I agree with hiding as much as we can. With that said, I will defer question to @larowlan.

Regarding Question 2: I am not sure if this class will only be extended or there will be places where we create new instance of the class... In the later case Protected would fail. I think we should crack this one in #2266817: Deprecate empty AccessInterface and remove usages. May be we explicitly define it's signature in interface.

1) is secure exposing roles machine names

I think we'd need to load the role and call $role->access('view label') for each one

2) I still like adding a method to simplify the complexity here, nested if/else can be removed with early returns in a new method

Why do we need to use $role->access('view label') in this case? I tried to use it but every route that the user does not have access, $role->access('view label') returns FALSE. The message is not displayed for the user, maybe we don't need the $role->access('view label'). What do you think?

For now I have addressed 28.2 pointed by larowlan.

+++ b/core/modules/user/src/Access/RoleAccessCheck.php
@@ -44,10 +45,37 @@ public function access(Route $route, AccountInterface $account) {
+    else {
+      return sprintf('One of the %s %s %s roles are required', implode(', ', $roles), $condition, $last);

No need for else here, as we return above, even more simplification from the new method 💪

I looked into the permissions question and roles require 'administer permissions' to view their labels, which we're not going to grant here, obviously.

I'll ask around

I have uploaded the patch
as per #30 comment.