Contrib.social

Add method to check if scope has permission

Open on Drupal.org →
Created on 30 October 2023, 8 months ago
Updated 9 January 2024, 6 months ago

Problem/Motivation

Currently in 6.0.0-beta5 scope permissions are checked by loading all permissions in the scope tree (from child scopes) into a single array of permissions. If a scope has granularity=role all of the role's permissions are copied to the array, with granularity=permission the single permission is copied to the array. Implemented in Oauth2ScopeProviderInterface::getFlattenPermissionTree.

This approach has one issue where "Admin" roles that specify the isAdmin boolean should have all access, but result with 403 errors. This is because "Admin" roles do not need to list their permissions, so depending on the logic to build a list of permissions does not work. Instead, there could be an additional check to see if it is an admin role, and thus grant access. But ideally we can defer to the existing logic: https://git.drupalcode.org/project/drupal/-/blob/10.1.5/core/modules/use...

This also causes an issue for farmOS where we are overriding the Role storage class to provide custom logic for the RoleStorage::isPermissionInRoles() method. I know this is a bit of an edge case, but I think that this storage class should be used for consistency when determining permission checking on roles.

Changing the current logic to use the RoleStorage::isPermissionInRoles() service method and/or checking if the requested permission exactly matches a scope's permission would solve both of these issues. I also think that using this approach would make permission checking of a larger scope tree faster because we can potentially return "TRUE" higher up the tree and without processing lower parts of the tree. This would have the most impact on a large scope tree with nested scopes and roles (both config entities) which use separate queries to load each from the DB. Perhaps this aspect could be further optimized in the future.

I'm including a MR with my proposed change (without tests for now). With this change I'm not sure if Oauth2ScopeProviderInterface::getFlattenPermissionTree is needed.

Steps to reproduce

Create an "Admin" role and assign to a new user (not UID=1). Try and make requests requiring permissions, observed 403 errors.

Proposed resolution

Change the OAuth token permission checking to use the RoleStorage::isPermissionInRoles() service method for roles with granularity=scope. Do not load all permissions, instead return early once access is granted.

Remaining tasks

Get review, determine if this is an improvement
Add tests

User interface changes

None.

API changes

Introduce Oauth2ScopeProviderInterface::scopeHasPermission($permission, $scope) method.

Data model changes

None.

📌 Task
Status

Fixed

Version

6.0

Component

Code

Created by

🇺🇸United States paul121 Spokane, WA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @paul121
  • @paul121 opened merge request.
  • Status changed to Needs review 8 months ago
  • Comment 8 months ago →
    🇺🇸United States paul121 Spokane, WA
  • Comment 8 months ago →
    🇺🇸United States paul121 Spokane, WA

    Update IS to clarify issue with "Admin" roles

  • Comment 8 months ago →
    🇺🇸United States paul121 Spokane, WA

    I did not add additional tests with this MR but all existing tests are passing. Including a patch for use in the meantime.

  • Comment 7 months ago →
    🇳🇿New Zealand Josh Waihi

    I did some testing of the above patch in #3404804 and it worked as expected but I did find one bug.
    In the UI, when you select an authenticate user to have a permission, all other roles inherit that permission. If that permission was not explicitly set for a scope role, then the role doesn't provide the permission even though the UI suggests implicitly selects it.

    As a work around, I had to uncheck the permission for the authenticated user, check the permission for the role, then recheck the permission for the authenticated user.

  • Status changed to RTBC 7 months ago
  • Comment 7 months ago →
    🇳🇿New Zealand Josh Waihi
  • First commit to issue fork.
  • Comment 6 months ago →
    System Message
  • Status changed to Fixed 6 months ago
  • Comment 6 months ago →
    🇳🇱Netherlands bojan_dev

    Thanks @paul121, this is a nice improvement.

    FYI: I have removed the Oauth2ScopeProviderInterface::getFlattenPermissionTree method (which is redundant now), updated the tests and fixed some coding standard/deprecations errors.

  • Comment 6 months ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024