RFC: Disable access bypass for administrator role

Created on 2 April 2019, about 6 years ago
Updated 16 August 2024, 8 months ago

Field permissions wasn't behaving as configured from Field UI.
Users with the "administrator" role assigned were still able to see the field.

I found a field permissions bypass hardcoded in FieldPermissionsService.php
Request for change: Remove this administrator-role bypass.

Permissions should be "fully" controlled from the Field UI.
And have no hidden exceptions, resulting in unexpected permission behaviour.

Thank you.

🐛 Bug report
Status

Needs work

Version

1.0

Component

Code

Created by

🇳🇱Netherlands stefan looij

Live updates comments and jobs are added and updated live.
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.

  • 🇨🇦Canada teknocat

    The patch provided here does not make sense. The access checks should simply remove the condition that the user has a role named "administrator", as this is a straight up security access bypass issue.

    If the "administrator" role has the "is_admin" flag, then it will automatically have no access restrictions. This is already taken care of properly by this module when doing the actual field access checking. So why there is a need to bypass the proper checks based on the configured permissions if the user has the role called "administrator", I do not know. Any sites that have a different role with the "is_admin" flag set to true but want to let the role called "administrator" still bypass all field access restrictions are configuring their sites incorrectly.

    This hasn't come up as an issue for us before because we typically grant full access to any restricted fields to the "administrator" role anyway. However, we currently have a use case where only our "developer" role (which has the is_admin flag set to true) has access to certain fields, not the "administrator". So now this is an access bypass issue that's a problem, but simply removing the check for the role "administrator" from the if statement makes it work exactly as expected.

    @caspervoogt, to solve your issue you should not give the end user the "administrator" role if it has the all powerful "is_admin" flag and instead do like we do and have a developer role to reserve that access for you so you can then give your site's users the "administrator" role if you want to and then still be able to restrict their access to certain things. Except this issue will still be a problem for you so long as you keep the role with that name.

    Now, if there are use cases where you do want a certain role that doesn't have the "is_admin" flag to still be able to have no field access restrictions, then this module should provide a "Bypass field access control" permission that can be granted to any role, just like the core node module has a "Bypass content access control" permission you can grant if you want to.

    For now, though, IMO this security hole should be plugged and if doing so breaks a lot of sites out there that are relying on it then it's their problem to deal with. It's not uncommon for a module update to break something in the sites we manage and it's up to me as the developer to deal with that when it happens. It's my job to adjust configuration to accommodate changes in behaviour of modules, especially if a change has been done to address a security issue. I feel very strongly that it's more important for a module developer to make sure their module is secure than worry about breaking sites.

    That said, if you want to make sure devs of existing sites relying on this functionality are aware of what could break and can deal with it, then make a new major version release that includes this fix and update the module description to flag it when upgrading to that new version. And then at the same time you could add in a "Bypass field access control" permission so that way users upgrading can make use of it if they want or need to.

    I will create an issue fork and a merge request to at least remove the access bypass for now.

  • Pipeline finished with Failed
    8 months ago
    Total: 647s
    #256058
Production build 0.71.5 2024