RFC: Disable access bypass for administrator role

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.

Expanding on the earlier patches and MRs, I would like to suggest a different approach.

The current “Private” field permission type is misleading. It claims to make a field private, but in practice it still allows administrators to view and edit. That means fields marked “Private” are never truly private to the user.

In my setup, the “access private fields” permission is automatically enabled for the Administrator role when configuring this module, and it cannot be disabled. This makes it impossible to have a field that is genuinely private to the author.

I reviewed teknocat’s MR, but that only modifies the Custom permissions type and does not change the behaviour of Private. I tried using the new “bypass custom field permissions” permission, but ran into the same problem, because it is also forced on for Administrators.

My solution was to:

1. Remove the hard-coded in_array('administrator') checks.
2. Remove the “access private fields” permission and its associated checks.

With this change:

• The Private field type now means what it says: only the author can view and edit.
• If I want administrators (or any other role) to have access, I can use Custom permissions instead and assign roles explicitly.

I have attached a patch showing this approach. I have not yet done a full audit of all code paths or updated tests, because I think it is important that we agree on the intended behaviour as a community before polishing the implementation. This is now the third attempt at solving the same issue, so it would be good to agree on a clear direction before refining further.