#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.
Comment | File | Size | Author |
---|---|---|---|
#108 | administer_fields_permission.png | 29.03 KB | David_Rothstein |
#103 | interdiff.txt | 1.78 KB | David_Rothstein |
#103 | 611294-103.patch | 13.03 KB | David_Rothstein |
#100 | fe-perms.png | 58.75 KB | jenlampton |
#92 | 611294.patch | 12.58 KB | klausi |
Comments
Comment #1
yched CreditAttribution: yched commentedFunny, I was thinking exactly the same thing while washing the dishes 10 minutes ago...
Comment #2
jim0203 CreditAttribution: jim0203 commentedsubscribe
Comment #3
sunThis should be a fairly easy task, no?
Comment #4
sun.core CreditAttribution: sun.core commentedComment #5
catchDowngrading all D8 criticals to major per http://drupal.org/node/45111
Comment #6
marcingy CreditAttribution: marcingy commentedThis is really a task not a bug.
Comment #7
xjmI think the case can be made that at this point in the release cycle, this is not even a task, but a feature request.
Comment #8
tsphethean CreditAttribution: tsphethean commentedComment #9
tsphethean CreditAttribution: tsphethean commentedOk, 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.
Comment #11
tsphethean CreditAttribution: tsphethean commentedI 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.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedJust wanted to mention that for Drupal 7, I have a contrib module posted (Field UI permissions) which does something similar to this.
Comment #13
swentel CreditAttribution: swentel commentedAssigning to myself, will work on this during the weekend.
Comment #14
attiks CreditAttribution: attiks commentedI 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.
Comment #15
attiks CreditAttribution: attiks commentedFor D7 Field UI permissions works, and the tab from ds can be hidden using #1837156: Hide "manage display" tab when permissions are revoked
Comment #16
xjmStill not really a bug.
Comment #17
swentel CreditAttribution: swentel commented@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.
Comment #18
swentel CreditAttribution: swentel commentedOk content types aren't ok yet - let me check that, it seems #access doesn't work on links/operations.
Comment #19
swentel CreditAttribution: swentel commentedBetter one. Do we need an upgrade path for this ?
Comment #20
attiks CreditAttribution: attiks commented#19 is looking good, about the upgrade path, I don't think it's needed, we'll have to make to many assumptions.
Comment #21
swentel CreditAttribution: swentel commentedWell, 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 :)
Comment #23
swentel CreditAttribution: swentel commentedProgress.
Comment #24
swentel CreditAttribution: swentel commentedActually, this one should be green.
Comment #26
swentel CreditAttribution: swentel commentedGreen!
Comment #27
attiks CreditAttribution: attiks commentedNice work, I cannot find anything wrong with this and it works!
Comment #28
yched CreditAttribution: yched commentedThis 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 :
and in _field_ui_view_mode_menu_access() :
Comment #29
swentel CreditAttribution: swentel commentedOh yes, much nicer, less code, great!
Comment #30
yched CreditAttribution: yched commentedYup :-)
Still RTBC if it comes back green.
Comment #31
sunOh finally! AWESOME! :)
I only have a minor detail, which can be quickly addressed before commit, or as a quick follow-up patch:
These need to use proper t() placeholders — e.g., %entity-label
Comment #32
swentel CreditAttribution: swentel commentedDone. 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 :)
Comment #33
sunActually, 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)
Comment #34
swentel CreditAttribution: swentel commentedOh yes, makes much sense, new patch + interdiff + screenshot.
Comment #35
sunCoolio, thanks! :)
Comment #36
catchI'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'.
Comment #37
swentel CreditAttribution: swentel commentedThose 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
Comment #38
yched CreditAttribution: yched commentedRename field_ui to entity_ui totally gets my +1, but that's for post freeze and definitely not for this patch :-)
Comment #39
yched CreditAttribution: yched commentedThe 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...
Comment #40
swentel CreditAttribution: swentel commentedSo, back to rtbc, and refactor in february, we have all the time now right ? :)
- edit - crosspost
Comment #41
attiks CreditAttribution: attiks commentedAdmin content types allows you to edit published, promoted, preview, ... as well. Makes sense to keep this separate.
Comment #42
yched CreditAttribution: yched commentedback to RTBC then.
Comment #43
catchSo 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.
Comment #44
attiks CreditAttribution: attiks commentedIsn't it easier to fix the upgrade path, if the user had administer content types, he'll get the new permissions during upgrade?
Comment #45
swentel CreditAttribution: swentel commentedSo, upgrade tests added.
Comment #47
swentel CreditAttribution: swentel commentedComment #48
attiks CreditAttribution: attiks commentedSmall typo
garantuee --> guarantee
Comment #49
swentel CreditAttribution: swentel commentedFixed - old school interdiff :)
Comment #51
attiks CreditAttribution: attiks commented#49: 611294-49.patch queued for re-testing.
Comment #52
swentel CreditAttribution: swentel commentedChasing HEAD
Comment #54
swentel CreditAttribution: swentel commentedComment #56
sunHm. 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.
Comment #57
swentel CreditAttribution: swentel commentedComment #58
swentel CreditAttribution: swentel commentedDidn't mean to remove the tags.
Comment #59
attiks CreditAttribution: attiks commented#57 fixes #56 and looks good to me.
Comment #60
catchIs 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.
Comment #61
swentel CreditAttribution: swentel commentedAre 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
Comment #62
swentel CreditAttribution: swentel commented#57: 611294-57.patch queued for re-testing.
Comment #64
swentel CreditAttribution: swentel commentedReroll
Comment #65
catchOpened #1891686: Consolidate / standardize entity permissions. Committed/pushed this one to 8.x, thanks!
Comment #66
larowlanWe need a change notice here, this will break other tests in people's patches eg #1871772: Convert custom blocks to content entities
Comment #67
larowlanForgot tag
Comment #68
larowlanNote 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
Comment #69
swentel CreditAttribution: swentel commentedChange notice: http://drupal.org/node/1894418
Comment #70
larowlanChange notice looks the goods
Comment #72
yched CreditAttribution: yched commentedFollowup ?
#1944344: field_ui_update_8001 deletes permissions
Comment #73
yched CreditAttribution: yched commentedRight, 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)
Comment #74
jibranRemoving Tag.
Comment #75
David_Rothstein CreditAttribution: David_Rothstein commentedLet'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:
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:
Comment #76
David_Rothstein CreditAttribution: David_Rothstein commentedComment #77
David_Rothstein CreditAttribution: David_Rothstein commentedNot going to assume swentel will necessarily swoop in to do the backport, 2 years after this was fixed in Drupal 8 :)
Comment #78
klausiComment #79
klausiklausi opened a new pull request for this issue.
Comment #81
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #82
benjy CreditAttribution: benjy at CodeDrop commentedThis 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.
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:
NW because the patch no longer applies, I tested against f1e15c1
Comment #83
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #84
klausiRerolled 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.
Comment #87
benjy CreditAttribution: benjy at CodeDrop commentedLooks good to me.
Comment #88
klausiCool, can you set this to RTBC?
Comment #89
benjy CreditAttribution: benjy at CodeDrop commentedYeah, happy to set this to RTBC, I thought maybe someone else should review since it's quite a significant change.
Comment #90
dcam CreditAttribution: dcam commentedI'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.
Comment #91
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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:
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.
Comment #92
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #93
klausi* 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.
Comment #94
swentel CreditAttribution: swentel commentedRegarding 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.
Comment #95
klausiI 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.
Comment #96
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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.)
Comment #97
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI'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.
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?
Comment #98
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOne 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.
Comment #99
jenlamptonI 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.
Comment #100
jenlamptonMeant to add this screenshot.
Comment #101
Dave ReidThe 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.
Comment #102
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedLooks 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.
Comment #103
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere'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:
Feedback on the pros/cons of going ahead with this change in Drupal 7 is welcome here.
Comment #104
klausiPatch 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.
Comment #105
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #106
Bojhan CreditAttribution: Bojhan as a volunteer commentedLooks like the usability issue doesn't exist anymore?
Comment #107
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAn 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: [policy] 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.
Comment #108
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@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:
Comment #109
Bojhan CreditAttribution: Bojhan as a volunteer commentedCould you explain what this function does? Its not really clear from reading the screenshot.
Comment #110
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt provides access to the "Manage fields" and "Manage display" pages for entities that the user can otherwise administer. In other words:
1. Without this permission you can't access "Manage fields" or "Manage display" at all.
2. With this permission plus the "administer content types" permission, you can access "Manage fields" and "Manage display" for fields attached to content types.
3. etc.
Comment #111
hass CreditAttribution: hass commentedCan we get this committed, please?
Comment #112
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
I also did the following:
The release that includes this will probably be labeled Drupal 7.50 to indicate it contains some big changes (including this issue).
Comment #114
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented