Posted by bjaspan on October 22, 2009 at 3:18am
22 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field_ui.module |
| Category: | task |
| Priority: | normal |
| Assigned: | swentel |
| Status: | closed (fixed) |
| Issue tags: | API clean-up |
Issue Summary
#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.
Comments
#1
Funny, I was thinking exactly the same thing while washing the dishes 10 minutes ago...
#2
subscribe
#3
This should be a fairly easy task, no?
#4
#5
Downgrading all D8 criticals to major per http://drupal.org/node/45111
#6
This is really a task not a bug.
#7
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.
#8
#9
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.
#10
The last submitted patch, field_ui-granular_permissions-611294-9.patch, failed testing.
#11
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.
#12
Just wanted to mention that for Drupal 7, I have a contrib module posted (Field UI permissions) which does something similar to this.
#13
Assigning to myself, will work on this during the weekend.
#14
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.
#15
For D7 Field UI permissions works, and the tab from ds can be hidden using #1837156: Hide "manage display" tab when permissions are revoked
#16
Still not really a bug.
#17
@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.
#18
Ok content types aren't ok yet - let me check that, it seems #access doesn't work on links/operations.
#19
Better one. Do we need an upgrade path for this ?
#20
#19 is looking good, about the upgrade path, I don't think it's needed, we'll have to make to many assumptions.
#21
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 :)
#22
The last submitted patch, 611294-19.patch, failed testing.
#23
Progress.
#24
Actually, this one should be green.
#25
The last submitted patch, 611294-24.patch, failed testing.
#26
Green!
#27
Nice work, I cannot find anything wrong with this and it works!
#28
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);
}
#29
Oh yes, much nicer, less code, great!
#30
Yup :-)
Still RTBC if it comes back green.
#31
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
#32
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 :)
#33
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)
#34
Oh yes, makes much sense, new patch + interdiff + screenshot.
#35
Coolio, thanks! :)
#36
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'.
#37
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
#38
Rename field_ui to entity_ui totally gets my +1, but that's for post freeze and definitely not for this patch :-)
#39
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...
#40
So, back to rtbc, and refactor in february, we have all the time now right ? :)
- edit - crosspost
#41
Admin content types allows you to edit published, promoted, preview, ... as well. Makes sense to keep this separate.
#42
back to RTBC then.
#43
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.
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.
#44
Isn't it easier to fix the upgrade path, if the user had administer content types, he'll get the new permissions during upgrade?
#45
So, upgrade tests added.
#46
The last submitted patch, 611294-45.patch, failed testing.
#47
#48
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
#49
Fixed - old school interdiff :)
#50
The last submitted patch, 611294-49.patch, failed testing.
#51
#49: 611294-49.patch queued for re-testing.
#52
Chasing HEAD
#53
The last submitted patch, 611294-52.patch, failed testing.
#54
#55
The last submitted patch, 611294-54.patch, failed testing.
#56
+++ 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.
#57
#58
Didn't mean to remove the tags.
#59
#57 fixes #56 and looks good to me.
#60
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.
#61
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
#62
#57: 611294-57.patch queued for re-testing.
#63
The last submitted patch, 611294-57.patch, failed testing.
#64
Reroll
#65
Opened #1891686: Consolidate / standardize entity permissions. Committed/pushed this one to 8.x, thanks!
#66
We need a change notice here, this will break other tests in people's patches eg #1871772: Convert custom blocks to content entities
#67
Forgot tag
#68
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] might not be needed
#69
Change notice: http://drupal.org/node/1894418
#70
Change notice looks the goods
#71
Automatically closed -- issue fixed for 2 weeks with no activity.
#72
Followup ?
#1944344: field_ui_update_8001 deletes permissions
#73
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)
#74
Removing Tag.