#608118: Where can we tie a generic Field UI? points out that field ui permissions are rather coarse, e.g.: someone with "administer users" can also manage fields for users. We need to figure out a sensible scheme and implement it.
Only local images are allowed.

Only local images are allowed.

Only local images are allowed.

Files: 
CommentFileSizeAuthor
#108 administer_fields_permission.png29.03 KBDavid_Rothstein
#103 interdiff.txt1.78 KBDavid_Rothstein
#103 611294-103.patch13.03 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,767 pass(es). View
#100 fe-perms.png58.75 KBjenlampton
#92 611294.patch12.58 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 41,478 pass(es). View
#83 611294.patch12.56 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 41,463 pass(es). View
#81 611294.patch16.13 KBklausi
PASSED: [[SimpleTest]]: [MySQL] 41,106 pass(es). View
#79 611294.patch6.54 KBklausi
FAILED: [[SimpleTest]]: [MySQL] 40,812 pass(es), 392 fail(s), and 16 exception(s). View
#64 611294-64.patch31.69 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,810 pass(es). View
#57 611294-57.patch32.32 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 611294-57.patch. Unable to apply patch. See the log in the details link for more information. View
#57 interdiff.txt4.94 KBswentel
#54 611294-54.patch32.42 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,207 pass(es), 33 fail(s), and 0 exception(s). View
#52 611294-52.patch31.56 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,361 pass(es), 1 fail(s), and 0 exception(s). View
#49 611294-49.patch31.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es). View
#49 interdiff.txt310 bytesswentel
#47 611294-47.patch31.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,054 pass(es). View
#47 interdiff.txt2.79 KBswentel
#45 611294-45-fail.patch30.08 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,981 pass(es), 10 fail(s), and 0 exception(s). View
#45 611294-45.patch31.48 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,030 pass(es), 3 fail(s), and 4 exception(s). View
#45 interdiff.txt6.51 KBswentel
#34 Screen Shot 2012-11-24 at 23.17.14.png41.66 KBswentel
#34 611294-34.patch24.94 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,773 pass(es). View
#34 interdiff.txt1017 bytesswentel
#32 611294-32.patch24.95 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,804 pass(es). View
#32 interdiff.txt1008 bytesswentel
#29 611294-29.patch24.9 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,765 pass(es). View
#29 interdiff.txt2.44 KBswentel
#26 611294-26.patch23.45 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 48,730 pass(es). View
#24 611294-24.patch23.18 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,773 pass(es), 3 fail(s), and 0 exception(s). View
#23 611294-22.patch23.17 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 48,760 pass(es), 6 fail(s), and 0 exception(s). View
#19 611294-19.patch15.28 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 47,801 pass(es), 960 fail(s), and 250 exception(s). View
#17 Screen Shot 2012-11-23 at 19.40.31.png51.95 KBswentel
#17 611294-17.patch15.74 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 47,777 pass(es), 964 fail(s), and 251 exception(s). View
#9 field_ui-granular_permissions-611294-9.patch1.76 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 35,394 pass(es), 381 fail(s), and 15 exception(s). View

Comments

yched’s picture

Funny, I was thinking exactly the same thing while washing the dishes 10 minutes ago...

jim0203’s picture

subscribe

sun’s picture

Issue tags:+Novice

This should be a fairly easy task, no?

sun.core’s picture

Version:7.x-dev» 8.x-dev
catch’s picture

Priority:Critical» Major

Downgrading all D8 criticals to major per http://drupal.org/node/45111

marcingy’s picture

Category:bug» task

This is really a task not a bug.

xjm’s picture

Category:task» feature

I think the case can be made that at this point in the release cycle, this is not even a task, but a feature request.

tsphethean’s picture

Assigned:Unassigned» tsphethean
tsphethean’s picture

Status:Active» Needs review
StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] 35,394 pass(es), 381 fail(s), and 15 exception(s). View

Ok, I've taken a stab at this to start discussion but not expecting this to be a final solution or even close so please let me know if this is the wrong approach entirely or needs to be more granular. I'm pretty new to core contribution so any advice is gratefully received.

The attached patch creates a new permission per fieldable entity type. I've then replaced the existing code which defines the access check for the menu callbacks for editing and managing fields with a check on this per entity permission.

The bit that definitely still needs doing is changing the entity summary page (i.e. admin/structure/types) to hide the manage fields link - but I've so far not found out where this link is generated so haven't included in this patch. Any pointers on this would be great!

Like I say, tear it apart or let me know if the approach is wrong.

Status:Needs review» Needs work

The last submitted patch, field_ui-granular_permissions-611294-9.patch, failed testing.

tsphethean’s picture

I guess all these patches failed because the tests don't add the new permission to the test user - will wait for confirmation that the approach is correct before beginning to modify the existing tests and adding test cases to cover the new scenario of a user without permission trying to edit the fields.

David_Rothstein’s picture

Just wanted to mention that for Drupal 7, I have a contrib module posted (Field UI permissions) which does something similar to this.

swentel’s picture

Assigned:tsphethean» swentel

Assigning to myself, will work on this during the weekend.

attiks’s picture

Category:feature» bug

I just discovered this bug, building a D7 site

I wanted some people to create/edit users, but not be able to add/edit fields on users, but apparently this isn't possible unless I start overwriting the access callbacks.

attiks’s picture

xjm’s picture

Category:bug» feature

Still not really a bug.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
FAILED: [[SimpleTest]]: [MySQL] 47,777 pass(es), 964 fail(s), and 251 exception(s). View
new51.95 KB

@tsphethean The approach was ok, sorry to take it over right now, but wanted to make sure we have a patch before feature freeze which is in 7 days :)

Patch attached, there are now permissions to manage the fields and display per entity type. I think I made all changes in the tests too, let's see.

Screen Shot 2012-11-23 at 19.40.31.png

swentel’s picture

Ok content types aren't ok yet - let me check that, it seems #access doesn't work on links/operations.

swentel’s picture

StatusFileSize
new15.28 KB
FAILED: [[SimpleTest]]: [MySQL] 47,801 pass(es), 960 fail(s), and 250 exception(s). View

Better one. Do we need an upgrade path for this ?

attiks’s picture

#19 is looking good, about the upgrade path, I don't think it's needed, we'll have to make to many assumptions.

swentel’s picture

we'll have to make to many assumptions

Well, we can check for all roles and then investigate for the core entities whether they have the old permission enabled and if so enable fields and display permissions. Shouldn't be that hard I guess, but if I don't have to write it, the better :)

Status:Needs review» Needs work

The last submitted patch, 611294-19.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new23.17 KB
FAILED: [[SimpleTest]]: [MySQL] 48,760 pass(es), 6 fail(s), and 0 exception(s). View

Progress.

swentel’s picture

StatusFileSize
new23.18 KB
FAILED: [[SimpleTest]]: [MySQL] 48,773 pass(es), 3 fail(s), and 0 exception(s). View

Actually, this one should be green.

Status:Needs review» Needs work

The last submitted patch, 611294-24.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new23.45 KB
PASSED: [[SimpleTest]]: [MySQL] 48,730 pass(es). View

Green!

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Nice work, I cannot find anything wrong with this and it works!

yched’s picture

This approach works for me.

One remark : _field_ui_view_mode_menu_access() could be greatly simplified since we don't have to handle an unknown 'access_callback' specified by the entity type. The access callback is now known, it's user_access.
So it could be just :

  'access callback' => '_field_ui_view_mode_menu_access',
  'access arguments' => array($entity_type, $bundle_arg, $view_mode, 'administer ' . $entity_type . ' display')

and in _field_ui_view_mode_menu_access() :

$bundle = field_extract_bundle($entity_type, $bundle);
$view_mode_settings = field_view_mode_settings($entity_type, $bundle);
$visibility = ($view_mode == 'default') || !empty($view_mode_settings[$view_mode]['custom_settings']);

// Then, determine access according to the $perm parameter.
if ($visibility) {
  return user_access($perm);
}
swentel’s picture

StatusFileSize
new2.44 KB
new24.9 KB
PASSED: [[SimpleTest]]: [MySQL] 48,765 pass(es). View

Oh yes, much nicer, less code, great!

yched’s picture

Yup :-)
Still RTBC if it comes back green.

sun’s picture

Oh finally! AWESOME! :)

I only have a minor detail, which can be quickly addressed before commit, or as a quick follow-up patch:

+++ b/core/modules/field_ui/field_ui.module
@@ -193,6 +197,30 @@ function field_ui_menu() {
+        'title' => t('Administer ' . $label . ' fields'),
...
+        'title' => t('Administer ' . $label . ' display')

These need to use proper t() placeholders — e.g., %entity-label

swentel’s picture

StatusFileSize
new1008 bytes
new24.95 KB
PASSED: [[SimpleTest]]: [MySQL] 48,804 pass(es). View

Done. I moved the entity label to the end of the title, like the 'Edit terms in %vocabulary / Delete terms from vocabulary' permissions do, it looks so weird to have an italic in the middle :)

sun’s picture

Actually, I think we want to orient at the node permissions: The dynamic label should come first - that helps with visual orientation.

(see #738512: Node permissions are ordered oddly)

swentel’s picture

StatusFileSize
new1017 bytes
new24.94 KB
PASSED: [[SimpleTest]]: [MySQL] 48,773 pass(es). View
new41.66 KB

Oh yes, makes much sense, new patch + interdiff + screenshot.

Screen Shot 2012-11-24 at 23.17.14.png

sun’s picture

Coolio, thanks! :)

catch’s picture

Category:feature» task
Priority:Major» Normal
Status:Reviewed & tested by the community» Needs review

I'm not clear at all how 'administer content types' differs from 'administer node fields' now - they seem the same to me.

What this really looks like is "administer $entity type" vs. "administer $entity(ies)" - since most of field UI should really move to entity type administration UI anyway.

Also this looks like just tidying up inconsistent permissions to me rather than a feature, moving to task, although I don't think it's 'major'.

swentel’s picture

I'm not clear at all how 'administer content types' differs from 'administer node fields' now - they seem the same to me.

Those are kind of the same probably yes. What we also gain from this patch is that the 'Manage display' can now be assigned differently where you now need the 'administer content types' permission as well. The split of permissions is even more interesting for users. There's no way now (unless implementing hook_menu_alter()) to give permission to say an administrator to only allow managing users and not the fields and/or display of that user.

I'm not sure how to proceed now, rename field ui to entity ui ? :)

- edit - fix last sentence

yched’s picture

Rename field_ui to entity_ui totally gets my +1, but that's for post freeze and definitely not for this patch :-)

yched’s picture

The separate «manage display» perm makes sense as is anyway.
«administer node fields» and «administer content types» could probably be merged, but the 1st one is generic across entity types while the second is node-only.
We'd need to find a formulation for "administer [entity type] bundles" that makes sense in a UI...

swentel’s picture

So, back to rtbc, and refactor in february, we have all the time now right ? :)

- edit - crosspost

attiks’s picture

Admin content types allows you to edit published, promoted, preview, ... as well. Makes sense to keep this separate.

yched’s picture

Status:Needs review» Reviewed & tested by the community

back to RTBC then.

catch’s picture

Status:Reviewed & tested by the community» Needs work

So now if I have administer content types permission I can't access 'manage display'. That's weird for new sites, and it's an upgrade path bug for existing sites.

«administer node fields» and «administer content types» could probably be merged, but the 1st one is generic across entity types while the second is node-only.

Yes and it's confusing. This patch doesn't necessarily have to fix that (except for the first sentence of this comment which just looks like a bug), but we should open a follow-up to thrash it out if there isn't one already.

attiks’s picture

Status:Needs work» Needs review

Isn't it easier to fix the upgrade path, if the user had administer content types, he'll get the new permissions during upgrade?

swentel’s picture

StatusFileSize
new6.51 KB
new31.48 KB
FAILED: [[SimpleTest]]: [MySQL] 49,030 pass(es), 3 fail(s), and 4 exception(s). View
new30.08 KB
FAILED: [[SimpleTest]]: [MySQL] 48,981 pass(es), 10 fail(s), and 0 exception(s). View

So, upgrade tests added.

Status:Needs review» Needs work

The last submitted patch, 611294-45.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
new31.88 KB
PASSED: [[SimpleTest]]: [MySQL] 49,054 pass(es). View
attiks’s picture

Small typo

+++ b/core/modules/field_ui/field_ui.installundefined
@@ -0,0 +1,53 @@
+  // There's no garantuee hook_permission() will be available for this

garantuee --> guarantee

swentel’s picture

StatusFileSize
new310 bytes
new31.88 KB
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es). View

Fixed - old school interdiff :)

Status:Needs review» Needs work
Issue tags:-Novice

The last submitted patch, 611294-49.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
Issue tags:+Novice

#49: 611294-49.patch queued for re-testing.

swentel’s picture

StatusFileSize
new31.56 KB
FAILED: [[SimpleTest]]: [MySQL] 49,361 pass(es), 1 fail(s), and 0 exception(s). View

Chasing HEAD

Status:Needs review» Needs work

The last submitted patch, 611294-52.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new32.42 KB
FAILED: [[SimpleTest]]: [MySQL] 49,207 pass(es), 33 fail(s), and 0 exception(s). View

Status:Needs review» Needs work

The last submitted patch, 611294-54.patch, failed testing.

sun’s picture

Issue tags:-Novice+API clean-up

+++ b/core/modules/field_ui/field_ui.install
@@ -0,0 +1,53 @@
+function field_ui_update_8001() {
...
+  // There's no guarantee hook_permission() will be available for all
+  // modules, so get the current known permissions to make sure this
+  // update doesn't crash.
+  $modules = user_permission_get_modules();
...
+      user_role_grant_permissions($role_id, $new_permissions);

Hm. That invokes hooks.

I think we need to use direct database queries on the user permissions table here; i.e., just use a SELECT query to determine which permissions have been granted to which roles, and then INSERT new permissions records accordingly.

swentel’s picture

Status:Needs work» Needs review
Issue tags:-API clean-up+Novice
StatusFileSize
new4.94 KB
new32.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 611294-57.patch. Unable to apply patch. See the log in the details link for more information. View
swentel’s picture

Issue tags:-Novice+API clean-up

Didn't mean to remove the tags.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

#57 fixes #56 and looks good to me.

catch’s picture

Is there an issue to merge the generic vs. specific permissions for entities anywhere? While it's good to consolidate things, currently the patch is adding extra permissions rather than removing any and I'm not sure we can leave things in that state for release.

swentel’s picture

Are you referring to "«administer node fields» and «administer content types» could probably be merged, but the 1st one is generic across entity types while the second is node-only."

As Attiks says in #41: "Admin content types allows you to edit published, promoted, preview, ... as well. Makes sense to keep this separate." I kind of agree with that, also you don't need Field UI to manage content types, it's the node module who owns that screen. In case we'd want to merge those and follow the logic, I guess that would apply to users as well, although those permissions are bit WTF too imo, you can't assign roles to a user if you don't have 'administer permissions for instance.

Going to trigger a retest as well to check if this isn't broken now by #1814916: Convert menus into entities

swentel’s picture

Issue tags:-API clean-up

#57: 611294-57.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+API clean-up

The last submitted patch, 611294-57.patch, failed testing.

swentel’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new31.69 KB
PASSED: [[SimpleTest]]: [MySQL] 49,810 pass(es). View

Reroll

catch’s picture

Status:Reviewed & tested by the community» Fixed

Opened #1891686: Consolidate / standardize entity permissions. Committed/pushed this one to 8.x, thanks!

larowlan’s picture

Title:Refine permissions for Field UI» Change notice : Refine permissions for Field UI
Priority:Normal» Critical
Status:Fixed» Needs work

We need a change notice here, this will break other tests in people's patches eg #1871772: Convert custom blocks to content entities

larowlan’s picture

Issue tags:+Needs change record

Forgot tag

larowlan’s picture

Note the change notice should note that the ability to edit fields is a secure permission and perhaps we should look at an addendum http://drupal.org/node/372836 as things like #1892530: XSS in image file description (forward port of SA-CORE-2013-003) might not be needed

swentel’s picture

Status:Needs work» Needs review
larowlan’s picture

Title:Change notice : Refine permissions for Field UI» Refine permissions for Field UI
Priority:Critical» Normal
Status:Needs review» Fixed

Change notice looks the goods

Status:Fixed» Closed (fixed)

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

yched’s picture

yched’s picture

Right, field_ui_update_8001() deletes the 'administer taxonomy' perm, but that perm is still exposed in taxonomy_permission(), and used for access check on admin/structure/taxonomy and children (as well as in many tests)

jibran’s picture

Issue tags:-Needs change record

Removing Tag.

David_Rothstein’s picture

Version:8.0.x-dev» 7.x-dev
Issue summary:View changes
Status:Closed (fixed)» Patch (to be ported)
Issue tags:+needs backport to D7

Let's consider if we can backport some version of this to Drupal 7.

The situation right now in Drupal 7 doesn't make much sense. Here's the history:

  1. Somewhere in the Drupal 6 cycle, "administer content types" was added to the list of permissions that only high-level administrators should ever have, basically because it allows you to administer fields (https://www.drupal.org/node/372836).
  2. Then when Drupal 7 came out, it was possible to administer fields on many other kinds of entities, not just nodes, but the list of high-level permissions (https://www.drupal.org/security-advisory-policy) doesn't take that into account, and there's no real policy or guidelines for how contrib modules should handle it for their entities either.
  3. So now we have a situation where we have done various security releases involving the field UI, and basically saying that it's mostly only trusted users who can exploit it so in those cases it's not a real security issue, but for some entity types it could be less-trusted users (like "administer taxonomy") and therefore is a security issue.

This is confusing and not benefiting anyone. We should try to change things so that in Drupal 7, you need to be a fully-trusted user (i.e. a permission with 'restrict access' => TRUE) in order to manage fields, period.

I don't think we can backport the Drupal 8 patch directly, but perhaps we could do a version like this:

  1. Add a single "administer fields" permission to Drupal 7 core (and mark it with 'restrict access' => TRUE).
  2. Add an update function that assigns this permission to existing trusted users, maybe those with "administer site configuration".
  3. Enforce that users must have this new permission to use the Field UI, but make it additive with the existing permissions (for example, to administer taxonomy fields you'd need both the "administer fields" and "administer taxonomy" permission)...
David_Rothstein’s picture

Priority:Normal» Major
David_Rothstein’s picture

Assigned:swentel» Unassigned

Not going to assume swentel will necessarily swoop in to do the backport, 2 years after this was fixed in Drupal 8 :)

klausi’s picture

Issue tags:+Security improvements
klausi’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new6.54 KB
FAILED: [[SimpleTest]]: [MySQL] 40,812 pass(es), 392 fail(s), and 16 exception(s). View

klausi opened a new pull request for this issue.

Status:Needs review» Needs work

The last submitted patch, 79: 611294.patch, failed testing.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 41,106 pass(es). View

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

benjy’s picture

Status:Needs review» Needs work

This is a great addition for D7. I tested the patch and it all works as expected for both content types and taxonomy. I also had to give "Administer content types" or "Administer vocabularies and terms" as well to be able to manage fields for content types but that makes sense.

+++ b/modules/field_ui/field_ui.module
@@ -104,10 +104,26 @@ function field_ui_menu() {
+            $access += array(
+              'access callback' => 'user_access',
+              'access arguments' => array('administer fields'),
+            );

I can't see where $access is used again, can we not simply inline the values into the $field_ui_access array right below if we don't need it?

Also, if that's the case we can simplify the whole thing to:

          $field_ui_access = array(
            'access callback' => 'user_access',
            'access arguments' => array('administer fields'),
          );
          if (!empty($access)) {
            $field_ui_access['access callback'] = 'field_ui_admin_access';
          }

NW because the patch no longer applies, I tested against f1e15c1

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new12.56 KB
PASSED: [[SimpleTest]]: [MySQL] 41,463 pass(es). View

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Rerolled patch.

Moved the permission "administer fields" to field.module, because that permission is useful not only for Field UI but in general for the field system.

Simplified field_ui_menu(), I hope its clear now that we are adding "administer fields" in addition to the access information provided by the entity type. Example: To have access to the field UI for nodes you need the "administer fields" permission AND the "administer nodes" permission.

Status:Needs review» Needs work

The last submitted patch, 83: 611294.patch, failed testing.

Status:Needs work» Needs review

klausi queued 83: 611294.patch for re-testing.

benjy’s picture

Looks good to me.

klausi’s picture

Cool, can you set this to RTBC?

benjy’s picture

Status:Needs review» Reviewed & tested by the community

Yeah, happy to set this to RTBC, I thought maybe someone else should review since it's quite a significant change.

dcam’s picture

I'll give it another RTBC+1.

Everything looks ok to me. The update function did its job for me. Without the module-specific permission I'm unable to access that entity's field UI page.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

This is too big of a change that came too late to get into this week's release unfortunately (since it's going to be a relatively small release, I don't want to rush something big like this in). But it looks like we can aim for the next one, and hopefully get it committed a fair amount in advance of the actual release.

That will also give some time to get the word out about this; we should write up a change notice in advance, etc.

Clearly this will be a bit disruptive (it's especially likely to break people's automated tests, and could require small changes to install profiles too), although I don't think it's terribly disruptive as these are all easy fixes. It still seems worth doing to me, due to the security benefits of having one permission that controls this, and reducing confusion over what does and doesn't need a Security Advisory.

I am curious what the Field maintainers think of this backport, though?

Also wondering if there are any ways to mitigate possible disruption. For example, in the update function, maybe we should assign the new permission not just to people with 'administer site configuration', but also to people who have an existing 'restrict access' permission that grants them access to the field UI for some entity type. For example, if you have 'administer users' but not 'administer site configuration', you are trusted and can currently administer fields on users, so you should be able to after this issue also.

Overall the patch looked really good to me. One thing I noticed:

+function field_update_7004() {
...
+  foreach ($roles as $rid => $role_name) {
+    user_role_grant_permissions($rid, array('administer fields'));
+  }

This should use _update_7000_user_role_grant_permissions() instead (it might matter for a site upgrading from Drupal 6).

Note to self: We should update https://www.drupal.org/security-advisory-policy after the patch here is committed.

klausi’s picture

Status:Needs work» Needs review
StatusFileSize
new12.58 KB
PASSED: [[SimpleTest]]: [MySQL] 41,478 pass(es). View

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

* Fixed the update function
* Added change record draft: https://www.drupal.org/node/2483307
* Pinged yched as field API maintainer: https://twitter.com/_klausi_/status/595604645655015424

I think we should not grant the permission to any other user role since that is really up to the specific site to decide. I don't think we need to update https://www.drupal.org/security-advisory-policy since it already applies to any permission marked as 'restrict access' => TRUE, which we added to the new "administer fields" permission.

swentel’s picture

Regarding the place of hook_permission and the new permission in the patch,:

I'm wondering whether this belongs in Field UI module or not. You could argue that this is a permission that should never be deleted, since Field UI can be disabled etc and turning it on again makes sure that we'll still have decent access control on Field UI.

Maybe we should change the description a bit so that's clear this permission is used for Field UI.

In case we do move the permission to field ui, the update hook should move there as well then, but I guess we should implement hook_modules_enabled then to assign the right permission ? Note, not a blocker for this patch for me, rest looks fine.

klausi’s picture

I had the permission in field_ui first, but since the meaning of the permission is not only field_ui specific I think it is better located in field.module. I don't think we should mention field_ui in the permission description, but feel free to make a suggestion.

David_Rothstein’s picture

Yeah, it's a close call, but I think we want this permission to be universal so having it in the Field module (which can never be disabled) makes a bit more sense.

The new patch and the change notice look pretty good to me.

For the permission description, I'm not sure we like to use the word "entities" in user interface text, at least not without further description giving examples such as "content" and "users"? (The case of "entities" is not mentioned at https://www.drupal.org/node/604342 and I'm not sure what the current best practice for that is - tagging this issue for usability review for now.)

David_Rothstein’s picture

I'm still not sure about only granting this to people with "administer site configuration" by the way. That is not the only trusted permission that currently allows them to edit fields.

I think we should not grant the permission to any other user role since that is really up to the specific site to decide.

It is (and they can) but my point is that why would we want to change the default out from under them when we don't have to? If an account has "administer users" permission and therefore is currently a trusted administrator who can edit fields on users, why should they suddenly stop being allowed to edit fields after updating to the new version of Drupal core?

David_Rothstein’s picture

One more thing, I'm probably going to start doing occasional "Drupal 7 updates" posts soon (similar to the Drupal 8 ones currently at https://groups.drupal.org/core/updates) and if so the first post will be relatively soon, and I will plan to highlight this issue. That way we can start spreading the word about this and hopefully get any necessary feedback from module or distro maintainers before it's committed to Drupal 7.

jenlampton’s picture

Status:Needs review» Reviewed & tested by the community

I threw this patch onto a site that's using the file_entity module since that's where I ran into this permissions problem. I'm a little concerned that the "Manage fields" and "Manage display" links appear for file entities, even to those who don't have access to those pages. That creates a bit of a UX concern for users of existing contrib entity types.

The good news is that I'm only seeing that problem for file entity and not on any of the core entity pages, which all behave correctly. Hopefully contrib updates quickly to use the new permission.

So far this is looking/working well, so changing to RTBC. I'll push it up to production and will report back if we run into any issues.

jenlampton’s picture

Issue summary:View changes
StatusFileSize
new58.75 KB

Meant to add this screenshot. file entity permissions

Dave Reid’s picture

The problem with File Entity is that we didn't list the right permission in our hook_entity_info_alter() for the file bundles, so, even though the user had the 'administer file types' permission, they couldn't access the Manage Fields and Manage Display tabs. We would need an easy way to determine if this permission exists in File Entity since we cannot just rely on people using a 100% updated version of Drupal core with this new permisison.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs subsystem maintainer review

Looks like we still need to address #96, and especially #97 - we don't want someone with a permission like "administer users" to lose access to the field UI for no reason after this update runs.

I am planning the first Drupal 7 update post for sometime this month (possibly next week) so I will go ahead and highlight this issue and direct people here to try to get some discussion of the pros/cons. It is a big change with some potential for disruption -- I still definitely lean towards doing it, but we could use more input. The patch won't make it into the October release window, but we should hopefully have another D7 release relatively soon after that (perhaps in concert with 8.0.0) so it could still get in then.

Also tagging this to get some more input from maintainers, should they be interested.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new13.03 KB
PASSED: [[SimpleTest]]: [MySQL] 41,767 pass(es). View
new1.78 KB

Here's a new patch addressing #96 and #97. I also modified the draft change notice at https://www.drupal.org/node/2483307 accordingly.

I tested this with https://www.drupal.org/project/field_ui_permissions and it played nicely with that module, which is a good sign.

The Drupal 7 update post mentioned above is expected to go out on Monday, and I'll point people to this issue for feedback. A quick summary, therefore:

  1. We are considering splitting off "administer fields" to a new permission in order to make the other permissions more useful (e.g. "administer taxonomy" may be for someone who is supposed to manage the terms only, but right now it lets them change the field structure too).
  2. This also would improve Drupal's security model; it allows us to enforce that administering fields is only for trusted users (see above).
  3. However, this could cause some disruptions, including:
    • Automated tests: This would likely break contrib module automated tests. The solution would be to assign "administer fields" to appropriate users in the tests.
    • Other contributed module code: This could cause some minor broken links for administrators (e.g., "Manage fields" showing to users who do not have access to manage fields) but they won't be hard to fix.
    • Installation profiles: Profiles that intend to grant the ability to administer fields to roles other than the built-in administrator role would need to update the profile to assign these roles the "administer fields" permission directly.
    • Existing sites: The impact should be minor since we make a good effort to assign "administer fields" automatically to users who need it. But in some cases sites would need to grant the new "administer fields" permission to particular roles to preserve their ability to use the field UI.

Feedback on the pros/cons of going ahead with this change in Drupal 7 is welcome here.

klausi’s picture

Status:Needs review» Reviewed & tested by the community

Patch improvements look good.

I think the security improvement of this patch is totally worth it, reducing risk for site owners that for example need to hand out the "administer taxonomy" permission to some editors on the site. Now they cannot mess with fields on terms anymore, yay! This improvement also reduces the burden for the security team dealing with vulnerabilities where attackers have access to the Field UI. Only trusted admins should have access to that interface and now we make that assumption explicit with this new permission.

I will have to export the new permission with Features to only grant it to super admins, but that is the goal of this patch in the first place, so not really a downside.

David_Rothstein’s picture

The Drupal 7 update post mentioned above is expected to go out on Monday, and I'll point people to this issue for feedback.

I wound up delaying it until today instead (so as to not overlap with the Drupal 8 update post which also went out on Monday): https://groups.drupal.org/node/487408

We'll see if we get any interesting feedback as a result of that. If not, it sounds like people who have participated in this issue so far are completely on board, so in that case we'll go ahead with the current patch.

Bojhan’s picture

Looks like the usability issue doesn't exist anymore?

David_Rothstein’s picture

An update here: I got a small amount of positive feedback as a result of the above (and none negative) - though apparently none of the feedback was left on this issue :)

So the plan is definitely to go ahead with this. However given the possibility of #2598382: Adopt a 6-month feature release schedule for Drupal 7 (similar to Drupal 8) and use pseudo-semantic versioning I'm waiting one more release for this, since if we do what's discussed there, this issue is a perfect fit for that release (which would be on April 20). So... stay tuned and wait a little longer, but this should be coming down the pike eventually.

David_Rothstein’s picture

Issue tags:+Needs usability review
StatusFileSize
new29.03 KB

@Bojhan, well the usability question was about the permission description for the new "Administer fields" permission.

One question was whether it is OK to mention "entities" in that, although the latest version was written to avoid using that term.

I'm attaching a screenshot of the current permission description from the patch..... Any reviews of that are certainly still welcome:

Administer Fields permission description