Permission of text format is not checked when editing an entity and instead reset to something a user can use.

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.

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

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.

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.

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

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

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.

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

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

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

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

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.

There's a patch here.

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

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.

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

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

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.

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.

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

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.

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

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

This one will pass.

Incorporated catch's feedback.

/me should read his own patches better. :P

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

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

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

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?

#102: drupal.filter-format-access.101.patch queued for re-testing.

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.

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.

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.

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.

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.

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

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

Probably ready to fly.

I am good with this approach, yes.

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.

Critical bump.

OK. Committed to CVS HEAD.

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.

This bug is epic. Updating statuses.

sub

subscribe

subscribe for the D6 backport

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

Is there any related issue for CCK ?

I have no idea what to review here..

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.

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

Describe current status