Current status: Fixed

Drupal 7: This was fixed with the patch in #114 (sun), committed in #118.
Drupal 6: Use the Reduced node edit module

Original issue description

I've found this to be a problem on several of my sites. Users lose (or don't have) edit access to nodes that they should be able to edit, given their roles and associated access control settings.

The node.module node_access() function denies 'update' access to a user, if a node has an input filter assigned that the user cannot access.

function node_access($op, $node = NULL, $uid = NULL) {
  // Convert the node to an object if necessary:
  if ($op != 'create') {
    $node = (object)$node;
  }
  // If the node is in a restricted format, disallow editing.
  if ($op == 'update' && !filter_access($node->format)) {
    return FALSE;
  }

This can be a problem because a user can create a node, the administrator may alter input filter permissions so that the input filter used to create the node is no longer accessible to the user (or edit the node and assign a restricted input filter to the node) - and the original user can no longer edit a node (the 'edit' tab disappears from the node menu, and if the user tries to access the edit function via /node/x/edit URL, the users receives an access denied message.)

As far as I've seen, no warning is given in the watchdog so it's a complete mystery as to why a user should not be able to edit their own content.

I think at a minimum, node_access() should create a watchdog entry of some kind so that an admin will know why the update permission was blocked for a user attempting to edit content. I'm not sure what else can be done. (Might also be nice to let admins know that changing default input filters, or changing access permissions for input filters, can trigger loss of user edit capability in some circumstances.)

Is this a known issue? (Is there another issue that covers this?) I'd like to submit a node.module patch for consideration if it's not already being dealt with, and people think this is

I've written up a slightly more detailed note about it on this page.

Files: 
CommentFileSizeAuthor
#114 drupal.filter-format-access.114.patch9.03 KBsun
PASSED: [[SimpleTest]]: [MySQL] 19,038 pass(es). View
#102 drupal.filter-format-access.101.patch7.2 KBsun
PASSED: [[SimpleTest]]: [MySQL] 19,073 pass(es). View
#100 drupal.filter-format-access.100.patch7.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] 18,701 pass(es), 1 fail(s), and 0 exception(es). View
#99 drupal.filter-format-access.98.patch7.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] 18,702 pass(es), 1 fail(s), and 0 exception(es). View
#97 drupal.filter-format-access.96.patch6.18 KBsun
FAILED: [[SimpleTest]]: [MySQL] 18,694 pass(es), 1 fail(s), and 0 exception(es). View
#96 drupal.filter-format-access.96.patch8.39 KBsun
FAILED: [[SimpleTest]]: [MySQL] 18,702 pass(es), 1 fail(s), and 0 exception(es). View
#82 drupal.edit-tabs.patch6.6 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.edit-tabs.patch. View
#63 summary.png23.46 KBcatch
#60 drupal.allow_edit_4.patch8.76 KBBerdir
Unable to apply patch drupal.allow_edit_4.patch View
#58 drupal.allow_edit_3.patch1.99 KBBerdir
Failed: Failed to apply patch. View
#58 textarea.png43.05 KBBerdir
#56 Ugly.png42.83 KBBerdir
#50 drupal.allow_edit_2.patch7.9 KBBerdir
Failed: Failed to apply patch. View
#46 drupal.allow_edit_1.patch10.06 KBBerdir
Failed: Failed to apply patch. View
#48 EditTab.png44.04 KBBerdir
#38 node_filter_access.patch2.99 KBchx
Failed: Failed to apply patch. View
#37 webchick_will_love_this.patch2.94 KBchx
Failed: Failed to apply patch. View
#25 filter_access_no_form_alter.patch5.11 KBswentel
Failed: Failed to apply patch. View
#24 filter_access_no_form_alter.patch5.16 KBswentel
Failed: Failed to apply patch. View
#21 filter_access.patch1.92 KBblackdog
Passed: 10688 passes, 0 fails, 0 exceptions View
#16 drupal.filter-access.patch1.9 KBsun
Failed: Failed to install HEAD. View
#14 disappearing_tabs.patch1.4 KBswentel
Failed: 9661 passes, 0 fails, 1 exception View
#8 filter_watchdog.patch436 bytesblackdog
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filter_watchdog.patch. View

Comments

mcurry’s picture

Version: 4.7.3 » 4.7.4

Oops - hit submit too soon. Next to last paragraph should have ended in: ...and people think this is something that should be fixed.

Changing version to 4.7.4 - I've checked and it appears to exist in 4.7.4 CVS code.

mcurry’s picture

Category: feature » bug

Making this a bug report since I've not seen any comment on this. Is this a bug, or a "feature" as-designed?

mcurry’s picture

Assigned: mcurry » Unassigned

*bump* - is this as-designed behavior? Can anyone provide some insight into this behavior?

Gábor Hojtsy’s picture

Well, a verbose error message on the filter format based permission situations would be great. I don't see an easy way to solve the permission problem itself. Filter formats not allowed to be used by the user should be clearly forbidden.

mcurry’s picture

Version: 4.7.4 » 5.x-dev
Assigned: Unassigned » mcurry

(Moving to Drupal 5.x since the problem seems to remain in current Drupal distros)

In order to help the harried Drupal site administrator, all that's really needed is a watchdog entry and some kind of warning (drupal_set_message) informing them of the "unusual" condition - since it's keeping someone from editing what is supposed to be their content, it causes much confusion, and there is absolutely *no* feedback as to why - creating much anguish and hair-pulling. :D

I had to step through the Drupal core code to determine why I (and others using my site) couldn't edit content... and that, IMO, is just not a good way to deal with the problem.

I just received another comment on my website about this issue so people are searching for solutions to this problem, and landing on my web page.

Anyway, I can submit a patch that creates a watchdog entry and a warning to the user experiencing the problem so that the admin can deal with the problem...

banebanne’s picture

I had same problem. I could change blocks,menus but not content. This is what I did and it is working.

Post seting set Preview post to optional

JirkaRybka’s picture

As far as I can tell, the fact that filter formats are limiting edits is correct: I understand that as a security measure, especially if admin uses "php code" or "full HTML" formats to alter untrusted user's post, without realizing the consequences.

We really need the watchdog entry and error message! So, inactivist, please attach your patch here ;-) It may be a good idea to do so for 6.x first, but I guess this may be considered a bugfix, so 5.x might be OK too.

blackdog’s picture

Assigned: mcurry » blackdog
Status: Active » Needs review
FileSize
436 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filter_watchdog.patch. View

Attached a patch that adds a watchdog entry when this occurs. I'm not really sure where we should put out an error message, so I've left that out.

This issue has bugged be for two days now, not understanding why a user couldn't edit his own pages, so I'd really like this to get in both D5 and D6 (if it's still an issue in D6). I'll provide a patch for D6 when this one is done.

drumm’s picture

Version: 5.x-dev » 7.x-dev
Status: Needs review » Needs work

I would like to see this fixed in the current development version and then backported as necessary.

I do not think a watchdog message is the appropriate solution. Ideally, the user could access the edit page, but have the textarea replaced with a notice of why it can not be edited. The other fields would act as usual.

s.Daniel’s picture

Ideally, the user could access the edit page, but have the textarea replaced with a notice of why it can not be edited. The other fields would act as usual.

Great suggestion!
I ran into this bug before and it is just one of those things that can make you feel really stupid so some feedback for bugtracking would be a good step forward.

catch’s picture

Title: User cannot (or loses ability to) edit content that user has permission to edit, no reason given » Usability: Disappearing edit tabs due to input format permissions.
Priority: Normal » Critical

I think this should be marked as critical, reposting what I wrote on http://drupal.org/node/244390 - looks like me and drumm had the same idea with the disabled textarea, I think that'd be an ideal solution for this.

--

As stated on #243728, if a user has access to edit x content type, but doesn't have access to the filter format of that post, they lose the edit tab with no explanation. This has the effect of very confused support requests from site users (I've personally had many), and I've probably answered 4-5 #support requests in the past month or so from Drupal admins with the same issue.

The issue manifests itself in a few ways:
1. users have permission to edit any $node_type, but can only edit some
2. admin users/editors edit a node using a different input format, and the user loses access to edit their own node.

In both cases, there's no access denied or warning to inform the user that they no longer have access to edit, they just lose the edit tab.

Additionally, there's nothing to warn editors when changing an input format that the owner of the node doesn't have access to it.

So instead I think we've got a couple of options:
1. leave the edit tab but disable text areas which users don't have permission to edit - this would mean they could still change titles and stuff, and would have an explanation of what exactly the issue was.
2. add a small javascript warning to the input format selection which informs editors that they're choosing a restricted input format - either not available to the node author or not available to auth users or whatever.

Making input formats into a first class permission will help, but I think we can go a step further.

Here's the results of a quick google search for disappearing edit tabs:
http://drupal.org/node/126504
http://drupal.org/node/15283 <<- note the nid.

blackdog’s picture

Assigned: blackdog » Unassigned

I like what drumm and catch has proposed, but I don't have time to work on this right now, so unassigning myself.

catch’s picture

Title: Usability: Disappearing edit tabs due to input format permissions. » Disappearing edit tabs due to input format permissions.
Component: node.module » usability

Moving to usability. Going to look at this again soon.

swentel’s picture

FileSize
1.4 KB
Failed: 9661 passes, 0 fails, 1 exception View

1. leave the edit tab but disable text areas which users don't have permission to edit - this would mean they could still change titles and stuff, and would have an explanation of what exactly the issue was.

I did this once for a client implementing a simple form_alter. Attached is a patch which removes the access check in node.module and adds a filter_form_alter in the filter module. I'm not sure if this is the way to go, but it's the easiest way to backport if that's also the plan of this issue. Tested it on a clean drupal 7 and works as Drumm and Catch suggested . Not sure what happens though with cck textfields using a filter format (and if this patch should care of this?)

Any comments are appreciated, I like the idea and I'm willing to help out. Let me know if my approach is 'good' or if it should be solved otherwhise.

catch’s picture

Status: Needs work » Needs review

Would be nice if the patch handles CCK et al, but I don't think that's a showstopper to it getting in.

Marking this as needs review. I agree a form_alter is a nice way to handle this. Is it worth considering readonly + a #prefix though?

sun’s picture

FileSize
1.9 KB
Failed: Failed to install HEAD. View

Implementing this via filter_form_alter() sounds absolutely sane. Support for CCK should not be dealt with in this issue.

And... yay! This works like a charm!

- Moved _form_alter() a bit down in filter.module
- Updated code for recent changes in core coding standards

Only two remaining issues:

- Iterating over all children, but skipping recursive children, may be unnecessary (but could be a basis for upcoming CCK support). If I'm not wrong here, we might want to replace this code with simply checking for 'body_filter' (or whatever it's called in HEAD).
- I'm not 100% sure we can safely back-port this patch to D6 (and perhaps D5). That would be amazing, but might break modules in contrib that do not expect this to happen.

sun’s picture

Is it worth considering readonly + a #prefix though?

No, that would expose internal PHP code.

Last comment was actually a positive test result for this patch.

catch’s picture

@sun, good point. Although the PHP input format is now a module, so we could add module_exists('php') to set #access, otherwise make the textarea disabled - but maybe that's overkill, this is still a massive improvement.

I'd support a backport to D6 - it's really, really annoying when this bug crops up.

earnie’s picture

bookmark

greenSkin’s picture

bookmark

blackdog’s picture

FileSize
1.92 KB
Passed: 10688 passes, 0 fails, 0 exceptions View

This works like a charm! Tested with a page with Full HTML in the body, editing with a user without access to that filter disables the field.

I've just updated the patch a little - added a

wrapper around the message to make things clear it. Not sure if this (

-tags in the form) is the right way to do this though, so still needs review.

webchick’s picture

Status: Needs review » Needs work

This looks like a really nice improvement. Could we add some brief comments to the top of filter_form_alter to explain what the code there is doing, and add some tests to ensure that this stays working and isn't creeping in additional bugs? It's rather important.

swentel’s picture

I've been thinking about this patch the last few days and although I came up with the form_alter suggestion, I think we should consider another approach and alter the filter_form function. For instance, pass along the form by reference so we can set the element's access more easily to false than with a form_alter. I think this results in nicer, cleaner and faster code. Back-port is another issue, but I don't think the patch at this point is good to be backported anyways - and would cause tremendous bugs as sun also said in #16. I'm going to think a bit more about this week and try to suggest another patch.

swentel’s picture

FileSize
5.16 KB
Failed: Failed to apply patch. View

Complete other approach then filter_form_alter. Instead, the filter_form function is a bit altered. Let me know what you think.

swentel’s picture

FileSize
5.11 KB
Failed: Failed to apply patch. View

Small update in the patch.

Damien Tournoud’s picture

Category: bug » feature
Priority: Critical » Normal

Who qualified that as a bug? This behavior is well know (#1 on our FAQ list at drupalfr.org :p) and has been here for ages. Looks more like a feature request.

At that point the filtering facilities of D7 is about to change. Thanks to #125315: Textarea with input format attached and #304330: Text format widget we will have a full featured "textarea with input format" widget, and it will become much easier to implement the feature discussed in that issue.

Before postponing this, there is only one question remaining: does this feature request is important enough to be implemented in D6? I'll like to hear the word of core maintainers on that.

sun’s picture

Both patches are not related, only a few changes might be in conflict unfortunately. So there is no need to mark this issue postponed.

TBH, I liked the first approach more than the second, since it looked a lot cleaner. Requiring so many arguments for filter_form() will probably be a pain for developers. However, I would agree that permissions should be checked earlier in the process - which makes me think of a filter_elements() implementation and another #process callback, similar to #125315: Textarea with input format attached. But then again, while reviewing the latest patch, all of these access checks somehow remind me of CCK Field Permissions module...
So if we really want to treat this issue as a feature, we should research whether there might be analogies in CCK (Field Permissions) that could be translated into an API, allowing every module to decide whether a user has access to a(ny) field [not limited to input format enabled textareas] and take the appropriate action if she does not. However, if we revert this issue to a bug report, then I'd be in favor of the patch in #16, or a patch following this approach.

sun’s picture

And yes, #27 seems the right way to go - #152473: Make #access more flexible contains a patch that centralizes #access property checking, which could be enhanced to allow access callbacks to return alternative contents if access to a form element is prohibited.

catch’s picture

Category: feature » bug
Priority: Normal » Critical
Status: Needs work » Postponed

@Damz, I really think this is a critical bug - a quick search of d.o. finds examples of the full range of Drupal users being confused by this over the past 5 years. However, we should do it cleanly once #152473: Make #access more flexible gets in.

moshe weitzman’s picture

Status: Postponed » Active

One solution that has no dependencies is to let drupal_access_denied( take a $msg parameter and it prints that out by default. node_access would set a $msg that is tuned to format denied.

Gábor Hojtsy’s picture

Status: Active » Needs work

Since the #input_format FAPI key landed, the latest patch is quite outdated. Great progress on displaying the edit tab even though the body format is not allowed and only skipping the body field. There is lots of others to modify on a node even then. Let's get this fixed sooner then later in Drupal 7. Work from #25 onwards :)

cozzi’s picture

I just got bit by this situation. Luckily Google led me here within minutes of the first report.

I'm not sure of the real solution but whatever is chosen the test should be done at "save" not after the node is handed off. The person saving the node, who has changed the author, should be the one alerted to the problem. IMHO

Thanks for this post - it alone saved me a good deal of time.

KarenS’s picture

Component: usability » filter.module

I've now been bit by this one, it took a couple hours to realize the reason why a user couldn't edit a node he should have been able to edit was because another user had changed the filter to one he didn't have access to. This behavior is totally un-intuitive and quite easy to do -- most users have no idea when they change the filter that they might be making the node un-editable to someone else. And when you're debugging permission problems you're looking at the user permissions screen for clues, and the filter permissions are not there (which makes this even harder to find). I'm no newbie, but it was not at all obvious to me that this was triggered by a problem with filter permissions, so I agree this is critical.

I was also thinking about the idea above of warning the previous user on node_save(), since that's the easiest time for someone to fix it, but it would require a test to see if there are other users who have edit access to the node who don't have access to the new filter, which sounds unnecessarily complicated. And it might be OK at the time they save the node, but later changes to permissions might break it. So a message on the node itself that makes it easy to see why it can't be edited might be the best we can do.

I'm not sure which approach we prefer though. The original patch was the simplest. Turning this into a permissions API is a nice idea, but that's going to slow this down even more. Can't we simply fix the bug first, using an approach that should be fairly easily backported, then deal with better ways of handling field permissions as a separate patch?

And I'm not sure I see any problem with backporting it. I can't imagine any application that won't work if we allow a user who would have been allowed to edit the node except for filter permissions to edit everything but that field.

earnie’s picture

KarenS, what about an admin report giving the permissions allowed for the user? It would show as a task tab on the user form with a tab title of Permissions. At least an administrator would be able to debug the issue more easily. I haven't thought through it enough to have a plan of action but thought I would throw the light bulb idea in the open anyway.

earnie’s picture

Issue tags: +Usability, +admin user
kenorb’s picture

I had this problem many times as well on 6.x
And other one, probably related to this as well: #360421: user can't edit his own forum post

chx’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
Failed: Failed to apply patch. View
chx’s picture

FileSize
2.99 KB
Failed: Failed to apply patch. View

An even more secure version. Please add a test.

mfer’s picture

subscribing.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Issue tags: +DrupalWTF

While this patch is being re-rolled for HEAD and tests, I think we should also add a drupal_set_message() or similar to inform people why they're only seeing the markup version of the text. Otherwise we're just pushing the WTF one level deeper.

Robin Monks’s picture

Subscribing.

jarlbrendan’s picture

Subscribing.

David_Rothstein’s picture

Subscribe. This is definitely a clever approach!

I think block.module (and possibly comment.module?) might do something funny in this case too (in terms of denying access or hiding fields when the user does not have permission to edit the text format). Not sure if it should be part of this patch or a followup -- the node.module bug is obviously the worst one.

KarenS’s picture

This issue has come up in #372743: Body and teaser as fields, we'll want to accomplish the same goal there. But this patch can be back-ported and that one cannot, so if there is interest in back-porting this fix this patch will still be needed.

Berdir’s picture

Priority: Normal » Critical
FileSize
10.06 KB
Failed: Failed to apply patch. View

Ok, attached is a first patch, it is not perfect but should work.

A few comments:
- Just displaying markup didn't worked for me.
- Instead, I choose to display the unchangeable fields as disabled textareas.
- The current code sets the max height (rows) to 5 and disables the resizeable feature. I thought it doesn't make sense to have a huge, un-editable, field and atleast the resizeable stuff was quite buggy for me on a disabled field.
- The patch appends "(read only)" to the title and "You are not allowed to change this field." in bold to the description. I'm open for suggestions to improve that :)
- Added a test which verifies that everything works as it should.
- Teaser needs to be handled specially, The only way I could get it work is to completetly disable all teaser related things if the body field is disabled. I've done this in the already existing #after_build functions of node.module. This is ugly but I haven't found another way. Enabled teaser field allows a user to "break" out by sending a new teaser and it is *very* hard to handle.
- It *should* work for every field with an attached #text_format, but I've only tested it with Teaser/Body.

@KarenS
I assume that this should work for #372743: Body and teaser as fields too (except of the node.module specific stuff, of course), but I haven't looked at that patch there.

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

FileSize
44.04 KB

Attaching a screenshot for the limited edit screen...

sun’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

If a user does not have access to a used format, we must render the value filtered.

Reason: Consider the field is using PHP format. Unprivileged users would see the PHP source code. Not so good.

Why is this critical again? This bug we have since Drupal 4.x, no?

Berdir’s picture

Priority: Critical » Normal
Status: Needs work » Needs review
FileSize
7.9 KB
Failed: Failed to apply patch. View

Ok.

- Got it finally working with #markup. However, this does look *very* ugly with the field displayed without any indication what it actually is. I thing we should either leave it completetly out or make it obvious that it is the content of field X. But I'm uploading it to get some review and let the tests run..
- The current behavior has a big advantage: There are almost no differences to the normal form. Only access = FALSE on value and format is type value instead markup. This should make it pretty safe to backport.
- Updated tests

David_Rothstein’s picture

The patch looks very good in general, but I'm noticing an issue with this whole approach that didn't occur to me before. The previous code checked filter_access($node->format), i.e. the actual format stored with the object (if there is one). The new code checks filter_access($element['#text_format']), which is a very different thing -- it's simply the initial format that happens to be preselected on the edit form. While this distinction does not matter for Drupal core itself, there are situations where it does matter, and this could result in very serious bugs (editing being allowed when it should be denied, or vice versa).

Is there any simple way for this function to have access to the object's format itself, so it can check access on that? I can't immediately think of one, unfortunately.

Berdir’s picture

Can you give an example what could go wrong, as I can't follow you. The #text_format is set with:
'#text_format' => isset($node->format) ? $node->format : FILTER_FORMAT_DEFAULT,

It is the value of $node->format, unless someone changes it with hook_form_alter or so. And he could also change $node->format there, I can't see a difference here.

Berdir’s picture

Another point, with CCK, there can be multiple form fields with a (different) text format, so you can't rely on $node->format.

catch’s picture

Can we have a screenshot of the ugly?

@sun:

If a user does not have access to a used format, we must render the value filtered.

Reason: Consider the field is using PHP format. Unprivileged users would see the PHP source code. Not so good.

I agree we need this, but it's a shame - the disabled textarea is the nicest option visually IMO.

If the rendered value looks bad, then we could we maybe replace with a note to say "you do not have access to edit $field in $format"? That'd fix the wtf element and it might be a bit abrupt but possibly less so than just displaying the rendered content of the field on the edit screen.

Originally marked this as critical because there's a bunch of support requests about it all over the place (issue queues, forums, Drupal.org webmasters when project maintainers can't edit their own project nodes because someone added a screenshot for them) - and it's nearly impossible to work out what's going on unless you're already familiar with the problem - but in the scheme of Drupal usability, maybe not.

However, as far as I know there's no mechanism for dealing with this in Fields in core at all - so sun's concerns about exposing the PHP format are probably a big hole in HEAD (unless someone can correct me), back to critical for that :)

catch’s picture

Priority: Normal » Critical

hum.

Berdir’s picture

FileSize
42.83 KB

Image of "teh ugly" :)

Something is still wrong, the comment should not be displayed.

yoroy’s picture

Had a quick chat with catch about this in IRC:

So I'd think, if showing the actual content is a concern (for some formats), then why try to show it on [edit] if you can't edit it anyway.
A generic message "Sorry, no can do for you here" is more helpful I think.

Disabled textfields don't communicate their non-editableness very well by itself. I found myself hammering on disabled textfields in cck lately. "Ooh wait, have to click the unlock button first". You can add labels and descriptions around it but the eyes will be focussed on the actual content, so let's just put the message right there.

Berdir’s picture

Issue tags: -Needs screenshots
FileSize
43.05 KB
1.99 KB
Failed: Failed to apply patch. View

Discussed this with yoroy and catch in IRC, and this is the solution we came up with:

This is a mix of patch 1 and 2. It re-introduces the disabled textfield but only with a text that editing is not allowed, the actual value is never displayed (in the form, it is on preview and so on).

Someone who knows the FAPI system really well should have a look at this, It should be safe, but I'm not 100% sure. Especially as FAPI does _not_ deny the editing of disabled textfields/areas (only for checkboxes). But #value is hardcoded to #default_value so there should be no way to change this.

Attaching patch and Screenshot

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.76 KB
Unable to apply patch drupal.allow_edit_4.patch View

Re-rolled to include changes in #304330: Text format widget

sun’s picture

Status: Needs review » Needs work

You seem to ignore my reviews.

We don't want to introduce weird complexity and specifically test whether PHP filter is enabled for the text format. Because, if it is, we cannot display the field's value. Period.

So, sometimes you would see a disabled field, sometimes not. I must really wonder what inconsistency has in common with usability.

Additionally, who said that PHP filter is the only filter that can expose information that no one without proper permissions should ever see?

To get this done, start over with chx's patch in #38 and add a test.

Berdir’s picture

@sun

Have you looked at textarea.png? The content is not displayed with the latest patches. Never. Independetly of the chosen filter.

The idea yoroy, catch and I had is that the textfield is always displayed, so that it doesn't look completely different at the first glance. However, if the user does not have "write access" to that field, the textarea is disabled with a fixed text telling the user that he is not allowed to edit it.

chx's approach removed the field without any information. As webchick mentioned in #41, this just moves the WTF one level deeper (Why is there no body ?!)

However, what *needs* special handling is teaser stuff. That needs to be disabled because it does not make sense to be enabled and would break the "blocking" (it would be possible to edit the teaser field after you click on split teaser button, but that would use the same filter, which you are not allowed to). This special handling is done in node.module and will probably go away when the body is converted to a field.

I also decided to limit the number of rows and disable #resizable, but that is optional, I did it because I think it does not make sense to have a fieldarea which takes up the whole screen but you can't do anything with it.

catch’s picture

FileSize
23.46 KB

Sun, please take another look. With Berdir's new approach we get both visually consistent disabled textareas and no exposure of content.

Here's what it looks like:

screenshot

However latest patch appears to have some bugs in it - no disabled textarea and half the teaser splitter is still there.

sun’s picture

Please tell me: Why do we need a completely custom textarea with completely custom styling just to display a message that says "This field has been disabled because you do not have sufficient permissions to edit it." ?

This patch implements various hacks just to achieve a custom, disabled textarea. A disabled textarea with custom styling the user can't even use in the end.

I fail to see a usability improvement in that.

Berdir’s picture

Oh, yes it is possible that I broke it with the last patch, I haven't actually tested it. Will do it soon.

yoroy’s picture

We show a 'fake' text area to avoid the WTF moment and meet the user's expectation of seeing a textarea where the body text is. Avoiding the WTF moment == keeping the user's mental model intact == user experience win.

catch’s picture

"I fail to see a usability improvement" isn't an argument, it's just ranting.

The whole point of this issue is that the user can't use the textarea. It's how we convey this information that's important. Showing this information in the same context as the user would normally be able to edit the field makes perfect sense to me.

Currently, we just disable the edit tab with zero explanation. Have you seen the webmasters issues where module maintainers 'lose access' to their project nodes because another maintainer with higher permissions added a chipin. This bug shows up all over place - on sites I run, on Drupal.org itself with 'expert' users, and it's on forum topics going back to at least 2004.

Other solutions that have been proposed and rejected in this issue:

# Just hide the field - as mentioned this just defers the wtf down a level.

# Show the rendered content - Really ugly and could make the rest of the form unmanageable (imagine 40,000 word dissertations in full HTML format, views_get_view() or hook_page_alter()).

# Just show a message - That makes it unrecognisable as the field the user will be used to seeing.

The implementation is a bit elaborate, but no more a hack than using menu access for this, and it very clearly states what's happening by showing the disabled field in the same position, and the same styling as it would appear normally. It also takes the special casing out of node module and allows this to work for all textfields - something we're going to need in many other places with fields in core.

Any other ideas?

sun’s picture

Status: Needs work » Postponed

Good. It seems I can't convince.

To prevent scope creep, please create a separate issue that properly implements handling of disabled textareas. Copy:

1) Disabled textareas should neither have a teaser splitter, nor a "show summary in full view" checkbox.

2) Disabled textareas should automatically move #default_value into #value.

3) Disabled textareas may or may not be resizable. Derail over there.

Afterwards, we can continue here.

earnie’s picture

2) Disabled textareas should automatically move #default_value into #value.

#410926: Using FAPI #disabled on some element types causes values to be ignored may be a good issue to start the work.

Berdir’s picture

Status: Postponed » Needs work

I agree that the #disabled handling should not be part of this patch, but that will make it imposs.... hard to backport, which was one of the goals so far and should be possible with the current solution.

A short personal feedback to your points, I will post it here for now, I'm not sure if it makes sense to create such an issue right now, before we have a text_format #type. Or maybe split it even more up, see below.

1) Except of the theme function, handling of teaser lies in completely in node.module. Especially the body field knows _nothing_ about the teaser stuff, instead the teaser field knows about the body. This will probably (I assume that) change with #372743. With the previous patch, we may not need to handle that specially (hopefully). However, we have now a more or less cyclic dependendy between this, body as field and text_format

2) The "correct" way would imho be to ignore values for a disabled fields in FAPI, which currently doesn't happen. This is maybe not even related to text_format and could be handled in its own issue. I tried to look into that, but I was not able to find out were we would have to change that.

Another point is that "#disabled handling" (which currently only adds a disabled attribute, nothing more) happens before the process functions are called. We need to change the order if we want to make it possible to just set #disabled = TRUE inside such a process function. (Either way would be possible or set #disabled somewhere else).

Berdir’s picture

Status: Needs work » Postponed

Didn't want to change the status

Berdir’s picture

Status: Postponed » Needs work

Setting this back to needs work, because..

To prevent scope creep, please create a separate issue that properly implements handling of disabled textareas. Copy:
1) Disabled textareas should neither have a teaser splitter, nor a "show summary in full view" checkbox.

2) Disabled textareas should automatically move #default_value into #value.

3) Disabled textareas may or may not be resizable. Derail over there.

1) As I already said, teaser handling lies mostly in node.module and will hopefully be solved in a much cleaner way with #372743: Body and teaser as fields.

2) Not all disabled fields stay disabled, many of them can be enabled with JS (for example, teaser) and there is no way FAPI can know about that. Because of that, there can't be a generic handling of #disabled, this is specific. If at all, the only possible thing would be an additional field #read_only => TRUE or something like that.

3) Yes, this needs to be discussed. But again, this is specific and can't be handled in a generic way imho.

David_Rothstein’s picture

Regarding point (2), there's already an issue in the queue for enforcing that #disabled actually means disabled... and it seems like it should be workable even in the case of Javascript. See #426056: Server-side enforcement of #disabled is inconsistent.

Also, I realized that I owed an answer to your question in #52, oops :) The short answer is that yes, hook_form_alter() is what I was worried about.... someone can use that to change #text_format, and when doing so they don't necessarily have any knowledge of what the actual text format of the "object" (a.k.a., node, field, whatever) was, so big problems can ensue. However, this is really a separate issue, I guess, and it can be dealt with at #414424: Introduce Form API #type 'text_format'. It's worth keeping that one in mind, though.

Berdir’s picture

#426056 looks interesting, however, this is "special". Because it does display a different value than it actually uses internally. It could still work if we set both elements to #disabled.

Thanks for the clarification, I can see that this could generate problems. However, incorrect usage of hook_form_alter() can break other stuff too, I'm not sure if and how we should handle that. Especially with fields/CCK, a node can have different formats, #text_format doesn't necessarly match $node->format.

cburschka’s picture

Edit: Didn't read earlier posts, never mind.

Berdir’s picture

Right now, in HEAD, textareas with a input filter that is not accessible for a user are just reset to a filter that can be used by the user that is editing the node. We need to fix this.

#426056: Server-side enforcement of #disabled is inconsistent now contains a working (as in, the tests) patch, but it needs to be reviewed and discussed as it *does* change how #disabled works. Once that patch is commited, we can do this in a clean, generic way for all textareas with a input filter chooser.

mcurry’s picture

I really appreciate all the work that has gone into discussing solutions to the problem.

After three years and several patch submissions, the problem remains unresolved. People still lose ability to edit their nodes, and many Drupal admins have no clue why. The issue impacts numerous module maintainers who receive support requests or bug reports that they have to deal with, costing the community time and lost productivity.

Would it be a good idea to return this issue to a proposal of just creating a watchdog entry when the condition occurs? After all, the administrator created the error condition when he or she removed access to the input filter(s), so it's probably "good enough" to just let the admin know that there is a problem and let them resolve it.

In this case, it seems that we're allowing the perfect to be the enemy of the good. Or has a fix of any kind been made to Drupal core, and I've missed it?

Berdir’s picture

As I said above, this is currently a security bug in Drupal 7 which simply means that it *needs* to be fixed before D7 is released in one way or another.

What I'm wondering is if the linked issue regarding #disabled fields can be fixed after code-freeze, probably not, as it is not a api change but the functionality changes.

We still *can* fix this in the same way without that issue, but it will result in a bit uglier code, but its just +/- 5 loc

BenK’s picture

Subscribing....

crea’s picture

subscribing

Cliff’s picture

Subscribing.

sun’s picture

Issue tags: -admin user +FilterSystemRevamp, +API clean-up
FileSize
6.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.edit-tabs.patch. View

Tagging.

Postponing on #414424: Introduce Form API #type 'text_format'.

Re-rolling something in between.

sun’s picture

Status: Needs work » Postponed
Berdir’s picture

Issue tags: +Release blocker

As described already in #91663-78: Permission of text format is not checked when editing an entity and instead reset to something a user can use., this is a release blocker as it is now a security bug. Adding the proper tag as discussed with sun in #drupal.

sun’s picture

Status: Postponed » Active

Shockingly, this is a release blocker now, so no longer postponed. We can continue here after #414424: Introduce Form API #type 'text_format' landed.

donquixote’s picture

Subscribing, I need this!
Is there any solution that can be applied as a custom module?
The hook_form_alter stuff sounds promising, but that is now some time ago..

drumm (#9)

Ideally, the user could access the edit page, but have the textarea replaced with a notice of why it can not be edited. The other fields would act as usual.

+1, currently this is exactly what I need, with some fields filled with PHP.
I can imagine that someone wants to have choice if the entire node should be blocked, or just the field - but, blocking the entire node should rather be done with a dedicated access module, not as a side effect of input filters.

catch (#11)

2. add a small javascript warning to the input format selection which informs editors that they're choosing a restricted input format - either not available to the node author or not available to auth users or whatever.

I would prefer to have that warning displayed in the list of content types - maybe a font color indicating which of them are allowed for the owner of the post, and which are not.
Then, when you have chosen an input filter and closed that rollup fieldset, a short warning text could be displayed along with the field.
I'm not such a friend of javascript alerts and other disruptive modal dialogs.

sun et al (#64 ff)

I fail to see a usability improvement in that.

You should also think of users who are told via phone or email to go to that edit page they have never seen before. This user really needs a strong clue that he is on the right track, and there is supposed to be a textarea.
A greyed-out textarea with an explanation does the trick, anything less than that will rather make the user feel that he's looking in the wrong place.

catch’s picture

Status: Active » Needs work

There's a patch here.

sun’s picture

#414424: Introduce Form API #type 'text_format' should make this possible, as we are then dealing with widgets/form elements in a central processor.

frankcarey’s picture

I have a related issue, that I don't see anywhere else, but I think it might apply here as well. If someone thinks It should be it's own issue, no problem.

I actually had the (mis)fortune to know about this bug before, but when i went into my content_type settings none of the cck fields were set to something the user couldn't edit, which is when this became very frustrating. The answer was that during development, we had created some body field content with a non-accessible filter, and then had later disabled the body on the content (removing the body label in D6). Even though the label was gone, and no body appeared on the edit form for the node, this bug still showed up resulting in a missing edit tab.

I'm not sure if this is an issue in D7 with the new body as fields api.

sun.core’s picture

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

#414424: Introduce Form API #type 'text_format' still needs to go in. When that happens, this change is quite simple. However, since the other issue isn't RTBC yet, it's unlikely that this one will make it.

Berdir’s picture

Title: Disappearing edit tabs due to input format permissions. » Permission of text format is not checked when editing an entity and instead reset to something a user can use.
Version: 8.x-dev » 7.x-dev

@sun
Sorry for being stubborn, but we discussed this already, see Comment #84 and #78, this is currently an open security issue as the text format is simply reset to something the user can use when he does edit an entity. there is no permission check involved anymore. Changing the title...

We might have to live without a nice solution, but we need *something*.

sun’s picture

I've tried to come up with a simple/ugly solution, but it simply doesn't work. We need to get #414424: Introduce Form API #type 'text_format' in.

Berdir’s picture

One more thing to consider: There are cases where we might not want a default behavior (hide that field) but something different. Example: #520760: SA-CORE-2009-007 user signature format, where we do want (It seems, original patch wasn't written by me ;)) to set a text format a user is allowed to use when he does hot have access to the provided one.

sun’s picture

No, that case must be handled elsewhere; at the API level. The usual case is mostly covered, Filter module exposes hooks to allow modules to react on text format changes. In other words: This particular security update needs to assign a text format the user is able to access.

Any automatism is strictly prohibited. See also #562694: Text formats should throw an error and refuse to process if a plugin goes missing

sun’s picture

Status: Needs work » Needs review
FileSize
8.39 KB
FAILED: [[SimpleTest]]: [MySQL] 18,702 pass(es), 1 fail(s), and 0 exception(es). View

Alrighty. Almost 4 years later, this bug can finally be fixed.

Not sure whether this solution is acceptable, but yes, it works reliably. Patch also contains some minor JS behavior/markup fixes that should probably rather go into #414424: Introduce Form API #type 'text_format' as quick follow-up fix.

sun’s picture

FileSize
6.18 KB
FAILED: [[SimpleTest]]: [MySQL] 18,694 pass(es), 1 fail(s), and 0 exception(es). View

Merged those minor fixes into follow-up patch in #414424-131: Introduce Form API #type 'text_format'

catch’s picture

+    // Prepend #pre_render callback to replace field value with user notice
+    // prior rendering.
+    if (!isset($element['value']['#pre_render'])) {
+      $element['value']['#pre_render'] = array();
+    }
+    array_unshift($element['value']['#pre_render'], 'filter_form_access_denied');
+
+    // Cosmetical adjustments.

Comments should be:
... prior to rendering.

.. Cosmetic adjustments.

In terms of desired behaviour this looks spot on, much better than missing edit tabs or magic format changes. Code itself looks fine to me as well, didn't apply and have a look or anything.

sun’s picture

FileSize
7.19 KB
FAILED: [[SimpleTest]]: [MySQL] 18,702 pass(es), 1 fail(s), and 0 exception(es). View

This one will pass.

sun’s picture

FileSize
7.19 KB
FAILED: [[SimpleTest]]: [MySQL] 18,701 pass(es), 1 fail(s), and 0 exception(es). View

Incorporated catch's feedback.

Status: Needs review » Needs work

The last submitted patch, drupal.filter-format-access.100.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.2 KB
PASSED: [[SimpleTest]]: [MySQL] 19,073 pass(es). View

/me should read his own patches better. :P

sun’s picture

The #pre_render approach of this patch needs approval of moshe, chx, and effulgentsia.

The idea is simple: During entire form processing, validation, and submission, the original form elements and their values are kept. Only if, and only if this form will be rendered, and the current user does not have access to the stored and preselected text format, then we are replacing the field value prior to rendering in a #pre_render callback.

In other words: As long as the form is not presented to the user, it contains the real values. If it happens to be presented, the #access properties take effect and the field value is replaced with the user notice.

I'm happy with this solution, but the opinion of aforementioned people may differ. If so, please state so. Also not sure whether I explained the above sufficiently well in code.

Note that when manually testing this patch, the disabled and non-resizable textareas will look wonky. We're already solving this over in #735628: Resizable textarea behavior leads to unpredictable results

moshe weitzman’s picture

+++ modules/filter/filter.module	8 Mar 2010 05:10:38 -0000
@@ -885,6 +915,22 @@ function filter_form_after_build($elemen
+function filter_form_access_denied($element) {
+  $element['#value'] = t('This field has been disabled because you do not have sufficient permissions to edit it.');
+  return $element;

Do we really need this? It is harmless to show the real value in the textarea, isn't it? Thats what we do for admins. It would still be #disabled so uneditable. This would simplify the code a little.

Otherwise, looks good to me.

Powered by Dreditor.

catch’s picture

@moshe - sun doesn't want people being able to see php code they wouldn't have been able to in D6, seems sensible to me until we nuke PHP module, or at least the PHP filter.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

makes sense. the approach looks sound to me. i would rtbc this, but now it hasn't been tested in 3 weeks. why does bot not test pending patches anymore? not enough capacity?

Berdir’s picture

sun’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for reviewing, @moshe! However, last time I pinged @chx about this patch, he replied that he needs to spare some time to validate this approach.

He additionally requested that @Heine also needs to approve it.

As this is a quite creative solution, and at this stage of the cycle, I agree that we want to be 100% sure about this solution.

moshe weitzman’s picture

Well, perhaps we are overestimating the security aspect of this issue. From a conceptual point of view (i.e. D8), i don't think it is a sound strategy to rely on text format to protect the contents of your field. You would use field permissions for that. I am fine with the approach taken here, but I would also be fine without the whole #pre_render bit. My point is that this doesn't merit double secret signoff IMO.

sun’s picture

If, by that time, we managed to remove any other #type 'text_format' (and therefore also any other check_markup() call) throughout core, so that every filtered text indeed originates from a text field, then I'd agree that field permissions would be the proper way. I like that vision; and we're actually not too far away from it. Definitely worth to put on the roadmap for D8.

sun’s picture

Assigned: Unassigned » sun
Status: Needs review » Needs work

Just stumbled over my own comment in #562694-62: Text formats should throw an error and refuse to process if a plugin goes missing and realized that a vanishing text format will likely trigger the no-access condition of this patch. Not 100% sure, but has to be ruled out in the test.

Heine’s picture

My initial reaction to the issue when prompted by chx, was mislead due to the issue title; I thought it was a very bad idea to change the format to something the user could use. The current approach is the best we can do in D7 imo, (and a clever (and working!) hack).

I do agree with Moshe that we shouldn't use input formats as access control. For now, we need to, to prevent users from clobbering markup on minor edits, but we should think of something better (a button that unlocks the field?) for D8.

donquixote’s picture

and a permission to unlock such fields. Or let contrib do this.

sun’s picture

Status: Needs work » Needs review
FileSize
9.03 KB
PASSED: [[SimpleTest]]: [MySQL] 19,038 pass(es). View

Resolved #111. Also moved this new test into filter.test.

Probably ready to fly.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am good with this approach, yes.

sun’s picture

yay! Thanks for your final reviews, moshe, Heine, and chx!

It's great to see yet another bug languishing around since Drupal 4.7.3 to be finally fixed for Drupal 7. :)

A summary of the solution in this patch can be found in #103.

sun’s picture

Critical bump.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

OK. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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

verta’s picture

Status: Closed (fixed) » Needs review

Can this be backported to D6? It's really a hair-pulling problem when the user you've granted permission on a content type loses edit links because you changed permission on a filter after the content was created. Even assigning the content TO THEM does not fix this in D6.19.

I applaud the solution, thanks to all who contributed.

rschwab’s picture

Version: 7.x-dev » 6.x-dev
Assigned: sun » Unassigned
Priority: Critical » Normal
Status: Needs review » Needs work

This bug is epic. Updating statuses.

SilviaT’s picture

sub

AndyF’s picture

subscribe

Hitby’s picture

subscribe for the D6 backport

AndyF’s picture

@Hitby FWIW I've written a simple D6 module that keeps the edit tab in place and hides the body field if a user has content type permission but not input format. I've found it useful, YMMV.

donquixote’s picture

Is there any related issue for CCK ?

Bojhan’s picture

I have no idea what to review here..

rcross’s picture

Status: Needs work » Closed (fixed)

I think this issue can be closed.

It has been addressed for D7 and there is a module available (http://drupal.org/project/rnedit) that attempts a work-around for the problem.

Unless someone provides a patch to backport this, which seems unlikely at this point.

David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev
Priority: Normal » Critical

Moving back to the last version in which this was fixed.

donquixote’s picture

Issue summary: View changes

Describe current status