#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.

Files: 
CommentFileSizeAuthor
#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

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

subscribe

Issue tags:+Novice

This should be a fairly easy task, no?

Version:7.x-dev» 8.x-dev

Priority:Critical» Major

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

Category:bug» task

This is really a task not a bug.

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.

Assigned:Unassigned» tsphethean

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.

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.

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

Assigned:tsphethean» swentel

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

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.

For D7 Field UI permissions works, and the tab from ds can be hidden using #1837156: Hide "manage display" tab when permissions are revoked

Category:bug» feature

Still not really a bug.

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

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

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 ?

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

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.

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.

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.

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

Green!

Status:Needs review» Reviewed & tested by the community

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

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);
}

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

Oh yes, much nicer, less code, great!

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

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

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 :)

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)

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

Coolio, thanks! :)

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'.

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

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

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...

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

- edit - crosspost

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

Status:Needs review» Reviewed & tested by the community

back to RTBC then.

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.

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?

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.

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

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

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.

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

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

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.

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.

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.

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 ]

Issue tags:-Novice+API clean-up

Didn't mean to remove the tags.

Status:Needs review» Reviewed & tested by the community

#57 fixes #56 and looks good to me.

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.

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

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.

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

Reroll

Status:Reviewed & tested by the community» Fixed

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

Title:Refine permissions for Field UIChange 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

Issue tags:+Needs change record

Forgot tag

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

Status:Needs work» Needs review

Title:Change notice : Refine permissions for Field UIRefine 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.

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)

Issue tags:-Needs change record

Removing Tag.