Disappearing edit tabs due to input format permissions.

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.

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

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

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.

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

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

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.

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.

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.

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.

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.

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

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

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.

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?

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.

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.

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

bookmark

bookmark

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.

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.

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.

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

Small update in the patch.

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.

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.

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.

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

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.

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

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.

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.

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.

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

An even more secure version. Please add a test.

subscribing.

The last submitted patch failed testing.

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.

Subscribing.

Subscribing.

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.

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.

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.

Attaching a screenshot for the limited edit screen...

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?

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

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.

Can you give an example what could go wrong, as I can't follow you. The #text_format is set with:

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.

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

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

hum.

Image of "teh ugly" :)

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

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.

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

The last submitted patch failed testing.

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

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.

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

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:

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

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.

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

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.

"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?

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.

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.

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

Didn't want to change the status

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.

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: No form validation error for manipulated values of #disabled fields.

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.

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

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

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: No form validation error for manipulated values of #disabled fields 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.

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?

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

Subscribing....

subscribing

Subscribing.

Tagging.

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

Re-rolling something in between.

As described already in #91663-78: Disappearing edit tabs due to input format permissions., this is a release blocker as it is now a security bug. Adding the proper tag as discussed with sun in #drupal.

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